| Message ID | 1693806261-12958-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
On Mon, Sep 4, 2023 at 7:44 AM Wentong Wu <wentong.wu@intel.com> wrote: > > Add driver for Intel La Jolla Cove Adapter (LJCA) device. This > IO-expander expands additional functions to the host system > such as GPIO, I2C and SPI with USB host interface. We add 4 > drivers to support this device: a USB driver, a GPIO chip driver, > a I2C controller driver and a SPI controller driver. > Please stop spamming the list with resends. Give reviewers at least a couple days to respond. Bartosz
Hi, On 9/4/23 07:44, Wentong Wu 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. > > The sub-modules of LJCA can use ljca_transfer() to issue a transfer > between host and hardware. And ljca_register_event_cb is exported > to LJCA sub-module drivers for hardware event subscription. > > 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: Wentong Wu <wentong.wu@intel.com> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > Tested-by: Hans de Goede <hdegoede@redhat.com> > --- <snip> > +#ifdef CONFIG_ACPI > +struct ljca_match_ids_walk_data { > + const struct acpi_device_id *ids; > + const char *uid; > + struct acpi_device *adev; > +}; > + > +static const struct acpi_device_id ljca_gpio_hids[] = { > + { "INTC1074" }, > + { "INTC1096" }, > + { "INTC100B" }, > + { "INTC10D1" }, > + {}, > +}; > + > +static const struct acpi_device_id ljca_i2c_hids[] = { > + { "INTC1075" }, > + { "INTC1097" }, > + { "INTC100C" }, > + { "INTC10D2" }, > + {}, > +}; > + > +static const struct acpi_device_id ljca_spi_hids[] = { > + { "INTC1091" }, > + { "INTC1098" }, > + { "INTC100D" }, > + { "INTC10D3" }, > + {}, > +}; > + > +static int ljca_match_device_ids(struct acpi_device *adev, void *data) > +{ > + struct ljca_match_ids_walk_data *wd = data; > + const char *uid = acpi_device_uid(adev); > + > + if (acpi_match_device_ids(adev, wd->ids)) > + return 0; > + > + if (!wd->uid) > + goto match; > + > + if (!uid) > + uid = "0"; > + else > + uid = memchr(uid, wd->uid[0], strlen(uid)); Note this line can be simplified to: uid = strchr(uid, wd->uid[0]); Regards, Hans
> From: Hans de Goede <hdegoede@redhat.com> > > Hi, > > On 9/4/23 07:44, Wentong Wu wrote: > > Add driver for Intel La Jolla Cove Adapter (LJCA) device. This > > IO-expander expands additional functions to the host system such as > > GPIO, I2C and SPI with USB host interface. We add 4 drivers to support > > this device: a USB driver, a GPIO chip driver, a I2C controller driver > > and a SPI controller driver. > > > > --- > > v14: > > - fix build error: implicit declaration of function > 'acpi_dev_clear_dependencies' > > > > v13: > > - make ljca-usb more robust with the help of Hans de Goede > > - call acpi_dev_clear_dependencies() to mark _DEP ACPI dependencies > > on the I2C controller as satisfied, and patch is from Hans de Goede > > Thank you I can confirm that v14 works well on my ThinkPad x1 yoga gen 8: > > Tested-by: Hans de Goede <hdegoede@redhat.com> Thanks, v14 also works well on my setup(RPL platform). BR, Wentong > > Note I still have one small remark for patch 1/4, I'll reply to the patch itself with > the remark. > > Regards, > > Hans > > > > > v12: > > - switch dev_err to dev_dbg for i2c-ljca driver > > - avoid err printing because of calling usb_kill_urb when attempts to > > resubmit the rx urb > > > > v11: > > - switch dev_err to dev_dbg for i2c-ljca driver > > - remove message length check because of defined quirk structure > > - remove I2C_FUNC_SMBUS_EMUL support > > > > v10: > > - remove ljca_i2c_format_slave_addr > > - remove memset before write write w_packet > > - make ljca_i2c_stop void and print err message in case failure > > - use dev_err_probe in ljca_i2c_probe function > > > > v9: > > - overhaul usb-ljca driver to make it more structured and easy > > understand > > - fix memory leak issue for usb-ljca driver > > - add spinlock to protect tx_buf and ex_buf > > - change exported APIs for usb-ljca driver > > - unify prefix for structures and functions for i2c-ljca driver > > - unify prefix for structures and functions for spi-ljca driver > > - unify prefix for structures and functions for gpio-ljca driver > > - update gpio-ljca, i2c-ljca and spi-ljca drivers according to > > usb-ljca's changes > > > > Wentong Wu (4): > > usb: Add support for Intel LJCA device > > i2c: Add support for Intel LJCA USB I2C driver > > spi: Add support for Intel LJCA USB SPI driver > > gpio: update Intel LJCA USB GPIO driver > > > > drivers/gpio/Kconfig | 4 +- > > drivers/gpio/gpio-ljca.c | 246 +++++++------ > > drivers/i2c/busses/Kconfig | 11 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-ljca.c | 334 +++++++++++++++++ > > drivers/spi/Kconfig | 11 + > > drivers/spi/Makefile | 1 + > > drivers/spi/spi-ljca.c | 297 +++++++++++++++ > > drivers/usb/misc/Kconfig | 14 + > > drivers/usb/misc/Makefile | 1 + > > drivers/usb/misc/usb-ljca.c | 834 > ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/usb/ljca.h | 113 ++++++ > > 12 files changed, 1762 insertions(+), 105 deletions(-) create mode > > 100644 drivers/i2c/busses/i2c-ljca.c create mode 100644 > > drivers/spi/spi-ljca.c create mode 100644 drivers/usb/misc/usb-ljca.c > > create mode 100644 include/linux/usb/ljca.h
On 04.09.23 07:44, Wentong Wu wrote: > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > + const void *obuf, unsigned int obuf_len, void *ibuf, > + unsigned int ibuf_len, bool ack, unsigned long timeout) > +{ > + unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len; > + struct ljca_msg *header = adap->tx_buf; > + unsigned long flags; > + unsigned int actual; > + int ret = 0; > + > + if (msg_len > adap->tx_buf_len) > + return -EINVAL; > + > + mutex_lock(&adap->mutex); > + > + spin_lock_irqsave(&adap->lock, flags); > + > + header->type = type; > + header->cmd = cmd; > + header->len = obuf_len; > + if (obuf) > + memcpy(header->data, obuf, obuf_len); > + > + header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0); > + > + adap->ex_buf = ibuf; > + adap->ex_buf_len = ibuf_len; > + adap->actual_length = 0; > + > + spin_unlock_irqrestore(&adap->lock, flags); > + > + reinit_completion(&adap->cmd_completion); > + > + usb_autopm_get_interface(adap->intf); > + > + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header, > + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS); > + if (!ret && ack) { > + ret = wait_for_completion_timeout(&adap->cmd_completion, > + timeout); Let's suppose we are here, when a device is unplugged. timeout is arbitrary as it is passed down to us. > + if (ret < 0) { > + goto out; > + } if (!ret) { > + ret = -ETIMEDOUT; > + goto out; > + } > + } > + ret = adap->actual_length; > + > +out: > + spin_lock_irqsave(&adap->lock, flags); > + adap->ex_buf = NULL; > + adap->ex_buf_len = 0; > + > + memset(header, 0, sizeof(*header)); > + spin_unlock_irqrestore(&adap->lock, flags); > + > + usb_autopm_put_interface(adap->intf); > + > + mutex_unlock(&adap->mutex); > + > + return ret; > +} > + [..] > +static void ljca_disconnect(struct usb_interface *interface) > +{ > + struct ljca_adapter *adap = usb_get_intfdata(interface); > + struct ljca_client *client, *next; > + > + usb_kill_urb(adap->rx_urb); > + > + list_for_each_entry_safe(client, next, &adap->client_list, link) { > + auxiliary_device_delete(&client->auxdev); > + auxiliary_device_uninit(&client->auxdev); > + > + list_del_init(&client->link); > + kfree(client); > + } > + > + usb_free_urb(adap->rx_urb); > + > + usb_put_intf(adap->intf); > + > + mutex_destroy(&adap->mutex); > +} And we execute this. rx_urb is killed and does nothing. I see nothing that terminates the waiting if you hit the wrong window. It seems like this needs to complete the waiting. Regards Oliver
On 04.09.23 15:52, Wu, Wentong wrote: >> From: Oliver Neukum <oneukum@suse.com> >> And we execute this. rx_urb is killed and does nothing. >> I see nothing that terminates the waiting if you hit the wrong window. > > I think the auxiliary_device_delete will trigger the remove function of ljca > client in his own sub system, and the delete() or remove() of every subsystem > will not return until the ongoing transfer(probably blocked by above > cmd_completion) complete. And that also makes sure no more transfers > comes to there. Sure, you will not free used memory. But what allows you to be sure that you make any progress at all? That is that you will hang arbitrarily long in disconnect? Regards Oliver
Hi Oliver, Thanks for your review > From: Oliver Neukum <oneukum@suse.com> > On 04.09.23 15:52, Wu, Wentong wrote: > >> From: Oliver Neukum <oneukum@suse.com> > > >> And we execute this. rx_urb is killed and does nothing. > >> I see nothing that terminates the waiting if you hit the wrong window. > > > > I think the auxiliary_device_delete will trigger the remove function > > of ljca client in his own sub system, and the delete() or remove() of > > every subsystem will not return until the ongoing transfer(probably > > blocked by above > > cmd_completion) complete. And that also makes sure no more transfers > > comes to there. > > Sure, you will not free used memory. But what allows you to be sure that you make any progress at all? > > That is that you will hang arbitrarily long in disconnect? This routine isn't called in an interrupt context, and it allows sleep or wait something before the real shutdown like many drivers' remove() or disconnect() do. If we want to speed up the disconnect(), below changes is to complete the cmd_completion if usb_kill_urb() has been called, but there is still possibility ljca client init one more transfer before auxiliary_device_delete() @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) if (urb->status) { /* sync/async unlink faults aren't errors */ - if (urb->status == -ENOENT || urb->status == -ECONNRESET || + if (urb->status == -ENOENT) { + complete(&adap->cmd_completion); + return; + } + if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) return; perhaps we could add one more 'status' field in ljca_adapter to represent disconnect() happen or not, and it will be checked in the beginning of ljca_send() to avoid any new transfer. BR, Wentong > > Regards > Oliver
On 05.09.23 04:20, Wu, Wentong wrote: Hi, >> That is that you will hang arbitrarily long in disconnect? > This routine isn't called in an interrupt context, and it allows sleep or wait > something before the real shutdown like many drivers' remove() or > disconnect() do. It is, however, in the context of a kernel thread. We can wait, but not for arbitrary periods. > If we want to speed up the disconnect(), below changes is to complete the > cmd_completion if usb_kill_urb() has been called, but there is still possibility > ljca client init one more transfer before auxiliary_device_delete() > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > if (urb->status) { > /* sync/async unlink faults aren't errors */ > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > + if (urb->status == -ENOENT) { > + complete(&adap->cmd_completion); > + return; I'd say you'd break suspend() by such a change. You cannot complete in the interrupt handler, unless you can determine why the URB is killed. > + } > + if (urb->status == -ECONNRESET || > urb->status == -ESHUTDOWN) > return; > > perhaps we could add one more 'status' field in ljca_adapter to represent > disconnect() happen or not, and it will be checked in the beginning of > ljca_send() to avoid any new transfer. That would be a solution. Regards Oliver
> From: Oliver Neukum <oneukum@suse.com> > > On 05.09.23 04:20, Wu, Wentong wrote: > > Hi, > > >> That is that you will hang arbitrarily long in disconnect? > > This routine isn't called in an interrupt context, and it allows sleep > > or wait something before the real shutdown like many drivers' remove() > > or > > disconnect() do. > > It is, however, in the context of a kernel thread. We can wait, but not for > arbitrary periods. AFAIK, this is very common. > > > If we want to speed up the disconnect(), below changes is to complete > > the cmd_completion if usb_kill_urb() has been called, but there is > > still possibility ljca client init one more transfer before > > auxiliary_device_delete() > > > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > > > if (urb->status) { > > /* sync/async unlink faults aren't errors */ > > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > > + if (urb->status == -ENOENT) { > > + complete(&adap->cmd_completion); > > + return; > > I'd say you'd break suspend() by such a change. > You cannot complete in the interrupt handler, unless you can determine why the > URB is killed. With below status field in ljca_adapter to determine if it's killed by disconnect(). If this is preferred, I could cook the patch for review. If this is fixed, could you please help merge this usb-ljca driver so that it won't block others which depends on this driver? BR, Wentong > > > + } > > + if (urb->status == -ECONNRESET || > > urb->status == -ESHUTDOWN) > > return; > > > > perhaps we could add one more 'status' field in ljca_adapter to > > represent > > disconnect() happen or not, and it will be checked in the beginning of > > ljca_send() to avoid any new transfer. > > That would be a solution. > > > Regards > Oliver
On Tue, Sep 05, 2023 at 08:53:43AM +0000, Wu, Wentong wrote: > > From: Oliver Neukum <oneukum@suse.com> > > > > On 05.09.23 04:20, Wu, Wentong wrote: > > > > Hi, > > > > >> That is that you will hang arbitrarily long in disconnect? > > > This routine isn't called in an interrupt context, and it allows sleep > > > or wait something before the real shutdown like many drivers' remove() > > > or > > > disconnect() do. > > > > It is, however, in the context of a kernel thread. We can wait, but not for > > arbitrary periods. > > AFAIK, this is very common. > > > > > > If we want to speed up the disconnect(), below changes is to complete > > > the cmd_completion if usb_kill_urb() has been called, but there is > > > still possibility ljca client init one more transfer before > > > auxiliary_device_delete() > > > > > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > > > > > if (urb->status) { > > > /* sync/async unlink faults aren't errors */ > > > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > > > + if (urb->status == -ENOENT) { > > > + complete(&adap->cmd_completion); > > > + return; > > > > I'd say you'd break suspend() by such a change. > > You cannot complete in the interrupt handler, unless you can determine why the > > URB is killed. > > With below status field in ljca_adapter to determine if it's killed by disconnect(). > > If this is preferred, I could cook the patch for review. > > If this is fixed, could you please help merge this usb-ljca driver so that it won't > block others which depends on this driver? Please relax, we can't do anything until after -rc1 is out, and for me, that includes reviewing the code. There is no rush, or deadline, here at all. It will be merged when it is acceptable. thanks, greg k-h
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > On Tue, Sep 05, 2023 at 08:53:43AM +0000, Wu, Wentong wrote: > > > From: Oliver Neukum <oneukum@suse.com> > > > > > > On 05.09.23 04:20, Wu, Wentong wrote: > > > > > > Hi, > > > > > > >> That is that you will hang arbitrarily long in disconnect? > > > > This routine isn't called in an interrupt context, and it allows > > > > sleep or wait something before the real shutdown like many > > > > drivers' remove() or > > > > disconnect() do. > > > > > > It is, however, in the context of a kernel thread. We can wait, but > > > not for arbitrary periods. > > > > AFAIK, this is very common. > > > > > > > > > If we want to speed up the disconnect(), below changes is to > > > > complete the cmd_completion if usb_kill_urb() has been called, but > > > > there is still possibility ljca client init one more transfer > > > > before > > > > auxiliary_device_delete() > > > > > > > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > > > > > > > if (urb->status) { > > > > /* sync/async unlink faults aren't errors */ > > > > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > > > > + if (urb->status == -ENOENT) { > > > > + complete(&adap->cmd_completion); > > > > + return; > > > > > > I'd say you'd break suspend() by such a change. > > > You cannot complete in the interrupt handler, unless you can > > > determine why the URB is killed. > > > > With below status field in ljca_adapter to determine if it's killed by disconnect(). > > > > If this is preferred, I could cook the patch for review. > > > > If this is fixed, could you please help merge this usb-ljca driver so > > that it won't block others which depends on this driver? > > Please relax, we can't do anything until after -rc1 is out, and for me, that > includes reviewing the code. Thanks for your review. > > There is no rush, or deadline, here at all. It will be merged when it is acceptable. Understood, thanks BR, Wentong > > thanks, > > greg k-h