usbatm : modprobe & rmmod

[Duncan Sands]

Hi Roman,

> While adjusting cxacru2 to the new API, I've come across a few points
> I'd like to clarify:
> 
> 1) ->bind, ->heavy_init, and ->unbind are all passed a struct
>    usbatm_data which already has a valid reference to the usb_intf.

Since the usbatm instance and the interface are the main bits of data
these methods will work on, it seemed polite to pass them both.  These
methods could also retrieve the usbatm_data from the interface (well OK,
they can't, but only because I put the data into the interface after the
call to bind returns), so you could also ask: why pass the usbatm_data?
Also, I'm still undecided as to whether usbatm_data should be "opaque":
which fields should be exposed to mini-drivers?  Should they be allowed
to extract usb_intf from the usbatm_data?  While waiting to get a better
feeling for this, I've chosen to pass in the interface also.  Another
reason is that I only added the usb_intf field recently - maybe I will
remove it later.  If methods used the passed parameter then they won't
need to change if the field is removed.  Anyway, as you can see this is
mostly a stylistic point, so doesn't matter much one way or the other.

>    Does it need to be present in the function parameters?
> 
> 2) what's the purpose of ->atm_stop?  I can't imagine needing this given
>    it is immediately followed by ->unbind.

It is only called if atm_start was called (and succeeded).  If heavy_init
fails or the modem is unplugged before heavy_init finishes, then atm_start
is not called, and then neither is atm_stop.  If heavy_init is not needed by
a driver, then atm_start and atm_stop are less useful, but still a little bit
useful: (A) the ATM device is only available when atm_start is called, not
when bind is called, so how to set up the device in bind? [*];  (B) the logical
separation of USB initialisation code and ATM initialisation code may make for
cleaner drivers.

> 3) usbatm_heavy_init is always called if ->heavy_init is available, even
>    if the modem is already initialized.  In this case creating a kthread
>    just to detect it and exit seems a bit of an overkill.  How about
>    exporting usbatm_heavy_init to be called by the minidriver in ->bind,
>    if needed?
>    Alternatively, ->heavy_init can be made to
>    take a parameter saying if we really want it heavy.  Then it's first
>    called synchronously in the light version, and if it fails with, say,
>    -ETRYAGAIN, spawn a kthread with the heavy version.

It is overkill, but who cares?  Well OK, I care a bit and I see you do too.
I will think about your suggestions.  Another possibility is to add a method
need_heavy_init which is called to determine whether heavy initialisation is
needed or not.

> 4) I have a couple of delays in cxacru_upload_firmware for the modem to
>    digest the commands I'm sending.  The delays are rather big:
>    msleep(1000) and msleep(4000).  How can I interrupt these, so that
>    kthread_stop in usbatm_disconnect doesn't wait too long?

msleep_interruptible

It is quite new.

Ciao,

Duncan.

[*] The code could be reorganised so that the ATM device is available in
bind if heavy_init does not exist.  But I would rather have this kind of
thing be the same whether or not heavy_init exists.