Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

Sakari Ailus
2025-08-11T09:32:43Z

Hi Hans,

On Mon, Aug 11, 2025 at 11:13:16AM +0200, Hans de Goede wrote:
> >> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> >
> > Please use a different name than "adev" for the argument, which is also the
> > struct field of interest.
>
> As gkh mentioned doing things this way is quite normal.

Isn't this exactly what people have been warning against because it'll lead
to hard-to-find bugs and requesting static inline functions being used
instead? Why not now?

...

> >> +static void usbio_disconnect(struct usb_interface *intf)
> >> +{
> >> + struct usbio_device *usbio = usb_get_intfdata(intf);
> >> + struct usbio_client *client, *prev;
> >> +
> >> + list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> >> + auxiliary_device_delete(&client->adev);
> >> + list_del_init(&client->link);
> >> + auxiliary_device_uninit(&client->adev);
> >> + }
> >> +
> >> + usb_kill_urb(usbio->urb);
> >> + usb_free_urb(usbio->urb);
> >
> > What will happen on client drivers if they're working with the bridge while
> > disconnect happens?
>
> All clients will have their remove() function called. If clients
> somehow need to have some userspace-api related bits stick around
> longer they will need to handle that themselves. Clients are no longer
> allowed to call an usbio_*() functions after having their remove()
> function called.
>
> > One easy solution to this could be to use an rw_semaphore where client
> > acquire it for readingin conjunction (in a helper that also checks the
> > interface status) and disconnect callback for writing.
>
> That is not necessary.
>
> One thing which should be done here is set a disconnected flag to make new
> usbio_*() calls fail immediately and signal the "done" completion to wakeup
> any waiters, something like this:
>
> static void usbio_disconnect(struct usb_interface *intf)
> {
> struct usbio_device *usbio = usb_get_intfdata(intf);
> struct usbio_client *client, *prev;
>
> list_for_each_entry(client, &usbio->cli_list, link)
> client->disconnected = true;
>
> complete(&usbio->done);
>
> /* Ensure all clients see the disconnect flag */
> mutex_lock(&usbio->mutex);
> mutex_unlock(&usbio->mutex);
>
> /* From here on clients will no longer touch struct usbio_device */
> usb_kill_urb(usbio->urb);
> usb_free_urb(usbio->urb);
>
> list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> auxiliary_device_delete(&client->adev);
> list_del_init(&client->link);
> auxiliary_device_uninit(&client->adev);
> }
> }
>
> Combined with checking client->disconnected in entry points
> and after waiting for the completion.
>
> This will even allow the client to keep the struct auxdev
> around by keeping a reference and it will even be safe for
> the client to make usbio_*() calls on the auxdev since as
> long as the reference is there the memory of the usbio_client
> which embeds the auxdev will stick around and the disconnected
> flag will ensure this is safe. Note client drivers really should
> not do that though!

Ack, sounds good. I'll wait for v2 then.

>
>
>
>
>
>
>
> >
> >> +}
> >> +
> >> +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >> +{
> >> + struct usb_device *udev = interface_to_usbdev(intf);
> >> + struct usb_endpoint_descriptor *ep_in, *ep_out;
> >> + struct device *dev = &intf->dev;
> >> + struct usbio_device *usbio;
> >> + int ret;
> >> +
> >> + usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
> >
> > usbio will be released at the exit from usbio_disconnect(). I think you'll
> > need to use a release callback in struct device to release usbio once all
> > clients are gone.
>
> This is fine, see above it is up to the client drivers to no longer
> call any usbio_*() also for v2 I'll make all the public usbio_*()
> functions test client->disconnected and then they will never deref
> client->bridge.

Yes, keeping this information on the client side should be equally good.

--
Regards,

Sakari Ailus