LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
@ 2008-01-14 13:32 Mathieu Segaud
  2008-01-14 14:18 ` Matthew Wilcox
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Segaud @ 2008-01-14 13:32 UTC (permalink / raw)
  To: James.Bottomley; +Cc: linux-scsi, linux-kernel, Mathieu Segaud

As of now, compat_ioctl already runs without the BKL, whereas ioctl runs
with the BKL. This patch first converts changer_fops to use a .unlocked_ioctl
member. It applies the same locking rationale than ch_ioctl_compat() uses
to ch_ioctl().

Signed-off-by: Mathieu Segaud <mathieu.segaud@regala.cx>
---
 drivers/scsi/ch.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2311019..b8e005d 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -21,6 +21,7 @@
 #include <linux/compat.h>
 #include <linux/chio.h>			/* here are all the ioctls */
 #include <linux/mutex.h>
+#include <linux/smp_lock.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -92,8 +93,7 @@ static int  ch_probe(struct device *);
 static int  ch_remove(struct device *);
 static int  ch_open(struct inode * inode, struct file * filp);
 static int  ch_release(struct inode * inode, struct file * filp);
-static int  ch_ioctl(struct inode * inode, struct file * filp,
-		     unsigned int cmd, unsigned long arg);
+static long ch_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 #ifdef CONFIG_COMPAT
 static long ch_ioctl_compat(struct file * filp,
 			    unsigned int cmd, unsigned long arg);
@@ -130,12 +130,12 @@ static struct scsi_driver ch_template =
 
 static const struct file_operations changer_fops =
 {
-	.owner        = THIS_MODULE,
-	.open         = ch_open,
-	.release      = ch_release,
-	.ioctl        = ch_ioctl,
+	.owner          = THIS_MODULE,
+	.open           = ch_open,
+	.release        = ch_release,
+	.unlocked_ioctl = ch_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = ch_ioctl_compat,
+	.compat_ioctl   = ch_ioctl_compat,
 #endif
 };
 
@@ -626,7 +626,7 @@ ch_checkrange(scsi_changer *ch, unsigned int type, unsigned int unit)
 	return 0;
 }
 
-static int ch_ioctl(struct inode * inode, struct file * file,
+static long ch_ioctl(struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
 	scsi_changer *ch = file->private_data;
@@ -887,8 +887,7 @@ static long ch_ioctl_compat(struct file * file,
 	case CHIOINITELEM:
 	case CHIOSVOLTAG:
 		/* compatible */
-		return ch_ioctl(NULL /* inode, unused */,
-				file, cmd, arg);
+		return ch_ioctl(file, cmd, arg);
 	case CHIOGSTATUS32:
 	{
 		struct changer_element_status32 ces32;
-- 
1.5.3.8


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
  2008-01-14 13:32 [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl Mathieu Segaud
@ 2008-01-14 14:18 ` Matthew Wilcox
  2008-01-14 14:22   ` Mathieu SEGAUD
  0 siblings, 1 reply; 7+ messages in thread
From: Matthew Wilcox @ 2008-01-14 14:18 UTC (permalink / raw)
  To: Mathieu Segaud; +Cc: James.Bottomley, linux-scsi, linux-kernel

On Mon, Jan 14, 2008 at 02:32:13PM +0100, Mathieu Segaud wrote:
> +#include <linux/smp_lock.h>

You don't add any uses of lock_kernel() and there are none in the
driver currently.

> -	.owner        = THIS_MODULE,
> -	.open         = ch_open,
> -	.release      = ch_release,
> -	.ioctl        = ch_ioctl,
> +	.owner          = THIS_MODULE,
> +	.open           = ch_open,
> +	.release        = ch_release,
> +	.unlocked_ioctl = ch_ioctl,

If you're going to do the gratuitous reformatting, at least use tabs
instead of spaces.

Other than that, should be fine.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
  2008-01-14 14:18 ` Matthew Wilcox
