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

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

> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote:
> > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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