| Message ID | 20220911200244.549029-7-colin.foster@in-advantage.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for the the vsc7512 internal copper phys | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote: > phylink_generic_validate() requires that mac_capabilities is correctly > populated. While no existing drivers have used phylink_generic_validate(), > the ocelot_ext.c driver will. Populate this element so the use of existing > functions is possible. Ocelot always fills in the .phylink_validate method in struct dsa_switch_ops, mac_capabilities won't be used as phylink_generic_validate() will not be called by dsa_port_phylink_validate(). Also "no existing drivers have used phylink_generic_validate()" I wonder which drivers you are referring to there. If you are referring to DSA drivers, then it is extensively used. The following is from Linus' tree as of today: $ grep -rl 'dsa_switch_ops' drivers/net/dsa | xargs grep -l phylink_mac_ | xargs grep -L phylink_validate drivers/net/dsa/xrs700x/xrs700x.c drivers/net/dsa/mt7530.c drivers/net/dsa/qca/ar9331.c drivers/net/dsa/qca/qca8k-8xxx.c drivers/net/dsa/bcm_sf2.c drivers/net/dsa/rzn1_a5psw.c drivers/net/dsa/b53/b53_common.c drivers/net/dsa/mv88e6xxx/chip.c drivers/net/dsa/microchip/ksz_common.c drivers/net/dsa/sja1105/sja1105_main.c drivers/net/dsa/lantiq_gswip.c drivers/net/dsa/realtek/rtl8366rb.c drivers/net/dsa/realtek/rtl8365mb.c So, I don't think the commit description is anywhere near correct. Secondly, I don't see a purpose for this patch in the following patches, as Ocelot continues to always fill in .phylink_validate, and as I mentioned above, as long as that member is filled in, mac_capabilities won't be used unless you explicitly call phylink_generic_validate() in your .phylink_validate() callback. Therefore, I think you can drop this patch from your series and you won't see any functional change. > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- > > v1 from previous RFC: > * New patch > > --- > drivers/net/dsa/ocelot/felix.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > index 95a5c5d0815c..201bf3bdd67d 100644 > --- a/drivers/net/dsa/ocelot/felix.c > +++ b/drivers/net/dsa/ocelot/felix.c > @@ -958,6 +958,9 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > > __set_bit(ocelot->ports[port]->phy_mode, > config->supported_interfaces); > + > + config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 | > + MAC_100 | MAC_1000FD | MAC_2500FD; > } > > static void felix_phylink_validate(struct dsa_switch *ds, int port, > -- > 2.25.1 > >
On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote: > On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote: > > phylink_generic_validate() requires that mac_capabilities is correctly > > populated. While no existing drivers have used phylink_generic_validate(), > > the ocelot_ext.c driver will. Populate this element so the use of existing > > functions is possible. > > Ocelot always fills in the .phylink_validate method in struct > dsa_switch_ops, mac_capabilities won't be used as > phylink_generic_validate() will not be called by > dsa_port_phylink_validate(). Correct, but felix_phylink_validate() _can_ still directly call phylink_validate(), right? Colin does not have the full support for ocelot_ext in this patch set, but this is what he intends to do. > Also "no existing drivers have used phylink_generic_validate()" I > wonder which drivers you are referring to there. If you are referring > to DSA drivers, then it is extensively used. The following is from > Linus' tree as of today: By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c, two drivers in their own right, which use the common felix.c to talk to (a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/. It is true that these existing drivers do not use phylink_generic_validate(). Furthermore, Colin's new ocelot_ext.c is on the same level as felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way, and will want to use phylink_generic_validate(). > Secondly, I don't see a purpose for this patch in the following > patches, as Ocelot continues to always fill in .phylink_validate, > and as I mentioned above, as long as that member is filled in, > mac_capabilities won't be used unless you explicitly call > phylink_generic_validate() in your .phylink_validate() callback. Yes, explicit calling is what Colin explained that he wants to do. > Therefore, I think you can drop this patch from your series and > you won't see any functional change. This is true. I am also a bit surprised at Colin's choices to (a) not ask the netdev maintainers to pull into net-next the immutable branch that Lee provided here: https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/ and instead send some patches for review which are difficult to apply directly to any tree (b) split the work he submitted such that he populates mac_capabilities but does not make any use of it (not call phylink_generic_validate from anywhere). We try as much as possible to not leave dead code behind in the mainline tree, even if future work is intended to bring it to life. I do understand that this is an RFC so the patches weren't intended to be applied as is, but it is still confusing to review a change which, as you've correctly pointed out, has no effect to the git tree as it stands.
On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote: > > Therefore, I think you can drop this patch from your series and > > you won't see any functional change. > > This is true. I am also a bit surprised at Colin's choices to > (b) split the work he submitted such that he populates mac_capabilities > but does not make any use of it (not call phylink_generic_validate > from anywhere). We try as much as possible to not leave dead code > behind in the mainline tree, even if future work is intended to > bring it to life. I do understand that this is an RFC so the patches > weren't intended to be applied as is, but it is still confusing to > review a change which, as you've correctly pointed out, has no > effect to the git tree as it stands. Ah, I retract this comment; after actually looking at all the patches, I do see that in patch 8/8, Colin does call phylink_generic_validate().
On Mon, Sep 12, 2022 at 11:41:18AM +0000, Vladimir Oltean wrote: > On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote: > > > Therefore, I think you can drop this patch from your series and > > > you won't see any functional change. > > > > This is true. I am also a bit surprised at Colin's choices to > > (b) split the work he submitted such that he populates mac_capabilities > > but does not make any use of it (not call phylink_generic_validate > > from anywhere). We try as much as possible to not leave dead code > > behind in the mainline tree, even if future work is intended to > > bring it to life. I do understand that this is an RFC so the patches > > weren't intended to be applied as is, but it is still confusing to > > review a change which, as you've correctly pointed out, has no > > effect to the git tree as it stands. > > Ah, I retract this comment; after actually looking at all the patches, I > do see that in patch 8/8, Colin does call phylink_generic_validate(). Good point, I obviously missed that in the series.
On Mon, Sep 12, 2022 at 04:32:59PM +0100, Russell King (Oracle) wrote: > On Mon, Sep 12, 2022 at 11:41:18AM +0000, Vladimir Oltean wrote: > > On Mon, Sep 12, 2022 at 01:16:21PM +0300, Vladimir Oltean wrote: > > > > Therefore, I think you can drop this patch from your series and > > > > you won't see any functional change. > > > > > > This is true. I am also a bit surprised at Colin's choices to > > > (b) split the work he submitted such that he populates mac_capabilities > > > but does not make any use of it (not call phylink_generic_validate > > > from anywhere). We try as much as possible to not leave dead code > > > behind in the mainline tree, even if future work is intended to > > > bring it to life. I do understand that this is an RFC so the patches > > > weren't intended to be applied as is, but it is still confusing to > > > review a change which, as you've correctly pointed out, has no > > > effect to the git tree as it stands. > > > > Ah, I retract this comment; after actually looking at all the patches, I > > do see that in patch 8/8, Colin does call phylink_generic_validate(). > > Good point, I obviously missed that in the series. Whew... I was getting confused as I was reading this. Double checking that I did indeed add this to the set. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Sep 12, 2022 at 10:16:21AM +0000, Vladimir Oltean wrote: > On Mon, Sep 12, 2022 at 09:48:36AM +0100, Russell King (Oracle) wrote: > > On Sun, Sep 11, 2022 at 01:02:42PM -0700, Colin Foster wrote: > > > phylink_generic_validate() requires that mac_capabilities is correctly > > > populated. While no existing drivers have used phylink_generic_validate(), > > > the ocelot_ext.c driver will. Populate this element so the use of existing > > > functions is possible. > > > > Ocelot always fills in the .phylink_validate method in struct > > dsa_switch_ops, mac_capabilities won't be used as > > phylink_generic_validate() will not be called by > > dsa_port_phylink_validate(). > > Correct, but felix_phylink_validate() _can_ still directly call > phylink_validate(), right? Colin does not have the full support for > ocelot_ext in this patch set, but this is what he intends to do. As you mentioned, I do in fact call phylink_generic_validate() in 8/8. > > > Also "no existing drivers have used phylink_generic_validate()" I > > wonder which drivers you are referring to there. If you are referring > > to DSA drivers, then it is extensively used. The following is from > > Linus' tree as of today: > > By "existing drivers", it is meant felix_vsc9959.c and seville_vsc9953.c, > two drivers in their own right, which use the common felix.c to talk to > (a) DSA and (b) the ocelot switch lib in drivers/net/ethernet/mscc/. > It is true that these existing drivers do not use phylink_generic_validate(). > Furthermore, Colin's new ocelot_ext.c is on the same level as > felix_vsc9959.c and seville_vsc9953.c, will use felix.c in the same way, > and will want to use phylink_generic_validate(). > > > Secondly, I don't see a purpose for this patch in the following > > patches, as Ocelot continues to always fill in .phylink_validate, > > and as I mentioned above, as long as that member is filled in, > > mac_capabilities won't be used unless you explicitly call > > phylink_generic_validate() in your .phylink_validate() callback. > > Yes, explicit calling is what Colin explained that he wants to do. > > > Therefore, I think you can drop this patch from your series and > > you won't see any functional change. > > This is true. I am also a bit surprised at Colin's choices to > (a) not ask the netdev maintainers to pull into net-next the immutable > branch that Lee provided here: > https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/ > and instead send some patches for review which are difficult to > apply directly to any tree As mentioned in the cover letter, I don't expect this to necessarily be ready by the next merge window. But seemingly I misjudged whether merging the net-next and Lee's tree would be more tedious for the netdev maintainers than looking at the RFC for reviewers. I'm trying to create as little hassle for people as I can. Apologies.
On Mon, Sep 12, 2022 at 08:47:47AM -0700, Colin Foster wrote: > > This is true. I am also a bit surprised at Colin's choices to > > (a) not ask the netdev maintainers to pull into net-next the immutable > > branch that Lee provided here: > > https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/ > > and instead send some patches for review which are difficult to > > apply directly to any tree > > As mentioned in the cover letter, I don't expect this to necessarily be > ready by the next merge window. But seemingly I misjudged whether > merging the net-next and Lee's tree would be more tedious for the netdev > maintainers than looking at the RFC for reviewers. I'm trying to create > as little hassle for people as I can. Apologies. What is it exactly that is keeping this patch set from being ready for 6.1? There's still time... It mostly looks ok to me, I'm in the process of reviewing it. You mentioned documentation in the cover letter; I suppose you're talking about dt-schema? If so, I just started off by converting ocelot.txt to mscc,ocelot.yaml, since I know that the conversion process is typically a bit daunting to even start. https://patchwork.kernel.org/project/netdevbpf/patch/20220912153702.246206-1-vladimir.oltean@nxp.com/
On Mon, Sep 12, 2022 at 03:52:35PM +0000, Vladimir Oltean wrote: > On Mon, Sep 12, 2022 at 08:47:47AM -0700, Colin Foster wrote: > > > This is true. I am also a bit surprised at Colin's choices to > > > (a) not ask the netdev maintainers to pull into net-next the immutable > > > branch that Lee provided here: > > > https://lore.kernel.org/lkml/YxrjyHcceLOFlT%2Fc@google.com/ > > > and instead send some patches for review which are difficult to > > > apply directly to any tree > > > > As mentioned in the cover letter, I don't expect this to necessarily be > > ready by the next merge window. But seemingly I misjudged whether > > merging the net-next and Lee's tree would be more tedious for the netdev > > maintainers than looking at the RFC for reviewers. I'm trying to create > > as little hassle for people as I can. Apologies. > > What is it exactly that is keeping this patch set from being ready for 6.1? > There's still time... > > It mostly looks ok to me, I'm in the process of reviewing it. You > mentioned documentation in the cover letter; I suppose you're talking > about dt-schema? If so, I just started off by converting ocelot.txt to > mscc,ocelot.yaml, since I know that the conversion process is typically > a bit daunting to even start. > https://patchwork.kernel.org/project/netdevbpf/patch/20220912153702.246206-1-vladimir.oltean@nxp.com/ Yes - checkpatch correclty gave warnings about mscc,vsc7512-ext-switch being undocumented. Thanks for that patch - I just saw it! I'll wait for your review before I get optimistic. But if it boils down to separating the last patch (per Lee's suggestion) and adding the dt-bindings, maybe it could be ready in another round or two.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 95a5c5d0815c..201bf3bdd67d 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -958,6 +958,9 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, __set_bit(ocelot->ports[port]->phy_mode, config->supported_interfaces); + + config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 | + MAC_100 | MAC_1000FD | MAC_2500FD; } static void felix_phylink_validate(struct dsa_switch *ds, int port,
phylink_generic_validate() requires that mac_capabilities is correctly populated. While no existing drivers have used phylink_generic_validate(), the ocelot_ext.c driver will. Populate this element so the use of existing functions is possible. Signed-off-by: Colin Foster <colin.foster@in-advantage.com> --- v1 from previous RFC: * New patch --- drivers/net/dsa/ocelot/felix.c | 3 +++ 1 file changed, 3 insertions(+)