Date:   Wed, 1 Feb 2023 13:45:09 +0100
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Arun.Ramadoss@microchip.com
Subject: Re: [PATCH net-next v3 15/15] net: fec: add support for PHYs with
 SmartEEE support
Message-ID: <20230201124509.GA31030@pengutronix.de>
References: <20230130080714.139492-1-o.rempel@pengutronix.de>
 <20230130080714.139492-16-o.rempel@pengutronix.de>
 <20230131205231.ck3xnziejgtr64ig@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230131205231.ck3xnziejgtr64ig@skbuf>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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@pengutronix.de>
> > ---
> >  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 |