On Fri, 2015-04-24 at 21:53 +0300, Sergei Shtylyov wrote:
> On 04/23/2015 02:22 AM, Florian Fainelli wrote:
>
> [...]
>
> >>>> + if (ecmd->duplex == DUPLEX_FULL)
> >>>> + priv->duplex = 1;
> >>>> + else
> >>>> + priv->duplex = 0;
>
> >>> Why not use what priv->phydev->duplex has cached for you?
>
> >> Because we compare 'priv->duplex' with 'priv->phydev->duplex' in
> >> ravb_adjust_link(). Or what did you mean?
>
> > Oh I see how you are using this now, but it does not look like it is
> > necessary, since you use phy_ethtool_sset() using phydev->duplex
>
> It only writes to it, doesn't use it AFAICS...
>
> > directly ought to be enough anywhere in your driver?
>
> 'priv->phydev' is NULL when the device is closed, so I just can't call
> phy_ethtool_sset().
>
> > Unless there is a
> > mode where you are running PHY-less, and not using a fixed PHY to
> > emulate a PHY...
>
> No such mode.
>
> >> [...]
>
> >>>> +static int ravb_nway_reset(struct net_device *ndev)
> >>>> +{
> >>>> + struct ravb_private *priv = netdev_priv(ndev);
> >>>> + int error = -ENODEV;
> >>>> + unsigned long flags;
> >>>> +
> >>>> + if (priv->phydev) {
>
> >>> Is checking against priv->phydev really necessary, it does not look like
> >>> the driver will work or accept an invalid PHY device at all anyway?
>
> This check was copied from sh_eth that was fixed by Ben ot to crash due to
> 'ethtool' being called on closed device, see:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/renesas/sh_eth.c?id=4f9dce230b32eec45cec8c28cae61efdfa2f7d57
>
> That commit refers to a dangling pointer, not sure what does this mean...
> The PHy device doesn't seem to be freed by phy_disconnect(). Ben?
[...]
In practice the phy_device is unlikely to be freed immediately. Bt it
is certainly not valid for a net driver to pass a phy_device pointer to
phylib functions after calling phy_disconnect() on it.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
