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@xxxxxxxxxxx> wrote:
> > From: Phong Hoang <phong.hoang.wz@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > [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@xxxxxxxxxxx>
> > Tested-by: Hoan Nguyen An <na-hoan@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
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)
