Skip to contentSkip to conversation

IR/lirc: further ioctl portability fixups

PATCH In review Jarod Wilson 3 participants 4 messages Oct 8, 2010 – Oct 9, 2010

Subsystems (2)
Media (V4L/DVB) Staging
lore
Share & tools
lore

Jarod Wilson

author
#0 lore raw

PATCH 1/2 IR/lirc: further ioctl portability fixups

From: Joris van Rantwijk <[email protected]>

----8<----
I tested lirc_serial and found that it works fine. Except the LIRC ioctls do not work in my 64-bit-kernel/32-bit-user setup. I added compat_ioctl entries in the drivers to fix this.

While doing so, I noticed inconsistencies in the argument type of the LIRC ioctls. All ioctls are declared in lirc.h as having argument type __u32, however there are a few places where the driver calls get_user/put_user with an unsigned long argument.

The patch below changes lirc_dev and lirc_serial to use __u32 for all ioctl arguments, and adds compat_ioctl entries.
It should probably also be done in the other low-level drivers, but I don't have hardware to test those.
----8<----

I've dropped the .compat_ioctl addition from Joris' original patch, as I swear the non-compat definition should now work for both 32-bit and 64-bit userspace. Technically, I think we still need/want a
Signed-off-by: from Joris here. Joris? (And sorry for the lengthy delay in getting a reply to you).

Signed-off-by Jarod Wilson <[email protected]>
---
 drivers/media/IR/ir-lirc-codec.c |   10 +++++-----
 drivers/media/IR/lirc_dev.c      |   14 +++++++-------
 include/media/lirc_dev.h         |    4 ++--
 3 files changed, 14 insertions(+), 14 deletions(-)
drivers/media/IR/ir-lirc-codec.c +5 -5
index e63f757..c6d5b3e 100644
@@ -102,7 +102,7 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 	struct ir_input_dev *ir_dev;
 	int ret = 0;
 	void *drv_data;
-	unsigned long val = 0;
+	__u32 val = 0;
 
 	lirc = lirc_get_pdata(filep);
 	if (!lirc)

@@ -115,7 +115,7 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 	drv_data = ir_dev->props->priv;
 
 	if (_IOC_DIR(cmd) & _IOC_WRITE) {
-		ret = get_user(val, (unsigned long *)arg);
+		ret = get_user(val, (__u32 *)arg);
 		if (ret)
 			return ret;
 	}

@@ -135,14 +135,14 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 	/* TX settings */
 	case LIRC_SET_TRANSMITTER_MASK:
 		if (ir_dev->props->s_tx_mask)
-			ret = ir_dev->props->s_tx_mask(drv_data, (u32)val);
+			ret = ir_dev->props->s_tx_mask(drv_data, val);
 		else
 			return -EINVAL;
 		break;
 
 	case LIRC_SET_SEND_CARRIER:
 		if (ir_dev->props->s_tx_carrier)
-			ir_dev->props->s_tx_carrier(drv_data, (u32)val);
+			ir_dev->props->s_tx_carrier(drv_data, val);
 		else
 			return -EINVAL;
 		break;

@@ -212,7 +212,7 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 	}
 
 	if (_IOC_DIR(cmd) & _IOC_READ)
-		ret = put_user(val, (unsigned long *)arg);
+		ret = put_user(val, (__u32 *)arg);
 
 	return ret;
 }
drivers/media/IR/lirc_dev.c +7 -7
index 0572053..e4e4d99 100644
@@ -524,7 +524,7 @@ EXPORT_SYMBOL(lirc_dev_fop_poll);
 
 long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	unsigned long mode;
+	__u32 mode;
 	int result = 0;
 	struct irctl *ir = file->private_data;
 

