> 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
