Re: [PATCH v0] USB: storage: karma: fix rio_karma_init return

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alan,

> Not exactly.  rio_karma_init() is a usb-storage initFunction (see 
> the usb_stor_acquire_resources() routine in usb.c), and these functions 
> are supposed to return either 0 or a negative error code.
> 
> So you should make the routine return -ENOMEM, not 
> USB_STOR_TRANSPORT_ERROR.  You can simplify the patch by changing the 
> line where ret is defined; initialize it to -ENOMEM rather than to 0

Thanks for pointing out that, and as I dig deeper, I found that the use of error code
is "totally a mess" in the usb storage.

Taking the initfunction for example, there are below 6 cases
1 karam: rio_karma_init()
2 realtek_cr: init_realtek_cr()
3 aluda: init_alauda()
4 isd200: isd200_init_info()
5 shuttle_usbat: init_usbat()
6 onetouch: onetouch_connect_input()

The (1) is erroneously return 0 even when the kzalloc if failed, must be fixed.
The (2) and (6) uses -ENOMEM when allocation fails. (2) will also return -EIO when
another kzalloc fails or rts51x_check_status() fails.
The (3) uses USB_STOR_TRANSPORT_ERROR when allocation fails (seems that I learned a
mistake from here).
The (4) uses custom ISD200_ERROR when allocation fails.
The (5) uses constant 1 when allocation fails.

It's worth pointed out that (except (1)), the others though not following the standard,
it won't cause bad thing because the callsite of these initFunction just check whether the
return is zero.

/*
 * Just before we start our control thread, initialize
 * the device if it needs initialization
 */
 if (us->unusual_dev->initFunction) {
     p = us->unusual_dev->initFunction(us);
     if (p)
         return p;
 }

I will then send patches to make sure all initFunction follow the standard.

> 
> And don't forget the error code for when the rio_karma_send_command() 
> call fails.  In that case the return value should be -EIO.
> 

Okay, will add this in next version of this patch.

> 
> Shouldn't this also be marked for the stable kernels?
> 

Sorry, I didn't get it, do you mean add another tag like "Cc: stable@xxxxxxxxxxxxxxx"?
Or I just need to CC the mail to stable mail list?

Regards
Lin




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux