On Thu, Jan 11, 2018 at 4:27 AM, Oliver Neukum <oneukum@xxxxxxxx> wrote:
>
> Am Mittwoch, den 10.01.2018, 10:58 -0600 schrieb 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.
> >
Thanks for the review. Fixed a couple of definite bugs.
Responses (or requests for clarification) for some of your comments
follow. The others are fixed in a v2, which I'll sent once you answer
my clarifying questions.
> > +static int xapea00x_br_bulk_write(struct xapea00x_device *dev,
> > + struct xapea00x_br_bulk_command *header,
> > + const void *data, int len)
> > +{
> > + u8 *buf;
> > + unsigned int pipe;
> > + int buf_len, actual_len, retval;
> > +
> > + buf_len = sizeof(struct xapea00x_br_bulk_command) + len;
> > + buf = kzalloc(buf_len, GFP_KERNEL);
>
> Why zeroed? You copy all over it.
It's easiest to audit the code for incorrect usages of kalloc if we
just always use kzalloc.
This device isn't used very frequently --- once shortly after boot if
used correctly and once each time the internet connection is
re-established if used poorly. Each use sends just a couple KB
between device and host. So I'd rather optimize for ease of
functional correctness and safety than saving the few cycles needed to
zero something that is immediately overwritten.
Same logic for using kzfree everywhere, even on structures or buffers
that don't contain user or other potentially sensitive data.
> > + memcpy(buf, header, sizeof(struct xapea00x_br_bulk_command));
> > + memcpy(buf + sizeof(struct xapea00x_br_bulk_command), data, len);
> > +
> > + pipe = usb_sndbulkpipe(dev->udev, dev->bulk_out->bEndpointAddress);
> > + retval = usb_bulk_msg(dev->udev, pipe, buf, 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;
>
> Does this make any sense?
It does to me. What specifically do you think is odd?
> > + }
> > +
> > + retval = 0;
> > +
> > +free_buf:
> > + kzfree(buf);
> > +
> > +out:
> > + return retval;
> > +}
> > +static int xapea00x_br_ctrl_write(struct xapea00x_device *dev, u8 bRequest,
> > + u16 wValue, u16 wIndex, u8 *data, u16 len)
>
> Combine with xapea00x_br_bulk_write()?
No. xapea00x_br_bulk_write and xapea00x_br_ctrl_write conceptually do
very different things and take very different parameters.
> > +static int xapea00x_spi_setup(struct spi_device *spi)
> > +{
> > + struct xapea00x_device *dev;
> > + int retval;
> > +
> > + dev = spi_master_get_devdata(spi->master);
> > +
> > + mutex_lock(&dev->usb_mutex);
> > + if (!dev->interface) {
> > + retval = -ENODEV;
> > + goto out;
> > + }
> > +
> > + /* Verify that this is the TPM device */
> > + if (spi->chip_select != 0) {
> > + retval = -EINVAL;
> > + goto err;
> > + }
> > +
> > + /*
> > + * Disable auto chip select for the TPM channel.
> > + * Must be done after setting the SPI parameters.
> > + */
> > + retval = xapea00x_br_disable_cs(dev, 0);
> > + if (retval)
> > + goto err;
> > +
> > + /* De-assert chip select for the TPM channel. */
> > + retval = xapea00x_br_deassert_cs(dev, 0);
> > + if (retval)
> > + goto err;
> > +
> > + dev_dbg(&dev->interface->dev, "configured spi channel for tpm\n");
> > + retval = 0;
> > + goto out;
>
> The control flow is innovative.
Thanks ;)
I've removed the "retval = 0;" as that is unnecessary. Otherwise, do
you have a better control flow that doesn't involve repeating either
the following "dev_err" or "mutex_unlock". I've found this style to
be the safest in the face of future changes.
> > +
> > +err:
> > + dev_err(&dev->interface->dev,
> > + "configuring SPI channel failed with %d\n", retval);
> > +
> > +out:
> > + mutex_unlock(&dev->usb_mutex);
> > + return retval;
> > +}
> > +
> > + /* Deassert chip select, if requested */
> > + if (!cs_hold)
> > + retval = xapea00x_br_deassert_cs(dev, 0);
> > +
> > + /* Delay for the requested time */
> > + udelay(delay_usecs);
>
> Ouch. Do we really need to unconditionally delay?
> How about saving the time and using udelay only when required?
A delay before a subsequent SPI operation is required. The exact time
is specified by the SPI chip datasheet. The bookkeeping needed to
determine if the delay was met implicitly and thus avoid the explicit
delay would be complex and error prone. No one does it this way ---
an unconditional udelay is standard for spi controller drivers in the
kernel as far as I know.
Luckily, for most devices (this one included) delay_usecs is 0, so the
udelay doesn't actually delay.
> > +static int xapea00x_spi_probe(struct xapea00x_device *dev)
> > +{
> > + struct spi_master *spi_master;
> > + int retval;
> > +
> > + spi_master = spi_alloc_master(&dev->interface->dev, sizeof(void *));
> > + if (!spi_master) {
> > + retval = -ENOMEM;
> > + goto err_out;
> > + }
> > +
> > + spi_master_set_devdata(spi_master, dev);
> > +
> > + spi_master->min_speed_hz = 93 * 1000 + 800; /* 93.9kHz */
> > + spi_master->max_speed_hz = 12 * 1000 * 1000; /* 12 MHz */
> > +
> > + spi_master->bus_num = -1; /* dynamically assigned */
> > + spi_master->num_chipselect = 1;
> > + spi_master->mode_bits = SPI_MODE_0;
> > +
> > + spi_master->flags = 0;
> > + spi_master->setup = xapea00x_spi_setup;
> > + spi_master->cleanup = xapea00x_spi_cleanup;
> > + spi_master->transfer_one_message = xapea00x_spi_transfer_one_message;
> > +
> > + retval = spi_register_master(spi_master);
> > +
> > + if (retval)
> > + goto free_spi;
> > +
> > + dev->spi_master = spi_master;
>
> Looks like a race condition
>
I'm not seeing the possible race condition. Can you please elaborate?
> > + dev_dbg(&dev->interface->dev, "registered SPI master\n");
> > +
> > + return 0;
> > +
> > +free_spi:
> > + spi_master_put(spi_master);
> > + dev->spi_master = NULL;
> > +
> > +err_out:
> > + return retval;
> > +}
> > +static void xapea00x_tpm_probe(struct work_struct *work)
> > +{
> > + struct xapea00x_async_probe *probe = work_to_probe(work);
> > + struct xapea00x_device *dev = probe->dev;
> > + struct spi_master *spi_master = dev->spi_master;
> > + struct spi_device *tpm;
> > + int retval;
> > +
> > + tpm = spi_new_device(spi_master, &tpm_board_info);
> > + mutex_lock(&dev->usb_mutex);
> > + if (!dev->interface) {
> > + retval = -ENODEV;
> > + goto out;
> > + }
> > + if (!tpm) {
>
> Why test this under a mutex?
>
dev->interface being NULL/non-NULL is used as a flag to determine if
the USB device has been unregistered. So, the mutex needs to be held
when dereferencing dev->interface subsequently.
Performing the test before acquiring the lock would be a race condition.
> > + retval = -ENODEV;
> > + dev_err(&dev->interface->dev,
> > + "unable to add spi device for TPM\n");
> > + goto err;
> > + }
> > +
> > + dev->tpm = tpm;
> > + dev_info(&dev->interface->dev, "TPM initialization complete\n");
> > +
> > + retval = 0;
> > + goto out;
> > +
> > +err:
> > + dev_err(&dev->interface->dev,
> > + "TPM initialization failed with %d\n", retval);
> > +
> > +out:
> > + mutex_unlock(&dev->usb_mutex);
> > + xapea00x_cleanup_async_probe(probe);
> > + kzfree(probe);
> > +}
Thanks much,
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
