RE: [PATCH v8 1/6] usb: Add support for Intel LJCA device

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

Hi Oliver,

Thanks for your review

> -----Original Message-----
> From: Oliver Neukum <oneukum@xxxxxxxx>
> Sent: Monday, May 22, 2023 5:22 PM
> 
> On 11.05.23 19:58, Ye Xiang wrote:
> > Implements the USB part of Intel USB-I2C/GPIO/SPI adapter device named
> > "La Jolla Cove Adapter" (LJCA).
> >
> > The communication between the various LJCA module drivers and the
> > hardware will be muxed/demuxed by this driver. Three modules ( I2C,
> > GPIO, and SPI) are supported currently.
> >
> > Each sub-module of LJCA device is identified by type field within the
> > LJCA message header.
> >
> 
> Hi,
> 
> I am terribly sorry to come up with issues after so many iterations, but I am
> really afraid there are issues left.
> 
> > The minimum code in ASL that covers this board is Scope
> > (\_SB.PCI0.DWC3.RHUB.HS01)
> >      {
> >          Device (GPIO)
> >          {
> >              Name (_ADR, Zero)
> >              Name (_STA, 0x0F)
> >          }
> >
> >          Device (I2C)
> >          {
> >              Name (_ADR, One)
> >              Name (_STA, 0x0F)
> >          }
> >
> >          Device (SPI)
> >          {
> >              Name (_ADR, 0x02)
> >              Name (_STA, 0x0F)
> >          }
> >      }
> >
> > Signed-off-by: Ye Xiang <xiang.ye@xxxxxxxxx>
> > ---
> 
> > +
> > +/* MNG stub commands */
> > +enum ljca_mng_cmd {
> > +	LJCA_MNG_GET_VERSION = 1,
> > +	LJCA_MNG_RESET_NOTIFY,
> > +	LJCA_MNG_RESET,
> > +	LJCA_MNG_ENUM_GPIO,
> > +	LJCA_MNG_ENUM_I2C,
> > +	LJCA_MNG_POWER_STATE_CHANGE,
> > +	LJCA_MNG_SET_DFU_MODE,
> > +	LJCA_MNG_ENUM_SPI,
> > +};
> > +
> > +/* DIAG commands */
> > +enum ljca_diag_cmd {
> > +	LJCA_DIAG_GET_STATE = 1,
> > +	LJCA_DIAG_GET_STATISTIC,
> > +	LJCA_DIAG_SET_TRACE_LEVEL,
> > +	LJCA_DIAG_SET_ECHO_MODE,
> > +	LJCA_DIAG_GET_FW_LOG,
> > +	LJCA_DIAG_GET_FW_COREDUMP,
> > +	LJCA_DIAG_TRIGGER_WDT,
> > +	LJCA_DIAG_TRIGGER_FAULT,
> > +	LJCA_DIAG_FEED_WDT,
> > +	LJCA_DIAG_GET_SECURE_STATE,
> > +};
> 
> Should those really be enum? They just happen to be nummerically dense.
> 
> 
> > +static int ljca_parse(struct ljca_dev *dev, struct ljca_msg *header)
> > +{
> > +	struct ljca_stub *stub;
> > +	unsigned int *ibuf_len;
> > +	u8 *ibuf;
> > +
> > +	stub = ljca_stub_find(dev, header->type);
> > +	if (IS_ERR(stub))
> > +		return PTR_ERR(stub);
> > +
> > +	if (!(header->flags & LJCA_ACK_FLAG)) {
> > +		ljca_stub_notify(stub, header->cmd, header->data, header-
> >len);
> > +		return 0;
> > +	}
> 
> First you ack ...

This is checking whether the message is cmd ack or event, if not cmd ack, it will
be event.

> 
> > +
> > +	if (stub->cur_cmd != header->cmd) {
> > +		dev_err(&dev->intf->dev, "header and stub current command
> mismatch (%x vs %x)\n",
> > +			header->cmd, stub->cur_cmd);
> > +		return -EINVAL;
> > +	}
> 
> ... then you check whether this is for the correct command?

If it's cmd ack, we check if it's correct command.

> 
> > +
> > +	ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
> > +	ibuf = READ_ONCE(stub->ipacket.ibuf);
> 
> This races against stub_write(). Yes, every value now is consistent within this
> function. But that does not solve the issue. The pair needs to be consistent. You
> need to rule out that you are reading a different length and buffer location. I am
> afraid this needs a spinlock.

Sorry, based on my understanding, the stub's ibuf_len and ibuf won't be changed by
stub_write until firmware ack the current command.
This memory barrier here is for potential cache consistency issue in some arches though
most likely this driver will only be run on X86 platform.

> 
> > +
> > +	if (ibuf && ibuf_len) {
> > +		unsigned int newlen;
> > +
> > +		newlen = min_t(unsigned int, header->len, *ibuf_len);
> > +
> > +		*ibuf_len = newlen;
> > +		memcpy(ibuf, header->data, newlen);
> > +	}
> > +
> > +	stub->acked = true;
> > +	wake_up(&dev->ack_wq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ljca_stub_write(struct ljca_stub *stub, u8 cmd, const void *obuf,
> unsigned int obuf_len,
> > +			   void *ibuf, unsigned int *ibuf_len, bool wait_ack,
> unsigned
> > +long timeout) {
> > +	struct ljca_dev *dev = usb_get_intfdata(stub->intf);
> > +	u8 flags = LJCA_CMPL_FLAG;
> > +	struct ljca_msg *header;
> > +	unsigned int msg_len = sizeof(*header) + obuf_len;
> > +	int actual;
> > +	int ret;
> > +
> > +	if (msg_len > LJCA_MAX_PACKET_SIZE)
> > +		return -EINVAL;
> > +
> > +	if (wait_ack)
> > +		flags |= LJCA_ACK_FLAG;

Will be removed in next version

> > +
> > +	header = kmalloc(msg_len, GFP_KERNEL);
> > +	if (!header)
> > +		return -ENOMEM;
> 
> Do you really want to first set a flag, then error out?

Thanks, in next version, it will be like blow:
header->flags = wait_ack ? flags | LJCA_ACK_FLAG : flags;

> 
> > +	header->type = stub->type;
> > +	header->cmd = cmd;
> > +	header->flags = flags;
> > +	header->len = obuf_len;
> > +
> > +	if (obuf)
> > +		memcpy(header->data, obuf, obuf_len);
> > +
> > +	dev_dbg(&dev->intf->dev, "send: type:%d cmd:%d flags:%d len:%d\n",
> header->type,
> > +		header->cmd, header->flags, header->len);
> > +
> > +	usb_autopm_get_interface(dev->intf);
> > +	if (!dev->started) {
> > +		kfree(header);
> > +		ret = -ENODEV;
> 
> Again, the flag remains set.

The flag is local variable, and the header has been freed as well, I don't think we have
to take care the flag here.

> 
> > +		goto error_put;
> > +	}
> > +
> > +	mutex_lock(&dev->mutex);
> > +	stub->cur_cmd = cmd;
> > +	stub->ipacket.ibuf = ibuf;
> > +	stub->ipacket.ibuf_len = ibuf_len;
> > +	stub->acked = false;
> > +	ret = usb_bulk_msg(interface_to_usbdev(dev->intf),
> > +			   usb_sndbulkpipe(interface_to_usbdev(dev->intf), dev-
> >out_ep), header,
> > +			   msg_len, &actual, LJCA_USB_WRITE_TIMEOUT_MS);
> > +	kfree(header);
> > +	if (ret) {
> > +		dev_err(&dev->intf->dev, "bridge write failed ret:%d\n", ret);
> > +		goto error_unlock;
> > +	}
> > +
> > +	if (actual != msg_len) {
> > +		dev_err(&dev->intf->dev, "bridge write length mismatch (%d
> vs %d)\n", msg_len,
> > +			actual);
> > +		ret = -EINVAL;
> > +		goto error_unlock;
> > +	}
> > +
> > +	if (wait_ack) {
> > +		ret = wait_event_timeout(dev->ack_wq, stub->acked,
> msecs_to_jiffies(timeout));
> > +		if (!ret) {
> > +			dev_err(&dev->intf->dev, "acked wait timeout\n");
> > +			ret = -ETIMEDOUT;
> > +			goto error_unlock;
> > +		}
> > +	}
> > +
> > +	ret = 0;
> > +error_unlock:
> > +	stub->ipacket.ibuf = NULL;
> > +	stub->ipacket.ibuf_len = NULL;
> > +	mutex_unlock(&dev->mutex);
> > +error_put:
> > +	usb_autopm_put_interface(dev->intf);
> > +
> > +	return ret;
> > +}
> > +
> 
> [..]
> > +static int ljca_start(struct ljca_dev *dev) {
> > +	int ret;
> > +
> > +	usb_fill_bulk_urb(dev->in_urb, interface_to_usbdev(dev->intf),
> > +			  usb_rcvbulkpipe(interface_to_usbdev(dev->intf), dev-
> >in_ep), dev->ibuf,
> > +			  dev->ibuf_len, ljca_read_complete, dev);
> > +
> > +	ret = usb_submit_urb(dev->in_urb, GFP_KERNEL);
> > +	if (ret) {
> > +		dev_err(&dev->intf->dev, "failed submitting read urb,
> error %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&dev->mutex);
> > +	dev->started = true;
> > +	mutex_unlock(&dev->mutex);
> 
> Why do you take a mutex here? Either this function cannot race with anything
> else, or you set the flag too late.

Thanks, I will consider the 'started' again.

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> > +#ifdef CONFIG_ACPI
> > +static void ljca_aux_dev_acpi_bind(struct ljca_dev *dev, struct
> auxiliary_device *auxdev,
> > +				   unsigned int adr)
> > +{
> > +	struct acpi_device *parent;
> > +	struct acpi_device *adev = NULL;
> > +
> > +	/* new auxiliary device bind to acpi device */
> > +	parent = ACPI_COMPANION(&dev->intf->dev);
> > +	if (!parent)
> > +		return;
> > +
> > +	adev = acpi_find_child_device(parent, adr, false);
> > +	ACPI_COMPANION_SET(&auxdev->dev, adev ?: parent); } #else static
> > +void ljca_aux_dev_acpi_bind(struct ljca_dev *dev, struct auxiliary_device
> *auxdev,
> > +				   unsigned int adr)
> > +{
> > +}
> > +#endif
> > +
> > +static int ljca_add_aux_dev(struct ljca_dev *dev, char *name, u8 type, u8 id,
> u8 adr, void *data,
> > +			    unsigned int len)
> > +{
> > +	struct auxiliary_device *auxdev;
> > +	struct ljca *ljca;
> > +	int ret;
> > +
> > +	if (dev->ljca_count >= ARRAY_SIZE(dev->ljcas))
> > +		return -EINVAL;
> > +
> > +	ljca = kzalloc(sizeof(*ljca), GFP_KERNEL);
> > +	if (!ljca)
> > +		return -ENOMEM;
> > +
> > +	ljca->type = type;
> > +	ljca->id = id;
> > +	ljca->dev = dev;
> > +
> > +	auxdev = &ljca->auxdev;
> > +	auxdev->name = name;
> > +	auxdev->id = id;
> > +	auxdev->dev.platform_data = kmemdup(data, len, GFP_KERNEL);
> > +	if (!auxdev->dev.platform_data)
> > +		return -ENOMEM;
> 
> memory leak of struct ljca *ljca

ack, this will be fixed in next version, thanks

> 
> > +
> > +	auxdev->dev.parent = &dev->intf->dev;
> > +	auxdev->dev.release = ljca_aux_release;
> 
> 	Regards
> 		Oliver