Messages in this thread | | | Date | Tue, 20 Jun 2023 14:11:10 +0200 | Subject | Re: [PATCH v8 1/6] usb: Add support for Intel LJCA device | From | Oliver Neukum <> |
| |
On 20.06.23 09:39, Wu, Wentong wrote:
Hi,
>>> + >>> + 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.
Or a timeout. There are two places in the driver ibuf_len and ibuf are touched. Both are in ljca_stub_write(), but in case of a timeout they are set to NULL. And you are checking for that case. That is good, but not sufficient, as READ_ONCE does not guarantee ordering, nor do you use WRITE_ONCE to set them.
> 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.
Well, you can move them to architecture dependent directories. But if not, they will have to work generically.
A correct version would be something like:
+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; + } + + 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; + } +
smp_rmb();
+ ibuf_len = READ_ONCE(stub->ipacket.ibuf_len); + ibuf = READ_ONCE(stub->ipacket.ibuf); + + 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; + + 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; + goto error_put; + } + + mutex_lock(&dev->mutex); + stub->cur_cmd = cmd;
WRITE_ONCE(stub->ipacket.ibuf, ibuf); WRITE_ONCE(stub->ipacket.ibuf_len, ibuf_len);
+ stub->acked = false;
smp_wmb();
[..]
+error_unlock:
WRITE_ONCE(stub->ipacket.ibuf, NULL); WRITE_ONCE(stub->ipacket.ibuf_len, NULL); smp_wmb();
+ mutex_unlock(&dev->mutex);
Regards Oliver
| |