[v2,1/2] usb: misc: xapea00x: add driver for Xaptum ENF Access Card

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@xaptum.com>");
> +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html