@ 2008-01-14 14:22   ` Mathieu SEGAUD
  0 siblings, 0 replies; 7+ messages in thread
From: Mathieu SEGAUD @ 2008-01-14 14:22 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: James.Bottomley, linux-scsi, linux-kernel

Vous m'avez dit récemment :

> On Mon, Jan 14, 2008 at 02:32:13PM +0100, Mathieu Segaud wrote:
>> +#include <linux/smp_lock.h>
> You don't add any uses of lock_kernel() and there are none in the
> driver currently.

yep, it was before I noticed the locking semantics of
ch_ioctl_compat()

>> -	.owner        = THIS_MODULE,
>> -	.open         = ch_open,
>> -	.release      = ch_release,
>> -	.ioctl        = ch_ioctl,
>> +	.owner          = THIS_MODULE,
>> +	.open           = ch_open,
>> +	.release        = ch_release,
>> +	.unlocked_ioctl = ch_ioctl,
>
> If you're going to do the gratuitous reformatting, at least use tabs
> instead of spaces.

thanks, will do

> Other than that, should be fine.

I repost this one
thanks a lot.

-- 
Mathieu


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
@ 2008-01-14 15:21 Mathieu Segaud
  0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Segaud @ 2008-01-14 15:21 UTC (permalink / raw)
  To: linux-scsi; +Cc: linux-kernel, akpm, Mathieu Segaud

As of now, compat_ioctl already runs without the BKL, whereas ioctl runs
with the BKL. This patch first converts changer_fops to use a .unlocked_ioctl
member. It applies the same locking rationale than ch_ioctl_compat() uses
to ch_ioctl().

Signed-off-by: Mathieu Segaud <mathieu.segaud@regala.cx>
Reviewed-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/scsi/ch.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2311019..cead0f5 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -92,8 +92,7 @@ static int  ch_probe(struct device *);
 static int  ch_remove(struct device *);
 static int  ch_open(struct inode * inode, struct file * filp);
 static int  ch_release(struct inode * inode, struct file * filp);
-static int  ch_ioctl(struct inode * inode, struct file * filp,
-		     unsigned int cmd, unsigned long arg);
+static long ch_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 #ifdef CONFIG_COMPAT
 static long ch_ioctl_compat(struct file * filp,
 			    unsigned int cmd, unsigned long arg);
@@ -130,12 +129,12 @@ static struct scsi_driver ch_template =
 
 static const struct file_operations changer_fops =
 {
-	.owner        = THIS_MODULE,
-	.open         = ch_open,
-	.release      = ch_release,
-	.ioctl        = ch_ioctl,
+	.owner		= THIS_MODULE,
+	.open		= ch_open,
+	.release	= ch_release,
+	.unlocked_ioctl	= ch_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = ch_ioctl_compat,
+	.compat_ioctl	= ch_ioctl_compat,
 #endif
 };
 
@@ -626,7 +625,7 @@ ch_checkrange(scsi_changer *ch, unsigned int type, unsigned int unit)
 	return 0;
 }
 
-static int ch_ioctl(struct inode * inode, struct file * file,
+static long ch_ioctl(struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
 	scsi_changer *ch = file->private_data;
@@ -887,8 +886,7 @@ static long ch_ioctl_compat(struct file * file,
 	case CHIOINITELEM:
 	case CHIOSVOLTAG:
 		/* compatible */
-		return ch_ioctl(NULL /* inode, unused */,
-				file, cmd, arg);
+		return ch_ioctl(file, cmd, arg);
 	case CHIOGSTATUS32:
 	{
 		struct changer_element_status32 ces32;
-- 
1.5.3.8


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
@ 2008-01-14 14:43 Mathieu Segaud
  0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Segaud @ 2008-01-14 14:43 UTC (permalink / raw)
  To: linux-scsi; +Cc: linux-kernel, Mathieu Segaud

As of now, compat_ioctl already runs without the BKL, whereas ioctl runs
with the BKL. This patch first converts changer_fops to use a .unlocked_ioctl
member. It applies the same locking rationale than ch_ioctl_compat() uses
to ch_ioctl().

Signed-off-by: Mathieu Segaud <mathieu.segaud@regala.cx>
Reviewed-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/scsi/ch.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2311019..cead0f5 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -92,8 +92,7 @@ static int  ch_probe(struct device *);
 static int  ch_remove(struct device *);
 static int  ch_open(struct inode * inode, struct file * filp);
 static int  ch_release(struct inode * inode, struct file * filp);
-static int  ch_ioctl(struct inode * inode, struct file * filp,
-		     unsigned int cmd, unsigned long arg);
+static long ch_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 #ifdef CONFIG_COMPAT
 static long ch_ioctl_compat(struct file * filp,
 			    unsigned int cmd, unsigned long arg);
@@ -130,12 +129,12 @@ static struct scsi_driver ch_template =
 
 static const struct file_operations changer_fops =
 {
-	.owner        = THIS_MODULE,
-	.open         = ch_open,
-	.release      = ch_release,
-	.ioctl        = ch_ioctl,
+	.owner		= THIS_MODULE,
+	.open		= ch_open,
+	.release	= ch_release,
+	.unlocked_ioctl	= ch_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = ch_ioctl_compat,
+	.compat_ioctl	= ch_ioctl_compat,
 #endif
 };
 
@@ -626,7 +625,7 @@ ch_checkrange(scsi_changer *ch, unsigned int type, unsigned int unit)
 	return 0;
 }
 
-static int ch_ioctl(struct inode * inode, struct file * file,
+static long ch_ioctl(struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
 	scsi_changer *ch = file->private_data;
@@ -887,8 +886,7 @@ static long ch_ioctl_compat(struct file * file,
 	case CHIOINITELEM:
 	case CHIOSVOLTAG:
 		/* compatible */
-		return ch_ioctl(NULL /* inode, unused */,
-				file, cmd, arg);
+		return ch_ioctl(file, cmd, arg);
 	case CHIOGSTATUS32:
 	{
 		struct changer_element_status32 ces32;
-- 
1.5.3.8


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
  2008-01-14 14:31 Mathieu Segaud
@ 2008-01-14 14:37 ` Matthew Wilcox
  0 siblings, 0 replies; 7+ messages in thread
