| Message ID | 994278b96189033970d0a2add95645110c716fbb.1448480385.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
Hi Lucas, Sorry for the late reply, please see comments below. 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? Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/11/30 14:27, Hanjun Guo wrote: > Hi Lucas, > > Sorry for the late reply, please see comments below. > > 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. Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. The list was added with e49bd2dd5a50 ("ACPI: use PNPID:instance_no as bus_id of ACPI device") 9 years ago and the author even mentioned that the list elements are never freed: "NOTE: Now I don't take the memory free work in charge. If necessary, I can add a reference count in struct acpi_device_bus_id, and check the reference and when unregister a device, i.e. memory is freed when the reference count of a given PNPID is 0." Best regards, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, a gentle ping, not sure if you just haven't had time to look at this thread or if it's fallen between the cracks. (Further comments from me below.) On Wed, Dec 02, 2015 at 04:56:47PM +0800, Hanjun Guo wrote: > 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? Yes, with this patch the instance number is no longer strictly increasing and if devices are unplugged and replaced, instance numbers are recycled for the newly plugged devices. I have no idea if this can break things. If this patch is not acceptable, the semantics of patch [v2 2/2] change in that acpi_dev_present() doesn't return the presence of a HID at the time of invocation, but rather whether such a device has ever been present since boot. It wouldn't be a problem because acpi_dev_present() is meant for presence detection of devices which are built into the system (same goes for the PCI counterpart pci_dev_present(), see the kerneldoc in drivers/pci/search.c). I'd have to adjust the kerneldoc though to reflect the changed semantics. The other option would be to traverse the acpi_device tree instead of iterating over acpi_bus_id_list, then it would be possible to retain the semantics of detecting presence at the moment of invocation. Please let me know which of these options makes the most sense to you so that I can adjust the patches. Thank you, Lukas > > Thanks > Hanjun > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, December 06, 2015 10:09:19 PM Lukas Wunner wrote: > Hi Rafael, Hi, > a gentle ping, not sure if you just haven't had time to look at this > thread or if it's fallen between the cracks. Neither of these. I'm actually going to apply your patches. > (Further comments from me below.) > > On Wed, Dec 02, 2015 at 04:56:47PM +0800, Hanjun Guo wrote: > > 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? > > Yes, with this patch the instance number is no longer strictly increasing > and if devices are unplugged and replaced, instance numbers are recycled > for the newly plugged devices. I have no idea if this can break things. Maybe. I guess it may confuse user space on some systems if it cached the old device names somehow, but since the instance numbers change right now anyway, that is unlikely. > If this patch is not acceptable, the semantics of patch [v2 2/2] change > in that acpi_dev_present() doesn't return the presence of a HID at the > time of invocation, but rather whether such a device has ever been present > since boot. It wouldn't be a problem because acpi_dev_present() is > meant for presence detection of devices which are built into the system > (same goes for the PCI counterpart pci_dev_present(), see the kerneldoc > in drivers/pci/search.c). I'd have to adjust the kerneldoc though to > reflect the changed semantics. > > The other option would be to traverse the acpi_device tree instead of > iterating over acpi_bus_id_list, then it would be possible to retain > the semantics of detecting presence at the moment of invocation. > > Please let me know which of these options makes the most sense to you > so that I can adjust the patches. The simplest one that works for all of the users in question should be fine. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 78d5f02..a0e942b 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -471,10 +471,24 @@ static void acpi_device_release(struct device *dev) static void acpi_device_del(struct acpi_device *device) { + struct acpi_device_bus_id *acpi_device_bus_id; + mutex_lock(&acpi_device_lock); if (device->parent) list_del(&device->node); + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) + if (!strcmp(acpi_device_bus_id->bus_id, + acpi_device_hid(device))) { + if (acpi_device_bus_id->instance_no > 0) + acpi_device_bus_id->instance_no--; + else { + list_del(&acpi_device_bus_id->node); + kfree(acpi_device_bus_id); + } + break; + } + list_del(&device->wakeup_list); mutex_unlock(&acpi_device_lock);
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. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/acpi/scan.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)