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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Greg,

On Mon, Aug 11, 2025 at 10:31:22AM +0200, Greg Kroah-Hartman wrote:
> 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?

You're cc'd. :-)

> 
> > > > > +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 :)

In practice this depends on device vendor and we all know the quality of
firmware from random sources. These are not Intel-implemented devices.

> 
> > 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.

See functions exported by the main USBIO driver.

-- 
Regards,

Sakari Ailus