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: stable@vger.kernel.org Reviewed-by: Voon Weifeng voon.weifeng@intel.com Signed-off-by: Wong Vee Khee vee.khee.wong@intel.com --- 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 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: stable@vger.kernel.org Reviewed-by: Voon Weifeng voon.weifeng@intel.com Signed-off-by: Wong Vee Khee vee.khee.wong@intel.com
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?
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
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: stable@vger.kernel.org Reviewed-by: Voon Weifeng voon.weifeng@intel.com Signed-off-by: Wong Vee Khee vee.khee.wong@intel.com
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.
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-p... 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 oliver.sang@intel.com
[ 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/lkp@lists.01.org Intel Corporation
Thanks, Oliver Sang
linux-stable-mirror@lists.linaro.org
-
Florian Fainelli -
Greg KH -
Heiner Kallweit -
kernel test robot -
Russell King - ARM Linux admin -
Wong Vee Khee -
Wong, Vee Khee