linux-kernel - Re: [PATCH 1/4] acpi,memory-hotplug : add memory offline code to acpi_memory_device

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <507E77D1.3030709@cn.fujitsu.com>
Date:	Wed, 17 Oct 2012 17:18:09 +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/13/2012 03:10 AM, KOSAKI Motohiro Wrote:
>>>> -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?

It is another issue. And we only can implement it here with current acpi
implemention. Some other acpi devices(for example: cpu) do it like this.

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

REMOVAL_NORMAL means the user want to unbind the memory device from this driver.
It is no need to eject the device, and we can still use this device after unbinding.
So it can be ignored.

REMOVAL_EJECT means the user want to eject and remove the device, and we should
not use the device. So we should offline and remove the memory here.

Thanks
Wen Congyang
--
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/