Skip to contentSkip to conversation

watchdog: ts72xx_wdt: cleanup return codes in ioctl

PATCH In review Dan Carpenter 3 participants 3 messages Aug 23, 2013 – Oct 29, 2013

Subsystems (2)
Kernel janitors Watchdog device drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 watchdog: ts72xx_wdt: cleanup return codes in ioctl

There seems to be some confusion here which functions return positive numbers and which return negative error codes.

copy_to_user() returns the number of bytes remaining to be copied but we want to return -EFAULT.

The rest is just clean up. get_user() actually returns zero on success and -EFAULT on error so we can preserve the error code. The timeout_to_regval() function returns -EINVAL on failure, but we can propogate that back instead of hardcoding -EINVAL ourselves.

Signed-off-by Dan Carpenter <[email protected]>
drivers/watchdog/ts72xx_wdt.c +16 -20
index 4da59b4..9125b0f 100644
@@ -305,7 +305,8 @@ static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
-		error = copy_to_user(argp, &winfo, sizeof(winfo));
+		if (copy_to_user(argp, &winfo, sizeof(winfo)))
+			error = -EFAULT;
 		break;
 
 	case WDIOC_GETSTATUS:

@@ -319,10 +320,9 @@ static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
 	case WDIOC_SETOPTIONS: {
 		int options;
 
-		if (get_user(options, p)) {
-			error = -EFAULT;
+		error = get_user(options, p);
+		if (error)
 			break;
-		}
 
 		error = -EINVAL;
 

@@ -340,30 +340,26 @@ static long ts72xx_wdt_ioctl(struct file *file, unsigned int cmd,
 
 	case WDIOC_SETTIMEOUT: {
 		int new_timeout;
+		int regval;
 
-		if (get_user(new_timeout, p)) {
-			error = -EFAULT;
-		} else {
-			int regval;
-
-			regval = timeout_to_regval(new_timeout);
-			if (regval < 0) {
-				error = -EINVAL;
-			} else {
-				ts72xx_wdt_stop(wdt);
-				wdt->regval = regval;
-				ts72xx_wdt_start(wdt);
-			}
-		}
+		error = get_user(new_timeout, p);
 		if (error)
 			break;
 
+		regval = timeout_to_regval(new_timeout);
+		if (regval < 0) {
+			error = regval;
+			break;
+		}
+		ts72xx_wdt_stop(wdt);
+		wdt->regval = regval;
+		ts72xx_wdt_start(wdt);
+
 		/*FALLTHROUGH*/
 	}
 
 	case WDIOC_GETTIMEOUT:
-		if (put_user(regval_to_timeout(wdt->regval), p))
-			error = -EFAULT;
+		error = put_user(regval_to_timeout(wdt->regval), p);
 		break;
 
 	default:
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 lines · Dan Carpenter

There seems to be some confusion here which functions return positive numbers and which return negative error codes.

copy_to_user() returns the number of bytes remaining to be copied but we want to return -EFAULT.

Quoted 7 lines

The rest is just clean up. get_user() actually returns zero on success and -EFAULT on error so we can preserve the error code. The timeout_to_regval() function returns -EINVAL on failure, but we can propogate that back instead of hardcoding -EINVAL ourselves.

Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Guenter Roeck <[email protected]>
#2 lore raw
In reply to Dan Carpenter

Hi Dan,

Quoted 6 lines

There seems to be some confusion here which functions return positive numbers and which return negative error codes.

copy_to_user() returns the number of bytes remaining to be copied but we want to return -EFAULT.

Quoted 6 lines

The rest is just clean up. get_user() actually returns zero on success and -EFAULT on error so we can preserve the error code. The timeout_to_regval() function returns -EINVAL on failure, but we can propogate that back instead of hardcoding -EINVAL ourselves.

Signed-off-by Dan Carpenter <[email protected]>

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.