Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
Hi Shimoda-san,
On Tue, Mar 12, 2019 at 4:17 AM Yoshihiro Shimoda
wrote:
> From: Phong Hoang
>
> This patch fixes deadlock warning if removing PWM device
> when CONFIG_PROVE_LOCKING is enabled.
>
> This issue can be reproceduced by the following steps on
> the R-Car H3 Salvator-X board if the backlight is disabled:
>
> # cd /sys/class/pwm/pwmchip0
> # echo 0 > export
> # ls
> device export npwm power pwm0 subsystem uevent unexport
> # cd device/driver
> # ls
> bind e6e31000.pwm uevent unbind
> # echo e6e31000.pwm > unbind
>
> [ 87.659974] ==
> [ 87.666149] WARNING: possible circular locking dependency detected
> [ 87.672327] 5.0.0 #7 Not tainted
[...]
> This warning occurs because pwmchip_remove still keeps pwm_lock
> when removing sysfs. That's why it leads to that conflict.
> Hence, this patch unlocks pwm_lock before removing sysfs.
> Also, pwmchip_sysfs_export() doesn't seem to need the pwm_lock
> held so that to achieve consistance between export and
> unexport this patch also modifies it.
>
> Signed-off-by: Phong Hoang
> [shimoda: revise the commit log and code]
> Fixes: 76abbdde2d95 ("pwm: Add sysfs interface")
> Signed-off-by: Yoshihiro Shimoda
> Tested-by: Hoan Nguyen An
Thanks for your patch!
Looks good to me: class_find_device() seems to use its own locking in
the class to protect against concurrent modification.
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote: > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 1581f6a..2fdd6611 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -311,10 +311,12 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, > if (IS_ENABLED(CONFIG_OF)) > of_pwmchip_add(chip); > > - pwmchip_sysfs_export(chip); > - > out: > mutex_unlock(&pwm_lock); > + > + if (!ret) > + pwmchip_sysfs_export(chip); > + > return ret; > } > EXPORT_SYMBOL_GPL(pwmchip_add_with_polarity); > @@ -368,10 +370,12 @@ int pwmchip_remove(struct pwm_chip *chip) > > free_pwms(chip); > > - pwmchip_sysfs_unexport(chip); > - > out: > mutex_unlock(&pwm_lock); > + > + if (!ret) > + pwmchip_sysfs_unexport(chip); > + I wonder if this needs to be done before free_pwms is called. Otherwise the pwmchip is already gone and then something is requested via sysfs. Also a comment about why it is important not to unexport while holding the lock would be great. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
Hi Uwe,
On Tue, Mar 12, 2019 at 10:23 AM Uwe Kleine-König
wrote:
> On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 1581f6a..2fdd6611 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -368,10 +370,12 @@ int pwmchip_remove(struct pwm_chip *chip)
> >
> > free_pwms(chip);
> >
> > - pwmchip_sysfs_unexport(chip);
> > -
> > out:
> > mutex_unlock(&pwm_lock);
> > +
> > + if (!ret)
> > + pwmchip_sysfs_unexport(chip);
> > +
>
> I wonder if this needs to be done before free_pwms is called. Otherwise
> the pwmchip is already gone and then something is requested via sysfs.
The chip itself is not freed, only the pwms array inside, which is not needed
for matching in pwmchip_sysfs_unexport(), right?
> Also a comment about why it is important not to unexport while holding
> the lock would be great.
Thanks, good idea!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
On Tue, Mar 12, 2019 at 10:49:59AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Tue, Mar 12, 2019 at 10:23 AM Uwe Kleine-König > wrote: > > On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote: > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > index 1581f6a..2fdd6611 100644 > > > --- a/drivers/pwm/core.c > > > +++ b/drivers/pwm/core.c > > > > @@ -368,10 +370,12 @@ int pwmchip_remove(struct pwm_chip *chip) > > > > > > free_pwms(chip); > > > > > > - pwmchip_sysfs_unexport(chip); > > > - > > > out: > > > mutex_unlock(&pwm_lock); > > > + > > > + if (!ret) > > > + pwmchip_sysfs_unexport(chip); > > > + > > > > I wonder if this needs to be done before free_pwms is called. Otherwise > > the pwmchip is already gone and then something is requested via sysfs. > > The chip itself is not freed, only the pwms array inside, which is not needed > for matching in pwmchip_sysfs_unexport(), right? OK, then make this: I wonder if pwmchip_sysfs_unexport needs to be done before free_pwms is called. Otherwise the PWMs's representation is already gone and then something might be requested via sysfs. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 2/2] mtd: spi-nor: refine Spansion S25FL512S ID
On Tue, Mar 5, 2019 at 3:05 PM Geert Uytterhoeven wrote:
> On Wed, Jan 16, 2019 at 6:53 PM Sergei Shtylyov
> wrote:
> > Spansion S25FL512S ID is erroneously using 5-byte JEDEC ID, while the chip
> > family ID is stored in the 6th byte. Due to using only 5-byte ID, it's also
> > covering S25FS512S and now that we have added 6-byte ID for that chip, we
> > can convert S25FL512S to using a proper 6-byte ID as well...
> >
> > Signed-off-by: Sergei Shtylyov
>
> This is now commit a2126b0a010905e5 ("mtd: spi-nor: refine Spansion
> S25FL512S ID"), and turns out to cause a regression on r8a7791/koelsch.
> Dmesg diff before/after:
>
> -m25p80 spi0.0: s25fl512s (65536 Kbytes)
> -3 fixed-partitions partitions found on MTD device spi0.0
> -Creating 3 MTD partitions on "spi0.0":
> -0x-0x0008 : "loader"
> -0x0008-0x0060 : "user"
> -0x0060-0x0400 : "flash"
> +m25p80 spi0.0: unrecognized JEDEC id bytes: 01, 02, 20
>
> As the (old) U-Boot on my Koelsch keeps many module clocks enabled, I
> typically merge in my topic/renesas-debug branch, which makes sure all
> non-critical module clocks are disabled early during boot, to catch
> drivers not properly implementing Runtime PM.
>
> However, it turns out this has some impact on JEDEC ID detection:
> - When module clocks are left untouched, spi_nor_read_id() reads
> 0x01:0x02:0x20:0x4d:0x00:0x80.
> - When my debug code has disabled module clocks during early boot,
> The last byte is 0x00.
>
> Before the above commit, only the first 5 bytes were compared, and the
> last byte was ignored, thus not causing problems.
> When comparing all 6 bytes, detection fails if the last byte is 0x00.
>
> I believe mainline U-Boot for R-Car Gen2 boards doesn't keep the QSPI
> module clock enabled, so this commit may breaks those boards.
>
> To be investigated more (e.g. with a logic analyzer)...
The FLASH returns the correct data, but it is not received correctly.
When reading 16 bytes, thus including the ASCII model at offset 6/7:
- actual: 01 02 20 4d 00 80 47 31 82 ff ff ff ff ff ff ff
- received: 01 02 20 4d 00 00 8e 63 05 ff ff ff ff ff ff fe
When retrying the operation, data is received correctly.
This happens due to a Runtime PM related bug in the initialization code
of the spi-rspi driver: if the module clock is not running (disabled by
my debug code, the boot loader, or the clk_disable_unused late
initcall), the SPI controller is not initialized properly. The first
transfer still manages to read some correct data, which used to be
sufficient to identify the S25FL512S FLASH chip, before the sixth byte
was also considered.
Will send a quick fix, and a proper solution requiring more refactoring
later.
Sorry for the noise...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
On Tue, Mar 12, 2019 at 10:54:33AM +0100, Uwe Kleine-König wrote: > On Tue, Mar 12, 2019 at 10:49:59AM +0100, Geert Uytterhoeven wrote: > > Hi Uwe, > > > > On Tue, Mar 12, 2019 at 10:23 AM Uwe Kleine-König > > wrote: > > > On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote: > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > > index 1581f6a..2fdd6611 100644 > > > > --- a/drivers/pwm/core.c > > > > +++ b/drivers/pwm/core.c > > > > > > @@ -368,10 +370,12 @@ int pwmchip_remove(struct pwm_chip *chip) > > > > > > > > free_pwms(chip); > > > > > > > > - pwmchip_sysfs_unexport(chip); > > > > - > > > > out: > > > > mutex_unlock(&pwm_lock); > > > > + > > > > + if (!ret) > > > > + pwmchip_sysfs_unexport(chip); > > > > + > > > > > > I wonder if this needs to be done before free_pwms is called. Otherwise > > > the pwmchip is already gone and then something is requested via sysfs. > > > > The chip itself is not freed, only the pwms array inside, which is not > > needed > > for matching in pwmchip_sysfs_unexport(), right? > > OK, then make this: > > I wonder if pwmchip_sysfs_unexport needs to be done before free_pwms is > called. Otherwise the PWMs's representation is already gone and then > something might be requested via sysfs. Agreed, I think sysfs needs to disappear before the chip does, otherwise we could have userspace racing with the kernel for access to sysfs while the PWM chip is already/only halfway gone. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote:
> From: Phong Hoang
>
> This patch fixes deadlock warning if removing PWM device
> when CONFIG_PROVE_LOCKING is enabled.
>
> This issue can be reproceduced by the following steps on
> the R-Car H3 Salvator-X board if the backlight is disabled:
>
> # cd /sys/class/pwm/pwmchip0
> # echo 0 > export
> # ls
> device export npwm power pwm0 subsystem uevent unexport
> # cd device/driver
> # ls
> bind e6e31000.pwm uevent unbind
> # echo e6e31000.pwm > unbind
>
> [ 87.659974] ==
> [ 87.666149] WARNING: possible circular locking dependency detected
> [ 87.672327] 5.0.0 #7 Not tainted
> [ 87.675549] --
> [ 87.681723] bash/2986 is trying to acquire lock:
> [ 87.686337] 5ea0e178 (kn->count#58){}, at:
> kernfs_remove_by_name_ns+0x50/0xa0
> [ 87.694528]
> [ 87.694528] but task is already holding lock:
> [ 87.700353] 6313b17c (pwm_lock){+.+.}, at:
> pwmchip_remove+0x28/0x13c
> [ 87.707405]
> [ 87.707405] which lock already depends on the new lock.
> [ 87.707405]
> [ 87.715574]
> [ 87.715574] the existing dependency chain (in reverse order) is:
> [ 87.723048]
> [ 87.723048] -> #1 (pwm_lock){+.+.}:
> [ 87.728017]__mutex_lock+0x70/0x7e4
> [ 87.732108]mutex_lock_nested+0x1c/0x24
> [ 87.736547]pwm_request_from_chip.part.6+0x34/0x74
> [ 87.741940]pwm_request_from_chip+0x20/0x40
> [ 87.746725]export_store+0x6c/0x1f4
> [ 87.750820]dev_attr_store+0x18/0x28
> [ 87.754998]sysfs_kf_write+0x54/0x64
> [ 87.759175]kernfs_fop_write+0xe4/0x1e8
> [ 87.763615]__vfs_write+0x40/0x184
> [ 87.767619]vfs_write+0xa8/0x19c
> [ 87.771448]ksys_write+0x58/0xbc
> [ 87.775278]__arm64_sys_write+0x18/0x20
> [ 87.779721]el0_svc_common+0xd0/0x124
> [ 87.783986]el0_svc_compat_handler+0x1c/0x24
> [ 87.788858]el0_svc_compat+0x8/0x18
> [ 87.792947]
> [ 87.792947] -> #0 (kn->count#58){}:
> [ 87.798260]lock_acquire+0xc4/0x22c
> [ 87.802353]__kernfs_remove+0x258/0x2c4
> [ 87.806790]kernfs_remove_by_name_ns+0x50/0xa0
> [ 87.811836]remove_files.isra.1+0x38/0x78
> [ 87.816447]sysfs_remove_group+0x48/0x98
> [ 87.820971]sysfs_remove_groups+0x34/0x4c
> [ 87.825583]device_remove_attrs+0x6c/0x7c
> [ 87.830197]device_del+0x11c/0x33c
> [ 87.834201]device_unregister+0x14/0x2c
> [ 87.838638]pwmchip_sysfs_unexport+0x40/0x4c
> [ 87.843509]pwmchip_remove+0xf4/0x13c
> [ 87.847773]rcar_pwm_remove+0x28/0x34
> [ 87.852039]platform_drv_remove+0x24/0x64
> [ 87.856651]device_release_driver_internal+0x18c/0x21c
> [ 87.862391]device_release_driver+0x14/0x1c
> [ 87.867175]unbind_store+0xe0/0x124
> [ 87.871265]drv_attr_store+0x20/0x30
> [ 87.875442]sysfs_kf_write+0x54/0x64
> [ 87.879618]kernfs_fop_write+0xe4/0x1e8
> [ 87.884055]__vfs_write+0x40/0x184
> [ 87.888057]vfs_write+0xa8/0x19c
> [ 87.891887]ksys_write+0x58/0xbc
> [ 87.895716]__arm64_sys_write+0x18/0x20
> [ 87.900154]el0_svc_common+0xd0/0x124
> [ 87.904417]el0_svc_compat_handler+0x1c/0x24
> [ 87.909289]el0_svc_compat+0x8/0x18
I'm not sure I understand this correctly. The above looks like pwm_lock
is held as part of the syscall writing 0 to the export attribute and the
second callchain looks like it's originating from the write to the
unbind attribute for the driver. But pwm_request_from_chip() should have
already released the lock before it returned, so how can the above
situation even happen?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
Hi Thierry,
On Tue, Mar 12, 2019 at 12:55 PM Thierry Reding
wrote:
> On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote:
> > From: Phong Hoang
> >
> > This patch fixes deadlock warning if removing PWM device
> > when CONFIG_PROVE_LOCKING is enabled.
> >
> > This issue can be reproceduced by the following steps on
> > the R-Car H3 Salvator-X board if the backlight is disabled:
> >
> > # cd /sys/class/pwm/pwmchip0
> > # echo 0 > export
> > # ls
> > device export npwm power pwm0 subsystem uevent unexport
> > # cd device/driver
> > # ls
> > bind e6e31000.pwm uevent unbind
> > # echo e6e31000.pwm > unbind
> >
> > [ 87.659974] ==
> > [ 87.666149] WARNING: possible circular locking dependency detected
> > [ 87.672327] 5.0.0 #7 Not tainted
> > [ 87.675549] --
> > [ 87.681723] bash/2986 is trying to acquire lock:
> > [ 87.686337] 5ea0e178 (kn->count#58){}, at:
> > kernfs_remove_by_name_ns+0x50/0xa0
> > [ 87.694528]
> > [ 87.694528] but task is already holding lock:
> > [ 87.700353] 6313b17c (pwm_lock){+.+.}, at:
> > pwmchip_remove+0x28/0x13c
> > [ 87.707405]
> > [ 87.707405] which lock already depends on the new lock.
> > [ 87.707405]
> > [ 87.715574]
> > [ 87.715574] the existing dependency chain (in reverse order) is:
> > [ 87.723048]
> > [ 87.723048] -> #1 (pwm_lock){+.+.}:
> > [ 87.728017]__mutex_lock+0x70/0x7e4
> > [ 87.732108]mutex_lock_nested+0x1c/0x24
> > [ 87.736547]pwm_request_from_chip.part.6+0x34/0x74
> > [ 87.741940]pwm_request_from_chip+0x20/0x40
> > [ 87.746725]export_store+0x6c/0x1f4
> > [ 87.750820]dev_attr_store+0x18/0x28
> > [ 87.754998]sysfs_kf_write+0x54/0x64
> > [ 87.759175]kernfs_fop_write+0xe4/0x1e8
> > [ 87.763615]__vfs_write+0x40/0x184
> > [ 87.767619]vfs_write+0xa8/0x19c
> > [ 87.771448]ksys_write+0x58/0xbc
> > [ 87.775278]__arm64_sys_write+0x18/0x20
> > [ 87.779721]el0_svc_common+0xd0/0x124
> > [ 87.783986]el0_svc_compat_handler+0x1c/0x24
> > [ 87.788858]el0_svc_compat+0x8/0x18
> > [ 87.792947]
> > [ 87.792947] -> #0 (kn->count#58){}:
> > [ 87.798260]lock_acquire+0xc4/0x22c
> > [ 87.802353]__kernfs_remove+0x258/0x2c4
> > [ 87.806790]kernfs_remove_by_name_ns+0x50/0xa0
> > [ 87.811836]remove_files.isra.1+0x38/0x78
> > [ 87.816447]sysfs_remove_group+0x48/0x98
> > [ 87.820971]sysfs_remove_groups+0x34/0x4c
> > [ 87.825583]device_remove_attrs+0x6c/0x7c
> > [ 87.830197]device_del+0x11c/0x33c
> > [ 87.834201]device_unregister+0x14/0x2c
> > [ 87.838638]pwmchip_sysfs_unexport+0x40/0x4c
> > [ 87.843509]pwmchip_remove+0xf4/0x13c
> > [ 87.847773]rcar_pwm_remove+0x28/0x34
> > [ 87.852039]platform_drv_remove+0x24/0x64
> > [ 87.856651]device_release_driver_internal+0x18c/0x21c
> > [ 87.862391]device_release_driver+0x14/0x1c
> > [ 87.867175]unbind_store+0xe0/0x124
> > [ 87.871265]drv_attr_store+0x20/0x30
> > [ 87.875442]sysfs_kf_write+0x54/0x64
> > [ 87.879618]kernfs_fop_write+0xe4/0x1e8
> > [ 87.884055]__vfs_write+0x40/0x184
> > [ 87.888057]vfs_write+0xa8/0x19c
> > [ 87.891887]ksys_write+0x58/0xbc
> > [ 87.895716]__arm64_sys_write+0x18/0x20
> > [ 87.900154]el0_svc_common+0xd0/0x124
> > [ 87.904417]el0_svc_compat_handler+0x1c/0x24
> > [ 87.909289]el0_svc_compat+0x8/0x18
>
> I'm not sure I understand this correctly. The above looks like pwm_lock
> is held as part of the syscall writing 0 to the export attribute and the
> second callchain looks like it's originating from the write to the
> unbind attribute for the driver. But pwm_request_from_chip() should have
> already released the lock before it returned, so how can the above
> situation even happen?
Note that it says "_possible_ circular locking dependency detected".
AFAIU, this does not mean that the above two callchains actually did
happen at the same time.
Lockdep keeps tracks of the order in which locks are taken. If it
notices that one chain takes locks in one order, and a second chain
takes those locks in a different order, it prints a warning, as this
could cause a deadlock if/when the two callchains would ever happen
at the same time.
Note that there may be other reasons, outside the view of lockdep, which
guarantee this cannot actually happen...
So far my understanding...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" o
Re: [PATCH] i2c: add extra check to safe DMA buffer helper
> Yes. But I think the debug message here is just to tell caller that > threshold == 0 might not make sense. I can agree to the debug message. I'll send V2 soon. signature.asc Description: PGP signature
[PATCH v2] i2c: add extra check to safe DMA buffer helper
Make sure we report 'no buffer' for 0-length messages. This can only
happen if threshold is set to 0 which is kind of bogus but we should
still handle this situation. Update the docs and add a debug message
to educate callers of this function.
Reported-by: Hsin-Yi Wang
Fixes: e94bc5d18be0 ("i2c: add helpers to ease DMA handling")
Signed-off-by: Wolfram Sang
---
Changes since v1:
* add debug print
* update kernel doc
* update commit message
drivers/i2c/i2c-core-base.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cb6c5cb0df0b..986e56cee44b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2258,7 +2258,8 @@ EXPORT_SYMBOL(i2c_put_adapter);
/**
* i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
* @msg: the message to be checked
- * @threshold: the minimum number of bytes for which using DMA makes sense
+ * @threshold: the minimum number of bytes for which using DMA makes sense.
+ *Should at least be 1.
*
* Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
*Or a valid pointer to be used with DMA. After use, release it by
@@ -2268,7 +2269,11 @@ EXPORT_SYMBOL(i2c_put_adapter);
*/
u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
{
- if (msg->len < threshold)
+ /* also skip 0-length msgs for bogus thresholds of 0 */
+ if (!threshold)
+ pr_debug("DMA buffer for addr=0x%02x with length 0 is bogus\n",
+msg->addr);
+ if (msg->len < threshold || msg->len == 0)
return NULL;
if (msg->flags & I2C_M_DMA_SAFE)
--
2.11.0
Re: [PATCH] pwm: Avoid deadlock warning when removing PWM device
On Tue, Mar 12, 2019 at 12:16:34PM +0900, Yoshihiro Shimoda wrote:
> From: Phong Hoang
>
> This patch fixes deadlock warning if removing PWM device
> when CONFIG_PROVE_LOCKING is enabled.
>
> This issue can be reproceduced by the following steps on
> the R-Car H3 Salvator-X board if the backlight is disabled:
>
> # cd /sys/class/pwm/pwmchip0
> # echo 0 > export
> # ls
> device export npwm power pwm0 subsystem uevent unexport
> # cd device/driver
> # ls
> bind e6e31000.pwm uevent unbind
> # echo e6e31000.pwm > unbind
>
> [ 87.659974] ==
> [ 87.666149] WARNING: possible circular locking dependency detected
> [ 87.672327] 5.0.0 #7 Not tainted
> [ 87.675549] --
> [ 87.681723] bash/2986 is trying to acquire lock:
> [ 87.686337] 5ea0e178 (kn->count#58){}, at:
> kernfs_remove_by_name_ns+0x50/0xa0
> [ 87.694528]
> [ 87.694528] but task is already holding lock:
> [ 87.700353] 6313b17c (pwm_lock){+.+.}, at:
> pwmchip_remove+0x28/0x13c
> [ 87.707405]
> [ 87.707405] which lock already depends on the new lock.
> [ 87.707405]
> [ 87.715574]
> [ 87.715574] the existing dependency chain (in reverse order) is:
> [ 87.723048]
> [ 87.723048] -> #1 (pwm_lock){+.+.}:
> [ 87.728017]__mutex_lock+0x70/0x7e4
> [ 87.732108]mutex_lock_nested+0x1c/0x24
> [ 87.736547]pwm_request_from_chip.part.6+0x34/0x74
> [ 87.741940]pwm_request_from_chip+0x20/0x40
> [ 87.746725]export_store+0x6c/0x1f4
> [ 87.750820]dev_attr_store+0x18/0x28
> [ 87.754998]sysfs_kf_write+0x54/0x64
> [ 87.759175]kernfs_fop_write+0xe4/0x1e8
> [ 87.763615]__vfs_write+0x40/0x184
> [ 87.767619]vfs_write+0xa8/0x19c
> [ 87.771448]ksys_write+0x58/0xbc
> [ 87.775278]__arm64_sys_write+0x18/0x20
> [ 87.779721]el0_svc_common+0xd0/0x124
> [ 87.783986]el0_svc_compat_handler+0x1c/0x24
> [ 87.788858]el0_svc_compat+0x8/0x18
> [ 87.792947]
> [ 87.792947] -> #0 (kn->count#58){}:
> [ 87.798260]lock_acquire+0xc4/0x22c
> [ 87.802353]__kernfs_remove+0x258/0x2c4
> [ 87.806790]kernfs_remove_by_name_ns+0x50/0xa0
> [ 87.811836]remove_files.isra.1+0x38/0x78
> [ 87.816447]sysfs_remove_group+0x48/0x98
> [ 87.820971]sysfs_remove_groups+0x34/0x4c
> [ 87.825583]device_remove_attrs+0x6c/0x7c
> [ 87.830197]device_del+0x11c/0x33c
> [ 87.834201]device_unregister+0x14/0x2c
> [ 87.838638]pwmchip_sysfs_unexport+0x40/0x4c
> [ 87.843509]pwmchip_remove+0xf4/0x13c
> [ 87.847773]rcar_pwm_remove+0x28/0x34
> [ 87.852039]platform_drv_remove+0x24/0x64
> [ 87.856651]device_release_driver_internal+0x18c/0x21c
> [ 87.862391]device_release_driver+0x14/0x1c
> [ 87.867175]unbind_store+0xe0/0x124
> [ 87.871265]drv_attr_store+0x20/0x30
> [ 87.875442]sysfs_kf_write+0x54/0x64
> [ 87.879618]kernfs_fop_write+0xe4/0x1e8
> [ 87.884055]__vfs_write+0x40/0x184
> [ 87.888057]vfs_write+0xa8/0x19c
> [ 87.891887]ksys_write+0x58/0xbc
> [ 87.895716]__arm64_sys_write+0x18/0x20
> [ 87.900154]el0_svc_common+0xd0/0x124
> [ 87.904417]el0_svc_compat_handler+0x1c/0x24
> [ 87.909289]el0_svc_compat+0x8/0x18
> [ 87.913378]
> [ 87.913378] other info that might help us debug this:
> [ 87.913378]
> [ 87.921374] Possible unsafe locking scenario:
> [ 87.921374]
> [ 87.927286]CPU0CPU1
> [ 87.931808]
> [ 87.936331] lock(pwm_lock);
> [ 87.939293]lock(kn->count#58);
> [ 87.945120]lock(pwm_lock);
> [ 87.950599] lock(kn->count#58);
> [ 87.953908]
> [ 87.953908] *** DEADLOCK ***
> [ 87.953908]
> [ 87.959821] 4 locks held by bash/2986:
> [ 87.963563] #0: ace7bc30 (sb_writers#6){.+.+}, at:
> vfs_write+0x188/0x19c
> [ 87.971044] #1: 287991b2 (&of->mutex){+.+.}, at:
> kernfs_fop_write+0xb4/0x1e8
> [ 87.978872] #2: f739d016 (&dev->mutex){}, at:
> device_release_driver_internal+0x40/0x21c
> [ 87.988001] #3: 6313b17c (pwm_lock){+.+.}, at:
> pwmchip_remove+0x28/0x13c
> [ 87.995481]
> [ 87.995481] stack backtrace:
> [ 87.999836] CPU: 0 PID: 2986 Comm: bash Not tainted 5.0.0 #7
> [ 88.005489] Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x
> (DT)
> [ 88.012791] Call trace:
> [ 88.015235] dump_backtrace+0x0/0x190
> [ 88.018891] show_stack+0x14/0x1c
> [ 88.022204] dump_stack+0xb0/0xec
> [ 88.025514] print_circular_bug.isra.32+0x1d0/0x2e0
> [ 88.030385] __lock_acquire+0x1318/0x1864
> [ 88.034388] lock_acquire+0xc4/0x22c
> [ 88.0
Re: [PATCH 3/3] i2c: rcar: wait for data empty before starting DMA
> > + /* Check if DMA can be enabled and take over */ > > + if (priv->pos == 1 && rcar_i2c_dma(priv)) > > Shouldn't you check if MSR.MAT is set? It was set before, when priv->pos == 0. > > > + return; > > Hence ICMCR.ESG is not cleared, violating 57.3.8 (Master Transmit > Operation), Step 3? Clearing ESG is the very first thing we do in the ISR. Otherwise, we run into the race issue we have on this HW and generate an unwanted repeated message. signature.asc Description: PGP signature
Re: [PATCH v2] i2c: add extra check to safe DMA buffer helper
On Tue, Mar 12, 2019 at 8:44 PM Wolfram Sang
wrote:
>
> Make sure we report 'no buffer' for 0-length messages. This can only
> happen if threshold is set to 0 which is kind of bogus but we should
> still handle this situation. Update the docs and add a debug message
> to educate callers of this function.
>
> Reported-by: Hsin-Yi Wang
> Fixes: e94bc5d18be0 ("i2c: add helpers to ease DMA handling")
> Signed-off-by: Wolfram Sang
Thanks!
Reviewed-by: Hsin-Yi Wang
> ---
>
> Changes since v1:
> * add debug print
> * update kernel doc
> * update commit message
>
> drivers/i2c/i2c-core-base.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index cb6c5cb0df0b..986e56cee44b 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2258,7 +2258,8 @@ EXPORT_SYMBOL(i2c_put_adapter);
> /**
> * i2c_get_dma_safe_msg_buf() - get a DMA safe buffer for the given i2c_msg
> * @msg: the message to be checked
> - * @threshold: the minimum number of bytes for which using DMA makes sense
> + * @threshold: the minimum number of bytes for which using DMA makes sense.
> + *Should at least be 1.
> *
> * Return: NULL if a DMA safe buffer was not obtained. Use msg->buf with PIO.
> *Or a valid pointer to be used with DMA. After use, release it by
> @@ -2268,7 +2269,11 @@ EXPORT_SYMBOL(i2c_put_adapter);
> */
> u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold)
> {
> - if (msg->len < threshold)
> + /* also skip 0-length msgs for bogus thresholds of 0 */
> + if (!threshold)
> + pr_debug("DMA buffer for addr=0x%02x with length 0 is
> bogus\n",
> +msg->addr);
> + if (msg->len < threshold || msg->len == 0)
> return NULL;
>
> if (msg->flags & I2C_M_DMA_SAFE)
> --
> 2.11.0
>
Re: [PATCH 1/3] i2c: rcar: sanity check for minimal DMA length
On Mon, Mar 11, 2019 at 11:08:13AM +0100, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Tue, Mar 5, 2019 at 7:52 PM Wolfram Sang > wrote: > > Use a macro for the hardcoded value and apply a build check. If it is > > not met, the driver logic will not work anymore. > > > > Signed-off-by: Wolfram Sang > > --- > > drivers/i2c/busses/i2c-rcar.c | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > > index 3ce74edcd70c..925858915569 100644 > > --- a/drivers/i2c/busses/i2c-rcar.c > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > @@ -921,6 +922,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) > > struct i2c_timings i2c_t; > > int irq, ret; > > > > + /* Otherwise logic will break because some bytes must always use > > PIO */ > > + BUILD_BUG_ON_MSG(RCAR_MIN_DMA_LEN < 3, "Invalid min DMA length"); > > Given patch 3/3, it should still work with RCAR_MIN_DMA_LEN == 2, right? Nope. It is not that we transfer one byte more with PIO now. The change in patch 3 is that we explicitly wait for an interrupt when the (already existing) PIO transfer ended. Before that patch, we assumed DMA would take over on its own once the data register is empty again. Should I update the commit message to make this more clear? Also, it is the _read_ case which needs the minimum lenght of 3. This is fixing the _write_ code path :) signature.asc Description: PGP signature
Re: [PATCH 3/3] i2c: rcar: wait for data empty before starting DMA
Hi Wolfram,
On Tue, Mar 12, 2019 at 1:50 PM Wolfram Sang wrote:
> > > + /* Check if DMA can be enabled and take over */
> > > + if (priv->pos == 1 && rcar_i2c_dma(priv))
> >
> > Shouldn't you check if MSR.MAT is set?
>
> It was set before, when priv->pos == 0.
>
> >
> > > + return;
> >
> > Hence ICMCR.ESG is not cleared, violating 57.3.8 (Master Transmit
> > Operation), Step 3?
>
> Clearing ESG is the very first thing we do in the ISR. Otherwise, we run
> into the race issue we have on this HW and generate an unwanted repeated
> message.
Thanks!
Obviously I should work on improving my i2c-rcar foo ... ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 3/3] i2c: rcar: wait for data empty before starting DMA
> Obviously I should work on improving my i2c-rcar foo ... ;-) There be dragons! ;) signature.asc Description: PGP signature
Re: [PATCH 3/3] i2c: rcar: wait for data empty before starting DMA
Hi Wolfram,
On Tue, Mar 12, 2019 at 2:18 PM Wolfram Sang wrote:
> > Obviously I should work on improving my i2c-rcar foo ... ;-)
>
> There be dragons! ;)
Fortunately I've enjoyed "How to Train Your Dragon 3" with my girls last
weekend ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH i2c-tools 0/2] tools: improvements to handling restricted addresses
Hi Wolfram, Thanks for your work. On 2019-03-11 23:33:33 +0100, Wolfram Sang wrote: > For debugging, I needed to use the restricted address range. That's when I > noticed that the docs for i2ctransfer were missing an update (patch 1). But > even more surprising, I noticed that the reserved addresses 0x03-0x07 were not > restricted. It seems we all are so used to our lovely i2c-tools that nobody > (including me) never questioned that :) But it should be fixed (patch 2), I > think. > > Open for comments. Happy hacking! I'm no expert in i2c but this matches my understanding of reserved addresses in the specification, feel free to add for the whole series Reviewed-by: Niklas Söderlund > > > Wolfram Sang (2): > tools: i2ctransfer: man: mention -a everywhere > tools: restrict all addresses defined by the standard > > CHANGES | 2 ++ > tools/i2cbusses.c | 2 +- > tools/i2cdetect.8 | 2 +- > tools/i2cdetect.c | 2 +- > tools/i2cdump.8 | 4 ++-- > tools/i2cdump.c | 2 +- > tools/i2cget.8 | 4 ++-- > tools/i2cget.c | 2 +- > tools/i2cset.8 | 4 ++-- > tools/i2cset.c | 2 +- > tools/i2ctransfer.8 | 12 +++- > 11 files changed, 21 insertions(+), 17 deletions(-) > > -- > 2.11.0 > -- Regards, Niklas Söderlund
[PATCH] spi: rspi: Fix sequencer reset during initialization
While the sequencer is reset after each SPI message since commit
880c6d114fd79a69 ("spi: rspi: Add support for Quad and Dual SPI
Transfers on QSPI"), it was never reset for the first message, thus
relying on reset state or bootloader settings.
Fix this by initializing it explicitly during configuration.
Fixes: 0b2182ddac4b8837 ("spi: add support for Renesas RSPI")
Signed-off-by: Geert Uytterhoeven
---
drivers/spi/spi-rspi.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index b30fed824e6643a2..3be8fbe80b084ac0 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -271,7 +271,8 @@ static int rspi_set_config_register(struct rspi_data *rspi,
int access_size)
/* Sets parity, interrupt mask */
rspi_write8(rspi, 0x00, RSPI_SPCR2);
- /* Sets SPCMD */
+ /* Resets sequencer */
+ rspi_write8(rspi, 0, RSPI_SPSCR);
rspi->spcmd |= SPCMD_SPB_8_TO_16(access_size);
rspi_write16(rspi, rspi->spcmd, RSPI_SPCMD0);
@@ -315,7 +316,8 @@ static int rspi_rz_set_config_register(struct rspi_data
*rspi, int access_size)
rspi_write8(rspi, 0x00, RSPI_SSLND);
rspi_write8(rspi, 0x00, RSPI_SPND);
- /* Sets SPCMD */
+ /* Resets sequencer */
+ rspi_write8(rspi, 0, RSPI_SPSCR);
rspi->spcmd |= SPCMD_SPB_8_TO_16(access_size);
rspi_write16(rspi, rspi->spcmd, RSPI_SPCMD0);
@@ -366,7 +368,8 @@ static int qspi_set_config_register(struct rspi_data *rspi,
int access_size)
/* Sets buffer to allow normal operation */
rspi_write8(rspi, 0x00, QSPI_SPBFCR);
- /* Sets SPCMD */
+ /* Resets sequencer */
+ rspi_write8(rspi, 0, RSPI_SPSCR);
rspi_write16(rspi, rspi->spcmd, RSPI_SPCMD0);
/* Sets RSPI mode */
--
2.17.1
