On Mon, Apr 30, 2018 at 07:54:17AM -0500, David R. Bild wrote:
> This commit adds a driver for the Xaptum ENF Access Card, a TPM2.0
> hardware module for authenticating IoT devices and gateways.
>
> The card consists of a SPI TPM 2.0 chip and a USB-SPI bridge. This
> driver configures the bridge, registers the bridge as an SPI
> controller, and adds the TPM 2.0 as an SPI device. The in-kernel TPM
> 2.0 driver is then automatically loaded to configure the TPM and
> expose it to userspace.
A few random USB driver-like review comments of minor things to tweak
for your next submission. Overall it looks good to me, but you should
also get the TPM maintainers/developers to review it as I will require
their review before I can take it in the USB tree.
> --- /dev/null
> +++ b/drivers/usb/misc/xapea00x/Kconfig
> @@ -0,0 +1,16 @@
> +config USB_XAPEA00X
> + tristate "Xaptum ENF Access card support (XAP-EA-00x)"
> + depends on USB_SUPPORT
> + select SPI
> + select TCG_TPM
> + select TCG_TIS_SPI
Do you really want to 'select' these? Why not just depend on them?
> --- /dev/null
> +++ b/drivers/usb/misc/xapea00x/xapea00x-bridge.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for the XAP-EA-00x series of the Xaptum Edge Access Card, a
> + * TPM 2.0-based hardware module for authenticating IoT devices and
> + * gateways.
> + *
> + * Copyright (c) 2017 Xaptum, Inc.
It's 2018 now :)
> + */
> +
> +#include "xapea00x.h"
> +
> +#define XAPEA00X_BR_CMD_READ 0x00
> +#define XAPEA00X_BR_CMD_WRITE 0x01
> +#define XAPEA00X_BR_CMD_WRITEREAD 0x02
> +
> +#define XAPEA00X_BR_BREQTYP_SET 0x40
> +
> +#define XAPEA00X_BR_BREQ_SET_GPIO_VALUES 0x21
> +#define XAPEA00X_BR_BREQ_SET_GPIO_CS 0x25
> +#define XAPEA00X_BR_BREQ_SET_SPI_WORD 0x31
> +
> +#define XAPEA00X_BR_USB_TIMEOUT 1000 // msecs
> +
> +#define XAPEA00X_BR_CS_DISABLED 0x00
Odd placement of spaces after the tabs, why?
> +
> +/*******************************************************************************
> + * Bridge USB transfers
> + */
> +
> +struct xapea00x_br_bulk_command {
> + __u16 reserved1;
> + __u8 command;
> + __u8 reserved2;
> + __le32 length;
> +} __attribute__((__packed__));
> +
> +/**
> + * xapea00x_br_prep_bulk_command - Prepares the bulk command header with
> + * the supplied values.
> + * @hdr: pointer to header to prepare
> + * @command: the command id for the command
> + * @length: length in bytes of the command data
> + *
> + * Context: !in_interrupt()
> + *
> + * Return: If successful, 0. Otherwise a negative error number.
> + */
You don't need kerneldoc comments for static functions. Nice, but not a
requirement at all.
> +static int xapea00x_br_bulk_read(struct xapea00x_device *dev, void *data,
> + int len)
> +{
> + unsigned int pipe;
> + void *buf;
> + int actual_len, retval;
> +
> + buf = kzalloc(len, GFP_KERNEL);
> + if (!buf) {
> + retval = -ENOMEM;
> + goto out;
> + }
> +
> + pipe = usb_rcvbulkpipe(dev->udev, dev->bulk_in->bEndpointAddress);
> + retval = usb_bulk_msg(dev->udev, pipe, buf, len, &actual_len,
> + XAPEA00X_BR_USB_TIMEOUT);
> +
> + if (retval) {
> + dev_warn(&dev->interface->dev,
> + "%s: usb_bulk_msg() failed with %d\n",
> + __func__, retval);
> + goto free_buf;
> + }
Is this going to get noisy when the device gets removed from the system?
> +
> + memcpy(data, buf, actual_len);
> + retval = 0;
> +
> +free_buf:
> + kzfree(buf);
> +
> +out:
> + return retval;
> +}
> +
> +/**
> + * xapea00x_br_ctrl_write - Issues a send control transfer to the bridge
> + * chip.
> + * @dev: pointer to the device to write to
> + * @bRequest: the command
> + * @wValue: the command value
> + * @wIndex: the command index
> + * @data: pointer to the command data
> + * @len: length in bytes of the command data
> + *
> + * The possible bRequest, wValue, and wIndex values and data format
> + * are specified in the hardware datasheet.
> + *
> + * Context: !in_interrupt()
> + *
> + * Return: If successful, 0. Otherwise a negative error code.
> + */
> +static int xapea00x_br_ctrl_write(struct xapea00x_device *dev, u8 bRequest,
> + u16 wValue, u16 wIndex, u8 *data, u16 len)
> +{
> + unsigned int pipe;
> + void *buf;
> + int retval;
> +
> + /* control_msg buffer must be dma-capable (e.g.k kmalloc-ed,
> + * not stack). Copy data into such buffer here, so we can use
> + * simpler stack allocation in the callers - we have no
> + * performance concerns given the small buffers and low
> + * throughputs of this device.
> + */
This is well known and a requirement for all USB drivers, no need to
have to document it here :)
> + /* ---------------------- TPM SPI Device ------------------------ */
> + probe = kzalloc(sizeof(*probe), GFP_KERNEL);
> + if (!probe) {
> + retval = -ENOMEM;
> + goto free_spi;
> + }
> + xapea00x_init_async_probe(probe, dev, xapea00x_tpm_probe);
> +
> + schedule_work(&probe->work);
> + dev_info(&interface->dev, "scheduled initialization of TPM\n");
We do not care :)
Drivers should be silent, unless something bad happens. Please remove
noisy stuff like this, and:
> + /* ---------------------- Finished ------------------------ */
> + dev_info(&interface->dev, "device connected\n");
That line. Not needed at all.
> + return 0;
> +
> +free_spi:
> + spi_unregister_master(dev->spi_master);
> +
> +free_dev:
> + kref_put(&dev->kref, xapea00x_delete);
> +
> + dev_err(&interface->dev, "device failed with %d\n", retval);
> + return retval;
> +}
> +
> +static void xapea00x_disconnect(struct usb_interface *interface)
> +{
> + struct xapea00x_device *dev = usb_get_intfdata(interface);
> +
> + usb_set_intfdata(interface, NULL);
> + spi_unregister_master(dev->spi_master);
> +
> + mutex_lock(&dev->usb_mutex);
> + dev->interface = NULL;
> + mutex_unlock(&dev->usb_mutex);
> +
> + kref_put(&dev->kref, xapea00x_delete);
> +
> + dev_info(&dev->udev->dev, "device disconnected\n");
No need for that either.
> +}
> +
> +static struct usb_driver xapea00x_driver = {
> + .name = "xapea00x",
KBUILD_MOD_NAME instead?
> + .probe = xapea00x_probe,
> + .disconnect = xapea00x_disconnect,
> + .suspend = NULL,
> + .resume = NULL,
> + .reset_resume = NULL,
Leave NULL fields alone, no need to even list them here.
> + .id_table = xapea00x_devices,
> + .supports_autosuspend = 0
And drop this, as it's set to 0 by default.
> +};
> +
> +module_usb_driver(xapea00x_driver);
> +
> +MODULE_AUTHOR("David R. Bild <david.bild@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Xaptum XAP-EA-00x ENF Access card");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_ALIAS("xapea00x");
Ick, no, the build system will add the properly MODULE_ALIAS to the
code, do not write it out "by hand" here at all. Bad things can happen
if you get it wrong.
> +struct xapea00x_device {
> + struct kref kref;
> +
> + struct usb_device *udev;
> + /*
> + * The interface pointer will be set NULL when the device
> + * disconnects. Accessing it safe only while holding the
> + * usb_mutex.
> + */
> + struct usb_interface *interface;
> + /*
> + * Th usb_mutex must be held while synchronous USB requests are
> + * in progress. It is acquired during disconnect to be sure
> + * that there is not an outstanding request.
> + */
> + struct mutex usb_mutex;
> +
> + struct usb_endpoint_descriptor *bulk_in;
> + struct usb_endpoint_descriptor *bulk_out;
> +
> + u16 pid;
> + u16 vid;
Why do you care about these vid/pid values? You set them and never use
them. If you really needed them, you can get them from the pointer to
the interface above.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
