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

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

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

> 
> Remember, we write code for people first, compilers second.  Make it
> totally obvious what is going on here as you want PEOPLE to catch your
> issues.

I guess people can have differing opinions on this; I find the above much
easier to read without the extra parentheses.

GCC actually nowadays generates warnings for code for which the evaluation
order is perfectly well defined (namely mixing bitwise and logical
operations AFAIR, for relying on the evaluation order between && and || I
can somehow understand that).

> 
> The statement is fine as-is.
> 
> > > +		return -EMSGSIZE;
> > > +
> > > +	/* Prepare Control Packet Header */
> > > +	cpkt = usbio->ctrlbuf;
> > > +	cpkt->header.type = type;
> > > +	cpkt->header.cmd = cmd;
> > > +	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
> > > +		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
> > > +	else
> > > +		cpkt->header.flags = USBIO_PKTFLAG_CMP;
> > > +	cpkt->len = obuf_len;
> > > +
> > > +	/* Copy the data */
> > > +	memcpy(cpkt->data, obuf, obuf_len);
> > > +
> > > +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
> > > +	cpkt_len = sizeof(*cpkt) + obuf_len;
> > > +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
> > > +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> > > +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
> > > +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> > 
> > Instead of casting, how about using %zu for printing a size_t?
> > 
> > > +
> > > +	if (ret != cpkt_len) {
> > > +		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
> > > +		return (ret < 0) ? ret : -EPROTO;
> > 
> > Redundant parentheses.
> 
> Nope!  Again, please mat it obvioue.
> 
> Actually, I hate ? : lines, this should be:
> 	if (ret < 0)
> 		return ret
> 	else
> 		return -EPROTO
> 
> by spelling it all out.
> 
> > > +static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
> > > +	int ret, i;
> > 
> > unsigned int i, please.
> 
> Nope, 'int' is just fine.  Please see Dan's many rants about why this is
> acceptable for loops.
> 
> > > +static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
> > > +	int ret, i;
> > 
> > unsigned int i, please.
> 
> Nope, 'int' is fine.
> 
> > > +static int usbio_ctrl_enumspis(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_spi_bus_desc *spi = usbio->spis;
> > > +	int ret, i;
> > 
> > Ditto.
> 
> Nope :)
> 
> > > +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.

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.

> 
> > > +}
> > > +
> > > +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.
> 
> Which clients?  The disconnect will cause that to happen, it can not
> happen at the same time probe is called.

Not probe but disconnect. See the two other patches in the set for client
drivers.

> 
> > > +		uid = acpi_device_uid(adev);
> > > +		if (uid)
> > > +			for (int i = 0; i < strlen(uid); i++)
> > 
> > size_t i?
> 
> Ick, no, "int" is fine.

-- 
Regards,

Sakari Ailus