| Message ID | 20170621062647.6072-3-zajec5@gmail.com |
|---|---|
| State | Accepted |
| Commit | c5ceaba74083daf619bdb34d4871e297a177eebf |
| Delegated to: | Brian Norris |
| Headers | show |
On Wed, Jun 21, 2017 at 08:26:43AM +0200, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > When support for sysfs "offset" file was added it missed to update the > del_mtd_partitions function. It deletes partitions just like > mtd_del_partition does so both should also take care of removing sysfs > files. > > This change moves sysfs_remove_files call to the shared function to fix > this issue. > > Fixes: a62c24d755291 ("mtd: part: Add sysfs variable for offset of partition") > Cc: Dan Ehrenberg <dehrenberg@chromium.org> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V7: Introduction of this patch, it was separated from the 1/6. It should > be clear now that this change is introduced. If something goes wrong > it should also be easier to revert it this way. Thanks, splitting this out is nice actually. And thanks for looking up the background on this oversight. I'm not really sure what the actual effect of that omission would be; the file won't be left dangling in sysfs (the device is removed entirely), but I'm not sure if that'd trigger some kind of internal kobject memory leak. Anyway, looks good to me. Side note: this patch series sort of massages the definition of this "offset" file. Now, IIUC, subpartitions will have an "offset" file relative to their parent partition, not the master flash device. I'm not sure which way is expected. Brian
On 22 June 2017 at 23:05, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Jun 21, 2017 at 08:26:43AM +0200, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> When support for sysfs "offset" file was added it missed to update the >> del_mtd_partitions function. It deletes partitions just like >> mtd_del_partition does so both should also take care of removing sysfs >> files. >> >> This change moves sysfs_remove_files call to the shared function to fix >> this issue. >> >> Fixes: a62c24d755291 ("mtd: part: Add sysfs variable for offset of partition") >> Cc: Dan Ehrenberg <dehrenberg@chromium.org> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> V7: Introduction of this patch, it was separated from the 1/6. It should >> be clear now that this change is introduced. If something goes wrong >> it should also be easier to revert it this way. > > Thanks, splitting this out is nice actually. And thanks for looking up > the background on this oversight. > > I'm not really sure what the actual effect of that omission would be; > the file won't be left dangling in sysfs (the device is removed > entirely), but I'm not sure if that'd trigger some kind of internal > kobject memory leak. > > Anyway, looks good to me. In the worst case it looks like a small cleanup of the exit path. > Side note: this patch series sort of massages the definition of this > "offset" file. Now, IIUC, subpartitions will have an "offset" file > relative to their parent partition, not the master flash device. I'm not > sure which way is expected. I didn't think about this. I should be able to easily get an absolute offset if needed (a simple recursion), but I'm not sure what behavior is actually expected. Since we use a tree struct in the sysfs now, maybe relative offsets are actually correct ones?
On Thu, Jun 22, 2017 at 11:40:47PM +0200, Rafał Miłecki wrote: > On 22 June 2017 at 23:05, Brian Norris <computersforpeace@gmail.com> wrote: > > Side note: this patch series sort of massages the definition of this > > "offset" file. Now, IIUC, subpartitions will have an "offset" file > > relative to their parent partition, not the master flash device. I'm not > > sure which way is expected. > > I didn't think about this. I should be able to easily get an absolute > offset if needed (a simple recursion), but I'm not sure what behavior > is actually expected. Since we use a tree struct in the sysfs now, > maybe relative offsets are actually correct ones? Yeah, that seems OK to me. The only holdup is that the current description uses the term "master", where perhaps it should just mean say "parent" now: Documentation/ABI/testing/sysfs-class-mtd: What: /sys/class/mtd/mtdX/offset Date: March 2015 KernelVersion: 4.1 Contact: linux-mtd@lists.infradead.org Description: For a partition, the offset of that partition from the start of the master device in bytes. This attribute is absent on main devices, so it can be used to distinguish between partitions and devices that aren't partitions. Brian
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index bcec72148d0b..9434050accc7 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -660,6 +660,8 @@ static int __mtd_del_partition(struct mtd_part *priv) { int err; + sysfs_remove_files(&priv->mtd.dev.kobj, mtd_partition_attrs); + err = del_mtd_device(&priv->mtd); if (err) return err; @@ -700,8 +702,6 @@ int mtd_del_partition(struct mtd_info *master, int partno) list_for_each_entry_safe(slave, next, &mtd_partitions, list) if ((slave->master == master) && (slave->mtd.index == partno)) { - sysfs_remove_files(&slave->mtd.dev.kobj, - mtd_partition_attrs); ret = __mtd_del_partition(slave); break; }