netdev - RE: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MWHPR11MB16939B94E6A64234CA14167DEF7A9@MWHPR11MB1693.namprd11.prod.outlook.com>
Date:   Fri, 2 Sep 2022 20:10:46 +0000
From:   <Jerry.Ray@...rochip.com>
To:     <olteanv@...il.com>
CC:     <andrew@...n.ch>, <vivien.didelot@...il.com>,
        <f.fainelli@...il.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354

>On Mon, Aug 29, 2022 at 01:00:36PM -0500, Jerry Ray wrote:
>> Add initial BYTE_ORDER read to sync to improve driver robustness
>
>Please don't post 2 different patches with the same commit message.
>I think here, the first paragraph is what the commit message should actually be.
>

Understood.

>>
>> The lan9303 expects two mdio read transactions back-to-back to read a 
>> 32-bit register. The first read transaction causes the other half of 
>> the 32-bit register to get latched.  The subsequent read returns the 
>> latched second half of the 32-bit read. The BYTE_ORDER register is an 
>> exception to this rule. As it is a constant value, there is no need to 
>> latch the second half. We read this register first in case there were 
>> reads during the boot loader process that might have occurred prior to 
>> this driver taking over ownership of accessing this device.
>>
>> This patch has been tested on the SAMA5D3-EDS with a LAN9303 RMII 
>> daughter card.
>
>Is this patch fixing a problem for any existing platforms supported by this driver?
>

This patch is fixing a problem I ran into that probably doesn't occur under
normal use case conditions.  I was probing around on the mdio bus within
u-boot, then booting linux.  This change makes the linux driver more robust
(tolerant) to this situation.

>>
>> Signed-off-by: Jerry Ray <jerry.ray@...rochip.com>
>> ---
>>  drivers/net/dsa/lan9303-core.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c 
>> b/drivers/net/dsa/lan9303-core.c index e03ff1f267bb..17ae02a56bfe 
>> 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -32,6 +32,7 @@
>>  #define LAN9303_INT_EN 0x17
>>  # define LAN9303_INT_EN_PHY_INT2_EN BIT(27)  # define 
>> LAN9303_INT_EN_PHY_INT1_EN BIT(26)
>> +#define LAN9303_BYTE_ORDER 0x19
>>  #define LAN9303_HW_CFG 0x1D
>>  # define LAN9303_HW_CFG_READY BIT(27)  # define 
>> LAN9303_HW_CFG_AMDX_EN_PORT2 BIT(26) @@ -847,9 +848,10 @@ static int 
>> lan9303_check_device(struct lan9303 *chip)
>>       int ret;
>>       u32 reg;
>>
>> -     ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>> +     // Dummy read to ensure MDIO access is in 32-bit sync.
>
>C-style comments /* */ are more typical in the Linux kernel coding style.
>

Understood.

>> +     ret = lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);
>
>Pretty strange to see the dummy read in lan9303_check_device().
>Bootloader leaving things in a messy state is only a problem if we don't have a reset GPIO, right?
>
>How about introducing the logic here, right in lan9303_probe():
>
>        lan9303_handle_reset(chip);
>
>        if (!chip->reset_gpio) {
>                /* Dummy read to ensure MDIO access is in 32-bit sync. */
>                ret = lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);
>                if (ret) {
>                        dev_err(chip->dev, "failed to access the device: %pe\n",
>                                ERR_PTR(ret));
>                        return ret;
>                }
>        }
>
>        ret = lan9303_check_device(chip);
>

I'll look to move it.

>>       if (ret) {
>> -             dev_err(chip->dev, "failed to read chip revision register: %d\n",
>> +             dev_err(chip->dev, "failed to access the device: %d\n",
>>                       ret);
>>               if (!chip->reset_gpio) {
>>                       dev_dbg(chip->dev,
>
>The context here reads:
>                if (!chip->reset_gpio) {
>                        dev_dbg(chip->dev,
>                                "hint: maybe failed due to missing reset GPIO\n");
>                }
>
>Is the comment still accurate after the change, or do you feel that it can be removed? Looks like you are fixing a known issue.
>

The 'hint' message applies to the first chip access, which has now changed to the BYTE_ORDER read.

>> @@ -858,6 +860,13 @@ static int lan9303_check_device(struct lan9303 *chip)
>>               return ret;
>>       }
>>
>> +     ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>> +     if (ret) {
>> +             dev_err(chip->dev, "failed to read chip revision register: %d\n",
>> +                     ret);
>> +             return ret;
>> +     }
>> +
>>       if ((reg >> 16) != LAN9303_CHIP_ID) {
>>               dev_err(chip->dev, "expecting LAN9303 chip, but found: %X\n",
>>                       reg >> 16);
>> --
>> 2.17.1
>>

Thanks,
Jerry.