Re: [PATCH] [media] serial_ir: ensure we're ready to receive interrupts

[Mauro Carvalho Chehab <mchehab-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

Em Sat, 25 Feb 2017 11:28:16 +0000
Sean Young <sean-hENCXIMQXOg@public.gmane.org> escreveu:

> When the interrupt requested with devm_request_irq(), serial_ir.rcdev
> is still null so will cause null deference if the irq handler is called
> early on.
> 
> Also ensure that timeout_timer is setup.
> 
> Link: http://lkml.kernel.org/r/CA+55aFxsh2uF8gi5sN_guY3Z+tiLv7LpJYKBw+y8vqLzp+TsnQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # 4.10
> Signed-off-by: Sean Young <sean-hENCXIMQXOg@public.gmane.org>
> ---
>  drivers/media/rc/serial_ir.c | 243 +++++++++++++++++++++----------------------
>  1 file changed, 118 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/media/rc/serial_ir.c b/drivers/media/rc/serial_ir.c
> index 923fb22..22144b4 100644
> --- a/drivers/media/rc/serial_ir.c
> +++ b/drivers/media/rc/serial_ir.c
> @@ -487,74 +487,6 @@ static void serial_ir_timeout(unsigned long arg)
>  	ir_raw_event_handle(serial_ir.rcdev);
>  }
>  
> -static int serial_ir_probe(struct platform_device *dev)
> -{
> -	int i, nlow, nhigh, result;

Hmm... why did you move this function to be after serial_ir_open()?

That messes with the diff without no good reason, making harder to
identify what you changed here.

> -
> -	result = devm_request_irq(&dev->dev, irq, serial_ir_irq_handler,
> -				  share_irq ? IRQF_SHARED : 0,
> -				  KBUILD_MODNAME, &hardware);
> -	if (result < 0) {
> -		if (result == -EBUSY)
> -			dev_err(&dev->dev, "IRQ %d busy\n", irq);
> -		else if (result == -EINVAL)
> -			dev_err(&dev->dev, "Bad irq number or handler\n");
> -		return result;
> -	}
> -
> -	/* Reserve io region. */
> -	if ((iommap &&
> -	     (devm_request_mem_region(&dev->dev, iommap, 8 << ioshift,
> -				      KBUILD_MODNAME) == NULL)) ||
> -	     (!iommap && (devm_request_region(&dev->dev, io, 8,
> -			  KBUILD_MODNAME) == NULL))) {
> -		dev_err(&dev->dev, "port %04x already in use\n", io);
> -		dev_warn(&dev->dev, "use 'setserial /dev/ttySX uart none'\n");
> -		dev_warn(&dev->dev,
> -			 "or compile the serial port driver as module and\n");
> -		dev_warn(&dev->dev, "make sure this module is loaded first\n");
> -		return -EBUSY;
> -	}
> -
> -	setup_timer(&serial_ir.timeout_timer, serial_ir_timeout,
> -		    (unsigned long)&serial_ir);
> -
> -	result = hardware_init_port();
> -	if (result < 0)
> -		return result;
> -
> -	/* Initialize pulse/space widths */
> -	init_timing_params(50, 38000);
> -
> -	/* If pin is high, then this must be an active low receiver. */
> -	if (sense == -1) {
> -		/* wait 1/2 sec for the power supply */
> -		msleep(500);
> -
> -		/*
> -		 * probe 9 times every 0.04s, collect "votes" for
> -		 * active high/low
> -		 */
> -		nlow = 0;
> -		nhigh = 0;
> -		for (i = 0; i < 9; i++) {
> -			if (sinp(UART_MSR) & hardware[type].signal_pin)
> -				nlow++;
> -			else
> -				nhigh++;
> -			msleep(40);
> -		}
> -		sense = nlow >= nhigh ? 1 : 0;
> -		dev_info(&dev->dev, "auto-detected active %s receiver\n",
> -			 sense ? "low" : "high");
> -	} else
> -		dev_info(&dev->dev, "Manually using active %s receiver\n",
> -			 sense ? "low" : "high");
> -
> -	dev_dbg(&dev->dev, "Interrupt %d, port %04x obtained\n", irq, io);
> -	return 0;
> -}
> -
>  static int serial_ir_open(struct rc_dev *rcdev)
>  {
>  	unsigned long flags;
> @@ -679,6 +611,123 @@ static int serial_ir_resume(struct platform_device *dev)
>  	return 0;
>  }
>  
> +static int serial_ir_probe(struct platform_device *dev)
> +{
> +	struct rc_dev *rcdev;
> +	int i, nlow, nhigh, result;
> +
> +	rcdev = devm_rc_allocate_device(&dev->dev, RC_DRIVER_IR_RAW);
> +	if (!rcdev)
> +		return -ENOMEM;
> +
> +	if (hardware[type].send_pulse && hardware[type].send_space)
> +		rcdev->tx_ir = serial_ir_tx;
> +	if (hardware[type].set_send_carrier)
> +		rcdev->s_tx_carrier = serial_ir_tx_carrier;
> +	if (hardware[type].set_duty_cycle)
> +		rcdev->s_tx_duty_cycle = serial_ir_tx_duty_cycle;
> +
> +	switch (type) {
> +	case IR_HOMEBREW:
> +		rcdev->input_name = "Serial IR type home-brew";
> +		break;
> +	case IR_IRDEO:
> +		rcdev->input_name = "Serial IR type IRdeo";
> +		break;
> +	case IR_IRDEO_REMOTE:
> +		rcdev->input_name = "Serial IR type IRdeo remote";
> +		break;
> +	case IR_ANIMAX:
> +		rcdev->input_name = "Serial IR type AnimaX";
> +		break;
> +	case IR_IGOR:
> +		rcdev->input_name = "Serial IR type IgorPlug";
> +		break;
> +	}
> +
> +	rcdev->input_phys = KBUILD_MODNAME "/input0";
> +	rcdev->input_id.bustype = BUS_HOST;
> +	rcdev->input_id.vendor = 0x0001;
> +	rcdev->input_id.product = 0x0001;
> +	rcdev->input_id.version = 0x0100;
> +	rcdev->open = serial_ir_open;
> +	rcdev->close = serial_ir_close;
> +	rcdev->dev.parent = &serial_ir.pdev->dev;
> +	rcdev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
> +	rcdev->driver_name = KBUILD_MODNAME;
> +	rcdev->map_name = RC_MAP_RC6_MCE;
> +	rcdev->min_timeout = 1;
> +	rcdev->timeout = IR_DEFAULT_TIMEOUT;
> +	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> +	rcdev->rx_resolution = 250000;
> +
> +	serial_ir.rcdev = rcdev;
> +
> +	setup_timer(&serial_ir.timeout_timer, serial_ir_timeout,
> +		    (unsigned long)&serial_ir);
> +
> +	result = devm_request_irq(&dev->dev, irq, serial_ir_irq_handler,
> +				  share_irq ? IRQF_SHARED : 0,
> +				  KBUILD_MODNAME, &hardware);
> +	if (result < 0) {
> +		if (result == -EBUSY)
> +			dev_err(&dev->dev, "IRQ %d busy\n", irq);
> +		else if (result == -EINVAL)
> +			dev_err(&dev->dev, "Bad irq number or handler\n");
> +		return result;
> +	}
> +
> +	/* Reserve io region. */
> +	if ((iommap &&
> +	     (devm_request_mem_region(&dev->dev, iommap, 8 << ioshift,
> +				      KBUILD_MODNAME) == NULL)) ||
> +	     (!iommap && (devm_request_region(&dev->dev, io, 8,
> +			  KBUILD_MODNAME) == NULL))) {
> +		dev_err(&dev->dev, "port %04x already in use\n", io);
> +		dev_warn(&dev->dev, "use 'setserial /dev/ttySX uart none'\n");
> +		dev_warn(&dev->dev,
> +			 "or compile the serial port driver as module and\n");
> +		dev_warn(&dev->dev, "make sure this module is loaded first\n");
> +		return -EBUSY;
> +	}
> +
> +	result = hardware_init_port();
> +	if (result < 0)
> +		return result;
> +
> +	/* Initialize pulse/space widths */
> +	init_timing_params(50, 38000);
> +
> +	/* If pin is high, then this must be an active low receiver. */
> +	if (sense == -1) {
> +		/* wait 1/2 sec for the power supply */
> +		msleep(500);
> +
> +		/*
> +		 * probe 9 times every 0.04s, collect "votes" for
> +		 * active high/low
> +		 */
> +		nlow = 0;
> +		nhigh = 0;
> +		for (i = 0; i < 9; i++) {
> +			if (sinp(UART_MSR) & hardware[type].signal_pin)
> +				nlow++;
> +			else
> +				nhigh++;
> +			msleep(40);
> +		}
> +		sense = nlow >= nhigh ? 1 : 0;
> +		dev_info(&dev->dev, "auto-detected active %s receiver\n",
> +			 sense ? "low" : "high");
> +	} else
> +		dev_info(&dev->dev, "Manually using active %s receiver\n",
> +			 sense ? "low" : "high");
> +
> +	dev_dbg(&dev->dev, "Interrupt %d, port %04x obtained\n", irq, io);
> +
> +	return devm_rc_register_device(&dev->dev, rcdev);
> +}
> +
>  static struct platform_driver serial_ir_driver = {
>  	.probe		= serial_ir_probe,
>  	.suspend	= serial_ir_suspend,
> @@ -723,7 +772,6 @@ static void serial_ir_exit(void)
>  
>  static int __init serial_ir_init_module(void)
>  {
> -	struct rc_dev *rcdev;
>  	int result;
>  
>  	switch (type) {
> @@ -754,63 +802,9 @@ static int __init serial_ir_init_module(void)
>  		sense = !!sense;
>  
>  	result = serial_ir_init();
> -	if (result)
> -		return result;
> -
> -	rcdev = devm_rc_allocate_device(&serial_ir.pdev->dev, RC_DRIVER_IR_RAW);
> -	if (!rcdev) {
> -		result = -ENOMEM;
> -		goto serial_cleanup;
> -	}
> -
> -	if (hardware[type].send_pulse && hardware[type].send_space)
> -		rcdev->tx_ir = serial_ir_tx;
> -	if (hardware[type].set_send_carrier)
> -		rcdev->s_tx_carrier = serial_ir_tx_carrier;
> -	if (hardware[type].set_duty_cycle)
> -		rcdev->s_tx_duty_cycle = serial_ir_tx_duty_cycle;
> -
> -	switch (type) {
> -	case IR_HOMEBREW:
> -		rcdev->input_name = "Serial IR type home-brew";
> -		break;
> -	case IR_IRDEO:
> -		rcdev->input_name = "Serial IR type IRdeo";
> -		break;
> -	case IR_IRDEO_REMOTE:
> -		rcdev->input_name = "Serial IR type IRdeo remote";
> -		break;
> -	case IR_ANIMAX:
> -		rcdev->input_name = "Serial IR type AnimaX";
> -		break;
> -	case IR_IGOR:
> -		rcdev->input_name = "Serial IR type IgorPlug";
> -		break;
> -	}
> -
> -	rcdev->input_phys = KBUILD_MODNAME "/input0";
> -	rcdev->input_id.bustype = BUS_HOST;
> -	rcdev->input_id.vendor = 0x0001;
> -	rcdev->input_id.product = 0x0001;
> -	rcdev->input_id.version = 0x0100;
> -	rcdev->open = serial_ir_open;
> -	rcdev->close = serial_ir_close;
> -	rcdev->dev.parent = &serial_ir.pdev->dev;
> -	rcdev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
> -	rcdev->driver_name = KBUILD_MODNAME;
> -	rcdev->map_name = RC_MAP_RC6_MCE;
> -	rcdev->min_timeout = 1;
> -	rcdev->timeout = IR_DEFAULT_TIMEOUT;
> -	rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> -	rcdev->rx_resolution = 250000;
> -
> -	serial_ir.rcdev = rcdev;
> -
> -	result = rc_register_device(rcdev);
> -
>  	if (!result)
>  		return 0;
> -serial_cleanup:
> +
>  	serial_ir_exit();
>  	return result;
>  }
> @@ -818,7 +812,6 @@ static int __init serial_ir_init_module(void)
>  static void __exit serial_ir_exit_module(void)
>  {
>  	del_timer_sync(&serial_ir.timeout_timer);
> -	rc_unregister_device(serial_ir.rcdev);
>  	serial_ir_exit();
>  }
>  



Thanks,
Mauro