* [PATCH net-next] net: phylink: require supported_interfaces to be filled
@ 2023-05-20 10:41 Russell King (Oracle)
2023-05-21 15:23 ` Andrew Lunn
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Russell King (Oracle) @ 2023-05-20 10:41 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev
We have been requiring the supported_interfaces bitmap to be filled in
by MAC drivers that have a mac_select_pcs() method. Now that all MAC
drivers fill in the supported_interfaces bitmap, it is time to enforce
this. We have already required supported_interfaces to be set in order
for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
use phy_interface_t bitmaps for optical modules").
Refuse phylink creation if supported_interfaces is empty, and remove
code to deal with cases where this mask is empty.
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
I believe what I've said above is indeed the case, but there is always
the chance that something has been missed and this will cause breakage.
I would post as RFC and ask for testing, but in my experience that is
a complete waste of time as it doesn't result in any testing feedback.
So, it's probably better to get it merged into net-next and then wait
for any reports of breakage.
drivers/net/phy/phylink.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a4dd5197355a..093b7b6e0263 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -712,14 +712,11 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
{
const unsigned long *interfaces = pl->config->supported_interfaces;
- if (!phy_interface_empty(interfaces)) {
- if (state->interface == PHY_INTERFACE_MODE_NA)
- return phylink_validate_mask(pl, supported, state,
- interfaces);
+ if (state->interface == PHY_INTERFACE_MODE_NA)
+ return phylink_validate_mask(pl, supported, state, interfaces);
- if (!test_bit(state->interface, interfaces))
- return -EINVAL;
- }
+ if (!test_bit(state->interface, interfaces))
+ return -EINVAL;
return phylink_validate_mac_and_pcs(pl, supported, state);
}
@@ -1513,19 +1510,18 @@ struct phylink *phylink_create(struct phylink_config *config,
struct phylink *pl;
int ret;
- if (mac_ops->mac_select_pcs &&
- mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
- ERR_PTR(-EOPNOTSUPP))
- using_mac_select_pcs = true;
-
/* Validate the supplied configuration */
- if (using_mac_select_pcs &&
- phy_interface_empty(config->supported_interfaces)) {
+ if (phy_interface_empty(config->supported_interfaces)) {
dev_err(config->dev,
- "phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
+ "phylink: error: empty supported_interfaces\n");
return ERR_PTR(-EINVAL);
}
+ if (mac_ops->mac_select_pcs &&
+ mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
+ ERR_PTR(-EOPNOTSUPP))
+ using_mac_select_pcs = true;
+
pl = kzalloc(sizeof(*pl), GFP_KERNEL);
if (!pl)
return ERR_PTR(-ENOMEM);
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-05-20 10:41 [PATCH net-next] net: phylink: require supported_interfaces to be filled Russell King (Oracle)
@ 2023-05-21 15:23 ` Andrew Lunn
2023-05-23 2:20 ` patchwork-bot+netdevbpf
2023-11-21 14:19 ` Greg Ungerer
2 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2023-05-21 15:23 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
On Sat, May 20, 2023 at 11:41:42AM +0100, Russell King (Oracle) wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
>
> Refuse phylink creation if supported_interfaces is empty, and remove
> code to deal with cases where this mask is empty.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> I believe what I've said above is indeed the case, but there is always
> the chance that something has been missed and this will cause breakage.
> I would post as RFC and ask for testing, but in my experience that is
> a complete waste of time as it doesn't result in any testing feedback.
> So, it's probably better to get it merged into net-next and then wait
> for any reports of breakage.
Agreed.
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-05-20 10:41 [PATCH net-next] net: phylink: require supported_interfaces to be filled Russell King (Oracle)
2023-05-21 15:23 ` Andrew Lunn
@ 2023-05-23 2:20 ` patchwork-bot+netdevbpf
2023-11-21 14:19 ` Greg Ungerer
2 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-05-23 2:20 UTC (permalink / raw)
To: Russell King; +Cc: andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sat, 20 May 2023 11:41:42 +0100 you wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
>
> [...]
Here is the summary with links:
- [net-next] net: phylink: require supported_interfaces to be filled
https://git.kernel.org/netdev/net-next/c/de5c9bf40c45
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-05-20 10:41 [PATCH net-next] net: phylink: require supported_interfaces to be filled Russell King (Oracle)
2023-05-21 15:23 ` Andrew Lunn
2023-05-23 2:20 ` patchwork-bot+netdevbpf
@ 2023-11-21 14:19 ` Greg Ungerer
2023-11-21 14:29 ` Andrew Lunn
2023-11-21 14:41 ` Russell King (Oracle)
2 siblings, 2 replies; 10+ messages in thread
From: Greg Ungerer @ 2023-11-21 14:19 UTC (permalink / raw)
To: Russell King (Oracle), Andrew Lunn, Heiner Kallweit
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev
Hi Russell,
On 20/5/23 20:41, Russell King (Oracle) wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
>
> Refuse phylink creation if supported_interfaces is empty, and remove
> code to deal with cases where this mask is empty.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> I believe what I've said above is indeed the case, but there is always
> the chance that something has been missed and this will cause breakage.
> I would post as RFC and ask for testing, but in my experience that is
> a complete waste of time as it doesn't result in any testing feedback.
> So, it's probably better to get it merged into net-next and then wait
> for any reports of breakage.
This commit breaks a platform I have with a Marvell 88e6350 switch.
During boot up it now fails with:
...
mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
error creating PHYLINK: -22
mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
...
The 6350 looks to be similar to the 6352 in many respects, though it lacks
a SERDES interface, but it otherwise mostly seems compatible. Using the 6352
phylink_get_caps function instead of the 6185 one fixes this:
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
.stu_getnext = mv88e6352_g1_stu_getnext,
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6352_phylink_get_caps,
};
static const struct mv88e6xxx_ops mv88e6351_ops = {
The story doesn't quite end here though. With this fix in place support
for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
on boot up:
...
mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
Hardware name: Marvell Armada 370/XP (Device Tree)
Workqueue: events_unbound deferred_probe_work_func
PC is at mv88e6xxx_port_setup+0x1c/0x44
LR is at dsa_port_devlink_setup+0x74/0x154
pc : [<c057ea24>] lr : [<c0819598>] psr: a0000013
sp : c184fce0 ip : c542b8f4 fp : 00000000
r10: 00000001 r9 : c542a540 r8 : c542bc00
r7 : c542b838 r6 : c5244580 r5 : 00000005 r4 : c5244580
r3 : 00000000 r2 : c542b840 r1 : 00000005 r0 : c1a02040
...
I can see that the mv88e6350_ops struct doesn't have an initializer for
pcs_ops. Based on the similarity with the 6352 again I tried using the
mv88e6352_pcs_ops for that:
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5069,7 +5069,8 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.stu_getnext = mv88e6352_g1_stu_getnext,
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6352_phylink_get_caps,
+ .pcs_ops = &mv88e6352_pcs_ops,
};
This gets the 6350 switch back to working again (on current linux 6.7-rc2).
I am not entirely sure if this needs a dedicated phylink_get_caps
and pcs_ops for the 6350, or if it is safe to use the 6352 ones?
Looking at the mv88e6351_ops I am guessing it is going to suffer the
same problems too.
Regards
Greg
> drivers/net/phy/phylink.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a4dd5197355a..093b7b6e0263 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -712,14 +712,11 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> {
> const unsigned long *interfaces = pl->config->supported_interfaces;
>
> - if (!phy_interface_empty(interfaces)) {
> - if (state->interface == PHY_INTERFACE_MODE_NA)
> - return phylink_validate_mask(pl, supported, state,
> - interfaces);
> + if (state->interface == PHY_INTERFACE_MODE_NA)
> + return phylink_validate_mask(pl, supported, state, interfaces);
>
> - if (!test_bit(state->interface, interfaces))
> - return -EINVAL;
> - }
> + if (!test_bit(state->interface, interfaces))
> + return -EINVAL;
>
> return phylink_validate_mac_and_pcs(pl, supported, state);
> }
> @@ -1513,19 +1510,18 @@ struct phylink *phylink_create(struct phylink_config *config,
> struct phylink *pl;
> int ret;
>
> - if (mac_ops->mac_select_pcs &&
> - mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> - ERR_PTR(-EOPNOTSUPP))
> - using_mac_select_pcs = true;
> -
> /* Validate the supplied configuration */
> - if (using_mac_select_pcs &&
> - phy_interface_empty(config->supported_interfaces)) {
> + if (phy_interface_empty(config->supported_interfaces)) {
> dev_err(config->dev,
> - "phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
> + "phylink: error: empty supported_interfaces\n");
> return ERR_PTR(-EINVAL);
> }
>
> + if (mac_ops->mac_select_pcs &&
> + mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> + ERR_PTR(-EOPNOTSUPP))
> + using_mac_select_pcs = true;
> +
> pl = kzalloc(sizeof(*pl), GFP_KERNEL);
> if (!pl)
> return ERR_PTR(-ENOMEM);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-21 14:19 ` Greg Ungerer
@ 2023-11-21 14:29 ` Andrew Lunn
2023-11-22 4:12 ` Greg Ungerer
2023-11-21 14:41 ` Russell King (Oracle)
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2023-11-21 14:29 UTC (permalink / raw)
To: Greg Ungerer
Cc: Russell King (Oracle),
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
> The 6350 looks to be similar to the 6352 in many respects, though it lacks
> a SERDES interface, but it otherwise mostly seems compatible.
Not having the SERDES is important. Without that SERDES, the bit about
Port 4 in mv88e6352_phylink_get_caps() is
incorrect. mv88e61852_phylink_get_caps() looks reasonable for this
hardware.
> Using the 6352
> phylink_get_caps function instead of the 6185 one fixes this:
>
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
> .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
> .stu_getnext = mv88e6352_g1_stu_getnext,
> .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
> - .phylink_get_caps = mv88e6185_phylink_get_caps,
> + .phylink_get_caps = mv88e6352_phylink_get_caps,
> };
>
> static const struct mv88e6xxx_ops mv88e6351_ops = {
>
>
> The story doesn't quite end here though. With this fix in place support
> for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
> mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
> on boot up:
PCS is approximately another name of a SERDES. Since there is no
SERDES, you don't don't want any of the pcs ops filled in.
Russell knows this code much better than i do. Let see what he says.
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-21 14:19 ` Greg Ungerer
2023-11-21 14:29 ` Andrew Lunn
@ 2023-11-21 14:41 ` Russell King (Oracle)
2023-11-22 4:41 ` Greg Ungerer
1 sibling, 1 reply; 10+ messages in thread
From: Russell King (Oracle) @ 2023-11-21 14:41 UTC (permalink / raw)
To: Greg Ungerer
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
On Wed, Nov 22, 2023 at 12:19:38AM +1000, Greg Ungerer wrote:
> Hi Russell,
>
> On 20/5/23 20:41, Russell King (Oracle) wrote:
> > We have been requiring the supported_interfaces bitmap to be filled in
> > by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> > drivers fill in the supported_interfaces bitmap, it is time to enforce
> > this. We have already required supported_interfaces to be set in order
> > for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> > use phy_interface_t bitmaps for optical modules").
> >
> > Refuse phylink creation if supported_interfaces is empty, and remove
> > code to deal with cases where this mask is empty.
> >
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > I believe what I've said above is indeed the case, but there is always
> > the chance that something has been missed and this will cause breakage.
> > I would post as RFC and ask for testing, but in my experience that is
> > a complete waste of time as it doesn't result in any testing feedback.
> > So, it's probably better to get it merged into net-next and then wait
> > for any reports of breakage.
>
> This commit breaks a platform I have with a Marvell 88e6350 switch.
> During boot up it now fails with:
>
> ...
> mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
> mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
> error creating PHYLINK: -22
> mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
> ...
>
> The 6350 looks to be similar to the 6352 in many respects, though it lacks
> a SERDES interface, but it otherwise mostly seems compatible. Using the 6352
> phylink_get_caps function instead of the 6185 one fixes this:
>
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
> .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
> .stu_getnext = mv88e6352_g1_stu_getnext,
> .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
> - .phylink_get_caps = mv88e6185_phylink_get_caps,
> + .phylink_get_caps = mv88e6352_phylink_get_caps,
> };
Based on what you say, this is probably correct, but I've no idea as I
don't have any data on the 88e6350.
> The story doesn't quite end here though. With this fix in place support
> for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
> mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
> on boot up:
>
> ...
> mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
> [00000000] *pgd=00000000
> Internal error: Oops: 5 [#1] ARM
> Modules linked in:
> CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
> Hardware name: Marvell Armada 370/XP (Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> PC is at mv88e6xxx_port_setup+0x1c/0x44
> LR is at dsa_port_devlink_setup+0x74/0x154
> pc : [<c057ea24>] lr : [<c0819598>] psr: a0000013
> sp : c184fce0 ip : c542b8f4 fp : 00000000
> r10: 00000001 r9 : c542a540 r8 : c542bc00
> r7 : c542b838 r6 : c5244580 r5 : 00000005 r4 : c5244580
> r3 : 00000000 r2 : c542b840 r1 : 00000005 r0 : c1a02040
> ...
>
> I can see that the mv88e6350_ops struct doesn't have an initializer for
> pcs_ops.
You mentioned that the 6350 doesn't have serdes, so there should be no
PCS. mv88e6xxx_port_setup() probably should be doing:
- if (chip->info->ops->pcs_ops->pcs_init) {
+ if (chip->info->ops->pcs_ops &&
+ chip->info->ops->pcs_ops->pcs_init) {
> This gets the 6350 switch back to working again (on current linux 6.7-rc2).
> I am not entirely sure if this needs a dedicated phylink_get_caps
> and pcs_ops for the 6350, or if it is safe to use the 6352 ones?
Without knowing what the 6350 cmode values are, I can't comment.
> Looking at the mv88e6351_ops I am guessing it is going to suffer the
> same problems too.
Again, it's a similar problem - I have no information for the 6351
so I could only make guesses.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-21 14:29 ` Andrew Lunn
@ 2023-11-22 4:12 ` Greg Ungerer
2023-11-22 9:27 ` Russell King (Oracle)
0 siblings, 1 reply; 10+ messages in thread
From: Greg Ungerer @ 2023-11-22 4:12 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King (Oracle),
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
On 22/11/23 00:29, Andrew Lunn wrote:
>> The 6350 looks to be similar to the 6352 in many respects, though it lacks
>> a SERDES interface, but it otherwise mostly seems compatible.
>
> Not having the SERDES is important. Without that SERDES, the bit about
> Port 4 in mv88e6352_phylink_get_caps() is
> incorrect. mv88e61852_phylink_get_caps() looks reasonable for this
> hardware.
^^^^^^^^^^
The problem with mv88e6185_phylink_get_caps() is the cmode check fails
for me. For my 6350 hardware chip->ports[port].cmode is "9", so set to
MV88E6XXX_PORT_STS_CMODE_1000BASEX. But that is not part of the defines
used in mv88e6185_phy_interface_modes[].
Doesn't it need to be checking in mv88e6xxx_phy_interface_modes[]
for the cmode?
I see another similar function, mv88e6250_phylink_get_caps().
But that is only 10/100 capable.
So I am thinking that something like this actually makes more sense now:
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -577,6 +577,18 @@ static void mv88e6250_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100;
}
+static void mv88e6350_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
+ struct phylink_config *config)
+{
+ unsigned long *supported = config->supported_interfaces;
+
+ /* Translate the default cmode */
+ mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported);
+
+ config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 |
+ MAC_1000FD;
+}
+
static int mv88e6352_get_port4_serdes_cmode(struct mv88e6xxx_chip *chip)
{
u16 reg, val;
@@ -5069,7 +5082,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.stu_getnext = mv88e6352_g1_stu_getnext,
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6350_phylink_get_caps,
};
static const struct mv88e6xxx_ops mv88e6351_ops = {
@@ -5117,7 +5130,7 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
.avb_ops = &mv88e6352_avb_ops,
.ptp_ops = &mv88e6352_ptp_ops,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6350_phylink_get_caps,
};
static const struct mv88e6xxx_ops mv88e6352_ops = {
>> Using the 6352
>> phylink_get_caps function instead of the 6185 one fixes this:
>>
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
>> .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
>> .stu_getnext = mv88e6352_g1_stu_getnext,
>> .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
>> - .phylink_get_caps = mv88e6185_phylink_get_caps,
>> + .phylink_get_caps = mv88e6352_phylink_get_caps,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6351_ops = {
>>
>>
>> The story doesn't quite end here though. With this fix in place support
>> for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
>> mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
>> on boot up:
>
> PCS is approximately another name of a SERDES. Since there is no
> SERDES, you don't don't want any of the pcs ops filled in.
>
> Russell knows this code much better than i do. Let see what he says.
Ok, that makes sense. Russell had a suggestion for this one and I will
follow up with that.
Thanks
Greg
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-21 14:41 ` Russell King (Oracle)
@ 2023-11-22 4:41 ` Greg Ungerer
0 siblings, 0 replies; 10+ messages in thread
From: Greg Ungerer @ 2023-11-22 4:41 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
On 22/11/23 00:41, Russell King (Oracle) wrote:
> On Wed, Nov 22, 2023 at 12:19:38AM +1000, Greg Ungerer wrote:
>> Hi Russell,
>>
>> On 20/5/23 20:41, Russell King (Oracle) wrote:
>>> We have been requiring the supported_interfaces bitmap to be filled in
>>> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
>>> drivers fill in the supported_interfaces bitmap, it is time to enforce
>>> this. We have already required supported_interfaces to be set in order
>>> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
>>> use phy_interface_t bitmaps for optical modules").
>>>
>>> Refuse phylink creation if supported_interfaces is empty, and remove
>>> code to deal with cases where this mask is empty.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>> I believe what I've said above is indeed the case, but there is always
>>> the chance that something has been missed and this will cause breakage.
>>> I would post as RFC and ask for testing, but in my experience that is
>>> a complete waste of time as it doesn't result in any testing feedback.
>>> So, it's probably better to get it merged into net-next and then wait
>>> for any reports of breakage.
>>
>> This commit breaks a platform I have with a Marvell 88e6350 switch.
>> During boot up it now fails with:
>>
>> ...
>> mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>> mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
>> error creating PHYLINK: -22
>> mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
>> ...
>>
>> The 6350 looks to be similar to the 6352 in many respects, though it lacks
>> a SERDES interface, but it otherwise mostly seems compatible. Using the 6352
>> phylink_get_caps function instead of the 6185 one fixes this:
>>
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
>> .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
>> .stu_getnext = mv88e6352_g1_stu_getnext,
>> .stu_loadpurge = mv88e6352_g1_stu_loadpurge,
>> - .phylink_get_caps = mv88e6185_phylink_get_caps,
>> + .phylink_get_caps = mv88e6352_phylink_get_caps,
>> };
>
> Based on what you say, this is probably correct, but I've no idea as I
> don't have any data on the 88e6350.
I am thinking a dedicated 6350 phylink_get_caps may be better here now,
since the 6350 does not have a SERDES lane like the 6352.
>> The story doesn't quite end here though. With this fix in place support
>> for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
>> mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
>> on boot up:
>>
>> ...
>> mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
>> 8<--- cut here ---
>> Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
>> [00000000] *pgd=00000000
>> Internal error: Oops: 5 [#1] ARM
>> Modules linked in:
>> CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
>> Hardware name: Marvell Armada 370/XP (Device Tree)
>> Workqueue: events_unbound deferred_probe_work_func
>> PC is at mv88e6xxx_port_setup+0x1c/0x44
>> LR is at dsa_port_devlink_setup+0x74/0x154
>> pc : [<c057ea24>] lr : [<c0819598>] psr: a0000013
>> sp : c184fce0 ip : c542b8f4 fp : 00000000
>> r10: 00000001 r9 : c542a540 r8 : c542bc00
>> r7 : c542b838 r6 : c5244580 r5 : 00000005 r4 : c5244580
>> r3 : 00000000 r2 : c542b840 r1 : 00000005 r0 : c1a02040
>> ...
>>
>> I can see that the mv88e6350_ops struct doesn't have an initializer for
>> pcs_ops.
>
> You mentioned that the 6350 doesn't have serdes, so there should be no
> PCS. mv88e6xxx_port_setup() probably should be doing:
>
> - if (chip->info->ops->pcs_ops->pcs_init) {
> + if (chip->info->ops->pcs_ops &&
> + chip->info->ops->pcs_ops->pcs_init) {
Yes, that probably makes more sense.
With that change in place the probing works again, and does not need
a pcs_ops entry for the 6350.
Thanks
Greg
>> This gets the 6350 switch back to working again (on current linux 6.7-rc2).
>> I am not entirely sure if this needs a dedicated phylink_get_caps
>> and pcs_ops for the 6350, or if it is safe to use the 6352 ones?
>
> Without knowing what the 6350 cmode values are, I can't comment.
>
>> Looking at the mv88e6351_ops I am guessing it is going to suffer the
>> same problems too.
>
> Again, it's a similar problem - I have no information for the 6351
> so I could only make guesses.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-22 4:12 ` Greg Ungerer
@ 2023-11-22 9:27 ` Russell King (Oracle)
2023-11-22 13:10 ` Greg Ungerer
0 siblings, 1 reply; 10+ messages in thread
From: Russell King (Oracle) @ 2023-11-22 9:27 UTC (permalink / raw)
To: Greg Ungerer
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
On Wed, Nov 22, 2023 at 02:12:44PM +1000, Greg Ungerer wrote:
> So I am thinking that something like this actually makes more sense now:
>
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -577,6 +577,18 @@ static void mv88e6250_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100;
> }
> +static void mv88e6350_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
> + struct phylink_config *config)
> +{
> + unsigned long *supported = config->supported_interfaces;
> +
> + /* Translate the default cmode */
> + mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported);
> +
> + config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 |
> + MAC_1000FD;
> +}
> +
Looks sensible to me - but I do notice that a black line has been lost
between mv88e6250_phylink_get_caps() and your new function - probably
down to your email client being stupid with whitespace because it's
broken the patch context. Just be aware of that when you come to send
the patch for real.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phylink: require supported_interfaces to be filled
2023-11-22 9:27 ` Russell King (Oracle)
@ 2023-11-22 13:10 ` Greg Ungerer
0 siblings, 0 replies; 10+ messages in thread
From: Greg Ungerer @ 2023-11-22 13:10 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
On 22/11/23 19:27, Russell King (Oracle) wrote:
> On Wed, Nov 22, 2023 at 02:12:44PM +1000, Greg Ungerer wrote:
>> So I am thinking that something like this actually makes more sense now:
>>
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -577,6 +577,18 @@ static void mv88e6250_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
>> config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100;
>> }
>> +static void mv88e6350_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
>> + struct phylink_config *config)
>> +{
>> + unsigned long *supported = config->supported_interfaces;
>> +
>> + /* Translate the default cmode */
>> + mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported);
>> +
>> + config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 |
>> + MAC_1000FD;
>> +}
>> +
>
> Looks sensible to me - but I do notice that a black line has been lost
> between mv88e6250_phylink_get_caps() and your new function - probably
> down to your email client being stupid with whitespace because it's
> broken the patch context. Just be aware of that when you come to send
> the patch for real.
Oh, yes, for sure. The above was a cut and paste into email client.
I'll send proper patches via git send-email soon.
Regards
Greg
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-11-22 13:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-20 10:41 [PATCH net-next] net: phylink: require supported_interfaces to be filled Russell King (Oracle)
2023-05-21 15:23 ` Andrew Lunn
2023-05-23 2:20 ` patchwork-bot+netdevbpf
2023-11-21 14:19 ` Greg Ungerer
2023-11-21 14:29 ` Andrew Lunn
2023-11-22 4:12 ` Greg Ungerer
2023-11-22 9:27 ` Russell King (Oracle)
2023-11-22 13:10 ` Greg Ungerer
2023-11-21 14:41 ` Russell King (Oracle)
2023-11-22 4:41 ` Greg Ungerer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).