* [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
@ 2012-03-16 16:23 santosh nayak
2012-03-16 16:32 ` Oliver Neukum
0 siblings, 1 reply; 6+ messages in thread
From: santosh nayak @ 2012-03-16 16:23 UTC (permalink / raw)
To: mchehab
Cc: gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors, 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.
Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
drivers/staging/media/go7007/s2250-board.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/media/go7007/s2250-board.c b/drivers/staging/media/go7007/s2250-board.c
index 014d384..2823bbf 100644
--- a/drivers/staging/media/go7007/s2250-board.c
+++ b/drivers/staging/media/go7007/s2250-board.c
@@ -657,6 +657,10 @@ static int s2250_probe(struct i2c_client *client,
kfree(data);
}
mutex_unlock(&usb->i2c_lock);
+ } else {
+ i2c_unregister_device(audio);
+ kfree(state);
+ return -EINTR;
}
v4l2_info(sd, "initialized successfully\n");
--
1.7.4.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
2012-03-16 16:23 [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock santosh nayak
@ 2012-03-16 16:32 ` Oliver Neukum
2012-03-16 16:56 ` santosh prasad nayak
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2012-03-16 16:32 UTC (permalink / raw)
To: santosh nayak
Cc: mchehab, gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
2012-03-16 16:32 ` Oliver Neukum
@ 2012-03-16 16:56 ` santosh prasad nayak
2012-03-16 17:59 ` Oliver Neukum
0 siblings, 1 reply; 6+ messages in thread
From: santosh prasad nayak @ 2012-03-16 16:56 UTC (permalink / raw)
To: Oliver Neukum
Cc: mchehab, gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
2012-03-16 16:56 ` santosh prasad nayak
@ 2012-03-16 17:59 ` Oliver Neukum
2012-03-17 16:00 ` santosh prasad nayak
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2012-03-16 17:59 UTC (permalink / raw)
To: santosh prasad nayak
Cc: mchehab, gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
2012-03-16 17:59 ` Oliver Neukum
@ 2012-03-17 16:00 ` santosh prasad nayak
2012-03-17 16:20 ` Oliver Neukum
0 siblings, 1 reply; 6+ messages in thread
From: santosh prasad nayak @ 2012-03-17 16:00 UTC (permalink / raw)
To: Oliver Neukum
Cc: mchehab, gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock.
2012-03-17 16:00 ` santosh prasad nayak
@ 2012-03-17 16:20 ` Oliver Neukum
0 siblings, 0 replies; 6+ messages in thread
From: Oliver Neukum @ 2012-03-17 16:20 UTC (permalink / raw)
To: santosh prasad nayak
Cc: mchehab, gregkh, khoroshilov, linux-media, devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-17 16:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-16 16:23 [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock santosh nayak
2012-03-16 16:32 ` Oliver Neukum
2012-03-16 16:56 ` santosh prasad nayak
2012-03-16 17:59 ` Oliver Neukum
2012-03-17 16:00 ` santosh prasad nayak
2012-03-17 16:20 ` Oliver Neukum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®