| Message ID | 20220714010021.1786616-1-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next:] net: dsa: mv88e6xxx: fix speed setting for CPU/DSA ports | expand |
On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > stopped relying on SPEED_MAX constant and hardcoded speed settings > for the switch ports and rely on phylink configuration. > > It turned out, however, that when the relevant code is called, > the mac_capabilites of CPU/DSA port remain unset. > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > dsa_tree_setup_switches(), which precedes setting the caps in > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > port is 10M at the start. To fix that execute phylink_get_caps() callback > which fills port's mac_capabilities before they are processed. > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > Signed-off-by: Marcin Wojtas <mw@semihalf.com> Please don't merge this - the plan is to submit the RFC series I sent on Wednesday which deletes this code, and I'd rather not re-spin the series and go through the testing again because someone else changed the code. Marcin - please can you test with my RFC series, which can be found at: https://lore.kernel.org/all/Ys7RdzGgHbYiPyB1@shell.armlinux.org.uk/ Thanks.
Hi Russell, czw., 14 lip 2022 o 14:32 Russell King (Oracle) <linux@armlinux.org.uk> napisał(a): > > On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > stopped relying on SPEED_MAX constant and hardcoded speed settings > > for the switch ports and rely on phylink configuration. > > > > It turned out, however, that when the relevant code is called, > > the mac_capabilites of CPU/DSA port remain unset. > > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > > dsa_tree_setup_switches(), which precedes setting the caps in > > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > > port is 10M at the start. To fix that execute phylink_get_caps() callback > > which fills port's mac_capabilities before they are processed. > > > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > Please don't merge this - the plan is to submit the RFC series I sent on > Wednesday which deletes this code, and I'd rather not re-spin the series > and go through the testing again because someone else changed the code. Thank for the heads-up. Are you planning to resend the series or willing to get it merged as-is? I have perhaps one comment, but I can apply it later as a part of fwnode_/device_ migration. > > Marcin - please can you test with my RFC series, which can be found at: > > https://lore.kernel.org/all/Ys7RdzGgHbYiPyB1@shell.armlinux.org.uk/ > The thing is my v2 of DSA fwnode_/device_ migration is tested and ready to send. There will be conflicts (rather easy) with your patchset - I volunteer to resolve it this way or another, depending on what lands first. I have 2 platforms to test it with + also ACPI case locally. I'd like to make things as smooth as possible and make it before the upcoming merge window - please share your thoughts on this. Best regards, Marcin
On Thu, Jul 14, 2022 at 07:18:57PM +0200, Marcin Wojtas wrote: > Hi Russell, > > czw., 14 lip 2022 o 14:32 Russell King (Oracle) > <linux@armlinux.org.uk> napisał(a): > > > > On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > > > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > > stopped relying on SPEED_MAX constant and hardcoded speed settings > > > for the switch ports and rely on phylink configuration. > > > > > > It turned out, however, that when the relevant code is called, > > > the mac_capabilites of CPU/DSA port remain unset. > > > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > > > dsa_tree_setup_switches(), which precedes setting the caps in > > > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > > > > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > > > port is 10M at the start. To fix that execute phylink_get_caps() callback > > > which fills port's mac_capabilities before they are processed. > > > > > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > > > Please don't merge this - the plan is to submit the RFC series I sent on > > Wednesday which deletes this code, and I'd rather not re-spin the series > > and go through the testing again because someone else changed the code. > > Thank for the heads-up. Are you planning to resend the series or > willing to get it merged as-is? I have perhaps one comment, but I can > apply it later as a part of fwnode_/device_ migration. > > > > > Marcin - please can you test with my RFC series, which can be found at: > > > > https://lore.kernel.org/all/Ys7RdzGgHbYiPyB1@shell.armlinux.org.uk/ > > > > The thing is my v2 of DSA fwnode_/device_ migration is tested and > ready to send. There will be conflicts (rather easy) with your > patchset - I volunteer to resolve it this way or another, depending on > what lands first. I have 2 platforms to test it with + also ACPI case > locally. > > I'd like to make things as smooth as possible and make it before the > upcoming merge window - please share your thoughts on this. I've also been trying to get the mv88e6xxx PCS conversion in, but it's been held up because there's a fundamental problem in DSA that this series is addressing. This series is addressing a faux pas on my part, where I had forgotten that phylink doesn't get used in DSA unless firmware specifies a fixed-link (or a PHY) - in other words when the firmware lacks a description of the link. So, what do we do...
Hi Russell, czw., 14 lip 2022 o 21:44 Russell King (Oracle) <linux@armlinux.org.uk> napisał(a): > > On Thu, Jul 14, 2022 at 07:18:57PM +0200, Marcin Wojtas wrote: > > Hi Russell, > > > > czw., 14 lip 2022 o 14:32 Russell King (Oracle) > > <linux@armlinux.org.uk> napisał(a): > > > > > > On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > > > > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > > > stopped relying on SPEED_MAX constant and hardcoded speed settings > > > > for the switch ports and rely on phylink configuration. > > > > > > > > It turned out, however, that when the relevant code is called, > > > > the mac_capabilites of CPU/DSA port remain unset. > > > > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > > > > dsa_tree_setup_switches(), which precedes setting the caps in > > > > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > > > > > > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > > > > port is 10M at the start. To fix that execute phylink_get_caps() callback > > > > which fills port's mac_capabilities before they are processed. > > > > > > > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > > > > > Please don't merge this - the plan is to submit the RFC series I sent on > > > Wednesday which deletes this code, and I'd rather not re-spin the series > > > and go through the testing again because someone else changed the code. > > > > Thank for the heads-up. Are you planning to resend the series or > > willing to get it merged as-is? I have perhaps one comment, but I can > > apply it later as a part of fwnode_/device_ migration. > > > > > > > > Marcin - please can you test with my RFC series, which can be found at: > > > > > > https://lore.kernel.org/all/Ys7RdzGgHbYiPyB1@shell.armlinux.org.uk/ > > > > > > > The thing is my v2 of DSA fwnode_/device_ migration is tested and > > ready to send. There will be conflicts (rather easy) with your > > patchset - I volunteer to resolve it this way or another, depending on > > what lands first. I have 2 platforms to test it with + also ACPI case > > locally. > > > > I'd like to make things as smooth as possible and make it before the > > upcoming merge window - please share your thoughts on this. > > I've also been trying to get the mv88e6xxx PCS conversion in, but > it's been held up because there's a fundamental problem in DSA that > this series is addressing. > > This series is addressing a faux pas on my part, where I had forgotten > that phylink doesn't get used in DSA unless firmware specifies a > fixed-link (or a PHY) - in other words when the firmware lacks a > description of the link. > > So, what do we do... > Ok, thanks. I tested your patchset in 2 setups with multiple combinations - all worked fine, so this patch can be abandoned. I already rebased my series on top of yours, so I'll submit my v2 this way. Best regards, Marcin
Hi Marcin, On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > stopped relying on SPEED_MAX constant and hardcoded speed settings > for the switch ports and rely on phylink configuration. > > It turned out, however, that when the relevant code is called, > the mac_capabilites of CPU/DSA port remain unset. > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > dsa_tree_setup_switches(), which precedes setting the caps in > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > port is 10M at the start. To fix that execute phylink_get_caps() callback > which fills port's mac_capabilities before they are processed. > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 37b649501500..9fab76f256bb 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -3293,7 +3293,12 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > * port and all DSA ports to their maximum bandwidth and full duplex. > */ > if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) { > - unsigned long caps = dp->pl_config.mac_capabilities; > + unsigned long caps; > + > + if (ds->ops->phylink_get_caps) > + ds->ops->phylink_get_caps(ds, port, &dp->pl_config); > + > + caps = dp->pl_config.mac_capabilities; We'll need this bug fixed in net-next one way or another. If you resend this patch, please change the following: (1) it's silly to (a) check for the presence of ds->ops->phylink_get_caps and (b) do an indirect function call when you know that the implementation is mv88e6xxx_get_caps(). So just call that. (2) please don't touch &dp->pl_config, just create an on-stack struct phylink_config pl_config, and let DSA do its thing with &dp->pl_config whenever the timing of dsa_port_phylink_create() is. > > if (chip->info->ops->port_max_speed_mode) > mode = chip->info->ops->port_max_speed_mode(port); > -- > 2.29.0 >
Hi Vladimir, pon., 25 lip 2022 o 01:38 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > Hi Marcin, > > On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote: > > Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > stopped relying on SPEED_MAX constant and hardcoded speed settings > > for the switch ports and rely on phylink configuration. > > > > It turned out, however, that when the relevant code is called, > > the mac_capabilites of CPU/DSA port remain unset. > > mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in > > dsa_tree_setup_switches(), which precedes setting the caps in > > phylink_get_caps down in the chain of dsa_tree_setup_ports(). > > > > As a result the mac_capabilites are 0 and the default speed for CPU/DSA > > port is 10M at the start. To fix that execute phylink_get_caps() callback > > which fills port's mac_capabilities before they are processed. > > > > Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > --- > > drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > > index 37b649501500..9fab76f256bb 100644 > > --- a/drivers/net/dsa/mv88e6xxx/chip.c > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > @@ -3293,7 +3293,12 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > > * port and all DSA ports to their maximum bandwidth and full duplex. > > */ > > if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) { > > - unsigned long caps = dp->pl_config.mac_capabilities; > > + unsigned long caps; > > + > > + if (ds->ops->phylink_get_caps) > > + ds->ops->phylink_get_caps(ds, port, &dp->pl_config); > > + > > + caps = dp->pl_config.mac_capabilities; > > We'll need this bug fixed in net-next one way or another. > If you resend this patch, please change the following: > > (1) it's silly to (a) check for the presence of ds->ops->phylink_get_caps and > (b) do an indirect function call when you know that the implementation is > mv88e6xxx_get_caps(). So just call that. > > (2) please don't touch &dp->pl_config, just create an on-stack > struct phylink_config pl_config, and let DSA do its thing with > &dp->pl_config whenever the timing of dsa_port_phylink_create() is. > I can of course apply both suggestions, however, I am wondering if I should resend them at all, as Russell's series is still being discussed. IMO it may be worth waiting whether it makes it before the merge window - if not, I can resend this patch after v5.20-rc1, targetting the net branch. What do you think? Thanks, Marcin > > > > if (chip->info->ops->port_max_speed_mode) > > mode = chip->info->ops->port_max_speed_mode(port); > > -- > > 2.29.0 > >
On Mon, Jul 25, 2022 at 02:18:45AM +0200, Marcin Wojtas wrote: > I can of course apply both suggestions, however, I am wondering if I > should resend them at all, as Russell's series is still being > discussed. IMO it may be worth waiting whether it makes it before the > merge window - if not, I can resend this patch after v5.20-rc1, > targetting the net branch. What do you think? I just don't want a fix for a known regression to slip through the cracks. You can resend whenever you consider, but I believe that if you do so now (today or in the following few days), you won't conflict with anybody's work, considering that this has been said: On Fri, Jul 15, 2022 at 11:57:20PM +0100, Russell King (Oracle) wrote: > Well, at this point, I'm just going to give up with this kernel cycle. > It seems impossible to get this sorted. It seems impossible to move > forward with the conversion of Marvell DSA to phylink_pcs.
On Mon, Jul 25, 2022 at 03:21:44PM +0300, Vladimir Oltean wrote: > On Mon, Jul 25, 2022 at 02:18:45AM +0200, Marcin Wojtas wrote: > > I can of course apply both suggestions, however, I am wondering if I > > should resend them at all, as Russell's series is still being > > discussed. IMO it may be worth waiting whether it makes it before the > > merge window - if not, I can resend this patch after v5.20-rc1, > > targetting the net branch. What do you think? > > I just don't want a fix for a known regression to slip through the cracks. > You can resend whenever you consider, but I believe that if you do so now > (today or in the following few days), you won't conflict with anybody's work, > considering that this has been said: > > On Fri, Jul 15, 2022 at 11:57:20PM +0100, Russell King (Oracle) wrote: > > Well, at this point, I'm just going to give up with this kernel cycle. > > It seems impossible to get this sorted. It seems impossible to move > > forward with the conversion of Marvell DSA to phylink_pcs. That is correct - I'm not intending to submit it, because there's not enough time to sort out the mess that has been created by comments on the approach coming way too late. And in fact, I'm now _scared_ to submit a revision of it. I don't want to get into writing lots more replies that take hours to compose only to have responses that require yet more multi-hour sessions to reply to, which only then lead to the cycle repeating with no sign of an end to it. Something is very wrong with email as a communication tool when things get to that point. So, I won't be working on this. Someone else can sort the problem.
pon., 25 lip 2022 o 15:32 Russell King (Oracle) <linux@armlinux.org.uk> napisał(a): > > On Mon, Jul 25, 2022 at 03:21:44PM +0300, Vladimir Oltean wrote: > > On Mon, Jul 25, 2022 at 02:18:45AM +0200, Marcin Wojtas wrote: > > > I can of course apply both suggestions, however, I am wondering if I > > > should resend them at all, as Russell's series is still being > > > discussed. IMO it may be worth waiting whether it makes it before the > > > merge window - if not, I can resend this patch after v5.20-rc1, > > > targetting the net branch. What do you think? > > > > I just don't want a fix for a known regression to slip through the cracks. > > You can resend whenever you consider, but I believe that if you do so now > > (today or in the following few days), you won't conflict with anybody's work, > > considering that this has been said: > > > > On Fri, Jul 15, 2022 at 11:57:20PM +0100, Russell King (Oracle) wrote: > > > Well, at this point, I'm just going to give up with this kernel cycle. > > > It seems impossible to get this sorted. It seems impossible to move > > > forward with the conversion of Marvell DSA to phylink_pcs. > > That is correct - I'm not intending to submit it, because there's not > enough time to sort out the mess that has been created by comments > on the approach coming way too late. > > And in fact, I'm now _scared_ to submit a revision of it. I don't want > to get into writing lots more replies that take hours to compose only > to have responses that require yet more multi-hour sessions to reply > to, which only then lead to the cycle repeating with no sign of an end > to it. Something is very wrong with email as a communication tool when > things get to that point. > > So, I won't be working on this. Someone else can sort the problem. > Thank you for the heads-up, I understand your concerns. I'll resubmit this patch then and rebase my 'fwnode_' v3 onto it. Best regards, Marcin
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 37b649501500..9fab76f256bb 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3293,7 +3293,12 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) * port and all DSA ports to their maximum bandwidth and full duplex. */ if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) { - unsigned long caps = dp->pl_config.mac_capabilities; + unsigned long caps; + + if (ds->ops->phylink_get_caps) + ds->ops->phylink_get_caps(ds, port, &dp->pl_config); + + caps = dp->pl_config.mac_capabilities; if (chip->info->ops->port_max_speed_mode) mode = chip->info->ops->port_max_speed_mode(port);
Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") stopped relying on SPEED_MAX constant and hardcoded speed settings for the switch ports and rely on phylink configuration. It turned out, however, that when the relevant code is called, the mac_capabilites of CPU/DSA port remain unset. mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in dsa_tree_setup_switches(), which precedes setting the caps in phylink_get_caps down in the chain of dsa_tree_setup_ports(). As a result the mac_capabilites are 0 and the default speed for CPU/DSA port is 10M at the start. To fix that execute phylink_get_caps() callback which fills port's mac_capabilities before they are processed. Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting") Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)