Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device

From: Andy Shevchenko
Date: Tue May 29 2018 - 20:55:50 EST


On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
<marcus.folkesson@xxxxxxxxx> wrote:
> Chip Card Interface Device (CCID) protocol is a USB protocol that
> allows a smartcard device to be connected to a computer via a card
> reader using a standard USB interface, without the need for each manufacturer
> of smartcards to provide its own reader or protocol.
>
> This gadget driver makes Linux show up as a CCID device to the host and let a
> userspace daemon act as the smartcard.
>
> This is useful when the Linux gadget itself should act as a cryptographic
> device or forward APDUs to an embedded smartcard device.

> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@xxxxxxxxx>

> + *

Redundant line

> +static DEFINE_IDA(ccidg_ida);

Where is it destroyed?

> + ccidg_class = NULL;
> + return PTR_ERR(ccidg_class);

Are you sure?

> + if (!list_empty(head)) {
> + req = list_first_entry(head, struct usb_request, list);

list_first_entry_or_null()

> + req->length = len;

Perhaps assign this obly if malloc successedeed ?

> + req->buf = kmalloc(len, GFP_ATOMIC);

> + if (req->buf == NULL) {

if (!req->buf) ?

> + usb_ep_free_request(ep, req);
> + return ERR_PTR(-ENOMEM);
> + }

> +static void ccidg_request_free(struct usb_request *req, struct usb_ep *ep)
> +{

> + if (req) {

Is it even possible?

What about

if (!req)
return;

?

> + kfree(req->buf);
> + usb_ep_free_request(ep, req);
> + }
> +}

> + *(__le32 *) req->buf = ccid_class_desc.dwDefaultClock;

Hmm... put_unaligned()? cpu_to_le32()? cpu_to_le32p()?

> + *(__le32 *) req->buf = ccid_class_desc.dwDataRate;

Ditto.

> + }
> + }

Indentation.

> + /* responded with data transfer or status phase? */
> + if (ret >= 0) {

Why not

if (ret < 0)
return ret;

?

> + }
> +
> + return ret;
> +}

> + atomic_set(&ccidg->online, 1);
> + return ret;

return 0; ?

> + struct f_ccidg *ccidg;

> + ccidg = container_of(inode->i_cdev, struct f_ccidg, cdev);

One line ?

> + xfer = (req->actual < count) ? req->actual : count;

min_t()

> + ret = wait_event_interruptible(bulk_dev->write_wq,
> + ((req = ccidg_req_get(ccidg, &bulk_dev->tx_idle))));
> +
> + if (ret < 0)
> + return -ERESTARTSYS;

Redundant blank line above.

> +static void ccidg_function_free(struct usb_function *f)
> +{

> + struct f_ccidg_opts *opts;

> + opts = container_of(f->fi, struct f_ccidg_opts, func_inst);

One line.


> + mutex_lock(&opts->lock);
> + --opts->refcnt;

-- will work

> + mutex_unlock(&opts->lock);
> +}

> + struct f_ccidg_opts *opts;

> + opts = container_of(fi, struct f_ccidg_opts, func_inst);

Perhaps one line ?
> + ++opts->refcnt;
X++ would work as well.
> + struct f_ccidg_opts *opts;
> +
> + opts = container_of(f, struct f_ccidg_opts, func_inst);

Perhaps one line?

> +#define CCID_PINSUPOORT_NONE 0x00

(0 << 0)

?

for sake of consistency

> +#define CCID_PINSUPOORT_VERIFICATION (1 << 1)
> +#define CCID_PINSUPOORT_MODIFICATION (1 << 2)

--
With Best Regards,
Andy Shevchenko