[WATCHDOG] sa1100_wdt.c sparse cleanups

[WATCHDOG] sa1100_wdt.c sparse cleanups

[Ian Campbell]

The following makes drivers/char/watchdog/sa1100_wdt.c sparse clean.

Signed-off-by: Ian Campbell <icampbell@arcom.com>

Index: 2.6/drivers/char/watchdog/sa1100_wdt.c
===================================================================
--- 2.6.orig/drivers/char/watchdog/sa1100_wdt.c	2005-10-28 14:31:05.000000000 +0100
+++ 2.6/drivers/char/watchdog/sa1100_wdt.c	2005-10-28 14:31:16.000000000 +0100
@@ -74,7 +74,7 @@
 	return 0;
 }
 
-static ssize_t sa1100dog_write(struct file *file, const char *data, size_t len, loff_t *ppos)
+static ssize_t sa1100dog_write(struct file *file, const char __user *data, size_t len, loff_t *ppos)
 {
 	if (len)
 		/* Refresh OSMR3 timer. */
@@ -96,20 +96,20 @@
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info *)arg, &ident,
+		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
 				   sizeof(ident)) ? -EFAULT : 0;
 		break;
 
 	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int *)arg);
+		ret = put_user(0, (int __user *)arg);
 		break;
 
 	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int *)arg);
+		ret = put_user(boot_status, (int __user *)arg);
 		break;
 
 	case WDIOC_SETTIMEOUT:
-		ret = get_user(time, (int *)arg);
+		ret = get_user(time, (int __user *)arg);
 		if (ret)
 			break;
 
@@ -123,7 +123,7 @@
 		/*fall through*/
 
 	case WDIOC_GETTIMEOUT:
-		ret = put_user(pre_margin / OSCR_FREQ, (int *)arg);
+		ret = put_user(pre_margin / OSCR_FREQ, (int __user *)arg);
 		break;
 
 	case WDIOC_KEEPALIVE:

-- 
Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road, 			Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom	Phone:  +44 (0)1223 411 200


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been virus scanned by MessageLabs.

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Russell King]

On Wed, Nov 02, 2005 at 08:56:49AM +0000, Ian Campbell wrote:
> @@ -96,20 +96,20 @@
>  
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
> -		ret = copy_to_user((struct watchdog_info *)arg, &ident,
> +		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
>  				   sizeof(ident)) ? -EFAULT : 0;

It's probably better to use a union with these, eg:

	union {
		void __user *arg;
		struct watchdog_info __user *info;
		int __user *i;
	} u;

	u.arg = (void __user *)arg;

...

	ret = copy_to_user(u.info, &ident, sizeof(ident)) ? -EFAULT : 0;

etc


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Al Viro]

On Sat, Nov 05, 2005 at 10:10:27AM +0000, Russell King wrote:
> It's probably better to use a union with these, eg:
> 
> 	union {
> 		void __user *arg;
> 		struct watchdog_info __user *info;
> 		int __user *i;
> 	} u;
> 
> 	u.arg = (void __user *)arg;
> 
> ...
> 
> 	ret = copy_to_user(u.info, &ident, sizeof(ident)) ? -EFAULT : 0;

Just use void __user *.

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Russell King]

On Sat, Nov 05, 2005 at 02:51:34PM +0000, Al Viro wrote:
> On Sat, Nov 05, 2005 at 10:10:27AM +0000, Russell King wrote:
> > It's probably better to use a union with these, eg:
> > 
> > 	union {
> > 		void __user *arg;
> > 		struct watchdog_info __user *info;
> > 		int __user *i;
> > 	} u;
> > 
> > 	u.arg = (void __user *)arg;
> > 
> > ...
> > 
> > 	ret = copy_to_user(u.info, &ident, sizeof(ident)) ? -EFAULT : 0;
> 
> Just use void __user *.

That works for copy_to_user, but not for put_user() where the type of
the pointer matters.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Ian Campbell]

On Sat, 2005-11-05 at 10:10 +0000, Russell King wrote:

> It's probably better to use a union with these, eg:

The common idiom in the watchdog drivers seems to be to use separate
variables. I'll leave it up to Wim if he wants to change that.

