| Message ID | 20230325154711.2419569-1-xiang.ye@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
On Sat, Mar 25, 2023 at 11:47:06PM +0800, Ye Xiang wrote: > This patch 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. > > The minimum code in ASL that covers this board is As this requires ACPI, why are you not saying so in your Kconfig entry? What good would this driver be without ACPI enabled? thanks, greg k-h
Hi Greg, Thanks for the review. On Sat, Mar 25, 2023 at 05:20:09PM +0100, Greg Kroah-Hartman wrote: > On Sat, Mar 25, 2023 at 11:47:06PM +0800, Ye Xiang wrote: > > This patch 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. > > > > The minimum code in ASL that covers this board is > > As this requires ACPI, why are you not saying so in your Kconfig entry? I would add a `depends on ACPI` item on the Kconfig entry so that the CONFI_ACPI macro can be removed from usb-ljca, because our use case with LJCA currently needs the ACPI binding. > What good would this driver be without ACPI enabled? Before, I just tried to make it compatible with other platforms that don't support ACPI but want to use LJCA device. But, We don't have this use case until now. Thanks Ye Xiang
On Sun, Mar 26, 2023 at 01:26:55AM +0800, Ye, Xiang wrote: > Hi Greg, > > Thanks for the review. > On Sat, Mar 25, 2023 at 05:20:09PM +0100, Greg Kroah-Hartman wrote: > > On Sat, Mar 25, 2023 at 11:47:06PM +0800, Ye Xiang wrote: > > > This patch 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. > > > > > > The minimum code in ASL that covers this board is > > > > As this requires ACPI, why are you not saying so in your Kconfig entry? > I would add a `depends on ACPI` item on the Kconfig entry so that the > CONFI_ACPI macro can be removed from usb-ljca, because our use case with > LJCA currently needs the ACPI binding. That would make more sense. thanks, greg k-h
On Sun, Mar 26, 2023 at 01:26:55AM +0800, Ye, Xiang wrote: > On Sat, Mar 25, 2023 at 05:20:09PM +0100, Greg Kroah-Hartman wrote: > > As this requires ACPI, why are you not saying so in your Kconfig entry? > I would add a `depends on ACPI` item on the Kconfig entry so that the > CONFI_ACPI macro can be removed from usb-ljca, because our use case with > LJCA currently needs the ACPI binding. > > What good would this driver be without ACPI enabled? > Before, I just tried to make it compatible with other platforms that > don't support ACPI but want to use LJCA device. But, We don't have > this use case until now. If it builds without ACPI then making the dependency ACPI || COMPILE_TEST will help improve build coverage for people doing treewide or subsystem work.
On Sat, Mar 25, 2023 at 11:47:06PM +0800, Ye Xiang wrote:
> +static void ljca_aux_release(struct device *dev) {}
Sorry, but creating an empty release function just to shut the kernel up
is NOT how to properly do this, you all know better. This is totally
broken. The kernel was telling you what you had to do, don't think that
you are being smarter than it by doing this, otherwise we would have
never had the kernel spit out that error in the first place, right?
Now I will ask you to follow the proper Intel kernel review rules and
get proper approval for the patchset before submitting it to us again,
as basic things like this are not supposed to be caught by us, but by
your internal review process first.
good luck!
greg k-h
On Sat, Mar 25, 2023 at 11:27:03PM +0100, Greg Kroah-Hartman wrote: > On Sat, Mar 25, 2023 at 11:47:06PM +0800, Ye Xiang wrote: > > +static void ljca_aux_release(struct device *dev) {} > > Sorry, but creating an empty release function just to shut the kernel up > is NOT how to properly do this, you all know better. This is totally > broken. The kernel was telling you what you had to do, don't think that > you are being smarter than it by doing this, otherwise we would have > never had the kernel spit out that error in the first place, right? Got it. Will use the release callback to free memory used by the auxiliary device instead. > > Now I will ask you to follow the proper Intel kernel review rules and > get proper approval for the patchset before submitting it to us again, > as basic things like this are not supposed to be caught by us, but by > your internal review process first. Thanks. I will get approval on Intel's internal review first before sending the next version out for review. -- Thanks Ye Xiang
On 25.03.23 16:47, Ye Xiang wrote: Hi, > +static int ljca_parse(struct ljca_dev *dev, struct ljca_msg *header) > +{ > + struct ljca_stub *stub; > + > + 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; > + } > + > + if (stub->ipacket.ibuf && stub->ipacket.ibuf_len) { Here you read those values from ipacket. > + unsigned int newlen; > + > + newlen = min_t(unsigned int, header->len, *stub->ipacket.ibuf_len); Here you read them again. > + > + *stub->ipacket.ibuf_len = newlen; > + memcpy(stub->ipacket.ibuf, header->data, newlen); And here you read them again. The compiler is free to generate a memory access for ipacket.ibuf, which may read a new value from RAM. That means here you have a window in which you can no longer guarantee stub->ipacket.ibuf != NULL, if you set it to NULL elsewhere. > + } > + > + 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; > + 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; And here you set stub->ipacket.ibuf to NULL. If you do that and want consistency, you'll need to use READ_ONCE and a local variable if you read this in interrupt. > + mutex_unlock(&dev->mutex); > +error_put: > + usb_autopm_put_interface(dev->intf); > + > + return ret; > +} > + HTH, Oliver