RE: [PATCH v2] pwm: Fix deadlock warning when removing PWM device

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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)