@@ -541,7 +541,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	switch (cmd) {
 	case LIRC_GET_FEATURES:
-		result = put_user(ir->d.features, (unsigned long *)arg);
+		result = put_user(ir->d.features, (__u32 *)arg);
 		break;
 	case LIRC_GET_REC_MODE:
 		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {

@@ -551,7 +551,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		result = put_user(LIRC_REC2MODE
 				  (ir->d.features & LIRC_CAN_REC_MASK),
-				  (unsigned long *)arg);
+				  (__u32 *)arg);
 		break;
 	case LIRC_SET_REC_MODE:
 		if (!(ir->d.features & LIRC_CAN_REC_MASK)) {

@@ -559,7 +559,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		result = get_user(mode, (unsigned long *)arg);
+		result = get_user(mode, (__u32 *)arg);
 		if (!result && !(LIRC_MODE2REC(mode) & ir->d.features))
 			result = -EINVAL;
 		/*

@@ -568,7 +568,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		 */
 		break;
 	case LIRC_GET_LENGTH:
-		result = put_user(ir->d.code_length, (unsigned long *)arg);
+		result = put_user(ir->d.code_length, (__u32 *)arg);
 		break;
 	case LIRC_GET_MIN_TIMEOUT:
 		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||

@@ -577,7 +577,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		result = put_user(ir->d.min_timeout, (unsigned long *)arg);
+		result = put_user(ir->d.min_timeout, (__u32 *)arg);
 		break;
 	case LIRC_GET_MAX_TIMEOUT:
 		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||

@@ -586,7 +586,7 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		result = put_user(ir->d.max_timeout, (unsigned long *)arg);
+		result = put_user(ir->d.max_timeout, (__u32 *)arg);
 		break;
 	default:
 		result = -EINVAL;
include/media/lirc_dev.h +2 -2
index 71a896e..54780a5 100644
@@ -125,10 +125,10 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
 struct lirc_driver {
 	char name[40];
 	int minor;
-	unsigned long code_length;
+	__u32 code_length;
 	unsigned int buffer_size; /* in chunks holding one code each */
 	int sample_rate;
-	unsigned long features;
+	__u32 features;
 
 	unsigned int chunk_size;
Signature · 5 lines
1.7.1

-- 
Jarod Wilson
[email protected]
author
#1 lore raw

PATCH 2/2 staging/lirc: ioctl portability fixups

Signed-off-by Jarod Wilson <[email protected]>
---
 drivers/staging/lirc/lirc_it87.c     |   17 ++++++++---------
 drivers/staging/lirc/lirc_ite8709.c  |    6 +++---
 drivers/staging/lirc/lirc_parallel.c |   32 ++++++++++++++++----------------
 drivers/staging/lirc/lirc_serial.c   |   21 ++++++++++-----------
 drivers/staging/lirc/lirc_sir.c      |   21 ++++++++++-----------
 5 files changed, 47 insertions(+), 50 deletions(-)
drivers/staging/lirc/lirc_it87.c +8 -9
index ec11c0e..7d1b427 100644
@@ -239,8 +239,7 @@ static ssize_t lirc_write(struct file *file, const char *buf,
 static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	int retval = 0;
-	unsigned long value = 0;
-	unsigned int ivalue;
+	__u32 value = 0;
 	unsigned long hw_flags;
 
 	if (cmd == LIRC_GET_FEATURES)

@@ -256,24 +255,24 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	case LIRC_GET_FEATURES:
 	case LIRC_GET_SEND_MODE:
 	case LIRC_GET_REC_MODE:
-		retval = put_user(value, (unsigned long *) arg);
+		retval = put_user(value, (__u32 *) arg);
 		break;
 
 	case LIRC_SET_SEND_MODE:
 	case LIRC_SET_REC_MODE:
-		retval = get_user(value, (unsigned long *) arg);
+		retval = get_user(value, (__u32 *) arg);
 		break;
 
 	case LIRC_SET_SEND_CARRIER:
-		retval = get_user(ivalue, (unsigned int *) arg);
+		retval = get_user(value, (__u32 *) arg);
 		if (retval)
 			return retval;
-		ivalue /= 1000;
-		if (ivalue > IT87_CIR_FREQ_MAX ||
-		    ivalue < IT87_CIR_FREQ_MIN)
+		value /= 1000;
+		if (value > IT87_CIR_FREQ_MAX ||
+		    value < IT87_CIR_FREQ_MIN)
 			return -EINVAL;
 
-		it87_freq = ivalue;
+		it87_freq = value;
 
 		spin_lock_irqsave(&hardware_lock, hw_flags);
 		outb(((inb(io + IT87_CIR_TCR2) & IT87_CIR_TCR2_TXMPW) |
drivers/staging/lirc/lirc_ite8709.c +3 -3
index 9352f45..cb20cfd 100644
@@ -102,8 +102,8 @@ struct ite8709_device {
 	int io;
 	int irq;
 	spinlock_t hardware_lock;
-	unsigned long long acc_pulse;
-	unsigned long long acc_space;
+	__u64 acc_pulse;
+	__u64 acc_space;
 	char lastbit;
 	struct timeval last_tv;
 	struct lirc_driver driver;

@@ -220,7 +220,7 @@ static void ite8709_set_use_dec(void *data)
 }
 
 static void ite8709_add_read_queue(struct ite8709_device *dev, int flag,
-					unsigned long long val)
+				   __u64 val)
 {
 	int value;
drivers/staging/lirc/lirc_parallel.c +16 -16
index 6da4a8c..be3d60d 100644
@@ -301,9 +301,9 @@ static void irq_handler(void *blah)
 
 	if (signal != 0) {
 		/* ajust value to usecs */
-		unsigned long long helper;
+		__u64 helper;
 
-		helper = ((unsigned long long) signal)*1000000;
+		helper = ((__u64) signal)*1000000;
 		do_div(helper, timer);
 		signal = (long) helper;
 

@@ -404,9 +404,9 @@ static ssize_t lirc_write(struct file *filep, const char *buf, size_t n,
 
 	/* adjust values from usecs */
 	for (i = 0; i < count; i++) {
-		unsigned long long helper;
+		__u64 helper;
 
-		helper = ((unsigned long long) wbuf[i])*timer;
+		helper = ((__u64) wbuf[i])*timer;
 		do_div(helper, 1000000);
 		wbuf[i] = (int) helper;
 	}

@@ -464,48 +464,48 @@ static unsigned int lirc_poll(struct file *file, poll_table *wait)
 static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	int result;
-	unsigned long features = LIRC_CAN_SET_TRANSMITTER_MASK |
-				 LIRC_CAN_SEND_PULSE | LIRC_CAN_REC_MODE2;
-	unsigned long mode;
-	unsigned int ivalue;
+	__u32 features = LIRC_CAN_SET_TRANSMITTER_MASK |
+			 LIRC_CAN_SEND_PULSE | LIRC_CAN_REC_MODE2;
+	__u32 mode;
+	__u32 value;
 
 	switch (cmd) {
 	case LIRC_GET_FEATURES:
-		result = put_user(features, (unsigned long *) arg);
+		result = put_user(features, (__u32 *) arg);
 		if (result)
 			return result;
 		break;
 	case LIRC_GET_SEND_MODE:
-		result = put_user(LIRC_MODE_PULSE, (unsigned long *) arg);
+		result = put_user(LIRC_MODE_PULSE, (__u32 *) arg);
 		if (result)
 			return result;
 		break;
 	case LIRC_GET_REC_MODE:
-		result = put_user(LIRC_MODE_MODE2, (unsigned long *) arg);
+		result = put_user(LIRC_MODE_MODE2, (__u32 *) arg);
 		if (result)
 			return result;
 		break;
 	case LIRC_SET_SEND_MODE:
-		result = get_user(mode, (unsigned long *) arg);
+		result = get_user(mode, (__u32 *) arg);
 		if (result)
 			return result;
 		if (mode != LIRC_MODE_PULSE)
 			return -EINVAL;
 		break;
 	case LIRC_SET_REC_MODE:
-		result = get_user(mode, (unsigned long *) arg);
+		result = get_user(mode, (__u32 *) arg);
 		if (result)
 			return result;
 		if (mode != LIRC_MODE_MODE2)
 			return -ENOSYS;
 		break;
 	case LIRC_SET_TRANSMITTER_MASK:
-		result = get_user(ivalue, (unsigned int *) arg);
+		result = get_user(value, (__u32 *) arg);
 		if (result)
 			return result;
-		if ((ivalue & LIRC_PARALLEL_TRANSMITTER_MASK) != ivalue)
+		if ((value & LIRC_PARALLEL_TRANSMITTER_MASK) != value)
 			return LIRC_PARALLEL_MAX_TRANSMITTERS;
-		tx_mask = ivalue;
+		tx_mask = value;
 		break;
 	default:
 		return -ENOIOCTLCMD;
drivers/staging/lirc/lirc_serial.c +10 -11
index 9456f8e..02906b4 100644
@@ -372,7 +372,7 @@ static unsigned long conv_us_to_clocks;
 static int init_timing_params(unsigned int new_duty_cycle,
 		unsigned int new_freq)
 {
-	unsigned long long loops_per_sec, work;
+	__u64 loops_per_sec, work;
 
 	duty_cycle = new_duty_cycle;
 	freq = new_freq;

@@ -987,8 +987,7 @@ static ssize_t lirc_write(struct file *file, const char *buf,
 static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	int result;
-	unsigned long value;
-	unsigned int ivalue;
+	__u32 value;
 
 	switch (cmd) {
 	case LIRC_GET_SEND_MODE:

@@ -997,7 +996,7 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 
 		result = put_user(LIRC_SEND2MODE
 				  (hardware[type].features&LIRC_CAN_SEND_MASK),
-				  (unsigned long *) arg);
+				  (__u32 *) arg);
 		if (result)
 			return result;
 		break;

@@ -1006,7 +1005,7 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		if (!(hardware[type].features&LIRC_CAN_SEND_MASK))
 			return -ENOIOCTLCMD;
 
-		result = get_user(value, (unsigned long *) arg);
+		result = get_user(value, (__u32 *) arg);
 		if (result)
 			return result;
 		/* only LIRC_MODE_PULSE supported */

@@ -1023,12 +1022,12 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		if (!(hardware[type].features&LIRC_CAN_SET_SEND_DUTY_CYCLE))
 			return -ENOIOCTLCMD;
 
-		result = get_user(ivalue, (unsigned int *) arg);
+		result = get_user(value, (__u32 *) arg);
 		if (result)
 			return result;
-		if (ivalue <= 0 || ivalue > 100)
+		if (value <= 0 || value > 100)
 			return -EINVAL;
-		return init_timing_params(ivalue, freq);
+		return init_timing_params(value, freq);
 		break;
 
 	case LIRC_SET_SEND_CARRIER:

@@ -1036,12 +1035,12 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		if (!(hardware[type].features&LIRC_CAN_SET_SEND_CARRIER))
 			return -ENOIOCTLCMD;
 
-		result = get_user(ivalue, (unsigned int *) arg);
+		result = get_user(value, (__u32 *) arg);
 		if (result)
 			return result;
-		if (ivalue > 500000 || ivalue < 20000)
+		if (value > 500000 || value < 20000)
 			return -EINVAL;
-		return init_timing_params(duty_cycle, ivalue);
+		return init_timing_params(duty_cycle, value);
 		break;
 
 	default:
drivers/staging/lirc/lirc_sir.c +10 -11
index eb08fa7..10354f9 100644
@@ -336,9 +336,8 @@ static ssize_t lirc_write(struct file *file, const char *buf, size_t n,
 static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	int retval = 0;
-	unsigned long value = 0;
+	__u32 value = 0;
 #ifdef LIRC_ON_SA1100
-	unsigned int ivalue;
 
 	if (cmd == LIRC_GET_FEATURES)
 		value = LIRC_CAN_SEND_PULSE |

@@ -362,22 +361,22 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	case LIRC_GET_FEATURES:
 	case LIRC_GET_SEND_MODE:
 	case LIRC_GET_REC_MODE:
-		retval = put_user(value, (unsigned long *) arg);
+		retval = put_user(value, (__u32 *) arg);
 		break;
 
 	case LIRC_SET_SEND_MODE:
 	case LIRC_SET_REC_MODE:
-		retval = get_user(value, (unsigned long *) arg);
+		retval = get_user(value, (__u32 *) arg);
 		break;
 #ifdef LIRC_ON_SA1100
 	case LIRC_SET_SEND_DUTY_CYCLE:
-		retval = get_user(ivalue, (unsigned int *) arg);
+		retval = get_user(value, (__u32 *) arg);
 		if (retval)
 			return retval;
-		if (ivalue <= 0 || ivalue > 100)
+		if (value <= 0 || value > 100)
 			return -EINVAL;
-		/* (ivalue/100)*(1000000/freq) */
-		duty_cycle = ivalue;
+		/* (value/100)*(1000000/freq) */
+		duty_cycle = value;
 		pulse_width = (unsigned long) duty_cycle*10000/freq;
 		space_width = (unsigned long) 1000000L/freq-pulse_width;
 		if (pulse_width >= LIRC_ON_SA1100_TRANSMITTER_LATENCY)

@@ -386,12 +385,12 @@ static long lirc_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 			space_width -= LIRC_ON_SA1100_TRANSMITTER_LATENCY;
 		break;
 	case LIRC_SET_SEND_CARRIER:
-		retval = get_user(ivalue, (unsigned int *) arg);
+		retval = get_user(value, (__u32 *) arg);
 		if (retval)
 			return retval;
-		if (ivalue > 500000 || ivalue < 20000)
+		if (value > 500000 || value < 20000)
 			return -EINVAL;
-		freq = ivalue;
+		freq = value;
 		pulse_width = (unsigned long) duty_cycle*10000/freq;
 		space_width = (unsigned long) 1000000L/freq-pulse_width;
 		if (pulse_width >= LIRC_ON_SA1100_TRANSMITTER_LATENCY)
Signature · 6 lines
1.7.1


-- 
Jarod Wilson
[email protected]
#2 lore raw
In reply to Jarod Wilson
Quoted 5 lines · On Friday, October 08, 2010 23:12:35 Jarod Wilson wrote:

I've dropped the .compat_ioctl addition from Joris' original patch, as I swear the non-compat definition should now work for both 32-bit and 64-bit userspace. Technically, I think we still need/want a
Signed-off-by: from Joris here. Joris? (And sorry for the lengthy delay in getting a reply to you).

No. I just tested the patch and it does not work without .compat_ioctl. I get this error:

[  313.614017] ioctl32(irrecord:4490): Unknown cmd fd(4) cmd(80046900 {t:'i';sz:4} arg(080705e8) on /dev/lirc0

irrecord: could not get hardware features
irrecord: this device driver does not support the new LIRC interface
irrecord: major number of /dev/lirc0 is 252
irrecord: LIRC major number is 61
irrecord: check if /dev/lirc0 is a LIRC device
irrecord: could not init hardware (lircd running ? --> close it, check permissions)

On the other hand it works fine when .compat_ioctl is added to ir_lirc_codec.c and lirc_dev.c and lirc_serial.c.

Regards,
Joris.

#3 lore raw
Quoted 8 lines · Joris van Rantwijk
Quoted 5 lines · On Friday, October 08, 2010 23:12:35 Jarod Wilson wrote:

I've dropped the .compat_ioctl addition from Joris' original patch, as I swear the non-compat definition should now work for both 32-bit and 64-bit userspace. Technically, I think we still need/want a
Signed-off-by: from Joris here. Joris? (And sorry for the lengthy delay in getting a reply to you).

No. I just tested the patch and it does not work without .compat_ioctl.

Okay, seems I misunderstood something then, thought the main one would be tried if the compat one wasn't wired.

Will tack on another patch adding back .compat_ioctl where needed, thanks for testing!

Signature · 2 lines
Jarod Wilson
[email protected]