From: Sean Anderson <hidden> Date: 2021-10-11 16:55:46
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
Signed-off-by: Sean Anderson <redacted>
---
This patch was originally submitted as [1].
[1] https://lore.kernel.org/netdev/20211004191527.1610759-9-sean.anderson@seco.com/
Changes in v2:
- Fix polarity of `one` being inverted
- Only set gigabit modes for NA if macb_is_gem()
drivers/net/ethernet/cadence/macb_main.c | 94 ++++++++++++------------
1 file changed, 45 insertions(+), 49 deletions(-)
From: Sean Anderson <hidden> Date: 2021-10-11 16:55:47
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well. This also simplifies the logic now that all gigabit modes depend
on the mac being a GEM.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Sean Anderson <redacted>
---
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-12 00:18:02
On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well. This also simplifies the logic now that all gigabit modes depend
on the mac being a GEM.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Sean Anderson <redacted>
Please make sure you CC the author of the original patch, they tend to
be a good person to review the change. Or at the very least validate
the Fixes tag. Adding Antoine.
From: Antoine Tenart <atenart@kernel.org> Date: 2021-10-12 08:27:09
Hello Sean,
Quoting Jakub Kicinski (2021-10-12 02:17:59)
On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
quoted
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well. This also simplifies the logic now that all gigabit modes depend
on the mac being a GEM.
This looks correct.
quoted
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
If this is a fix, the patch has to be first in the series as it is
depending on the first one which is not a fix.
Thanks,
Antoine
From: Antoine Tenart <atenart@kernel.org> Date: 2021-10-12 08:33:16
Hello Sean,
Quoting Sean Anderson (2021-10-11 18:55:16)
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
(Also having patch 1 first will improve things).
Thanks,
Antoine
From: Antoine Tenart <atenart@kernel.org> Date: 2021-10-12 08:34:40
Quoting Antoine Tenart (2021-10-12 10:33:04)
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
(Also having patch 1 first will improve things).
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2021-10-12 09:24:58
On 12/10/2021 at 10:33, Antoine Tenart wrote:
Hello Sean,
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
I agree with Antoine that the result is not much more readable.
Regards,
Nicolas
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
(Also having patch 1 first will improve things).
Thanks,
Antoine
From: Sean Anderson <hidden> Date: 2021-10-12 16:20:56
On 10/12/21 4:27 AM, Antoine Tenart wrote:
Hello Sean,
Quoting Jakub Kicinski (2021-10-12 02:17:59)
quoted
On Mon, 11 Oct 2021 12:55:17 -0400 Sean Anderson wrote:
quoted
This aligns mac_validate with mac_config. In mac_config, SGMII is only
enabled if macb_is_gem. Validate should care if the mac is a gem as
well. This also simplifies the logic now that all gigabit modes depend
on the mac being a GEM.
This looks correct.
quoted
quoted
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
If this is a fix, the patch has to be first in the series as it is
depending on the first one which is not a fix.
From: Sean Anderson <hidden> Date: 2021-10-12 16:35:01
On 10/12/21 4:33 AM, Antoine Tenart wrote:
Hello Sean,
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
The conditions are necessary to determine if the mac actually supports
the mode being requested. The jumps are all forward, and all but one
could be replaced with
bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
return;
The idea being that the NA mode goes from top to bottom, and all the
other modes do as well.
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
This is actually the issue I wanted to address. The interface checks are
effectively performed twice or sometimes three times. There are also
gotos in the original design to deal with e.g. 10GBASE not having
10/100/1000 modes. This makes it easy to introduce bugs when adding new
modes, such as what happened with SGMII.
(Also having patch 1 first will improve things).
Yes. Some of the complexity is simply to deal with SGMII being a special
case.
--Sean
From: Antoine Tenart <atenart@kernel.org> Date: 2021-10-12 16:53:59
Quoting Sean Anderson (2021-10-12 18:34:50)
On 10/12/21 4:33 AM, Antoine Tenart wrote:
quoted
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
quoted
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
This is actually the issue I wanted to address. The interface checks are
effectively performed twice or sometimes three times. There are also
gotos in the original design to deal with e.g. 10GBASE not having
10/100/1000 modes. This makes it easy to introduce bugs when adding new
modes, such as what happened with SGMII.
I don't think having 1) validity checks 2) availability checks is an
issue. It's a choice between having possible bugs because the two steps
aren't synced vs possible bugs because one of the multiple paths in the
switch gets slightly broken by a patch. IMHO the one easier to read and
follow should win here.
Antoine
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-14 16:34:40
On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:
Hello Sean,
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
Some of this could be improved if we add the ability for a MAC to
specify the phy_interface_t modes that it supports as a bitmap
before calling phylink_create() - then we can have phylink check
that the mode is supported itself prior to calling the validate
handler.
You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue
and we could add to phylink_validate():
if (!phy_interface_empty(pl->config->supported_interfaces) &&
!test_bit(state->interface, pl->config->supported_interfaces))
return -EINVAL;
which should go a long way to simplifying a lot of these validation
implementations.
Any thoughts on that?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Sean Anderson <hidden> Date: 2021-10-14 17:50:46
On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote:
quoted
Hello Sean,
Quoting Sean Anderson (2021-10-11 18:55:16)
quoted
As the number of interfaces grows, the number of if statements grows
ever more unweildy. Clean everything up a bit by using a switch
statement. No functional change intended.
I'm not 100% convinced this makes macb_validate more readable: there are
lots of conditions, and jumps, in the switch.
Maybe you could try a mixed approach; keeping the invalid modes checks
(bitmap_zero) at the beginning and once we know the mode is valid using
a switch statement. That might make it easier to read as this should
remove lots of conditionals. (We'll still have the one/_NA checks
though).
Some of this could be improved if we add the ability for a MAC to
specify the phy_interface_t modes that it supports as a bitmap
before calling phylink_create() - then we can have phylink check
that the mode is supported itself prior to calling the validate
handler.
You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue
and we could add to phylink_validate():
if (!phy_interface_empty(pl->config->supported_interfaces) &&
!test_bit(state->interface, pl->config->supported_interfaces))
return -EINVAL;
which should go a long way to simplifying a lot of these validation
implementations.
Any thoughts on that?
IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like
if (state->interface == PHY_INTERFACE_MODE_FOO) {
if (we_support_foo())
phylink_set(mask, Foo);
else if (state->interface != PHY_INTERFACE_MODE_NA) {
linkmode_zero(supported);
return;
}
}
which gets even worse when we want to have different interfaces share
logic. IMO validate() could be much cleaner if we never called it with
NA and instead did something like
if (state->interface == PHY_INTERFACE_MODE_NA) {
unsigned long *original;
linkmode_copy(original, supported);
for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (test_bit(i, pl->config->supported_interfaces)) {
unsigned long *iface_mode;
linkmode_copy(iface_mode, original);
state->interface = i;
pl->mac_ops->validate(pl->config, iface_mode, state);
linkmode_or(supported, supported, iface_mode);
}
}
state->interface = PHY_INTERFACE_MODE_NA;
}
This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.
--Sean
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-14 23:08:38
On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
quoted
You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue
and we could add to phylink_validate():
if (!phy_interface_empty(pl->config->supported_interfaces) &&
!test_bit(state->interface, pl->config->supported_interfaces))
return -EINVAL;
which should go a long way to simplifying a lot of these validation
implementations.
Any thoughts on that?
IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like
if (state->interface == PHY_INTERFACE_MODE_FOO) {
if (we_support_foo())
phylink_set(mask, Foo);
else if (state->interface != PHY_INTERFACE_MODE_NA) {
linkmode_zero(supported);
return;
}
}
which gets even worse when we want to have different interfaces share
logic.
There is always the option to use different operations structs if the
properties of the interfaces can be divided up in that way - and that
will probably be more efficient (not that the validate callback is a
performance critical path though.)
IMO validate() could be much cleaner if we never called it with
NA and instead did something like
if (state->interface == PHY_INTERFACE_MODE_NA) {
unsigned long *original;
linkmode_copy(original, supported);
for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (test_bit(i, pl->config->supported_interfaces)) {
unsigned long *iface_mode;
linkmode_copy(iface_mode, original);
state->interface = i;
pl->mac_ops->validate(pl->config, iface_mode, state);
linkmode_or(supported, supported, iface_mode);
}
}
state->interface = PHY_INTERFACE_MODE_NA;
}
This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.
In addition I think - I think we should use a non-empty
supported_interfaces as an indicator that we use the above, otherwise
we have to loop through all possible interface modes. That also
provides some encouragement to fill out the supported_interfaces
member.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Sean Anderson <hidden> Date: 2021-10-15 22:28:18
On 10/14/21 7:08 PM, Russell King (Oracle) wrote:
On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
quoted
On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
quoted
You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue
and we could add to phylink_validate():
if (!phy_interface_empty(pl->config->supported_interfaces) &&
!test_bit(state->interface, pl->config->supported_interfaces))
return -EINVAL;
which should go a long way to simplifying a lot of these validation
implementations.
Any thoughts on that?
IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like
if (state->interface == PHY_INTERFACE_MODE_FOO) {
if (we_support_foo())
phylink_set(mask, Foo);
else if (state->interface != PHY_INTERFACE_MODE_NA) {
linkmode_zero(supported);
return;
}
}
which gets even worse when we want to have different interfaces share
logic.
There is always the option to use different operations structs if the
properties of the interfaces can be divided up in that way - and that
will probably be more efficient (not that the validate callback is a
performance critical path though.)
quoted
IMO validate() could be much cleaner if we never called it with
NA and instead did something like
if (state->interface == PHY_INTERFACE_MODE_NA) {
unsigned long *original;
linkmode_copy(original, supported);
for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (test_bit(i, pl->config->supported_interfaces)) {
unsigned long *iface_mode;
linkmode_copy(iface_mode, original);
state->interface = i;
pl->mac_ops->validate(pl->config, iface_mode, state);
linkmode_or(supported, supported, iface_mode);
}
}
state->interface = PHY_INTERFACE_MODE_NA;
}
This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.
In addition I think - I think we should use a non-empty
supported_interfaces as an indicator that we use the above, otherwise
we have to loop through all possible interface modes. That also
provides some encouragement to fill out the supported_interfaces
member.
I had a stab at this today [1], but it is only compile-tested. In order
to compile "net: phylink: use phy_interface_t bitmaps for optical
modules", I needed to run
sed -ie 's/sfp_link_an_mode/cfg_link_an_mode/g' drivers/net/phy/phylink.c
Do you plan on making up a series for this? I think the end result is
much nicer that v3 of this series.
--Sean
[1] https://github.com/sean-anderson-seco/linux/commits/supported_interfaces_wip
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-15 22:47:47
On Fri, Oct 15, 2021 at 06:28:10PM -0400, Sean Anderson wrote:
On 10/14/21 7:08 PM, Russell King (Oracle) wrote:
quoted
On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote:
quoted
On 10/14/21 12:34 PM, Russell King (Oracle) wrote:
quoted
You can find some patches that add the "supported_interfaces" masks
in git.armlinux.org.uk/linux-arm.git net-queue
and we could add to phylink_validate():
if (!phy_interface_empty(pl->config->supported_interfaces) &&
!test_bit(state->interface, pl->config->supported_interfaces))
return -EINVAL;
which should go a long way to simplifying a lot of these validation
implementations.
Any thoughts on that?
IMO the actual issue here is PHY_INTERFACE_MODE_NA. Supporting this
tends to add complexity to validate(), because we have a lot of code
like
if (state->interface == PHY_INTERFACE_MODE_FOO) {
if (we_support_foo())
phylink_set(mask, Foo);
else if (state->interface != PHY_INTERFACE_MODE_NA) {
linkmode_zero(supported);
return;
}
}
which gets even worse when we want to have different interfaces share
logic.
There is always the option to use different operations structs if the
properties of the interfaces can be divided up in that way - and that
will probably be more efficient (not that the validate callback is a
performance critical path though.)
quoted
IMO validate() could be much cleaner if we never called it with
NA and instead did something like
if (state->interface == PHY_INTERFACE_MODE_NA) {
unsigned long *original;
linkmode_copy(original, supported);
for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
if (test_bit(i, pl->config->supported_interfaces)) {
unsigned long *iface_mode;
linkmode_copy(iface_mode, original);
state->interface = i;
pl->mac_ops->validate(pl->config, iface_mode, state);
linkmode_or(supported, supported, iface_mode);
}
}
state->interface = PHY_INTERFACE_MODE_NA;
}
This of course can be done in addition to/instead of your above
suggestion. I suggested something like this in v3 of this series, but it
would be even better to do this on the phylink level.
In addition I think - I think we should use a non-empty
supported_interfaces as an indicator that we use the above, otherwise
we have to loop through all possible interface modes. That also
provides some encouragement to fill out the supported_interfaces
member.
I had a stab at this today [1], but it is only compile-tested. In order
to compile "net: phylink: use phy_interface_t bitmaps for optical
modules", I needed to run
sed -ie 's/sfp_link_an_mode/cfg_link_an_mode/g' drivers/net/phy/phylink.c
Do you plan on making up a series for this? I think the end result is
much nicer that v3 of this series.
I have been working on it but haven't finished the patches yet. There's
a few issues that came up with e.g. DSA and mvneta being able to switch
between different speeds with some SFP modules that have needed other
tweaks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-19 15:02:30
On Fri, Oct 15, 2021 at 11:47:30PM +0100, Russell King (Oracle) wrote:
I have been working on it but haven't finished the patches yet. There's
a few issues that came up with e.g. DSA and mvneta being able to switch
between different speeds with some SFP modules that have needed other
tweaks.
Okay, have a look at:
http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
and the patches from "net: enetc: remove interface checks in
enetc_pl_mac_validate ()" down to the "net-merged" branch label.
That set of patches add the supported_interfaces bitmap, uses it for
validation purposes, converts all but one of the ethernet drivers
over to using it, and then simplifies the validate() implementations.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Sean Anderson <hidden> Date: 2021-10-22 17:37:45
Hi Russell,
On 10/19/21 11:02 AM, Russell King (Oracle) wrote:
On Fri, Oct 15, 2021 at 11:47:30PM +0100, Russell King (Oracle) wrote:
quoted
I have been working on it but haven't finished the patches yet. There's
a few issues that came up with e.g. DSA and mvneta being able to switch
between different speeds with some SFP modules that have needed other
tweaks.
Okay, have a look at:
http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
and the patches from "net: enetc: remove interface checks in
enetc_pl_mac_validate ()" down to the "net-merged" branch label.
That set of patches add the supported_interfaces bitmap, uses it for
validation purposes, converts all but one of the ethernet drivers
over to using it, and then simplifies the validate() implementations.
For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.
For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.
(I think you have fixed this in your latest revision)
"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like
I think "net: macb: populate supported_interfaces member" is wrong.
Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE. I know you
leave the check in validate(), but this is the sort of thing which
should be put in supported interfaces. Additionally, SGMII should
depend on PCS. So something like
@@ -889,6 +890,21 @@ static int macb_mii_probe(struct net_device *dev)bp->phylink_config.get_fixed_state=macb_get_pcs_fixed_state;}+if(bp->caps&MACB_CAPS_HIGH_SPEED&&+bp->caps&MACB_CAPS_PCS)+__set_bit(PHY_INTERFACE_MODE_10GBASER,supported);+if(macb_is_gem(bp)&&bp->caps&MACB_CAPS_GIGABIT_MODE_AVAILABLE){+__set_bit(PHY_INTERFACE_MODE_GMII,supported);+phy_interface_set_rgmii(supported);+if(bp->caps&MACB_CAPS_PCS)+__set_bit(PHY_INTERFACE_MODE_SGMII,supported);+}+__set_bit(PHY_INTERFACE_MODE_MII,supported);+__set_bit(PHY_INTERFACE_MODE_RMII,supported);+bp->phylink=phylink_create(&bp->phylink_config,bp->pdev->dev.fwnode,bp->phy_interface,&macb_phylink_ops);if(IS_ERR(bp->phylink)){--
Other than that, the commits in the range you mentioned above looks good
to me. For reference, my working branch with the above changes applied is [1].
[1] https://github.com/sean-anderson-seco/linux/tree/rking
--Sean
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-25 10:35:18
On Fri, Oct 22, 2021 at 01:37:34PM -0400, Sean Anderson wrote:
Hi Russell,
For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.
I think you actually mean phy_interface_or(). Given that we will need
MAC drivers to provide the union of their PCS support, I think that
would be a sensible addition. Thanks.
For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.
(I think you have fixed this in your latest revision)
I haven't - but when I move the patch series onto net-next, that will
need to be updated.
"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like
Yes, I haven't bothered with the doc updates yet... they will need to
be done before the patches are ready. Thanks for the suggestions though.
I think "net: macb: populate supported_interfaces member" is wrong.
Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE.
It is a conversion of what macb_validate() does - if the conversion is
incorrect, then macb_validate() is incorrect.
If MACB_CAPS_GIGABIT_MODE_AVAILABLE isn't set, but MACB_CAPS_HIGH_SPEED
and MACB_CAPS_PCS are both set, macb_validate() will not zero the
supported mask if e.g. PHY_INTERFACE_MODE_10GBASER is requested - it
will indicate 10baseT and 100baseT speeds are supported. So the current
macb_validate() code basically tells phylink that
PHY_INTERFACE_MODE_10GBASER supports 10baseT and 100baseT speeds!
This probably is not what is intended, but this is what the code does,
and I'm maintaining bug-compatibility with the current macb_validate()
implementation. Any changes to the behaviour should be a separate
patch - either fixing it before this patch, or fixing it afterwards.
As the series is currently based on v5.14, it may be that this has
already been fixed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Sean Anderson <hidden> Date: 2021-10-25 15:26:37
On 10/25/21 6:35 AM, Russell King (Oracle) wrote:
On Fri, Oct 22, 2021 at 01:37:34PM -0400, Sean Anderson wrote:
quoted
Hi Russell,
For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.
I think you actually mean phy_interface_or(). Given that we will need
MAC drivers to provide the union of their PCS support, I think that
would be a sensible addition. Thanks.
quoted
For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.
(I think you have fixed this in your latest revision)
I haven't - but when I move the patch series onto net-next, that will
need to be updated.
quoted
"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like
Yes, I haven't bothered with the doc updates yet... they will need to
be done before the patches are ready. Thanks for the suggestions though.
quoted
I think "net: macb: populate supported_interfaces member" is wrong.
Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE.
It is a conversion of what macb_validate() does - if the conversion is
incorrect, then macb_validate() is incorrect.
If MACB_CAPS_GIGABIT_MODE_AVAILABLE isn't set, but MACB_CAPS_HIGH_SPEED
and MACB_CAPS_PCS are both set, macb_validate() will not zero the
supported mask if e.g. PHY_INTERFACE_MODE_10GBASER is requested - it
will indicate 10baseT and 100baseT speeds are supported. So the current
macb_validate() code basically tells phylink that
PHY_INTERFACE_MODE_10GBASER supports 10baseT and 100baseT speeds!
This probably is not what is intended, but this is what the code does,
and I'm maintaining bug-compatibility with the current macb_validate()
implementation. Any changes to the behaviour should be a separate
patch - either fixing it before this patch, or fixing it afterwards.
As the series is currently based on v5.14, it may be that this has
already been fixed.
Ugh. This sort of thing is what I wanted to address in the first place.
The current logic lends itself well to these sorts of errors.
--Sean