| Message ID | 1425418844-25177-2-git-send-email-fransklaver@gmail.com |
|---|---|
| State | Accepted |
| Commit | 260e89a6e0d6dcaccd484cf13a69285c3d22268f |
| Headers | show |
On Tue, Mar 03, 2015 at 10:39:45PM +0100, Frans Klaver wrote: > add_mtd_device() has a comment suggesting that the caller should have > set dev.parent. This is required to have the device show up in sysfs, What do you mean "have the device show up in sysfs"? AFAICT, this only has bearing on whether the *parent* device shows up as a sysfs symlink within the MTD device directory. i.e.: /sys/class/mtd/mtd*/device For instance, this sort of symlink: /sys/class/mtd/mtd0/device -> ../../../f03e2800.nand It might be good to clarify this in the commit message, since you make the problem sound worse than (I think) it is. Brian > but not for proper operation of the mtd device itself. Currently we have > five drivers registering mtd devices during module initialization, so > they don't actually provide a parent device to link to. That means we > cannot WARN_ON() here, as it would trigger false positives. > > Make the comment a bit less firm in its assertion that dev.parent should > be set. > > Signed-off-by: Frans Klaver <fransklaver@gmail.com> > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 11883bd..2093676 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -422,7 +422,7 @@ int add_mtd_device(struct mtd_info *mtd) > } > > /* Caller should have set dev.parent to match the > - * physical device. > + * physical device, if appropriate. > */ > mtd->dev.type = &mtd_devtype; > mtd->dev.class = &mtd_class; > -- > 2.2.2 >
On Tue, Mar 10, 2015 at 12:14 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Mar 03, 2015 at 10:39:45PM +0100, Frans Klaver wrote: >> add_mtd_device() has a comment suggesting that the caller should have >> set dev.parent. This is required to have the device show up in sysfs, > > What do you mean "have the device show up in sysfs"? AFAICT, this only > has bearing on whether the *parent* device shows up as a sysfs symlink > within the MTD device directory. i.e.: > > /sys/class/mtd/mtd*/device > > For instance, this sort of symlink: > > /sys/class/mtd/mtd0/device -> ../../../f03e2800.nand > > It might be good to clarify this in the commit message, since you make > the problem sound worse than (I think) it is. I do? That was definitely not my intention. I'll look into it. Thanks, Frans
On Tue, Mar 10, 2015 at 08:47:46AM +0100, Frans Klaver wrote: > On Tue, Mar 10, 2015 at 12:14 AM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Tue, Mar 03, 2015 at 10:39:45PM +0100, Frans Klaver wrote: > >> add_mtd_device() has a comment suggesting that the caller should have > >> set dev.parent. This is required to have the device show up in sysfs, > > > > What do you mean "have the device show up in sysfs"? AFAICT, this only > > has bearing on whether the *parent* device shows up as a sysfs symlink > > within the MTD device directory. i.e.: > > > > /sys/class/mtd/mtd*/device > > > > For instance, this sort of symlink: > > > > /sys/class/mtd/mtd0/device -> ../../../f03e2800.nand > > > > It might be good to clarify this in the commit message, since you make > > the problem sound worse than (I think) it is. > > I do? That was definitely not my intention. I'll look into it. Maybe it's just my bias when reading, since some people have complained loudly about this, seemingly without understanding that the problem really isn't that significant. So my question was really just to confirm my own understanding, that this only affects the 'device' symlink. BTW, it'd be nice if you don't respam with another 60 patches, if you're only changing a few of them. I can probably take most of them as-is, after you confirm there are no more compile failures. Thanks, Brian
On Tue, Mar 10, 2015 at 6:39 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Mar 10, 2015 at 08:47:46AM +0100, Frans Klaver wrote: >> On Tue, Mar 10, 2015 at 12:14 AM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > On Tue, Mar 03, 2015 at 10:39:45PM +0100, Frans Klaver wrote: >> >> add_mtd_device() has a comment suggesting that the caller should have >> >> set dev.parent. This is required to have the device show up in sysfs, >> > >> > What do you mean "have the device show up in sysfs"? AFAICT, this only >> > has bearing on whether the *parent* device shows up as a sysfs symlink >> > within the MTD device directory. i.e.: >> > >> > /sys/class/mtd/mtd*/device >> > >> > For instance, this sort of symlink: >> > >> > /sys/class/mtd/mtd0/device -> ../../../f03e2800.nand >> > >> > It might be good to clarify this in the commit message, since you make >> > the problem sound worse than (I think) it is. >> >> I do? That was definitely not my intention. I'll look into it. > > Maybe it's just my bias when reading, since some people have complained > loudly about this, seemingly without understanding that the problem > really isn't that significant. > > So my question was really just to confirm my own understanding, that > this only affects the 'device' symlink. Ah right. I'll double check and reword where necessary. I already had the feeling that this wasn't very significant, as there weren't any real issues related to this using these drivers. > BTW, it'd be nice if you don't respam with another 60 patches, if you're > only changing a few of them. I can probably take most of them as-is, > after you confirm there are no more compile failures. Sure thing, I thought as much. Thanks, Frans
On Wed, Mar 11, 2015 at 09:03:46AM +0100, Frans Klaver wrote: > On Tue, Mar 10, 2015 at 6:39 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Tue, Mar 10, 2015 at 08:47:46AM +0100, Frans Klaver wrote: > >> On Tue, Mar 10, 2015 at 12:14 AM, Brian Norris > >> <computersforpeace@gmail.com> wrote: > >> > On Tue, Mar 03, 2015 at 10:39:45PM +0100, Frans Klaver wrote: > >> >> add_mtd_device() has a comment suggesting that the caller should have > >> >> set dev.parent. This is required to have the device show up in sysfs, > >> > > >> > What do you mean "have the device show up in sysfs"? AFAICT, this only > >> > has bearing on whether the *parent* device shows up as a sysfs symlink > >> > within the MTD device directory. i.e.: > >> > > >> > /sys/class/mtd/mtd*/device > >> > > >> > For instance, this sort of symlink: > >> > > >> > /sys/class/mtd/mtd0/device -> ../../../f03e2800.nand > >> > > >> > It might be good to clarify this in the commit message, since you make > >> > the problem sound worse than (I think) it is. > >> > >> I do? That was definitely not my intention. I'll look into it. > > > > Maybe it's just my bias when reading, since some people have complained > > loudly about this, seemingly without understanding that the problem > > really isn't that significant. > > > > So my question was really just to confirm my own understanding, that > > this only affects the 'device' symlink. > > Ah right. I'll double check and reword where necessary. I already had > the feeling that this wasn't very significant, as there weren't any > real issues related to this using these drivers. OK, I just made some small edits to the language to clarify that this patch is only fixing the 'parent device symlink' rather than the 'device is missing in sysfs'. I also tweaked the language on all the other patches to reflect this. > > BTW, it'd be nice if you don't respam with another 60 patches, if you're > > only changing a few of them. I can probably take most of them as-is, > > after you confirm there are no more compile failures. > > Sure thing, I thought as much. I ran the other 59 patches through a pretty wide range of compile tests and saw no issues, so I've pushed patches 1-29 and 31-60 to l2-mtd.git, with small tweaks to the commit descriptions. If you get a chance to rework patch 30, feel free. Thanks for the patches! Brian
On 10 April 2015 06:30:00 CEST, Brian Norris <computersforpeace@gmail.com> wrote: >OK, I just made some small edits to the language to clarify that this >patch is only fixing the 'parent device symlink' rather than the >'device >is missing in sysfs'. I also tweaked the language on all the other >patches to reflect this. Alright. >I ran the other 59 patches through a pretty wide range of compile tests >and saw no issues, so I've pushed patches 1-29 and 31-60 to l2-mtd.git, >with small tweaks to the commit descriptions. If you get a chance to >rework patch 30, feel free. I was in the process of checking this, until a hd failure threw me off track. I'll be able to send a reworked 30 after the weekend. >Thanks for the patches! Thanks for testing and rewording, Frans
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 11883bd..2093676 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -422,7 +422,7 @@ int add_mtd_device(struct mtd_info *mtd) } /* Caller should have set dev.parent to match the - * physical device. + * physical device, if appropriate. */ mtd->dev.type = &mtd_devtype; mtd->dev.class = &mtd_class;
add_mtd_device() has a comment suggesting that the caller should have set dev.parent. This is required to have the device show up in sysfs, but not for proper operation of the mtd device itself. Currently we have five drivers registering mtd devices during module initialization, so they don't actually provide a parent device to link to. That means we cannot WARN_ON() here, as it would trigger false positives. Make the comment a bit less firm in its assertion that dev.parent should be set. Signed-off-by: Frans Klaver <fransklaver@gmail.com> --- drivers/mtd/mtdcore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)