usb: misc: xapea00x: add driver for Xaptum ENF Access Card
Greg Kroah-Hartman
maintainerPATCH v2 1/2 usb: misc: xapea00x: add driver for Xaptum ENF Access Card
Quoted 8 lines · David R. Bild
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.
drivers/usb/misc/xapea00x/Kconfig line 6
Quoted 9 lines
drivers/usb/misc/xapea00x/Kconfig new file +6
-0@@ -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?
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 7
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +7
-0@@ -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 :)
Quoted 7 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +7
-0+*/ + +#include "xapea00x.h" + +#define XAPEA00X_BR_CMD_READ 0x00 +#define XAPEA00X_BR_CMD_WRITE 0x01 +#define XAPEA00X_BR_CMD_WRITEREAD 0x02
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 24
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+ +#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?
Quoted 13 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +13
-0+ +/******************************************************************************* +* Bridge USB transfers +*/ + +struct xapea00x_br_bulk_command { +__u16 reserved1; +__u8 command; +__u8 reserved2; +__le32 length; +} __attribute__((__packed__ )); + +/**
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 47
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+* 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.
Quoted 13 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +13
-0+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; + } +
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 70
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+ 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?
Quoted 31 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +31
-0+ +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) +{
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 111
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+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 :)
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 121
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+/* ---------------------- 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:
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 123
Quoted 2 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +2
-0+/* ---------------------- Finished ------------------------ */ +dev_info (&interface->dev,"device connected\n" );
That line. Not needed at all.
Quoted 16 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +16
-0+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); +
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 149
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+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.
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 153
Quoted 4 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +4
-0+} + +static struct usb_driver xapea00x_driver = { + .name ="xapea00x" ,
KBUILD_MOD_NAME instead?
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 158
Quoted 5 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +5
-0+ .probe = xapea00x_probe, + .disconnect = xapea00x_disconnect, + .suspend = NULL, + .resume = NULL, + .reset_resume = NULL,
Leave NULL fields alone, no need to even list them here.
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 160
Quoted 2 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +2
-0+ .id_table = xapea00x_devices, + .supports_autosuspend =0
And drop this, as it's set to 0 by default.
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 169
Quoted 9 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +9
-0+}; + +module_usb_driver (xapea00x_driver); + +MODULE_AUTHOR ("David R. Bild <[email protected]>" ); +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.
Quoted 12 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +12
-0+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
drivers/usb/misc/xapea00x/xapea00x-bridge.c line 191
Quoted 10 lines
drivers/usb/misc/xapea00x/xapea00x-bridge.c new file +10
-0+* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html