All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] pwm: Fix deadlock warning when removing PWM device
@ 2019-03-13  8:29 Yoshihiro Shimoda
  2019-03-13  9:04 ` Geert Uytterhoeven
  0 siblings, 1 reply; 3+ messages in thread
From: Yoshihiro Shimoda @ 2019-03-13  8:29 UTC (permalink / raw)
  To: thierry.reding
  Cc: linux-pwm, linux-renesas-soc, Phong Hoang, Yoshihiro Shimoda

From: Phong Hoang <phong.hoang.wz@renesas.com>

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] 000000005ea0e178 (kn->count#58){++++}, at: kernfs_remove_by_name_ns+0x50/0xa0
[   87.694528]
[   87.694528] but task is already holding lock:
[   87.700353] 000000006313b17c (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]        CPU0                    CPU1
[   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: 00000000ace7bc30 (sb_writers#6){.+.+}, at: vfs_write+0x188/0x19c
[   87.971044]  #1: 00000000287991b2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xb4/0x1e8
[   87.978872]  #2: 00000000f739d016 (&dev->mutex){....}, at: device_release_driver_internal+0x40/0x21c
[   87.988001]  #3: 000000006313b17c (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.037958]  __kernfs_remove+0x258/0x2c4
[   88.041874]  kernfs_remove_by_name_ns+0x50/0xa0
[   88.046398]  remove_files.isra.1+0x38/0x78
[   88.050487]  sysfs_remove_group+0x48/0x98
[   88.054490]  sysfs_remove_groups+0x34/0x4c
[   88.058580]  device_remove_attrs+0x6c/0x7c
[   88.062671]  device_del+0x11c/0x33c
[   88.066154]  device_unregister+0x14/0x2c
[   88.070070]  pwmchip_sysfs_unexport+0x40/0x4c
[   88.074421]  pwmchip_remove+0xf4/0x13c
[   88.078163]  rcar_pwm_remove+0x28/0x34
[   88.081906]  platform_drv_remove+0x24/0x64
[   88.085996]  device_release_driver_internal+0x18c/0x21c
[   88.091215]  device_release_driver+0x14/0x1c
[   88.095478]  unbind_store+0xe0/0x124
[   88.099048]  drv_attr_store+0x20/0x30
[   88.102704]  sysfs_kf_write+0x54/0x64
[   88.106359]  kernfs_fop_write+0xe4/0x1e8
[   88.110275]  __vfs_write+0x40/0x184
[   88.113757]  vfs_write+0xa8/0x19c
[   88.117065]  ksys_write+0x58/0xbc
[   88.120374]  __arm64_sys_write+0x18/0x20
[   88.124291]  el0_svc_common+0xd0/0x124
[   88.128034]  el0_svc_compat_handler+0x1c/0x24
[   88.132384]  el0_svc_compat+0x8/0x18

The sysfs unexport in pwmchip_remove() is completely asymmetric
to what we do in pwmchip_add_with_polarity() and commit 0733424c9ba9
("pwm: Unexport children before chip removal") is a strong indication
that this was wrong to begin with. We should just move
pwmchip_sysfs_unexport() where it belongs, which is right after
pwmchip_sysfs_unexport_children(). In that case, we do not need
separate functions anymore either.

We also really want to remove sysfs irrespective of whether or not
the chip will be removed as a result of pwmchip_remove(). We can only
assume that the driver will be gone after that, so we shouldn't leave
any dangling sysfs files around.

This warning disappears if we move pwmchip_sysfs_unexport() to
the top of pwmchip_remove(), right below pwmchip_sysfs_unexport_children().
That way it is also outside of the pwm_lock section, which indeed
doesn't seem to be needed.

Moving the pwmchip_sysfs_export() call outside of that section also
seems fine and it'd be perfectly symmetric with pwmchip_remove() again.

So, this patch fixes them.

Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
[shimoda: revise the commit log and code]
Fixes: 76abbdde2d95 ("pwm: Add sysfs interface")
Fixes: 0733424c9ba9 ("pwm: Unexport children before chip removal")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
---
 Changes from v1 (https://patchwork.kernel.org/patch/10848567/):
 - Change the subject from "Avoid" to "Fix".
 - Merge pwmchip_sysfs_unexport_children()'s code into
   pwmchip_sysfs_unexport() and move pwmchip_sysfs_unexport() to
   the top of pwmchip_remove().
 - Revise the commit log that is reffered from Therry's comments [1]
   because it seems very clear to me.
 - Add Fixes tag about the commit 0733424c9ba9. 
 - I got Geert's Reviewed-by on v1 patch, but I'm not sure
   I can add the Reviewed-by because v2 patch changes a bit.
   So, I didn't add the Reviewed-by tag.

[1]
https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg39167.html

 drivers/pwm/core.c  | 10 +++++-----
 drivers/pwm/sysfs.c | 28 ++++++++--------------------
 include/linux/pwm.h |  5 -----
 3 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6a..c45e571 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);
@@ -348,7 +350,7 @@ int pwmchip_remove(struct pwm_chip *chip)
 	unsigned int i;
 	int ret = 0;
 
-	pwmchip_sysfs_unexport_children(chip);
+	pwmchip_sysfs_unexport(chip);
 
 	mutex_lock(&pwm_lock);
 
@@ -368,8 +370,6 @@ int pwmchip_remove(struct pwm_chip *chip)
 
 	free_pwms(chip);
 
-	pwmchip_sysfs_unexport(chip);
-
 out:
 	mutex_unlock(&pwm_lock);
 	return ret;
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index ceb233d..a099066 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -411,36 +411,24 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
 void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 {
 	struct device *parent;
+	unsigned int i;
 
 	parent = class_find_device(&pwm_class, NULL, chip,
 				   pwmchip_sysfs_match);
 	if (parent) {
+		for (i = 0; i < chip->npwm; i++) {
+			struct pwm_device *pwm = &chip->pwms[i];
+
+			if (test_bit(PWMF_EXPORTED, &pwm->flags))
+				pwm_unexport_child(parent, pwm);
+		}
+
 		/* for class_find_device() */
 		put_device(parent);
 		device_unregister(parent);
 	}
 }
 
-void pwmchip_sysfs_unexport_children(struct pwm_chip *chip)
-{
-	struct device *parent;
-	unsigned int i;
-
-	parent = class_find_device(&pwm_class, NULL, chip,
-				   pwmchip_sysfs_match);
-	if (!parent)
-		return;
-
-	for (i = 0; i < chip->npwm; i++) {
-		struct pwm_device *pwm = &chip->pwms[i];
-
-		if (test_bit(PWMF_EXPORTED, &pwm->flags))
-			pwm_unexport_child(parent, pwm);
-	}
-
-	put_device(parent);
-}
-
 static int __init pwm_sysfs_init(void)
 {
 	return class_register(&pwm_class);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d5199b5..ea3939f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -597,7 +597,6 @@ static inline void pwm_remove_table(struct pwm_lookup *table, size_t num)
 #ifdef CONFIG_PWM_SYSFS
 void pwmchip_sysfs_export(struct pwm_chip *chip);
 void pwmchip_sysfs_unexport(struct pwm_chip *chip);
-void pwmchip_sysfs_unexport_children(struct pwm_chip *chip);
 #else
 static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
 {
@@ -606,10 +605,6 @@ static inline void pwmchip_sysfs_export(struct pwm_chip *chip)
 static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 {
 }
-
-static inline void pwmchip_sysfs_unexport_children(struct pwm_chip *chip)
-{
-}
 #endif /* CONFIG_PWM_SYSFS */
 
 #endif /* __LINUX_PWM_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] pwm: Fix deadlock warning when removing PWM device
  2019-03-13  8:29 [PATCH v2] pwm: Fix deadlock warning when removing PWM device Yoshihiro Shimoda
@ 2019-03-13  9:04 ` Geert Uytterhoeven
  2019-03-13  9:40   ` Yoshihiro Shimoda
  0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2019-03-13  9:04 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Thierry Reding, Linux PWM List, Linux-Renesas, Phong Hoang

Hi Shimoda-san,

On Wed, Mar 13, 2019 at 9:30 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> From: Phong Hoang <phong.hoang.wz@renesas.com>
>
> 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

[...]

> The sysfs unexport in pwmchip_remove() is completely asymmetric
> to what we do in pwmchip_add_with_polarity() and commit 0733424c9ba9
> ("pwm: Unexport children before chip removal") is a strong indication
> that this was wrong to begin with. We should just move
> pwmchip_sysfs_unexport() where it belongs, which is right after
> pwmchip_sysfs_unexport_children(). In that case, we do not need
> separate functions anymore either.
>
> We also really want to remove sysfs irrespective of whether or not
> the chip will be removed as a result of pwmchip_remove(). We can only
> assume that the driver will be gone after that, so we shouldn't leave
> any dangling sysfs files around.
>
> This warning disappears if we move pwmchip_sysfs_unexport() to
> the top of pwmchip_remove(), right below pwmchip_sysfs_unexport_children().

Drop the "right below..." part, as pwmchip_sysfs_unexport_children() is gone?

> That way it is also outside of the pwm_lock section, which indeed
> doesn't seem to be needed.
>
> Moving the pwmchip_sysfs_export() call outside of that section also
> seems fine and it'd be perfectly symmetric with pwmchip_remove() again.
>
> So, this patch fixes them.
>
> Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> [shimoda: revise the commit log and code]
> Fixes: 76abbdde2d95 ("pwm: Add sysfs interface")
> Fixes: 0733424c9ba9 ("pwm: Unexport children before chip removal")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
> ---
>  Changes from v1 (https://patchwork.kernel.org/patch/10848567/):
>  - Change the subject from "Avoid" to "Fix".
>  - Merge pwmchip_sysfs_unexport_children()'s code into
>    pwmchip_sysfs_unexport() and move pwmchip_sysfs_unexport() to
>    the top of pwmchip_remove().
>  - Revise the commit log that is reffered from Therry's comments [1]
>    because it seems very clear to me.
>  - Add Fixes tag about the commit 0733424c9ba9.
>  - I got Geert's Reviewed-by on v1 patch, but I'm not sure
>    I can add the Reviewed-by because v2 patch changes a bit.
>    So, I didn't add the Reviewed-by tag.

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c

> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -411,36 +411,24 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
>  void pwmchip_sysfs_unexport(struct pwm_chip *chip)
>  {
>         struct device *parent;
> +       unsigned int i;
>
>         parent = class_find_device(&pwm_class, NULL, chip,
>                                    pwmchip_sysfs_match);
>         if (parent) {

Perhaps "if (!parent) return", like pwmchip_sysfs_unexport_children()
used to do?

That way the reviewer has less context to store, indentation is
decreased, and the resulting diff may be smaller.

> +               for (i = 0; i < chip->npwm; i++) {
> +                       struct pwm_device *pwm = &chip->pwms[i];
> +
> +                       if (test_bit(PWMF_EXPORTED, &pwm->flags))
> +                               pwm_unexport_child(parent, pwm);
> +               }
> +
>                 /* for class_find_device() */
>                 put_device(parent);
>                 device_unregister(parent);
>         }
>  }
>
> -void pwmchip_sysfs_unexport_children(struct pwm_chip *chip)
> -{
> -       struct device *parent;
> -       unsigned int i;
> -
> -       parent = class_find_device(&pwm_class, NULL, chip,
> -                                  pwmchip_sysfs_match);
> -       if (!parent)
> -               return;
> -
> -       for (i = 0; i < chip->npwm; i++) {
> -               struct pwm_device *pwm = &chip->pwms[i];
> -
> -               if (test_bit(PWMF_EXPORTED, &pwm->flags))
> -                       pwm_unexport_child(parent, pwm);
> -       }
> -
> -       put_device(parent);
> -}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* RE: [PATCH v2] pwm: Fix deadlock warning when removing PWM device
  2019-03-13  9:04 ` Geert Uytterhoeven
