linux-kernel - Re: [PATCH] PNPACPI: Enable Power Management

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: <200812072129.29954.bjorn.helgaas@hp.com>
Date:	Sun, 7 Dec 2008 21:29:29 -0700
From:	Bjorn Helgaas <bjorn.helgaas@...com>
To:	Witold Szczeponik <Witold.Szczeponik@....net>
Cc:	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Adam Belay <abelay@....edu>, rjw@...k.pl
Subject: Re: [PATCH] PNPACPI: Enable Power Management

On Sunday 07 December 2008 5:41:43 am Witold Szczeponik wrote:
> Bjorn Helgaas wrote:
> > Do you know of anything that specifies the order of the _CRS/_PS0
> > and the _PS3/_DIS evaluation?  I don't know much about power
> > management, and I couldn't find anything obvious in the spec.
> > It seems plausible that we should run _CRS before turning on
> > the power, but I really don't know.
>
> There's some info the ACPI Specification that says a device needs
> to be put to D3 before _DIS is called (section 6.2.2) but there
> is no clear statement as to when to put a device to D0... :-(
>
> I think it should be _SRS/_PS0 and _PS3/_DIS.

Thanks for the pointer.  That makes sense, and your proposed order
makes the enable/disable paths symmetric, so I think you're right.
Can you put that spec pointer in your changelog?

> > Is pnpacpi_set_resources() the only place that needs this change?
> > For active devices, we normally don't call pnpacpi_set_resources()
> > at all.  So I suppose on these ThinkPads, we exercise this path
> > because the serial ports are initially disabled?
>
> I'm not sure.  I would guess that we need to put any device that is
> enabled (either via _SRS or by default) into D0.  My patch does that
> for the _SRS case but the generic ACPI code does not.  On my 600E,
> the serial port has power when the machine is booted but has no power
> once GRUB kicks in.  It remains in this state until the 8250-pnp module
> gets loaded, where my patch enables it.

Interesting.  I'd be surprised if GRUB does anything with ACPI to
disable the port.  I don't know how you determine when the port has
power.  Maybe the power-up default state is powered, and the BIOS
turns it off before launching GRUB?

I wonder if _STA is influenced by the power state.  I would think
if _STA said a device was "enabled and decoding resources," that
would only make sense if the device were already in D0.  But let's
say we start with an active device, then run _PS3 and _DIS.  What
would _STA say in the interval between _PS3 and _DIS?

This is just idle curiousity on my part, I guess.  I just don't
know much about ACPI power states, and I'm afraid of getting in
trouble if we change too much.  The _SRS path is a little less
scary because it won't be exercised much (most devices are already
enabled, and we don't change their settings).

> The ACPI Specification says for _SRS: the device is enabled after the
> method has returned.  There is no statement as to when to call _PS0...

> >>   static int pnpacpi_disable_resources(struct pnp_dev *dev)
> >>   {
> >> +	acpi_handle handle = dev->data;
> >> +	int ret = 0;
> >>   	acpi_status status;
> >>
> >> -	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
> >
> > Can you leave the "unregister_gsi" comment there, since it's not
> > related to your patch?  It's a reminder that we need to think about
> > how to handle interrupts when enabling/disabling devices.
>
> I'd rather remove the comment as it is misleading, IMHO.  This call
> should be made by a driver.  After all, the PNPACPI core does not
> register any IRQs, either.  Otherwise, we need to think about
> "pnp_irq(dev, 1)", too.  Either way, with my patch there should be
> no IRQ handling possible.

The comment is logically unrelated to the rest of your patch, so I
would at least split it into a separate patch just on that grounds.

The PNP core *does* register IRQs: we call acpi_register_gsi()
in pnpacpi_parse_allocated_irqresource(), but there's no
corresponding unregister.  I think this asymmetry is a bug,
but I haven't looked at how to handle it yet.  PNP currently
does the registration "eagerly," when the device is discovered.
I think we should instead do the registration/unregistration
"lazily," when a driver claims the device, as PCI does.

I haven't changed this yet because there are some issues related to
pcibios_penalize_isa_irq() -- we don't know the IRQ to penalize until
we register the GSI.

Anyway, that's a long-winded explanation of why that stupid-looking
comment still has some value to me :-)

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