diff mbox series

[v4,1/5] mfd: Add support for Intel LJCA device

Message ID 20230309071100.2856899-2-xiang.ye@intel.com (mailing list archive)
State Superseded
Headers show
Series Add Intel LJCA device driver | expand

Commit Message

Ye Xiang March 9, 2023, 7:10 a.m. UTC
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. The sub-module of
LJCA can use ljca_transfer() to issue a transfer between host
and hardware.

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
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>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/usb/misc/Kconfig  |  13 +
 drivers/usb/misc/Makefile |   1 +
 drivers/usb/misc/ljca.c   | 969 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ljca.h  |  95 ++++
 4 files changed, 1078 insertions(+)
 create mode 100644 drivers/usb/misc/ljca.c
 create mode 100644 include/linux/mfd/ljca.h

Comments

Greg KH March 9, 2023, 7:49 a.m. UTC | #1
On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> This patch implements the USB part of Intel USB-I2C/GPIO/SPI adapter
> device named "La Jolla Cove Adapter" (LJCA).

Then why is this called "mfd" in the subject line?

>  include/linux/mfd/ljca.h  |  95 ++++

Why is this .h file in the mfd directory?

thanks,

greg k-h
Greg KH March 9, 2023, 7:52 a.m. UTC | #2
On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> +{
> +	struct fw_version version = {};
> +	unsigned int len = sizeof(version);
> +	int ret;
> +
> +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> +	if (ret)
> +		return ret;
> +
> +	if (len != sizeof(version)) {
> +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> +		return -EINVAL;
> +	}
> +
> +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> +}

You have sysfs files, yet no Documentation/ABI/ entries?  That's not
allowed, you know this :(

> +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			 size_t count)
> +{
> +	struct usb_interface *intf = to_usb_interface(dev);
> +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> +
> +	if (sysfs_streq(buf, "dfu"))
> +		ljca_mng_set_dfu_mode(mng_stub);
> +	else if (sysfs_streq(buf, "debug"))
> +		ljca_diag_set_trace_level(diag_stub, 3);

Sorry, but no, you can't do this in a sysfs file.

> +
> +	return count;
> +}
> +
> +static ssize_t cmd_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", "supported cmd: [dfu, debug]");

sysfs files do not show "help text".

thanks,

greg k-h
Arnd Bergmann March 9, 2023, 7:56 a.m. UTC | #3
On Thu, Mar 9, 2023, at 08:10, Ye Xiang wrote:

> 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)
>         }
>     }

I'm a bit confused by this bit, does that mean this only works
if the device is integrated on the mainboard and the BIOS is
aware of it? This won't work if  you plug it into a random
USB port, or have no ACPI firmware, right?

> Signed-off-by: Ye Xiang <xiang.ye@intel.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/usb/misc/Kconfig  |  13 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/ljca.c   | 969 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ljca.h  |  95 ++++

Why is this in driver/usb/misc? It looks like a normal
mfd driver to me, and it evenhas the header in include/linux/mfd/

     Arnd
Ye Xiang March 9, 2023, 9:10 a.m. UTC | #4
Hi Greg,

Thanks for the review.
On Thu, Mar 09, 2023 at 08:49:33AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > This patch implements the USB part of Intel USB-I2C/GPIO/SPI adapter
> > device named "La Jolla Cove Adapter" (LJCA).
> 
> Then why is this called "mfd" in the subject line?
Sorry, it's a mistake. I forget to change mfd to usb in the commit message
because I just move the ljca.c from driver/mfd to drivers/usb/misc according
to previous review comments[1]. And I will address this on v5.

[1] https://www.spinics.net/lists/kernel/msg4708451.html
> 
> >  include/linux/mfd/ljca.h  |  95 ++++
> 
> Why is this .h file in the mfd directory?
It's a mistake as well. Will address it by moving include/linux/mfd/ljca.h
to include/linux/usb/ljca.h.
>
> thanks,
> 
> greg k-h

--
Thanks
Ye Xiang
Greg KH March 9, 2023, 9:26 a.m. UTC | #5
On Thu, Mar 09, 2023 at 05:10:48PM +0800, Ye, Xiang wrote:
> Hi Greg,
> 
> Thanks for the review.
> On Thu, Mar 09, 2023 at 08:49:33AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > This patch implements the USB part of Intel USB-I2C/GPIO/SPI adapter
> > > device named "La Jolla Cove Adapter" (LJCA).
> > 
> > Then why is this called "mfd" in the subject line?
> Sorry, it's a mistake. I forget to change mfd to usb in the commit message
> because I just move the ljca.c from driver/mfd to drivers/usb/misc according
> to previous review comments[1]. And I will address this on v5.
> 
> [1] https://www.spinics.net/lists/kernel/msg4708451.html
> > 
> > >  include/linux/mfd/ljca.h  |  95 ++++
> > 
> > Why is this .h file in the mfd directory?
> It's a mistake as well. Will address it by moving include/linux/mfd/ljca.h
> to include/linux/usb/ljca.h.

Why do you need a .h file at all for such a tiny .c file?  If you don't
need it, don't have one please.

thanks,

