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

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

Hi Greg,

Thanks for your review

> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> 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