Re: [PATCH net-next v2 1/6] net: dsa: add support for phylink mac_select_pcs()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Feb 21, 2022 at 01:30:09PM +0000, Russell King (Oracle) wrote:
> On Sat, Feb 19, 2022 at 11:22:24PM +0200, Vladimir Oltean wrote:
> > On Sat, Feb 19, 2022 at 11:12:41PM +0200, Vladimir Oltean wrote:
> > > >  static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> > > >  	.validate = dsa_port_phylink_validate,
> > > > +	.mac_select_pcs = dsa_port_phylink_mac_select_pcs,
> > > 
> > > This patch breaks probing on DSA switch drivers that weren't converted
> > > to supported_interfaces, due to this check in phylink_create():
> > 
> > And this is only the most superficial layer of breakage. Everywhere in
> > phylink.c where pl->mac_ops->mac_select_pcs() is used, its presence is
> > checked and non-zero return codes from it are treated as hard errors,
> > even -EOPNOTSUPP, even if this particular error code is probably
> > intended to behave identically as the absence of the function pointer,
> > for compatibility.
> 
> I don't understand what problem you're getting at here - and I don't
> think there is a problem.
> 
> While I know it's conventional in DSA to use EOPNOTSUPP to indicate
> that a called method is not implemented, this is not something that
> is common across the board - and is not necessary here.
> 
> The implementation of dsa_port_phylink_mac_select_pcs() returns a
> NULL PCS when the DSA operation for it is not implemented. This
> means that:
> 
> 1) phylink_validate_mac_and_pcs() won't fail due to mac_select_pcs()
>    being present but DSA drivers not implementing it.
> 
> 2) phylink_major_config() will not attempt to call phylink_set_pcs()
>    to change the PCS.
> 
> So, that much is perfectly safe.
> 
> As for your previous email reporting the problem with phylink_create(),
> thanks for the report and sorry for the breakage - the breakage was
> obviously not intended, and came about because of all the patch
> shuffling I've done over the last six months trying to get these
> changes in, and having forgotten about this dependency.
> 
> I imagine the reason you've raised EOPNOTSUPP is because you wanted to
> change dsa_port_phylink_mac_select_pcs() to return an error-pointer
> encoded with that error code rather than NULL, but you then (no
> surprises to me) caused phylink to fail.
> 
> Considering the idea of using EOPNOTSUPP, at the two places we call
> mac_select_pcs(), we would need to treat this the same way we currently
> treat NULL. We would also need phylink_create() to call
> mac_select_pcs() if the method is non-NULL to discover if the DSA
> sub-driver implements the method - but we would need to choose an
> interface at this point.
> 
> I think at this point, I'd rather:
> 
> 1) add a bool in struct phylink to indicate whether we should be calling
>    mac_select_pcs, and replace the
> 
> 	if (pl->mac_ops->mac_select_pcs)
> 
>    with
> 
>         if (pl->using_mac_select_pcs)
> 
> 2) have phylink_create() do:
> 
> 	bool using_mac_select_pcs = false;
> 
> 	if (mac_ops->mac_select_pcs &&
> 	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) != 
> 	      ERR_PTR(-EOPNOTSUPP))
> 		using_mac_select_pcs = true;
> 
> 	if (using_mac_select_pcs &&
> 	    phy_interface_empty(config->supported_interfaces)) {
> 		...
> 
> 	...
> 
> 	pl->using_mac_select_pcs = using_mac_select_pcs;
> 
> which should give what was intended until DSA drivers are all updated
> to fill in config->supported_interfaces.

Please try this patch, thanks:

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6c7ab4a7a3be..de0557bbd4a7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -74,6 +74,7 @@ struct phylink {
 	struct work_struct resolve;
 
 	bool mac_link_dropped;
+	bool using_mac_select_pcs;
 
 	struct sfp_bus *sfp_bus;
 	bool sfp_may_have_phy;
@@ -416,7 +417,7 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 	int ret;
 
 	/* Get the PCS for this interface mode */
-	if (pl->mac_ops->mac_select_pcs) {
+	if (pl->using_mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs))
 			return PTR_ERR(pcs);
@@ -791,7 +792,7 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 
 	phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
 
-	if (pl->mac_ops->mac_select_pcs) {
+	if (pl->using_mac_select_pcs) {
 		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
 		if (IS_ERR(pcs)) {
 			phylink_err(pl,
@@ -1204,11 +1205,17 @@ struct phylink *phylink_create(struct phylink_config *config,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *mac_ops)
 {
+	bool using_mac_select_pcs = false;
 	struct phylink *pl;
 	int ret;
 
-	/* Validate the supplied configuration */
 	if (mac_ops->mac_select_pcs &&
+	    mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
+	      ERR_PTR(-EOPNOTSUPP))
+		using_mac_select_pcs = true;
+
+	/* Validate the supplied configuration */
+	if (using_mac_select_pcs &&
 	    phy_interface_empty(config->supported_interfaces)) {
 		dev_err(config->dev,
 			"phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
@@ -1232,6 +1239,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
+	pl->using_mac_select_pcs = using_mac_select_pcs;
 	pl->phy_state.interface = iface;
 	pl->link_interface = iface;
 	if (iface == PHY_INTERFACE_MODE_MOCA)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 258782bf4271..367d141c6971 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1058,8 +1058,8 @@ dsa_port_phylink_mac_select_pcs(struct phylink_config *config,
 				phy_interface_t interface)
 {
 	struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+	struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP);
 	struct dsa_switch *ds = dp->ds;
-	struct phylink_pcs *pcs = NULL;
 
 	if (ds->ops->phylink_mac_select_pcs)
 		pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface);
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!