Message ID | 1552465792-25340-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | [v2] pwm: Fix deadlock warning when removing PWM device | expand |
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
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)
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 */