The following makes drivers/char/watchdog/sa1100_wdt.c sparse clean.

Signed-off-by: Ian Campbell <icampbell@arcom.com>

Index: 2.6/drivers/char/watchdog/sa1100_wdt.c
===================================================================
--- 2.6.orig/drivers/char/watchdog/sa1100_wdt.c	2005-11-03 11:02:05.000000000 +0000
+++ 2.6/drivers/char/watchdog/sa1100_wdt.c	2005-11-07 09:51:47.000000000 +0000
@@ -74,7 +74,7 @@
 	return 0;
 }
 
-static ssize_t sa1100dog_write(struct file *file, const char *data, size_t len, loff_t *ppos)
+static ssize_t sa1100dog_write(struct file *file, const char __user *data, size_t len, loff_t *ppos)
 {
 	if (len)
 		/* Refresh OSMR3 timer. */
@@ -93,23 +93,24 @@
 {
 	int ret = -ENOIOCTLCMD;
 	int time;
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
-		ret = copy_to_user((struct watchdog_info *)arg, &ident,
-				   sizeof(ident)) ? -EFAULT : 0;
+		ret = copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
 		break;
 
 	case WDIOC_GETSTATUS:
-		ret = put_user(0, (int *)arg);
+		ret = put_user(0, p);
 		break;
 
 	case WDIOC_GETBOOTSTATUS:
-		ret = put_user(boot_status, (int *)arg);
+		ret = put_user(boot_status, p);
 		break;
 
 	case WDIOC_SETTIMEOUT:
-		ret = get_user(time, (int *)arg);
+		ret = get_user(time, p);
 		if (ret)
 			break;
 
@@ -123,7 +124,7 @@
 		/*fall through*/
 
 	case WDIOC_GETTIMEOUT:
-		ret = put_user(pre_margin / OSCR_FREQ, (int *)arg);
+		ret = put_user(pre_margin / OSCR_FREQ, p);
 		break;
 
 	case WDIOC_KEEPALIVE:

-- 
Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road, 			Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom	Phone:  +44 (0)1223 411 200


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been virus scanned by MessageLabs.

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Wim Van Sebroeck]

Hi Ian,

> On Sat, 2005-11-05 at 10:10 +0000, Russell King wrote:
> 
> > It's probably better to use a union with these, eg:
> 
> The common idiom in the watchdog drivers seems to be to use separate
> variables. I'll leave it up to Wim if he wants to change that.
> 
> The following makes drivers/char/watchdog/sa1100_wdt.c sparse clean.
> 
> Signed-off-by: Ian Campbell <icampbell@arcom.com>

I seem to have missed this last e-mail (I was moving around that time...).
This is indeed how it's been done in other drivers. I just uploaded this "patch"
into my -mm test tree. Within a week or two I'll move it to the final watchdog tree.

We should look to the struct watchdog part in more detail though.
a union is an option, but probably not the only one :-)

Greetings,
Wim.

Re: [WATCHDOG] sa1100_wdt.c sparse cleanups

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Tue, 2006-01-17 at 20:04 +0100, Wim Van Sebroeck wrote:
> Hi Ian,
> 
> > On Sat, 2005-11-05 at 10:10 +0000, Russell King wrote:
> > 
> > > It's probably better to use a union with these, eg:
> > 
> > The common idiom in the watchdog drivers seems to be to use separate
> > variables. I'll leave it up to Wim if he wants to change that.
> > 
> > The following makes drivers/char/watchdog/sa1100_wdt.c sparse clean.
> > 
> > Signed-off-by: Ian Campbell <icampbell@arcom.com>
> 
> I seem to have missed this last e-mail (I was moving around that time...).
> This is indeed how it's been done in other drivers. I just uploaded this "patch"
> into my -mm test tree. Within a week or two I'll move it to the final watchdog tree.
> 
> We should look to the struct watchdog part in more detail though.
> a union is an option, but probably not the only one :-)

Hi Wim,

Thanks for applying the patch.

BTW I've changed jobs since I sent it and I'm no longer working with ARM
hardware.

Ian.

-- 
Ian Campbell

Q:	Why do the police always travel in threes?
A:	One to do the reading, one to do the writing, and the other keeps
	an eye on the two intellectuals.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]