greg k-h
Ye Xiang March 9, 2023, 9:31 a.m. UTC | #6
On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > +{
> > +	struct fw_version version = {};
> > +	unsigned int len = sizeof(version);
> > +	int ret;
> > +
> > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (len != sizeof(version)) {
> > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > +}
> 
> You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> allowed, you know this :(
The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> 
> > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > +			 size_t count)
> > +{
> > +	struct usb_interface *intf = to_usb_interface(dev);
> > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > +
> > +	if (sysfs_streq(buf, "dfu"))
> > +		ljca_mng_set_dfu_mode(mng_stub);
> > +	else if (sysfs_streq(buf, "debug"))
> > +		ljca_diag_set_trace_level(diag_stub, 3);
> 
> Sorry, but no, you can't do this in a sysfs file.
Do you mean that we can't use sysfs to send "debug" command to device?
Could you provide some detail or hints?
> 
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t cmd_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	return sysfs_emit(buf, "%s\n", "supported cmd: [dfu, debug]");
> 
> sysfs files do not show "help text".
Ok. I can remove this for this has been described in
Documentation/ABI/testing/sysfs-bus-usb-devices-ljca (added in patch 5
of this series).
> 
> thanks,
> 
> greg k-h

--
Thanks
Ye Xiang
Greg KH March 9, 2023, 9:41 a.m. UTC | #7
On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > +{
> > > +	struct fw_version version = {};
> > > +	unsigned int len = sizeof(version);
> > > +	int ret;
> > > +
> > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (len != sizeof(version)) {
> > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > +}
> > 
> > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > allowed, you know this :(
> The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/

Ah, missed that, sorry.

> > 
> > > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > > +			 size_t count)
> > > +{
> > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > +
> > > +	if (sysfs_streq(buf, "dfu"))
> > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > +	else if (sysfs_streq(buf, "debug"))
> > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > 
> > Sorry, but no, you can't do this in a sysfs file.
> Do you mean that we can't use sysfs to send "debug" command to device?

That is correct, use the kernel-wide debugging facilities that we have
for this NEVER create your own custom interface just for one tiny
driver, that is not allowed.

> Could you provide some detail or hints?

dev_dbg().

thanks,

greg k-h
Ye Xiang March 9, 2023, 10 a.m. UTC | #8
Hi Arnd,

Thanks for the review.
On Thu, Mar 09, 2023 at 08:56:05AM +0100, Arnd Bergmann wrote:
> On Thu, Mar 9, 2023, at 08:10, Ye Xiang wrote:
> 
> > 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)
> >         }
> >     }
> 
> I'm a bit confused by this bit, does that mean this only works
> if the device is integrated on the mainboard and the BIOS is
> aware of it? This won't work if  you plug it into a random
> USB port, or have no ACPI firmware, right?
LJCA can work both when there is an ACPI firmware or not.

When there is a config like "The minimum code in ASL" in DSDT, LJCA device and
its sub-devices will bind to right ACPI devices. Our current use case needs the
the ACPI binding. Because we have a camera sensor depending on this LJCA device,
We use the ACPI binding to control the probe order, making sure the camera
sensor probe after LJCA and LJCA-I2C.

When there isn't a config in DSDT, the LJCA device also can works. But
it won't have an acpi binding.

So, If you plug into a random USB port or have no ACPI binding, It just
does not have an ACPI binding.
> 
> > Signed-off-by: Ye Xiang <xiang.ye@intel.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/usb/misc/Kconfig  |  13 +
> >  drivers/usb/misc/Makefile |   1 +
> >  drivers/usb/misc/ljca.c   | 969 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/ljca.h  |  95 ++++
> 
> Why is this in driver/usb/misc? It looks like a normal
> mfd driver to me, and it evenhas the header in include/linux/mfd/
It's a mistake. I just moved it from driver/mfd and forget to move the header to right place.
It will be addressed on next version. 
> 
>      Arnd

--
Thanks
Ye Xiang
Andi Shyti March 9, 2023, 10:06 a.m. UTC | #9
On Thu, Mar 09, 2023 at 10:41:05AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> > On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > > +{
> > > > +	struct fw_version version = {};
> > > > +	unsigned int len = sizeof(version);
> > > > +	int ret;
> > > > +
> > > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (len != sizeof(version)) {
> > > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > > +}
> > > 
> > > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > > allowed, you know this :(
> > The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> > https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> 
> Ah, missed that, sorry.
> 
> > > 
> > > > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > > > +			 size_t count)
> > > > +{
> > > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > > > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > > +
> > > > +	if (sysfs_streq(buf, "dfu"))
> > > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > > +	else if (sysfs_streq(buf, "debug"))
> > > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > > 
> > > Sorry, but no, you can't do this in a sysfs file.
> > Do you mean that we can't use sysfs to send "debug" command to device?
> 
> That is correct, use the kernel-wide debugging facilities that we have
> for this NEVER create your own custom interface just for one tiny
> driver, that is not allowed.
> 
> > Could you provide some detail or hints?
> 
> dev_dbg().

I'm not sure this is the same thing, though, as it's not a drvier
to user debug message.

Ye, can you please explain better what this command does? You are
sending a LJCA_DIAG_SET_TRACE_LEVEL command to the device with a
parameter "3" which has a meaining only for you :)

What is ugly here is the parsing of "dfu" and "debug", I would
just make different boolean files, something like:

 echo 1 > ljca_dfu
 echo 0 > ljca_dfu

and

 echo N > ljca_trace_level

with a proper documentation of the trace levels.

The show counterparast can provide a feedback on how the
interfaces are set rather than providing some help message, which
is not what sysfs files are meant for.

Would this work?

Andi
Ye Xiang March 9, 2023, 10:16 a.m. UTC | #10
On Thu, Mar 09, 2023 at 10:26:27AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 05:10:48PM +0800, Ye, Xiang wrote:
> > Hi Greg,
> > 
> > Thanks for the review.
> > On Thu, Mar 09, 2023 at 08:49:33AM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > This patch implements the USB part of Intel USB-I2C/GPIO/SPI adapter
> > > > device named "La Jolla Cove Adapter" (LJCA).
> > > 
> > > Then why is this called "mfd" in the subject line?
> > Sorry, it's a mistake. I forget to change mfd to usb in the commit message
> > because I just move the ljca.c from driver/mfd to drivers/usb/misc according
> > to previous review comments[1]. And I will address this on v5.
> > 
> > [1] https://www.spinics.net/lists/kernel/msg4708451.html
> > > 
> > > >  include/linux/mfd/ljca.h  |  95 ++++
> > > 
> > > Why is this .h file in the mfd directory?
> > It's a mistake as well. Will address it by moving include/linux/mfd/ljca.h
> > to include/linux/usb/ljca.h.
> 
> Why do you need a .h file at all for such a tiny .c file?  If you don't
> need it, don't have one please.
The ljca.h file is needed because there are three function driver in
this series (LJCA-SPI, LJCA-GPIO, and LJCA-I2C) which needs LJCA API
to do actual transfers. LJCA-USB can be considered as a transport driver,
and the other three function driver use LJCA API to transfer their own
packages.

--
Thanks
Ye Xiang
Mark Brown March 9, 2023, 11:03 a.m. UTC | #11
On Thu, Mar 09, 2023 at 08:56:05AM +0100, Arnd Bergmann wrote:
> On Thu, Mar 9, 2023, at 08:10, Ye Xiang wrote:

> >  drivers/usb/misc/Kconfig  |  13 +
> >  drivers/usb/misc/Makefile |   1 +
> >  drivers/usb/misc/ljca.c   | 969 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/ljca.h  |  95 ++++

> Why is this in driver/usb/misc? It looks like a normal
> mfd driver to me, and it evenhas the header in include/linux/mfd/

It was a MFD in the original version, Lee asked for it to be moved to
USB: https://lore.kernel.org/r/20230305103456.GF2574592@google.com
Arnd Bergmann March 9, 2023, 11:30 a.m. UTC | #12
On Thu, Mar 9, 2023, at 12:03, Mark Brown wrote:
> On Thu, Mar 09, 2023 at 08:56:05AM +0100, Arnd Bergmann wrote:
>> On Thu, Mar 9, 2023, at 08:10, Ye Xiang wrote:
>
>> >  drivers/usb/misc/Kconfig  |  13 +
>> >  drivers/usb/misc/Makefile |   1 +
>> >  drivers/usb/misc/ljca.c   | 969 ++++++++++++++++++++++++++++++++++++++
>> >  include/linux/mfd/ljca.h  |  95 ++++
>
>> Why is this in driver/usb/misc? It looks like a normal
>> mfd driver to me, and it evenhas the header in include/linux/mfd/
>
> It was a MFD in the original version, Lee asked for it to be moved to
> USB: https://lore.kernel.org/r/20230305103456.GF2574592@google.com

Ok, I see. That should probably be mentioned in the patch
description then. I'm still not sure I follow the reasoning
for the split between "usb functionality" and and mfd part:
This is just a usb driver as it is attached to a usb bus, and
drivers usually get put into a directory based on what they
provide, not how they are attached to a parent bus.

    Arnd
Oliver Neukum March 9, 2023, 12:53 p.m. UTC | #13
On 09.03.23 08:10, Ye Xiang wrote:

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

Why do you make ibuf_len a pointer?

> +{
> +	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) {

Memory leak in error case. You must free 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(dev->udev, usb_sndbulkpipe(dev->udev, 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;

If that triggers, you may have a pending URB.
You must kill it.

> +			goto error_unlock;
> +		}
> +	}
> +
> +	stub->ipacket.ibuf = NULL;
> +	stub->ipacket.ibuf_len = NULL;
> +	ret = 0;
> +error_unlock:
> +	mutex_unlock(&dev->mutex);
> +error_put:
> +	usb_autopm_put_interface(dev->intf);
> +	return ret;
> +}


> +static int ljca_i2c_stub_init(struct ljca_dev *dev, struct ljca_i2c_descriptor *desc)
> +{
> +	struct ljca_i2c_info *i2c_info;
> +	struct ljca_stub *stub;
> +	int ret;
> +	int i;
> +
> +	stub = ljca_stub_alloc(dev, LJCA_I2C_STUB, size_mul(desc->num, sizeof(*i2c_info)));
> +	if (IS_ERR(stub))
> +		return PTR_ERR(stub);
> +
> +	i2c_info = ljca_priv(stub);
> +
> +	for (i = 0; i < desc->num; i++) {
> +		struct mfd_cell cell = {};
> +
> +		i2c_info[i].ljca = &stub->ljca;
> +		i2c_info[i].id = desc->info[i].id;
> +		i2c_info[i].capacity = desc->info[i].capacity;
> +		i2c_info[i].intr_pin = desc->info[i].intr_pin;
> +
> +		cell.name = "ljca-i2c";
> +		cell.platform_data = &i2c_info[i];
> +		cell.pdata_size = sizeof(i2c_info[i]);
> +
> +		if (i < ARRAY_SIZE(ljca_acpi_match_i2cs))
> +			cell.acpi_match = &ljca_acpi_match_i2cs[i];
> +
> +		ret = ljca_add_mfd_cell(dev, &cell);
> +		if (ret)
> +			return ret;

What happens to stub in the error case?

> +	}
> +
> +	return 0;
> +}


> +
> +static void ljca_disconnect(struct usb_interface *intf)
> +{
> +	struct ljca_dev *dev = usb_get_intfdata(intf);
> +
> +	ljca_stop(dev);

What prevents restarting the device here?

> +	mfd_remove_devices(&intf->dev);
> +	ljca_stub_cleanup(dev);
> +	ljca_delete(dev);
> +}
> +
> +static int ljca_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct ljca_dev *dev = usb_get_intfdata(intf);
> +
> +	ljca_stop(dev);
> +	return 0;
> +}
> +
> +static int ljca_resume(struct usb_interface *intf)
> +{
> +	struct ljca_dev *dev = usb_get_intfdata(intf);
> +
> +	return ljca_start(dev);

So here you report errors, but at the same time you set "started"
even if errors occur.

	Regards
		Oliver
Ye Xiang March 9, 2023, 3:45 p.m. UTC | #14
On Thu, Mar 09, 2023 at 11:06:25AM +0100, Andi Shyti wrote:
> On Thu, Mar 09, 2023 at 10:41:05AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> > > On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > > > +{
> > > > > +	struct fw_version version = {};
> > > > > +	unsigned int len = sizeof(version);
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	if (len != sizeof(version)) {
> > > > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > > > +}
> > > > 
> > > > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > > > allowed, you know this :(
> > > The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> > > https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> > 
> > Ah, missed that, sorry.
> > 
> > > > 
> > > > > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > > > > +			 size_t count)
> > > > > +{
> > > > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > > > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > > > > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > > > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > > > +
> > > > > +	if (sysfs_streq(buf, "dfu"))
> > > > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > > > +	else if (sysfs_streq(buf, "debug"))
> > > > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > > > 
> > > > Sorry, but no, you can't do this in a sysfs file.
> > > Do you mean that we can't use sysfs to send "debug" command to device?
> > 
> > That is correct, use the kernel-wide debugging facilities that we have
> > for this NEVER create your own custom interface just for one tiny
> > driver, that is not allowed.
> > 
> > > Could you provide some detail or hints?
> > 
> > dev_dbg().
But, this command is sent to SET LJCA Firmware logging level.
> 
> I'm not sure this is the same thing, though, as it's not a drvier
> to user debug message.
> 
> Ye, can you please explain better what this command does? You are
> sending a LJCA_DIAG_SET_TRACE_LEVEL command to the device with a
> parameter "3" which has a meaining only for you :)
Sure, the LJCA_DIAG_SET_TRACE_LEVEL command is used to set LJCA FW
logging level. 3 means debug level for FW. It is used for LJCA FW
debugging: when FW got some issue, we can send debug level to FW
to make FW print degging log for analysis.
> 
> What is ugly here is the parsing of "dfu" and "debug", I would
> just make different boolean files, something like:
> 
>  echo 1 > ljca_dfu
>  echo 0 > ljca_dfu
> 
> and
> 
>  echo N > ljca_trace_level
> 
> with a proper documentation of the trace levels.
> 
> The show counterparast can provide a feedback on how the
> interfaces are set rather than providing some help message, which
> is not what sysfs files are meant for.
> 
> Would this work?
Thanks. It should works, and I will try it.


--
Thanks
Ye Xiang
Greg KH March 9, 2023, 3:58 p.m. UTC | #15
On Thu, Mar 09, 2023 at 11:45:51PM +0800, Ye, Xiang wrote:
> On Thu, Mar 09, 2023 at 11:06:25AM +0100, Andi Shyti wrote:
> > On Thu, Mar 09, 2023 at 10:41:05AM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> > > > On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > > > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > > > > +{
> > > > > > +	struct fw_version version = {};
> > > > > > +	unsigned int len = sizeof(version);
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > > > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	if (len != sizeof(version)) {
> > > > > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > > > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > > > > +}
> > > > > 
> > > > > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > > > > allowed, you know this :(
> > > > The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> > > > https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> > > 
> > > Ah, missed that, sorry.
> > > 
> > > > > 
> > > > > > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > > > > > +			 size_t count)
> > > > > > +{
> > > > > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > > > > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > > > > > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > > > > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > > > > +
> > > > > > +	if (sysfs_streq(buf, "dfu"))
> > > > > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > > > > +	else if (sysfs_streq(buf, "debug"))
> > > > > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > > > > 
> > > > > Sorry, but no, you can't do this in a sysfs file.
> > > > Do you mean that we can't use sysfs to send "debug" command to device?
> > > 
> > > That is correct, use the kernel-wide debugging facilities that we have
> > > for this NEVER create your own custom interface just for one tiny
> > > driver, that is not allowed.
> > > 
> > > > Could you provide some detail or hints?
> > > 
> > > dev_dbg().
> But, this command is sent to SET LJCA Firmware logging level.

What command?

This isn't documented at all, sorry, I don't understand what you are
trying to do here.

> > I'm not sure this is the same thing, though, as it's not a drvier
> > to user debug message.
> > 
> > Ye, can you please explain better what this command does? You are
> > sending a LJCA_DIAG_SET_TRACE_LEVEL command to the device with a
> > parameter "3" which has a meaining only for you :)
> Sure, the LJCA_DIAG_SET_TRACE_LEVEL command is used to set LJCA FW
> logging level. 3 means debug level for FW. It is used for LJCA FW
> debugging: when FW got some issue, we can send debug level to FW
> to make FW print degging log for analysis.

And where is that printed?  In the kernel log?  Somewhere else?  What
does the firmware have to do with any of this?

thanks,

greg k-h
Ye Xiang March 9, 2023, 5:42 p.m. UTC | #16
On Thu, Mar 09, 2023 at 04:58:45PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 11:45:51PM +0800, Ye, Xiang wrote:
> > On Thu, Mar 09, 2023 at 11:06:25AM +0100, Andi Shyti wrote:
> > > On Thu, Mar 09, 2023 at 10:41:05AM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> > > > > On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > > > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > > > > > +{
> > > > > > > +	struct fw_version version = {};
> > > > > > > +	unsigned int len = sizeof(version);
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > > > > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	if (len != sizeof(version)) {
> > > > > > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > > > > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > > > > > +}
> > > > > > 
> > > > > > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > > > > > allowed, you know this :(
> > > > > The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> > > > > https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> > > > 
> > > > Ah, missed that, sorry.
> > > > 
> > > > > > 
> > > > > > > +static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > > > > > > +			 size_t count)
> > > > > > > +{
> > > > > > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > > > > > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > > > > > > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > > > > > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > > > > > +
> > > > > > > +	if (sysfs_streq(buf, "dfu"))
> > > > > > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > > > > > +	else if (sysfs_streq(buf, "debug"))
> > > > > > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > > > > > 
> > > > > > Sorry, but no, you can't do this in a sysfs file.
> > > > > Do you mean that we can't use sysfs to send "debug" command to device?
> > > > 
> > > > That is correct, use the kernel-wide debugging facilities that we have
> > > > for this NEVER create your own custom interface just for one tiny
> > > > driver, that is not allowed.
> > > > 
> > > > > Could you provide some detail or hints?
> > > > 
> > > > dev_dbg().
> > But, this command is sent to SET LJCA Firmware logging level.
> 
> What command?
ljca_diag_set_trace_level send LJCA_DIAG_SET_TRACE_LEVEL command to LJCA
FW to set FW log level.
> 
> This isn't documented at all, sorry, I don't understand what you are
> trying to do here.
Sorry for the doc missing. will try to add more details on comments and
Documentation/ABI/ entries.

> 
> > > I'm not sure this is the same thing, though, as it's not a drvier
> > > to user debug message.
> > > 
> > > Ye, can you please explain better what this command does? You are
> > > sending a LJCA_DIAG_SET_TRACE_LEVEL command to the device with a
> > > parameter "3" which has a meaining only for you :)
> > Sure, the LJCA_DIAG_SET_TRACE_LEVEL command is used to set LJCA FW
> > logging level. 3 means debug level for FW. It is used for LJCA FW
> > debugging: when FW got some issue, we can send debug level to FW
> > to make FW print degging log for analysis.
> 
> And where is that printed?  In the kernel log?  Somewhere else?  What
> does the firmware have to do with any of this?
The firmware log was printed through UART port of LJCA device to another
computer just for debugging purposes. After sending LJCA_DIAG_SET_TRACE_LEVEL
command to LJCA device, it will print logging according to the log level sent
before.

--
Thanks
Ye Xiang
Ye Xiang March 10, 2023, 4:14 a.m. UTC | #17
Hi Oliver,

Thanks for your review.
On Thu, Mar 09, 2023 at 01:53:28PM +0100, Oliver Neukum wrote:
> 
> 
> On 09.03.23 08:10, Ye Xiang wrote:
> 
> > +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)
> 
> Why do you make ibuf_len a pointer?
Because ibuf_len is also used as output of this function here.
It stores the actual length of ibuf receive from LJCA device.
> 
> > +{
> > +	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) {
> 
> Memory leak in error case. You must free header.
Good catch. Thanks. Will address it.
> 
> > +		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(dev->udev, usb_sndbulkpipe(dev->udev, 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;
> 
> If that triggers, you may have a pending URB.
> You must kill it.
which URB? I guess what you mean is dev->in_urb?
But the in_urb should always be up to waiting for message from firmware,
even through this timeout happen.

> 
> > +			goto error_unlock;
> > +		}
> > +	}
> > +
> > +	stub->ipacket.ibuf = NULL;
> > +	stub->ipacket.ibuf_len = NULL;
> > +	ret = 0;
> > +error_unlock:
> > +	mutex_unlock(&dev->mutex);
> > +error_put:
> > +	usb_autopm_put_interface(dev->intf);
> > +	return ret;
> > +}
> 
> 
> > +static int ljca_i2c_stub_init(struct ljca_dev *dev, struct ljca_i2c_descriptor *desc)
> > +{
> > +	struct ljca_i2c_info *i2c_info;
> > +	struct ljca_stub *stub;
> > +	int ret;
> > +	int i;
> > +
> > +	stub = ljca_stub_alloc(dev, LJCA_I2C_STUB, size_mul(desc->num, sizeof(*i2c_info)));
> > +	if (IS_ERR(stub))
> > +		return PTR_ERR(stub);
> > +
> > +	i2c_info = ljca_priv(stub);
> > +
> > +	for (i = 0; i < desc->num; i++) {
> > +		struct mfd_cell cell = {};
> > +
> > +		i2c_info[i].ljca = &stub->ljca;
> > +		i2c_info[i].id = desc->info[i].id;
> > +		i2c_info[i].capacity = desc->info[i].capacity;
> > +		i2c_info[i].intr_pin = desc->info[i].intr_pin;
> > +
> > +		cell.name = "ljca-i2c";
> > +		cell.platform_data = &i2c_info[i];
> > +		cell.pdata_size = sizeof(i2c_info[i]);
> > +
> > +		if (i < ARRAY_SIZE(ljca_acpi_match_i2cs))
> > +			cell.acpi_match = &ljca_acpi_match_i2cs[i];
> > +
> > +		ret = ljca_add_mfd_cell(dev, &cell);
> > +		if (ret)
> > +			return ret;
> 
> What happens to stub in the error case?
ljca_add_mfd_cell only failed when krealloc_array failing. When
ljca_add_mfd_cell fails, the related stub just be left alone here.

Maybe I should free the stub here when fails? what is your advice?
> 
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
> > +
> > +static void ljca_disconnect(struct usb_interface *intf)
> > +{
> > +	struct ljca_dev *dev = usb_get_intfdata(intf);
> > +
> > +	ljca_stop(dev);
> 
> What prevents restarting the device here?
Why need to restart the device here?
The ljca_disconnect function should be called when the LJCA device is
unplugged.
> 
> > +	mfd_remove_devices(&intf->dev);
> > +	ljca_stub_cleanup(dev);
> > +	ljca_delete(dev);
> > +}
> > +
> > +static int ljca_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > +	struct ljca_dev *dev = usb_get_intfdata(intf);
> > +
> > +	ljca_stop(dev);
> > +	return 0;
> > +}
> > +
> > +static int ljca_resume(struct usb_interface *intf)
> > +{
> > +	struct ljca_dev *dev = usb_get_intfdata(intf);
> > +
> > +	return ljca_start(dev);
> 
> So here you report errors, but at the same time you set "started"
> even if errors occur.
Thanks, Got it. Will address it on next version.

--
Thanks
Ye Xiang
Oliver Neukum March 13, 2023, 1:27 p.m. UTC | #18
On 10.03.23 05:14, Ye, Xiang wrote:
> Hi Oliver,

Hi,

sorry for the delayed answer.

> Thanks for your review.
> On Thu, Mar 09, 2023 at 01:53:28PM +0100, Oliver Neukum wrote:
>>
>>
>> On 09.03.23 08:10, Ye Xiang wrote:
>>
>>> +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)
>>
>> Why do you make ibuf_len a pointer?
> Because ibuf_len is also used as output of this function here.
> It stores the actual length of ibuf receive from LJCA device.

Yes, I understand that now, thank you for the explanation, yet
that is problematic, if we look at another issue. See further down:

>>> +		ret = -ENODEV;
>>> +		goto error_put;
>>> +	}
>>> +
>>> +	mutex_lock(&dev->mutex);
>>> +	stub->cur_cmd = cmd;
>>> +	stub->ipacket.ibuf = ibuf;
>>> +	stub->ipacket.ibuf_len = ibuf_len;

Here you store the pointer into the stub. Hence we must make sure
that the location it points to stays valid.

>>> +	stub->acked = false;
>>> +	ret = usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 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;

The function will return an error in the timeout case.
This may be a trivial observation but it becomes important.

>> If that triggers, you may have a pending URB.

I misspoke. Pending IO would have been correct.

>> You must kill it.
> which URB? I guess what you mean is dev->in_urb?
> But the in_urb should always be up to waiting for message from firmware,
> even through this timeout happen.


Now let's look at ljca_mng_reset_handshake(). I am afraid I have to quote
its first part in full:

+static int ljca_mng_reset_handshake(struct ljca_stub *stub)
+{
+	struct ljca_mng_priv *priv;
+	__le32 reset_id;
+	__le32 reset_id_ret = 0;
+	unsigned int ilen = sizeof(__le32);

This is on the _stack_
Highly important !!!

+	int ret;
+
+	priv = ljca_priv(stub);
+	reset_id = cpu_to_le32(priv->reset_id++);
+	ret = ljca_stub_write(stub, LJCA_MNG_RESET_NOTIFY, &reset_id, sizeof(reset_id),
+			      &reset_id_ret, &ilen, true, LJCA_USB_WRITE_ACK_TIMEOUT_MS);

If we run into the timeout error case, ret will be -ETIMEDOUT.

+	if (ret)
+		return ret;

And thus here we return and free the stack _including_ "ilen", which we
still have a pointer to. That means if the operation concludes after
a timeout, we _will_ follow a rogue pointer.
A couple of functions have this race condition.


>> What happens to stub in the error case?
> ljca_add_mfd_cell only failed when krealloc_array failing. When
> ljca_add_mfd_cell fails, the related stub just be left alone here.
> 
> Maybe I should free the stub here when fails? what is your advice?

Yes, that is the cleanest solution.

>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>>
>>> +
>>> +static void ljca_disconnect(struct usb_interface *intf)
>>> +{
>>> +	struct ljca_dev *dev = usb_get_intfdata(intf);
>>> +
>>> +	ljca_stop(dev);
>>
>> What prevents restarting the device here?

Sorry, you are calling ljca_start() only in probe(9 and resume()
Your code is correct.

	Regards
		Oliver
Ye Xiang March 14, 2023, 7:15 a.m. UTC | #19
Hi Oliver,

Thanks for the review.
On Mon, Mar 13, 2023 at 02:27:50PM +0100, Oliver Neukum wrote:
> On 10.03.23 05:14, Ye, Xiang wrote:
> > Hi Oliver,
> 
> Hi,
> 
> sorry for the delayed answer.
No problem.
> 
> > Thanks for your review.
> > On Thu, Mar 09, 2023 at 01:53:28PM +0100, Oliver Neukum wrote:
> > > 
> > > 
> > > On 09.03.23 08:10, Ye Xiang wrote:
> > > 
> > > > +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)
> > > 
> > > Why do you make ibuf_len a pointer?
> > Because ibuf_len is also used as output of this function here.
> > It stores the actual length of ibuf receive from LJCA device.
> 
> Yes, I understand that now, thank you for the explanation, yet
> that is problematic, if we look at another issue. See further down:
> 
> > > > +		ret = -ENODEV;
> > > > +		goto error_put;
> > > > +	}
> > > > +
> > > > +	mutex_lock(&dev->mutex);
> > > > +	stub->cur_cmd = cmd;
> > > > +	stub->ipacket.ibuf = ibuf;
> > > > +	stub->ipacket.ibuf_len = ibuf_len;
> 
> Here you store the pointer into the stub. Hence we must make sure
> that the location it points to stays valid.
> 

> Now let's look at ljca_mng_reset_handshake(). I am afraid I have to quote
> its first part in full:
> 
> +static int ljca_mng_reset_handshake(struct ljca_stub *stub)
> +{
> +	struct ljca_mng_priv *priv;
> +	__le32 reset_id;
> +	__le32 reset_id_ret = 0;
> +	unsigned int ilen = sizeof(__le32);
> 
> This is on the _stack_
> Highly important !!!
> 
> +	int ret;
> +
> +	priv = ljca_priv(stub);
> +	reset_id = cpu_to_le32(priv->reset_id++);
> +	ret = ljca_stub_write(stub, LJCA_MNG_RESET_NOTIFY, &reset_id, sizeof(reset_id),
> +			      &reset_id_ret, &ilen, true, LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> 
> If we run into the timeout error case, ret will be -ETIMEDOUT.
> 
> +	if (ret)
> +		return ret;
> 
> And thus here we return and free the stack _including_ "ilen", which we
> still have a pointer to. That means if the operation concludes after
> a timeout, we _will_ follow a rogue pointer.
> A couple of functions have this race condition.
Got it. Will fix that on next version.
> 
> 

Thanks
Ye Xiang
diff mbox series

Patch

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index a5f7652db7da..59ec120c26d4 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -273,6 +273,19 @@  config USB_LINK_LAYER_TEST
 	  Layer Test Device. Say Y only when you want to conduct USB Super Speed
 	  Link Layer Test for host controllers.
 
+config USB_LJCA
+	tristate "Intel La Jolla Cove Adapter support"
+	select MFD_CORE
+	depends on USB
+	help
+	  This adds support for Intel La Jolla Cove USB-I2C/SPI/GPIO
+	  Master Adapter (LJCA). Additional drivers such as I2C_LJCA,
+	  GPIO_LJCA and SPI_LJCA must be enabled in order to use the
+	  functionality of the device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called ljca.
+
 config USB_CHAOSKEY
 	tristate "ChaosKey random number generator driver support"
 	depends on HW_RANDOM
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 93581baec3a8..6f6adfbe17e0 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_USB_HUB_USB251XB)		+= usb251xb.o
 obj-$(CONFIG_USB_HSIC_USB3503)		+= usb3503.o
 obj-$(CONFIG_USB_HSIC_USB4604)		+= usb4604.o
 obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
+obj-$(CONFIG_USB_LJCA)			+= ljca.o
 
 obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
diff --git a/drivers/usb/misc/ljca.c b/drivers/usb/misc/ljca.c
new file mode 100644
index 000000000000..e9520f3d2904
--- /dev/null
+++ b/drivers/usb/misc/ljca.c
@@ -0,0 +1,969 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel La Jolla Cove Adapter USB driver
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ */
+
+#include <linux/dev_printk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/ljca.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+enum ljca_acpi_match_adr {
+	LJCA_ACPI_MATCH_GPIO,
+	LJCA_ACPI_MATCH_I2C1,
+	LJCA_ACPI_MATCH_I2C2,
+	LJCA_ACPI_MATCH_SPI1,
+	LJCA_ACPI_MATCH_SPI2,
+};
+
+static struct mfd_cell_acpi_match ljca_acpi_match_gpio = {
+	.adr = LJCA_ACPI_MATCH_GPIO,
+};
+
+static struct mfd_cell_acpi_match ljca_acpi_match_i2cs[] = {
+	{
+		.adr = LJCA_ACPI_MATCH_I2C1,
+	},
+	{
+		.adr = LJCA_ACPI_MATCH_I2C2,
+	},
+};
+
+static struct mfd_cell_acpi_match ljca_acpi_match_spis[] = {
+	{
+		.adr = LJCA_ACPI_MATCH_SPI1,
+	},
+	{
+		.adr = LJCA_ACPI_MATCH_SPI2,
+	},
+};
+
+struct ljca_msg {
+	u8 type;
+	u8 cmd;
+	u8 flags;
+	u8 len;
+	u8 data[];
+} __packed;
+
+struct fw_version {
+	u8 major;
+	u8 minor;
+	__le16 patch;
+	__le16 build;
+} __packed;
+
+/**
+ * enum ljca_stub_type - Stub type supported by LJCA.
+ * @LJCA_MNG_STUB: Provides Management messages.
+ * @LJCA_DIAG_STUB: provides Diagnose messages.
+ * @LJCA_GPIO_STUB: provides GPIO functionality.
+ * @LJCA_I2C_STUB: provides I2C functionality.
+ * @LJCA_SPI_STUB: provides SPI functionality.
+ */
+enum ljca_stub_type {
+	LJCA_MNG_STUB = 1,
+	LJCA_DIAG_STUB,
+	LJCA_GPIO_STUB,
+	LJCA_I2C_STUB,
+	LJCA_SPI_STUB,
+};
+
+/* command Flags */
+#define LJCA_ACK_FLAG	BIT(0)
+#define LJCA_RESP_FLAG	BIT(1)
+#define LJCA_CMPL_FLAG	BIT(2)
+
+/* 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,
+};
+
+struct ljca_i2c_ctr_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+} __packed;
+
+struct ljca_i2c_descriptor {
+	u8 num;
+	struct ljca_i2c_ctr_info info[];
+} __packed;
+
+struct ljca_spi_ctr_info {
+	u8 id;
+	u8 capacity;
+} __packed;
+
+struct ljca_spi_descriptor {
+	u8 num;
+	struct ljca_spi_ctr_info info[];
+} __packed;
+
+struct ljca_bank_descriptor {
+	u8 bank_id;
+	u8 pin_num;
+
+	/* 1 bit for each gpio, 1 means valid */
+	u32 valid_pins;
+} __packed;
+
+struct ljca_gpio_descriptor {
+	u8 pins_per_bank;
+	u8 bank_num;
+	struct ljca_bank_descriptor bank_desc[];
+} __packed;
+
+#define LJCA_MAX_PACKET_SIZE	64
+#define LJCA_MAX_PAYLOAD_SIZE (LJCA_MAX_PACKET_SIZE - sizeof(struct ljca_msg))
+#define LJCA_USB_WRITE_TIMEOUT_MS	200
+#define LJCA_USB_WRITE_ACK_TIMEOUT_MS	500
+#define LJCA_USB_ENUM_STUB_TIMEOUT_MS	20
+
+struct ljca_event_cb_entry {
+	ljca_event_cb_t notify;
+	void *context;
+};
+
+struct ljca_dev {
+	struct usb_device *udev;
+	struct usb_interface *intf;
+	u8 in_ep; /* the address of the bulk in endpoint */
+	u8 out_ep; /* the address of the bulk out endpoint */
+
+	/* the urb/buffer for read */
+	struct urb *in_urb;
+	unsigned char *ibuf;
+	size_t ibuf_len;
+
+	bool started;
+	struct list_head stubs_list;
+
+	/* to wait for an ongoing write ack */
+	wait_queue_head_t ack_wq;
+
+	struct mfd_cell *cells;
+	int cell_count;
+	/* mutex to protect package transfer with LJCA device */
+	struct mutex mutex;
+};
+
+struct ljca {
+	u8 type;
+	struct ljca_dev *dev;
+};
+
+struct ljca_stub_packet {
+	unsigned int *ibuf_len;
+	u8 *ibuf;
+};
+
+struct ljca_stub {
+	struct list_head list;
+	struct usb_interface *intf;
+	struct ljca_stub_packet ipacket;
+	u8 type;
+
+	/* for identify ack */
+	bool acked;
+	int cur_cmd;
+
+	struct ljca_event_cb_entry event_entry;
+	/* lock to protect event_entry */
+	spinlock_t event_cb_lock;
+
+	struct ljca ljca;
+	unsigned long priv[];
+};
+
+static inline void *ljca_priv(struct ljca_stub *stub)
+{
+	return stub->priv;
+}
+
+static bool ljca_validate(struct ljca_msg *header, u32 data_len)
+{
+	return header->len + sizeof(*header) == data_len;
+}
+
+static struct ljca_stub *ljca_stub_alloc(struct ljca_dev *dev, u8 type, int priv_size)
+{
+	struct ljca_stub *stub;
+
+	stub = kzalloc(struct_size(stub, priv, priv_size), GFP_KERNEL);
+	if (!stub)
+		return ERR_PTR(-ENOMEM);
+
+	stub->type = type;
+	stub->intf = dev->intf;
+	stub->ljca.dev = dev;
+	stub->ljca.type = stub->type;
+	spin_lock_init(&stub->event_cb_lock);
+	list_add_tail(&stub->list, &dev->stubs_list);
+	return stub;
+}
+
+static struct ljca_stub *ljca_stub_find(struct ljca_dev *dev, u8 type)
+{
+	struct ljca_stub *stub;
+
+	list_for_each_entry(stub, &dev->stubs_list, list) {
+		if (stub->type == type)
+			return stub;
+	}
+
+	dev_err(&dev->intf->dev, "USB stub not found, type:%d\n", type);
+	return ERR_PTR(-ENODEV);
+}
+
+static void ljca_stub_notify(struct ljca_stub *stub, u8 cmd, const void *evt_data, int len)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&stub->event_cb_lock, flags);
+	if (stub->event_entry.notify && stub->event_entry.context)
+		stub->event_entry.notify(stub->event_entry.context, cmd, evt_data, len);
+	spin_unlock_irqrestore(&stub->event_cb_lock, flags);
+}
+
+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) {
+		unsigned int newlen;
+
+		newlen = min_t(unsigned int, header->len, *stub->ipacket.ibuf_len);
+
+		*stub->ipacket.ibuf_len = newlen;
+		memcpy(stub->ipacket.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) {
+		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(dev->udev, usb_sndbulkpipe(dev->udev, 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;
+		}
+	}
+
+	stub->ipacket.ibuf = NULL;
+	stub->ipacket.ibuf_len = NULL;
+	ret = 0;
+error_unlock:
+	mutex_unlock(&dev->mutex);
+error_put:
+	usb_autopm_put_interface(dev->intf);
+	return ret;
+}
+
+static int ljca_transfer_internal(struct ljca *ljca, u8 cmd, const void *obuf,
+				  unsigned int obuf_len, void *ibuf, unsigned int *ibuf_len,
+				  bool wait_ack)
+{
+	struct ljca_stub *stub;
+
+	stub = ljca_stub_find(ljca->dev, ljca->type);
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	return ljca_stub_write(stub, cmd, obuf, obuf_len, ibuf, ibuf_len, wait_ack,
+			       LJCA_USB_WRITE_ACK_TIMEOUT_MS);
+}
+
+int ljca_transfer(struct ljca *ljca, u8 cmd, const void *obuf, unsigned int obuf_len, void *ibuf,
+		  unsigned int *ibuf_len)
+{
+	return ljca_transfer_internal(ljca, cmd, obuf, obuf_len, ibuf, ibuf_len, true);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
+
+int ljca_transfer_noack(struct ljca *ljca, u8 cmd, const void *obuf, unsigned int obuf_len)
+{
+	return ljca_transfer_internal(ljca, cmd, obuf, obuf_len, NULL, NULL, false);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
+
+int ljca_register_event_cb(struct ljca *ljca, ljca_event_cb_t event_cb, void *context)
+{
+	struct ljca_stub *stub;
+	unsigned long flags;
+
+	stub = ljca_stub_find(ljca->dev, ljca->type);
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	spin_lock_irqsave(&stub->event_cb_lock, flags);
+	stub->event_entry.notify = event_cb;
+	stub->event_entry.context = context;
+	spin_unlock_irqrestore(&stub->event_cb_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
+
+void ljca_unregister_event_cb(struct ljca *ljca)
+{
+	struct ljca_stub *stub;
+	unsigned long flags;
+
+	stub = ljca_stub_find(ljca->dev, ljca->type);
+	if (IS_ERR(stub))
+		return;
+
+	spin_lock_irqsave(&stub->event_cb_lock, flags);
+	stub->event_entry.notify = NULL;
+	stub->event_entry.context = NULL;
+	spin_unlock_irqrestore(&stub->event_cb_lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
+
+static void ljca_stub_cleanup(struct ljca_dev *dev)
+{
+	struct ljca_stub *stub, *next;
+
+	list_for_each_entry_safe(stub, next, &dev->stubs_list, list) {
+		list_del_init(&stub->list);
+		kfree(stub);
+	}
+}
+
+static void ljca_read_complete(struct urb *urb)
+{
+	struct ljca_msg *header = urb->transfer_buffer;
+	struct ljca_dev *dev = urb->context;
+	int len = urb->actual_length;
+	int ret;
+
+	WARN_ON_ONCE(!dev);
+	WARN_ON_ONCE(!header);
+
+	if (urb->status) {
+		/* sync/async unlink faults aren't errors */
+		if (urb->status == -ENOENT || urb->status == -ECONNRESET ||
+		    urb->status == -ESHUTDOWN)
+			return;
+
+		dev_err(&dev->intf->dev, "read bulk urb transfer failed: %d\n", urb->status);
+		goto resubmit;
+	}
+
+	dev_dbg(&dev->intf->dev, "receive: type:%d cmd:%d flags:%d len:%d\n", header->type,
+		header->cmd, header->flags, header->len);
+
+	if (!ljca_validate(header, len)) {
+		dev_err(&dev->intf->dev, "data not correct header->len:%d payload_len:%d\n ",
+			header->len, len);
+		goto resubmit;
+	}
+
+	ret = ljca_parse(dev, header);
+	if (ret)
+		dev_err(&dev->intf->dev, "failed to parse data: ret:%d type:%d len:%d\n", ret,
+			header->type, header->len);
+
+resubmit:
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret)
+		dev_err(&dev->intf->dev, "failed submitting read urb, error %d\n", ret);
+}
+
+static int ljca_start(struct ljca_dev *dev)
+{
+	int ret;
+
+	usb_fill_bulk_urb(dev->in_urb, dev->udev, usb_rcvbulkpipe(dev->udev, 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);
+
+	dev->started = true;
+	return ret;
+}
+
+struct ljca_mng_priv {
+	u32 reset_id;
+};
+
+static int ljca_mng_reset_handshake(struct ljca_stub *stub)
+{
+	struct ljca_mng_priv *priv;
+	__le32 reset_id;
+	__le32 reset_id_ret = 0;
+	unsigned int ilen = sizeof(__le32);
+	int ret;
+
+	priv = ljca_priv(stub);
+	reset_id = cpu_to_le32(priv->reset_id++);
+	ret = ljca_stub_write(stub, LJCA_MNG_RESET_NOTIFY, &reset_id, sizeof(reset_id),
+			      &reset_id_ret, &ilen, true, LJCA_USB_WRITE_ACK_TIMEOUT_MS);
+	if (ret)
+		return ret;
+
+	if (ilen != sizeof(reset_id_ret) || reset_id_ret != reset_id) {
+		dev_err(&stub->intf->dev, "mng reset notify failed reset_id:%u/%u\n",
+			le32_to_cpu(reset_id_ret), le32_to_cpu(reset_id));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ljca_add_mfd_cell(struct ljca_dev *dev, struct mfd_cell *cell)
+{
+	struct mfd_cell *new_cells;
+
+	new_cells = krealloc_array(dev->cells, dev->cell_count + 1, sizeof(*new_cells), GFP_KERNEL);
+	if (!new_cells)
+		return -ENOMEM;
+
+	memcpy(&new_cells[dev->cell_count], cell, sizeof(*cell));
+	dev->cells = new_cells;
+	dev->cell_count++;
+
+	return 0;
+}
+
+static int ljca_gpio_stub_init(struct ljca_dev *dev, struct ljca_gpio_descriptor *desc)
+{
+	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
+	struct ljca_gpio_info *gpio_info;
+	struct mfd_cell cell = {};
+	struct ljca_stub *stub;
+	int gpio_num;
+	int i;
+
+	gpio_num = desc->pins_per_bank * desc->bank_num;
+	if (gpio_num > LJCA_MAX_GPIO_NUM)
+		return -EINVAL;
+
+	stub = ljca_stub_alloc(dev, LJCA_GPIO_STUB, sizeof(*gpio_info));
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	gpio_info = ljca_priv(stub);
+	gpio_info->ljca = &stub->ljca;
+	gpio_info->num = gpio_num;
+
+	for (i = 0; i < desc->bank_num; i++)
+		valid_pin[i] = desc->bank_desc[i].valid_pins;
+
+	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
+
+	cell.name = "ljca-gpio";
+	cell.platform_data = gpio_info;
+	cell.pdata_size = sizeof(*gpio_info);
+	cell.acpi_match = &ljca_acpi_match_gpio;
+
+	return ljca_add_mfd_cell(dev, &cell);
+}
+
+static int ljca_mng_enum_gpio(struct ljca_stub *stub)
+{
+	struct ljca_dev *dev = usb_get_intfdata(stub->intf);
+	char buf[LJCA_MAX_PAYLOAD_SIZE];
+	struct ljca_gpio_descriptor *desc;
+	unsigned int len = LJCA_MAX_PAYLOAD_SIZE;
+	int ret;
+
+	ret = ljca_stub_write(stub, LJCA_MNG_ENUM_GPIO, NULL, 0, buf, &len, true,
+			      LJCA_USB_ENUM_STUB_TIMEOUT_MS);
+	if (ret)
+		return ret;
+
+	desc = (struct ljca_gpio_descriptor *)buf;
+	if (len != struct_size(desc, bank_desc, desc->bank_num)) {
+		dev_err(&stub->intf->dev, "GPIO enumeration failed, len:%u bank_num:%u\n", len,
+			desc->bank_num);
+		return -EINVAL;
+	}
+
+	return ljca_gpio_stub_init(dev, desc);
+}
+
+static int ljca_i2c_stub_init(struct ljca_dev *dev, struct ljca_i2c_descriptor *desc)
+{
+	struct ljca_i2c_info *i2c_info;
+	struct ljca_stub *stub;
+	int ret;
+	int i;
+
+	stub = ljca_stub_alloc(dev, LJCA_I2C_STUB, size_mul(desc->num, sizeof(*i2c_info)));
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	i2c_info = ljca_priv(stub);
+
+	for (i = 0; i < desc->num; i++) {
+		struct mfd_cell cell = {};
+
+		i2c_info[i].ljca = &stub->ljca;
+		i2c_info[i].id = desc->info[i].id;
+		i2c_info[i].capacity = desc->info[i].capacity;
+		i2c_info[i].intr_pin = desc->info[i].intr_pin;
+
+		cell.name = "ljca-i2c";
+		cell.platform_data = &i2c_info[i];
+		cell.pdata_size = sizeof(i2c_info[i]);
+
+		if (i < ARRAY_SIZE(ljca_acpi_match_i2cs))
+			cell.acpi_match = &ljca_acpi_match_i2cs[i];
+
+		ret = ljca_add_mfd_cell(dev, &cell);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ljca_mng_enum_i2c(struct ljca_stub *stub)
+{
+	struct ljca_dev *dev = usb_get_intfdata(stub->intf);
+	char buf[LJCA_MAX_PAYLOAD_SIZE];
+	struct ljca_i2c_descriptor *desc;
+	unsigned int len = LJCA_MAX_PAYLOAD_SIZE;
+	int ret;
+
+	ret = ljca_stub_write(stub, LJCA_MNG_ENUM_I2C, NULL, 0, buf, &len, true,
+			      LJCA_USB_ENUM_STUB_TIMEOUT_MS);
+	if (ret) {
+		dev_err(&stub->intf->dev, "I2C enumeration failed, ret:%d len:%u\n", ret, len);
+		return ret;
+	}
+
+	desc = (struct ljca_i2c_descriptor *)buf;
+	return ljca_i2c_stub_init(dev, desc);
+}
+
+static int ljca_spi_stub_init(struct ljca_dev *dev, struct ljca_spi_descriptor *desc)
+{
+	struct ljca_spi_info *spi_info;
+	struct ljca_stub *stub;
+	int i;
+	int ret;
+
+	stub = ljca_stub_alloc(dev, LJCA_SPI_STUB, size_mul(desc->num, sizeof(*spi_info)));
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	spi_info = ljca_priv(stub);
+	for (i = 0; i < desc->num; i++) {
+		struct mfd_cell cell = {};
+
+		spi_info[i].ljca = &stub->ljca;
+		spi_info[i].id = desc->info[i].id;
+		spi_info[i].capacity = desc->info[i].capacity;
+
+		cell.name = "ljca-spi";
+		cell.platform_data = &spi_info[i];
+		cell.pdata_size = sizeof(spi_info[i]);
+		if (i < ARRAY_SIZE(ljca_acpi_match_spis))
+			cell.acpi_match = &ljca_acpi_match_spis[i];
+
+		ret = ljca_add_mfd_cell(dev, &cell);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ljca_mng_enum_spi(struct ljca_stub *stub)
+{
+	struct ljca_dev *dev = usb_get_intfdata(stub->intf);
+	char buf[LJCA_MAX_PAYLOAD_SIZE];
+	struct ljca_spi_descriptor *desc;
+	unsigned int len = LJCA_MAX_PAYLOAD_SIZE;
+	int ret;
+
+	ret = ljca_stub_write(stub, LJCA_MNG_ENUM_SPI, NULL, 0, buf, &len, true,
+			      LJCA_USB_ENUM_STUB_TIMEOUT_MS);
+	if (ret) {
+		dev_err(&stub->intf->dev, "SPI enumeration failed, ret:%d len:%d\n", ret, len);
+		return ret;
+	}
+
+	desc = (struct ljca_spi_descriptor *)buf;
+	return ljca_spi_stub_init(dev, desc);
+}
+
+static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
+{
+	struct fw_version version = {};
+	unsigned int len = sizeof(version);
+	int ret;
+
+	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
+			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
+	if (ret)
+		return ret;
+
+	if (len != sizeof(version)) {
+		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
+			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
+}
+
+static inline int ljca_mng_set_dfu_mode(struct ljca_stub *stub)
+{
+	return ljca_stub_write(stub, LJCA_MNG_SET_DFU_MODE, NULL, 0, NULL, NULL, true,
+			       LJCA_USB_WRITE_ACK_TIMEOUT_MS);
+}
+
+static int ljca_mng_link(struct ljca_dev *dev, struct ljca_stub *stub)
+{
+	int ret;
+
+	ret = ljca_mng_reset_handshake(stub);
+	if (ret)
+		return ret;
+
+	/* try to enum all the stubs */
+	ljca_mng_enum_gpio(stub);
+	ljca_mng_enum_i2c(stub);
+	ljca_mng_enum_spi(stub);
+
+	return 0;
+}
+
+static int ljca_mng_init(struct ljca_dev *dev)
+{
+	struct ljca_stub *stub;
+	struct ljca_mng_priv *priv;
+	int ret;
+
+	stub = ljca_stub_alloc(dev, LJCA_MNG_STUB, sizeof(*priv));
+	if (IS_ERR(stub))
+		return PTR_ERR(stub);
+
+	priv = ljca_priv(stub);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->reset_id = 0;
+
+	ret = ljca_mng_link(dev, stub);
+	if (ret)
+		dev_err(&dev->intf->dev, "mng stub link failed, ret:%d\n", ret);
+
+	return ret;
+}
+
+static inline int ljca_diag_set_trace_level(struct ljca_stub *stub, u8 level)
+{
+	return ljca_stub_write(stub, LJCA_DIAG_SET_TRACE_LEVEL, &level, sizeof(level), NULL, NULL,
+			       true, LJCA_USB_WRITE_ACK_TIMEOUT_MS);
+}
+
+static int ljca_diag_init(struct ljca_dev *dev)
+{
+	struct ljca_stub *stub;
+
+	stub = ljca_stub_alloc(dev, LJCA_DIAG_STUB, 0);
+
+	return PTR_ERR_OR_ZERO(stub);
+}
+
+static void ljca_delete(struct ljca_dev *dev)
+{
+	mutex_destroy(&dev->mutex);
+	usb_free_urb(dev->in_urb);
+	usb_put_intf(dev->intf);
+	usb_put_dev(dev->udev);
+	kfree(dev->ibuf);
+	kfree(dev->cells);
+	kfree(dev);
+}
+
+static int ljca_init(struct ljca_dev *dev)
+{
+	mutex_init(&dev->mutex);
+	init_waitqueue_head(&dev->ack_wq);
+	INIT_LIST_HEAD(&dev->stubs_list);
+
+	return 0;
+}
+
+static void ljca_stop(struct ljca_dev *dev)
+{
+	dev->started = false;
+	usb_kill_urb(dev->in_urb);
+}
+
+static ssize_t cmd_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			 size_t count)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
+	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
+	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
+
+	if (sysfs_streq(buf, "dfu"))
+		ljca_mng_set_dfu_mode(mng_stub);
+	else if (sysfs_streq(buf, "debug"))
+		ljca_diag_set_trace_level(diag_stub, 3);
+
+	return count;
+}
+
+static ssize_t cmd_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "supported cmd: [dfu, debug]");
+}
+static DEVICE_ATTR_RW(cmd);
+
+static ssize_t version_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
+	struct ljca_stub *stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
+
+	return ljca_mng_get_version(stub, buf);
+}
+static DEVICE_ATTR_RO(version);
+
+static struct attribute *ljca_attrs[] = {
+	&dev_attr_version.attr,
+	&dev_attr_cmd.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ljca);
+
+static int ljca_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+	struct ljca_dev *dev;
+	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+	int ret;
+
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	ljca_init(dev);
+	dev->udev = usb_get_dev(interface_to_usbdev(intf));
+	dev->intf = usb_get_intf(intf);
+
+	/* set up the endpoint information use only the first bulk-in and bulk-out endpoints */
+	ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
+	if (ret) {
+		dev_err(&intf->dev, "could not find both bulk-in and bulk-out endpoints\n");
+		goto error;
+	}
+
+	dev->ibuf_len = usb_endpoint_maxp(bulk_in);
+	dev->in_ep = bulk_in->bEndpointAddress;
+	dev->ibuf = kzalloc(dev->ibuf_len, GFP_KERNEL);
+	if (!dev->ibuf) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	dev->in_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!dev->in_urb) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	dev->out_ep = bulk_out->bEndpointAddress;
+	/* save our data pointer in this intf device */
+	usb_set_intfdata(intf, dev);
+	ret = ljca_start(dev);
+	if (ret) {
+		dev_err(&intf->dev, "bridge read start failed ret %d\n", ret);
+		goto error;
+	}
+
+	ret = ljca_mng_init(dev);
+	if (ret) {
+		dev_err(&intf->dev, "register mng stub failed ret %d\n", ret);
+		goto error_stop;
+	}
+
+	ret = ljca_diag_init(dev);
+	if (ret) {
+		dev_err(&intf->dev, "register diag stub failed ret %d\n", ret);
+		goto error_stop;
+	}
+
+	ret = mfd_add_hotplug_devices(&intf->dev, dev->cells, dev->cell_count);
+	if (ret) {
+		dev_err(&intf->dev, "failed to add mfd devices\n");
+		goto error_stop;
+	}
+
+	usb_enable_autosuspend(dev->udev);
+	return 0;
+error_stop:
+	ljca_stop(dev);
+error:
+	dev_err(&intf->dev, "LJCA USB device init failed\n");
+	/* this frees allocated memory */
+	ljca_stub_cleanup(dev);
+	ljca_delete(dev);
+	return ret;
+}
+
+static void ljca_disconnect(struct usb_interface *intf)
+{
+	struct ljca_dev *dev = usb_get_intfdata(intf);
+
+	ljca_stop(dev);
+	mfd_remove_devices(&intf->dev);
+	ljca_stub_cleanup(dev);
+	ljca_delete(dev);
+}
+
+static int ljca_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct ljca_dev *dev = usb_get_intfdata(intf);
+
+	ljca_stop(dev);
+	return 0;
+}
+
+static int ljca_resume(struct usb_interface *intf)
+{
+	struct ljca_dev *dev = usb_get_intfdata(intf);
+
+	return ljca_start(dev);
+}
+
+static const struct usb_device_id ljca_table[] = {
+	{USB_DEVICE(0x8086, 0x0b63)},
+	{}
+};
+MODULE_DEVICE_TABLE(usb, ljca_table);
+
+static struct usb_driver ljca_driver = {
+	.name = "ljca",
+	.probe = ljca_probe,
+	.disconnect = ljca_disconnect,
+	.suspend = ljca_suspend,
+	.resume = ljca_resume,
+	.id_table = ljca_table,
+	.dev_groups = ljca_groups,
+	.supports_autosuspend = 1,
+};
+module_usb_driver(ljca_driver);
+
+MODULE_AUTHOR("Ye Xiang <xiang.ye@intel.com>");
+MODULE_AUTHOR("Wang Zhifeng <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Zhang Lixu <lixu.zhang@intel.com>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/ljca.h b/include/linux/mfd/ljca.h
new file mode 100644
index 000000000000..9ae3ea242294
--- /dev/null
+++ b/include/linux/mfd/ljca.h
@@ -0,0 +1,95 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _LINUX_USB_LJCA_H_
+#define _LINUX_USB_LJCA_H_
+
+#include <linux/types.h>
+
+struct ljca;
+
+#define LJCA_MAX_GPIO_NUM 64
+struct ljca_gpio_info {
+	struct ljca *ljca;
+	unsigned int num;
+	DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM);
+};
+
+struct ljca_i2c_info {
+	struct ljca *ljca;
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+};
+
+struct ljca_spi_info {
+	struct ljca *ljca;
+	u8 id;
+	u8 capacity;
+};
+
+/**
+ * typedef ljca_event_cb_t - event callback function signature
+ *
+ * @context: the execution context of who registered this callback
+ * @cmd: the command from device for this event
+ * @evt_data: the event data payload
+ * @len: the event data payload length
+ *
+ * The callback function is called in interrupt context and the data payload is
+ * only valid during the call. If the user needs later access of the data, it
+ * must copy it.
+ */
+typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void *evt_data, int len);
+
+/**
+ * ljca_register_event_cb - register a callback function to receive events
+ *
+ * @ljca: ljca device handle
+ * @event_cb: callback function
+ * @context: execution context of event callback
+ *
+ * Return: 0 in case of success, negative value in case of error
+ */
+int ljca_register_event_cb(struct ljca *ljca, ljca_event_cb_t event_cb, void *context);
+
+/**
+ * ljca_unregister_event_cb - unregister the callback function for an event
+ *
+ * @ljca: ljca device handle
+ */
+void ljca_unregister_event_cb(struct ljca *ljca);
+
+/**
+ * ljca_transfer - issue a LJCA command and wait for a response and the
+ * associated data
+ *
+ * @ljca: ljca device handle
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device; it should
+ *	be 0 when obuf is NULL
+ * @ibuf: any data associated with the response will be copied here; it can be
+ *	NULL if the user doesn't need the response data
+ * @ibuf_len: must be initialized to the input buffer size; it will be modified
+ *	to indicate the actual data transferred; it shouldn't be NULL as well
+ *	when ibuf isn't NULL
+ *
+ * Return: 0 for success, negative value for errors
+ */
+int ljca_transfer(struct ljca *ljca, u8 cmd, const void *obuf, unsigned int obuf_len,
+		  void *ibuf, unsigned int *ibuf_len);
+
+/**
+ * ljca_transfer_noack - issue a LJCA command without a response
+ *
+ * @ljca: ljca device handle
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device
+ *
+ * Return: 0 for success, negative value for errors
+ */
+int ljca_transfer_noack(struct ljca *ljca, u8 cmd, const void *obuf, unsigned int obuf_len);
+
+#endif