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