RE: [PATCH resent] lis3: Fix Oops with NULL platform data

[samu.p.onkalo]

Added lm-sensors list...

>-----Original Message-----
>From: ext Takashi Iwai [mailto:tiwai@xxxxxxx]
>Sent: 22 September, 2010 16:47
>To: Onkalo Samu.P (Nokia-MS/Tampere)
>Cc: akpm@xxxxxxxxxxxxxxxxxxxx; Eric.Piel@xxxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH resent] lis3: Fix Oops with NULL platform data
>
>At Wed, 22 Sep 2010 13:56:15 +0200,
><samu.p.onkalo@xxxxxxxxx> wrote:
>>
>>
>> Hi
>>
>> >-----Original Message-----
>> >From: ext Takashi Iwai [mailto:tiwai@xxxxxxx]
>> >Sent: 22 September, 2010 14:34
>> >To: Andrew Morton
>> >Cc: Onkalo Samu.P (Nokia-MS/Tampere); Ãric Piel; linux-
>> >kernel@xxxxxxxxxxxxxxx
>> >Subject: [PATCH resent] lis3: Fix Oops with NULL platform data
>> >
>> >The recent addition of threaded irq handler causes a NULL dereference
>> >when used with hp_accel driver, which has NULL pdata.
>> >
>> >Cc: <stable@xxxxxxxxxx>
>> >Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>> >---
>> >
>> >This should go to 2.6.36 and 2.6.35-stable tree.
>> >
>> > drivers/hwmon/lis3lv02d.c |    6 ++++--
>> > 1 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>> >index 5e15967..1c266ec 100644
>> >--- a/drivers/hwmon/lis3lv02d.c
>> >+++ b/drivers/hwmon/lis3lv02d.c
>> >@@ -354,7 +354,8 @@ static irqreturn_t
>lis302dl_interrupt_thread1_8b(int
>> >irq, void *data)
>> >
>> > 	struct lis3lv02d *lis3 = data;
>> >
>> >-	if ((lis3->pdata->irq_cfg & LIS3_IRQ1_MASK) == LIS3_IRQ1_CLICK)
>> >+	if (lis3->pdata &&
>> >+	    (lis3->pdata->irq_cfg & LIS3_IRQ1_MASK) == LIS3_IRQ1_CLICK)
>> > 		lis302dl_interrupt_handle_click(lis3);
>> > 	else
>> > 		lis302dl_interrupt_handle_ff_wu(lis3);
>> >@@ -367,7 +368,8 @@ static irqreturn_t
>lis302dl_interrupt_thread2_8b(int
>> >irq, void *data)
>> >
>> > 	struct lis3lv02d *lis3 = data;
>> >
>> >-	if ((lis3->pdata->irq_cfg & LIS3_IRQ2_MASK) == LIS3_IRQ2_CLICK)
>> >+	if (lis3->pdata &&
>> >+	    (lis3->pdata->irq_cfg & LIS3_IRQ2_MASK) == LIS3_IRQ2_CLICK)
>> > 		lis302dl_interrupt_handle_click(lis3);
>> > 	else
>> > 		lis302dl_interrupt_handle_ff_wu(lis3);
>>
>> Yes, that will remove the kernel oops definitely, but I'm wondering if
>> this thread should be executed at all if there is no pdata available.
>
>We may control the caller of request_threaded_irq(), instead.
>The revised patch is below.  (With this patch, the patch for the new
>LIS3CD chip must be revised, too.)
>
>> Eric, is it so that in laptops interrupts are used for free fall
>detection
>> and polled input device is for joystick?
>
>Yes, it's so for HP laptops.
>
>> Interrupts can (and are used) also for other purposes but in that case
>> some additional configurations are done based on platform data.
>>
>> With this patch, there are just some additional input event refreshes
>in
>> top of polled device updates. It should really not matter. So,
>>
>> Acked-by: Samu Onkalo <samu.p.onkalo@xxxxxxxxx>
>>
>> -Samu
>
>
>thanks,
>
>Takashi
>
>===
>From 0e5bdc7516915e92fe471a77da36ecb75d995c0f Mon Sep 17 00:00:00 2001
>From: Takashi Iwai <tiwai@xxxxxxx>
>Date: Wed, 22 Sep 2010 12:16:04 +0200
>Subject: [PATCH] lis3: Fix Oops with NULL platform data
>
>The recent addition of threaded irq handler causes a NULL dereference
>when used with hp_accel driver, which has NULL pdata.
>
>Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>---
>this version avoids the unnecessary thread_irq call instead of checking
>pdata in the irq handler.
>
> drivers/hwmon/lis3lv02d.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>index 5e15967..0a24177 100644
>--- a/drivers/hwmon/lis3lv02d.c
>+++ b/drivers/hwmon/lis3lv02d.c
>@@ -301,7 +301,7 @@ static irqreturn_t lis302dl_interrupt(int irq, void
>*dummy)
> 	wake_up_interruptible(&lis3_dev.misc_wait);
> 	kill_fasync(&lis3_dev.async_queue, SIGIO, POLL_IN);
> out:
>-	if (lis3_dev.whoami == WAI_8B && lis3_dev.idev &&
>+	if (lis3_dev.pdata && lis3_dev.whoami == WAI_8B && lis3_dev.idev
>&&
> 	    lis3_dev.idev->input->users)
> 		return IRQ_WAKE_THREAD;
> 	return IRQ_HANDLED;
>@@ -742,7 +742,7 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
> 	 * io-apic is not configurable (and generates a warning) but I
>keep it
> 	 * in case of support for other hardware.
> 	 */
>-	if (dev->whoami == WAI_8B)
>+	if (dev->pdata && dev->whoami == WAI_8B)
> 		thread_fn = lis302dl_interrupt_thread1_8b;
> 	else
> 		thread_fn = NULL;
>--


With hp_accel.c irq-thread is not needed so this is ok from that point of view.
On the other hand, wakeup / click flags are not configured without platformdata
so there is no need to run irq-thread. If the pdata is not prodived at all
there is no need to IRQ thread.

Acked-by: Samu Onkalo <samu.p.onkalo@xxxxxxxxx>


èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—