| 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
| ||
|
Date: Fri, 22 Feb 2013 21:59:33 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: Toshi Kani <toshi.kani@...com>
Cc: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
LKML <linux-kernel@...r.kernel.org>,
Yinghai Lu <yinghai@...nel.org>, Jiang Liu <liuj97@...il.com>
Subject: Re: [Update 3][PATCH 2/7] ACPI / scan: Introduce common code for ACPI-based device hotplug
On Friday, February 22, 2013 08:54:14 AM Toshi Kani wrote:
> On Fri, 2013-02-22 at 13:37 +0100, Rafael J. Wysocki wrote:
> > On Friday, February 22, 2013 05:51:28 PM Yasuaki Ishimatsu wrote:
> > > 2013/02/22 10:50, Rafael J. Wysocki wrote:
> > > > On Thursday, February 21, 2013 06:12:21 PM Toshi Kani wrote:
> > > >> On Fri, 2013-02-22 at 00:06 +0100, Rafael J. Wysocki wrote:
> > > >>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > >>>
> > > >>> Multiple drivers handling hotplug-capable ACPI device nodes install
> > > >>> notify handlers covering the same types of events in a very similar
> > > >>> way. Moreover, those handlers are installed in separate namespace
> > > >>> walks, although that really should be done during namespace scans
> > > >>> carried out by acpi_bus_scan(). This leads to substantial code
> > > >>> duplication, unnecessary overhead and behavior that is hard to
> > > >>> follow.
> > > >>>
> > > >>> For this reason, introduce common code in drivers/acpi/scan.c for
> > > >>> handling hotplug-related notification and carrying out device
> > > >>> insertion and eject operations in a generic fashion, such that it
> > > >>> may be used by all of the relevant drivers in the future. To cover
> > > >>> the existing differences between those drivers introduce struct
> > > >>> acpi_hotplug_profile for representing collections of hotplug
> > > >>> settings associated with different ACPI scan handlers that can be
> > > >>> used by the drivers to make the common code reflect their current
> > > >>> behavior.
> > > >>>
> > > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > >>> ---
> > > >>>
> > > >>> This update fixes an issue pointed out by Toshi Kani (that
> > > >>> ACPI_OST_EC_OSPM_* event source codes should not be used with _OST for events
> > > >>> that we received a notification for from the platform firmware).
> > > >>>
> > > >>> Thanks,
> > > >>> Rafael
> > > >>>
> > > >>> ---
> > > >>> drivers/acpi/scan.c | 270 ++++++++++++++++++++++++++++++++++++++----------
> > > >>> include/acpi/acpi_bus.h | 7 +
> > > >>> 2 files changed, 224 insertions(+), 53 deletions(-)
> > > >> :
> > > >>> +static void acpi_bus_device_eject(void *context)
> > > >>> +{
> > > >>> + acpi_handle handle = context;
> > > >>> + struct acpi_device *device = NULL;
> > > >>> + struct acpi_scan_handler *handler;
> > > >>> + u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
> > > >>> +
> > > >>> + mutex_lock(&acpi_scan_lock);
> > > >>> +
> > > >>> + acpi_bus_get_device(handle, &device);
> > > >>> + if (!device)
> > > >>> + goto err_out;
> > > >>> +
> > > >>> + handler = device->handler;
> > > >>> + if (!handler || !handler->hotplug.enabled) {
> > > >>> + ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
> > > >>> + goto err_out;
> > > >>> + }
> > > >>> + acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
> > > >>> + ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
> > > >>> + if (handler->hotplug.autoeject) {
> > > >>> + int error;
> > > >>> +
> > > >>> + get_device(&device->dev);
> > > >>> + error = acpi_scan_hot_remove(device);
> > > >>> + if (error)
> > > >>> + goto err_out;
> > > >>> + } else {
> > > >>> + device->flags.eject_pending = true;
> > > >>> }
> > > >>> + if (handler->hotplug.uevents)
> > > >>> + kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> > > >>
> > > >> I confirmed that the _OST issue with hot-add is fixed. Here is a new
> > > >> one. When autoeject is enabled, it crashes in kobject_uevent() since
> > > >> the device is no longer valid.
> > > >
> > > > Well, this one is more difficult.
> > > >
> > > > I can change the ordering so that kobject_uevent() is called before
> > > > acpi_scan_hot_remove(), but then user space may not know that the device is
> > > > being removed at the moment (which still may fail). Still, maybe this is
> > > > OK, because user space will get KOBJ_REMOVE when the device actually goes
> > > > away anyway.
> > > >
> > > > Or perhaps we can emit KOBJ_OFFLINE before acpi_scan_hot_remove() and if
> > > > it fails, emit KOBJ_ONLINE?
> > >
> > > How about following patch? My system cannot send EJECT notification.
> > > So I have not tested this patch.
> >
> > No, that's not correct, acpi_scan_hot_remove(device) will remove the device
> > from sysfs, if successful, among other things.
> >
> > We can't emit uevents for a device that has been, even though the data
> > structure is still around.
> >
> > The following are the choices we have, in my opinion:
> > - Emit KOBJ_OFFLINE before removal.
> > - Emit KOBJ_OFFLINE before removal and KOBJ_ONLINE afterwards if it fails.
> > - Do not emit KOBJ_OFFLINE at all with autoeject.
> >
> > Each of them has some disadvantages, so I'm not sure. The last one is the
> > easiest, so I'll probably send another update implementing it.
>
> I agree with the 3rd option. KOBJ_REMOVE is emitted when a device is
> removed, so it should be OK to not emitting KOBJ_OFFLINE here. Besides,
> we are going to rely on a target offlined beforehand, so this code path
> won't have to do it anyway.
Yeah.
OK, I'll update the patch this way, thanks!
> BTW, it appears that KOBJ_OFFLINE is used for two different purposes
> today. sysfs cpu/memory emits KOBJ_OFFLINE when a target has been
> offlined. The container driver (or autoeject is not set with this
> patch) emits KOBJ_OFFLINE to request a user to offline a target.
> Expected action from user space is different from these cases.
What a mess. Well, we'll need to resolve it somehow, probably by special
casing the processor driver, but let's do that later.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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/
Powered by blists - more mailing lists