| Message ID | E1nKlY3-009aKs-Oo@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bde018222c6b084ac32933a9f933581dd83da18e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: qca8k: convert to phylink_pcs and mark as non-legacy | expand |
Hello, On Thu, Feb 17, 2022 at 06:30:35PM +0000, Russell King (Oracle) wrote: > Add DSA support for the phylink mac_select_pcs() method so DSA drivers > can return provide phylink with the appropriate PCS for the PHY > interface mode. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > include/net/dsa.h | 3 +++ > net/dsa/port.c | 15 +++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 85cb9aed4c51..87aef3ed88a7 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -788,6 +788,9 @@ struct dsa_switch_ops { > void (*phylink_validate)(struct dsa_switch *ds, int port, > unsigned long *supported, > struct phylink_link_state *state); > + struct phylink_pcs *(*phylink_mac_select_pcs)(struct dsa_switch *ds, > + int port, > + phy_interface_t iface); > int (*phylink_mac_link_state)(struct dsa_switch *ds, int port, > struct phylink_link_state *state); > void (*phylink_mac_config)(struct dsa_switch *ds, int port, > diff --git a/net/dsa/port.c b/net/dsa/port.c > index cca5cf686f74..d8534fd9fab9 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -1053,6 +1053,20 @@ static void dsa_port_phylink_mac_pcs_get_state(struct phylink_config *config, > } > } > > +static struct phylink_pcs * > +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 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); > + > + return pcs; > +} > + > static void dsa_port_phylink_mac_config(struct phylink_config *config, > unsigned int mode, > const struct phylink_link_state *state) > @@ -1119,6 +1133,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config, > > 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(): /* Validate the supplied configuration */ if (mac_ops->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"); return ERR_PTR(-EINVAL); } > .mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state, > .mac_config = dsa_port_phylink_mac_config, > .mac_an_restart = dsa_port_phylink_mac_an_restart, > -- > 2.30.2 >
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.
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.
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);
Hello, 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. I didn't study the problem enough to be in the position to suggest the best solution. As you've explained*, phylink works properly when mac_select_pcs() returns NULL but not special error codes, so extra handling would be required for those - and you've shown an approach that seems reasonable if we use -EOPNOTSUPP. Alternatively, phylink_create() gets the initial PHY interface mode passed to it, I wonder, couldn't we call mac_select_pcs() with that in order to determine whether the function is a stub or not? I see that only axienet_mac_select_pcs returns NULL based on the interface mode, and I assume it will never get passed an invalid-to-it PHY interface mode. For this reason we can't pass PHY_INTERFACE_MODE_NA as long as we check for NULL instead of -EOPNOTSUPP - because drivers may not expect this PHY interface type. So this second approach may also work and may require less phylink rework, although it may be slightly more prone to subtle breakage, if for whatever reason I don't see right now, the phylink instance gets created with an undetermined PHY mode. Either of these solutions is fine by me, with -EOPNOTSUPP probably being preferable for the extra safety at the expense of extra rework. *and as I haven't considered, to be honest. When phylink_major_config() gets called after a SGMII to 10GBaseR switchover, and mac_select_pcs is called and returns NULL, the current behavior is to keep working with the PCS for SGMII. Is that intended?
On Mon, Feb 21, 2022 at 02:15:26PM +0000, Russell King (Oracle) wrote:
> Please try this patch, thanks:
Probing, bring the interface up, down, and testing with traffic over a
QSGMII port with a pre-March 2020 phylink_pcs works as before, thanks.
On Mon, Feb 21, 2022 at 04:32:54PM +0200, Vladimir Oltean wrote: > Alternatively, phylink_create() gets the initial PHY interface mode > passed to it, I wonder, couldn't we call mac_select_pcs() with that in > order to determine whether the function is a stub or not? That would be rather prone to odd behaviour depending on how phylink_create() is called, depending on the initial interface mode. If the initial interface mode causes mac_select_pcs() to return NULL but it actually needed to return a PCS for a different interface mode, then we fail. > *and as I haven't considered, to be honest. When phylink_major_config() > gets called after a SGMII to 10GBaseR switchover, and mac_select_pcs is > called and returns NULL, the current behavior is to keep working with > the PCS for SGMII. Is that intended? It was not originally intended, but as a result of the discussion around this patch which didn't go anywhere useful, I dropped it as a means to a path of least resistance. https://patchwork.kernel.org/project/linux-arm-kernel/patch/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/
On Mon, Feb 21, 2022 at 02:44:34PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 21, 2022 at 04:32:54PM +0200, Vladimir Oltean wrote: > > Alternatively, phylink_create() gets the initial PHY interface mode > > passed to it, I wonder, couldn't we call mac_select_pcs() with that in > > order to determine whether the function is a stub or not? > > That would be rather prone to odd behaviour depending on how > phylink_create() is called, depending on the initial interface mode. > If the initial interface mode causes mac_select_pcs() to return NULL > but it actually needed to return a PCS for a different interface mode, > then we fail. I agree. I just wanted to make it clear that if you have a better idea than a pointer-encoded -EOPNOTSUPP, I'm not bent on going with -EOPNOTSUPP. > > *and as I haven't considered, to be honest. When phylink_major_config() > > gets called after a SGMII to 10GBaseR switchover, and mac_select_pcs is > > called and returns NULL, the current behavior is to keep working with > > the PCS for SGMII. Is that intended? > > It was not originally intended, but as a result of the discussion > around this patch which didn't go anywhere useful, I dropped it as > a means to a path of least resistance. > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/ Oh, but that patch didn't close exactly this condition that we're talking about here, did it? It allows phylink_set_pcs() to be called with NULL, but phylink_major_config() still has the non-NULL check, which prevents it from having any effect in this scenario: /* If we have a new PCS, switch to the new PCS after preparing the MAC * for the change. */ if (pcs) phylink_set_pcs(pl, pcs); I re-read the conversation and I still don't see this argument being given, otherwise I wouldn't have opposed...
On Mon, Feb 21, 2022 at 04:55:40PM +0200, Vladimir Oltean wrote: > On Mon, Feb 21, 2022 at 02:44:34PM +0000, Russell King (Oracle) wrote: > > > *and as I haven't considered, to be honest. When phylink_major_config() > > > gets called after a SGMII to 10GBaseR switchover, and mac_select_pcs is > > > called and returns NULL, the current behavior is to keep working with > > > the PCS for SGMII. Is that intended? > > > > It was not originally intended, but as a result of the discussion > > around this patch which didn't go anywhere useful, I dropped it as > > a means to a path of least resistance. > > > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/ > > Oh, but that patch didn't close exactly this condition that we're > talking about here, did it? It allows phylink_set_pcs() to be called > with NULL, but phylink_major_config() still has the non-NULL check, > which prevents it from having any effect in this scenario: > > /* If we have a new PCS, switch to the new PCS after preparing the MAC > * for the change. > */ > if (pcs) > phylink_set_pcs(pl, pcs); > > I re-read the conversation and I still don't see this argument being > given, otherwise I wouldn't have opposed... The reason for that condition above is to avoid disrupting the case where drivers which do not (yet) provide a mac_select_pcs() implementation (therefore pcs will be NULL here) but instead register their PCS by calling phylink_set_pcs() immediately after a call to phylink_create(). If the above call to phylink_set_pcs() was unconditional, then: 1) it would break these existing drivers, 2) we would definitely need to make phylink_set_pcs() safe to call with a NULL pcs argument to avoid crashing when in e.g. pcs-less modes. If that usage was eliminated, then the problem goes away... but that means changing drivers, and changing drivers is always a long hard slog that takes months and several kernel cycles to do.
diff --git a/include/net/dsa.h b/include/net/dsa.h index 85cb9aed4c51..87aef3ed88a7 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -788,6 +788,9 @@ struct dsa_switch_ops { void (*phylink_validate)(struct dsa_switch *ds, int port, unsigned long *supported, struct phylink_link_state *state); + struct phylink_pcs *(*phylink_mac_select_pcs)(struct dsa_switch *ds, + int port, + phy_interface_t iface); int (*phylink_mac_link_state)(struct dsa_switch *ds, int port, struct phylink_link_state *state); void (*phylink_mac_config)(struct dsa_switch *ds, int port, diff --git a/net/dsa/port.c b/net/dsa/port.c index cca5cf686f74..d8534fd9fab9 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1053,6 +1053,20 @@ static void dsa_port_phylink_mac_pcs_get_state(struct phylink_config *config, } } +static struct phylink_pcs * +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 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); + + return pcs; +} + static void dsa_port_phylink_mac_config(struct phylink_config *config, unsigned int mode, const struct phylink_link_state *state) @@ -1119,6 +1133,7 @@ static void dsa_port_phylink_mac_link_up(struct phylink_config *config, 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, .mac_pcs_get_state = dsa_port_phylink_mac_pcs_get_state, .mac_config = dsa_port_phylink_mac_config, .mac_an_restart = dsa_port_phylink_mac_an_restart,
Add DSA support for the phylink mac_select_pcs() method so DSA drivers can return provide phylink with the appropriate PCS for the PHY interface mode. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- include/net/dsa.h | 3 +++ net/dsa/port.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+)