Re: [PATCH net-next v3 15/15] net: fec: add support for PHYs with SmartEEE support

[Oleksij Rempel]

On Tue, Jan 31, 2023 at 10:52:31PM +0200, Vladimir Oltean wrote:
> On Mon, Jan 30, 2023 at 09:07:14AM +0100, Oleksij Rempel wrote:
> > Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
> > But this chips are used sometimes in combinations with SmartEEE capable
> > PHYs.
> > So, instead of aborting get/set_eee access on MACs without EEE support,
> > ask PHY if it is able to do the EEE job by using SmartEEE.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index e6238e53940d..25a2a9d860de 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> >  	struct ethtool_eee *p = &fep->eee;
> >  
> > -	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
> > -		return -EOPNOTSUPP;
> > +	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
> > +		if (!netif_running(ndev))
> > +			return -ENETDOWN;
> > +
> > +		if (!phy_has_smarteee(ndev->phydev))
> > +			return -EOPNOTSUPP;
> > +
> > +		return phy_ethtool_get_eee(ndev->phydev, edata);
> 
> I see many places in the fec driver guarding against a NULL
> ndev->phydev, and TBH I don't completely understand why.
> I guess it's because ndev->phydev is populated at fec_enet_open() time.
> 
> But then again, if the netif_running() check is sufficient to imply
> presence of ndev->phydev as you suggest, then why does fec_enet_ioctl()
> have this?
> 
> 	if (!netif_running(ndev))
> 		return -EINVAL;
> 
> 	if (!phydev)
> 		return -ENODEV;
> 
> Asking because phy_init_eee(), phy_ethtool_set_eee() and
> phy_ethtool_get_eee() don't support being called with a NULL phydev.

Hm..
phy_start() is protected against NULL phydev and it is used in
fec_enet_open().

Right now i do not know what is better way go. Any preferences?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |