Re: [PATCH net-next v3 07/47] net: phy: Add support for rate adaptation

[Sean Anderson]

On 7/16/22 3:39 PM, Andrew Lunn wrote:
> >   drivers/net/phy/phy.c | 21 +++++++++++++++++++++
> >   include/linux/phy.h   | 38 ++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 8d3ee3a6495b..cf4a8b055a42 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -114,6 +114,27 @@ void phy_print_status(struct phy_device *phydev)
> >   }
> >   EXPORT_SYMBOL(phy_print_status);
> >   
> > +/**
> > + * phy_get_rate_adaptation - determine if rate adaptation is supported
> > + * @phydev: The phy device to return rate adaptation for
> > + * @iface: The interface mode to use
> > + *
> > + * This determines the type of rate adaptation (if any) that @phy supports
> > + * using @iface. @iface may be %PHY_INTERFACE_MODE_NA to determine if any
> > + * interface supports rate adaptation.
> > + *
> > + * Return: The type of rate adaptation @phy supports for @iface, or
> > + *         %RATE_ADAPT_NONE.
> > + */
> > +enum rate_adaptation phy_get_rate_adaptation(struct phy_device *phydev,
> > +					     phy_interface_t iface)
> > +{
> > +	if (phydev->drv->get_rate_adaptation)
> > +		return phydev->drv->get_rate_adaptation(phydev, iface);
>
> It is normal that any call into the driver is performed with the
> phydev->lock held.

Ah, so like phy_ethtool_get_strings.

> >   #define PHY_INIT_TIMEOUT	100000
> >   #define PHY_FORCE_TIMEOUT	10
> > @@ -570,6 +588,7 @@ struct macsec_ops;
> >    * @lp_advertising: Current link partner advertised linkmodes
> >    * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
> >    * @autoneg: Flag autoneg being used
> > + * @rate_adaptation: Current rate adaptation mode
> >    * @link: Current link state
> >    * @autoneg_complete: Flag auto negotiation of the link has completed
> >    * @mdix: Current crossover
> > @@ -637,6 +656,8 @@ struct phy_device {
> >   	unsigned irq_suspended:1;
> >   	unsigned irq_rerun:1;
> >   
> > +	enum rate_adaptation rate_adaptation;
>
> It is not clear what the locking is on this member. Is it only safe to
> access it during the adjust_link callback, when it is guaranteed that
> the phydev->lock is held, so the value is consistent? Or is the MAC
> allowed to access this at other times?

The former. My intention is that this has the same access as link/interface/speed/duplex.

--Sean