From: Matthew Wilcox @ 2008-01-14 14:37 UTC (permalink / raw)
  To: Mathieu Segaud; +Cc: linux-scsi, linux-kernel

On Mon, Jan 14, 2008 at 03:31:17PM +0100, Mathieu Segaud wrote:
> +	.owner			= THIS_MODULE,
> +	.open			= ch_open,
> +	.release		= ch_release,
> +	.unlocked_ioctl	= ch_ioctl,

Do you have a tab setting that's not 8?  You seem to have used one tab
too many for the first three lines.

Please correct that one, and then add my:

Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

Thanks!

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl
@ 2008-01-14 14:31 Mathieu Segaud
  2008-01-14 14:37 ` Matthew Wilcox
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Segaud @ 2008-01-14 14:31 UTC (permalink / raw)
  To: linux-scsi; +Cc: linux-kernel, Mathieu Segaud

As of now, compat_ioctl already runs without the BKL, whereas ioctl runs
with the BKL. This patch first converts changer_fops to use a .unlocked_ioctl
member. It applies the same locking rationale than ch_ioctl_compat() uses
to ch_ioctl().

Signed-off-by: Mathieu Segaud <mathieu.segaud@regala.cx>
---
 drivers/scsi/ch.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 2311019..389777c 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -92,8 +92,7 @@ static int  ch_probe(struct device *);
 static int  ch_remove(struct device *);
 static int  ch_open(struct inode * inode, struct file * filp);
 static int  ch_release(struct inode * inode, struct file * filp);
-static int  ch_ioctl(struct inode * inode, struct file * filp,
-		     unsigned int cmd, unsigned long arg);
+static long ch_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 #ifdef CONFIG_COMPAT
 static long ch_ioctl_compat(struct file * filp,
 			    unsigned int cmd, unsigned long arg);
@@ -130,12 +129,12 @@ static struct scsi_driver ch_template =
 
 static const struct file_operations changer_fops =
 {
-	.owner        = THIS_MODULE,
-	.open         = ch_open,
-	.release      = ch_release,
-	.ioctl        = ch_ioctl,
+	.owner			= THIS_MODULE,
+	.open			= ch_open,
+	.release		= ch_release,
+	.unlocked_ioctl	= ch_ioctl,
 #ifdef CONFIG_COMPAT
-	.compat_ioctl = ch_ioctl_compat,
+	.compat_ioctl	= ch_ioctl_compat,
 #endif
 };
 
@@ -626,7 +625,7 @@ ch_checkrange(scsi_changer *ch, unsigned int type, unsigned int unit)
 	return 0;
 }
 
-static int ch_ioctl(struct inode * inode, struct file * file,
+static long ch_ioctl(struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
 	scsi_changer *ch = file->private_data;
@@ -887,8 +886,7 @@ static long ch_ioctl_compat(struct file * file,
 	case CHIOINITELEM:
 	case CHIOSVOLTAG:
 		/* compatible */
-		return ch_ioctl(NULL /* inode, unused */,
-				file, cmd, arg);
+		return ch_ioctl(file, cmd, arg);
 	case CHIOGSTATUS32:
 	{
 		struct changer_element_status32 ces32;
-- 
1.5.3.8


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-01-14 15:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-14 13:32 [PATCH] Convert drivers/scsi/ch.c to use unlocked_ioctl Mathieu Segaud
2008-01-14 14:18 ` Matthew Wilcox
2008-01-14 14:22   ` Mathieu SEGAUD
2008-01-14 14:31 Mathieu Segaud
2008-01-14 14:37 ` Matthew Wilcox
2008-01-14 14:43 Mathieu Segaud
2008-01-14 15:21 Mathieu Segaud

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).