Message ID | f83b7dd1edbee967b352aa3c2dab676bafd63a56.1474450295.git.dwalter@sigma-star.at |
---|---|
State | Changes Requested |
Delegated to: | Boris Brezillon |
Headers | show |
On Wed, 21 Sep 2016 11:45:12 +0200 Daniel Walter <dwalter@sigma-star.at> wrote: > From: Richard Weinberger <richard@nod.at> > > del_mtd_device() is allowed to fail. > i.e. when the MTD is busy. > Unregister the reboot notifier only when we're really > about to delete the MTD. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/mtdcore.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index e3936b8..36e5fb0 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -654,17 +654,22 @@ int mtd_device_unregister(struct mtd_info *master) > { > int err; > > - if (master->_reboot) > - unregister_reboot_notifier(&master->reboot_notifier); > - > err = del_mtd_partitions(master); > if (err) > return err; > > if (!device_is_registered(&master->dev)) > - return 0; > + goto unregister; > > - return del_mtd_device(master); > + err = del_mtd_device(master); > + if (err) > + return err; > + > +unregister: > + if (master->_reboot) > + unregister_reboot_notifier(&master->reboot_notifier); > + > + return 0; How about: if (device_is_registered(&master->dev)) { err = del_mtd_device(master); if (err) return err; } if (master->_reboot) unregister_reboot_notifier(&master->reboot_notifier); return 0; This way you get rid of the unregister label, which IMHO improves readability. > } > EXPORT_SYMBOL_GPL(mtd_device_unregister); >
On 09/21/2016 04:31 PM, Boris Brezillon wrote: > On Wed, 21 Sep 2016 11:45:12 +0200 > Daniel Walter <dwalter@sigma-star.at> wrote: > >> From: Richard Weinberger <richard@nod.at> >> >> del_mtd_device() is allowed to fail. >> i.e. when the MTD is busy. >> Unregister the reboot notifier only when we're really >> about to delete the MTD. >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> drivers/mtd/mtdcore.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index e3936b8..36e5fb0 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -654,17 +654,22 @@ int mtd_device_unregister(struct mtd_info *master) >> { >> int err; >> >> - if (master->_reboot) >> - unregister_reboot_notifier(&master->reboot_notifier); >> - >> err = del_mtd_partitions(master); >> if (err) >> return err; >> >> if (!device_is_registered(&master->dev)) >> - return 0; >> + goto unregister; >> >> - return del_mtd_device(master); >> + err = del_mtd_device(master); >> + if (err) >> + return err; >> + >> +unregister: >> + if (master->_reboot) >> + unregister_reboot_notifier(&master->reboot_notifier); >> + >> + return 0; > > How about: > > if (device_is_registered(&master->dev)) { > err = del_mtd_device(master); > if (err) > return err; > } > > if (master->_reboot) > unregister_reboot_notifier(&master->reboot_notifier); > > return 0; > > This way you get rid of the unregister label, which IMHO improves > readability. Agree, will fix this in v3 of the series.
I realize I didn't comment on the latest copy of this patch, so copying my questions here: On Wed, Sep 21, 2016 at 11:45:12AM +0200, Daniel Walter wrote: > From: Richard Weinberger <richard@nod.at> > > del_mtd_device() is allowed to fail. > i.e. when the MTD is busy. > Unregister the reboot notifier only when we're really > about to delete the MTD. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/mtdcore.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index e3936b8..36e5fb0 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -654,17 +654,22 @@ int mtd_device_unregister(struct mtd_info *master) > { > int err; > > - if (master->_reboot) > - unregister_reboot_notifier(&master->reboot_notifier); > - > err = del_mtd_partitions(master); > if (err) > return err; > > if (!device_is_registered(&master->dev)) > - return 0; > + goto unregister; > > - return del_mtd_device(master); > + err = del_mtd_device(master); > + if (err) > + return err; > + > +unregister: > + if (master->_reboot) > + unregister_reboot_notifier(&master->reboot_notifier); Is there any kind of race issue with unregistering the notifier *after* we've deleted the device? I had intentionally unregistered first, because I didn't want any chance of the driver/module and/or data structures being freed before we call the notifier. I can't think of any particular issue yet, but I wanted to ask. Brian > + return 0; > } > EXPORT_SYMBOL_GPL(mtd_device_unregister); > > -- > 2.8.3 >
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index e3936b8..36e5fb0 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -654,17 +654,22 @@ int mtd_device_unregister(struct mtd_info *master) { int err; - if (master->_reboot) - unregister_reboot_notifier(&master->reboot_notifier); - err = del_mtd_partitions(master); if (err) return err; if (!device_is_registered(&master->dev)) - return 0; + goto unregister; - return del_mtd_device(master); + err = del_mtd_device(master); + if (err) + return err; + +unregister: + if (master->_reboot) + unregister_reboot_notifier(&master->reboot_notifier); + + return 0; } EXPORT_SYMBOL_GPL(mtd_device_unregister);