netdev - RE: [PATCH 1/3] net: macb: fix for fixed-link mode

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: <BY5PR07MB65144B9F265EDF74FB7BB621D3470@BY5PR07MB6514.namprd07.prod.outlook.com>
Date:   Thu, 28 Nov 2019 06:50:10 +0000
From:   Milind Parab <mparab@...ence.com>
To:     "Nicolas.Ferre@...rochip.com" <Nicolas.Ferre@...rochip.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "antoine.tenart@...tlin.com" <antoine.tenart@...tlin.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Dhananjay Vilasrao Kangude <dkangude@...ence.com>,
        Parshuram Raju Thombare <pthombar@...ence.com>,
        "a.fatoum@...gutronix.de" <a.fatoum@...gutronix.de>,
        "brad.mouring@...com" <brad.mouring@...com>,
        "rmk+kernel@...linux.org.uk" <rmk+kernel@...linux.org.uk>
Subject: RE: [PATCH 1/3] net: macb: fix for fixed-link mode


>-----Original Message-----
>From: Nicolas.Ferre@...rochip.com <Nicolas.Ferre@...rochip.com>
>Sent: Wednesday, November 27, 2019 11:32 PM
>To: Milind Parab <mparab@...ence.com>; andrew@...n.ch;
>antoine.tenart@...tlin.com; f.fainelli@...il.com
>Cc: davem@...emloft.net; netdev@...r.kernel.org; hkallweit1@...il.com;
>linux-kernel@...r.kernel.org; Dhananjay Vilasrao Kangude
><dkangude@...ence.com>; Parshuram Raju Thombare
><pthombar@...ence.com>; a.fatoum@...gutronix.de;
>brad.mouring@...com; rmk+kernel@...linux.org.uk
>Subject: Re: [PATCH 1/3] net: macb: fix for fixed-link mode
>
>EXTERNAL MAIL
>
>
>On 26/11/2019 at 10:09, Milind Parab wrote:
>> This patch fix the issue with fixed link mode in macb.
>
>I would need more context here. What needs to be fixed?
>
>I think we had several attempts, at the phylib days, to have this part of the
>driver behave correctly, so providing us more insight will help understand
>what is going wrong now.
>For instance, is it related to the patch that converts the driver to the phylink
>interface done by this patch in net-next "net: macb: convert to phylink"?
>
Yes, this is related to the patch that converts the driver to phylink
With phylink patch, in fixed-link the device open is failing. The reason for failure is because
here an attempt is made to search and connect to PHY even for the  fixed-link.
phylink_of_phy_connect() handles this case well, and for the fixed-link it just returns 0. 
So, further steps to search and connect to PHY are not needed. 
This patch solves this problem by allowing phylink_of_phy_connect() to take this decision

>
>> Signed-off-by: Milind Parab <mparab@...ence.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index d5ae2e1e0b0e..5e6d27d33d43 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp)
>>          struct phy_device *phydev;
>>          int ret;
>>
>> -       if (bp->pdev->dev.of_node &&
>> -           of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) {
>
>You mean we don't need to parse this phandle anymore because it's better
>handled by phylink_of_phy_connect() below that takes care of the fixed-link
>case?
>If yes, then telling it in commit message is worth it...

Yes, this we will explain in the commit message

>
>> +       if (bp->pdev->dev.of_node)
>>                  ret = phylink_of_phy_connect(bp->phylink, bp->pdev-
>>dev.of_node,
>>                                               0);
>> -               if (ret) {
>> -                       netdev_err(dev, "Could not attach PHY (%d)\n", ret);
>> -                       return ret;
>> -               }
>> -       } else {
>> +
>> +       if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus)
>> + {
>>                  phydev = phy_find_first(bp->mii_bus);
>>                  if (!phydev) {
>>                          netdev_err(dev, "no PHY found\n"); @@ -635,7
>> +631,7 @@ static int macb_phylink_connect(struct macb *bp)
>>                  /* attach the mac to the phy */
>>                  ret = phylink_connect_phy(bp->phylink, phydev);
>>                  if (ret) {
>> -                       netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
>> +                       netdev_err(dev, "Could not attach to PHY\n");
>
>Why modifying this?

This is by mistake. This will be corrected in the revision patch

>
>>                          return ret;
>>                  }
>>          }
>> --
>> 2.17.1
>>
>
>Best regards,
>   Nicolas
>
>--
>Nicolas Ferre