Re: [lm-sensors] [PATCH resent] lis3: Fix Oops with NULL platform data

[Guenter Roeck <guenter.roeck@ericsson.com>]

On Thu, Sep 23, 2010 at 01:06:22AM -0400, samu.p.onkalo@nokia.com wrote:
> Added lm-sensors list...
> 
> >-----Original Message-----
> >From: ext Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: 22 September, 2010 16:47
> >To: Onkalo Samu.P (Nokia-MS/Tampere)
> >Cc: akpm@linux-foundation.org; Eric.Piel@tremplin-utc.net; linux-
> >kernel@vger.kernel.org
> >Subject: Re: [PATCH resent] lis3: Fix Oops with NULL platform data
> >
> >At Wed, 22 Sep 2010 13:56:15 +0200,
> ><samu.p.onkalo@nokia.com> wrote:
> >>
> >>
> >> Hi
> >>
> >> >-----Original Message-----
> >> >From: ext Takashi Iwai [mailto:tiwai@suse.de]
> >> >Sent: 22 September, 2010 14:34
> >> >To: Andrew Morton
> >> >Cc: Onkalo Samu.P (Nokia-MS/Tampere); Éric Piel; linux-
> >> >kernel@vger.kernel.org
> >> >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@kernel.org>
> >> >Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >> >---
> >> >
> >> >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@nokia.com>
> >>
> >> -Samu
> >
> >
> >thanks,
> >
> >Takashi
> >
> >===
> >From 0e5bdc7516915e92fe471a77da36ecb75d995c0f Mon Sep 17 00:00:00 2001
> >From: Takashi Iwai <tiwai@suse.de>
> >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@suse.de>
> >---
> >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@nokia.com>
> 
Looks ok to me.

Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>