At 10/05/2012 04:53 AM, KOSAKI Motohiro Wrote:
> On Wed, Oct 3, 2012 at 5:58 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@xxxxxxxxxxxxxx> wrote:
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
>>
>> The memory device can be removed by 2 ways:
>> 1. send eject request by SCI
>> 2. echo 1 >/sys/bus/pci/devices/PNP0C80:XX/eject
>>
>> In the 1st case, acpi_memory_disable_device() will be called.
>> In the 2nd case, acpi_memory_device_remove() will be called.
>> acpi_memory_device_remove() will also be called when we unbind the
>> memory device from the driver acpi_memhotplug.
>>
>> acpi_memory_disable_device() has already implemented a code which
>> offlines memory and releases acpi_memory_info struct . But
>> acpi_memory_device_remove() has not implemented it yet.
>>
>> So the patch implements acpi_memory_remove_memory() for offlining
>> memory and releasing acpi_memory_info struct. And it is used by both
>> acpi_memory_device_remove() and acpi_memory_disable_device().
>>
>> Additionally, if the type is ACPI_BUS_REMOVAL_EJECT in
>> acpi_memory_device_remove() , it means that the user wants to eject
>> the memory device. In this case, acpi_memory_device_remove() calls
>> acpi_memory_remove_memory().
>>
>> CC: David Rientjes <rientjes@xxxxxxxxxx>
>> CC: Jiang Liu <liuj97@xxxxxxxxx>
>> CC: Len Brown <len.brown@xxxxxxxxx>
>> CC: Christoph Lameter <cl@xxxxxxxxx>
>> Cc: Minchan Kim <minchan.kim@xxxxxxxxx>
>> CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> CC: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
>> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
>> ---
>> drivers/acpi/acpi_memhotplug.c | 44 +++++++++++++++++++++++++++++++----------
>> 1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> Index: linux-3.6/drivers/acpi/acpi_memhotplug.c
>> ===================================================================
>> --- linux-3.6.orig/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:33.386378909 +0900
>> +++ linux-3.6/drivers/acpi/acpi_memhotplug.c 2012-10-03 18:55:58.624380688 +0900
>> @@ -306,24 +306,37 @@ static int acpi_memory_powerdown_device(
>> return 0;
>> }
>>
>> -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...
>
>
>> + if (!info->enabled)
>> + return -EBUSY;
>> +
>> + result = remove_memory(info->start_addr, info->length);
>> + if (result)
>> + return result;
>
> I suspect you need to implement rollback code instead of just return.
>
>
>> +
>> + list_del(&info->list);
>> + kfree(info);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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().
>
>
>
>
>
>> @@ -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...
Thanks
Wen Congyang
>
>
>> + result = acpi_memory_remove_memory(mem_device);
>> + if (result)
>> + return result;
>> + }
>> +
>> kfree(mem_device);
>>
>> return 0;
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>