linux-kernel - Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

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]

Date:	Wed, 21 May 2014 13:05:11 +0300
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Mike Turquette <mturquette@...aro.org>,
	Jin Yao <yao.jin@...ux.intel.com>,
	Li Aubrey <aubrey.li@...ux.intel.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

Hi Rafael,

On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > +#ifdef CONFIG_PM
> 
> It would be good to add a kerneldoc explaining what's being saved here and why.

OK.

> > +static void acpi_lpss_save_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> > +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> > +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
> > +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> > +	}
> > +}
> > +
> > +static void acpi_lpss_restore_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
> > +	msleep(10);
> 
> Two questions here.
> 
> First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> such delays (this is not a PCI device formally).

Unfortunately that is not the case. There is nothing in the AML for
this. Mika, correct me if I'm wrong.

> And because this is not a PCI device formally, why is the comment talking about
> the PCI spec?  Why is PCI relevant in any way here?

Under the hood the devices are still PCI devices, even if they
formally aren't. Maybe I should point that out in the comment..

We put the sleep there because without it there was no guarantee if
the device was properly resumed by the time the drivers resume hooks
were called. The symptom in case of a failure was simply that the
registers could not be written, which leads into timeouts at least in
case of the I2C and UART and making them unusable until the next
suspend followed by resume.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/