linux-kernel - Re: [PATCH 1/2] ACPI: reintroduce acpi_device

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, 1 Jul 2009 10:20:59 +0200 (CEST)
From:	David Härdeman <david@...deman.nu>
To:	"Bjorn Helgaas" <bjorn.helgaas@...com>
Cc:	"Andrew Morton" <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	linux-input@...r.kernel.org, jbarnes@...tuousgeek.org,
	"Len Brown" <lenb@...nel.org>
Subject: Re: [PATCH 1/2] ACPI: reintroduce acpi_device_ops .shutdown method

On Wed, July 1, 2009 01:11, Bjorn Helgaas wrote:
> On Tuesday 30 June 2009 3:10:47 pm Andrew Morton wrote:
>> On Sat, 27 Jun 2009 07:18:31 +0200
>> David Härdeman <david@...deman.nu> wrote:
>>>
>>> This reintroduces the .shutdown method which is used by the
>>> winbond-cir driver. A normal revert wasn't possible since there
>>> had been other changes to include/acpi/acpi_bus.h since.
>>
>> Len, Bjorn: is this OK?  Or is there some other mechanism which the
>> driver should have used?
>
> I'm on vacation and don't have time to read the new winbond driver
> right now, but maybe it could be changed so that wbcir_shutdown()
> is an internal function called by wbcir_suspend() (as it is already)
> and wbcir_remove().

I didn't want to call wbcir_shutdown from wbcir_remove since wbcir_remove
is called on rmmod and the wbcir_shutdown method programs the chip so that
wake-on-a-specific-ir-command is enabled (and I imagine that people would
expect rmmod to disable the hardware completely). I will clarify the
function names a bit in the next version of the patch...

As far as I could understand from my testing, the acpi .suspend method is
not called during shutdown which is why I needed to hook into both
.suspend and .shutdown separately.

> I hate to re-introduce .shutdown when it's only used by a single
> driver.  That makes me think either we have a bunch of drivers that
> are buggy because they *should* have .shutdown methods but don't,
> or the single user of .shutdown doesn't have a real dependency on it.

I think this single user does have a real dependency because of its
wake-from-poweroff and wake-from-suspend capability. But I could be
mistaken...

> The winbond driver does not use any ACPI-specific functionality, so
> it might be simpler to write it as a PNP driver (which would depend
> on PNPACPI, of course).

As far as I could tell from a quick look at include/linux/pnp.h, a
pnp_driver doesn't seem to have any .shutdown methods either, so I'm not
sure how it would help?

(On a related note, it seems inconsistent to me that platform_driver has a
.shutdown method while pnp_driver and acpi_driver doesn't.)

If you disagree with acpi_driver regaining the .shutdown method I guess
the only options are to use register_reboot_notifier or to rewrite the
driver as a platform_driver (Alan Cox seemed to suggest earlier that the
driver was not a good fit for a platform driver).

> Nice looking driver, by the way.  Even from a cursory glance it's
> obvious that you've taken a lot of care with it.

Thank you, and have a nice vacation.

Regards,
David

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