[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <56cb8302-b6a0-4c95-86cc-769fd7ddf815@kernel.org>
Date: Wed, 22 Nov 2023 14:41:04 +1000
From: Greg Ungerer <gerg@...nel.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phylink: require supported_interfaces to be
filled
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@...linux.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.
>
Powered by blists - more mailing lists