Hi Greg, On Wed Jul 31, 2024 at 11:16 AM CEST, Greg Kroah-Hartman wrote: > On Wed, Jul 31, 2024 at 10:57:04AM +0200, Michael Walle wrote: > > Hi Greg, > > > > On Wed Jul 31, 2024 at 10:32 AM CEST, Greg Kroah-Hartman wrote: > > > On Tue, Jul 30, 2024 at 09:43:37PM +0200, Michael Walle wrote: > > > > struct f_serial_opts { > > > > struct usb_function_instance func_inst; > > > > u8 port_num; > > > > + u8 protocol; > > > > + > > > > + struct mutex lock; > > > > + int refcnt; > > > > > > Attempting to "roll your own" reference count is almost never a good > > > idea. If you really need one, please use the proper in-kernel apis for > > > this. But you need to justify it as well, I didn't see why this was > > > needed at all. > > > > Honestly, I couldn't grok all that usb gadget magic, so I've looked > > at similar gadgets and took that from there: > > grep refcnt drivers/usb/gadget/function/ -r > > > > They are all doing the same, so maybe that code is old or didn't use > > the proper APIs back then. > > It's old code, please do things properly. Sorry, I have to come back to this. What do you have in mind? The mutex is needed in any case to protect the members of f_serial_ops from concurrent access between the .bind() and the configfs write. Which leaves us with refcnt. I don't think refcount_t is suitable here. For example, refcount_inc() will warn if you increment from 0; which makes sense for resource reference counting. But doesn't make sense in this case; there is no resource handling or freeing if the refcnt is 0. It just prohibits the write to the configfs attribute if refcnt != 0, that is, there is at least one instance of the gadget function. Maybe I should just rename "refcnt" to "users" and add a comment to the mutex what it will protect. What do you think? -michael