| Message ID | 20211011165517.2857893-1-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/2] net: macb: Clean up macb_validate | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | WARNING: labels should not be indented |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 25 this patch: 25 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
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
Quoting Antoine Tenart (2021-10-12 10:33:04) > 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). Patch 2 *
On 12/10/2021 at 10:33, Antoine Tenart wrote: > 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. 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 >
On 10/12/21 4:33 AM, Antoine Tenart wrote: > 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. 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
Quoting Sean Anderson (2021-10-12 18:34:50) > On 10/12/21 4:33 AM, Antoine Tenart wrote: > > 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. > > > 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
On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote: > 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). 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?
On 10/14/21 12:34 PM, Russell King (Oracle) wrote: > On Tue, Oct 12, 2021 at 10:33:04AM +0200, Antoine Tenart wrote: >> 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). > > 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
On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote: > On 10/14/21 12:34 PM, Russell King (Oracle) wrote: > > 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.
On 10/14/21 7:08 PM, Russell King (Oracle) wrote: > On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote: >> On 10/14/21 12:34 PM, Russell King (Oracle) wrote: >> > 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. 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
On Fri, Oct 15, 2021 at 06:28:10PM -0400, Sean Anderson wrote: > On 10/14/21 7:08 PM, Russell King (Oracle) wrote: > > On Thu, Oct 14, 2021 at 01:50:36PM -0400, Sean Anderson wrote: > > > On 10/14/21 12:34 PM, Russell King (Oracle) wrote: > > > > 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. > > 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.
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.
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: >> 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 diff --git a/include/linux/phylink.h b/include/linux/phylink.h index bc4b866cd99b..a911872c12d8 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -134,11 +134,14 @@ struct phylink_mac_ops { * based on @state->advertising and/or @state->speed and update * @state->interface accordingly. See phylink_helper_basex_speed(). * - * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the - * MAC driver to return all supported link modes. + * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink expects the MAC + * driver to return all supported link modes. If @config->supported_interfaces + * is populated, phylink will handle this, and it is not necessary for + * validate() to support %PHY_INTERFACE_MODE_NA. * - * If the @state->interface mode is not supported, then the @supported - * mask must be cleared. + * If the @state->interface mode is not supported, then the @supported mask + * must be cleared. If @config->supported_interfaces is populated, validate() + * will only be called with values of @state->interfaces present in the bitmap. */ void validate(struct phylink_config *config, unsigned long *supported, struct phylink_link_state *state); -- 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 diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index c1f976a79a44..02eff23adcfb 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -880,6 +880,7 @@ static void macb_get_pcs_fixed_state(struct phylink_config *config, static int macb_mii_probe(struct net_device *dev) { struct macb *bp = netdev_priv(dev); + unsigned long *supported = bp->phylink_config.supported_interfaces; bp->phylink_config.dev = &dev->dev; bp->phylink_config.type = PHYLINK_NETDEV; @@ -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
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.
On 10/25/21 6:35 AM, Russell King (Oracle) wrote: > 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. 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
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 683f14665c2c..a9105ec1b885 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -510,32 +510,55 @@ static void macb_validate(struct phylink_config *config, unsigned long *supported, struct phylink_link_state *state) { + bool one = state->interface != PHY_INTERFACE_MODE_NA; struct net_device *ndev = to_net_dev(config->dev); __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; struct macb *bp = netdev_priv(ndev); - /* We only support MII, RMII, GMII, RGMII & SGMII. */ - if (state->interface != PHY_INTERFACE_MODE_NA && - state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_RMII && - state->interface != PHY_INTERFACE_MODE_GMII && - state->interface != PHY_INTERFACE_MODE_SGMII && - state->interface != PHY_INTERFACE_MODE_10GBASER && - !phy_interface_mode_is_rgmii(state->interface)) { - bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); - return; - } - - if (!macb_is_gem(bp) && - (state->interface == PHY_INTERFACE_MODE_GMII || - phy_interface_mode_is_rgmii(state->interface))) { - bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); - return; - } - - if (state->interface == PHY_INTERFACE_MODE_10GBASER && - !(bp->caps & MACB_CAPS_HIGH_SPEED && - bp->caps & MACB_CAPS_PCS)) { + switch (state->interface) { + case PHY_INTERFACE_MODE_NA: + case PHY_INTERFACE_MODE_10GBASER: + if (bp->caps & MACB_CAPS_HIGH_SPEED && + bp->caps & MACB_CAPS_PCS && + bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { + phylink_set_10g_modes(mask); + phylink_set(mask, 10000baseKR_Full); + } else if (one) { + goto none; + } + if (one) + break; + fallthrough; + case PHY_INTERFACE_MODE_GMII: + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_TXID: + if (!macb_is_gem(bp)) { + if (one) + goto none; + else + goto mii; + } + fallthrough; + case PHY_INTERFACE_MODE_SGMII: + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) { + phylink_set(mask, 1000baseT_Full); + phylink_set(mask, 1000baseX_Full); + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) + phylink_set(mask, 1000baseT_Half); + } + fallthrough; + mii: + case PHY_INTERFACE_MODE_MII: + case PHY_INTERFACE_MODE_RMII: + phylink_set(mask, 10baseT_Half); + phylink_set(mask, 10baseT_Full); + phylink_set(mask, 100baseT_Half); + phylink_set(mask, 100baseT_Full); + break; + none: + default: bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); return; } @@ -543,33 +566,6 @@ static void macb_validate(struct phylink_config *config, phylink_set_port_modes(mask); phylink_set(mask, Autoneg); phylink_set(mask, Asym_Pause); - - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && - (state->interface == PHY_INTERFACE_MODE_NA || - state->interface == PHY_INTERFACE_MODE_10GBASER)) { - phylink_set_10g_modes(mask); - phylink_set(mask, 10000baseKR_Full); - if (state->interface != PHY_INTERFACE_MODE_NA) - goto out; - } - - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 100baseT_Full); - - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && - (state->interface == PHY_INTERFACE_MODE_NA || - state->interface == PHY_INTERFACE_MODE_GMII || - state->interface == PHY_INTERFACE_MODE_SGMII || - phy_interface_mode_is_rgmii(state->interface))) { - phylink_set(mask, 1000baseT_Full); - phylink_set(mask, 1000baseX_Full); - - if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF)) - phylink_set(mask, 1000baseT_Half); - } -out: bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS); bitmap_and(state->advertising, state->advertising, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
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 <sean.anderson@seco.com> --- 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(-)