@ 2019-03-13  9:40   ` Yoshihiro Shimoda
  0 siblings, 0 replies; 3+ messages in thread
From: Yoshihiro Shimoda @ 2019-03-13  9:40 UTC (permalink / raw)
  To: Geert Uytterhoeven, Thierry Reding
  Cc: Linux PWM List, Linux-Renesas, Phong Hoang

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Wednesday, March 13, 2019 6:05 PM
> 
> Hi Shimoda-san,
> 
> On Wed, Mar 13, 2019 at 9:30 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Phong Hoang <phong.hoang.wz@renesas.com>
> >
> > 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
> 
> [...]
> 
> > The sysfs unexport in pwmchip_remove() is completely asymmetric
> > to what we do in pwmchip_add_with_polarity() and commit 0733424c9ba9
> > ("pwm: Unexport children before chip removal") is a strong indication
> > that this was wrong to begin with. We should just move
> > pwmchip_sysfs_unexport() where it belongs, which is right after
> > pwmchip_sysfs_unexport_children(). In that case, we do not need
> > separate functions anymore either.
> >
> > We also really want to remove sysfs irrespective of whether or not
> > the chip will be removed as a result of pwmchip_remove(). We can only
> > assume that the driver will be gone after that, so we shouldn't leave
> > any dangling sysfs files around.
> >
> > This warning disappears if we move pwmchip_sysfs_unexport() to
> > the top of pwmchip_remove(), right below pwmchip_sysfs_unexport_children().
> 
> Drop the "right below..." part, as pwmchip_sysfs_unexport_children() is gone?

I got it. I'll fix v3 patch. (Maybe I'll submit it tomorrow (Japan time zone).)

> > That way it is also outside of the pwm_lock section, which indeed
> > doesn't seem to be needed.
> >
> > Moving the pwmchip_sysfs_export() call outside of that section also
> > seems fine and it'd be perfectly symmetric with pwmchip_remove() again.
> >
> > So, this patch fixes them.
> >
> > Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> > [shimoda: revise the commit log and code]
> > Fixes: 76abbdde2d95 ("pwm: Add sysfs interface")
> > Fixes: 0733424c9ba9 ("pwm: Unexport children before chip removal")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Tested-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
> > ---
> >  Changes from v1 (https://patchwork.kernel.org/patch/10848567/):
> >  - Change the subject from "Avoid" to "Fix".
> >  - Merge pwmchip_sysfs_unexport_children()'s code into
> >    pwmchip_sysfs_unexport() and move pwmchip_sysfs_unexport() to
> >    the top of pwmchip_remove().
> >  - Revise the commit log that is reffered from Therry's comments [1]
> >    because it seems very clear to me.
> >  - Add Fixes tag about the commit 0733424c9ba9.
> >  - I got Geert's Reviewed-by on v1 patch, but I'm not sure
> >    I can add the Reviewed-by because v2 patch changes a bit.
> >    So, I didn't add the Reviewed-by tag.
> 
> Thanks for the update!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you for review!

> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> 
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -411,36 +411,24 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
> >  void pwmchip_sysfs_unexport(struct pwm_chip *chip)
> >  {
> >         struct device *parent;
> > +       unsigned int i;
> >
> >         parent = class_find_device(&pwm_class, NULL, chip,
> >                                    pwmchip_sysfs_match);
> >         if (parent) {
> 
> Perhaps "if (!parent) return", like pwmchip_sysfs_unexport_children()
> used to do?
> 
> That way the reviewer has less context to store, indentation is
> decreased, and the resulting diff may be smaller.

I was thinking such a way. As you said, the resulting diff is smaller.

< v2 >
 drivers/pwm/core.c  | 10 +++++-----
 drivers/pwm/sysfs.c | 28 ++++++++--------------------
 include/linux/pwm.h |  5 -----
 3 files changed, 13 insertions(+), 30 deletions(-)

< if keeping "if (!parent)" >
 drivers/pwm/core.c  | 10 +++++-----
 drivers/pwm/sysfs.c | 14 +-------------
 include/linux/pwm.h |  5 -----
 3 files changed, 6 insertions(+), 23 deletions(-)

However, if we do "git annotate drivers/pwm/sysfs.c" on "keeping if (!parent)" code,
the commit 0733424c9ba9 remains on the sysfs.c. That's why I decided this v2 code.
Thierry, which code do you prefer? JFYI, I also copied and paste the keeping
if (!parent) patch as the following.

Best regards,
Yoshihiro Shimoda
---
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index ceb233d..13d9bd5 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -411,19 +411,6 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
 void pwmchip_sysfs_unexport(struct pwm_chip *chip)
 {
 	struct device *parent;
-
-	parent = class_find_device(&pwm_class, NULL, chip,
-				   pwmchip_sysfs_match);
-	if (parent) {
-		/* for class_find_device() */
-		put_device(parent);
-		device_unregister(parent);
-	}
-}
-
-void pwmchip_sysfs_unexport_children(struct pwm_chip *chip)
-{
-	struct device *parent;
 	unsigned int i;
 
 	parent = class_find_device(&pwm_class, NULL, chip,
@@ -439,6 +426,7 @@ void pwmchip_sysfs_unexport_children(struct pwm_chip *chip)
 	}
 
 	put_device(parent);
+	device_unregister(parent);
 }
 
 static int __init pwm_sysfs_init(void)


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-13  9:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13  8:29 [PATCH v2] pwm: Fix deadlock warning when removing PWM device Yoshihiro Shimoda
2019-03-13  9:04 ` Geert Uytterhoeven
2019-03-13  9:40   ` Yoshihiro Shimoda

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.