lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 08 Oct 2012 14:58:12 +0800 From: Wen Congyang <wency@...fujitsu.com> To: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com> CC: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>, x86@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org, rientjes@...gle.com, liuj97@...il.com, len.brown@...el.com, cl@...ux.com, minchan.kim@...il.com, akpm@...ux-foundation.org Subject: Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device_remove() At 10/05/2012 04:53 AM, KOSAKI Motohiro Wrote: > On Wed, Oct 3, 2012 at 5:58 AM, Yasuaki Ishimatsu > <isimatu.yasuaki@...fujitsu.com> wrote: >> From: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com> >> >> 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@...gle.com> >> CC: Jiang Liu <liuj97@...il.com> >> CC: Len Brown <len.brown@...el.com> >> CC: Christoph Lameter <cl@...ux.com> >> Cc: Minchan Kim <minchan.kim@...il.com> >> CC: Andrew Morton <akpm@...ux-foundation.org> >> CC: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com> >> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com> >> Signed-off-by: Wen Congyang <wency@...fujitsu.com> >> --- >> 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@...ck.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists