> 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