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

[Greg Kroah-Hartman]

On Wed, May 30, 2018 at 01:24:59PM +0200, Marcus Folkesson wrote:
> 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.

When your module is removed, you need to clean up any remaining memory
that the ida used.  It's not obvious at all, and is a pain as you would
think that if you statically allocate one, like you have here, it would
not be needed.  You need to just call:
	ida_destroy(&ccidg_ida);
in your module exit function.

Hope this helps,

greg k-h