Re: [RFC PATCH v3 0/3] acpi: Introduce prepare_remove device operation

[Jiang Liu]

On 11/29/2012 06:15 PM, Rafael J. Wysocki wrote:
> On Wednesday, November 28, 2012 11:41:36 AM Toshi Kani wrote:
>> On Wed, 2012-11-28 at 19:05 +0800, Hanjun Guo wrote:
>>> On 2012/11/24 1:50, Vasilis Liaskovitis wrote:
>>>> As discussed in https://patchwork.kernel.org/patch/1581581/
>>>> the driver core remove function needs to always succeed. This means we need
>>>> to know that the device can be successfully removed before acpi_bus_trim / 
>>>> acpi_bus_hot_remove_device are called. This can cause panics when OSPM-initiated
>>>> or SCI-initiated eject of memory devices fail e.g with:
>>>> echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>>>
>>>> since the ACPI core goes ahead and ejects the device regardless of whether the
>>>> the memory is still in use or not.
>>>>
>>>> For this reason a new acpi_device operation called prepare_remove is introduced.
>>>> This operation should be registered for acpi devices whose removal (from kernel
>>>> perspective) can fail.  Memory devices fall in this category.
>>>>
>>>> acpi_bus_remove() is changed to handle removal in 2 steps:
>>>> - preparation for removal i.e. perform part of removal that can fail. Should
>>>>   succeed for device and all its children.
>>>> - if above step was successfull, proceed to actual device removal
>>>
>>> Hi Vasilis,
>>> We met the same problem when we doing computer node hotplug, It is a good idea
>>> to introduce prepare_remove before actual device removal.
>>>
>>> I think we could do more in prepare_remove, such as rollback. In most cases, we can
>>> offline most of memory sections except kernel used pages now, should we rollback
>>> and online the memory sections when prepare_remove failed ?
>>
>> I think hot-plug operation should have all-or-nothing semantics.  That
>> is, an operation should either complete successfully, or rollback to the
>> original state.
> 
> That's correct.
> 
>>> As you may know, the ACPI based hotplug framework we are working on already addressed
>>> this problem, and the way we slove this problem is a bit like yours.
>>>
>>> We introduce hp_ops in struct acpi_device_ops:
>>> struct acpi_device_ops {
>>> 	acpi_op_add add;
>>> 	acpi_op_remove remove;
>>> 	acpi_op_start start;
>>> 	acpi_op_bind bind;
>>> 	acpi_op_unbind unbind;
>>> 	acpi_op_notify notify;
>>> #ifdef	CONFIG_ACPI_HOTPLUG
>>> 	struct acpihp_dev_ops *hp_ops;
>>> #endif	/* CONFIG_ACPI_HOTPLUG */
>>> };
>>>
>>> in hp_ops, we divide the prepare_remove into six small steps, that is:
>>> 1) pre_release(): optional step to mark device going to be removed/busy
>>> 2) release(): reclaim device from running system
>>> 3) post_release(): rollback if cancelled by user or error happened
>>> 4) pre_unconfigure(): optional step to solve possible dependency issue
>>> 5) unconfigure(): remove devices from running system
>>> 6) post_unconfigure(): free resources used by devices
>>>
>>> In this way, we can easily rollback if error happens.
>>> How do you think of this solution, any suggestion ? I think we can achieve
>>> a better way for sharing ideas. :)
>>
>> Yes, sharing idea is good. :)  I do not know if we need all 6 steps (I
>> have not looked at all your changes yet..), but in my mind, a hot-plug
>> operation should be composed with the following 3 phases.
>>
>> 1. Validate phase - Verify if the request is a supported operation.  All
>> known restrictions are verified at this phase.  For instance, if a
>> hot-remove request involves kernel memory, it is failed in this phase.
>> Since this phase makes no change, no rollback is necessary to fail.  
> 
> Actually, we can't do it this way, because the conditions may change between
> the check and the execution.  So the first phase needs to involve execution
> to some extent, although only as far as it remains reversible.
Hi Rafael,
	A possible way to solve this issue is:
1) mark device busy
2) check condition and mark device as normal if condition check fails.
3) reclaim the device and mark device as normal if reclaim fails.
4) remove the device.

> 
>> 2. Execute phase - Perform hot-add / hot-remove operation that can be
>> rolled-back in case of error or cancel.
> 
> I would just merge 1 and 2.
> 
>> 3. Commit phase - Perform the final hot-add / hot-remove operation that
>> cannot be rolled-back.  No error / cancel is allowed in this phase.  For
>> instance, eject operation is performed at this phase.  
> 
> Yup.
> 
> Thanks,
> Rafael
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/