On 2015/12/1 21:08, Lukas Wunner wrote: > Hi, > > On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote: >> On 2015/11/30 14:27, Hanjun Guo wrote: >>> On 2015/11/26 4:19, Lukas Wunner wrote: >>>> acpi_device_add() allocates and adds an element to acpi_bus_id_list >>>> (or increments the instance count if the device's HID is already >>>> present in the list), but the element is never deleted from the list >>>> nor freed. Fix it. >>> Hmm, I didn't get it here. Seems the device's ID already freed in device core: >>> >>> In acpi_add_single_object(), acpi_device_release() registered as a callback, >>> ... >>> result = acpi_device_add(device, acpi_device_release); >>> ... >>> >>> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the >>> IDs, did I miss some something? >> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device, >> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise. > Yes, I should have been more clear about this in the first place: > > When the bus is scanned and acpi_device_add() is called for each device, > not only do we initialize a struct acpi_device and attach it to the > device tree, but we also add an element to acpi_bus_id_list. > > Hence there are two ways to detect the presence of a HID: By traversing > the device tree or by iterating over the list. I chose the latter because > it is obviously cheaper and requires less code. > > However elements only ever get added to the list, never deleted. I'm not > sure if hotpluggable ACPI devices exist but if they do, this is a bug > which is fixed by this patch. ACPI devices can be hotpluggable :) , but it will have no memory leak I think, it only increase the instance number for ACPI devices removed and hot-added later, I don't know if it make sense to do that, for example, if you remove device A and B with same HID (such as ACPI0007) with your patch added: remove processor 0, the sysfs for device A /sys/bus/acpi/devices/ACPI0007:00 will be removed; remove processer 1, the sysfs for device B /sys/bus/acpi/devices/ACPI0007:01 will be removed; But if we add it in reverse with your patch: Add dprocesser 1, the sysfs /sys/bus/acpi/devices/ACPI0007:00 will be created, add processor 0... I'm not sure it will confuse user space or not. Rafael, what's your opinion here? Thanks Hanjun -- 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
