Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Wednesday, May 18, 2011 11:47:44 PM Cyril Hrubis wrote: > Hi! > I've applied this patch over 2.6.39-rc3 and did couple of suspends. After > about ten of them I've got attached trace (instead of the usuall NULL > pointer dereference in complete()). > > And yes, the MMC is still broken after this change it seems that there are > more bugs in zaurus SPI drivers. Looks like corgi-bl, I think there was a patch for this I had, dunno if it was applied. Eric, was that gpio_set_value_cansleep() for corgi-bl merged ? ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Saturday, May 21, 2011 10:45:02 PM Pavel Herrmann wrote: > On Saturday, May 21, 2011 10:28:07 PM Pavel Machek wrote: > > Unfortunately that one does not apply, due to > > > > > @@ -213,6 +224,7 @@ static int __devexit max_remove(struct > > > spi_device *spi) > > > > Whitespace damage. (but if you just delete this extra newline, its > > fine). > > sorry for that one, I followed Mareks advice on sending the pach inline, > and my mail client does automatic text reflow (defaults to 78 columns). I > still have to find the correct formating settings to send these patches. > > thanks for the ACK and testing > > Pavel Use git send-email, I recall telling you this ;-) ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Saturday, May 21, 2011 10:28:07 PM Pavel Machek wrote: > Unfortunately that one does not apply, due to > > > @@ -213,6 +224,7 @@ static int __devexit max_remove(struct spi_device > > *spi) > > Whitespace damage. (but if you just delete this extra newline, its > fine). sorry for that one, I followed Mareks advice on sending the pach inline, and my mail client does automatic text reflow (defaults to 78 columns). I still have to find the correct formating settings to send these patches. thanks for the ACK and testing Pavel ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
Hi! > > I think you want to hold the mutex at the point you setup the data to be > > transferred, do the transfer, and then release the mutex once you've read > > the results of the transfer. > > oh no, not again... > this was the earliest version, not the cleaned-up one (notice the lock in > setup-transfer, which I said was unnecessary) > here is the cleaner (newest) version: Unfortunately that one does not apply, due to > @@ -213,6 +224,7 @@ static int __devexit max_remove(struct spi_device > *spi) Whitespace damage. (but if you just delete this extra newline, its fine). Other than that, it seems to work. I was playing with max a lot today, and no oops. ACK. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Fri, May 20, 2011 at 12:13:20AM +0200, Pavel Herrmann wrote:
> From bd55d6b18fa4fcb884980825b43b43df01767149 Mon Sep 17 00:00:00 2001
> From: Pavel Herrmann
> Date: Mon, 16 May 2011 14:18:18 +0200
> Subject: [PATCH] MAX: Fix Race condition causing NULL pointer exception
>
> spi_sync call uses its spi_message parameter to keep completion information,
> having this structure static is not thread-safe, potentially causing one
> thread having pointers to memory on or above other threads stack. use mutex
> to prevent multiple access
>
> Signed-off-by: Pavel Herrmann
Looks good, thanks.
Acked-by: Russell King
> ---
> drivers/hwmon/max.c | 12
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/max.c b/drivers/hwmon/max.c
> index 12a54aa..d872f57 100644
> --- a/drivers/hwmon/max.c
> +++ b/drivers/hwmon/max.c
> @@ -40,6 +40,7 @@ struct max_data {
> struct spi_transfer xfer[2];
> uint8_t *tx_buf;
> uint8_t *rx_buf;
> + struct mutexmsg_lock_mutex;
> };
>
> static int max_read(struct device *dev, int channel)
> @@ -48,6 +49,11 @@ static int max_read(struct device *dev, int channel)
> uint8_t v1, v2;
> int err;
>
> + /* spi_sync requires data not to be freed before function returns
> + * for static data, any access is dangerous, use locks
> + */
> + mutex_lock(&data->msg_lock_mutex);
> +
> data->tx_buf[0] = (channel << MAX_CTRL_SEL_SH) |
> MAX_CTRL_PD0 | MAX_CTRL_PD1 |
> MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR;
> @@ -55,12 +61,15 @@ static int max_read(struct device *dev, int channel)
> err = spi_sync(data->spi, &data->msg);
> if (err < 0) {
> dev_err(dev, "spi_sync failed with %d\n", err);
> + mutex_unlock(&data->msg_lock_mutex);
> return err;
> }
>
> v1 = data->rx_buf[0];
> v2 = data->rx_buf[1];
>
> + mutex_unlock(&data->msg_lock_mutex);
> +
> if ((v1 & 0xc0) || (v2 & 0x3f))
> return -EINVAL;
>
> @@ -176,6 +185,8 @@ static int __devinit max_probe(struct spi_device *spi)
> if (err)
> goto err_free_data;
>
> + mutex_init(&data->msg_lock_mutex);
> +
> data->spi = spi;
> spi_set_drvdata(spi, data);
>
> @@ -213,6 +224,7 @@ static int __devexit max_remove(struct spi_device
> *spi)
>
> hwmon_device_unregister(data->hwmon_dev);
> sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
> + mutex_destroy(data->msg_lock_mutex);
> kfree(data->rx_buf);
> kfree(data->tx_buf);
> kfree(data);
> --
> 1.7.5.rc3
>
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Thursday, May 19, 2011 09:31:21 PM Russell King - ARM Linux wrote:
> I'm not sure that this is right. Taking the lock around spi_sync() ensures
> that two concurrent spi_sync()s can't happen in parallel, but with this you
> could end up with another happening as soon as this lock is released -
> before you've accessed the data which was transferred.
>
> I think you want to hold the mutex at the point you setup the data to be
> transferred, do the transfer, and then release the mutex once you've read
> the results of the transfer.
oh no, not again...
this was the earliest version, not the cleaned-up one (notice the lock in
setup-transfer, which I said was unnecessary)
here is the cleaner (newest) version:
>From bd55d6b18fa4fcb884980825b43b43df01767149 Mon Sep 17 00:00:00 2001
From: Pavel Herrmann
Date: Mon, 16 May 2011 14:18:18 +0200
Subject: [PATCH] MAX: Fix Race condition causing NULL pointer exception
spi_sync call uses its spi_message parameter to keep completion information,
having this structure static is not thread-safe, potentially causing one
thread having pointers to memory on or above other threads stack. use mutex
to prevent multiple access
Signed-off-by: Pavel Herrmann
---
drivers/hwmon/max.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/max.c b/drivers/hwmon/max.c
index 12a54aa..d872f57 100644
--- a/drivers/hwmon/max.c
+++ b/drivers/hwmon/max.c
@@ -40,6 +40,7 @@ struct max_data {
struct spi_transfer xfer[2];
uint8_t *tx_buf;
uint8_t *rx_buf;
+ struct mutexmsg_lock_mutex;
};
static int max_read(struct device *dev, int channel)
@@ -48,6 +49,11 @@ static int max_read(struct device *dev, int channel)
uint8_t v1, v2;
int err;
+ /* spi_sync requires data not to be freed before function returns
+* for static data, any access is dangerous, use locks
+*/
+ mutex_lock(&data->msg_lock_mutex);
+
data->tx_buf[0] = (channel << MAX_CTRL_SEL_SH) |
MAX_CTRL_PD0 | MAX_CTRL_PD1 |
MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR;
@@ -55,12 +61,15 @@ static int max_read(struct device *dev, int channel)
err = spi_sync(data->spi, &data->msg);
if (err < 0) {
dev_err(dev, "spi_sync failed with %d\n", err);
+ mutex_unlock(&data->msg_lock_mutex);
return err;
}
v1 = data->rx_buf[0];
v2 = data->rx_buf[1];
+ mutex_unlock(&data->msg_lock_mutex);
+
if ((v1 & 0xc0) || (v2 & 0x3f))
return -EINVAL;
@@ -176,6 +185,8 @@ static int __devinit max_probe(struct spi_device *spi)
if (err)
goto err_free_data;
+ mutex_init(&data->msg_lock_mutex);
+
data->spi = spi;
spi_set_drvdata(spi, data);
@@ -213,6 +224,7 @@ static int __devexit max_remove(struct spi_device
*spi)
hwmon_device_unregister(data->hwmon_dev);
sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
+ mutex_destroy(data->msg_lock_mutex);
kfree(data->rx_buf);
kfree(data->tx_buf);
kfree(data);
--
1.7.5.rc3
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Thu, May 19, 2011 at 02:51:40PM +0200, Pavel Herrmann wrote: > @@ -52,7 +53,14 @@ static int max_read(struct device *dev, int channel) > MAX_CTRL_PD0 | MAX_CTRL_PD1 | > MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR; > > + /* spi_sync requires data not to be freed before function returns > + * for static data, any access is dangerous, use locks > + */ > + mutex_lock(&data->msg_lock_mutex); > + > err = spi_sync(data->spi, &data->msg); > + > + mutex_unlock(&data->msg_lock_mutex); I'm not sure that this is right. Taking the lock around spi_sync() ensures that two concurrent spi_sync()s can't happen in parallel, but with this you could end up with another happening as soon as this lock is released - before you've accessed the data which was transferred. I think you want to hold the mutex at the point you setup the data to be transferred, do the transfer, and then release the mutex once you've read the results of the transfer. ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
Hi
On Thursday, May 19, 2011 02:35:08 PM Pavel Machek wrote:
> > you're potentially doing with this:
> In some other mail, you said "just add the locking". Pavel H.
> actually produced patch doing so...
yes, that was the original version of the patch. while I agree with Marek on
locks not being the right way, I was going send a cleaner version of the
original locking patch today (locking in probe is not really necessary), you
just beat me to it.
Pavel Herrmann
From 5782df40e6c343b6dc9ff5ca008c1ee8b50cad51 Mon Sep 17 00:00:00 2001
From: Pavel Herrmann
Date: Mon, 16 May 2011 14:18:18 +0200
Subject: [PATCH 1/2] Fix NULL pointer exception in max
Signed-off-by: Pavel Herrmann
---
drivers/hwmon/max.c | 15 +++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/max.c b/drivers/hwmon/max.c
index 12a54aa..c748fa3 100644
--- a/drivers/hwmon/max.c
+++ b/drivers/hwmon/max.c
@@ -40,6 +40,7 @@ struct max_data {
struct spi_transfer xfer[2];
uint8_t *tx_buf;
uint8_t *rx_buf;
+ struct mutex msg_lock_mutex;
};
static int max_read(struct device *dev, int channel)
@@ -52,7 +53,14 @@ static int max_read(struct device *dev, int channel)
MAX_CTRL_PD0 | MAX_CTRL_PD1 |
MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR;
+ /* spi_sync requires data not to be freed before function returns
+ * for static data, any access is dangerous, use locks
+ */
+ mutex_lock(&data->msg_lock_mutex);
+
err = spi_sync(data->spi, &data->msg);
+
+ mutex_unlock(&data->msg_lock_mutex);
if (err < 0) {
dev_err(dev, "spi_sync failed with %d\n", err);
return err;
@@ -138,6 +146,8 @@ static int setup_transfer(struct max_data *data)
return -ENOMEM;
}
+ mutex_lock(&data->msg_lock_mutex);
+
m = &data->msg;
x = &data->xfer[0];
@@ -152,6 +162,8 @@ static int setup_transfer(struct max_data *data)
x->len = 2;
spi_message_add_tail(x, m);
+ mutex_unlock(&data->msg_lock_mutex);
+
return 0;
}
@@ -172,6 +184,8 @@ static int __devinit max_probe(struct spi_device *spi)
return -ENOMEM;
}
+ mutex_init(&data->msg_lock_mutex);
+
err = setup_transfer(data);
if (err)
goto err_free_data;
@@ -213,6 +227,7 @@ static int __devexit max_remove(struct spi_device *spi)
hwmon_device_unregister(data->hwmon_dev);
sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
+ mutex_destroy(data->msg_lock_mutex);
kfree(data->rx_buf);
kfree(data->tx_buf);
kfree(data);
--
1.7.5.rc1
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Thursday, May 19, 2011 02:51:40 PM Pavel Herrmann wrote: > Hi > > On Thursday, May 19, 2011 02:35:08 PM Pavel Machek wrote: > > > you're potentially doing with this: > > In some other mail, you said "just add the locking". Pavel H. > > actually produced patch doing so... > > yes, that was the original version of the patch. while I agree with Marek > on locks not being the right way, I was going send a cleaner version of > the original locking patch today (locking in probe is not really > necessary), you just beat me to it. Please send the patch inline next time ;-) > > Pavel Herrmann ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
Hi!
On Wed 2011-05-18 16:29:35, Russell King - ARM Linux wrote:
> On Wed, May 18, 2011 at 05:18:38PM +0200, Pavel Herrmann wrote:
> > spi_sync call uses its spi_message parameter to keep completion information,
> > having this structure static is not thread-safe, potentially causing one
> > thread having pointers to memory on or above other threads stack. use
> > per-call spi_message on stack to fix this
>
> I assume this has not been tested with DMA debugging enabled.
>
> The DMA API does not like mapping memory from the stack, which is what
> you're potentially doing with this:
In some other mail, you said "just add the locking". Pavel H.
actually produced patch doing so...
From: Pavel Herrmann
To: Marek Vasut
>From 14063b017123233a8b56d6706a9ff046a791eaf4 Mon Sep 17 00:00:00 2001
From: Pavel Herrmann
Date: Mon, 16 May 2011 14:18:18 +0200
Subject: [PATCH] Fix NULL pointer exception in max
Signed-off-by: Pavel Herrmann
---
drivers/hwmon/max.c | 16
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/max.c b/drivers/hwmon/max.c
index 12a54aa..7be50e5 100644
--- a/drivers/hwmon/max.c
+++ b/drivers/hwmon/max.c
@@ -40,6 +40,7 @@ struct max_data {
struct spi_transfer xfer[2];
uint8_t *tx_buf;
uint8_t *rx_buf;
+ struct mutexmsg_lock_mutex;
};
static int max_read(struct device *dev, int channel)
@@ -48,6 +49,11 @@ static int max_read(struct device *dev, int channel)
uint8_t v1, v2;
int err;
+ /* spi_sync requires data not to be freed before function returns
+* for static data, any access is dangerous, use locks
+*/
+ mutex_lock(&data->msg_lock_mutex);
+
data->tx_buf[0] = (channel << MAX_CTRL_SEL_SH) |
MAX_CTRL_PD0 | MAX_CTRL_PD1 |
MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR;
@@ -55,12 +61,15 @@ static int max_read(struct device *dev, int channel)
err = spi_sync(data->spi, &data->msg);
if (err < 0) {
dev_err(dev, "spi_sync failed with %d\n", err);
+ mutex_unlock(&data->msg_lock_mutex);
return err;
}
v1 = data->rx_buf[0];
v2 = data->rx_buf[1];
+ mutex_unlock(&data->msg_lock_mutex);
+
if ((v1 & 0xc0) || (v2 & 0x3f))
return -EINVAL;
@@ -138,6 +147,8 @@ static int setup_transfer(struct max_data *data)
return -ENOMEM;
}
+ mutex_lock(&data->msg_lock_mutex);
+
m = &data->msg;
x = &data->xfer[0];
@@ -152,6 +163,8 @@ static int setup_transfer(struct max_data *data)
x->len = 2;
spi_message_add_tail(x, m);
+ mutex_unlock(&data->msg_lock_mutex);
+
return 0;
}
@@ -172,6 +185,8 @@ static int __devinit max_probe(struct spi_device *spi)
return -ENOMEM;
}
+ mutex_init(&data->msg_lock_mutex);
+
err = setup_transfer(data);
if (err)
goto err_free_data;
@@ -213,6 +228,7 @@ static int __devexit max_remove(struct spi_device *spi)
hwmon_device_unregister(data->hwmon_dev);
sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
+ mutex_destroy(data->msg_lock_mutex);
kfree(data->rx_buf);
kfree(data->tx_buf);
kfree(data);
--
1.7.5.rc1
- End forwarded message -
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Wed, May 18, 2011 at 07:36:54PM +0200, Marek Vasut wrote: > > On Wed, May 18, 2011 at 05:18:38PM +0200, Pavel Herrmann wrote: > > > spi_sync call uses its spi_message parameter to keep completion > > > information, having this structure static is not thread-safe, > > > potentially causing one thread having pointers to memory on or above > > > other threads stack. use per-call spi_message on stack to fix this > > > > I assume this has not been tested with DMA debugging enabled. > > > > The DMA API does not like mapping memory from the stack, which is what > > you're potentially doing with this: > > Yikes, good catch, but kmallocing this and kfreeing it again is not > something I'd like to see either. You could use a semaphore to protect against other threads. However, this driver just gives us yet more problems, as it overlaps the DMA'd data with the DMA metadata (spi message/spi transfer structures.) And yes we do get bug reports on that too... I think its about time driver and subsystem authors got a clue about DMA incoherent architectures, and these things called 'cache lines' which have a direct impact on whether code is buggy or not. Sharing cache lines between DMA buffers and other data is Really Bad News for data integrity - even sharing a cache line between two DMA buffers can be a problem. ___ Zaurus-devel mailing list Zaurus-devel@lists.linuxtogo.org http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
Hi! I've applied this patch over 2.6.39-rc3 and did couple of suspends. After about ten of them I've got attached trace (instead of the usuall NULL pointer dereference in complete()). And yes, the MMC is still broken after this change it seems that there are more bugs in zaurus SPI drivers. -- metan PM: Syncing filesystems ... done. mmc0: card 0002 removed Freezing user space processes ... (elapsed 0.03 seconds) done. Freezing remaining freezable tasks ... (elapsed 0.07 seconds) done. Suspending console(s) (use no_console_suspend to debug) [ cut here ] WARNING: at lib/kref.c:34 kref_get+0x20/0x44() Modules linked in: ---[ end trace 435fc8b0048da5e9 ]--- sd 0:0:0:0: [sda] Stopping disk [ cut here ] WARNING: at lib/kref.c:34 kref_get+0x20/0x44() Modules linked in: ---[ end trace 435fc8b0048da5ea ]--- PM: suspend of devices complete after 81.489 msecs [ cut here ] WARNING: at lib/kref.c:34 kref_get+0x20/0x44() Modules linked in: ---[ end trace 435fc8b0048da5eb ]--- PM: late suspend of devices complete after 1.993 msecs [ cut here ] WARNING: at lib/kref.c:34 kref_get+0x20/0x44() Modules linked in: ---[ end trace 435fc8b0048da5ec ]--- PM: early resume of devices complete after 728.029 msecs [ cut here ] WARNING: at lib/kref.c:34 kref_get+0x20/0x44() Modules linked in: ---[ end trace 435fc8b0048da5ed ]--- sd 0:0:0:0: [sda] Starting disk PM: resume of devices complete after 232.192 msecs Restarting tasks ... done. mmc0: error -95 whilst initialising SD card PM: Syncing filesystems ... done. haruka-chan login: root kernel BUG at kernel/cred.c:171! Unable to handle kernel NULL pointer dereference at virtual address pgd = c399c000 [] *pgd=a39ac831, *pte=, *ppte= Internal error: Oops: 817 [#1] PREEMPT last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness Modules linked in: CPU: 0Tainted: GW(2.6.39-rc7-00259-g09c1ce4 #14) PC is at __bug+0x18/0x24 LR is at __bug+0x14/0x24 pc : []lr : []psr: 4013 sp : c3bc5f78 ip : fp : bef2ea2c r10: 0002 r9 : c3bc4000 r8 : fffe r7 : ff9c r6 : c38d2d80 r5 : r4 : c38d2d80 r3 : r2 : c3bc5f6c r1 : c02d6e9b r0 : 0027 Flags: nZcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 397f Table: a399c000 DAC: 0015 Process login (pid: 1105, stack limit = 0xc3bc4278) Stack: (0xc3bc5f78 to 0xc3bc6000) 5f60: c38d2d80 c0054c2c 5f80: c38d2d80 c009c0e0 ff9c 400e6c14 0001 0021 5fa0: c0023184 c0023000 0001 400e6c14 400f0f18 5fc0: 0001 0021 400f1000 400f1000 fdc8 bef2ea2c 5fe0: bef2e9b8 400e0ed8 400e247c 6010 400e6c14 Code: e1a01000 e59f000c eb08ff53 e3a03000 (e5833000) Unable to handle kernel NULL pointer dereference at virtual address 0002 pgd = c397c000 [0002] *pgd=a3b71831, *pte=, *ppte= Internal error: Oops: f3 [#2] PREEMPT last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness Modules linked in: CPU: 0Tainted: G D W(2.6.39-rc7-00259-g09c1ce4 #14) PC is at kmem_cache_alloc+0x2c/0x78 LR is at scsi_pool_alloc_command+0x38/0x60 pc : []lr : []psr: 2093 sp : c388bd88 ip : fp : ec30 r10: c38f r9 : c38bd0b8 r8 : 1000 r7 : 0002 r6 : 0020 r5 : a093 r4 : c3802100 r3 : c03c8018 r2 : r1 : 0020 r0 : c3802100 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 397f Table: a397c000 DAC: 0017 Process sync_supers (pid: 139, stack limit = 0xc388a278) Stack: (0xc388bd88 to 0xc388c000) bd80: c0329330 c380fdc0 0020 0020 1000 c0172ed8 bda0: c38bd000 0020 c38bc800 c0172f38 c38bd000 c38bc800 c38bd0b8 c0173010 bdc0: c38bd000 c38bd0b8 c38bd0b8 c01730e4 c38f4a90 c38bd000 003f bde0: 1000 c0178834 c38f4a90 c38bd000 c393c340 c0181918 c380fd20 c38f80a4 be00: 1000 c38f9638 c393c340 0001 c38f4a90 c38f4ad8 0007 c0117d64 be20: c38f4a90 c38f4a90 c38f4a90 c393c340 c393c340 c38bd0b8 0008 be40: ec30 c01197f8 c38bd000 c38bc800 c38bd024 c388a000 c393c340 c0177e44 be60: c393c340 c393c498 00012a00 c393c340 c3b1fc00 0003 0091 c393c340 be80: 0001 0008 ec30 c0118060 c393c340 c011ad38 c393c340 c38f4a90 bea0: c3b1fc00 c388a000 c388beb8 c393c340 c0119150 c3896b00 c388a000 bec0: c388beec c0265b20 c388a000 c3854aa0 00011210 c388bef4 bee0: c3896b00 c3b1fc00 0008 0001 0001 c0322fdc bf00: c01192d8 c388bf40 c3408000 c3854a60 0091 0010 0001 bf20: c00c85fc c3b1fc00 c00c86b4 0010 000f c3408000 c3408000 bf40: c3b1fc00 0091
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
> On Wed, May 18, 2011 at 05:18:38PM +0200, Pavel Herrmann wrote:
> > spi_sync call uses its spi_message parameter to keep completion
> > information, having this structure static is not thread-safe,
> > potentially causing one thread having pointers to memory on or above
> > other threads stack. use per-call spi_message on stack to fix this
>
> I assume this has not been tested with DMA debugging enabled.
>
> The DMA API does not like mapping memory from the stack, which is what
> you're potentially doing with this:
Yikes, good catch, but kmallocing this and kfreeing it again is not something
I'd like to see either.
What other options do you suggest?
Btw note, this isn't the only driver doing this, maybe we have a horde of
patches on the way?
>
> > + uint8_t rx_buf[2] = {0, 0};
> > + uint8_t tx_buf = (channel << MAX_CTRL_SEL_SH) |
> > + MAX_CTRL_PD0 | MAX_CTRL_PD1 |
> > + MAX_CTRL_SGL | MAX_CTRL_UNI |
> > + MAX_CTRL_STR;
> > +
> > + spi_message_init(&m);
> > + memset(t, 0, sizeof(t));
> > +
> > + t[0].tx_buf = &tx_buf;
> > + t[0].len = 1;
> > + spi_message_add_tail(&t[0], &m);
> > +
> > + t[1].rx_buf = rx_buf;
> > + t[1].len = 2;
> > + spi_message_add_tail(&t[1], &m);
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Wed, May 18, 2011 at 05:18:38PM +0200, Pavel Herrmann wrote:
> spi_sync call uses its spi_message parameter to keep completion information,
> having this structure static is not thread-safe, potentially causing one
> thread having pointers to memory on or above other threads stack. use
> per-call spi_message on stack to fix this
I assume this has not been tested with DMA debugging enabled.
The DMA API does not like mapping memory from the stack, which is what
you're potentially doing with this:
> + uint8_t rx_buf[2] = {0, 0};
> + uint8_t tx_buf = (channel << MAX_CTRL_SEL_SH) |
> + MAX_CTRL_PD0 | MAX_CTRL_PD1 |
> + MAX_CTRL_SGL | MAX_CTRL_UNI |
> + MAX_CTRL_STR;
> +
> + spi_message_init(&m);
> + memset(t, 0, sizeof(t));
> +
> + t[0].tx_buf = &tx_buf;
> + t[0].len = 1;
> + spi_message_add_tail(&t[0], &m);
> +
> + t[1].rx_buf = rx_buf;
> + t[1].len = 2;
> + spi_message_add_tail(&t[1], &m);
___
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel
Re: [Zaurus-devel] [PATCH] MAX1111: Fix race condition causing NULL pointer exception
On Wed, May 18, 2011 at 11:18 PM, Pavel Herrmann
wrote:
> spi_sync call uses its spi_message parameter to keep completion information,
> having this structure static is not thread-safe, potentially causing one
> thread having pointers to memory on or above other threads stack. use
> per-call spi_message on stack to fix this
>
> Signed-off-by: Pavel Herrmann
> Signed-off-by: Marek Vasut
OK
> ---
> drivers/hwmon/max.c | 86
> +--
> 1 files changed, 24 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/hwmon/max.c b/drivers/hwmon/max.c
> index 12a54aa..6422baf 100644
> --- a/drivers/hwmon/max.c
> +++ b/drivers/hwmon/max.c
> @@ -22,9 +22,6 @@
> #include
> #include
>
> -#define MAX_TX_BUF_SIZE 1
> -#define MAX_RX_BUF_SIZE 2
> -
> /* MAX Commands */
> #define MAX_CTRL_PD0 (1u << 0)
> #define MAX_CTRL_PD1 (1u << 1)
> @@ -36,35 +33,41 @@
> struct max_data {
> struct spi_device *spi;
> struct device *hwmon_dev;
> - struct spi_message msg;
> - struct spi_transfer xfer[2];
> - uint8_t *tx_buf;
> - uint8_t *rx_buf;
> };
>
> static int max_read(struct device *dev, int channel)
> {
> - struct max_data *data = dev_get_drvdata(dev);
> - uint8_t v1, v2;
> int err;
> -
> - data->tx_buf[0] = (channel << MAX_CTRL_SEL_SH) |
> - MAX_CTRL_PD0 | MAX_CTRL_PD1 |
> - MAX_CTRL_SGL | MAX_CTRL_UNI | MAX_CTRL_STR;
> -
> - err = spi_sync(data->spi, &data->msg);
> + struct max_data *data = dev_get_drvdata(dev);
> + struct spi_message m;
> + struct spi_transfer t[2];
> + uint8_t rx_buf[2] = {0, 0};
> + uint8_t tx_buf = (channel << MAX_CTRL_SEL_SH) |
> + MAX_CTRL_PD0 | MAX_CTRL_PD1 |
> + MAX_CTRL_SGL | MAX_CTRL_UNI |
> + MAX_CTRL_STR;
> +
> + spi_message_init(&m);
> + memset(t, 0, sizeof(t));
> +
> + t[0].tx_buf = &tx_buf;
> + t[0].len = 1;
> + spi_message_add_tail(&t[0], &m);
> +
> + t[1].rx_buf = rx_buf;
> + t[1].len = 2;
> + spi_message_add_tail(&t[1], &m);
> +
> + err = spi_sync(data->spi, &m);
> if (err < 0) {
> dev_err(dev, "spi_sync failed with %d\n", err);
> return err;
> }
>
> - v1 = data->rx_buf[0];
> - v2 = data->rx_buf[1];
> -
> - if ((v1 & 0xc0) || (v2 & 0x3f))
> + if ((rx_buf[0] & 0xc0) || (rx_buf[1] & 0x3f))
> return -EINVAL;
>
> - return (v1 << 2) | (v2 >> 6);
> + return (rx_buf[0] << 2) | (rx_buf[1] >> 6);
> }
>
> #ifdef CONFIG_SHARPSL_PM
> @@ -123,38 +126,6 @@ static const struct attribute_group max_attr_group =
> {
> .attrs = max_attributes,
> };
>
> -static int setup_transfer(struct max_data *data)
> -{
> - struct spi_message *m;
> - struct spi_transfer *x;
> -
> - data->tx_buf = kmalloc(MAX_TX_BUF_SIZE, GFP_KERNEL);
> - if (!data->tx_buf)
> - return -ENOMEM;
> -
> - data->rx_buf = kmalloc(MAX_RX_BUF_SIZE, GFP_KERNEL);
> - if (!data->rx_buf) {
> - kfree(data->tx_buf);
> - return -ENOMEM;
> - }
> -
> - m = &data->msg;
> - x = &data->xfer[0];
> -
> - spi_message_init(m);
> -
> - x->tx_buf = &data->tx_buf[0];
> - x->len = 1;
> - spi_message_add_tail(x, m);
> -
> - x++;
> - x->rx_buf = &data->rx_buf[0];
> - x->len = 2;
> - spi_message_add_tail(x, m);
> -
> - return 0;
> -}
> -
> static int __devinit max_probe(struct spi_device *spi)
> {
> struct max_data *data;
> @@ -172,17 +143,13 @@ static int __devinit max_probe(struct spi_device
> *spi)
> return -ENOMEM;
> }
>
> - err = setup_transfer(data);
> - if (err)
> - goto err_free_data;
> -
> data->spi = spi;
> spi_set_drvdata(spi, data);
>
> err = sysfs_create_group(&spi->dev.kobj, &max_attr_group);
> if (err) {
> dev_err(&spi->dev, "failed to create attribute group\n");
> - goto err_free_all;
> + goto err_free_data;
> }
>
> data->hwmon_dev = hwmon_device_register(&spi->dev);
> @@ -199,9 +166,6 @@ static int __devinit max_probe(struct spi_device *spi)
>
> err_remove:
> sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
> -err_free_all:
> - kfree(data->rx_buf);
> - kfree(data->tx_buf);
> err_free_data:
> kfree(data);
> return err;
> @@ -213,8 +177,6 @@ static int __devexit max_remove(struct spi_device
> *spi)
>
> hwmon_device_unregister(data->hwmon_dev);
> sysfs_remove_group(&spi->dev.kobj, &max_attr_group);
> - kfree(data->rx_
