| Message ID | 1694890416-14409-2-git-send-email-wentong.wu@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote: > +/* ljca cmd message structure */ > +struct ljca_msg { > + u8 type; > + u8 cmd; > + u8 flags; > + u8 len; > + u8 data[]; Shouldn't you be using the __counted_by attributes for all of these [] arrays so that the compiler knows what to do and how to check that you don't overrun the buffer? Adding that now will save you having to add it later, AND it might catch bugs you already have so please add 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 */ > + u8 *ex_buf; > + unsigned int ex_buf_len; > + /* actual data length copied to external buffer */ > + unsigned int actual_length; > + > + /* buffer for send */ > + void *tx_buf; > + unsigned int tx_buf_len; > + > + /* lock to protect tx_buf and ex_buf */ > + spinlock_t lock; > + > + struct completion cmd_completion; > + > + /* mutex to protect command download */ > + struct mutex mutex; > + > + /* client device list */ > + struct list_head client_list; > + > + /* disconnect ongoing or not */ > + bool disconnect; > + > + /* used to reset firmware */ Can you use proper kernel doc for this structure instead of inline comments? > + u32 reset_id; > +}; > + > +struct ljca_match_ids_walk_data { > + const struct acpi_device_id *ids; > + const char *uid; > + struct acpi_device *adev; > +}; > + > +static const struct acpi_device_id ljca_gpio_hids[] = { > + { "INTC1074" }, > + { "INTC1096" }, > + { "INTC100B" }, > + { "INTC10D1" }, > + {}, > +}; > + > +static const struct acpi_device_id ljca_i2c_hids[] = { > + { "INTC1075" }, > + { "INTC1097" }, > + { "INTC100C" }, > + { "INTC10D2" }, > + {}, > +}; > + > +static const struct acpi_device_id ljca_spi_hids[] = { > + { "INTC1091" }, > + { "INTC1098" }, > + { "INTC100D" }, > + { "INTC10D3" }, > + {}, > +}; > + > +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? > + if (client->type == header->type) { > + scoped_guard(spinlock_irqsave, &client->event_cb_lock) { > + client->event_cb(client->context, header->cmd, > + header->data, header->len); > + } > + > + break; > + } > + } > +} > + > +/* process command ack */ > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > + struct ljca_msg *header) We can use 100 columns... > +{ > + struct ljca_msg *tx_header = adap->tx_buf; > + unsigned int actual_len = 0; > + unsigned int ibuf_len; > + unsigned long flags; > + u8 *ibuf; > + > + spin_lock_irqsave(&adap->lock, flags); > + > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) { > + dev_err(adap->dev, "cmd ack mismatch error\n"); When you print error messages like this, who can do something about it? And why print with interrupts disabled? > + spin_unlock_irqrestore(&adap->lock, flags); > + return; > + } > + > + ibuf_len = adap->ex_buf_len; > + ibuf = adap->ex_buf; > + > + if (ibuf && ibuf_len) { > + actual_len = min_t(unsigned int, header->len, ibuf_len); You control both of these types, why aren't they the same type to start with? Why the force cast? > + > + /* copy received data to external buffer */ > + memcpy(ibuf, header->data, actual_len); > + } > + /* update copied data length */ > + adap->actual_length = actual_len; Wait, what happens if you don't actually copy the data? Why lie and say you did? Where is the error handling? > + > + spin_unlock_irqrestore(&adap->lock, flags); > + > + complete(&adap->cmd_completion); > +} > + > +static void ljca_recv(struct urb *urb) No error handling? > +{ > + struct ljca_msg *header = urb->transfer_buffer; > + struct ljca_adapter *adap = urb->context; > + int ret; > + > + if (urb->status) { > + /* sync/async unlink faults aren't errors */ > + if (urb->status == -ENOENT) { > + /* > + * directly complete the possible ongoing transfer > + * during disconnect > + */ > + if (adap->disconnect) > + complete(&adap->cmd_completion); > + return; > + } > + > + if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) > + return; > + > + dev_err(adap->dev, "recv urb error: %d\n", urb->status); > + goto resubmit; You have an error, yet you don't report it you just spam the kernel log? Why? Doesn't this happen when a device is removed? > + } > + > + if (header->len + sizeof(*header) != urb->actual_length) > + goto resubmit; > + > + if (header->flags & LJCA_ACK_FLAG) > + ljca_handle_cmd_ack(adap, header); > + else > + ljca_handle_event(adap, header); > + > +resubmit: > + ret = usb_submit_urb(urb, GFP_ATOMIC); Why atomic, is this in a urb callback? > + if (ret && ret != -EPERM) > + dev_err(adap->dev, "resubmit recv urb error %d\n", ret); What happens if ret is an error here? > +} > + > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > + const u8 *obuf, unsigned int obuf_len, u8 *ibuf, > + unsigned int 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 actual; > + int ret = 0; > + > + if (adap->disconnect) > + return -ENODEV; > + > + if (msg_len > adap->tx_buf_len) > + return -EINVAL; > + > + mutex_lock(&adap->mutex); > + > + scoped_guard(spinlock_irqsave, &adap->lock) { > + 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; > + } Do you really need a scoped guard when you can not fail out of the block? Why do you have both a mutex and spinlock grabed? > + > + reinit_completion(&adap->cmd_completion); > + > + usb_autopm_get_interface(adap->intf); This can fail. Why aren't you checking that? > + > + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header, > + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS); > + > + 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; Why are you not verifying that you sent what you wanted to send? 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...) > +out: > + scoped_guard(spinlock_irqsave, &adap->lock) { > + adap->ex_buf = NULL; > + adap->ex_buf_len = 0; > + memset(header, 0, sizeof(*header)); > + } Again, why a scoped guard for something like this that does not need it? > + > + mutex_unlock(&adap->mutex); > + > + return ret; > +} > + > +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf, > + unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len) > +{ > + return ljca_send(client->adapter, client->type, cmd, > + obuf, obuf_len, ibuf, ibuf_len, true, > + LJCA_WRITE_ACK_TIMEOUT_MS); > +} > +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA); > + > +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf, > + unsigned int obuf_len) > +{ > + return ljca_send(client->adapter, client->type, cmd, obuf, > + obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS); > +} > +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA); > + > +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, > + void *context) > +{ > + unsigned long flags; > + > + if (!event_cb) > + return -EINVAL; > + > + spin_lock_irqsave(&client->event_cb_lock, flags); > + > + if (client->event_cb) { > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > + return -EALREADY; > + } > + > + client->event_cb = event_cb; > + client->context = context; > + > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA); > + > +void ljca_unregister_event_cb(struct ljca_client *client) > +{ > + scoped_guard(spinlock_irqsave, &client->event_cb_lock) { > + client->event_cb = NULL; > + client->context = NULL; > + } > +} > +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA); > + > +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) > + uid = "0"; 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. Or am I mis-reading this function wrong, how can this ever be a valid UID to compare against? > + else > + uid = strchr(uid, wd->uid[0]); > + > + if (!uid || strcmp(uid, wd->uid)) > + return 0; > + > +match: > + wd->adev = adev; > + > + return 1; > +} > + > +/* bind auxiliary device to acpi device */ > +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > + struct auxiliary_device *auxdev, > + u64 adr, u8 id) > +{ > + struct ljca_match_ids_walk_data wd = { 0 }; > + struct acpi_device *parent, *adev; > + struct device *dev = adap->dev; > + char uid[4]; > + > + parent = ACPI_COMPANION(dev); > + if (!parent) > + return; > + > + /* > + * get auxdev ACPI handle from the ACPI device directly > + * under the parent that matches _ADR. > + */ > + adev = acpi_find_child_device(parent, adr, false); > + if (adev) { > + ACPI_COMPANION_SET(&auxdev->dev, adev); > + return; > + } > + > + /* > + * _ADR is a grey area in the ACPI specification, some > + * platforms use _HID to distinguish children devices. > + */ > + switch (adr) { > + case LJCA_GPIO_ACPI_ADR: > + wd.ids = ljca_gpio_hids; > + break; > + case LJCA_I2C1_ACPI_ADR: > + case LJCA_I2C2_ACPI_ADR: > + snprintf(uid, sizeof(uid), "%d", id); > + wd.uid = uid; > + wd.ids = ljca_i2c_hids; > + break; > + case LJCA_SPI1_ACPI_ADR: > + case LJCA_SPI2_ACPI_ADR: > + wd.ids = ljca_spi_hids; > + break; > + default: > + dev_warn(dev, "unsupported _ADR\n"); > + return; > + } > + > + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); > + if (wd.adev) { > + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); > + return; > + } > + > + parent = ACPI_COMPANION(dev->parent->parent); > + if (!parent) > + return; > + > + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); > + if (wd.adev) > + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); > +} > + > +static void ljca_auxdev_release(struct device *dev) > +{ > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > + > + kfree(auxdev->dev.platform_data); > +} > + > +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id, > + char *name, void *data, u64 adr) > +{ > + struct auxiliary_device *auxdev; > + struct ljca_client *client; > + int ret; > + > + client = kzalloc(sizeof *client, GFP_KERNEL); > + if (!client) > + return -ENOMEM; > + > + client->type = type; > + client->id = id; > + client->adapter = adap; > + spin_lock_init(&client->event_cb_lock); > + > + auxdev = &client->auxdev; > + auxdev->name = name; > + auxdev->id = id; > + > + auxdev->dev.parent = adap->dev; > + auxdev->dev.platform_data = data; > + auxdev->dev.release = ljca_auxdev_release; > + > + ret = auxiliary_device_init(auxdev); > + if (ret) > + goto err_free; > + > + ljca_auxdev_acpi_bind(adap, auxdev, adr, id); > + > + ret = auxiliary_device_add(auxdev); > + if (ret) > + goto err_uninit; > + > + list_add_tail(&client->link, &adap->client_list); > + > + return 0; > + > +err_uninit: > + auxiliary_device_uninit(auxdev); > + > +err_free: > + kfree(client); > + > + return ret; > +} > + > +static int ljca_enumerate_gpio(struct ljca_adapter *adap) > +{ > + u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)]; > + struct ljca_gpio_descriptor *desc; > + struct ljca_gpio_info *gpio_info; > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > + int ret, gpio_num; > + unsigned int i; > + > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf, > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > + if (ret < 0) > + return ret; > + > + /* check firmware response */ > + desc = (struct ljca_gpio_descriptor *)buf; > + if (ret != struct_size(desc, bank_desc, desc->bank_num)) > + return -EINVAL; > + > + gpio_num = desc->pins_per_bank * desc->bank_num; > + if (gpio_num > LJCA_MAX_GPIO_NUM) > + return -EINVAL; > + > + /* construct platform data */ > + gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL); > + if (!gpio_info) > + return -ENOMEM; > + gpio_info->num = gpio_num; > + > + for (i = 0; i < desc->bank_num; i++) > + valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins); > + bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num); > + > + ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio", > + gpio_info, LJCA_GPIO_ACPI_ADR); > + if (ret) > + kfree(gpio_info); > + > + return ret; > +} > + > +static int ljca_enumerate_i2c(struct ljca_adapter *adap) > +{ > + struct ljca_i2c_descriptor *desc; > + struct ljca_i2c_info *i2c_info; > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > + unsigned int i; > + int ret; > + > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf, > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > + if (ret < 0) > + return ret; > + > + /* check firmware response */ > + desc = (struct ljca_i2c_descriptor *)buf; > + if (ret != struct_size(desc, info, desc->num)) > + return -EINVAL; > + > + for (i = 0; i < desc->num; i++) { > + /* construct platform data */ > + i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL); > + if (!i2c_info) > + return -ENOMEM; > + > + i2c_info->id = desc->info[i].id; > + i2c_info->capacity = desc->info[i].capacity; > + i2c_info->intr_pin = desc->info[i].intr_pin; > + > + ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i, > + "ljca-i2c", i2c_info, > + LJCA_I2C1_ACPI_ADR + i); > + if (ret) { > + kfree(i2c_info); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int ljca_enumerate_spi(struct ljca_adapter *adap) > +{ > + struct ljca_spi_descriptor *desc; > + struct ljca_spi_info *spi_info; > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > + unsigned int i; > + int ret; > + > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf, > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > + if (ret < 0) > + return ret; > + > + desc = (struct ljca_spi_descriptor *)buf; > + for (i = 0; i < desc->num; i++) { > + /* construct platform data */ > + spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL); > + if (!spi_info) > + return -ENOMEM; > + > + spi_info->id = desc->info[i].id; > + spi_info->capacity = desc->info[i].capacity; > + > + ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i, > + "ljca-spi", spi_info, > + LJCA_SPI1_ACPI_ADR + i); > + if (ret) { > + kfree(spi_info); > + return ret; What about the other objects that you created here, don't you need to unwind the new ones if one in the chain fails? And if not, why stop at the first failure? > + } > + } > + > + return 0; > +} > + > +static int ljca_reset_handshake(struct ljca_adapter *adap) > +{ > + __le32 reset_id = cpu_to_le32(adap->reset_id); > + __le32 reset_id_ret = 0; > + int ret; > + > + adap->reset_id++; > + > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id, > + sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32), > + true, LJCA_WRITE_ACK_TIMEOUT_MS); > + if (ret < 0) > + return ret; > + > + if (reset_id_ret != reset_id) > + return -EINVAL; > + > + return 0; > +} > + > +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"); So none of these "errors" are actually errors: > + return 0; You return success? Why? Are they not actually problems? thanks, greg k-h
Hi Greg, Thanks for your review > From: Greg KH <gregkh@linuxfoundation.org> > On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote: > > +/* ljca cmd message structure */ > > +struct ljca_msg { > > + u8 type; > > + u8 cmd; > > + u8 flags; > > + u8 len; > > + u8 data[]; > > Shouldn't you be using the __counted_by attributes for all of these [] arrays so > that the compiler knows what to do and how to check that you don't overrun > the buffer? In this structure, len is not for data length, instead it's the length of header and data. > > Adding that now will save you having to add it later, AND it might catch bugs you > already have so please add that. But, I will add the __counted_by attribute for others like below: struct ljca_i2c_descriptor { u8 num; struct ljca_i2c_ctr_info info[] __counted_by(num); } __packed; struct ljca_spi_descriptor { u8 num; struct ljca_spi_ctr_info info[] __counted_by(num); } __packed; struct ljca_gpio_descriptor { u8 pins_per_bank; u8 bank_num; struct ljca_bank_descriptor bank_desc[] __counted_by(bank_num); } __packed; > > > + > > +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 */ > > + u8 *ex_buf; > > + unsigned int ex_buf_len; > > + /* actual data length copied to external buffer */ > > + unsigned int actual_length; > > + > > + /* buffer for send */ > > + void *tx_buf; > > + unsigned int tx_buf_len; > > + > > + /* lock to protect tx_buf and ex_buf */ > > + spinlock_t lock; > > + > > + struct completion cmd_completion; > > + > > + /* mutex to protect command download */ > > + struct mutex mutex; > > + > > + /* client device list */ > > + struct list_head client_list; > > + > > + /* disconnect ongoing or not */ > > + bool disconnect; > > + > > + /* used to reset firmware */ > > Can you use proper kernel doc for this structure instead of inline comments? Ack, thanks > > > + u32 reset_id; > > +}; > > + > > +struct ljca_match_ids_walk_data { > > + const struct acpi_device_id *ids; > > + const char *uid; > > + struct acpi_device *adev; > > +}; > > + > > +static const struct acpi_device_id ljca_gpio_hids[] = { > > + { "INTC1074" }, > > + { "INTC1096" }, > > + { "INTC100B" }, > > + { "INTC10D1" }, > > + {}, > > +}; > > + > > +static const struct acpi_device_id ljca_i2c_hids[] = { > > + { "INTC1075" }, > > + { "INTC1097" }, > > + { "INTC100C" }, > > + { "INTC10D2" }, > > + {}, > > +}; > > + > > +static const struct acpi_device_id ljca_spi_hids[] = { > > + { "INTC1091" }, > > + { "INTC1098" }, > > + { "INTC100D" }, > > + { "INTC10D3" }, > > + {}, > > +}; > > + > > +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 (client->type == header->type) { > > + scoped_guard(spinlock_irqsave, &client- > >event_cb_lock) { > > + client->event_cb(client->context, header->cmd, > > + header->data, header->len); > > + } > > + > > + break; > > + } > > + } > > +} > > + > > +/* process command ack */ > > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > > + struct ljca_msg *header) > > We can use 100 columns... Ok, I will make this in one row. > > > +{ > > + struct ljca_msg *tx_header = adap->tx_buf; > > + unsigned int actual_len = 0; > > + unsigned int ibuf_len; > > + unsigned long flags; > > + u8 *ibuf; > > + > > + spin_lock_irqsave(&adap->lock, flags); > > + > > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) > { > > + dev_err(adap->dev, "cmd ack mismatch error\n"); > > When you print error messages like this, who can do something about it? > And why print with interrupts disabled? Thanks, this will be like below: if (tx_header->type != header->type || tx_header->cmd != header->cmd) { spin_unlock_irqrestore(&adap->lock, flags); dev_err(adap->dev, "cmd ack mismatch error\n"); return; } > > > + spin_unlock_irqrestore(&adap->lock, flags); > > + return; > > + } > > + > > + ibuf_len = adap->ex_buf_len; > > + ibuf = adap->ex_buf; > > + > > + if (ibuf && ibuf_len) { > > + actual_len = min_t(unsigned int, header->len, ibuf_len); > > 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. > > > + > > + /* copy received data to external buffer */ > > + memcpy(ibuf, header->data, actual_len); > > + } > > + /* update copied data length */ > > + adap->actual_length = actual_len; > > Wait, what happens if you don't actually copy the data? This actual_length is the actual length of data copied to external buffer where is to save the command response. The API callers should check the response length according to the command you passed to this API. > Why lie and say you did? Where is the error handling? As I said, the API callers should have the error handing because they know the exact response format, and actually I already the error handling in this patch set. > > > + > > + spin_unlock_irqrestore(&adap->lock, flags); > > + > > + complete(&adap->cmd_completion); > > +} > > + > > +static void ljca_recv(struct urb *urb) > > No error handling? Actually I have, I re-structure this function as below to make it more clear, static void ljca_recv(struct urb *urb) { struct ljca_msg *header = urb->transfer_buffer; struct ljca_adapter *adap = urb->context; int ret; switch (urb->status) { case 0: /* success */ break; case -ENOENT: /* * directly complete the possible ongoing transfer * during disconnect */ if (adap->disconnect) complete(&adap->cmd_completion); return; case -ECONNRESET: case -ESHUTDOWN: case -EPIPE: /* rx urb is terminated */ dev_dbg(adap->dev, "rx urb terminated with status: %d\n", urb->status); return; default: dev_dbg(adap->dev, "rx urb error: %d\n", urb->status); goto resubmit; } if (header->len + sizeof(*header) != urb->actual_length) goto resubmit; if (header->flags & LJCA_ACK_FLAG) ljca_handle_cmd_ack(adap, header); else ljca_handle_event(adap, header); resubmit: ret = usb_submit_urb(urb, GFP_ATOMIC); if (ret && ret != -EPERM) dev_err(adap->dev, "resubmit rx urb error %d\n", ret); } > > > +{ > > + struct ljca_msg *header = urb->transfer_buffer; > > + struct ljca_adapter *adap = urb->context; > > + int ret; > > + > > + if (urb->status) { > > + /* sync/async unlink faults aren't errors */ > > + if (urb->status == -ENOENT) { > > + /* > > + * directly complete the possible ongoing transfer > > + * during disconnect > > + */ > > + if (adap->disconnect) > > + complete(&adap->cmd_completion); > > + return; > > + } > > + > > + if (urb->status == -ECONNRESET || urb->status == - > ESHUTDOWN) > > + return; > > + > > + dev_err(adap->dev, "recv urb error: %d\n", urb->status); > > + goto resubmit; > > You have an error, yet you don't report it you just spam the kernel log? > Why? Doesn't this happen when a device is removed? When device is removed, it will be covered by ESHUTDOWN case in the above 'if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) ' Most of the command error cases have been handled by the above 'if (usb->status == ', for others it will print error log, In the re-structured code, I changed it to dev_dbg to avoid spam. > > > + } > > + > > + if (header->len + sizeof(*header) != urb->actual_length) > > + goto resubmit; > > + > > + if (header->flags & LJCA_ACK_FLAG) > > + ljca_handle_cmd_ack(adap, header); > > + else > > + ljca_handle_event(adap, header); > > + > > +resubmit: > > + ret = usb_submit_urb(urb, GFP_ATOMIC); > > Why atomic, is this in a urb callback? Yes, > > > + if (ret && ret != -EPERM) > > + dev_err(adap->dev, "resubmit recv urb error %d\n", ret); > > What happens if ret is an error here? No command response, and there will be command timeout. > > > +} > > + > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > > + const u8 *obuf, unsigned int obuf_len, u8 *ibuf, > > + unsigned int 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 actual; > > + int ret = 0; > > + > > + if (adap->disconnect) > > + return -ENODEV; > > + > > + if (msg_len > adap->tx_buf_len) > > + return -EINVAL; > > + > > + mutex_lock(&adap->mutex); > > + > > + scoped_guard(spinlock_irqsave, &adap->lock) { > > + 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; > > + } > > 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. > > 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. > > > + > > + reinit_completion(&adap->cmd_completion); > > + > > + usb_autopm_get_interface(adap->intf); > > This can fail. Why aren't you checking that? Ack, thanks, and it will be like below: 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, &actual, LJCA_WRITE_TIMEOUT_MS); > > + > > + 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; > > 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(). > > 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. > > > +out: > > + scoped_guard(spinlock_irqsave, &adap->lock) { > > + adap->ex_buf = NULL; > > + adap->ex_buf_len = 0; > > + memset(header, 0, sizeof(*header)); > > + } > > Again, why a scoped guard for something like this that does not need it? As I said above, this is to address your last time review comment if I understand correctly, I can switch to the original one if we don't need this scoped guard. > > > + > > + mutex_unlock(&adap->mutex); > > + > > + return ret; > > +} > > + > > +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf, > > + unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len) { > > + return ljca_send(client->adapter, client->type, cmd, > > + obuf, obuf_len, ibuf, ibuf_len, true, > > + LJCA_WRITE_ACK_TIMEOUT_MS); > > +} > > +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA); > > + > > +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf, > > + unsigned int obuf_len) > > +{ > > + return ljca_send(client->adapter, client->type, cmd, obuf, > > + obuf_len, NULL, 0, false, > LJCA_WRITE_ACK_TIMEOUT_MS); } > > +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA); > > + > > +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, > > + void *context) > > +{ > > + unsigned long flags; > > + > > + if (!event_cb) > > + return -EINVAL; > > + > > + spin_lock_irqsave(&client->event_cb_lock, flags); > > + > > + if (client->event_cb) { > > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > > + return -EALREADY; > > + } > > + > > + client->event_cb = event_cb; > > + client->context = context; > > + > > + spin_unlock_irqrestore(&client->event_cb_lock, flags); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA); > > + > > +void ljca_unregister_event_cb(struct ljca_client *client) { > > + scoped_guard(spinlock_irqsave, &client->event_cb_lock) { > > + client->event_cb = NULL; > > + client->context = NULL; > > + } > > +} > > +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA); > > + > > +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) > > + uid = "0"; > > 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. > > Or am I mis-reading this function wrong, how can this ever be a valid UID to > compare against? > > > + else > > + uid = strchr(uid, wd->uid[0]); > > + > > + if (!uid || strcmp(uid, wd->uid)) > > + return 0; > > + > > +match: > > + wd->adev = adev; > > + > > + return 1; > > +} > > + > > +/* bind auxiliary device to acpi device */ static void > > +ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > > + struct auxiliary_device *auxdev, > > + u64 adr, u8 id) > > +{ > > + struct ljca_match_ids_walk_data wd = { 0 }; > > + struct acpi_device *parent, *adev; > > + struct device *dev = adap->dev; > > + char uid[4]; > > + > > + parent = ACPI_COMPANION(dev); > > + if (!parent) > > + return; > > + > > + /* > > + * get auxdev ACPI handle from the ACPI device directly > > + * under the parent that matches _ADR. > > + */ > > + adev = acpi_find_child_device(parent, adr, false); > > + if (adev) { > > + ACPI_COMPANION_SET(&auxdev->dev, adev); > > + return; > > + } > > + > > + /* > > + * _ADR is a grey area in the ACPI specification, some > > + * platforms use _HID to distinguish children devices. > > + */ > > + switch (adr) { > > + case LJCA_GPIO_ACPI_ADR: > > + wd.ids = ljca_gpio_hids; > > + break; > > + case LJCA_I2C1_ACPI_ADR: > > + case LJCA_I2C2_ACPI_ADR: > > + snprintf(uid, sizeof(uid), "%d", id); > > + wd.uid = uid; > > + wd.ids = ljca_i2c_hids; > > + break; > > + case LJCA_SPI1_ACPI_ADR: > > + case LJCA_SPI2_ACPI_ADR: > > + wd.ids = ljca_spi_hids; > > + break; > > + default: > > + dev_warn(dev, "unsupported _ADR\n"); > > + return; > > + } > > + > > + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); > > + if (wd.adev) { > > + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); > > + return; > > + } > > + > > + parent = ACPI_COMPANION(dev->parent->parent); > > + if (!parent) > > + return; > > + > > + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); > > + if (wd.adev) > > + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); } > > + > > +static void ljca_auxdev_release(struct device *dev) { > > + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); > > + > > + kfree(auxdev->dev.platform_data); > > +} > > + > > +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id, > > + char *name, void *data, u64 adr) { > > + struct auxiliary_device *auxdev; > > + struct ljca_client *client; > > + int ret; > > + > > + client = kzalloc(sizeof *client, GFP_KERNEL); > > + if (!client) > > + return -ENOMEM; > > + > > + client->type = type; > > + client->id = id; > > + client->adapter = adap; > > + spin_lock_init(&client->event_cb_lock); > > + > > + auxdev = &client->auxdev; > > + auxdev->name = name; > > + auxdev->id = id; > > + > > + auxdev->dev.parent = adap->dev; > > + auxdev->dev.platform_data = data; > > + auxdev->dev.release = ljca_auxdev_release; > > + > > + ret = auxiliary_device_init(auxdev); > > + if (ret) > > + goto err_free; > > + > > + ljca_auxdev_acpi_bind(adap, auxdev, adr, id); > > + > > + ret = auxiliary_device_add(auxdev); > > + if (ret) > > + goto err_uninit; > > + > > + list_add_tail(&client->link, &adap->client_list); > > + > > + return 0; > > + > > +err_uninit: > > + auxiliary_device_uninit(auxdev); > > + > > +err_free: > > + kfree(client); > > + > > + return ret; > > +} > > + > > +static int ljca_enumerate_gpio(struct ljca_adapter *adap) { > > + u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)]; > > + struct ljca_gpio_descriptor *desc; > > + struct ljca_gpio_info *gpio_info; > > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > > + int ret, gpio_num; > > + unsigned int i; > > + > > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, > NULL, 0, buf, > > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > > + if (ret < 0) > > + return ret; > > + > > + /* check firmware response */ > > + desc = (struct ljca_gpio_descriptor *)buf; > > + if (ret != struct_size(desc, bank_desc, desc->bank_num)) > > + return -EINVAL; > > + > > + gpio_num = desc->pins_per_bank * desc->bank_num; > > + if (gpio_num > LJCA_MAX_GPIO_NUM) > > + return -EINVAL; > > + > > + /* construct platform data */ > > + gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL); > > + if (!gpio_info) > > + return -ENOMEM; > > + gpio_info->num = gpio_num; > > + > > + for (i = 0; i < desc->bank_num; i++) > > + valid_pin[i] = get_unaligned_le32(&desc- > >bank_desc[i].valid_pins); > > + bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num); > > + > > + ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio", > > + gpio_info, LJCA_GPIO_ACPI_ADR); > > + if (ret) > > + kfree(gpio_info); > > + > > + return ret; > > +} > > + > > +static int ljca_enumerate_i2c(struct ljca_adapter *adap) { > > + struct ljca_i2c_descriptor *desc; > > + struct ljca_i2c_info *i2c_info; > > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > > + unsigned int i; > > + int ret; > > + > > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, > 0, buf, > > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > > + if (ret < 0) > > + return ret; > > + > > + /* check firmware response */ > > + desc = (struct ljca_i2c_descriptor *)buf; > > + if (ret != struct_size(desc, info, desc->num)) > > + return -EINVAL; > > + > > + for (i = 0; i < desc->num; i++) { > > + /* construct platform data */ > > + i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL); > > + if (!i2c_info) > > + return -ENOMEM; > > + > > + i2c_info->id = desc->info[i].id; > > + i2c_info->capacity = desc->info[i].capacity; > > + i2c_info->intr_pin = desc->info[i].intr_pin; > > + > > + ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i, > > + "ljca-i2c", i2c_info, > > + LJCA_I2C1_ACPI_ADR + i); > > + if (ret) { > > + kfree(i2c_info); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int ljca_enumerate_spi(struct ljca_adapter *adap) { > > + struct ljca_spi_descriptor *desc; > > + struct ljca_spi_info *spi_info; > > + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; > > + unsigned int i; > > + int ret; > > + > > + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, > 0, buf, > > + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > > + if (ret < 0) > > + return ret; > > + > > + desc = (struct ljca_spi_descriptor *)buf; > > + for (i = 0; i < desc->num; i++) { > > + /* construct platform data */ > > + spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL); > > + if (!spi_info) > > + return -ENOMEM; > > + > > + spi_info->id = desc->info[i].id; > > + spi_info->capacity = desc->info[i].capacity; > > + > > + ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i, > > + "ljca-spi", spi_info, > > + LJCA_SPI1_ACPI_ADR + i); > > + if (ret) { > > + kfree(spi_info); > > + return ret; > > What about the other objects that you created here, don't you need to unwind > the new ones if one in the chain fails? And if not, why stop at the first failure? > > > > + } > > + } > > + > > + return 0; > > +} > > + > > + > > +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"); > > So none of these "errors" are actually errors: > > > + return 0; > > You return success? Why? Are they not actually problems? This is to be compatible with old version FW which does not support full USB2xxx functions, so it just warn here as this is. To make things more clear, I re-structure this as below, hope that helps, thanks static int ljca_enumerate_clients(struct ljca_adapter *adap) { struct ljca_client *client, *next; int ret; ret = ljca_reset_handshake(adap); if (ret) return ret; ret = ljca_enumerate_gpio(adap); if (ret) goto err_free; ret = ljca_enumerate_i2c(adap); if (ret) goto err_free; ret = ljca_enumerate_spi(adap); if (ret) goto err_free; return 0; err_free: adap->disconnect = true; 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; } And accordingly, the ljca_enumerate_xxx() has slightly change to cover the unsupported case as below( take spi as an example) @@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap) ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf, sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); + if (ret == -ETIMEDOUT) { + dev_warn(adap->dev, "doesn't support SPI function\n"); + return 0; + } + if (ret < 0) return ret; Thanks Wentong > > thanks, > > greg k-h
On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote: > > From: Greg KH <gregkh@linuxfoundation.org> > > 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? :) > > > + spin_unlock_irqrestore(&adap->lock, flags); > > > + return; > > > + } > > > + > > > + ibuf_len = adap->ex_buf_len; > > > + ibuf = adap->ex_buf; > > > + > > > + if (ibuf && ibuf_len) { > > > + actual_len = min_t(unsigned int, header->len, ibuf_len); > > > > 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. > > > + > > > + /* copy received data to external buffer */ > > > + memcpy(ibuf, header->data, actual_len); > > > + } > > > + /* update copied data length */ > > > + adap->actual_length = actual_len; > > > > Wait, what happens if you don't actually copy the data? > > This actual_length is the actual length of data copied to external buffer > where is to save the command response. The API callers should check > the response length according to the command you passed to this API. But they aren't checking it as I pointed out elsewhere. > > > +} > > > + > > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > > > + const u8 *obuf, unsigned int obuf_len, u8 *ibuf, > > > + unsigned int 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 actual; > > > + int ret = 0; > > > + > > > + if (adap->disconnect) > > > + return -ENODEV; > > > + > > > + if (msg_len > adap->tx_buf_len) > > > + return -EINVAL; > > > + > > > + mutex_lock(&adap->mutex); > > > + > > > + scoped_guard(spinlock_irqsave, &adap->lock) { > > > + 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; > > > + } > > > > 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?) > > 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. > > > + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header, > > > + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS); > > > + > > > + 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; > > > > 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. > > 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. Please make the api easy to use, right now it's not. > > > +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) > > > + uid = "0"; > > > > 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. > > > +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"); > > > > So none of these "errors" are actually errors: > > > > > + return 0; > > > > You return success? Why? Are they not actually problems? > > 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? > To make things more clear, I re-structure this as below, hope that > helps, thanks > > static int ljca_enumerate_clients(struct ljca_adapter *adap) > { > struct ljca_client *client, *next; > int ret; > > ret = ljca_reset_handshake(adap); > if (ret) > return ret; > > ret = ljca_enumerate_gpio(adap); > if (ret) > goto err_free; > > ret = ljca_enumerate_i2c(adap); > if (ret) > goto err_free; > > ret = ljca_enumerate_spi(adap); > if (ret) > goto err_free; > > return 0; > > err_free: > adap->disconnect = true; > > 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; > } > > And accordingly, the ljca_enumerate_xxx() has slightly change to cover the > unsupported case as below( take spi as an example) > > @@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap) > > ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf, > sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); > + if (ret == -ETIMEDOUT) { > + dev_warn(adap->dev, "doesn't support SPI function\n"); > + return 0; 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. thanks, greg k-h
> From: Greg KH <gregkh@linuxfoundation.org> > On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote: > > > From: Greg KH <gregkh@linuxfoundation.org> 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; } Thanks, Wentong > > thanks, > > greg k-h
> From: Wu, Wentong > > From: Greg KH <gregkh@linuxfoundation.org> On Fri, Sep 29, 2023 at > > 11:31:21AM +0000, Wu, Wentong wrote: > > > > From: Greg KH <gregkh@linuxfoundation.org> 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
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig index 99b15b7..c510af7 100644 --- a/drivers/usb/misc/Kconfig +++ b/drivers/usb/misc/Kconfig @@ -165,6 +165,19 @@ config APPLE_MFI_FASTCHARGE It is safe to say M here. +config USB_LJCA + tristate "Intel La Jolla Cove Adapter support" + select AUXILIARY_BUS + depends on USB && ACPI + help + This adds support for Intel La Jolla Cove USB-I2C/SPI/GPIO + Master Adapter (LJCA). Additional drivers such as I2C_LJCA, + GPIO_LJCA and SPI_LJCA must be enabled in order to use the + functionality of the device. + + This driver can also be built as a module. If so, the module + will be called usb-ljca. + source "drivers/usb/misc/sisusbvga/Kconfig" config USB_LD diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile index 1992cc2..0bc732bc 100644 --- a/drivers/usb/misc/Makefile +++ b/drivers/usb/misc/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_USB_EMI26) += emi26.o obj-$(CONFIG_USB_EMI62) += emi62.o obj-$(CONFIG_USB_EZUSB_FX2) += ezusb.o obj-$(CONFIG_APPLE_MFI_FASTCHARGE) += apple-mfi-fastcharge.o +obj-$(CONFIG_USB_LJCA) += usb-ljca.o obj-$(CONFIG_USB_IDMOUSE) += idmouse.o obj-$(CONFIG_USB_IOWARRIOR) += iowarrior.o obj-$(CONFIG_USB_ISIGHTFW) += isight_firmware.o diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c new file mode 100644 index 0000000..673aa2f --- /dev/null +++ b/drivers/usb/misc/usb-ljca.c @@ -0,0 +1,837 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Intel La Jolla Cove Adapter USB driver + * + * Copyright (c) 2023, Intel Corporation. + */ + +#include <linux/acpi.h> +#include <linux/auxiliary_bus.h> +#include <linux/cleanup.h> +#include <linux/dev_printk.h> +#include <linux/kernel.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h> +#include <linux/usb.h> +#include <linux/usb/ljca.h> + +#include <asm/unaligned.h> + +/* command flags */ +#define LJCA_ACK_FLAG BIT(0) +#define LJCA_RESP_FLAG BIT(1) +#define LJCA_CMPL_FLAG BIT(2) + +#define LJCA_MAX_PACKET_SIZE 64u +#define LJCA_MAX_PAYLOAD_SIZE \ + (LJCA_MAX_PACKET_SIZE - sizeof(struct ljca_msg)) + +#define LJCA_WRITE_TIMEOUT_MS 200 +#define LJCA_WRITE_ACK_TIMEOUT_MS 500 +#define LJCA_ENUM_CLIENT_TIMEOUT_MS 20 + +/* ljca client type */ +enum ljca_client_type { + LJCA_CLIENT_MNG = 1, + LJCA_CLIENT_GPIO = 3, + LJCA_CLIENT_I2C = 4, + LJCA_CLIENT_SPI = 5, +}; + +/* MNG client commands */ +enum ljca_mng_cmd { + LJCA_MNG_RESET = 2, + LJCA_MNG_ENUM_GPIO = 4, + LJCA_MNG_ENUM_I2C = 5, + LJCA_MNG_ENUM_SPI = 8, +}; + +/* ljca client acpi _ADR */ +enum ljca_client_acpi_adr { + LJCA_GPIO_ACPI_ADR, + LJCA_I2C1_ACPI_ADR, + LJCA_I2C2_ACPI_ADR, + LJCA_SPI1_ACPI_ADR, + LJCA_SPI2_ACPI_ADR, + LJCA_CLIENT_ACPI_ADR_MAX, +}; + +/* ljca cmd message structure */ +struct ljca_msg { + u8 type; + u8 cmd; + u8 flags; + u8 len; + u8 data[]; +} __packed; + +struct ljca_i2c_ctr_info { + u8 id; + u8 capacity; + u8 intr_pin; +} __packed; + +struct ljca_i2c_descriptor { + u8 num; + struct ljca_i2c_ctr_info info[]; +} __packed; + +struct ljca_spi_ctr_info { + u8 id; + u8 capacity; +} __packed; + +struct ljca_spi_descriptor { + u8 num; + struct ljca_spi_ctr_info info[]; +} __packed; + +struct ljca_bank_descriptor { + u8 bank_id; + u8 pin_num; + + /* 1 bit for each gpio, 1 means valid */ + __le32 valid_pins; +} __packed; + +struct ljca_gpio_descriptor { + u8 pins_per_bank; + u8 bank_num; + struct ljca_bank_descriptor bank_desc[]; +} __packed; + +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 */ + u8 *ex_buf; + unsigned int ex_buf_len; + /* actual data length copied to external buffer */ + unsigned int actual_length; + + /* buffer for send */ + void *tx_buf; + unsigned int tx_buf_len; + + /* lock to protect tx_buf and ex_buf */ + spinlock_t lock; + + struct completion cmd_completion; + + /* mutex to protect command download */ + struct mutex mutex; + + /* client device list */ + struct list_head client_list; + + /* disconnect ongoing or not */ + bool disconnect; + + /* used to reset firmware */ + u32 reset_id; +}; + +struct ljca_match_ids_walk_data { + const struct acpi_device_id *ids; + const char *uid; + struct acpi_device *adev; +}; + +static const struct acpi_device_id ljca_gpio_hids[] = { + { "INTC1074" }, + { "INTC1096" }, + { "INTC100B" }, + { "INTC10D1" }, + {}, +}; + +static const struct acpi_device_id ljca_i2c_hids[] = { + { "INTC1075" }, + { "INTC1097" }, + { "INTC100C" }, + { "INTC10D2" }, + {}, +}; + +static const struct acpi_device_id ljca_spi_hids[] = { + { "INTC1091" }, + { "INTC1098" }, + { "INTC100D" }, + { "INTC10D3" }, + {}, +}; + +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. + */ + if (client->type == header->type) { + scoped_guard(spinlock_irqsave, &client->event_cb_lock) { + client->event_cb(client->context, header->cmd, + header->data, header->len); + } + + 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; + u8 *ibuf; + + spin_lock_irqsave(&adap->lock, flags); + + if (tx_header->type != header->type || tx_header->cmd != header->cmd) { + dev_err(adap->dev, "cmd ack mismatch error\n"); + spin_unlock_irqrestore(&adap->lock, flags); + return; + } + + ibuf_len = adap->ex_buf_len; + ibuf = adap->ex_buf; + + if (ibuf && ibuf_len) { + actual_len = min_t(unsigned int, header->len, ibuf_len); + + /* copy received data to external buffer */ + memcpy(ibuf, header->data, actual_len); + } + /* update copied data length */ + adap->actual_length = actual_len; + + spin_unlock_irqrestore(&adap->lock, flags); + + complete(&adap->cmd_completion); +} + +static void ljca_recv(struct urb *urb) +{ + struct ljca_msg *header = urb->transfer_buffer; + struct ljca_adapter *adap = urb->context; + int ret; + + if (urb->status) { + /* sync/async unlink faults aren't errors */ + if (urb->status == -ENOENT) { + /* + * directly complete the possible ongoing transfer + * during disconnect + */ + if (adap->disconnect) + complete(&adap->cmd_completion); + return; + } + + if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) + return; + + dev_err(adap->dev, "recv urb error: %d\n", urb->status); + goto resubmit; + } + + if (header->len + sizeof(*header) != urb->actual_length) + goto resubmit; + + if (header->flags & LJCA_ACK_FLAG) + ljca_handle_cmd_ack(adap, header); + else + ljca_handle_event(adap, header); + +resubmit: + ret = usb_submit_urb(urb, GFP_ATOMIC); + if (ret && ret != -EPERM) + dev_err(adap->dev, "resubmit recv urb error %d\n", ret); +} + +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, + const u8 *obuf, unsigned int obuf_len, u8 *ibuf, + unsigned int 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 actual; + int ret = 0; + + if (adap->disconnect) + return -ENODEV; + + if (msg_len > adap->tx_buf_len) + return -EINVAL; + + mutex_lock(&adap->mutex); + + scoped_guard(spinlock_irqsave, &adap->lock) { + 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; + } + + 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); + + 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: + scoped_guard(spinlock_irqsave, &adap->lock) { + adap->ex_buf = NULL; + adap->ex_buf_len = 0; + memset(header, 0, sizeof(*header)); + } + + mutex_unlock(&adap->mutex); + + return ret; +} + +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf, + unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len) +{ + return ljca_send(client->adapter, client->type, cmd, + obuf, obuf_len, ibuf, ibuf_len, true, + LJCA_WRITE_ACK_TIMEOUT_MS); +} +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA); + +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf, + unsigned int obuf_len) +{ + return ljca_send(client->adapter, client->type, cmd, obuf, + obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS); +} +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA); + +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, + void *context) +{ + unsigned long flags; + + if (!event_cb) + return -EINVAL; + + spin_lock_irqsave(&client->event_cb_lock, flags); + + if (client->event_cb) { + spin_unlock_irqrestore(&client->event_cb_lock, flags); + return -EALREADY; + } + + client->event_cb = event_cb; + client->context = context; + + spin_unlock_irqrestore(&client->event_cb_lock, flags); + + return 0; +} +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA); + +void ljca_unregister_event_cb(struct ljca_client *client) +{ + scoped_guard(spinlock_irqsave, &client->event_cb_lock) { + client->event_cb = NULL; + client->context = NULL; + } +} +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA); + +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) + uid = "0"; + else + uid = strchr(uid, wd->uid[0]); + + if (!uid || strcmp(uid, wd->uid)) + return 0; + +match: + wd->adev = adev; + + return 1; +} + +/* bind auxiliary device to acpi device */ +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, + struct auxiliary_device *auxdev, + u64 adr, u8 id) +{ + struct ljca_match_ids_walk_data wd = { 0 }; + struct acpi_device *parent, *adev; + struct device *dev = adap->dev; + char uid[4]; + + parent = ACPI_COMPANION(dev); + if (!parent) + return; + + /* + * get auxdev ACPI handle from the ACPI device directly + * under the parent that matches _ADR. + */ + adev = acpi_find_child_device(parent, adr, false); + if (adev) { + ACPI_COMPANION_SET(&auxdev->dev, adev); + return; + } + + /* + * _ADR is a grey area in the ACPI specification, some + * platforms use _HID to distinguish children devices. + */ + switch (adr) { + case LJCA_GPIO_ACPI_ADR: + wd.ids = ljca_gpio_hids; + break; + case LJCA_I2C1_ACPI_ADR: + case LJCA_I2C2_ACPI_ADR: + snprintf(uid, sizeof(uid), "%d", id); + wd.uid = uid; + wd.ids = ljca_i2c_hids; + break; + case LJCA_SPI1_ACPI_ADR: + case LJCA_SPI2_ACPI_ADR: + wd.ids = ljca_spi_hids; + break; + default: + dev_warn(dev, "unsupported _ADR\n"); + return; + } + + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); + if (wd.adev) { + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); + return; + } + + parent = ACPI_COMPANION(dev->parent->parent); + if (!parent) + return; + + acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd); + if (wd.adev) + ACPI_COMPANION_SET(&auxdev->dev, wd.adev); +} + +static void ljca_auxdev_release(struct device *dev) +{ + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + + kfree(auxdev->dev.platform_data); +} + +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id, + char *name, void *data, u64 adr) +{ + struct auxiliary_device *auxdev; + struct ljca_client *client; + int ret; + + client = kzalloc(sizeof *client, GFP_KERNEL); + if (!client) + return -ENOMEM; + + client->type = type; + client->id = id; + client->adapter = adap; + spin_lock_init(&client->event_cb_lock); + + auxdev = &client->auxdev; + auxdev->name = name; + auxdev->id = id; + + auxdev->dev.parent = adap->dev; + auxdev->dev.platform_data = data; + auxdev->dev.release = ljca_auxdev_release; + + ret = auxiliary_device_init(auxdev); + if (ret) + goto err_free; + + ljca_auxdev_acpi_bind(adap, auxdev, adr, id); + + ret = auxiliary_device_add(auxdev); + if (ret) + goto err_uninit; + + list_add_tail(&client->link, &adap->client_list); + + return 0; + +err_uninit: + auxiliary_device_uninit(auxdev); + +err_free: + kfree(client); + + return ret; +} + +static int ljca_enumerate_gpio(struct ljca_adapter *adap) +{ + u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)]; + struct ljca_gpio_descriptor *desc; + struct ljca_gpio_info *gpio_info; + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; + int ret, gpio_num; + unsigned int i; + + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf, + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); + if (ret < 0) + return ret; + + /* check firmware response */ + desc = (struct ljca_gpio_descriptor *)buf; + if (ret != struct_size(desc, bank_desc, desc->bank_num)) + return -EINVAL; + + gpio_num = desc->pins_per_bank * desc->bank_num; + if (gpio_num > LJCA_MAX_GPIO_NUM) + return -EINVAL; + + /* construct platform data */ + gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL); + if (!gpio_info) + return -ENOMEM; + gpio_info->num = gpio_num; + + for (i = 0; i < desc->bank_num; i++) + valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins); + bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num); + + ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio", + gpio_info, LJCA_GPIO_ACPI_ADR); + if (ret) + kfree(gpio_info); + + return ret; +} + +static int ljca_enumerate_i2c(struct ljca_adapter *adap) +{ + struct ljca_i2c_descriptor *desc; + struct ljca_i2c_info *i2c_info; + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; + unsigned int i; + int ret; + + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf, + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); + if (ret < 0) + return ret; + + /* check firmware response */ + desc = (struct ljca_i2c_descriptor *)buf; + if (ret != struct_size(desc, info, desc->num)) + return -EINVAL; + + for (i = 0; i < desc->num; i++) { + /* construct platform data */ + i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL); + if (!i2c_info) + return -ENOMEM; + + i2c_info->id = desc->info[i].id; + i2c_info->capacity = desc->info[i].capacity; + i2c_info->intr_pin = desc->info[i].intr_pin; + + ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i, + "ljca-i2c", i2c_info, + LJCA_I2C1_ACPI_ADR + i); + if (ret) { + kfree(i2c_info); + return ret; + } + } + + return 0; +} + +static int ljca_enumerate_spi(struct ljca_adapter *adap) +{ + struct ljca_spi_descriptor *desc; + struct ljca_spi_info *spi_info; + u8 buf[LJCA_MAX_PAYLOAD_SIZE]; + unsigned int i; + int ret; + + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf, + sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS); + if (ret < 0) + return ret; + + desc = (struct ljca_spi_descriptor *)buf; + for (i = 0; i < desc->num; i++) { + /* construct platform data */ + spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL); + if (!spi_info) + return -ENOMEM; + + spi_info->id = desc->info[i].id; + spi_info->capacity = desc->info[i].capacity; + + ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i, + "ljca-spi", spi_info, + LJCA_SPI1_ACPI_ADR + i); + if (ret) { + kfree(spi_info); + return ret; + } + } + + return 0; +} + +static int ljca_reset_handshake(struct ljca_adapter *adap) +{ + __le32 reset_id = cpu_to_le32(adap->reset_id); + __le32 reset_id_ret = 0; + int ret; + + adap->reset_id++; + + ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id, + sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32), + true, LJCA_WRITE_ACK_TIMEOUT_MS); + if (ret < 0) + return ret; + + if (reset_id_ret != reset_id) + return -EINVAL; + + return 0; +} + +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"); + + return 0; +} + +static int ljca_probe(struct usb_interface *interface, + const struct usb_device_id *id) +{ + struct usb_device *usb_dev = interface_to_usbdev(interface); + struct usb_host_interface *alt = interface->cur_altsetting; + struct usb_endpoint_descriptor *ep_in, *ep_out; + struct device *dev = &interface->dev; + struct ljca_adapter *adap; + int ret; + + adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL); + if (!adap) + return -ENOMEM; + + /* separate tx buffer allocation for alignment */ + adap->tx_buf = devm_kzalloc(dev, LJCA_MAX_PACKET_SIZE, GFP_KERNEL); + if (!adap->tx_buf) + return -ENOMEM; + adap->tx_buf_len = LJCA_MAX_PACKET_SIZE; + + mutex_init(&adap->mutex); + spin_lock_init(&adap->lock); + init_completion(&adap->cmd_completion); + INIT_LIST_HEAD(&adap->client_list); + + adap->intf = usb_get_intf(interface); + adap->usb_dev = usb_dev; + adap->dev = dev; + + /* + * find the first bulk in and out endpoints. + * ignore any others. + */ + ret = usb_find_common_endpoints(alt, &ep_in, &ep_out, NULL, NULL); + if (ret) { + dev_err(dev, "bulk endpoints not found\n"); + goto err; + } + adap->rx_pipe = usb_rcvbulkpipe(usb_dev, usb_endpoint_num(ep_in)); + adap->tx_pipe = usb_sndbulkpipe(usb_dev, usb_endpoint_num(ep_out)); + + /* setup rx buffer */ + adap->rx_len = usb_endpoint_maxp(ep_in); + adap->rx_buf = devm_kzalloc(dev, adap->rx_len, GFP_KERNEL); + if (!adap->rx_buf) { + ret = -ENOMEM; + goto err; + } + + /* alloc rx urb */ + adap->rx_urb = usb_alloc_urb(0, GFP_KERNEL); + if (!adap->rx_urb) { + ret = -ENOMEM; + goto err; + } + usb_fill_bulk_urb(adap->rx_urb, usb_dev, adap->rx_pipe, + adap->rx_buf, adap->rx_len, ljca_recv, adap); + + usb_set_intfdata(interface, adap); + + /* submit rx urb before enumerate clients */ + ret = usb_submit_urb(adap->rx_urb, GFP_KERNEL); + if (ret) { + dev_err(dev, "submit rx urb failed: %d\n", ret); + goto err_free; + } + + ret = ljca_enumerate_clients(adap); + if (ret) + goto err_kill; + + usb_enable_autosuspend(usb_dev); + + return 0; + +err_kill: + usb_kill_urb(adap->rx_urb); + +err_free: + usb_free_urb(adap->rx_urb); + +err: + usb_put_intf(adap->intf); + mutex_destroy(&adap->mutex); + + return ret; +} + +static void ljca_disconnect(struct usb_interface *interface) +{ + struct ljca_adapter *adap = usb_get_intfdata(interface); + struct ljca_client *client, *next; + + 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); + } + + usb_free_urb(adap->rx_urb); + + usb_put_intf(adap->intf); + + mutex_destroy(&adap->mutex); +} + +static int ljca_suspend(struct usb_interface *interface, pm_message_t message) +{ + struct ljca_adapter *adap = usb_get_intfdata(interface); + + usb_kill_urb(adap->rx_urb); + + return 0; +} + +static int ljca_resume(struct usb_interface *interface) +{ + struct ljca_adapter *adap = usb_get_intfdata(interface); + + return usb_submit_urb(adap->rx_urb, GFP_KERNEL); +} + +static const struct usb_device_id ljca_table[] = { + { USB_DEVICE(0x8086, 0x0b63) }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(usb, ljca_table); + +static struct usb_driver ljca_driver = { + .name = "ljca", + .id_table = ljca_table, + .probe = ljca_probe, + .disconnect = ljca_disconnect, + .suspend = ljca_suspend, + .resume = ljca_resume, + .supports_autosuspend = 1, +}; +module_usb_driver(ljca_driver); + +MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>"); +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>"); +MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>"); +MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/usb/ljca.h b/include/linux/usb/ljca.h new file mode 100644 index 0000000..502fc8b --- /dev/null +++ b/include/linux/usb/ljca.h @@ -0,0 +1,145 @@ +/* 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 - represent a ljca client device + * + * @type: ljca client type + * @id: ljca client id within same client type + * @link: ljca client on the same ljca adapter + * @auxdev: auxiliary device object + * @adapter: ljca adapter the ljca client sit on + * @context: the execution context of the event callback + * @event_cb: ljca client driver register this callback to get + * firmware asynchronous rx buffer pending notifications + * @event_cb_lock: spinlock to protect event callback + */ +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 - ljca gpio client device info + * + * @num: ljca gpio client device pin number + * @valid_pin_map: ljca gpio client device valid pin mapping + */ +struct ljca_gpio_info { + unsigned int num; + DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM); +}; + +/** + * struct ljca_i2c_info - ljca i2c client device info + * + * @id: ljca i2c client device identification number + * @capacity: ljca i2c client device capacity + * @intr_pin: ljca i2c client device interrupt pin number if exists + */ +struct ljca_i2c_info { + u8 id; + u8 capacity; + u8 intr_pin; +}; + +/** + * struct ljca_spi_info - ljca spi client device info + * + * @id: ljca spi client device identification number + * @capacity: ljca spi client device capacity + */ +struct ljca_spi_info { + u8 id; + u8 capacity; +}; + +/** + * ljca_register_event_cb - register a callback function to receive events + * + * @client: ljca client device + * @event_cb: callback function + * @context: execution context of event callback + * + * Return: 0 in case of success, negative value in case of error + */ +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, void *context); + +/** + * ljca_unregister_event_cb - unregister the callback function for an event + * + * @client: ljca client device + */ +void ljca_unregister_event_cb(struct ljca_client *client); + +/** + * ljca_transfer - issue a LJCA command and wait for a response + * + * @client: ljca client device + * @cmd: the command to be sent to the device + * @obuf: the buffer to be sent to the device; it can be NULL if the user + * doesn't need to transmit data with this command + * @obuf_len: the size of the buffer to be sent to the device; it should + * be 0 when obuf is NULL + * @ibuf: any data associated with the response will be copied here; it can be + * NULL if the user doesn't need the response data + * @ibuf_len: must be initialized to the input buffer size + * + * Return: the actual length data transferred for success, negative value for errors + */ +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf, + unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len); + +/** + * ljca_transfer_noack - issue a LJCA command without a response + * + * @client: ljca client device + * @cmd: the command to be sent to the device + * @obuf: the buffer to be sent to the device; it can be NULL if the user + * doesn't need to transmit data with this command + * @obuf_len: the size of the buffer to be sent to the device + * + * Return: 0 for success, negative value for errors + */ +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf, + unsigned int obuf_len); + +#endif
Implements the USB part of Intel USB-I2C/GPIO/SPI adapter device named "La Jolla Cove Adapter" (LJCA). The communication between the various LJCA module drivers and the hardware will be muxed/demuxed by this driver. Three modules ( I2C, GPIO, and SPI) are supported currently. Each sub-module of LJCA device is identified by type field within the LJCA message header. The sub-modules of LJCA can use ljca_transfer() to issue a transfer between host and hardware. And ljca_register_event_cb is exported to LJCA sub-module drivers for hardware event subscription. The minimum code in ASL that covers this board is Scope (\_SB.PCI0.DWC3.RHUB.HS01) { Device (GPIO) { Name (_ADR, Zero) Name (_STA, 0x0F) } Device (I2C) { Name (_ADR, One) Name (_STA, 0x0F) } Device (SPI) { Name (_ADR, 0x02) Name (_STA, 0x0F) } } Signed-off-by: Wentong Wu <wentong.wu@intel.com> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Tested-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/misc/Kconfig | 13 + drivers/usb/misc/Makefile | 1 + drivers/usb/misc/usb-ljca.c | 837 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/usb/ljca.h | 145 ++++++++ 4 files changed, 996 insertions(+) create mode 100644 drivers/usb/misc/usb-ljca.c create mode 100644 include/linux/usb/ljca.h