| Message ID | 1693970580-18967-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
On Wed, Sep 06, 2023 at 11:22:57AM +0800, Wentong Wu wrote: > +struct ljca_bank_descriptor { > + u8 bank_id; > + u8 pin_num; > + > + /* 1 bit for each gpio, 1 means valid */ > + u32 valid_pins; > +} __packed; This is an unaligned access, do you mean to have that? > +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 */ > + void *ex_buf; Shouldn't buffers be u8*? > +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. When is this fixme going to be addressed? > + * firmware message structure should include id when > + * multiple same type clients register event callback. > + */ > + if (client->type == header->type) { > + unsigned long flags; > + > + spin_lock_irqsave(&client->event_cb_lock, flags); > + client->event_cb(client->context, header->cmd, > + header->data, header->len); > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > + > + break; > + } > + } > +} > + > +/* process command ack */ > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > + struct ljca_msg *header) > +{ > + struct ljca_msg *tx_header = adap->tx_buf; > + unsigned int actual_len = 0; > + unsigned int ibuf_len; > + unsigned long flags; > + void *ibuf; > + > + spin_lock_irqsave(&adap->lock, flags); Why not use the functionality in cleanup.h for this lock? Makes this function much simpler. > + > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) { > + spin_unlock_irqrestore(&adap->lock, flags); > + No need for a blank line. And how can these things happen? No need to return an error if this is the case? > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > + const void *obuf, unsigned int obuf_len, void *ibuf, > + unsigned int ibuf_len, bool ack, unsigned long timeout) That's a lot of function parameters, whyh so many? And why void *? That should never be used in an internal function where you know the real type. > +{ > + unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len; > + struct ljca_msg *header = adap->tx_buf; > + unsigned long flags; > + unsigned int actual; > + int ret = 0; > + > + if (adap->disconnect) > + return -ENODEV; > + > + if (msg_len > adap->tx_buf_len) > + return -EINVAL; > + > + mutex_lock(&adap->mutex); > + > + spin_lock_irqsave(&adap->lock, flags); 2 locks? Why 2 locks for the same structure? > + > + 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); > + > + usb_autopm_get_interface(adap->intf); > + > + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header, > + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS); This function is slow. Really slow. You drop the spinlock which is good, but the mutex is still held. Does this call have to be synchronous? > + > + 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; > + > +out: > + spin_lock_irqsave(&adap->lock, flags); > + adap->ex_buf = NULL; > + adap->ex_buf_len = 0; > + > + memset(header, 0, sizeof(*header)); Why? > + spin_unlock_irqrestore(&adap->lock, flags); > + > + mutex_unlock(&adap->mutex); > + > + return ret; > +} > + > +int ljca_transfer(struct ljca_client *client, u8 cmd, const void *obuf, Again, drop all void * please, use real types in your apis. > +#else > +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > + struct auxiliary_device *auxdev, > + u64 adr, u8 id) > +{ > +} > +#endif Can't this go in a .h file? #ifdef in .c files are frowned apon. > +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"); You warn about these things, but keep on saying the code is working properly with a return of: > + return 0; That's not good. Why not unwind properly and handle the error? > --- /dev/null > +++ b/include/linux/usb/ljca.h > @@ -0,0 +1,113 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2023, Intel Corporation. All rights reserved. > + */ > +#ifndef _LINUX_USB_LJCA_H_ > +#define _LINUX_USB_LJCA_H_ > + > +#include <linux/auxiliary_bus.h> > +#include <linux/list.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +#define LJCA_MAX_GPIO_NUM 64 > + > +#define auxiliary_dev_to_ljca_client(auxiliary_dev) \ > + container_of(auxiliary_dev, struct ljca_client, auxdev) > + > +struct ljca_adapter; > + > +/** > + * typedef ljca_event_cb_t - event callback function signature > + * > + * @context: the execution context of who registered this callback > + * @cmd: the command from device for this event > + * @evt_data: the event data payload > + * @len: the event data payload length > + * > + * The callback function is called in interrupt context and the data payload is > + * only valid during the call. If the user needs later access of the data, it > + * must copy it. > + */ > +typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void *evt_data, int len); > + > +struct ljca_client { > + u8 type; > + u8 id; > + struct list_head link; > + struct auxiliary_device auxdev; > + struct ljca_adapter *adapter; > + > + void *context; > + ljca_event_cb_t event_cb; > + /* lock to protect event_cb */ > + spinlock_t event_cb_lock; > +}; > + > +struct ljca_gpio_info { > + unsigned int num; > + DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM); > +}; > + > +struct ljca_i2c_info { > + u8 id; > + u8 capacity; > + u8 intr_pin; > +}; > + > +struct ljca_spi_info { > + u8 id; > + u8 capacity; > +}; No documentation for these other public structures? thanks, greg k-h
Hi Greg, Thanks for your review > From: Greg KH Friday, September 8, 2023 9:39 PM > > +struct ljca_bank_descriptor { > > + u8 bank_id; > > + u8 pin_num; > > + > > + /* 1 bit for each gpio, 1 means valid */ > > + u32 valid_pins; > > +} __packed; > > This is an unaligned access, do you mean to have that? Yes, this is defined by the firmware, we have to keep the same structure with firmware. > > > +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 */ > > + void *ex_buf; > > Shouldn't buffers be u8*? It can be u8 *, but it can avoid many cast if with void *. > > > +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. > > When is this fixme going to be addressed? Actually this is following firmware, and it takes some time to fix firmware issue, but currently it doesn't impact the functionality. > > > + * firmware message structure should include id when > > + * multiple same type clients register event callback. > > + */ > > + if (client->type == header->type) { > > + unsigned long flags; > > + > > + spin_lock_irqsave(&client->event_cb_lock, flags); > > + client->event_cb(client->context, header->cmd, > > + header->data, header->len); > > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > > + > > + break; > > + } > > + } > > +} > > + > > +/* process command ack */ > > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > > + struct ljca_msg *header) > > +{ > > + struct ljca_msg *tx_header = adap->tx_buf; > > + unsigned int actual_len = 0; > > + unsigned int ibuf_len; > > + unsigned long flags; > > + void *ibuf; > > + > > + spin_lock_irqsave(&adap->lock, flags); > > Why not use the functionality in cleanup.h for this lock? Makes this function > much simpler. Sorry, I'm not familiar with this cleanup.h, and also I don't get how to simplify this function after review the cleanup.h, could you please help? Thanks > > > + > > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) > { > > + spin_unlock_irqrestore(&adap->lock, flags); > > + > > No need for a blank line. Ack, thanks > > And how can these things happen? No need to return an error if this is the case? In case something wrong happens in firmware, and yes, I will add an error message here, thanks. > > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > > + const void *obuf, unsigned int obuf_len, void *ibuf, > > + unsigned int ibuf_len, bool ack, unsigned long timeout) > > That's a lot of function parameters, whyh so many? This function sends command to firmware. The command is composed of the command header And data buffer which comes from the function parameters. Also some commands have response, and we need external buffer and length to copy to > > And why void *? That should never be used in an internal function where you > know the real type. Ack, I will change them to real type for the internal function, thanks > > > +{ > > + unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len; > > + struct ljca_msg *header = adap->tx_buf; > > + unsigned long flags; > > + unsigned int actual; > > + int ret = 0; > > + > > + if (adap->disconnect) > > + return -ENODEV; > > + > > + if (msg_len > adap->tx_buf_len) > > + return -EINVAL; > > + > > + mutex_lock(&adap->mutex); > > + > > + spin_lock_irqsave(&adap->lock, flags); > > 2 locks? Why 2 locks for the same structure? The mutex is to avoid command download concurrently. The spinlock is to protect tx_buf and ex_buf, which may be accessed by tx thread and rx thread at the same time. > > > + > > + 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); > > + > > + usb_autopm_get_interface(adap->intf); > > + > > + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header, > > + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS); > > This function is slow. Really slow. You drop the spinlock which is good, but the > mutex is still held. Does this call have to be synchronous? As I said above, the mutex is to avoid more commands downloading to firmware before the current one is completed. > > > + > > + 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; > > + > > +out: > > + spin_lock_irqsave(&adap->lock, flags); > > + adap->ex_buf = NULL; > > + adap->ex_buf_len = 0; > > + > > + memset(header, 0, sizeof(*header)); > > Why? In ljca_handle_cmd_ack(), we use the tx header to compare the received header. If we receive command ack after timeout(though most likely it won't happen), I want it just return, so I clear the tx header here. > > > + spin_unlock_irqrestore(&adap->lock, flags); > > + > > + mutex_unlock(&adap->mutex); > > + > > + return ret; > > +} > > + > > +int ljca_transfer(struct ljca_client *client, u8 cmd, const void > > +*obuf, > > Again, drop all void * please, use real types in your apis. This is the exposed API, usually it's void *. I can change it to real type, but is there any rule when to use void * and when to use real type? > > > +#else > > +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > > + struct auxiliary_device *auxdev, > > + u64 adr, u8 id) > > +{ > > +} > > +#endif > > Can't this go in a .h file? #ifdef in .c files are frowned apon. Ack, but I will remove the #ifdef because it ready has 'depends on ACPI' in Kconfig, Thanks. > > > +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"); > > You warn about these things, but keep on saying the code is working properly > with a return of: > > > + return 0; > > That's not good. Why not unwind properly and handle the error? This is to be compatible with old version FW which does not support full USB2xxx functions, so just warn here and keep the code going. Thanks > > > --- /dev/null > > +++ b/include/linux/usb/ljca.h > > @@ -0,0 +1,113 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (c) 2023, Intel Corporation. All rights reserved. > > + */ > > +#ifndef _LINUX_USB_LJCA_H_ > > +#define _LINUX_USB_LJCA_H_ > > + > > +#include <linux/auxiliary_bus.h> > > +#include <linux/list.h> > > +#include <linux/spinlock.h> > > +#include <linux/types.h> > > + > > +#define LJCA_MAX_GPIO_NUM 64 > > + > > +#define auxiliary_dev_to_ljca_client(auxiliary_dev) \ > > + container_of(auxiliary_dev, struct ljca_client, auxdev) > > + > > +struct ljca_adapter; > > + > > +/** > > + * typedef ljca_event_cb_t - event callback function signature > > + * > > + * @context: the execution context of who registered this callback > > + * @cmd: the command from device for this event > > + * @evt_data: the event data payload > > + * @len: the event data payload length > > + * > > + * The callback function is called in interrupt context and the data > > +payload is > > + * only valid during the call. If the user needs later access of the > > +data, it > > + * must copy it. > > + */ > > +typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void > > +*evt_data, int len); > > + > > +struct ljca_client { > > + u8 type; > > + u8 id; > > + struct list_head link; > > + struct auxiliary_device auxdev; > > + struct ljca_adapter *adapter; > > + > > + void *context; > > + ljca_event_cb_t event_cb; > > + /* lock to protect event_cb */ > > + spinlock_t event_cb_lock; > > +}; > > + > > +struct ljca_gpio_info { > > + unsigned int num; > > + DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM); }; > > + > > +struct ljca_i2c_info { > > + u8 id; > > + u8 capacity; > > + u8 intr_pin; > > +}; > > + > > +struct ljca_spi_info { > > + u8 id; > > + u8 capacity; > > +}; > > No documentation for these other public structures? Ack, I will add doc for these public structures in next version. Thanks Thanks Wentong > > thanks, > > greg k-h
Hi Greg, Thanks for your review > From: Greg KH <gregkh@linuxfoundation.org> > > > +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 */ > > + void *ex_buf; > > Shouldn't buffers be u8*? Ack, this has been fixed in my local version. > > > + > > +/* process command ack */ > > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > > + struct ljca_msg *header) > > +{ > > + struct ljca_msg *tx_header = adap->tx_buf; > > + unsigned int actual_len = 0; > > + unsigned int ibuf_len; > > + unsigned long flags; > > + void *ibuf; > > + > > + spin_lock_irqsave(&adap->lock, flags); > > Why not use the functionality in cleanup.h for this lock? Makes this function > much simpler. Ack, I understand the cleanup.h now, and use the functionality as much as I can. > > > + > > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) > { > > + spin_unlock_irqrestore(&adap->lock, flags); > > + > > No need for a blank line. > > And how can these things happen? No need to return an error if this is the case? Ack, removed the blank line and added error print here. > > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > > + const void *obuf, unsigned int obuf_len, void *ibuf, > > + unsigned int ibuf_len, bool ack, unsigned long timeout) > > That's a lot of function parameters, whyh so many? > > And why void *? That should never be used in an internal function where you > know the real type. Ack, have switched void * to real type > > > +{ > > > +#else > > +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > > + struct auxiliary_device *auxdev, > > + u64 adr, u8 id) > > +{ > > +} > > +#endif > > Can't this go in a .h file? #ifdef in .c files are frowned apon. Ack, this has been removed > > > --- /dev/null > > +++ b/include/linux/usb/ljca.h > > + > > +struct ljca_client { > > + u8 type; > > + u8 id; > > + struct list_head link; > > + struct auxiliary_device auxdev; > > + struct ljca_adapter *adapter; > > + > > + void *context; > > + ljca_event_cb_t event_cb; > > + /* lock to protect event_cb */ > > + spinlock_t event_cb_lock; > > +}; > > + > > +struct ljca_gpio_info { > > + unsigned int num; > > + DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM); }; > > + > > +struct ljca_i2c_info { > > + u8 id; > > + u8 capacity; > > + u8 intr_pin; > > +}; > > + > > +struct ljca_spi_info { > > + u8 id; > > + u8 capacity; > > +}; > > No documentation for these other public structures? Ack, This has been addressed. If no more comments, I will send out v16 for review. Thanks Wentong > > thanks, > > greg k-h