| Message ID | E1mpwRi-00D8L8-F1@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow DSA drivers to set all phylink capabilities | expand |
On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > Phylink needs slightly more information than phylink_get_interfaces() > allows us to get from the DSA drivers - we need the MAC capabilities. > Replace the phylink_get_interfaces() method with phylink_get_caps() to > allow DSA drivers to fill in the phylink_config MAC capabilities field > as well. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- The effects of submitting new API without any user :) Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > include/net/dsa.h | 4 ++-- > net/dsa/port.c | 5 ++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index eff5c44ba377..8ca9d50cbbc2 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -645,8 +645,8 @@ struct dsa_switch_ops { > /* > * PHYLINK integration > */ > - void (*phylink_get_interfaces)(struct dsa_switch *ds, int port, > - unsigned long *supported_interfaces); > + void (*phylink_get_caps)(struct dsa_switch *ds, int port, > + struct phylink_config *config); > void (*phylink_validate)(struct dsa_switch *ds, int port, > unsigned long *supported, > struct phylink_link_state *state); > diff --git a/net/dsa/port.c b/net/dsa/port.c > index d928be884f01..6d5ebe61280b 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -1094,9 +1094,8 @@ int dsa_port_phylink_create(struct dsa_port *dp) > if (err) > mode = PHY_INTERFACE_MODE_NA; > > - if (ds->ops->phylink_get_interfaces) > - ds->ops->phylink_get_interfaces(ds, dp->index, > - dp->pl_config.supported_interfaces); > + if (ds->ops->phylink_get_caps) > + ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config); > > dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), > mode, &dsa_port_phylink_mac_ops); > -- > 2.30.2 >
On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote: > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > > Phylink needs slightly more information than phylink_get_interfaces() > > allows us to get from the DSA drivers - we need the MAC capabilities. > > Replace the phylink_get_interfaces() method with phylink_get_caps() to > > allow DSA drivers to fill in the phylink_config MAC capabilities field > > as well. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > The effects of submitting new API without any user :) It had users at the time, but they were not submitted, and the addition of MAC capabilities was a future development. Had they been submitted at the time, then they would have required updating too. So that's entirely irrelevent.
On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote: > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > > > Phylink needs slightly more information than phylink_get_interfaces() > > > allows us to get from the DSA drivers - we need the MAC capabilities. > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to > > > allow DSA drivers to fill in the phylink_config MAC capabilities field > > > as well. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > > The effects of submitting new API without any user :) > > It had users at the time, but they were not submitted, and the addition > of MAC capabilities was a future development. Had they been submitted at > the time, then they would have required updating too. That was a bit rushed... to explain more fully. Prior to the merge window, the development work was centered around only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity that the PHY_INTERFACE_MODE_NA technique brought into the many validation functions. Users of this had already been merged, and included mvneta and mvpp2. See these commits, which are all in v5.16-rc1: b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate() 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed() 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate() 8498e17ed4c5 net: mvpp2: populate supported_interfaces member 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed() d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate() fdedb695e6a8 net: mvneta: populate supported_interfaces member The original commit adding phylink_get_interfaces() extended this into DSA, and the intention was to submit at least mv88e6xxx, but it was too close to the merge window to do so. Through making that change and eventually eliminating the basex helper from all drivers that were using it, thereby making the validate() behaviour much cleaner, it then became clear that it was possible to push this cleanup further by also introducing a MAC capabilities field to phylink_config. The addition of the supported_interfaces member and the addition of the mac_capabilities member are two entirely separate developments, but I have now chosen to combine the two after the merge window in order to reduce the number of patches. They were separate patches in my tree up until relatively recently, and still are for the mt7530 and b53 drivers currently. So no, this is not "The effects of submitting new API without any user". Thanks.
On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote: > > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote: > > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > > > > Phylink needs slightly more information than phylink_get_interfaces() > > > > allows us to get from the DSA drivers - we need the MAC capabilities. > > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to > > > > allow DSA drivers to fill in the phylink_config MAC capabilities field > > > > as well. > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > --- > > > > > > The effects of submitting new API without any user :) > > > > It had users at the time, but they were not submitted, and the addition > > of MAC capabilities was a future development. Had they been submitted at > > the time, then they would have required updating too. > > That was a bit rushed... to explain more fully. > > Prior to the merge window, the development work was centered around > only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity > that the PHY_INTERFACE_MODE_NA technique brought into the many > validation functions. Users of this had already been merged, and > included mvneta and mvpp2. See these commits, which are all in > v5.16-rc1: > > b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate() > 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed() > 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate() > 8498e17ed4c5 net: mvpp2: populate supported_interfaces member > > 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed() > d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate() > fdedb695e6a8 net: mvneta: populate supported_interfaces member > > The original commit adding phylink_get_interfaces() extended this > into DSA, and the intention was to submit at least mv88e6xxx, but > it was too close to the merge window to do so. > > Through making that change and eventually eliminating the basex helper > from all drivers that were using it, thereby making the validate() > behaviour much cleaner, it then became clear that it was possible to > push this cleanup further by also introducing a MAC capabilities field > to phylink_config. > > The addition of the supported_interfaces member and the addition of the > mac_capabilities member are two entirely separate developments, but I > have now chosen to combine the two after the merge window in order to > reduce the number of patches. They were separate patches in my tree up > until relatively recently, and still are for the mt7530 and b53 drivers > currently. > > So no, this is not "The effects of submitting new API without any user". > > Thanks. Ok, the patch is not the effect of submitting new API without any user. It is just the effect of more development done to API without any user, without any causal relationship between the two. My bad.
On Wed, Nov 24, 2021 at 08:26:13PM +0000, Vladimir Oltean wrote: > On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote: > > On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote: > > > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote: > > > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > > > > > Phylink needs slightly more information than phylink_get_interfaces() > > > > > allows us to get from the DSA drivers - we need the MAC capabilities. > > > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to > > > > > allow DSA drivers to fill in the phylink_config MAC capabilities field > > > > > as well. > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > --- > > > > > > > > The effects of submitting new API without any user :) > > > > > > It had users at the time, but they were not submitted, and the addition > > > of MAC capabilities was a future development. Had they been submitted at > > > the time, then they would have required updating too. > > > > That was a bit rushed... to explain more fully. > > > > Prior to the merge window, the development work was centered around > > only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity > > that the PHY_INTERFACE_MODE_NA technique brought into the many > > validation functions. Users of this had already been merged, and > > included mvneta and mvpp2. See these commits, which are all in > > v5.16-rc1: > > > > b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate() > > 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed() > > 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate() > > 8498e17ed4c5 net: mvpp2: populate supported_interfaces member > > > > 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed() > > d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate() > > fdedb695e6a8 net: mvneta: populate supported_interfaces member > > > > The original commit adding phylink_get_interfaces() extended this > > into DSA, and the intention was to submit at least mv88e6xxx, but > > it was too close to the merge window to do so. > > > > Through making that change and eventually eliminating the basex helper > > from all drivers that were using it, thereby making the validate() > > behaviour much cleaner, it then became clear that it was possible to > > push this cleanup further by also introducing a MAC capabilities field > > to phylink_config. > > > > The addition of the supported_interfaces member and the addition of the > > mac_capabilities member are two entirely separate developments, but I > > have now chosen to combine the two after the merge window in order to > > reduce the number of patches. They were separate patches in my tree up > > until relatively recently, and still are for the mt7530 and b53 drivers > > currently. > > > > So no, this is not "The effects of submitting new API without any user". > > > > Thanks. > > Ok, the patch is not the effect of submitting new API without any user. > It is just the effect of more development done to API without any user, > without any causal relationship between the two. My bad. I do wonder whether you intentionally missed where I said "It had users at the time, but they were not submitted". This is why we don't get on well, you're always so confrontational.
On Wed, Nov 24, 2021 at 08:56:33PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 24, 2021 at 08:26:13PM +0000, Vladimir Oltean wrote: > > On Wed, Nov 24, 2021 at 07:10:49PM +0000, Russell King (Oracle) wrote: > > > On Wed, Nov 24, 2021 at 06:26:48PM +0000, Russell King (Oracle) wrote: > > > > On Wed, Nov 24, 2021 at 06:15:08PM +0000, Vladimir Oltean wrote: > > > > > On Wed, Nov 24, 2021 at 05:52:38PM +0000, Russell King (Oracle) wrote: > > > > > > Phylink needs slightly more information than phylink_get_interfaces() > > > > > > allows us to get from the DSA drivers - we need the MAC capabilities. > > > > > > Replace the phylink_get_interfaces() method with phylink_get_caps() to > > > > > > allow DSA drivers to fill in the phylink_config MAC capabilities field > > > > > > as well. > > > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > --- > > > > > > > > > > The effects of submitting new API without any user :) > > > > > > > > It had users at the time, but they were not submitted, and the addition > > > > of MAC capabilities was a future development. Had they been submitted at > > > > the time, then they would have required updating too. > > > > > > That was a bit rushed... to explain more fully. > > > > > > Prior to the merge window, the development work was centered around > > > only eliminating the PHY_INTERFACE_MODE_xxx checks and the complexity > > > that the PHY_INTERFACE_MODE_NA technique brought into the many > > > validation functions. Users of this had already been merged, and > > > included mvneta and mvpp2. See these commits, which are all in > > > v5.16-rc1: > > > > > > b63f1117aefc net: mvpp2: clean up mvpp2_phylink_validate() > > > 76947a635874 net: mvpp2: drop use of phylink_helper_basex_speed() > > > 6c0c4b7ac06f net: mvpp2: remove interface checks in mvpp2_phylink_validate() > > > 8498e17ed4c5 net: mvpp2: populate supported_interfaces member > > > > > > 099cbfa286ab net: mvneta: drop use of phylink_helper_basex_speed() > > > d9ca72807ecb net: mvneta: remove interface checks in mvneta_validate() > > > fdedb695e6a8 net: mvneta: populate supported_interfaces member > > > > > > The original commit adding phylink_get_interfaces() extended this > > > into DSA, and the intention was to submit at least mv88e6xxx, but > > > it was too close to the merge window to do so. > > > > > > Through making that change and eventually eliminating the basex helper > > > from all drivers that were using it, thereby making the validate() > > > behaviour much cleaner, it then became clear that it was possible to > > > push this cleanup further by also introducing a MAC capabilities field > > > to phylink_config. > > > > > > The addition of the supported_interfaces member and the addition of the > > > mac_capabilities member are two entirely separate developments, but I > > > have now chosen to combine the two after the merge window in order to > > > reduce the number of patches. They were separate patches in my tree up > > > until relatively recently, and still are for the mt7530 and b53 drivers > > > currently. > > > > > > So no, this is not "The effects of submitting new API without any user". > > > > > > Thanks. > > > > Ok, the patch is not the effect of submitting new API without any user. > > It is just the effect of more development done to API without any user, > > without any causal relationship between the two. My bad. > > I do wonder whether you intentionally missed where I said "It had users > at the time, but they were not submitted". This is why we don't get on > well, you're always so confrontational. David who applied your patch can correct me, but my understanding from the little time I've spent on netdev is that dead code isn't a candidate for getting accepted into the tree, even more so in the last few days before the merge window, from where it got into v5.16-rc1. About the only exception I know of is when introducing a function which is only to be called later in the series, and both the caller and the callee are subject to review. Sure, your hook isn't doing any harm there, save for a few extra bytes of kernel .text, and I know that your intention was for Prasanna to use that new callback for the lan937x driver, but the truth is that there are other ways to achieve what you want, like for example ask Prasanna to pick your patch and submit it along with his lan937x driver, or you submit it yourself when the time comes for other drivers to be converted, whichever comes first. So yes, I take issue with that as a matter of principle, I very much expect that a kernel developer of your experience does not set a precedent and a pretext for people who submit various shady stuff to the kernel just to make their downstream life easier.
diff --git a/include/net/dsa.h b/include/net/dsa.h index eff5c44ba377..8ca9d50cbbc2 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -645,8 +645,8 @@ struct dsa_switch_ops { /* * PHYLINK integration */ - void (*phylink_get_interfaces)(struct dsa_switch *ds, int port, - unsigned long *supported_interfaces); + void (*phylink_get_caps)(struct dsa_switch *ds, int port, + struct phylink_config *config); void (*phylink_validate)(struct dsa_switch *ds, int port, unsigned long *supported, struct phylink_link_state *state); diff --git a/net/dsa/port.c b/net/dsa/port.c index d928be884f01..6d5ebe61280b 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1094,9 +1094,8 @@ int dsa_port_phylink_create(struct dsa_port *dp) if (err) mode = PHY_INTERFACE_MODE_NA; - if (ds->ops->phylink_get_interfaces) - ds->ops->phylink_get_interfaces(ds, dp->index, - dp->pl_config.supported_interfaces); + if (ds->ops->phylink_get_caps) + ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config); dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), mode, &dsa_port_phylink_mac_ops);
Phylink needs slightly more information than phylink_get_interfaces() allows us to get from the DSA drivers - we need the MAC capabilities. Replace the phylink_get_interfaces() method with phylink_get_caps() to allow DSA drivers to fill in the phylink_config MAC capabilities field as well. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- include/net/dsa.h | 4 ++-- net/dsa/port.c | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-)