Messages in this thread | | | Date | Mon, 22 May 2023 11:21:46 +0200 | Subject | Re: [PATCH v8 1/6] usb: Add support for Intel LJCA device | From | Oliver Neukum <> |
| |
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@intel.com> > ---
> + > +/* 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 ...
> + > + 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?
> + > + 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.
> + > + 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; > + > + header = kmalloc(msg_len, GFP_KERNEL); > + if (!header) > + return -ENOMEM;
Do you really want to first set a flag, then error out?
> + 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.
> + 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.
> + > + 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
> + > + auxdev->dev.parent = &dev->intf->dev; > + auxdev->dev.release = ljca_aux_release;
Regards Oliver
| |