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

[Andy Shevchenko]

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