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 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