| 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: <52da47b4-109a-dc4a-0fd4-023580fe86d4@gmail.com>
Date: Sun, 28 Mar 2021 22:31:13 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Måns Rullgård <mans@...sr.com>,
Andre Edich <andre.edich@...rochip.com>
Cc: 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
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?
Currently the PHY driver doesn't check the revision number
(last four bits of PHY ID). Maybe they differ.
Powered by blists - more mailing lists