netdev - RE: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while using mdio bus

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: <BLUPR03MB373DBCEB67F4E59BF04C9B2F5A00@BLUPR03MB373.namprd03.prod.outlook.com>
Date:	Tue, 23 Jun 2015 03:12:15 +0000
From:	Duan Andy <fugang.duan@...escale.com>
To:	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>
CC:	David Miller <davem@...emloft.net>,
	Cory Tusar <cory.tusar@...1solutions.com>,
	netdev <netdev@...r.kernel.org>
Subject: RE: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
 using mdio bus

From: Andrew Lunn <andrew@...n.ch> Sent: Tuesday, June 23, 2015 10:52 AM
> To: Duan Fugang-B38611; Florian Fainelli
> Cc: David Miller; Cory Tusar; netdev
> Subject: Re: [PATCHv3 net-next] net: fec: Ensure clocks are enabled while
> using mdio bus
> 
> > > int mii_id, int regnum)  {
> > >  	struct fec_enet_private *fep = bus->priv;
> > >  	unsigned long time_left;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(fep->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > >
> > >  	fep->mii_timeout = 0;
> > >  	init_completion(&fep->mdio_done);
> > > @@ -1779,11 +1785,14 @@ static int fec_enet_mdio_read(struct mii_bus
> > > *bus, int mii_id, int regnum)
> > >  	if (time_left == 0) {
> > >  		fep->mii_timeout = 1;
> > >  		netdev_err(fep->netdev, "MDIO read timeout\n");
> > > +		clk_disable_unprepare(fep->clk_ipg);
> > >  		return -ETIMEDOUT;
> > >  	}
> > >
> > > -	/* return value */
> > > -	return FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > +	ret = FEC_MMFR_DATA(readl(fep->hwp + FEC_MII_DATA));
> > > +	clk_disable_unprepare(fep->clk_ipg);
> > > +
> > > +	return ret;
> > >  }
> > >
> >
> 
> > I suggest you use runtime pm to enable/disable clock for performance
> > consideration. Not every time for mdio bus access needs to
> > enable/disable clock.
> 
> Can you explain that some more. When are you suggesting doing a runtime
> enable/disable? Given the current DSA architecture, i would probably do a
> runtime enable as soon as i lookup the mdio bus, and never do a runtime
> disable. Also, how would you include this runtime pm into the phy state
> machine which is going to be polling your mdio bus around once per second
> for normal phys?
> 

You can do it like this:

#define FEC_MDIO_PM_TIMEOUT  100 /* ms */

static int fec_probe(struct platform_device *pdev)
{
	...
	pm_runtime_enable(&pdev->dev);
	pm_runtime_set_autosuspend_delay(&pdev->dev, FEC_MDIO_PM_TIMEOUT);
	pm_runtime_use_autosuspend(&pdev->dev);
	...
}


static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
{
	...
	pm_runtime_get_sync(dev);

	...

	pm_runtime_mark_last_busy(dev);
	pm_runtime_put_autosuspend(dev);
}


> Florian, as Maintainer of the phy state machine, what do you think about
> adding runtime PM to it?
> 
>   Thanks
> 	Andrew
--
To unsubscribe from this list: send the line "unsubscribe netdev" in