[v19,1/4] usb: Add support for Intel LJCA device

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>
---
 drivers/usb/misc/Kconfig    |  13 +
 drivers/usb/misc/Makefile   |   1 +
 drivers/usb/misc/usb-ljca.c | 837 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/ljca.h    | 145 ++++++++
 4 files changed, 996 insertions(+)
 create mode 100644 drivers/usb/misc/usb-ljca.c
 create mode 100644 include/linux/usb/ljca.h

On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> +/* ljca cmd message structure */
> +struct ljca_msg {
> +	u8 type;
> +	u8 cmd;
> +	u8 flags;
> +	u8 len;
> +	u8 data[];

Shouldn't you be using the __counted_by attributes for all of these []
arrays so that the compiler knows what to do and how to check that you
don't overrun the buffer?

Adding that now will save you having to add it later, AND it might catch
bugs you already have so please add that.

> +
> +struct ljca_adapter {
> +	struct usb_interface *intf;
> +	struct usb_device *usb_dev;
> +	struct device *dev;
> +
> +	unsigned int rx_pipe;
> +	unsigned int tx_pipe;
> +
> +	/* urb for recv */
> +	struct urb *rx_urb;
> +	/* buffer for recv */
> +	void *rx_buf;
> +	unsigned int rx_len;
> +
> +	/* external buffer for recv */
> +	u8 *ex_buf;
> +	unsigned int ex_buf_len;
> +	/* actual data length copied to external buffer */
> +	unsigned int actual_length;
> +
> +	/* buffer for send */
> +	void *tx_buf;
> +	unsigned int tx_buf_len;
> +
> +	/* lock to protect tx_buf and ex_buf */
> +	spinlock_t lock;
> +
> +	struct completion cmd_completion;
> +
> +	/* mutex to protect command download */
> +	struct mutex mutex;
> +
> +	/* client device list */
> +	struct list_head client_list;
> +
> +	/* disconnect ongoing or not */
> +	bool disconnect;
> +
> +	/* used to reset firmware */

Can you use proper kernel doc for this structure instead of inline
comments?

> +	u32 reset_id;
> +};
> +
> +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 void ljca_handle_event(struct ljca_adapter *adap,
> +			      struct ljca_msg *header)
> +{
> +	struct ljca_client *client;
> +
> +	list_for_each_entry(client, &adap->client_list, link) {
> +		/*
> +		 * FIXME: currently only GPIO register event callback.
> +		 * firmware message structure should include id when
> +		 * multiple same type clients register event callback.
> +		 */

When will this be fixed?

If not now, why not?

> +		if (client->type == header->type) {
> +			scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> +				client->event_cb(client->context, header->cmd,
> +						 header->data, header->len);
> +			}
> +
> +			break;
> +		}
> +	}
> +}
> +
> +/* process command ack */
> +static void ljca_handle_cmd_ack(struct ljca_adapter *adap,
> +				struct ljca_msg *header)

We can use 100 columns...

> +{
> +	struct ljca_msg *tx_header = adap->tx_buf;
> +	unsigned int actual_len = 0;
> +	unsigned int ibuf_len;
> +	unsigned long flags;
> +	u8 *ibuf;
> +
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
> +		dev_err(adap->dev, "cmd ack mismatch error\n");

When you print error messages like this, who can do something about it?
And why print with interrupts disabled?

> +		spin_unlock_irqrestore(&adap->lock, flags);
> +		return;
> +	}
> +
> +	ibuf_len = adap->ex_buf_len;
> +	ibuf = adap->ex_buf;
> +
> +	if (ibuf && ibuf_len) {
> +		actual_len = min_t(unsigned int, header->len, ibuf_len);

You control both of these types, why aren't they the same type to start
with?  Why the force cast?

> +
> +		/* copy received data to external buffer */
> +		memcpy(ibuf, header->data, actual_len);
> +	}
> +	/* update copied data length */
> +	adap->actual_length = actual_len;

Wait, what happens if you don't actually copy the data?  Why lie and say
you did?  Where is the error handling?

> +
> +	spin_unlock_irqrestore(&adap->lock, flags);
> +
> +	complete(&adap->cmd_completion);
> +}
> +
> +static void ljca_recv(struct urb *urb)

No error handling?

> +{
> +	struct ljca_msg *header = urb->transfer_buffer;
> +	struct ljca_adapter *adap = urb->context;
> +	int ret;
> +
> +	if (urb->status) {
> +		/* sync/async unlink faults aren't errors */
> +		if (urb->status == -ENOENT) {
> +			/*
> +			 * directly complete the possible ongoing transfer
> +			 * during disconnect
> +			 */
> +			if (adap->disconnect)
> +				complete(&adap->cmd_completion);
> +			return;
> +		}
> +
> +		if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN)
> +			return;
> +
> +		dev_err(adap->dev, "recv urb error: %d\n", urb->status);
> +		goto resubmit;

You have an error, yet you don't report it you just spam the kernel log?
Why?  Doesn't this happen when a device is removed?

> +	}
> +
> +	if (header->len + sizeof(*header) != urb->actual_length)
> +		goto resubmit;
> +
> +	if (header->flags & LJCA_ACK_FLAG)
> +		ljca_handle_cmd_ack(adap, header);
> +	else
> +		ljca_handle_event(adap, header);
> +
> +resubmit:
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);

Why atomic, is this in a urb callback?

> +	if (ret && ret != -EPERM)
> +		dev_err(adap->dev, "resubmit recv urb error %d\n", ret);

What happens if ret is an error here?

> +}
> +
> +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> +		     const u8 *obuf, unsigned int obuf_len, u8 *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 int actual;
> +	int ret = 0;
> +
> +	if (adap->disconnect)
> +		return -ENODEV;
> +
> +	if (msg_len > adap->tx_buf_len)
> +		return -EINVAL;
> +
> +	mutex_lock(&adap->mutex);
> +
> +	scoped_guard(spinlock_irqsave, &adap->lock) {
> +		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;
> +	}

Do you really need a scoped guard when you can not fail out of the
block?

Why do you have both a mutex and spinlock grabed?

> +
> +	reinit_completion(&adap->cmd_completion);
> +
> +	usb_autopm_get_interface(adap->intf);

This can fail.  Why aren't you checking that?

> +
> +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> +
> +	usb_autopm_put_interface(adap->intf);
> +
> +	if (!ret && ack) {
> +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> +						  timeout);
> +		if (ret < 0) {
> +			goto out;
> +		} if (!ret) {
> +			ret = -ETIMEDOUT;
> +			goto out;
> +		}
> +	}
> +	ret = adap->actual_length;

Why are you not verifying that you sent what you wanted to send?

