Re: [RFC PATCH v3 3/3] acpi_memhotplug: Allow eject to proceed on rebind scenario

Hi,

On Wed, Nov 28, 2012 at 06:15:42PM -0700, Toshi Kani wrote:

On Wed, 2012-11-28 at 18:02 -0700, Toshi Kani wrote:

quoted

On Thu, 2012-11-29 at 00:49 +0100, Rafael J. Wysocki wrote:

quoted

On Wednesday, November 28, 2012 02:02:48 PM Toshi Kani wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Consider the following case:

We hotremove the memory device by SCI and unbind it from the driver at the same time:

CPUa                                                  CPUb
acpi_memory_device_notify()
                                       unbind it from the driver
    acpi_bus_hot_remove_device()

I see two reasons for calling acpi_bus_hot_remove_device() for memory (correct
me if I'm wrong): (1) from the memhotplug driver's notify handler and (2) from
acpi_eject_store() which is exposed through sysfs.  

Yes, that is correct.

quoted

If we disabled exposing
acpi_eject_store() for memory devices, then the only way would be from the
notify handler.  So I wonder if driver_unbind() shouldn't just uninstall the
notify handler for memory (so that memory eject events are simply dropped on
the floor after unbinding the driver)?

If driver_unbind() happens before an eject request, we do not have a
problem.  acpi_eject_store() fails if a driver is not bound to the
device.  acpi_memory_device_notify() fails as well.

The race condition Wen pointed out (see the top of this email) is that
driver_unbind() may come in while eject operation is in-progress.  This
is why I mentioned the following in previous email.

quoted

So, we basically need to either 1) serialize
acpi_bus_hot_remove_device() and driver_unbind(), or 2) make
acpi_bus_hot_remove_device() to fail if driver_unbind() is run
during the operation.

Forgot to mention.  The 3rd option is what Greg said -- use the
suppress_bind_attrs field.  I think this is a good option to address
this race condition for now.  For a long term solution, we should have a
better infrastructure in place to address such issue in general.

I like the suppress_bind_attrs idea, I 'll take a look.

As I said for option 2), acpi_bus_remove could check for driver presence.
But It's more a quick hack to abort the eject (the race with unbind can still
happen, but acpi_bus_remove can now detect it later in the eject path).
Something like:

 static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 {
+	int ret;
 	if (!dev)
 		return -EINVAL;
 
 	dev->removal_type = ACPI_BUS_REMOVAL_EJECT;
+
+	if (dev->driver && dev->driver->ops.prepare_remove) {
+		ret = dev->driver->ops.prepare_remove(dev);
+		if (ret)
+			return ret;
+	}
+	else if (!dev->driver)
+		return -ENODEV;
 	device_release_driver(&dev->dev);

thanks,

- Vasilis

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>