[media] staging: Return -EINTR in s2250_probe() if fails to get lock.

  From: Santosh Nayak <santoshprasadnayak@gmail.com>

In s2250_probe(), If locking attempt is interrupted by a signal then
it should return -EINTR after unregistering audio device and making free
the allocated memory.

At present, if locking is interrupted by signal it will display message
"initialized successfully" and return success.  This is wrong.

Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
 drivers/staging/media/go7007/s2250-board.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
  

Am Freitag, 16. März 2012, 17:23:58 schrieb santosh nayak:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> In s2250_probe(), If locking attempt is interrupted by a signal then
> it should return -EINTR after unregistering audio device and making free
> the allocated memory.
> 
> At present, if locking is interrupted by signal it will display message
> "initialized successfully" and return success.  This is wrong.

Indeed there's a lot wrong here. The idea of having an interruptible
sleep in probe() is arcane. You need a very, very, very good reason for that.
The sane fix is using an uninterruptable sleep here.

Second, while you are at it, fix the error case for no initialization
due to a failing kmalloc(). You need to return -ENOMEM.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Mar 16, 2012 at 10:02 PM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Freitag, 16. März 2012, 17:23:58 schrieb santosh nayak:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> In s2250_probe(), If locking attempt is interrupted by a signal then
>> it should return -EINTR after unregistering audio device and making free
>> the allocated memory.
>>
>> At present, if locking is interrupted by signal it will display message
>> "initialized successfully" and return success.  This is wrong.
>
> Indeed there's a lot wrong here. The idea of having an interruptible
> sleep in probe() is arcane. You need a very, very, very good reason for that.

Can you please explain why interruptible  sleep  should not be  in probe() ?
I am curious to know.


> The sane fix is using an uninterruptable sleep here.
>
> Second, while you are at it, fix the error case for no initialization
> due to a failing kmalloc(). You need to return -ENOMEM.

Are you talking about kmalloc or kzalloc ?
Because for failing kmalloc -ENOMEM is returned as shown below:

       state = kmalloc(sizeof(struct s2250), GFP_KERNEL);
        if (state == NULL) {
                i2c_unregister_device(audio);
                return -ENOMEM;     // ENOMEM is returned here.
        }


Regards
Santosh


>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Am Freitag, 16. März 2012, 17:56:20 schrieb santosh prasad nayak:
> On Fri, Mar 16, 2012 at 10:02 PM, Oliver Neukum <oliver@neukum.org> wrote:
> >
> > Indeed there's a lot wrong here. The idea of having an interruptible
> > sleep in probe() is arcane. You need a very, very, very good reason for that.
> 
> Can you please explain why interruptible  sleep  should not be  in probe() ?
> I am curious to know.

-EINTR is supposed to be returned to user space, so that it can repeat
an interrupted syscall.

- There is no user space for probe()
- probe() cannot be easily repeated from user space
- there is no syscall for probe
> 
> 
> > The sane fix is using an uninterruptable sleep here.
> >
> > Second, while you are at it, fix the error case for no initialization
> > due to a failing kmalloc(). You need to return -ENOMEM.
> 
> Are you talking about kmalloc or kzalloc ?
> Because for failing kmalloc -ENOMEM is returned as shown below:

                data = kzalloc(16, GFP_KERNEL);
                if (data != NULL) {
                        int rc;
                        rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
                                                       data, 16, 1);
                        if (rc > 0) {
                                u8 mask;
                                data[0] = 0;
                                mask = 1<<5;
                                data[0] &= ~mask;
                                data[1] |= mask;
                                go7007_usb_vendor_request(go, 0x40, 0,
                                                          (data[1]<<8)
                                                          + data[1],
                                                          data, 16, 0);
                        }
                        kfree(data);
                }
                mutex_unlock(&usb->i2c_lock)

This code has no error handling.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Oliver,

The following changes are for review only  not a formal patch.

-------------------------------------------------------------------------------------------------------------
-       if (mutex_lock_interruptible(&usb->i2c_lock) == 0) {
+       mutex_lock(&usb->i2c_lock);
                data = kzalloc(16, GFP_KERNEL);
-               if (data != NULL) {
+               if(data == NULL) {
+                       i2c_unregister_device(audio);
+                       kfree(state);
+                       return -ENOMEM;
+               } else {
                        int rc;
                        rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
                                                       data, 16, 1);
@@ -657,7 +661,7 @@ static int s2250_probe(struct i2c_client *client,
                        kfree(data);
                }
                mutex_unlock(&usb->i2c_lock);
-       }
+

----------------------------------------------------------------------


Is it ok ?


regards
Santosh


On Fri, Mar 16, 2012 at 11:29 PM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Freitag, 16. März 2012, 17:56:20 schrieb santosh prasad nayak:
>> On Fri, Mar 16, 2012 at 10:02 PM, Oliver Neukum <oliver@neukum.org> wrote:
>> >
>> > Indeed there's a lot wrong here. The idea of having an interruptible
>> > sleep in probe() is arcane. You need a very, very, very good reason for that.
>>
>> Can you please explain why interruptible  sleep  should not be  in probe() ?
>> I am curious to know.
>
> -EINTR is supposed to be returned to user space, so that it can repeat
> an interrupted syscall.
>
> - There is no user space for probe()
> - probe() cannot be easily repeated from user space
> - there is no syscall for probe
>>
>>
>> > The sane fix is using an uninterruptable sleep here.
>> >
>> > Second, while you are at it, fix the error case for no initialization
>> > due to a failing kmalloc(). You need to return -ENOMEM.
>>
>> Are you talking about kmalloc or kzalloc ?
>> Because for failing kmalloc -ENOMEM is returned as shown below:
>
>                data = kzalloc(16, GFP_KERNEL);
>                if (data != NULL) {
>                        int rc;
>                        rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
>                                                       data, 16, 1);
>                        if (rc > 0) {
>                                u8 mask;
>                                data[0] = 0;
>                                mask = 1<<5;
>                                data[0] &= ~mask;
>                                data[1] |= mask;
>                                go7007_usb_vendor_request(go, 0x40, 0,
>                                                          (data[1]<<8)
>                                                          + data[1],
>                                                          data, 16, 0);
>                        }
>                        kfree(data);
>                }
>                mutex_unlock(&usb->i2c_lock)
>
> This code has no error handling.
>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Am Samstag, 17. März 2012, 17:00:36 schrieb santosh prasad nayak:
> Oliver,
> 
> The following changes are for review only  not a formal patch.
> 
> -------------------------------------------------------------------------------------------------------------
> -       if (mutex_lock_interruptible(&usb->i2c_lock) == 0) {
> +       mutex_lock(&usb->i2c_lock);
>                 data = kzalloc(16, GFP_KERNEL);
> -               if (data != NULL) {
> +               if(data == NULL) {
> +                       i2c_unregister_device(audio);
> +                       kfree(state);
> +                       return -ENOMEM;
> +               } else {
>                         int rc;
>                         rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
>                                                        data, 16, 1);
> @@ -657,7 +661,7 @@ static int s2250_probe(struct i2c_client *client,
>                         kfree(data);
>                 }
>                 mutex_unlock(&usb->i2c_lock);
> -       }
> +
> 
> ----------------------------------------------------------------------
> 
> 
> Is it ok ?

Hi,

well done. That's correct.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html