net_device_ops support in bridging and fec_mpc52xx.c
Grant Likely
grant.likely at secretlab.ca
Sun Mar 22 09:00:34 EST 2009
Henk,
At the very least, I still need a signed-off-by: line from you on this one.
g.
On Tue, Mar 10, 2009 at 11:13 AM, Grant Likely
<grant.likely at secretlab.ca> wrote:
> Hi Henk,
>
> Acked-by: Grant Likely <grant.likely at secretlab.ca>
>
> Can you please repost with a blurb for the commit description and your
> signed-off-by line? The blub below makes sense in the context of this
> mailing list thread, but it won't be very useful for someone looking
> at the commit message in git. Also, your patch is line-wrap damaged
> (cut and paste into your mail client doesn't usually work) and has
> inconsistent whitespace (run it through scripts/checkpatch.pl).
>
> Jeff, after Henk provides his s-o-b line, do you want to pick it up,
> or should I merge it through my mpc52xx powerpc tree (via benh).
>
> Thanks,
> g.
>
> On Thu, Feb 19, 2009 at 3:45 AM, Henk Stegeman <henk.stegeman at gmail.com> wrote:
>> I must have made a mistake when I tested the previous patch, I
>> discovered later it still had errors:
>> - I had accidentally removed the base address in the fec_mpc52xx driver.
>> - The priv->phydev pointer was sometimes not initialized (NULL) but
>> still passed by the fec_mpc52xx driver, this pointer is then used
>> unchecked by the eth_tool_* functions (used by bridging to determine
>> port priority). As far as I see this depends on whether
>> mpc52xx_fec_open (or mpc52xx_fec_close) is called which in turn call
>> mpc52xx_init_phy to initialize priv->phydev. My work around checks the
>> priv->phydev pointer in the fec_mpc52xx driver and returns -ENODEV to
>> indicate there's no physical device. Big chance this is not the right
>> way to handle the problem, but it works, hopefully someone with some
>> more fundamental Linux network driver experience can pick this up or
>> give me some hints on this.
>>
>> At least bridging now works on my board in combination with the
>> fec_mpc52xx driver.
>>
>> ifconfig eth0 0.0.0.0 down
>> ifconfig eth1 0.0.0.0 down
>> brctl addbr br0
>> brctl setfd br0 0
>> brctl stp br0 off
>> ifconfig br0 192.168.1.30 down
>> ifconfig br0 up
>> brctl addif br0 eth0
>> ifconfig eth0 up
>> brctl addif br0 eth1
>> ifconfig eth1 up
>>
>>
>> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
>> index cd8e98b..e228973 100644
>> --- a/drivers/net/fec_mpc52xx.c
>> +++ b/drivers/net/fec_mpc52xx.c
>> @@ -847,24 +847,40 @@ static void mpc52xx_fec_get_drvinfo(struct
>> net_device *dev,
>> static int mpc52xx_fec_get_settings(struct net_device *dev, struct
>> ethtool_cmd *cmd)
>> {
>> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>> +
>> + if (!priv->phydev)
>> + return -ENODEV;
>> +
>> return phy_ethtool_gset(priv->phydev, cmd);
>> }
>>
>> static int mpc52xx_fec_set_settings(struct net_device *dev, struct
>> ethtool_cmd *cmd)
>> {
>> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>> +
>> + if (!priv->phydev)
>> + return -ENODEV;
>> +
>> return phy_ethtool_sset(priv->phydev, cmd);
>> }
>>
>> static u32 mpc52xx_fec_get_msglevel(struct net_device *dev)
>> {
>> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>> +
>> + if (!priv->phydev)
>> + return 0;
>> +
>> return priv->msg_enable;
>> }
>>
>> static void mpc52xx_fec_set_msglevel(struct net_device *dev, u32 level)
>> {
>> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>> +
>> + if (!priv->phydev)
>> + return;
>> +
>> priv->msg_enable = level;
>> }
>>
>> @@ -882,12 +898,31 @@ static int mpc52xx_fec_ioctl(struct net_device
>> *dev, struct ifreq *rq, int cmd)
>> {
>> struct mpc52xx_fec_priv *priv = netdev_priv(dev);
>>
>> + if (!priv->phydev)
>> + return -ENODEV;
>> +
>> return mpc52xx_fec_phy_mii_ioctl(priv, if_mii(rq), cmd);
>> }
>>
>> /* ======================================================================== */
>> /* OF Driver */
>> /* ======================================================================== */
>> +static const struct net_device_ops mpc52xx_fec_netdev_ops = {
>> + .ndo_open = mpc52xx_fec_open,
>> + .ndo_stop = mpc52xx_fec_close,
>> + .ndo_start_xmit = mpc52xx_fec_hard_start_xmit,
>> + .ndo_tx_timeout = mpc52xx_fec_tx_timeout,
>> + .ndo_get_stats = mpc52xx_fec_get_stats,
>> + .ndo_set_multicast_list = mpc52xx_fec_set_multicast_list,
>> + .ndo_validate_addr = eth_validate_addr,
>> + .ndo_set_mac_address = mpc52xx_fec_set_mac_address,
>> + .ndo_do_ioctl = mpc52xx_fec_ioctl,
>> +
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> + .ndo_poll_controller = mpc52xx_fec_poll_controller,
>> +#endif
>> +};
>> +
>>
>> static int __devinit
>> mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
>> @@ -929,20 +964,10 @@ mpc52xx_fec_probe(struct of_device *op, const
>> struct of_device_id *match)
>> return -EBUSY;
>>
>> /* Init ether ndev with what we have */
>> - ndev->open = mpc52xx_fec_open;
>> - ndev->stop = mpc52xx_fec_close;
>> - ndev->hard_start_xmit = mpc52xx_fec_hard_start_xmit;
>> - ndev->do_ioctl = mpc52xx_fec_ioctl;
>> ndev->ethtool_ops = &mpc52xx_fec_ethtool_ops;
>> - ndev->get_stats = mpc52xx_fec_get_stats;
>> - ndev->set_mac_address = mpc52xx_fec_set_mac_address;
>> - ndev->set_multicast_list = mpc52xx_fec_set_multicast_list;
>> - ndev->tx_timeout = mpc52xx_fec_tx_timeout;
>> ndev->watchdog_timeo = FEC_WATCHDOG_TIMEOUT;
>> ndev->base_addr = mem.start;
>> -#ifdef CONFIG_NET_POLL_CONTROLLER
>> - ndev->poll_controller = mpc52xx_fec_poll_controller;
>> -#endif
>> + ndev->netdev_ops = &mpc52xx_fec_netdev_ops;
>>
>> priv->t_irq = priv->r_irq = ndev->irq = NO_IRQ; /* IRQ are free for now */
>>
>>
>>
>> On Wed, Feb 18, 2009 at 10:48 PM, David Miller <davem at davemloft.net> wrote:
>>> From: Henk Stegeman <henk.stegeman at gmail.com>
>>> Date: Wed, 18 Feb 2009 11:41:14 +0100
>>>
>>> Please CC: netdev, now added, on all networking reports and patches.
>>>
>>> Thank you.
>>>
>>>> I discovered the hard way that because linux bridging uses
>>>> net_device_ops, bridging only works with network drivers that publish
>>>> their device operations trough net_device_ops.
>>>>
>>>> In my case running:
>>>>
>>>> brctl addif br0 eth0 (where eth0 fec_mpc52xx.c did not yet support
>>>> net_device_ops) gave me a:
>>>>
>>>> Unable to handle kernel paging request...
>>>>
>>>> After changing fec_mpc52xx.c to support net_device_ops the problem was fixed.
>>>>
>>>> If possible some kind of detection in the bridging software is i think
>>>> mostly appreciated for early detection of this problem, as it is
>>>> pretty hard to relate the error message to a not updated driver.
>>>>
>>>> cheers,
>>>>
>>>> Henk
>>>>
>>>> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
>>>> index cd8e98b..a2841eb 100644
>>>> --- a/drivers/net/fec_mpc52xx.c
>>>> +++ b/drivers/net/fec_mpc52xx.c
>>>> @@ -888,6 +888,22 @@ static int mpc52xx_fec_ioctl(struct net_device
>>>> *dev, struct ifreq *rq, int cmd)
>>>> /* ======================================================================== */
>>>> /* OF Driver */
>>>> /* ======================================================================== */
>>>> +static const struct net_device_ops mpc52xx_fec_netdev_ops = {
>>>> + .ndo_open = mpc52xx_fec_open,
>>>> + .ndo_stop = mpc52xx_fec_close,
>>>> + .ndo_start_xmit = mpc52xx_fec_hard_start_xmit,
>>>> + .ndo_tx_timeout = mpc52xx_fec_tx_timeout,
>>>> + .ndo_get_stats = mpc52xx_fec_get_stats,
>>>> + .ndo_set_multicast_list = mpc52xx_fec_set_multicast_list,
>>>> + .ndo_validate_addr = eth_validate_addr,
>>>> + .ndo_set_mac_address = mpc52xx_fec_set_mac_address,
>>>> + .ndo_do_ioctl = mpc52xx_fec_ioctl,
>>>> +
>>>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>>>> + .ndo_poll_controller = mpc52xx_fec_poll_controller,
>>>> +#endif
>>>> +};
>>>> +
>>>>
>>>> static int __devinit
>>>> mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
>>>> @@ -929,20 +945,7 @@ mpc52xx_fec_probe(struct of_device *op, const
>>>> struct of_device_id *match)
>>>> return -EBUSY;
>>>>
>>>> /* Init ether ndev with what we have */
>>>> - ndev->open = mpc52xx_fec_open;
>>>> - ndev->stop = mpc52xx_fec_close;
>>>> - ndev->hard_start_xmit = mpc52xx_fec_hard_start_xmit;
>>>> - ndev->do_ioctl = mpc52xx_fec_ioctl;
>>>> - ndev->ethtool_ops = &mpc52xx_fec_ethtool_ops;
>>>> - ndev->get_stats = mpc52xx_fec_get_stats;
>>>> - ndev->set_mac_address = mpc52xx_fec_set_mac_address;
>>>> - ndev->set_multicast_list = mpc52xx_fec_set_multicast_list;
>>>> - ndev->tx_timeout = mpc52xx_fec_tx_timeout;
>>>> - ndev->watchdog_timeo = FEC_WATCHDOG_TIMEOUT;
>>>> - ndev->base_addr = mem.start;
>>>> -#ifdef CONFIG_NET_POLL_CONTROLLER
>>>> - ndev->poll_controller = mpc52xx_fec_poll_controller;
>>>> -#endif
>>>> + ndev->netdev_ops = &mpc52xx_fec_netdev_ops;
>>>>
>>>> priv->t_irq = priv->r_irq = ndev->irq = NO_IRQ; /* IRQ are free for now */
>>>> _______________________________________________
>>>> Linuxppc-dev mailing list
>>>> Linuxppc-dev at ozlabs.org
>>>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev at ozlabs.org
>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
More information about the Linuxppc-dev
mailing list