Re: [cdi-devel] [PATCH v3 10/10] usb-storage: Add USB mass storage driver

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Mon, May 11, 2015 at 09:59:30PM +0200, Max Reitz wrote:
> + This adds a USB mass storage device driver. It is missing proper error
>   handling (e.g. for STALLed endpoints) and support for any USB MSD
>   class other than bulk-only (e.g. control-bulk-interrupt for USB floppy
>   drives).
> 
> Signed-off-by: Max Reitz <max@xxxxxxxxxx>

> +struct cdi_device *init_usb_device(struct cdi_bus_data *bus_data)
> +{
> +    static int dev_counter = 0;
> +    usb_mass_storage_device_t *dev = calloc(1, sizeof(*dev));
> +    dev->usb = CDI_UPCAST(bus_data, struct cdi_usb_device, bus_data);

I think *dev->usb needs to be copied instead of just taking a pointer
because CDI doesn't guarantee that the bus_data object isn't freed after
the function returns.

> +
> +    if ((dev->usb->subclass_id == USBMS_SUBCLS_SCSILEG ||
> +         dev->usb->subclass_id == USBMS_SUBCLS_SCSI) &&
> +        dev->usb->protocol_id == USBMS_PROTO_BBB)
> +    {
> +        dev->type = USBMS_BULK_ONLY;
> +    } else {
> +        free(dev);
> +        return NULL;
> +    }
> +
> +    if (dev->type == USBMS_BULK_ONLY) {
> +        if (bulk_only_reset(dev) < 0) {
> +            free(dev);
> +            return NULL;
> +        }
> +
> +        struct cdi_usb_endpoint_descriptor ep_desc;
> +        for (int ep = 1; ep < dev->usb->endpoint_count; ep++) {
> +            cdi_usb_get_endpoint_descriptor(dev->usb, ep, &ep_desc);
> +
> +            if ((ep_desc.bm_attributes & CDI_USB_EPDSC_ATTR_XFER_TYPE_MASK)
> +                == CDI_USB_EPDSC_ATTR_BULK)
> +            {
> +                bool is_in;
> +                is_in = ep_desc.b_endpoint_address & CDI_USB_EPDSC_EPADR_DIR;
> +                if (!dev->bulk_in && is_in) {
> +                    dev->bulk_in = ep;
> +                } else if (dev->bulk_in && !is_in) {

Are you sure you didn't want to check !dev->bulk_out && !is_in?

> +                    dev->bulk_out = ep;
> +                }
> +            }
> +        }
> +
> +        if (!dev->bulk_in || !dev->bulk_out) {
> +            free(dev);
> +            return NULL;
> +        }
> +    }
> +
> +    dev->dev.dev.name = malloc(5);
> +    sprintf((char *)dev->dev.dev.name, "msd%c", 'a' + dev_counter++);

Numbers instead of letters would be more consistent with the existing
block drivers.

> +    dev->dev.type = CDI_STORAGE;
> +    dev->dev.lun_count = dev->luns;
> +
> +    return &dev->dev.dev;
> +}

Rest looks good to me (whatever that means).

Kevin