Messages in this thread | | | Date | Wed, 12 Apr 2023 10:38:12 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH net v5 1/3] net: phylink: add phylink_expects_phy() method |
| |
Hello everyone,
On Thu, 30 Mar 2023 17:14:02 +0800 Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> wrote:
> Provide phylink_expects_phy() to allow MAC drivers to check if it > is expecting a PHY to attach to. Since fixed-linked setups do not > need to attach to a PHY. > > Provides a boolean value as to if the MAC should expect a PHY. > Returns true if a PHY is expected.
I'm currently working on the TSE rework for dwmac_socfpga, and I noticed one regression since this patch, when using an SFP, see details below :
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> > --- > drivers/net/phy/phylink.c | 19 +++++++++++++++++++ > include/linux/phylink.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 1a2f074685fa..30c166b33468 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1586,6 +1586,25 @@ void phylink_destroy(struct phylink *pl) > } > EXPORT_SYMBOL_GPL(phylink_destroy); > > +/** > + * phylink_expects_phy() - Determine if phylink expects a phy to be > attached > + * @pl: a pointer to a &struct phylink returned from phylink_create() > + * > + * When using fixed-link mode, or in-band mode with 1000base-X or > 2500base-X, > + * no PHY is needed. > + * > + * Returns true if phylink will be expecting a PHY. > + */ > +bool phylink_expects_phy(struct phylink *pl) > +{ > + if (pl->cfg_link_an_mode == MLO_AN_FIXED || > + (pl->cfg_link_an_mode == MLO_AN_INBAND && > + phy_interface_mode_is_8023z(pl->link_config.interface)))
From the discussion, at one point Russell mentionned [1] : "If there's a sfp bus, then we don't expect a PHY from the MAC driver (as there can only be one PHY attached), and as phylink_expects_phy() is for the MAC driver to use, we should be returning false if pl->sfp_bus != NULL."
This makes sense and indeed adding the relevant check solves the issue.
Am I correct in assuming this was an unintentional omission from this patch, or was the pl->sfp_bus check dropped on purpose ?
> + return false; > + return true; > +} > +EXPORT_SYMBOL_GPL(phylink_expects_phy);
Thanks,
Maxime
[1] : https://lore.kernel.org/netdev/ZCQJWcdfmualIjvX@shell.armlinux.org.uk/
| |