Hi, On 5/5/21 12:02 PM, Andy Shevchenko wrote: > On Wed, May 5, 2021 at 12:28 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> On 5/5/21 11:17 AM, Andy Shevchenko wrote: >>> On Wed, May 5, 2021 at 12:07 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>> On 5/4/21 9:52 AM, Andy Shevchenko wrote: >>>>> On Monday, May 3, 2021, Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: >>> >>> ... >>> >>>>> + fwnode = device_get_next_child_node(kdev, fwnode); >>> >>>>> Who is dropping reference counting on fwnode ? >>>> >>>> We are dealing with ACPI fwnode-s here and those are not ref-counted, they >>>> are embedded inside a struct acpi_device and their lifetime is tied to >>>> that struct. They should probably still be ref-counted (with the count >>>> never dropping to 0) so that the generic fwnode functions behave the same >>>> anywhere but atm the ACPI nodes are not refcounted, see: acpi_get_next_subnode() >>>> in drivers/acpi/property.c which is the get_next_child_node() implementation >>>> for ACPI fwnode-s. >>> >>> Yes, ACPI currently is exceptional, but fwnode API is not. >>> If you may guarantee that this case won't ever be outside of ACPI >> >> Yes I can guarantee that currently this code (which is for the i915 >> driver only) only deals with ACPI fwnodes. >> >>> and >>> even though if ACPI won't ever gain a reference counting for fwnodes, >>> we can leave it as is. >> >> Would it not be better to add fake ref-counting to the ACPI fwnode >> next_child_node() op though. I believe just getting a reference >> on the return value there should work fine; and then all fwnode >> implementations would be consistent ? > > But it's already there by absent put/get callbacks. Ah, I completely missed that the put/get-s are actually done through function pointers in fwnode_operations. I assumed that there was a kref embedded inside the fwnode_handle struct and that they operated directly on that. So this whole discussion is entirely based on that misunderstanding, my bad, sorry. So yes you are right, things are already consistent thanks to the absent put/get callbacks. But we do really need to document the behavior better here in the kdoc for fwnode_get_next_child_node() and device_get_next_child_node(). of_get_next_child has this bit, which applies to those too: * Returns a node pointer with refcount incremented, use of_node_put() on * it when done. Returns NULL when prev is the last child. Decrements the * refcount of prev. I'll prepare a patch to add this to the kdoc for fwnode_get_next_child_node() and device_get_next_child_node() once I'm done with readying v3 of this series. Regards, Hans
