Re: [PATCH] usb: roles: Cakk try_module_get() from usb_role_switch_find_by_fwnode()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/9/21 11:30 AM, Heikki Krogerus wrote:
> On Thu, Apr 08, 2021 at 04:09:04PM -0700, Guenter Roeck wrote:
>> On Thu, Apr 08, 2021 at 10:36:11PM +0200, Hans de Goede wrote:
>>> usb_role_switch_find_by_fwnode() returns a reference to the role-switch
>>> which must be put by calling usb_role_switch_put().
>>>
>>> usb_role_switch_put() calls module_put(sw->dev.parent->driver->owner),
>>> add a matching try_module_get() to usb_role_switch_find_by_fwnode(),
>>> making it behave the same as the other usb_role_switch functions
>>> which return a reference.
>>>
>>> This avoids a WARN_ON being hit at kernel/module.c:1158 due to the
>>> module-refcount going below 0.
>>>
>>
>> Took me a while to figure out what the subject line is supposed
>> to mean.
>>
>> s/Cakk/Call/
>>
>> Otherwise
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> It might be useful though to explain the difference between
>> fwnode_usb_role_switch_get() and usb_role_switch_find_by_fwnode(),
>> and why two different functions are needed, both passing fwnode
>> as parameter and returning a pointer to usb_role_switch.

Sorry about thetypo, I completely missed that while preparing the patch,
fixed for v2.

> Yes, the function names are confusing indeed. My proposal is to rename
> usb_role_switch_find_by_fwnode() to fwnode_to_usb_role_switch().
> 
> I can prepare a patch for that if you guys are OK with it, or Hans,
> would you prefer to send that together with this one?

If you can send a patch to apply on top of my v2 of this patch then
that would be great.

> Actually, shouldn't this be marked as a fix?

That is a good point I've added a fixes tag for v2.

Regards,

Hans