diff mbox

[v4] Input: mousedev - fix implicit conversion warning

Message ID 20170625180609.GA8697@dtor-ws (mailing list archive)
State New, archived
Headers show

Commit Message

Dmitry Torokhov June 25, 2017, 6:06 p.m. UTC 
Hi Nick,

On Mon, May 29, 2017 at 10:41:51PM -0700, Nick Desaulniers wrote:
> Clang warns:
> 
> drivers/input/mousedev.c:653:63: error: implicit conversion from 'int'
> to 'signed char' changes value from 200 to -56
> [-Wconstant-conversion]
>   client->ps2[1] = 0x60; client->ps2[2] = 3; client->ps2[3] = 200;
>                                                             ~ ^~~
> 
> As far as I can tell, from
> 
> http://www.computer-engineering.org/ps2mouse/
> 
> Under "Command Set" > "0xE9 (Status Request)"
> 
> the value 200 is a valid sample rate. Using unsigned char [], rather than
> signed char [], for client->ps2 silences this warning.
> 
> Also moves some reused logic into a helper function.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
> What's new in v4:
> * replace mousedev_limit_delta() with update_clamped() that also updates
>   the ps2_data and delta values. The use of the temporary val should
>   avoid integral conversion and promotion issues.
> 
>  drivers/input/mousedev.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
> index 0e0ff84088fd..e645b8c6f2cb 100644
> --- a/drivers/input/mousedev.c
> +++ b/drivers/input/mousedev.c
> @@ -103,7 +103,7 @@ struct mousedev_client {
>  	spinlock_t packet_lock;
>  	int pos_x, pos_y;
>  
> -	signed char ps2[6];
> +	unsigned char ps2[6];
>  	unsigned char ready, buffer, bufsiz;
>  	unsigned char imexseq, impsseq;
>  	enum mousedev_emul mode;
> @@ -571,27 +571,27 @@ static int mousedev_open(struct inode *inode, struct file *file)
>  	return error;
>  }
>  
> -static inline int mousedev_limit_delta(int delta, int limit)
> +static inline void
> +update_clamped(unsigned char *ps2_data, int *delta, int limit)
>  {
> -	return delta > limit ? limit : (delta < -limit ? -limit : delta);
> +	int val = *delta > limit ? limit : (*delta < -limit ? -limit : *delta);
> +	*ps2_data = (unsigned char) val;
> +	*delta -= val;

Since the time the code was written we got nice helpers to clamp the
values. Does the following work for you or it still leaves clang
unhappy?

Thanks.

Comments

Nick Desaulniers June 27, 2017, 1:39 a.m. UTC | #1 
On Sun, Jun 25, 2017 at 11:06:09AM -0700, Dmitry Torokhov wrote:
> Hi Nick,
>
> Since the time the code was written we got nice helpers to clamp the
> values. Does the following work for you or it still leaves clang
> unhappy?

LGTM, no more warnings with Clang.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Desaulniers July 12, 2017, 6:57 a.m. UTC | #2 
Hi Dmitry, did you get a chance to merge the sugguested revision, yet?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 0e0ff84088fd..6ef0c5314f69 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -15,6 +15,7 @@ 
 #define MOUSEDEV_MINORS		31
 #define MOUSEDEV_MIX		63
 
+#include <linux/bitops.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/poll.h>
@@ -103,7 +104,7 @@  struct mousedev_client {
 	spinlock_t packet_lock;
 	int pos_x, pos_y;
 
-	signed char ps2[6];
+	u8 ps2[6];
 	unsigned char ready, buffer, bufsiz;
 	unsigned char imexseq, impsseq;
 	enum mousedev_emul mode;
@@ -291,11 +292,10 @@  static void mousedev_notify_readers(struct mousedev *mousedev,
 		}
 
 		client->pos_x += packet->dx;
-		client->pos_x = client->pos_x < 0 ?
-			0 : (client->pos_x >= xres ? xres : client->pos_x);
+		client->pos_x = clamp_val(client->pos_x, 0, xres);
+
 		client->pos_y += packet->dy;
-		client->pos_y = client->pos_y < 0 ?
-			0 : (client->pos_y >= yres ? yres : client->pos_y);
+		client->pos_y = clamp_val(client->pos_y, 0, yres);
 
 		p->dx += packet->dx;
 		p->dy += packet->dy;
@@ -571,44 +571,48 @@  static int mousedev_open(struct inode *inode, struct file *file)
 	return error;
 }
 
-static inline int mousedev_limit_delta(int delta, int limit)
-{
-	return delta > limit ? limit : (delta < -limit ? -limit : delta);
-}
-
-static void mousedev_packet(struct mousedev_client *client,
-			    signed char *ps2_data)
+static void mousedev_packet(struct mousedev_client *client, u8 *ps2_data)
 {
 	struct mousedev_motion *p = &client->packets[client->tail];
+	s8 dx, dy, dz;
+
+	dx = clamp_val(p->dx, -127, 127);
+	p->dx -= dx;
+
+	dy = clamp_val(p->dy, -127, 127);
+	p->dy -= dy;
 
-	ps2_data[0] = 0x08 |
-		((p->dx < 0) << 4) | ((p->dy < 0) << 5) | (p->buttons & 0x07);
-	ps2_data[1] = mousedev_limit_delta(p->dx, 127);
-	ps2_data[2] = mousedev_limit_delta(p->dy, 127);
-	p->dx -= ps2_data[1];
-	p->dy -= ps2_data[2];
+	ps2_data[0] = BIT(3);
+	ps2_data[0] |= ((dx & BIT(7)) >> 3) | ((dy & BIT(7)) >> 2);
+	ps2_data[0] |= p->buttons & 0x07;
 
 	switch (client->mode) {
 	case MOUSEDEV_EMUL_EXPS:
-		ps2_data[3] = mousedev_limit_delta(p->dz, 7);
-		p->dz -= ps2_data[3];
-		ps2_data[3] = (ps2_data[3] & 0x0f) | ((p->buttons & 0x18) << 1);
+		dz = clamp_val(p->dz, -7, 7);
+		p->dz -= dz;
+
+		ps2_data[3] = (dz & 0x0f) | ((p->buttons & 0x18) << 1);
 		client->bufsiz = 4;
 		break;
 
 	case MOUSEDEV_EMUL_IMPS:
-		ps2_data[0] |=
-			((p->buttons & 0x10) >> 3) | ((p->buttons & 0x08) >> 1);
-		ps2_data[3] = mousedev_limit_delta(p->dz, 127);
-		p->dz -= ps2_data[3];
+		dz = clamp_val(p->dz, -127, 127);
+		p->dz -= dz;
+
+		ps2_data[0] |= ((p->buttons & 0x10) >> 3) |
+			       ((p->buttons & 0x08) >> 1);
+		ps2_data[3] = dz;
+
 		client->bufsiz = 4;
 		break;
 
 	case MOUSEDEV_EMUL_PS2:
 	default:
-		ps2_data[0] |=
-			((p->buttons & 0x10) >> 3) | ((p->buttons & 0x08) >> 1);
 		p->dz = 0;
+
+		ps2_data[0] |= ((p->buttons & 0x10) >> 3) |
+                               ((p->buttons & 0x08) >> 1);
+
 		client->bufsiz = 3;
 		break;
 	}
@@ -714,7 +718,7 @@  static ssize_t mousedev_read(struct file *file, char __user *buffer,
 {
 	struct mousedev_client *client = file->private_data;
 	struct mousedev *mousedev = client->mousedev;
-	signed char data[sizeof(client->ps2)];
+	u8 data[sizeof(client->ps2)];
 	int retval = 0;
 
 	if (!client->ready && !client->buffer && mousedev->exist &&