netdev - Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2

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: <8b9c187f-cc55-248e-ca03-794523ab202f@synopsys.com>
Date:   Tue, 7 Aug 2018 16:29:43 +0100
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Jose Abreu <Jose.Abreu@...opsys.com>, <netdev@...r.kernel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions
 for XGMAC2

On 07-08-2018 14:35, Jose Abreu wrote:
> Hi Florian,
>
> On 06-08-2018 16:25, Florian Fainelli wrote:
>> On August 6, 2018 12:59:54 AM PDT, Jose Abreu <Jose.Abreu@...opsys.com> wrote:
>>> On 03-08-2018 20:06, Florian Fainelli wrote:
>>>> On 08/03/2018 08:50 AM, Jose Abreu wrote:
>>>>> Add the MDIO related funcionalities for the new IP block XGMAC2.
>>>>>
>>>>> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
>>>>> Cc: David S. Miller <davem@...emloft.net>
>>>>> Cc: Joao Pinto <jpinto@...opsys.com>
>>>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>
>>>>> Cc: Alexandre Torgue <alexandre.torgue@...com>
>>>>> Cc: Andrew Lunn <andrew@...n.ch>
>>>>> ---
>>>>> +satic int stmmac_xgmac2_c22_format(struct stmmac_priv *priv, int
>>> phyaddr,
>>>>> +				    int phyreg, u32 *hw_addr)
>>>>> +{
>>>>> +	unsigned int mii_data = priv->hw->mii.data;
>>>>> +	u32 tmp;
>>>>> +
>>>>> +	/* HW does not support C22 addr >= 4 */
>>>>> +	if (phyaddr >= 4)
>>>>> +		return -ENODEV;
>>>> It would be nice if this could be moved at probe time so you don't
>>> have
>>>> to wait until you connect to the PHY, read its PHY OUI and find out
>>> it
>>>> has a MDIO address >= 4. Not a blocker, but something that could be
>>>> improved further on.
>>>>
>>>> In premise you could even scan the MDIO bus' device tree node, and
>>> find
>>>> that out ahead of time.
>>> Oh, but I use phylib ... I only provide the read/write callbacks
>>> so I think we should avoid duplicating the code that's already in
>>> the phylib ... No?
>> You can always extract the code that scans a MDIO bus into a helper function and make it parametrized with a callback of some kind. In that case I would be fine with you open coding the MDIO bus scan to find out if there is an address >= 4.
> Sorry but I dont' think thats the best solution because
> of_mdiobus_register() already scans the bus. 

My mistake. Its stmmac thats scanning the bus. See this:

359         for (addr = 0; addr < PHY_MAX_ADDR; addr++)
{                          
360                 struct phy_device *phydev =
mdiobus_get_phy(new_bus, addr);    
361                                                                                

362                 if
(!phydev)                                                   
363                        
continue;                                              

So, its just cycling through all possible phys (0..31) ... Do you
think it would be okay if I just limit the cycle max to 4 for
this xgmac2 for now ? I would maintain the if in  the
stmmac_xgmac2_c22_format though, as safe-guard.

Thanks and Best Regards,
Jose Miguel Abreu

> Duplicating this
> should be avoided, no?
>
> Note all of this is probably never needed because stmmac just
> picks the first phy it finds, if phy_addr is not specified ...
>
>>>>> +	/* Wait until any existing MII operation is complete */
>>>>> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
>>>>> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
>>>>> +		return -EBUSY;
>>>>> +
>>>>> +	/* Set port as Clause 22 */
>>>>> +	tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P);
>>>>> +	tmp |= BIT(phyaddr);
>>>> Since the registers are being Read/Modify/Write here, don't you need
>>> to
>>>> clear the previous address bits as well?
>>>>
>>>> You probably did not encounter any problems in your testing if you
>>> had
>>>> only one PHY on the MDIO bus, but this is not something that is
>>>> necessarily true, e.g: if you have an Ethernet switch, several MDIO
>>> bus
>>>> addresses are going to be responding.
>>>>
>>>> Your MDIO bus implementation must be able to support one transaction
>>>> with one PHY address and the next transaction with another PHY
>>> address ,
>>>> etc...
>>>>
>>>> That is something that should be easy to fix and be resubmitted as
>>> part
>>>> of v4.
>>> I'm not following you here... I only set/unset the bit for the
>>> corresponding phyaddr that phylib wants to read/write. Why would
>>> I clear the remaining addresses?
>> Because this is all about transactions, the HW must be in a state that it will be able to perform that transaction correctly. So here for instance if you needed to support a C45 transaction you would have to clear that bit for that particular PHY address. Since you don't appear to support those yet then yes the code appears fine though it would not hurt if you did clear all other PHY's c22 bits to make it clear what this does.
> I can't test C45 right now but I will in a near future. In that
> case then we will need to support C22 and C45 so I may want to
> only set one bit for a specific phy that only supports c22.
>
> Thanks and Best Regards,
> Jose Miguel Abreu
>