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

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

> From: Wu, Wentong
> > 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;
> }
> 

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