Message ID | 1331915038-11231-1-git-send-email-santoshprasadnayak@gmail.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1S8ZyN-0000we-Fy for patchwork@linuxtv.org; Fri, 16 Mar 2012 17:25:47 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-3) with esmtp for <patchwork@linuxtv.org> id 1S8ZyM-0001GR-G1; Fri, 16 Mar 2012 17:25:47 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032014Ab2CPQZ3 (ORCPT <rfc822;patchwork@linuxtv.org>); Fri, 16 Mar 2012 12:25:29 -0400 Received: from mail-pz0-f46.google.com ([209.85.210.46]:40719 "EHLO mail-pz0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031393Ab2CPQZ2 (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 16 Mar 2012 12:25:28 -0400 Received: by dajr28 with SMTP id r28so6335538daj.19 for <multiple recipients>; Fri, 16 Mar 2012 09:25:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer; bh=j3pVtmnm+ibY6o4KgwdGuUQHOgXTcHnf+zemzJItSZQ=; b=XfmxzhpvG7ld03gVRjhisAdk7nZKUXCpwD+AHapI22iPvlC+M0mWJbknCHza0OmTHj hJE4mRK7kAUJDY151F+AtrDFhaeRotGDqjjHkAM/QXdSA6D6oaIUmdX/7cl43Gf+MOsT uo8l30suIohOMUeYY9C+TB8pF+6mQZ1EmuXMhwlsAQH2EH4wo3eKgbD58YyI4+Q1bZVV Q7RPR7LYaIByauTc6N4h7mTUY9LNvFIylBJU5/W6aIaDfJi0NWkwEJyGkeU5k42mVxut zYeF6JEpjiCpMdE3hrvsRUs49V9yY7BgcK0OAJy39mvxldceU4i3GoZKpQqtsGrJODqp Zjsw== Received: by 10.68.130.67 with SMTP id oc3mr16055025pbb.147.1331915128290; Fri, 16 Mar 2012 09:25:28 -0700 (PDT) Received: from localhost.localdomain ([14.97.198.41]) by mx.google.com with ESMTPS id 6sm4622432pbv.5.2012.03.16.09.25.22 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 16 Mar 2012 09:25:27 -0700 (PDT) From: santosh nayak <santoshprasadnayak@gmail.com> To: mchehab@infradead.org Cc: gregkh@linuxfoundation.org, khoroshilov@ispras.ru, linux-media@vger.kernel.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Santosh Nayak <santoshprasadnayak@gmail.com> Subject: [PATCH] [media] staging: Return -EINTR in s2250_probe() if fails to get lock. Date: Fri, 16 Mar 2012 21:53:58 +0530 Message-Id: <1331915038-11231-1-git-send-email-santoshprasadnayak@gmail.com> X-Mailer: git-send-email 1.7.4.4 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2012.3.16.161816 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' FORGED_FROM_GMAIL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1200_1299 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __FRAUD_BODY_WEBMAIL 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __FROM_GMAIL 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_STRUCTURE_1 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
santosh nayak
March 16, 2012, 4:23 p.m. UTC
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(-)
Comments
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
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");