* [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®