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

Greg Kroah-Hartman
2025-08-11T08:31:26Z

On Mon, Aug 11, 2025 at 07:29:37AM +0000, Sakari Ailus wrote:
> Hi Greg,
>
> On Mon, Aug 11, 2025 at 09:12:36AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 11, 2025 at 06:51:03AM +0000, Sakari Ailus wrote:
> > > > +/**
> > > > + * struct usbio_client - represents a usbio client
> > > > + *
> > > > + * @adev: auxiliary device object
> > > > + * @bridge: usbio bridge who service the client
> > > > + * @link: usbio bridge clients list member
> > > > + */
> > > > +struct usbio_client {
> > > > + struct auxiliary_device adev;
> > > > + struct usbio_device *bridge;
> > > > + struct list_head link;
> > > > +};
> > > > +
> > > > +#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.
> >
> > Why? That's a very common way of doing this. My only complaint is that
> > it really should be "container_of_const()" instead of just
> > "container_of()"
>
> Because the struct field has the same name. The macro isn't intended for
> obtaining the container struct based on any field in the struct, only the
> field called "adev".

And that's fine, the macro works like this, so all should be ok.

> I'll post a patch to add the container_of() check to checkpatch.pl.

Patch to add it to do what?

> > > > +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> > > > + const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> > > > +{
> > > > + u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> > > > + struct usbio_ctrl_packet *cpkt;
> > > > + unsigned int pipe;
> > > > + u16 cpkt_len;
> > > > + int ret;
> > > > +
> > > > + lockdep_assert_held(&usbio->mutex);
> > > > +
> > > > + if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> > > > + (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> > >
> > > You can (and should) remove all parentheses except the outer ones here.
> >
> > No, don't do that. If you do that you will have to manually go and try
> > to remember the order of operations every time you read this code.
>
> I presume kernel developers in general do.
>
> But if in doubt: <URL:https://users.ece.utexas.edu/~adnan/c-refcard.pdf>.

Don't force me to look it up all the time, use () to make it obvious
please. That's the biggest thing I hate about that checkpatch "rule",
please do not follow it for any code that I am a maintainer for.

> > > > +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?
> > >
> > > 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.
> >
> > How is that going to change anything? And how can a disconnect happen?
> > Isn't this an onboard device?
>
> It is, but the device firmware is known to crash occasionally.

Then fix the firmware :)

> The documantation says you can't access USB interfaces once disconnect has
> returned. I'm not sure if there are checks to safeguard against ongoing or
> additional accesses in the USB stack but on many other buses this may
> simply lead to a system crash.

How can you access the USB interface after disconnect has returned on
these codepaths? The child devices should all be cleaned up properly
after disconnect happens so there should not be a pointer to even use
anymore.

thanks,

greg k-h