| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <yw1xft0fezfp.fsf@mansr.com>
Date: Sun, 28 Mar 2021 22:33:14 +0100
From: Måns Rullgård <mans@...sr.com>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: Andre Edich <andre.edich@...rochip.com>, netdev@...r.kernel.org,
UNGLinuxDriver@...rochip.com, steve.glendinning@...well.net,
Parthiban.Veerasooran@...rochip.com
Subject: Re: [PATCH net-next v5 3/3] smsc95xx: add phylib support
Heiner Kallweit <hkallweit1@...il.com> writes:
> On 28.03.2021 21:59, Måns Rullgård wrote:
>> Andre Edich <andre.edich@...rochip.com> writes:
>>
>>> Generally, each PHY has their own configuration and it can be done
>>> through an external PHY driver. The smsc95xx driver uses only the
>>> hard-coded internal PHY configuration.
>>>
>>> This patch adds phylib support to probe external PHY drivers for
>>> configuring external PHYs.
>>>
>>> The MDI-X configuration for the internal PHYs moves from
>>> drivers/net/usb/smsc95xx.c to drivers/net/phy/smsc.c.
>>>
>>> Signed-off-by: Andre Edich <andre.edich@...rochip.com>
>>> ---
>>> drivers/net/phy/smsc.c | 67 +++++++
>>> drivers/net/usb/Kconfig | 2 +
>>> drivers/net/usb/smsc95xx.c | 399 +++++++++++++------------------------
>>> 3 files changed, 203 insertions(+), 265 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>>> index 74568ae16125..638e8c3d1f4a 100644
>>> --- a/drivers/net/phy/smsc.c
>>> +++ b/drivers/net/phy/smsc.c
>>> @@ -21,6 +21,17 @@
>>> #include <linux/netdevice.h>
>>> #include <linux/smscphy.h>
>>>
>>> +/* Vendor-specific PHY Definitions */
>>> +/* EDPD NLP / crossover time configuration */
>>> +#define PHY_EDPD_CONFIG 16
>>> +#define PHY_EDPD_CONFIG_EXT_CROSSOVER_ 0x0001
>>> +
>>> +/* Control/Status Indication Register */
>>> +#define SPECIAL_CTRL_STS 27
>>> +#define SPECIAL_CTRL_STS_OVRRD_AMDIX_ 0x8000
>>> +#define SPECIAL_CTRL_STS_AMDIX_ENABLE_ 0x4000
>>> +#define SPECIAL_CTRL_STS_AMDIX_STATE_ 0x2000
>>> +
>>> struct smsc_hw_stat {
>>> const char *string;
>>> u8 reg;
>>> @@ -96,6 +107,54 @@ static int lan911x_config_init(struct phy_device *phydev)
>>> return smsc_phy_ack_interrupt(phydev);
>>> }
>>>
>>> +static int lan87xx_config_aneg(struct phy_device *phydev)
>>> +{
>>> + int rc;
>>> + int val;
>>> +
>>> + switch (phydev->mdix_ctrl) {
>>> + case ETH_TP_MDI:
>>> + val = SPECIAL_CTRL_STS_OVRRD_AMDIX_;
>>> + break;
>>> + case ETH_TP_MDI_X:
>>> + val = SPECIAL_CTRL_STS_OVRRD_AMDIX_ |
>>> + SPECIAL_CTRL_STS_AMDIX_STATE_;
>>> + break;
>>> + case ETH_TP_MDI_AUTO:
>>> + val = SPECIAL_CTRL_STS_AMDIX_ENABLE_;
>>> + break;
>>> + default:
>>> + return genphy_config_aneg(phydev);
>>> + }
>>> +
>>> + rc = phy_read(phydev, SPECIAL_CTRL_STS);
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + rc &= ~(SPECIAL_CTRL_STS_OVRRD_AMDIX_ |
>>> + SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
>>> + SPECIAL_CTRL_STS_AMDIX_STATE_);
>>> + rc |= val;
>>> + phy_write(phydev, SPECIAL_CTRL_STS, rc);
>>> +
>>> + phydev->mdix = phydev->mdix_ctrl;
>>> + return genphy_config_aneg(phydev);
>>> +}
>>> +
>>> +static int lan87xx_config_aneg_ext(struct phy_device *phydev)
>>> +{
>>> + int rc;
>>> +
>>> + /* Extend Manual AutoMDIX timer */
>>> + rc = phy_read(phydev, PHY_EDPD_CONFIG);
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + rc |= PHY_EDPD_CONFIG_EXT_CROSSOVER_;
>>> + phy_write(phydev, PHY_EDPD_CONFIG, rc);
>>> + return lan87xx_config_aneg(phydev);
>>> +}
>>> +
>>> /*
>>> * The LAN87xx suffers from rare absence of the ENERGYON-bit when Ethernet cable
>>> * plugs in while LAN87xx is in Energy Detect Power-Down mode. This leads to
>>> @@ -250,6 +309,9 @@ static struct phy_driver smsc_phy_driver[] = {
>>> .suspend = genphy_suspend,
>>> .resume = genphy_resume,
>>> }, {
>>> + /* This covers internal PHY (phy_id: 0x0007C0C3) for
>>> + * LAN9500 (PID: 0x9500), LAN9514 (PID: 0xec00), LAN9505 (PID: 0x9505)
>>> + */
>>> .phy_id = 0x0007c0c0, /* OUI=0x00800f, Model#=0x0c */
>>> .phy_id_mask = 0xfffffff0,
>>> .name = "SMSC LAN8700",
>>> @@ -262,6 +324,7 @@ static struct phy_driver smsc_phy_driver[] = {
>>> .read_status = lan87xx_read_status,
>>> .config_init = smsc_phy_config_init,
>>> .soft_reset = smsc_phy_reset,
>>> + .config_aneg = lan87xx_config_aneg,
>>>
>>> /* IRQ related */
>>> .ack_interrupt = smsc_phy_ack_interrupt,
>>> @@ -293,6 +356,9 @@ static struct phy_driver smsc_phy_driver[] = {
>>> .suspend = genphy_suspend,
>>> .resume = genphy_resume,
>>> }, {
>>> + /* This covers internal PHY (phy_id: 0x0007C0F0) for
>>> + * LAN9500A (PID: 0x9E00), LAN9505A (PID: 0x9E01)
>>> + */
>>> .phy_id = 0x0007c0f0, /* OUI=0x00800f, Model#=0x0f */
>>> .phy_id_mask = 0xfffffff0,
>>> .name = "SMSC LAN8710/LAN8720",
>>> @@ -306,6 +372,7 @@ static struct phy_driver smsc_phy_driver[] = {
>>> .read_status = lan87xx_read_status,
>>> .config_init = smsc_phy_config_init,
>>> .soft_reset = smsc_phy_reset,
>>> + .config_aneg = lan87xx_config_aneg_ext,
>>>
>>> /* IRQ related */
>>> .ack_interrupt = smsc_phy_ack_interrupt,
>>
>> This change seems to be causing some trouble I'm seeing with a LAN8710A.
>> Specifically lan87xx_config_aneg_ext() writes to register 16 which is
>> not documented for LAN8710A (nor for LAN8720A). The effect is somewhat
>> random. Sometimes, the device drops to 10 Mbps while the kernel still
>> reports the link speed as 100 Mbps. Other times, it doesn't work at
>> all. Everything works if I change config_aneg to lan87xx_config_aneg,
>> like this:
>>
>> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
>> index 10722fed666d..07c0a7e4a350 100644
>> --- a/drivers/net/phy/smsc.c
>> +++ b/drivers/net/phy/smsc.c
>> @@ -408,7 +408,7 @@ static struct phy_driver smsc_phy_driver[] = {
>> .read_status = lan87xx_read_status,
>> .config_init = smsc_phy_config_init,
>> .soft_reset = smsc_phy_reset,
>> - .config_aneg = lan87xx_config_aneg_ext,
>> + .config_aneg = lan87xx_config_aneg,
>>
>> /* IRQ related */
>> .ack_interrupt = smsc_phy_ack_interrupt,
>>
>> The internal phy of the LAN9500A does have a register 16 with
>> documentation matching the usage in this patch. Unfortunately, there
>> doesn't seem to be any way of distinguishing this from the LAN8710A
>> based on register values. Anyone got any clever ideas?
>>
> After reading register PHY_EDPD_CONFIG you could check whether the
> read value is plausible. On the PHY's not supporting this register,
> what is the read value? 0x00 or 0xff? And is this value plausible
> for PHY's supporting this register?
On the LAN8710A I have here, it reads as 0x40. That bit is "reserved"
without a specified value on the LAN9500A. On LAN8740A (which also has
a different ID value), it controls Energy Efficient Ethernet
> Currently the PHY driver doesn't check the revision number (last four
> bits of PHY ID). Maybe they differ.
Even if they are different today (I don't seem to have a LAN9500A to
check), nothing guarantees that this will remain the case with future
versions of the chips.
--
Måns Rullgård
Powered by blists - more mailing lists