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

[Marcus Folkesson]

Hi Andy,

Thank you for your comments!
Many good catches here!

On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> 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
> 

Yep

> > +static DEFINE_IDA(ccidg_ida);
> 
> Where is it destroyed?

Hm, I'm not sure it needs to be destroyed. From lib/idr.c:

 * You can also use ida_get_new_above() if you need an ID to be allocated
 * above a particular number.  ida_destroy() can be used to dispose of an
 * IDA without needing to free the individual IDs in it.  You can use
 * ida_is_empty() to find out whether the IDA has any IDs currently allocated.


An empty ccidg_ida is the indication that we should clean up our
mess:

static void ccidg_free_inst(struct usb_function_instance *f)
...
	if (ida_is_empty(&ccidg_ida))
		ccidg_cleanup();

If the IDA is empty, should I call ida_destroy() anyway?
Other similiar drivers does not seems to do that.

I must say that I'm not very familiar with the IDA API.

> 
> > +               ccidg_class = NULL;
> > +               return PTR_ERR(ccidg_class);
> 
> Are you sure?

Heh, :-)

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

Will do, thanks.

> 
> > +       req->length = len;
> 
> Perhaps assign this obly if malloc successedeed ?

Will do.

> 
> > +       req->buf = kmalloc(len, GFP_ATOMIC);
> 
> > +       if (req->buf == NULL) {
> 
> if (!req->buf) ?

Will do

> 
> > +               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;
> 
> ?

Hmm, maybe it is not.
I think I remove this check.

> 
> > +               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()?
> 

Hmm, dwDefaultClock is already represented in little endian format. If I
use any of those functions, would not the byte order be swaped on a big
endian system?

dwDefaultClock is set as:
	.dwDefaultClock =	cpu_to_le32(3580),

I'm not sure what the best practice is here.

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

I remove the extra brackets from the case instead.
> 
> > +       /* responded with data transfer or status phase? */
> > +       if (ret >= 0) {
> 
> Why not
> 
> if (ret < 0)
>  return ret;
> 
> ?
> 

Will do

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

Will do

> 
> > +       struct f_ccidg *ccidg;
> 
> > +       ccidg = container_of(inode->i_cdev, struct f_ccidg, cdev);
> 
> One line ?

The line exceeds 80 characters then, but I will put it like this:

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

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

Thanks, will do

> > +       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.
> 

I remove the extra blank line


> > +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.

See above

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

Will do

> 
> > +       mutex_unlock(&opts->lock);
> > +}
> 
> > +       struct f_ccidg_opts *opts;
> 
> > +       opts = container_of(fi, struct f_ccidg_opts, func_inst);
> 
> Perhaps one line ?

See above


> > +       ++opts->refcnt;
> X++ would work as well.

Will do

> > +       struct f_ccidg_opts *opts;
> > +
> > +       opts = container_of(f, struct f_ccidg_opts, func_inst);
> 
> Perhaps one line?
> 

See above


> > +#define CCID_PINSUPOORT_NONE           0x00
> 
> (0 << 0)
> 
>  ?
> 
> for sake of consistency

Yep, will change

> 
> > +#define CCID_PINSUPOORT_VERIFICATION   (1 << 1)
> > +#define CCID_PINSUPOORT_MODIFICATION   (1 << 2)
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Best regards,
Marcus Folkesson