Re: [PATCH v3] driver core: fw_devlink: Stop trying to optimize cycle detection logic

[Tomi Valkeinen]

Hi Saravana,

On 30/10/2024 19:10, Saravana Kannan wrote:
> In attempting to optimize fw_devlink runtime, I introduced numerous cycle
> detection bugs by foregoing cycle detection logic under specific
> conditions. Each fix has further narrowed the conditions for optimization.
>
> It's time to give up on these optimization attempts and just run the cycle
> detection logic every time fw_devlink tries to create a device link.
>
> The specific bug report that triggered this fix involved a supplier fwnode
> that never gets a device created for it. Instead, the supplier fwnode is
> represented by the device that corresponds to an ancestor fwnode.
>
> In this case, fw_devlink didn't do any cycle detection because the cycle
> detection logic is only run when a device link is created between the
> devices that correspond to the actual consumer and supplier fwnodes.
>
> With this change, fw_devlink will run cycle detection logic even when
> creating SYNC_STATE_ONLY proxy device links from a device that is an
> ancestor of a consumer fwnode.
>
> Reported-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/1a1ab663-d068-40fb-8c94-f0715403d276@xxxxxxxxxxxxxxxx/
> Fixes: 6442d79d880c ("driver core: fw_devlink: Improve detection of overlapping cycles")
> Tested-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
>   drivers/base/core.c | 55 ++++++++++++++++++++-------------------------
>   1 file changed, 24 insertions(+), 31 deletions(-)
>
> Tomi,
>
> Thanks for all the testing and debugging help! And do use
> post-init-providers even with this patch to improve ordering
> enforcement. I probably should change the cycle log from info to warn in
> a separate patch :)

I've had this patch in one of my WIP branches for a while. Today I 
started debugging a kmemleak in the branch, which I thought was related 
to my changes. However, it ended up being this patch.

One of the drivers touched in my WIP branch is 
drivers/media/i2c/ds90ub960.c. It does a i2c_new_client_device() and, at 
remove time, i2c_unregister_device().

Without this patch, with some debug prints, I see that the newly created 
i2c client has a kref count of 11, and when i2c_unregister_device() is 
called, I see kref dropping to 0 and i2c_client_dev_release() being called.

With this patch, after i2c_new_client_device(), I see a kref of 16, and 
after i2c_unregister_device(), a kref of 5 (and i2c_client_dev_release() 
not called).

So five new refs were added, and not released... Any thoughts?

 Tomi