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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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()"


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

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.

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?

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

> > +		uid = acpi_device_uid(adev);
> > +		if (uid)
> > +			for (int i = 0; i < strlen(uid); i++)
> 
> size_t i?

Ick, no, "int" is fine.

thanks,

greg k-h