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+t...@mail.gmail.com Cc: <sta...@vger.kernel.org> # 4.10 Signed-off-by: Sean Young <s...@mess.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; - - 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(); } -- 2.9.3 On Sat, Feb 25, 2017 at 11:14:25AM +0000, Sean Young wrote: > On Fri, Feb 24, 2017 at 11:15:51AM -0800, Linus Torvalds wrote: > > Added more relevant people. I've debugged the immediate problem below, > > but I think there's another problem that actually triggered this. > > > > On Fri, Feb 24, 2017 at 10:28 AM, kernel test robot > > <fengguang...@intel.com> wrote: > > > > > > 0day kernel testing robot got the below dmesg and the first bad commit is > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > > > > > commit ff58d005cd10fcd372787cceac547e11cf706ff6 > > > Merge: 5ab3566 9eeb0ed > > > > > > Merge tag 'media/v4.11-1' of > > > git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media > > [...] > > > [ 4.664940] rc rc0: lirc_dev: driver ir-lirc-codec (rc-loopback) > > > registered at minor = 0 > > > [ 4.666322] BUG: unable to handle kernel NULL pointer dereference at > > > 0000039c > > > [ 4.666675] IP: serial_ir_irq_handler+0x189/0x410 > > > > This merge being fingered ends up being a subtle interaction with other > > changes. > > > > Those "other changes" are (again) the interrupt retrigger code that > > was reverted for 4.10, and then we tried to merge them again this > > merge window. > > > > Because the immediate cause is: > > > > > [ 4.666675] EIP: serial_ir_irq_handler+0x189/0x410 > > > [ 4.666675] Call Trace: > > > [ 4.666675] <IRQ> > > > [ 4.666675] __handle_irq_event_percpu+0x57/0x100 > > > [ 4.666675] handle_irq_event_percpu+0x1d/0x50 > > > [ 4.666675] handle_irq_event+0x32/0x60 > > > [ 4.666675] handle_edge_irq+0xa5/0x120 > > > [ 4.666675] handle_irq+0x9d/0xd0 > > > [ 4.666675] </IRQ> > > > [ 4.666675] do_IRQ+0x5f/0x130 > > > [ 4.666675] common_interrupt+0x33/0x38 > > > [ 4.666675] EIP: hardware_init_port+0x3f/0x190 > > > [ 4.666675] EFLAGS: 00200246 CPU: 0 > > > [ 4.666675] EAX: c718990f EBX: 00000000 ECX: 00000000 EDX: 000003f9 > > > [ 4.666675] ESI: 000003f9 EDI: 000003f8 EBP: c0065d98 ESP: c0065d84 > > > [ 4.666675] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 > > > [ 4.666675] serial_ir_probe+0xbb/0x300 > > > [ 4.666675] platform_drv_probe+0x48/0xb0 > > ... > > > > ie an interrupt came in immediately after the request_irq(), before > > all the data was properly set up, which then causes the interrupt > > handler to take a fault because it tries to access some field that > > hasn't even been set up yet. > > Oh dear. I've pointed out others making the same mistake when doing code > reviews, clearly I need review my own code better. > > > > > The code line is helpful, the faulting instruction is > > > > mov 0x39c(%rax),%eax <--- fault > > call .. > > mov someglobalvar,%edx > > > > which together with the supplied config file makes me able to match it > > up with the assembly generation around it: > > > > inb %dx, %al # tmp254, value > > andb $1, %al #, tmp255 > > testb %al, %al # tmp255 > > je .L233 #, > > .L215: > > movl serial_ir+8, %eax # serial_ir.rcdev, serial_ir.rcdev > > xorl %edx, %edx # _66->timeout > > movl 924(%eax), %eax # _66->timeout, _66->timeout > > call nsecs_to_jiffies # > > movl jiffies, %edx # jiffies, jiffies.33_70 > > addl %eax, %edx # _69, tmp259 > > movl $serial_ir+16, %eax #, > > call mod_timer # > > movl serial_ir+8, %eax # serial_ir.rcdev, > > call ir_raw_event_handle # > > movl $1, %eax #, <retval> > > > > so it's that "serial_ir.rcdev->timeout" access that faults. So this is > > the faulting source code: > > > > drivers/media/rc/serial_ir.c: 402 > > > > mod_timer(&serial_ir.timeout_timer, > > jiffies + nsecs_to_jiffies(serial_ir.rcdev->timeout)); > > > > ir_raw_event_handle(serial_ir.rcdev); > > > > return IRQ_HANDLED; > > > > and serial_ir.rcdev is NULL when ti tries to look up the timeout. > > ir_raw_event_handle() call will also go bang if passed a null pointer, so > this problem existed before (since v4.10). > > Thanks for debugging this, I'll send a patch as a reply to this email. > > > Sean
