>>> -static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>>> +static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device)
>>> {
>>> int result;
>>> struct acpi_memory_info *info, *n;
>>>
>>> + list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
>>
>> Which lock protect this loop?
>
> There is no any lock to protect it now...
When iterate an item removal list, you should use lock for protecting from
memory corruption.
>>> +static int acpi_memory_disable_device(struct acpi_memory_device *mem_device)
>>> +{
>>> + int result;
>>>
>>> /*
>>> * Ask the VM to offline this memory range.
>>> * Note: Assume that this function returns zero on success
>>> */
>>
>> Write function comment instead of this silly comment.
>>
>>> - list_for_each_entry_safe(info, n, &mem_device->res_list, list) {
>>> - if (info->enabled) {
>>> - result = remove_memory(info->start_addr, info->length);
>>> - if (result)
>>> - return result;
>>> - }
>>> - kfree(info);
>>> - }
>>> + result = acpi_memory_remove_memory(mem_device);
>>> + if (result)
>>> + return result;
>>>
>>> /* Power-off and eject the device */
>>> result = acpi_memory_powerdown_device(mem_device);
>>
>> This patch move acpi_memory_powerdown_device() from ACPI_NOTIFY_EJECT_REQUEST
>> to release callback, but don't explain why.
>
> Hmm, it doesn't move the code. It just reuse the code in acpi_memory_powerdown_device().
Even if reuse or not reuse, you changed the behavior. If any changes
has no good rational, you cannot get an ack.
>>> @@ -473,12 +486,23 @@ static int acpi_memory_device_add(struct
>>> static int acpi_memory_device_remove(struct acpi_device *device, int type)
>>> {
>>> struct acpi_memory_device *mem_device = NULL;
>>> -
>>> + int result;
>>>
>>> if (!device || !acpi_driver_data(device))
>>> return -EINVAL;
>>>
>>> mem_device = acpi_driver_data(device);
>>> +
>>> + if (type == ACPI_BUS_REMOVAL_EJECT) {
>>> + /*
>>> + * offline and remove memory only when the memory device is
>>> + * ejected.
>>> + */
>>
>> This comment explain nothing. A comment should describe _why_ should we do.
>> e.g. Why REMOVAL_NORMAL and REMOVEL_EJECT should be ignored. Why
>> we need remove memory here instead of ACPI_NOTIFY_EJECT_REQUEST.
>
> Hmm, we have 2 ways to remove a memory:
> 1. SCI
> 2. echo 1 >/sys/bus/acpi/devices/PNP0C80:XX/eject
>
> In the 2nd case, there is no ACPI_NOTIFY_EJECT_REQUEST. We should offline
> the memory and remove it from kernel in the release callback. We will poweroff
> the memory device in acpi_bus_hot_remove_device(), so we must offline
> and remove it if the type is ACPI_BUS_REMOVAL_EJECT.
>
> I guess we should not poweroff the memory device when we fail to offline it.
> But device_release_driver() doesn't returns any error...
1) I think /sys/bus/acpi/devices/PNP0C80:XX/eject should emulate acpi
eject. Can't
you make a pseudo acpi eject event and detach device by acpi regular path?
2) Your explanation didn't explain why we should ignore REMOVAL_NORMAL
and REMOVEL_EJECT. As far as reviewers can't track your intention, we
can't maintain
the code and can't ack them.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
