netdev - Re: [PATCH v2] net: macb: do not scan PHYs manually

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: <5727A5C2.804@gmail.com>
Date:	Mon, 2 May 2016 12:08:50 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Josh Cartwright <joshc@...com>,
	Nicolas Ferre <nicolas.ferre@...el.com>
Cc:	Andrew Lunn <andrew@...n.ch>,
	Nathan Sullivan <nathan.sullivan@...com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
Subject: Re: [PATCH v2] net: macb: do not scan PHYs manually

On 02/05/16 11:36, Josh Cartwright wrote:
> On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
> [..]
>>>  static int macb_mii_init(struct macb *bp)
>>>  {
>>>  	struct macb_platform_data *pdata;
>>>  	struct device_node *np;
>>> -	int err = -ENXIO, i;
>>> +	int err = -ENXIO;
>>>  
>>>  	/* Enable management port */
>>>  	macb_writel(bp, NCR, MACB_BIT(MPE));
>>> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
>>>  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>>>  
>>>  	np = bp->pdev->dev.of_node;
>>> -	if (np) {
>>> -		/* try dt phy registration */
>>> -		err = of_mdiobus_register(bp->mii_bus, np);
>>> -
>>> -		/* fallback to standard phy registration if no phy were
>>> -		 * found during dt phy registration
>>> -		 */
>>> -		if (!err && !phy_find_first(bp->mii_bus)) {
>>> -			for (i = 0; i < PHY_MAX_ADDR; i++) {
>>> -				struct phy_device *phydev;
>>> -
>>> -				phydev = mdiobus_scan(bp->mii_bus, i);
>>> -				if (IS_ERR(phydev)) {
>>> -					err = PTR_ERR(phydev);
>>> -					break;
>>> -				}
>>> -			}
>>> -
>>> -			if (err)
>>> -				goto err_out_unregister_bus;
>>> -		}
>>> -	} else {
>>> -		if (pdata)
>>> -			bp->mii_bus->phy_mask = pdata->phy_mask;
>>> -
>>> -		err = mdiobus_register(bp->mii_bus);
>>> -	}
>>> +	if (np)
>>> +		err = macb_mii_of_init(bp, np);
>>> +	else
>>> +		err = macb_mii_pdata_init(bp, pdata);
>>>  
>>>  	if (err)
>>>  		goto err_out_free_mdiobus;
>>
>> I'm okay with this. Thanks for having taken the initiative to implement it.
> 
> Unfortunately, I don't think it's going to be as straightforward
> as I originally thought.  Still doable, but more complicated.
> 
> In particular, the macb bindings allow for a user to specify a
> 'reset-gpios' property _at the PHY_ level, which is consumed by the
> macb to adjust the PHY reset state on remove.

In fact, not just on remove, anytime there is an opportunity to save
power (interface down, closed) and putting the PHY into reset is usually
guaranteed to be saving more power than e.g: a BMCR power down.

> 
> My question is: why is the PHY reset GPIO management not the
> responsibility of the PHY driver/core itself?

Well, this is actually being worked on at the moment by Sergei, since
there is not necessarily a reason why PHYLIB can't deal with that:

https://lkml.org/lkml/2016/4/28/831
-- 
Florian