On Mon, Apr 30, 2018 at 11:05 AM, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
> >
> 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.
Thanks for the review.
> > --- /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?
Here's my reasoning, but I'm happy to change to "depends" if that
better follows common practice.
The device is a USB device and thus will only work on a machine with a
USB host. That's a property of the machine, so we just "depend" on it.
The SPI and TPM support requirements are internal to the device driver
(the device is SPI master, SPI device, and TPM). This driver needs
those systems enabled, but doesn't require any explicit SPI or TPM
support on the machine (e.g., a physical SPI master). Someone might
want to build a kernel with this driver, but would have no reason to
explicitly enable the SPI or TPM subsystems (after all, the machine
may not have any SPI or TPM hardware). So we "select" those here.
(I can also see using logic to argue for depending on USB_SUPPORT and
TCG_TPM, but selecting SPI and TCG_TIS_SPI...)
In the absence of any other guidance, that was my logic. I'm happy to
change to "depends".
> > +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.
They are used in the TPM initialization code (`xapea00x-tpm.c`).
There are a couple of models of xap-ea-00x with differing
initialization requirements.
Is there a shorter way to get the pid from the usb_device than
__le16_to_cpu(udev->descriptor.idProduct);
?
I found it cleaner to pull that out into a variable once, rather than
fill the TPM-specific code with that very USB-specific code.
Thanks,
David
--
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
