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 <[email protected]>
---
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 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 <[email protected]>
> ---
>
> 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
>
>
--
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 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%[email protected]/
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.
--
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 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 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%[email protected]/
> > 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/[email protected]/
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%[email protected]/
> 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 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%[email protected]/
> > > 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/[email protected]/
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.