Re: [PATCH v15 1/4] usb: Add support for Intel LJCA device

[Greg KH <gregkh@linuxfoundation.org>]

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