When you call this function, sometimes you check that the function sent
the proper amount of data, but in many places you do not, and you assume
that the full buffer was sent, which is not correct.  So please change
_this_ function to check that you sent the proper amount and then the
caller logic will be much simpler and actually work like you are using
it in many places (some places you got it right, some wrong, which is a
HUGE indication that the API is wrong because you wrote this code, and
if you can't get it right...)

> +out:
> +	scoped_guard(spinlock_irqsave, &adap->lock) {
> +		adap->ex_buf = NULL;
> +		adap->ex_buf_len = 0;
> +		memset(header, 0, sizeof(*header));
> +	}

Again, why a scoped guard for something like this that does not need it?

> +
> +	mutex_unlock(&adap->mutex);
> +
> +	return ret;
> +}
> +
> +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
> +		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len)
> +{
> +	return ljca_send(client->adapter, client->type, cmd,
> +			 obuf, obuf_len, ibuf, ibuf_len, true,
> +			 LJCA_WRITE_ACK_TIMEOUT_MS);
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
> +
> +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
> +			unsigned int obuf_len)
> +{
> +	return ljca_send(client->adapter, client->type, cmd, obuf,
> +			 obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS);
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
> +
> +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
> +			   void *context)
> +{
> +	unsigned long flags;
> +
> +	if (!event_cb)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&client->event_cb_lock, flags);
> +
> +	if (client->event_cb) {
> +		spin_unlock_irqrestore(&client->event_cb_lock, flags);
> +		return -EALREADY;
> +	}
> +
> +	client->event_cb = event_cb;
> +	client->context = context;
> +
> +	spin_unlock_irqrestore(&client->event_cb_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
> +
> +void ljca_unregister_event_cb(struct ljca_client *client)
> +{
> +	scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> +		client->event_cb = NULL;
> +		client->context = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
> +
> +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";

Are you sure this is a valid uid to use?  If so, why?  What happens if
this gets set to "0" for multiple ones?  Don't underestimate broken
firmware tables, but also don't paper over problems in ways that will be
impossible to notice and can cause problems.

Or am I mis-reading this function wrong, how can this ever be a valid
UID to compare against?

> +	else
> +		uid = strchr(uid, wd->uid[0]);
> +
> +	if (!uid || strcmp(uid, wd->uid))
> +		return 0;
> +
> +match:
> +	wd->adev = adev;
> +
> +	return 1;
> +}
> +
> +/* bind auxiliary device to acpi device */
> +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> +				  struct auxiliary_device *auxdev,
> +				  u64 adr, u8 id)
> +{
> +	struct ljca_match_ids_walk_data wd = { 0 };
> +	struct acpi_device *parent, *adev;
> +	struct device *dev = adap->dev;
> +	char uid[4];
> +
> +	parent = ACPI_COMPANION(dev);
> +	if (!parent)
> +		return;
> +
> +	/*
> +	 * get auxdev ACPI handle from the ACPI device directly
> +	 * under the parent that matches _ADR.
> +	 */
> +	adev = acpi_find_child_device(parent, adr, false);
> +	if (adev) {
> +		ACPI_COMPANION_SET(&auxdev->dev, adev);
> +		return;
> +	}
> +
> +	/*
> +	 * _ADR is a grey area in the ACPI specification, some
> +	 * platforms use _HID to distinguish children devices.
> +	 */
> +	switch (adr) {
> +	case LJCA_GPIO_ACPI_ADR:
> +		wd.ids = ljca_gpio_hids;
> +		break;
> +	case LJCA_I2C1_ACPI_ADR:
> +	case LJCA_I2C2_ACPI_ADR:
> +		snprintf(uid, sizeof(uid), "%d", id);
> +		wd.uid = uid;
> +		wd.ids = ljca_i2c_hids;
> +		break;
> +	case LJCA_SPI1_ACPI_ADR:
> +	case LJCA_SPI2_ACPI_ADR:
> +		wd.ids = ljca_spi_hids;
> +		break;
> +	default:
> +		dev_warn(dev, "unsupported _ADR\n");
> +		return;
> +	}
> +
> +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> +	if (wd.adev) {
> +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> +		return;
> +	}
> +
> +	parent = ACPI_COMPANION(dev->parent->parent);
> +	if (!parent)
> +		return;
> +
> +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> +	if (wd.adev)
> +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> +}
> +
> +static void ljca_auxdev_release(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +
> +	kfree(auxdev->dev.platform_data);
> +}
> +
> +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> +				  char *name, void *data, u64 adr)
> +{
> +	struct auxiliary_device *auxdev;
> +	struct ljca_client *client;
> +	int ret;
> +
> +	client = kzalloc(sizeof *client, GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->type = type;
> +	client->id = id;
> +	client->adapter = adap;
> +	spin_lock_init(&client->event_cb_lock);
> +
> +	auxdev = &client->auxdev;
> +	auxdev->name = name;
> +	auxdev->id = id;
> +
> +	auxdev->dev.parent = adap->dev;
> +	auxdev->dev.platform_data = data;
> +	auxdev->dev.release = ljca_auxdev_release;
> +
> +	ret = auxiliary_device_init(auxdev);
> +	if (ret)
> +		goto err_free;
> +
> +	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
> +
> +	ret = auxiliary_device_add(auxdev);
> +	if (ret)
> +		goto err_uninit;
> +
> +	list_add_tail(&client->link, &adap->client_list);
> +
> +	return 0;
> +
> +err_uninit:
> +	auxiliary_device_uninit(auxdev);
> +
> +err_free:
> +	kfree(client);
> +
> +	return ret;
> +}
> +
> +static int ljca_enumerate_gpio(struct ljca_adapter *adap)
> +{
> +	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
> +	struct ljca_gpio_descriptor *desc;
> +	struct ljca_gpio_info *gpio_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	int ret, gpio_num;
> +	unsigned int i;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* check firmware response */
> +	desc = (struct ljca_gpio_descriptor *)buf;
> +	if (ret != struct_size(desc, bank_desc, desc->bank_num))
> +		return -EINVAL;
> +
> +	gpio_num = desc->pins_per_bank * desc->bank_num;
> +	if (gpio_num > LJCA_MAX_GPIO_NUM)
> +		return -EINVAL;
> +
> +	/* construct platform data */
> +	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
> +	if (!gpio_info)
> +		return -ENOMEM;
> +	gpio_info->num = gpio_num;
> +
> +	for (i = 0; i < desc->bank_num; i++)
> +		valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
> +	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
> +
> +	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> +				     gpio_info, LJCA_GPIO_ACPI_ADR);
> +	if (ret)
> +		kfree(gpio_info);
> +
> +	return ret;
> +}
> +
> +static int ljca_enumerate_i2c(struct ljca_adapter *adap)
> +{
> +	struct ljca_i2c_descriptor *desc;
> +	struct ljca_i2c_info *i2c_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	unsigned int i;
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* check firmware response */
> +	desc = (struct ljca_i2c_descriptor *)buf;
> +	if (ret != struct_size(desc, info, desc->num))
> +		return -EINVAL;
> +
> +	for (i = 0; i < desc->num; i++) {
> +		/* construct platform data */
> +		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
> +		if (!i2c_info)
> +			return -ENOMEM;
> +
> +		i2c_info->id = desc->info[i].id;
> +		i2c_info->capacity = desc->info[i].capacity;
> +		i2c_info->intr_pin = desc->info[i].intr_pin;
> +
> +		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> +					     "ljca-i2c", i2c_info,
> +					     LJCA_I2C1_ACPI_ADR + i);
> +		if (ret) {
> +			kfree(i2c_info);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ljca_enumerate_spi(struct ljca_adapter *adap)
> +{
> +	struct ljca_spi_descriptor *desc;
> +	struct ljca_spi_info *spi_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	unsigned int i;
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	desc = (struct ljca_spi_descriptor *)buf;
> +	for (i = 0; i < desc->num; i++) {
> +		/* construct platform data */
> +		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
> +		if (!spi_info)
> +			return -ENOMEM;
> +
> +		spi_info->id = desc->info[i].id;
> +		spi_info->capacity = desc->info[i].capacity;
> +
> +		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> +					     "ljca-spi", spi_info,
> +					     LJCA_SPI1_ACPI_ADR + i);
> +		if (ret) {
> +			kfree(spi_info);
> +			return ret;

What about the other objects that you created here, don't you need to
unwind the new ones if one in the chain fails?  And if not, why stop at
the first failure?


> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ljca_reset_handshake(struct ljca_adapter *adap)
> +{
> +	__le32 reset_id = cpu_to_le32(adap->reset_id);
> +	__le32 reset_id_ret = 0;
> +	int ret;
> +
> +	adap->reset_id++;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id,
> +			sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32),
> +			true, LJCA_WRITE_ACK_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reset_id_ret != reset_id)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ljca_enumerate_clients(struct ljca_adapter *adap)
> +{
> +	int ret;
> +
> +	ret = ljca_reset_handshake(adap);
> +	if (ret)
> +		return ret;
> +
> +	ret = ljca_enumerate_gpio(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate GPIO error\n");
> +
> +	ret = ljca_enumerate_i2c(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate I2C error\n");
> +
> +	ret = ljca_enumerate_spi(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate SPI error\n");

So none of these "errors" are actually errors:

> +	return 0;

You return success?  Why?  Are they not actually problems?

thanks,

greg k-h

Hi Greg,

Thanks for your review

> From: Greg KH <gregkh@linuxfoundation.org>
> On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> > +/* ljca cmd message structure */
> > +struct ljca_msg {
> > +	u8 type;
> > +	u8 cmd;
> > +	u8 flags;
> > +	u8 len;
> > +	u8 data[];
> 
> Shouldn't you be using the __counted_by attributes for all of these [] arrays so
> that the compiler knows what to do and how to check that you don't overrun
> the buffer?

In this structure, len is not for data length, instead it's the length of header and data.

> 
> Adding that now will save you having to add it later, AND it might catch bugs you
> already have so please add that.

But, I will add the __counted_by attribute for others like below:

struct ljca_i2c_descriptor {
	u8 num;
	struct ljca_i2c_ctr_info info[] __counted_by(num);
} __packed;

struct ljca_spi_descriptor {
	u8 num;
	struct ljca_spi_ctr_info info[] __counted_by(num);
} __packed;

struct ljca_gpio_descriptor {
	u8 pins_per_bank;
	u8 bank_num;
	struct ljca_bank_descriptor bank_desc[] __counted_by(bank_num);
} __packed;

> 
> > +
> > +struct ljca_adapter {
> > +	struct usb_interface *intf;
> > +	struct usb_device *usb_dev;
> > +	struct device *dev;
> > +
> > +	unsigned int rx_pipe;
> > +	unsigned int tx_pipe;
> > +
> > +	/* urb for recv */
> > +	struct urb *rx_urb;
> > +	/* buffer for recv */
> > +	void *rx_buf;
> > +	unsigned int rx_len;
> > +
> > +	/* external buffer for recv */
> > +	u8 *ex_buf;
> > +	unsigned int ex_buf_len;
> > +	/* actual data length copied to external buffer */
> > +	unsigned int actual_length;
> > +
> > +	/* buffer for send */
> > +	void *tx_buf;
> > +	unsigned int tx_buf_len;
> > +
> > +	/* lock to protect tx_buf and ex_buf */
> > +	spinlock_t lock;
> > +
> > +	struct completion cmd_completion;
> > +
> > +	/* mutex to protect command download */
> > +	struct mutex mutex;
> > +
> > +	/* client device list */
> > +	struct list_head client_list;
> > +
> > +	/* disconnect ongoing or not */
> > +	bool disconnect;
> > +
> > +	/* used to reset firmware */
> 
> Can you use proper kernel doc for this structure instead of inline comments?

Ack, thanks

> 
> > +	u32 reset_id;
> > +};
> > +
> > +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 void ljca_handle_event(struct ljca_adapter *adap,
> > +			      struct ljca_msg *header)
> > +{
> > +	struct ljca_client *client;
> > +
> > +	list_for_each_entry(client, &adap->client_list, link) {
> > +		/*
> > +		 * FIXME: currently only GPIO register event callback.
> > +		 * firmware message structure should include id when
> > +		 * multiple same type clients register event callback.
> > +		 */
> 
> When will this be fixed?
> 
> If not now, why not?

Actually this doesn't impact current functionality because only GPIO register
event callback, but from coding perspective it should add the id in the message
structure. This fix should be done by firmware first, but many products have
already been running the firmware, it's not easy to update the firmware.

Can I just remove the 'FIXME' and leave the comment here?
 
> 
> > +		if (client->type == header->type) {
> > +			scoped_guard(spinlock_irqsave, &client-
> >event_cb_lock) {
> > +				client->event_cb(client->context, header->cmd,
> > +						 header->data, header->len);
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +/* process command ack */
> > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap,
> > +				struct ljca_msg *header)
> 
> We can use 100 columns...

Ok, I will make this in one row.

> 
> > +{
> > +	struct ljca_msg *tx_header = adap->tx_buf;
> > +	unsigned int actual_len = 0;
> > +	unsigned int ibuf_len;
> > +	unsigned long flags;
> > +	u8 *ibuf;
> > +
> > +	spin_lock_irqsave(&adap->lock, flags);
> > +
> > +	if (tx_header->type != header->type || tx_header->cmd != header->cmd)
> {
> > +		dev_err(adap->dev, "cmd ack mismatch error\n");
> 
> When you print error messages like this, who can do something about it?
> And why print with interrupts disabled?

Thanks, this will be like below: 

	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
		spin_unlock_irqrestore(&adap->lock, flags);
		dev_err(adap->dev, "cmd ack mismatch error\n");
		return;
	}

> 
> > +		spin_unlock_irqrestore(&adap->lock, flags);
> > +		return;
> > +	}
> > +
> > +	ibuf_len = adap->ex_buf_len;
> > +	ibuf = adap->ex_buf;
> > +
> > +	if (ibuf && ibuf_len) {
> > +		actual_len = min_t(unsigned int, header->len, ibuf_len);
> 
> You control both of these types, why aren't they the same type to start with?
> Why the force cast?

The len of header is defined by firmware, it should be u8 type. But the ex_buf_len
is passed by API users, I don't want users to do the cast if their buffer is large than
256.

> 
> > +
> > +		/* copy received data to external buffer */
> > +		memcpy(ibuf, header->data, actual_len);
> > +	}
> > +	/* update copied data length */
> > +	adap->actual_length = actual_len;
> 
> Wait, what happens if you don't actually copy the data?

This actual_length is the actual length of data copied to external buffer
where is to save the command response. The API callers should check
the response length according to the command you passed to this API.

> Why lie and say you did? Where is the error handling?

As I said, the API callers should have the error handing because they know
the exact response format, and actually I already the error handling in this
patch set.

> 
> > +
> > +	spin_unlock_irqrestore(&adap->lock, flags);
> > +
> > +	complete(&adap->cmd_completion);
> > +}
> > +
> > +static void ljca_recv(struct urb *urb)
> 
> No error handling?

Actually I have, I re-structure this function as below to make it more clear,

static void ljca_recv(struct urb *urb)
{
	struct ljca_msg *header = urb->transfer_buffer;
	struct ljca_adapter *adap = urb->context;
	int ret;

	switch (urb->status) {
	case 0:
		/* success */
		break;
	case -ENOENT:
		/*
		   * directly complete the possible ongoing transfer
		   * during disconnect
		   */
		if (adap->disconnect)
			complete(&adap->cmd_completion);
		return;
	case -ECONNRESET:
	case -ESHUTDOWN:
	case -EPIPE:
		/* rx urb is terminated */
		dev_dbg(adap->dev, "rx urb terminated with status: %d\n",
			urb->status);
		return;
	default:
		dev_dbg(adap->dev, "rx urb error: %d\n", urb->status);
		goto resubmit;
	}

	if (header->len + sizeof(*header) != urb->actual_length)
		goto resubmit;

	if (header->flags & LJCA_ACK_FLAG)
		ljca_handle_cmd_ack(adap, header);
	else
		ljca_handle_event(adap, header);

resubmit:
	ret = usb_submit_urb(urb, GFP_ATOMIC);
 	if (ret && ret != -EPERM)
		dev_err(adap->dev, "resubmit rx urb error %d\n", ret);
} 

> 
> > +{
> > +	struct ljca_msg *header = urb->transfer_buffer;
> > +	struct ljca_adapter *adap = urb->context;
> > +	int ret;
> > +
> > +	if (urb->status) {
> > +		/* sync/async unlink faults aren't errors */
> > +		if (urb->status == -ENOENT) {
> > +			/*
> > +			 * directly complete the possible ongoing transfer
> > +			 * during disconnect
> > +			 */
> > +			if (adap->disconnect)
> > +				complete(&adap->cmd_completion);
> > +			return;
> > +		}
> > +
> > +		if (urb->status == -ECONNRESET || urb->status == -
> ESHUTDOWN)
> > +			return;
> > +
> > +		dev_err(adap->dev, "recv urb error: %d\n", urb->status);
> > +		goto resubmit;
> 
> You have an error, yet you don't report it you just spam the kernel log?
> Why?  Doesn't this happen when a device is removed?

When device is removed, it will be covered by ESHUTDOWN case in the above
'if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) '

Most of the command error cases have been handled by the above
'if (usb->status == ', for others it will print error log, In the re-structured code,
I changed it to dev_dbg to avoid spam.

> 
> > +	}
> > +
> > +	if (header->len + sizeof(*header) != urb->actual_length)
> > +		goto resubmit;
> > +
> > +	if (header->flags & LJCA_ACK_FLAG)
> > +		ljca_handle_cmd_ack(adap, header);
> > +	else
> > +		ljca_handle_event(adap, header);
> > +
> > +resubmit:
> > +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> 
> Why atomic, is this in a urb callback?

Yes,

> 
> > +	if (ret && ret != -EPERM)
> > +		dev_err(adap->dev, "resubmit recv urb error %d\n", ret);
> 
> What happens if ret is an error here?

No command response, and there will be command timeout.

> 
> > +}
> > +
> > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> > +		     const u8 *obuf, unsigned int obuf_len, u8 *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 int actual;
> > +	int ret = 0;
> > +
> > +	if (adap->disconnect)
> > +		return -ENODEV;
> > +
> > +	if (msg_len > adap->tx_buf_len)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adap->mutex);
> > +
> > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > +		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;
> > +	}
> 
> Do you really need a scoped guard when you can not fail out of the block?

The scoped_guard is required by you with "Why not use the functionality in
cleanup.h for this lock? Makes this function much simpler." If I understand
correctly, so I use the scoped_guard where I can to make things simpler.

> 
> Why do you have both a mutex and spinlock grabed?

The mutex is to avoid command download concurrently

The spinlock is to protect tx_buf and ex_buf, which may be accessed by tx and rx
at the same time.

> 
> > +
> > +	reinit_completion(&adap->cmd_completion);
> > +
> > +	usb_autopm_get_interface(adap->intf);
> 
> This can fail.  Why aren't you checking that?

Ack, thanks, and it will be like below:

	ret = usb_autopm_get_interface(adap->intf);
	if (ret < 0)
		goto out;

> 
> > +
> > +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> > +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> > +
> > +	usb_autopm_put_interface(adap->intf);
> > +
> > +	if (!ret && ack) {
> > +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> > +						  timeout);
> > +		if (ret < 0) {
> > +			goto out;
> > +		} if (!ret) {
> > +			ret = -ETIMEDOUT;
> > +			goto out;
> > +		}
> > +	}
> > +	ret = adap->actual_length;
> 
> Why are you not verifying that you sent what you wanted to send?

As I said, the actual_length is the actual length of data copied to user buffer instead
of the length of what we want to send.

And even for verifying the length of what we want to send, the max length of the
message is sizeof(struct ljca_msg) + LJCA_MAX_PACKET_SIZE which is less than
the endpoint's max packet size, so I don't check the actual sent length in above
usb_bulk_msg().

> 
> When you call this function, sometimes you check that the function sent the
> proper amount of data, but in many places you do not, and you assume that the
> full buffer was sent, which is not correct.  So please change _this_ function to
> check that you sent the proper amount and then the caller logic will be much
> simpler and actually work like you are using it in many places (some places you
> got it right, some wrong, which is a HUGE indication that the API is wrong
> because you wrote this code, and if you can't get it right...)

As I said, the return value of this function is the response data length instead of
sent data length. And in this patch set, every caller has verified if the response
length matched with the sent command. 

> 
> > +out:
> > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > +		adap->ex_buf = NULL;
> > +		adap->ex_buf_len = 0;
> > +		memset(header, 0, sizeof(*header));
> > +	}
> 
> Again, why a scoped guard for something like this that does not need it?

As I said above, this is to address your last time review comment if I understand
correctly, I can switch to the original one if we don't need this scoped guard.

> 
> > +
> > +	mutex_unlock(&adap->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
> > +		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len) {
> > +	return ljca_send(client->adapter, client->type, cmd,
> > +			 obuf, obuf_len, ibuf, ibuf_len, true,
> > +			 LJCA_WRITE_ACK_TIMEOUT_MS);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
> > +
> > +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
> > +			unsigned int obuf_len)
> > +{
> > +	return ljca_send(client->adapter, client->type, cmd, obuf,
> > +			 obuf_len, NULL, 0, false,
> LJCA_WRITE_ACK_TIMEOUT_MS); }
> > +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
> > +
> > +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
> > +			   void *context)
> > +{
> > +	unsigned long flags;
> > +
> > +	if (!event_cb)
> > +		return -EINVAL;
> > +
> > +	spin_lock_irqsave(&client->event_cb_lock, flags);
> > +
> > +	if (client->event_cb) {
> > +		spin_unlock_irqrestore(&client->event_cb_lock, flags);
> > +		return -EALREADY;
> > +	}
> > +
> > +	client->event_cb = event_cb;
> > +	client->context = context;
> > +
> > +	spin_unlock_irqrestore(&client->event_cb_lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
> > +
> > +void ljca_unregister_event_cb(struct ljca_client *client) {
> > +	scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> > +		client->event_cb = NULL;
> > +		client->context = NULL;
> > +	}
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
> > +
> > +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";
> 
> Are you sure this is a valid uid to use?  If so, why?  What happens if this gets set
> to "0" for multiple ones?  Don't underestimate broken firmware tables, but also
> don't paper over problems in ways that will be impossible to notice and can
> cause problems.

This actually has been discussed in previous email as bellow: 

some DSDTs have only 1 ACPI companion for the 2 I2C controllers
and these don't set a UID at all. On these models only the first I2C
controller is used. So if a HID match has no UID use "0" for the HID.
assigning the ACPI companion to the first I2C controller.
An example device with this setup is the Dell Latitude 9420.

> 
> Or am I mis-reading this function wrong, how can this ever be a valid UID to
> compare against?
> 
> > +	else
> > +		uid = strchr(uid, wd->uid[0]);
> > +
> > +	if (!uid || strcmp(uid, wd->uid))
> > +		return 0;
> > +
> > +match:
> > +	wd->adev = adev;
> > +
> > +	return 1;
> > +}
> > +
> > +/* bind auxiliary device to acpi device */ static void
> > +ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> > +				  struct auxiliary_device *auxdev,
> > +				  u64 adr, u8 id)
> > +{
> > +	struct ljca_match_ids_walk_data wd = { 0 };
> > +	struct acpi_device *parent, *adev;
> > +	struct device *dev = adap->dev;
> > +	char uid[4];
> > +
> > +	parent = ACPI_COMPANION(dev);
> > +	if (!parent)
> > +		return;
> > +
> > +	/*
> > +	 * get auxdev ACPI handle from the ACPI device directly
> > +	 * under the parent that matches _ADR.
> > +	 */
> > +	adev = acpi_find_child_device(parent, adr, false);
> > +	if (adev) {
> > +		ACPI_COMPANION_SET(&auxdev->dev, adev);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * _ADR is a grey area in the ACPI specification, some
> > +	 * platforms use _HID to distinguish children devices.
> > +	 */
> > +	switch (adr) {
> > +	case LJCA_GPIO_ACPI_ADR:
> > +		wd.ids = ljca_gpio_hids;
> > +		break;
> > +	case LJCA_I2C1_ACPI_ADR:
> > +	case LJCA_I2C2_ACPI_ADR:
> > +		snprintf(uid, sizeof(uid), "%d", id);
> > +		wd.uid = uid;
> > +		wd.ids = ljca_i2c_hids;
> > +		break;
> > +	case LJCA_SPI1_ACPI_ADR:
> > +	case LJCA_SPI2_ACPI_ADR:
> > +		wd.ids = ljca_spi_hids;
> > +		break;
> > +	default:
> > +		dev_warn(dev, "unsupported _ADR\n");
> > +		return;
> > +	}
> > +
> > +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> > +	if (wd.adev) {
> > +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> > +		return;
> > +	}
> > +
> > +	parent = ACPI_COMPANION(dev->parent->parent);
> > +	if (!parent)
> > +		return;
> > +
> > +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> > +	if (wd.adev)
> > +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev); }
> > +
> > +static void ljca_auxdev_release(struct device *dev) {
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +
> > +	kfree(auxdev->dev.platform_data);
> > +}
> > +
> > +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> > +				  char *name, void *data, u64 adr) {
> > +	struct auxiliary_device *auxdev;
> > +	struct ljca_client *client;
> > +	int ret;
> > +
> > +	client = kzalloc(sizeof *client, GFP_KERNEL);
> > +	if (!client)
> > +		return -ENOMEM;
> > +
> > +	client->type = type;
> > +	client->id = id;
> > +	client->adapter = adap;
> > +	spin_lock_init(&client->event_cb_lock);
> > +
> > +	auxdev = &client->auxdev;
> > +	auxdev->name = name;
> > +	auxdev->id = id;
> > +
> > +	auxdev->dev.parent = adap->dev;
> > +	auxdev->dev.platform_data = data;
> > +	auxdev->dev.release = ljca_auxdev_release;
> > +
> > +	ret = auxiliary_device_init(auxdev);
> > +	if (ret)
> > +		goto err_free;
> > +
> > +	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
> > +
> > +	ret = auxiliary_device_add(auxdev);
> > +	if (ret)
> > +		goto err_uninit;
> > +
> > +	list_add_tail(&client->link, &adap->client_list);
> > +
> > +	return 0;
> > +
> > +err_uninit:
> > +	auxiliary_device_uninit(auxdev);
> > +
> > +err_free:
> > +	kfree(client);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ljca_enumerate_gpio(struct ljca_adapter *adap) {
> > +	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
> > +	struct ljca_gpio_descriptor *desc;
> > +	struct ljca_gpio_info *gpio_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	int ret, gpio_num;
> > +	unsigned int i;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO,
> NULL, 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* check firmware response */
> > +	desc = (struct ljca_gpio_descriptor *)buf;
> > +	if (ret != struct_size(desc, bank_desc, desc->bank_num))
> > +		return -EINVAL;
> > +
> > +	gpio_num = desc->pins_per_bank * desc->bank_num;
> > +	if (gpio_num > LJCA_MAX_GPIO_NUM)
> > +		return -EINVAL;
> > +
> > +	/* construct platform data */
> > +	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
> > +	if (!gpio_info)
> > +		return -ENOMEM;
> > +	gpio_info->num = gpio_num;
> > +
> > +	for (i = 0; i < desc->bank_num; i++)
> > +		valid_pin[i] = get_unaligned_le32(&desc-
> >bank_desc[i].valid_pins);
> > +	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
> > +
> > +	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> > +				     gpio_info, LJCA_GPIO_ACPI_ADR);
> > +	if (ret)
> > +		kfree(gpio_info);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ljca_enumerate_i2c(struct ljca_adapter *adap) {
> > +	struct ljca_i2c_descriptor *desc;
> > +	struct ljca_i2c_info *i2c_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL,
> 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* check firmware response */
> > +	desc = (struct ljca_i2c_descriptor *)buf;
> > +	if (ret != struct_size(desc, info, desc->num))
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < desc->num; i++) {
> > +		/* construct platform data */
> > +		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
> > +		if (!i2c_info)
> > +			return -ENOMEM;
> > +
> > +		i2c_info->id = desc->info[i].id;
> > +		i2c_info->capacity = desc->info[i].capacity;
> > +		i2c_info->intr_pin = desc->info[i].intr_pin;
> > +
> > +		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> > +					     "ljca-i2c", i2c_info,
> > +					     LJCA_I2C1_ACPI_ADR + i);
> > +		if (ret) {
> > +			kfree(i2c_info);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ljca_enumerate_spi(struct ljca_adapter *adap) {
> > +	struct ljca_spi_descriptor *desc;
> > +	struct ljca_spi_info *spi_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL,
> 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	desc = (struct ljca_spi_descriptor *)buf;
> > +	for (i = 0; i < desc->num; i++) {
> > +		/* construct platform data */
> > +		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
> > +		if (!spi_info)
> > +			return -ENOMEM;
> > +
> > +		spi_info->id = desc->info[i].id;
> > +		spi_info->capacity = desc->info[i].capacity;
> > +
> > +		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> > +					     "ljca-spi", spi_info,
> > +					     LJCA_SPI1_ACPI_ADR + i);
> > +		if (ret) {
> > +			kfree(spi_info);
> > +			return ret;
> 
> What about the other objects that you created here, don't you need to unwind
> the new ones if one in the chain fails?  And if not, why stop at the first failure?
> 
> 
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> > +	int ret;
> > +
> > +	ret = ljca_reset_handshake(adap);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ljca_enumerate_gpio(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate GPIO error\n");
> > +
> > +	ret = ljca_enumerate_i2c(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate I2C error\n");
> > +
> > +	ret = ljca_enumerate_spi(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate SPI error\n");
> 
> So none of these "errors" are actually errors:
> 
> > +	return 0;
> 
> You return success?  Why?  Are they not actually problems?

This is to be compatible with old version FW which does not support
full USB2xxx functions, so it just warn here as this is.
To make things more clear, I re-structure this as below, hope that
helps, thanks

static int ljca_enumerate_clients(struct ljca_adapter *adap)
{
	struct ljca_client *client, *next;
	int ret;

	ret = ljca_reset_handshake(adap);
	if (ret)
		return ret;

	ret = ljca_enumerate_gpio(adap);
	if (ret)
		goto err_free;

	ret = ljca_enumerate_i2c(adap);
	if (ret)
		goto err_free;

	ret = ljca_enumerate_spi(adap);
	if (ret)
		goto err_free;

	return 0;

err_free:
	adap->disconnect = true;

	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
		auxiliary_device_delete(&client->auxdev);
		auxiliary_device_uninit(&client->auxdev);

		list_del_init(&client->link);
		kfree(client);
	}

	return ret;
}

And accordingly, the ljca_enumerate_xxx() has slightly change to cover the
unsupported case as below( take spi as an example)

@@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
 
        ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
                        sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+       if (ret == -ETIMEDOUT) {
+               dev_warn(adap->dev, "doesn't support SPI function\n");
+               return 0;
+       }
+
        if (ret < 0)
                return ret;

Thanks
Wentong
> 
> thanks,
> 
> greg k-h

On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > +			      struct ljca_msg *header)
> > > +{
> > > +	struct ljca_client *client;
> > > +
> > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > +		/*
> > > +		 * FIXME: currently only GPIO register event callback.
> > > +		 * firmware message structure should include id when
> > > +		 * multiple same type clients register event callback.
> > > +		 */
> > 
> > When will this be fixed?
> > 
> > If not now, why not?
> 
> Actually this doesn't impact current functionality because only GPIO register
> event callback, but from coding perspective it should add the id in the message
> structure. This fix should be done by firmware first, but many products have
> already been running the firmware, it's not easy to update the firmware.
> 
> Can I just remove the 'FIXME' and leave the comment here?

If you are never going to fix it, it does not need a FIXME, right?  :)

> > > +		spin_unlock_irqrestore(&adap->lock, flags);
> > > +		return;
> > > +	}
> > > +
> > > +	ibuf_len = adap->ex_buf_len;
> > > +	ibuf = adap->ex_buf;
> > > +
> > > +	if (ibuf && ibuf_len) {
> > > +		actual_len = min_t(unsigned int, header->len, ibuf_len);
> > 
> > You control both of these types, why aren't they the same type to start with?
> > Why the force cast?
> 
> The len of header is defined by firmware, it should be u8 type. But the ex_buf_len
> is passed by API users, I don't want users to do the cast if their buffer is large than
> 256.

Then fix the api to use a u8 as obviously you can not handle more data
then that for now.

> > > +
> > > +		/* copy received data to external buffer */
> > > +		memcpy(ibuf, header->data, actual_len);
> > > +	}
> > > +	/* update copied data length */
> > > +	adap->actual_length = actual_len;
> > 
> > Wait, what happens if you don't actually copy the data?
> 
> This actual_length is the actual length of data copied to external buffer
> where is to save the command response. The API callers should check
> the response length according to the command you passed to this API.

But they aren't checking it as I pointed out elsewhere.

> > > +}
> > > +
> > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> > > +		     const u8 *obuf, unsigned int obuf_len, u8 *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 int actual;
> > > +	int ret = 0;
> > > +
> > > +	if (adap->disconnect)
> > > +		return -ENODEV;
> > > +
> > > +	if (msg_len > adap->tx_buf_len)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&adap->mutex);
> > > +
> > > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > > +		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;
> > > +	}
> > 
> > Do you really need a scoped guard when you can not fail out of the block?
> 
> The scoped_guard is required by you with "Why not use the functionality in
> cleanup.h for this lock? Makes this function much simpler." If I understand
> correctly, so I use the scoped_guard where I can to make things simpler.

But that's not making anything simpler here, cleanup.h works well when
you have error paths that would be more complex without it.  You do not
have that here at all now (maybe you did before?)

> > Why do you have both a mutex and spinlock grabed?
> 
> The mutex is to avoid command download concurrently
> 
> The spinlock is to protect tx_buf and ex_buf, which may be accessed by tx and rx
> at the same time.

Please document this somewhere.

> > > +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> > > +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> > > +
> > > +	usb_autopm_put_interface(adap->intf);
> > > +
> > > +	if (!ret && ack) {
> > > +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> > > +						  timeout);
> > > +		if (ret < 0) {
> > > +			goto out;
> > > +		} if (!ret) {
> > > +			ret = -ETIMEDOUT;
> > > +			goto out;
> > > +		}
> > > +	}
> > > +	ret = adap->actual_length;
> > 
> > Why are you not verifying that you sent what you wanted to send?
> 
> As I said, the actual_length is the actual length of data copied to user buffer instead
> of the length of what we want to send.
> 
> And even for verifying the length of what we want to send, the max length of the
> message is sizeof(struct ljca_msg) + LJCA_MAX_PACKET_SIZE which is less than
> the endpoint's max packet size, so I don't check the actual sent length in above
> usb_bulk_msg().

But you need to.

> > When you call this function, sometimes you check that the function sent the
> > proper amount of data, but in many places you do not, and you assume that the
> > full buffer was sent, which is not correct.  So please change _this_ function to
> > check that you sent the proper amount and then the caller logic will be much
> > simpler and actually work like you are using it in many places (some places you
> > got it right, some wrong, which is a HUGE indication that the API is wrong
> > because you wrote this code, and if you can't get it right...)
> 
> As I said, the return value of this function is the response data length instead of
> sent data length. And in this patch set, every caller has verified if the response
> length matched with the sent command. 

No, I found many users that did not do this.  Please make the api easy
to use, right now it's not.

> > > +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";
> > 
> > Are you sure this is a valid uid to use?  If so, why?  What happens if this gets set
> > to "0" for multiple ones?  Don't underestimate broken firmware tables, but also
> > don't paper over problems in ways that will be impossible to notice and can
> > cause problems.
> 
> This actually has been discussed in previous email as bellow: 
> 
> some DSDTs have only 1 ACPI companion for the 2 I2C controllers
> and these don't set a UID at all. On these models only the first I2C
> controller is used. So if a HID match has no UID use "0" for the HID.
> assigning the ACPI companion to the first I2C controller.
> An example device with this setup is the Dell Latitude 9420.

Then document this really really well in the code itself, otherwise it
looks broken.

> > > +static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> > > +	int ret;
> > > +
> > > +	ret = ljca_reset_handshake(adap);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ljca_enumerate_gpio(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate GPIO error\n");
> > > +
> > > +	ret = ljca_enumerate_i2c(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate I2C error\n");
> > > +
> > > +	ret = ljca_enumerate_spi(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate SPI error\n");
> > 
> > So none of these "errors" are actually errors:
> > 
> > > +	return 0;
> > 
> > You return success?  Why?  Are they not actually problems?
> 
> This is to be compatible with old version FW which does not support
> full USB2xxx functions, so it just warn here as this is.

Why do you have to support obsolete and broken firmware?  Can't it be
updated?

> To make things more clear, I re-structure this as below, hope that
> helps, thanks
> 
> static int ljca_enumerate_clients(struct ljca_adapter *adap)
> {
> 	struct ljca_client *client, *next;
> 	int ret;
> 
> 	ret = ljca_reset_handshake(adap);
> 	if (ret)
> 		return ret;
> 
> 	ret = ljca_enumerate_gpio(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	ret = ljca_enumerate_i2c(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	ret = ljca_enumerate_spi(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	return 0;
> 
> err_free:
> 	adap->disconnect = true;
> 
> 	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
> 		auxiliary_device_delete(&client->auxdev);
> 		auxiliary_device_uninit(&client->auxdev);
> 
> 		list_del_init(&client->link);
> 		kfree(client);
> 	}
> 
> 	return ret;
> }
> 
> And accordingly, the ljca_enumerate_xxx() has slightly change to cover the
> unsupported case as below( take spi as an example)
> 
> @@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
>  
>         ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
>                         sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +       if (ret == -ETIMEDOUT) {
> +               dev_warn(adap->dev, "doesn't support SPI function\n");
> +               return 0;

You warn, yet return success?  Again, that doesn't work well as you
never know if you need to unwind it or not.

Either report an error and handle it, or don't, but what you have here
looks broken as-is.

thanks,

greg k-h

> From: Greg KH <gregkh@linuxfoundation.org>
> On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote:
> > > From: Greg KH <gregkh@linuxfoundation.org> On Sun, Sep 17, 2023 at
> > > 02:53:33AM +0800, Wentong Wu wrote:
> > > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > > +			      struct ljca_msg *header) {
> > > > +	struct ljca_client *client;
> > > > +
> > > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > > +		/*
> > > > +		 * FIXME: currently only GPIO register event callback.
> > > > +		 * firmware message structure should include id when
> > > > +		 * multiple same type clients register event callback.
> > > > +		 */
> > >
> > > When will this be fixed?
> > >
> > > If not now, why not?
> >
> > Actually this doesn't impact current functionality because only GPIO
> > register event callback, but from coding perspective it should add the
> > id in the message structure. This fix should be done by firmware
> > first, but many products have already been running the firmware, it's not easy
> to update the firmware.
> >
> > Can I just remove the 'FIXME' and leave the comment here?
> 
> If you are never going to fix it, it does not need a FIXME, right?  :)

Thanks, I will remove 'FIXME' here.

> > > You control both of these types, why aren't they the same type to start with?
> > > Why the force cast?
> >
> > The len of header is defined by firmware, it should be u8 type. But
> > the ex_buf_len is passed by API users, I don't want users to do the
> > cast if their buffer is large than 256.
> 
> Then fix the api to use a u8 as obviously you can not handle more data then that
> for now.

Ack, thanks
 
> > > Do you really need a scoped guard when you can not fail out of the block?
> >
> > The scoped_guard is required by you with "Why not use the
> > functionality in cleanup.h for this lock? Makes this function much
> > simpler." If I understand correctly, so I use the scoped_guard where I can to
> make things simpler.
> 
> But that's not making anything simpler here, cleanup.h works well when you
> have error paths that would be more complex without it.  You do not have that
> here at all now (maybe you did before?)

I'll remove scoped guard

> 
> > > Why do you have both a mutex and spinlock grabed?
> >
> > The mutex is to avoid command download concurrently
> >
> > The spinlock is to protect tx_buf and ex_buf, which may be accessed by
> > tx and rx at the same time.
> 
> Please document this somewhere.

Ack, thanks. Below is the kernel doc for struct ljca_adapter where we have
spinlock and mutex document.

/**
 * struct ljca_adapter - represent a ljca adapter
 *
 * @intf: the usb interface for this ljca adapter
 * @usb_dev: the usb device for this ljca adapter
 * @dev: the specific device info of the usb interface
 * @rx_pipe: bulk in pipe for receive data from firmware
 * @tx_pipe: bulk out pipe for send data to firmware
 * @rx_urb: urb used for the bulk in pipe
 * @rx_buf: buffer used to receive command response and event
 * @rx_len: length of rx buffer
 * @ex_buf: external buffer to save command response
 * @ex_buf_len: length of external buffer
 * @actual_length: actual length of data copied to external buffer
 * @tx_buf: buffer used to download command to firmware
 * @tx_buf_len: length of tx buffer
 * @lock: spinlock to protect tx_buf and ex_buf
 * @cmd_completion: completion object as the command receives ack
 * @mutex: mutex to avoid command download concurrently
 * @client_list: client device list
 * @disconnect: usb disconnect ongoing or not
 * @reset_id: used to reset firmware
 */
struct ljca_adapter {
	struct usb_interface *intf;
	struct usb_device *usb_dev;
	struct device *dev;

	unsigned int rx_pipe;
	unsigned int tx_pipe;

	struct urb *rx_urb;
	void *rx_buf;
	unsigned int rx_len;

	u8 *ex_buf;
	u8 ex_buf_len;
	u8 actual_length;

	void *tx_buf;
	u8 tx_buf_len;

	spinlock_t lock;

	struct completion cmd_completion;
	struct mutex mutex;

	struct list_head client_list;

	bool disconnect;

	u32 reset_id;
};
 
> > > Why are you not verifying that you sent what you wanted to send?
> >
> > As I said, the actual_length is the actual length of data copied to
> > user buffer instead of the length of what we want to send.
> >
> > And even for verifying the length of what we want to send, the max
> > length of the message is sizeof(struct ljca_msg) +
> > LJCA_MAX_PACKET_SIZE which is less than the endpoint's max packet
> > size, so I don't check the actual sent length in above usb_bulk_msg().
> 
> But you need to.

Ack, thanks.

> 
> > > When you call this function, sometimes you check that the function
> > > sent the proper amount of data, but in many places you do not, and
> > > you assume that the full buffer was sent, which is not correct.  So
> > > please change _this_ function to check that you sent the proper
> > > amount and then the caller logic will be much simpler and actually
> > > work like you are using it in many places (some places you got it
> > > right, some wrong, which is a HUGE indication that the API is wrong
> > > because you wrote this code, and if you can't get it right...)
> >
> > As I said, the return value of this function is the response data
> > length instead of sent data length. And in this patch set, every
> > caller has verified if the response length matched with the sent command.
> 
> No, I found many users that did not do this.  

Ack, I will check more, thanks

>Please make the api easy to use, right now it's not.

I post the new ljca_send() here to try to address several comments.

First it changes the type of buffer length to u8, second it checks the actual sent
of length usb_bulk_msg(). It removes the scoped guard as well.

And below gives an explanation of the parameters and return value.

The parameters(type, cmd, obuf_len) are used to construct message header,
obuf is used for message data. And ibuf and ibuf_len are for response buffer
and buffer length passed by API users. ack indicates if the command
need an ack from firmware, timeout is timeout value to wait command ack.

And the return value is the response copied to response buffer passed by API
users, normally the users know how large buffer they have to pass to this API,
but from coding perspective we should check the passed response buffer
length(ibuf_len) and return the actual copied data length to the buffer.

Hope that helps, thanks

static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
		      const u8 *obuf, u8 obuf_len, u8 *ibuf, u8 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 int transferred;
	unsigned long flags;
	int ret;

	if (adap->disconnect)
		return -ENODEV;

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

	ret = usb_autopm_get_interface(adap->intf);
	if (ret < 0)
		goto out;

	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
			    msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);

	usb_autopm_put_interface(adap->intf);

	if (ret < 0)
		goto out;
	if (transferred != msg_len) {
		ret = -EIO;
		goto out;
	}

	if (ack) {
		ret = wait_for_completion_timeout(&adap->cmd_completion,
						       timeout);
		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);

	mutex_unlock(&adap->mutex);

	return ret;
}

> > > Are you sure this is a valid uid to use?  If so, why?  What happens
> > > if this gets set to "0" for multiple ones?  Don't underestimate
> > > broken firmware tables, but also don't paper over problems in ways
> > > that will be impossible to notice and can cause problems.
> >
> > This actually has been discussed in previous email as bellow:
> >
> > some DSDTs have only 1 ACPI companion for the 2 I2C controllers and
> > these don't set a UID at all. On these models only the first I2C
> > controller is used. So if a HID match has no UID use "0" for the HID.
> > assigning the ACPI companion to the first I2C controller.
> > An example device with this setup is the Dell Latitude 9420.
> 
> Then document this really really well in the code itself, otherwise it looks broken.

Ack, thanks, I post the new ljca_match_device_ids() here, hope that helps, thanks.
 
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)
		/*
		 * Some DSDTs have only one ACPI companion for the two I2C
		 * controllers and they don't set a UID at all (e.g. Dell
 		 * Latitude 9420). On these platforms only the first I2C
		 * controller is used, so if a HID match has no UID we use
		 * "0" as the UID and assign ACPI companion to the first
		 * I2C controller.
		 */ 
		uid = "0";
	else
		uid = strchr(uid, wd->uid[0]);

	if (!uid || strcmp(uid, wd->uid))
		return 0;

match:
	wd->adev = adev;

	return 1;
}

> > This is to be compatible with old version FW which does not support
> > full USB2xxx functions, so it just warn here as this is.
> 
> Why do you have to support obsolete and broken firmware?  Can't it be updated?

make sense, and probably they have to update, thanks

> You warn, yet return success?  Again, that doesn't work well as you never know
> if you need to unwind it or not.
> 
> Either report an error and handle it, or don't, but what you have here looks
> broken as-is.

Ack, it should report error and handle it. Thanks, the function will be like below:

static int ljca_enumerate_clients(struct ljca_adapter *adap)
{
	struct ljca_client *client, *next;
	int ret;

	ret = ljca_reset_handshake(adap);
	if (ret)
		goto err_kill;

	ret = ljca_enumerate_gpio(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate GPIO error\n");
		goto err_kill;
        	}

	ret = ljca_enumerate_i2c(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate I2C error\n");
		goto err_kill;
	}

	ret = ljca_enumerate_spi(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate SPI error\n");
		goto err_kill;
	}

	return 0;

err_kill:
	adap->disconnect = true;

	usb_kill_urb(adap->rx_urb);

	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
		auxiliary_device_delete(&client->auxdev);
		auxiliary_device_uninit(&client->auxdev);

		list_del_init(&client->link);
		kfree(client);
	}

	return ret;
}

Thanks,

Wentong

> 
> thanks,
> 
> greg k-h

> From: Wu, Wentong
> > From: Greg KH <gregkh@linuxfoundation.org> On Fri, Sep 29, 2023 at
> > 11:31:21AM +0000, Wu, Wentong wrote:
> > > > From: Greg KH <gregkh@linuxfoundation.org> On Sun, Sep 17, 2023 at
> > > > 02:53:33AM +0800, Wentong Wu wrote:
> > > > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > > > +			      struct ljca_msg *header) {
> > > > > +	struct ljca_client *client;
> > > > > +
> > > > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > > > +		/*
> > > > > +		 * FIXME: currently only GPIO register event callback.
> > > > > +		 * firmware message structure should include id when
> > > > > +		 * multiple same type clients register event callback.
> > > > > +		 */
> > > >
> > > > When will this be fixed?
> > > >
> > > > If not now, why not?
> > >
> > > Actually this doesn't impact current functionality because only GPIO
> > > register event callback, but from coding perspective it should add
> > > the id in the message structure. This fix should be done by firmware
> > > first, but many products have already been running the firmware,
> > > it's not easy
> > to update the firmware.
> > >
> > > Can I just remove the 'FIXME' and leave the comment here?
> >
> > If you are never going to fix it, it does not need a FIXME, right?  :)
> 
> Thanks, I will remove 'FIXME' here.
> 
> > > > You control both of these types, why aren't they the same type to start
> with?
> > > > Why the force cast?
> > >
> > > The len of header is defined by firmware, it should be u8 type. But
> > > the ex_buf_len is passed by API users, I don't want users to do the
> > > cast if their buffer is large than 256.
> >
> > Then fix the api to use a u8 as obviously you can not handle more data
> > then that for now.
> 
> Ack, thanks
> 
> > > > Do you really need a scoped guard when you can not fail out of the block?
> > >
> > > The scoped_guard is required by you with "Why not use the
> > > functionality in cleanup.h for this lock? Makes this function much
> > > simpler." If I understand correctly, so I use the scoped_guard where
> > > I can to
> > make things simpler.
> >
> > But that's not making anything simpler here, cleanup.h works well when
> > you have error paths that would be more complex without it.  You do
> > not have that here at all now (maybe you did before?)
> 
> I'll remove scoped guard
> 
> >
> > > > Why do you have both a mutex and spinlock grabed?
> > >
> > > The mutex is to avoid command download concurrently
> > >
> > > The spinlock is to protect tx_buf and ex_buf, which may be accessed
> > > by tx and rx at the same time.
> >
> > Please document this somewhere.
> 
> Ack, thanks. Below is the kernel doc for struct ljca_adapter where we have
> spinlock and mutex document.
> 
> /**
>  * struct ljca_adapter - represent a ljca adapter
>  *
>  * @intf: the usb interface for this ljca adapter
>  * @usb_dev: the usb device for this ljca adapter
>  * @dev: the specific device info of the usb interface
>  * @rx_pipe: bulk in pipe for receive data from firmware
>  * @tx_pipe: bulk out pipe for send data to firmware
>  * @rx_urb: urb used for the bulk in pipe
>  * @rx_buf: buffer used to receive command response and event
>  * @rx_len: length of rx buffer
>  * @ex_buf: external buffer to save command response
>  * @ex_buf_len: length of external buffer
>  * @actual_length: actual length of data copied to external buffer
>  * @tx_buf: buffer used to download command to firmware
>  * @tx_buf_len: length of tx buffer
>  * @lock: spinlock to protect tx_buf and ex_buf
>  * @cmd_completion: completion object as the command receives ack
>  * @mutex: mutex to avoid command download concurrently
>  * @client_list: client device list
>  * @disconnect: usb disconnect ongoing or not
>  * @reset_id: used to reset firmware
>  */
> struct ljca_adapter {
> 	struct usb_interface *intf;
> 	struct usb_device *usb_dev;
> 	struct device *dev;
> 
> 	unsigned int rx_pipe;
> 	unsigned int tx_pipe;
> 
> 	struct urb *rx_urb;
> 	void *rx_buf;
> 	unsigned int rx_len;
> 
> 	u8 *ex_buf;
> 	u8 ex_buf_len;
> 	u8 actual_length;
> 
> 	void *tx_buf;
> 	u8 tx_buf_len;
> 
> 	spinlock_t lock;
> 
> 	struct completion cmd_completion;
> 	struct mutex mutex;
> 
> 	struct list_head client_list;
> 
> 	bool disconnect;
> 
> 	u32 reset_id;
> };
> 
> > > > Why are you not verifying that you sent what you wanted to send?
> > >
> > > As I said, the actual_length is the actual length of data copied to
> > > user buffer instead of the length of what we want to send.
> > >
> > > And even for verifying the length of what we want to send, the max
> > > length of the message is sizeof(struct ljca_msg) +
> > > LJCA_MAX_PACKET_SIZE which is less than the endpoint's max packet
> > > size, so I don't check the actual sent length in above usb_bulk_msg().
> >
> > But you need to.
> 
> Ack, thanks.
> 
> >
> > > > When you call this function, sometimes you check that the function
> > > > sent the proper amount of data, but in many places you do not, and
> > > > you assume that the full buffer was sent, which is not correct.
> > > > So please change _this_ function to check that you sent the proper
> > > > amount and then the caller logic will be much simpler and actually
> > > > work like you are using it in many places (some places you got it
> > > > right, some wrong, which is a HUGE indication that the API is
> > > > wrong because you wrote this code, and if you can't get it
> > > > right...)
> > >
> > > As I said, the return value of this function is the response data
> > > length instead of sent data length. And in this patch set, every
> > > caller has verified if the response length matched with the sent command.
> >
> > No, I found many users that did not do this.
> 
> Ack, I will check more, thanks
> 
> >Please make the api easy to use, right now it's not.
> 
> I post the new ljca_send() here to try to address several comments.
> 
> First it changes the type of buffer length to u8, second it checks the actual sent of
> length usb_bulk_msg(). It removes the scoped guard as well.
> 
> And below gives an explanation of the parameters and return value.
> 
> The parameters(type, cmd, obuf_len) are used to construct message header, obuf
> is used for message data. And ibuf and ibuf_len are for response buffer and buffer
> length passed by API users. ack indicates if the command need an ack from
> firmware, timeout is timeout value to wait command ack.
> 
> And the return value is the response copied to response buffer passed by API
> users, normally the users know how large buffer they have to pass to this API, but
> from coding perspective we should check the passed response buffer
> length(ibuf_len) and return the actual copied data length to the buffer.
> 
> Hope that helps, thanks
> 
> static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> 		      const u8 *obuf, u8 obuf_len, u8 *ibuf, u8 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 int transferred;
> 	unsigned long flags;
> 	int ret;
> 
> 	if (adap->disconnect)
> 		return -ENODEV;
> 
> 	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);
> 
> 	ret = usb_autopm_get_interface(adap->intf);
> 	if (ret < 0)
> 		goto out;
> 
> 	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> 			    msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
> 
> 	usb_autopm_put_interface(adap->intf);
> 
> 	if (ret < 0)
> 		goto out;
> 	if (transferred != msg_len) {
> 		ret = -EIO;
> 		goto out;
> 	}
> 
> 	if (ack) {
> 		ret = wait_for_completion_timeout(&adap->cmd_completion,
> 						       timeout);
> 		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);
> 
> 	mutex_unlock(&adap->mutex);
> 
> 	return ret;
> }
> 
> > > > Are you sure this is a valid uid to use?  If so, why?  What
> > > > happens if this gets set to "0" for multiple ones?  Don't
> > > > underestimate broken firmware tables, but also don't paper over
> > > > problems in ways that will be impossible to notice and can cause problems.
> > >
> > > This actually has been discussed in previous email as bellow:
> > >
> > > some DSDTs have only 1 ACPI companion for the 2 I2C controllers and
> > > these don't set a UID at all. On these models only the first I2C
> > > controller is used. So if a HID match has no UID use "0" for the HID.
> > > assigning the ACPI companion to the first I2C controller.
> > > An example device with this setup is the Dell Latitude 9420.
> >
> > Then document this really really well in the code itself, otherwise it looks
> broken.
> 
> Ack, thanks, I post the new ljca_match_device_ids() here, hope that helps, thanks.
> 
> 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)
> 		/*
> 		 * Some DSDTs have only one ACPI companion for the two I2C
> 		 * controllers and they don't set a UID at all (e.g. Dell
>  		 * Latitude 9420). On these platforms only the first I2C
> 		 * controller is used, so if a HID match has no UID we use
> 		 * "0" as the UID and assign ACPI companion to the first
> 		 * I2C controller.
> 		 */
> 		uid = "0";
> 	else
> 		uid = strchr(uid, wd->uid[0]);
> 
> 	if (!uid || strcmp(uid, wd->uid))
> 		return 0;
> 
> match:
> 	wd->adev = adev;
> 
> 	return 1;
> }
> 
> > > This is to be compatible with old version FW which does not support
> > > full USB2xxx functions, so it just warn here as this is.
> >
> > Why do you have to support obsolete and broken firmware?  Can't it be
> updated?
> 
> make sense, and probably they have to update, thanks
> 
> > You warn, yet return success?  Again, that doesn't work well as you
> > never know if you need to unwind it or not.
> >
> > Either report an error and handle it, or don't, but what you have here
> > looks broken as-is.
> 
> Ack, it should report error and handle it. Thanks, the function will be like below:
> 
> static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> 	struct ljca_client *client, *next;
> 	int ret;
> 
> 	ret = ljca_reset_handshake(adap);
> 	if (ret)
> 		goto err_kill;
> 
> 	ret = ljca_enumerate_gpio(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate GPIO error\n");
> 		goto err_kill;
>         	}
> 
> 	ret = ljca_enumerate_i2c(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate I2C error\n");
> 		goto err_kill;
> 	}
> 
> 	ret = ljca_enumerate_spi(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate SPI error\n");
> 		goto err_kill;
> 	}
> 
> 	return 0;
> 
> err_kill:
> 	adap->disconnect = true;
> 
> 	usb_kill_urb(adap->rx_urb);
> 
> 	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
> 		auxiliary_device_delete(&client->auxdev);
> 		auxiliary_device_uninit(&client->auxdev);
> 
> 		list_del_init(&client->link);
> 		kfree(client);
> 	}
> 
> 	return ret;
> }
> 

Hi Greg,

Could you please share your comment about this? If no more comment, I will
sent out next version patch set. Thanks

BR,
Wentong

> 
> >
> > thanks,
> >
> > greg k-h