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] [day] [month] [year] [list]

Message-ID: <20160502193804.GD31001@jcartwri.amer.corp.natinst.com>
Date:	Mon, 2 May 2016 14:38:04 -0500
From:	Josh Cartwright <joshc@...com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	Nicolas Ferre <nicolas.ferre@...el.com>,
	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 Mon, May 02, 2016 at 12:08:50PM -0700, Florian Fainelli wrote:
> 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.

I can understand how that might have been a long term goal of managing a
reset GPIO in general, however as it stands in the macb driver the only
callsite where the reset gpio is tweaked macb_remove().

> > 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

Cool, thanks.  I was about to see about implementing this...but since
it's already been done, I'll rebase my set on Sergei's changes.

Thanks,
  Josh

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)