| Message ID | 20121206093019.GA4584@dhcp-192-168-178-175.profitbricks.localdomain (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thursday, December 06, 2012 10:30:19 AM Vasilis Liaskovitis wrote: > Hi, > On Thu, Nov 29, 2012 at 10:44:11AM -0700, Toshi Kani wrote: > > On Thu, 2012-11-29 at 12:04 +0100, Vasilis Liaskovitis wrote: > > > > Yes, that's what I had in mind along with device_lock(). I think the > > lock is necessary to close the window. > > http://www.spinics.net/lists/linux-mm/msg46973.html > > > > But as I mentioned in other email, I prefer option 3 with > > suppress_bind_attrs. So, yes, please take a look to see how it works > > out. > > I tested the suppress_bind_attrs and it works by simply setting it to true > before driver registration e.g. > > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -783,7 +783,8 @@ int acpi_bus_register_driver(struct acpi_driver *driver) > driver->drv.name = driver->name; > driver->drv.bus = &acpi_bus_type; > driver->drv.owner = driver->owner; > - > + if (!strcmp(driver->class, "memory")) > + driver->drv.suppress_bind_attrs = true; > ret = driver_register(&driver->drv); > return ret; > } > > No bind/unbind sysfs files are created when using this, as expected. > I assume we only want to suppress for acpi_memhotplug > (class=ACPI_MEMORY_DEVICE_CLASS i.e. "memory") devices. > > Is there agreement on what acpi_bus_trim behaviour and rollback (if any) we > want to have for the current ACPI framework (partial trim or full trim on > failure)? Last time I suggested to split the trimming so that first we only unbind drivers (and roll back that part, ie. rebind the drivers on errors) and next we remove the struct acpi_device objects, just before doing the actual eject. So there would be two walks of the hierarchy below the device we want to eject, one for driver unbinding (that can be rolled back) and one for the actual removal. Toshi Kani seemed to agree with that and there were no follow-ups. Thanks, Rafael
On Thu, 2012-12-06 at 13:50 +0100, Rafael J. Wysocki wrote: > On Thursday, December 06, 2012 10:30:19 AM Vasilis Liaskovitis wrote: > > Hi, > > On Thu, Nov 29, 2012 at 10:44:11AM -0700, Toshi Kani wrote: > > > On Thu, 2012-11-29 at 12:04 +0100, Vasilis Liaskovitis wrote: > > > > > > Yes, that's what I had in mind along with device_lock(). I think the > > > lock is necessary to close the window. > > > http://www.spinics.net/lists/linux-mm/msg46973.html > > > > > > But as I mentioned in other email, I prefer option 3 with > > > suppress_bind_attrs. So, yes, please take a look to see how it works > > > out. > > > > I tested the suppress_bind_attrs and it works by simply setting it to true > > before driver registration e.g. > > > > --- a/drivers/acpi/scan.c > > +++ b/drivers/acpi/scan.c > > @@ -783,7 +783,8 @@ int acpi_bus_register_driver(struct acpi_driver *driver) > > driver->drv.name = driver->name; > > driver->drv.bus = &acpi_bus_type; > > driver->drv.owner = driver->owner; > > - > > + if (!strcmp(driver->class, "memory")) > > + driver->drv.suppress_bind_attrs = true; > > ret = driver_register(&driver->drv); > > return ret; > > } > > > > No bind/unbind sysfs files are created when using this, as expected. > > I assume we only want to suppress for acpi_memhotplug > > (class=ACPI_MEMORY_DEVICE_CLASS i.e. "memory") devices. > > > > Is there agreement on what acpi_bus_trim behaviour and rollback (if any) we > > want to have for the current ACPI framework (partial trim or full trim on > > failure)? > > Last time I suggested to split the trimming so that first we only unbind > drivers (and roll back that part, ie. rebind the drivers on errors) and > next we remove the struct acpi_device objects, just before doing the actual > eject. So there would be two walks of the hierarchy below the device we want > to eject, one for driver unbinding (that can be rolled back) and one for the > actual removal. > > Toshi Kani seemed to agree with that and there were no follow-ups. I was hoping to have a short term solution to fix the panic on attempting to delete a kernel memory range, assuming that the memory hot-plug feature is going to make into 3.8. It's a blocker issue for testing the feature. Now that the VM patchset does not seem to make into 3.8, I think we can step back and focus on a long term solution toward 3.9. I agree that we should separate resource online/offlining step and acpi_device creation/deletion step. It can address the panic and make rollback easier to handle. For 3.9, we should have a better framework in place to handle it in general. So, I am currently working on a framework proposal, and hopefully able to send it out in a week or so. Lastly, thanks Vasilis for testing the suppress_bind_attrs change. I think we may still need it for 3.9. Thanks, -Toshi -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, December 06, 2012 08:41:29 AM Toshi Kani wrote: > On Thu, 2012-12-06 at 13:50 +0100, Rafael J. Wysocki wrote: > > On Thursday, December 06, 2012 10:30:19 AM Vasilis Liaskovitis wrote: > > > Hi, > > > On Thu, Nov 29, 2012 at 10:44:11AM -0700, Toshi Kani wrote: > > > > On Thu, 2012-11-29 at 12:04 +0100, Vasilis Liaskovitis wrote: > > > > > > > > Yes, that's what I had in mind along with device_lock(). I think the > > > > lock is necessary to close the window. > > > > http://www.spinics.net/lists/linux-mm/msg46973.html > > > > > > > > But as I mentioned in other email, I prefer option 3 with > > > > suppress_bind_attrs. So, yes, please take a look to see how it works > > > > out. > > > > > > I tested the suppress_bind_attrs and it works by simply setting it to true > > > before driver registration e.g. > > > > > > --- a/drivers/acpi/scan.c > > > +++ b/drivers/acpi/scan.c > > > @@ -783,7 +783,8 @@ int acpi_bus_register_driver(struct acpi_driver *driver) > > > driver->drv.name = driver->name; > > > driver->drv.bus = &acpi_bus_type; > > > driver->drv.owner = driver->owner; > > > - > > > + if (!strcmp(driver->class, "memory")) > > > + driver->drv.suppress_bind_attrs = true; > > > ret = driver_register(&driver->drv); > > > return ret; > > > } > > > > > > No bind/unbind sysfs files are created when using this, as expected. > > > I assume we only want to suppress for acpi_memhotplug > > > (class=ACPI_MEMORY_DEVICE_CLASS i.e. "memory") devices. > > > > > > Is there agreement on what acpi_bus_trim behaviour and rollback (if any) we > > > want to have for the current ACPI framework (partial trim or full trim on > > > failure)? > > > > Last time I suggested to split the trimming so that first we only unbind > > drivers (and roll back that part, ie. rebind the drivers on errors) and > > next we remove the struct acpi_device objects, just before doing the actual > > eject. So there would be two walks of the hierarchy below the device we want > > to eject, one for driver unbinding (that can be rolled back) and one for the > > actual removal. > > > > Toshi Kani seemed to agree with that and there were no follow-ups. > > I was hoping to have a short term solution to fix the panic on > attempting to delete a kernel memory range, assuming that the memory > hot-plug feature is going to make into 3.8. It's a blocker issue for > testing the feature. Now that the VM patchset does not seem to make > into 3.8, I think we can step back and focus on a long term solution > toward 3.9. > > I agree that we should separate resource online/offlining step and > acpi_device creation/deletion step. It can address the panic and make > rollback easier to handle. For 3.9, we should have a better framework > in place to handle it in general. So, I am currently working on a > framework proposal, and hopefully able to send it out in a week or so. Cool, thanks for doing this! > Lastly, thanks Vasilis for testing the suppress_bind_attrs change. I > think we may still need it for 3.9. Well, we'll see. :-) Thanks, Rafael
--- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -783,7 +783,8 @@ int acpi_bus_register_driver(struct acpi_driver *driver) driver->drv.name = driver->name; driver->drv.bus = &acpi_bus_type; driver->drv.owner = driver->owner; - + if (!strcmp(driver->class, "memory")) + driver->drv.suppress_bind_attrs = true; ret = driver_register(&driver->drv); return ret; }