When using Clause-22 to probe for PHY devices such as the Marvell
88E2110, PHY ID with value 0 is read from the MII PHYID registers
which caused the PHY framework failed to attach the Marvell PHY
driver.
Fixed this by adding a check of PHY ID equals to all zeroes.
Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID handling")
Cc: [email protected]
Reviewed-by: Voon Weifeng <[email protected]>
Signed-off-by: Wong Vee Khee <[email protected]>
---
v2 changelog:
- added fixes tag
- marked for net instead of net-next
---
drivers/net/phy/phy_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index cc38e326405a..c12c30254c11 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -809,8 +809,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
*phy_id |= phy_reg;
- /* If the phy_id is mostly Fs, there is no device there */
- if ((*phy_id & 0x1fffffff) == 0x1fffffff)
+ /* If the phy_id is mostly Fs or all zeroes, there is no device there */
+ if (((*phy_id & 0x1fffffff) == 0x1fffffff) || (*phy_id == 0))
return -ENODEV;
return 0;
--
2.25.1
On 18.03.2021 10:09, Wong Vee Khee wrote:
> When using Clause-22 to probe for PHY devices such as the Marvell
> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> which caused the PHY framework failed to attach the Marvell PHY
> driver.
>
> Fixed this by adding a check of PHY ID equals to all zeroes.
>
I was wondering whether we have, and may break, use cases where a PHY,
for whatever reason, reports PHY ID 0, but works with the genphy
driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
the patch may break the fixed phy.
Having said that I think your patch is ok, but we need a change of
the PHY ID reported by swphy_read_reg() first.
At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
should be sufficient. This value shouldn't collide with any real world
PHY ID.
> Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID handling")
> Cc: [email protected]
> Reviewed-by: Voon Weifeng <[email protected]>
> Signed-off-by: Wong Vee Khee <[email protected]>
> ---
> v2 changelog:
> - added fixes tag
> - marked for net instead of net-next
> ---
> drivers/net/phy/phy_device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index cc38e326405a..c12c30254c11 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -809,8 +809,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
>
> *phy_id |= phy_reg;
>
> - /* If the phy_id is mostly Fs, there is no device there */
> - if ((*phy_id & 0x1fffffff) == 0x1fffffff)
> + /* If the phy_id is mostly Fs or all zeroes, there is no device there */
> + if (((*phy_id & 0x1fffffff) == 0x1fffffff) || (*phy_id == 0))
> return -ENODEV;
>
> return 0;
>
On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
> On 18.03.2021 10:09, Wong Vee Khee wrote:
>> When using Clause-22 to probe for PHY devices such as the Marvell
>> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
>> which caused the PHY framework failed to attach the Marvell PHY
>> driver.
>>
>> Fixed this by adding a check of PHY ID equals to all zeroes.
>>
>
> I was wondering whether we have, and may break, use cases where a PHY,
> for whatever reason, reports PHY ID 0, but works with the genphy
> driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
> the patch may break the fixed phy.
> Having said that I think your patch is ok, but we need a change of
> the PHY ID reported by swphy_read_reg() first.
> At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
> should be sufficient. This value shouldn't collide with any real world
> PHY ID.
It most likely would not, but it could be considered an ABI breakage,
unless we filter out what we report to user-space via SIOGCMIIREG and
/sys/class/mdio_bus/*/*/phy_id
Ideally we would have assigned an unique PHY OUI to the fixed PHY but
that would have required registering Linux as a vendor, and the process
is not entirely clear to me about how to go about doing that.
--
Florian
On Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
> On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
> > On 18.03.2021 10:09, Wong Vee Khee wrote:
> >> When using Clause-22 to probe for PHY devices such as the Marvell
> >> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> >> which caused the PHY framework failed to attach the Marvell PHY
> >> driver.
> >>
> >> Fixed this by adding a check of PHY ID equals to all zeroes.
> >>
> >
> > I was wondering whether we have, and may break, use cases where a PHY,
> > for whatever reason, reports PHY ID 0, but works with the genphy
> > driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
> > the patch may break the fixed phy.
> > Having said that I think your patch is ok, but we need a change of
> > the PHY ID reported by swphy_read_reg() first.
> > At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
> > should be sufficient. This value shouldn't collide with any real world
> > PHY ID.
>
> It most likely would not, but it could be considered an ABI breakage,
> unless we filter out what we report to user-space via SIOGCMIIREG and
> /sys/class/mdio_bus/*/*/phy_id
>
> Ideally we would have assigned an unique PHY OUI to the fixed PHY but
> that would have required registering Linux as a vendor, and the process
> is not entirely clear to me about how to go about doing that.
Doesn't that also involve yearly fees?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 18.03.2021 17:02, Florian Fainelli wrote:
>
>
> On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
>> On 18.03.2021 10:09, Wong Vee Khee wrote:
>>> When using Clause-22 to probe for PHY devices such as the Marvell
>>> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
>>> which caused the PHY framework failed to attach the Marvell PHY
>>> driver.
>>>
>>> Fixed this by adding a check of PHY ID equals to all zeroes.
>>>
>>
>> I was wondering whether we have, and may break, use cases where a PHY,
>> for whatever reason, reports PHY ID 0, but works with the genphy
>> driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
>> the patch may break the fixed phy.
>> Having said that I think your patch is ok, but we need a change of
>> the PHY ID reported by swphy_read_reg() first.
>> At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
>> should be sufficient. This value shouldn't collide with any real world
>> PHY ID.
>
> It most likely would not, but it could be considered an ABI breakage,
> unless we filter out what we report to user-space via SIOGCMIIREG and
> /sys/class/mdio_bus/*/*/phy_id
>
> Ideally we would have assigned an unique PHY OUI to the fixed PHY but
> that would have required registering Linux as a vendor, and the process
> is not entirely clear to me about how to go about doing that.
> --
In the OUI list I found entry 58-9C-FC, belonging to FreeBSD Foundation.
Not sure what they use it for, but it seems adding Linux as a vendor
wouldn't be a total exception.
> Florian
>
Heiner
On Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
>
>
> On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
> > On 18.03.2021 10:09, Wong Vee Khee wrote:
> >> When using Clause-22 to probe for PHY devices such as the Marvell
> >> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> >> which caused the PHY framework failed to attach the Marvell PHY
> >> driver.
> >>
> >> Fixed this by adding a check of PHY ID equals to all zeroes.
> >>
> >
> > I was wondering whether we have, and may break, use cases where a PHY,
> > for whatever reason, reports PHY ID 0, but works with the genphy
> > driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
> > the patch may break the fixed phy.
> > Having said that I think your patch is ok, but we need a change of
> > the PHY ID reported by swphy_read_reg() first.
> > At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
> > should be sufficient. This value shouldn't collide with any real world
> > PHY ID.
>
> It most likely would not, but it could be considered an ABI breakage,
> unless we filter out what we report to user-space via SIOGCMIIREG and
> /sys/class/mdio_bus/*/*/phy_id
>
> Ideally we would have assigned an unique PHY OUI to the fixed PHY but
> that would have required registering Linux as a vendor, and the process
> is not entirely clear to me about how to go about doing that.
If you need me to do that under the umbrella of the Linux Foundation,
I'll be glad to do so if you point me at the proper group to do that
with.
We did this for a few years with the USB-IF and have a vendor id
assigned to us for Linux through them, until they kicked us out because.
But as the number is in a global namespace, it can't be reused so we
keep it :)
thanks,
greg k-h
On 3/18/2021 11:14 AM, Greg KH wrote:
> On Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
>>
>>
>> On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
>>> On 18.03.2021 10:09, Wong Vee Khee wrote:
>>>> When using Clause-22 to probe for PHY devices such as the Marvell
>>>> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
>>>> which caused the PHY framework failed to attach the Marvell PHY
>>>> driver.
>>>>
>>>> Fixed this by adding a check of PHY ID equals to all zeroes.
>>>>
>>>
>>> I was wondering whether we have, and may break, use cases where a PHY,
>>> for whatever reason, reports PHY ID 0, but works with the genphy
>>> driver. And indeed in swphy_read_reg() we return PHY ID 0, therefore
>>> the patch may break the fixed phy.
>>> Having said that I think your patch is ok, but we need a change of
>>> the PHY ID reported by swphy_read_reg() first.
>>> At a first glance changing the PHY ID to 0x00000001 in swphy_read_reg()
>>> should be sufficient. This value shouldn't collide with any real world
>>> PHY ID.
>>
>> It most likely would not, but it could be considered an ABI breakage,
>> unless we filter out what we report to user-space via SIOGCMIIREG and
>> /sys/class/mdio_bus/*/*/phy_id
>>
>> Ideally we would have assigned an unique PHY OUI to the fixed PHY but
>> that would have required registering Linux as a vendor, and the process
>> is not entirely clear to me about how to go about doing that.
>
> If you need me to do that under the umbrella of the Linux Foundation,
> I'll be glad to do so if you point me at the proper group to do that
> with.
>
> We did this for a few years with the USB-IF and have a vendor id
> assigned to us for Linux through them, until they kicked us out because.
> But as the number is in a global namespace, it can't be reused so we
> keep it :)
We would still be creating what is technically an user-space interface
breakage since prior to a given kernel version we would return 0 for
that PHY OUI, and after another point it would be something different. I
don't know what software out there may be expecting to find 0 and not
determine that the PHY was fixed already because it is under
/sys/class/mdio_bus/fixed-0
--
Florian
On 18.03.2021 10:09, Wong Vee Khee wrote:
> When using Clause-22 to probe for PHY devices such as the Marvell
> 88E2110, PHY ID with value 0 is read from the MII PHYID registers
> which caused the PHY framework failed to attach the Marvell PHY
> driver.
>
> Fixed this by adding a check of PHY ID equals to all zeroes.
>
> Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID handling")
> Cc: [email protected]
> Reviewed-by: Voon Weifeng <[email protected]>
> Signed-off-by: Wong Vee Khee <[email protected]>
> ---
> v2 changelog:
> - added fixes tag
> - marked for net instead of net-next
> ---
> drivers/net/phy/phy_device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index cc38e326405a..c12c30254c11 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -809,8 +809,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
>
> *phy_id |= phy_reg;
>
> - /* If the phy_id is mostly Fs, there is no device there */
> - if ((*phy_id & 0x1fffffff) == 0x1fffffff)
> + /* If the phy_id is mostly Fs or all zeroes, there is no device there */
> + if (((*phy_id & 0x1fffffff) == 0x1fffffff) || (*phy_id == 0))
> return -ENODEV;
>
> return 0;
>
+ the authors of 0cc8fecf041d ("net: phy: Allow mdio buses to auto-probe c45 devices")
In case of MDIOBUS_C22_C45 we probe c22 first, and then c45.
This causes problems with c45 PHY's that have rudimentary c22 support
and return 0 when reading the c22 PHY ID registers.
Is there a specific reason why c22 is probed first? Reversing the order
would solve the issue we speak about here.
c45-probing of c22-only PHY's shouldn't return false positives
(at least at a first glance).
On Fri, Mar 19, 2021 at 08:40:45AM +0100, Heiner Kallweit wrote:
> Is there a specific reason why c22 is probed first? Reversing the order
> would solve the issue we speak about here.
> c45-probing of c22-only PHY's shouldn't return false positives
> (at least at a first glance).
That would likely cause problems for the I2f MDIO driver, since a
C45 read is indistinguishable from a C22 write on the I2C bus.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Mar 19, 2021 at 04:56PM +0800, Russell King - ARM Linux admin wrote:
> On Fri, Mar 19, 2021 at 08:40:45AM +0100, Heiner Kallweit wrote:
>> Is there a specific reason why c22 is probed first? Reversing the order
>> would solve the issue we speak about here.
>> c45-probing of c22-only PHY's shouldn't return false positives
>> (at least at a first glance).
>
> That would likely cause problems for the I2f MDIO driver, since a
> C45 read is indistinguishable from a C22 write on the I2C bus.
>
Hi Russell,
STMMAC is capable of supporting external PHYs that accessible using
C22 or C45.
Accordng to patch [1] send earlier, it should solve the problem.
As for any other drivers, if it is not using MDIOBUS_C45_C22,
It should still work as it is by using MDIOBUS_C22.
[1] https://lkml.org/lkml/2020/11/9/443
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: c0da0048be0d27455dfb3f4c81604e275b4776d9 ("[PATCH net V2 1/1] net: phy: fix invalid phy id when probe using C22")
url: https://github.com/0day-ci/linux/commits/Wong-Vee-Khee/net-phy-fix-invalid-phy-id-when-probe-using-C22/20210318-170748
base: https://git.kernel.org/cgit/linux/kernel/git/davem/net.git e65eaded4cc4de6bf153def9dde6b25392d9a236
in testcase: trinity
version: trinity-static-i386-x86_64-f93256fb_2019-08-28
with following parameters:
group: group-04
test-description: Trinity is a linux system call fuzz tester.
test-url: http://codemonkey.org.uk/projects/trinity/
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-------------------------------------------------------+------------+------------+
| | e65eaded4c | c0da0048be |
+-------------------------------------------------------+------------+------------+
| WARNING:at_net/core/devlink.c:#devlink_port_type_warn | 0 | 8 |
| EIP:devlink_port_type_warn | 0 | 8 |
+-------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 3904.863424] WARNING: CPU: 1 PID: 3806 at net/core/devlink.c:8317 devlink_port_type_warn (kbuild/src/consumer/net/core/devlink.c:8317)
[ 3904.868535] Modules linked in:
[ 3904.869305] CPU: 1 PID: 3806 Comm: kworker/1:4 Not tainted 5.12.0-rc2-00394-gc0da0048be0d #1
[ 3904.871261] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 3904.873493] Workqueue: events devlink_port_type_warn
[ 3904.878296] EIP: devlink_port_type_warn (kbuild/src/consumer/net/core/devlink.c:8317)
[ 3904.880591] Code: 00 00 00 66 90 66 66 66 66 90 55 31 c9 ba 01 00 00 00 b8 c8 ca cc 5b 89 e5 6a 01 e8 c5 91 23 ff 68 5c 98 6b 5b e8 89 07 4c 00 <0f> 0b 6a 01 31 c9 ba 01 00 00 00 b8 b0 ca cc 5b e8 a6 91 23 ff 83
All code
========
0: 00 00 add %al,(%rax)
2: 00 66 90 add %ah,-0x70(%rsi)
5: 66 66 66 66 90 data16 data16 data16 xchg %ax,%ax
a: 55 push %rbp
b: 31 c9 xor %ecx,%ecx
d: ba 01 00 00 00 mov $0x1,%edx
12: b8 c8 ca cc 5b mov $0x5bcccac8,%eax
17: 89 e5 mov %esp,%ebp
19: 6a 01 pushq $0x1
1b: e8 c5 91 23 ff callq 0xffffffffff2391e5
20: 68 5c 98 6b 5b pushq $0x5b6b985c
25: e8 89 07 4c 00 callq 0x4c07b3
2a:* 0f 0b ud2 <-- trapping instruction
2c: 6a 01 pushq $0x1
2e: 31 c9 xor %ecx,%ecx
30: ba 01 00 00 00 mov $0x1,%edx
35: b8 b0 ca cc 5b mov $0x5bcccab0,%eax
3a: e8 a6 91 23 ff callq 0xffffffffff2391e5
3f: 83 .byte 0x83
Code starting with the faulting instruction
===========================================
0: 0f 0b ud2
2: 6a 01 pushq $0x1
4: 31 c9 xor %ecx,%ecx
6: ba 01 00 00 00 mov $0x1,%edx
b: b8 b0 ca cc 5b mov $0x5bcccab0,%eax
10: e8 a6 91 23 ff callq 0xffffffffff2391bb
15: 83 .byte 0x83
[ 3904.890286] EAX: 00000022 EBX: 00000000 ECX: 00000000 EDX: 00000000
[ 3904.893771] ESI: bfb0f8d8 EDI: 4e985420 EBP: 4eb1fefc ESP: 4eb1fef4
[ 3904.897519] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010202
[ 3904.900728] CR0: 80050033 CR2: 08e9032c CR3: 15705000 CR4: 00000690
[ 3904.904242] Call Trace:
[ 3904.905724] ? process_one_work (kbuild/src/consumer/arch/x86/include/asm/jump_label.h:25 kbuild/src/consumer/include/linux/jump_label.h:200 kbuild/src/consumer/include/trace/events/workqueue.h:108 kbuild/src/consumer/kernel/workqueue.c:2280)
[ 3904.907975] ? __this_cpu_preempt_check (kbuild/src/consumer/lib/smp_processor_id.c:71)
[ 3904.910231] ? worker_thread (kbuild/src/consumer/include/linux/list.h:282 kbuild/src/consumer/kernel/workqueue.c:2422)
[ 3904.912453] ? kthread (kbuild/src/consumer/kernel/kthread.c:292)
[ 3904.913933] ? process_one_work (kbuild/src/consumer/kernel/workqueue.c:2364)
[ 3904.915948] ? kthread_bind (kbuild/src/consumer/kernel/kthread.c:245)
[ 3904.917826] ? ret_from_fork (kbuild/src/consumer/arch/x86/entry/entry_32.S:856)
[ 3904.919542] ---[ end trace aff30e8d06df1b5f ]---
[ 3904.921680] ------------[ cut here ]------------
[ 3904.924042] Type was not set for devlink port.
To reproduce:
# build kernel
cd linux
cp config-5.12.0-rc2-00394-gc0da0048be0d .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang