From: Wong Vee Khee <hidden> Date: 2021-03-18 09:06:29
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 <redacted>
Signed-off-by: Wong Vee Khee <redacted>
---
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(-)
@@ -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;return0;
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.
quoted hunk
Fixes: ee951005e95e ("net: phy: clean up get_phy_c22_id() invalid ID handling")
Cc: stable@vger.kernel.org
Reviewed-by: Voon Weifeng <redacted>
Signed-off-by: Wong Vee Khee <redacted>
---
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(-)
@@ -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;return0;
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
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-03-18 16:22:30
On Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
quoted
On 18.03.2021 10:09, Wong Vee Khee wrote:
quoted
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.
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.
On Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
quoted
On 18.03.2021 10:09, Wong Vee Khee wrote:
quoted
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 Thu, Mar 18, 2021 at 09:02:22AM -0700, Florian Fainelli wrote:
quoted
On 3/18/2021 6:25 AM, Heiner Kallweit wrote:
quoted
On 18.03.2021 10:09, Wong Vee Khee wrote:
quoted
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
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 <redacted>
Signed-off-by: Wong Vee Khee <redacted>
---
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(-)
@@ -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;return0;
+ 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).
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-03-19 08:57:38
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!
From: Wong, Vee Khee <hidden> Date: 2021-03-22 12:54:45
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:
quoted
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