| Message ID | 1307557519-31269-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote: > Also fix some punctuation, indentation, and capitalization that went > along with the affected lines. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/nand/nand_base.c | 65 ++++++++++++++++++++---------------------- > drivers/mtd/nand/nand_bbt.c | 39 ++++++++++++------------- > 2 files changed, 50 insertions(+), 54 deletions(-) > Pushed with few tweaks, thanks! > if (id_data[0] != *maf_id || id_data[1] != *dev_id) { > - printk(KERN_INFO "%s: second ID read did not match " > + pr_info("%s: second ID read did not match " > "%02x,%02x against %02x,%02x\n", __func__, > *maf_id, *dev_id, id_data[0], id_data[1]); This was not properly aligned - I've aligned it: pr_info("%s: second ID read did not match " "%02x,%02x against %02x,%02x\n", __func__, *maf_id, *dev_id, id_data[0], id_data[1]); > return ERR_PTR(-ENODEV); > @@ -3068,10 +3067,10 @@ ident_done: > * chip correct! > */ > if (busw != (chip->options & NAND_BUSWIDTH_16)) { > - printk(KERN_INFO "NAND device: Manufacturer ID:" > + pr_info("NAND device: Manufacturer ID:" > " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, > *dev_id, nand_manuf_ids[maf_idx].name, mtd->name); > - printk(KERN_WARNING "NAND bus width %d instead %d bit\n", > + pr_warning("NAND bus width %d instead %d bit\n", > (chip->options & NAND_BUSWIDTH_16) ? 16 : 8, > busw ? 16 : 8); And this, I've changed it: pr_info("NAND device: Manufacturer ID:" " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id, nand_manuf_ids[maf_idx].name, mtd->name); pr_warning("NAND bus width %d instead %d bit\n", (chip->options & NAND_BUSWIDTH_16) ? 16 : 8, busw ? 16 : 8); > - printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%012llx\n", > + pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n", > (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift); > this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06); > mtd->ecc_stats.bbtblocks++; > @@ -229,7 +229,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num, > * Leave it for now, if it's matured we can > * move this message to MTD_DEBUG_LEVEL0. > */ > - printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%012llx\n", > + pr_debug("nand_read_bbt: Bad block at 0x%012llx\n", > (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift); Here I aligned lines too. They are also quite long, but that would need to put this piece of code to a separate function, which is a different story. > /* Factory marked bad or worn out? */ > if (tmp == 0) > @@ -383,7 +383,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf, > scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift, > mtd->writesize, td); > td->version[0] = buf[bbt_get_ver_offs(mtd, td)]; > - printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n", > + pr_debug("Bad block table at page %d, version 0x%02X\n", > td->pages[0], td->version[0]); Was not aligned. > } > > @@ -392,7 +392,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf, > scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift, > mtd->writesize, td); > md->version[0] = buf[bbt_get_ver_offs(mtd, md)]; > - printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n", > + pr_debug("Bad block table at page %d, version 0x%02X\n", > md->pages[0], md->version[0]); Was not aligned. > + pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n", > chip + 1, this->numchips); And this and other, I amended them. > + pr_info("nand_bbt: Error " > "reading block for writing " > "the bad block table\n"); > return res; > } > - printk(KERN_WARNING "nand_bbt: ECC error " > + pr_warn("nand_bbt: ECC error " > "while reading block for writing " > "bad block table\n"); Tweaked these 2. Plus some other minor tweaks.
On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote: > - printk(KERN_DEBUG "Bad block table written to 0x%012llx, version " > + pr_debug("Bad block table written to 0x%012llx, version " > "0x%02X\n", (unsigned long long)to, td->version[chip]); Hmm, the side effect of this change is this message will disappear if DEBUG is undefined / dynamic debugging is disabled. So I think we need to change all these pr_debug to pr_info, because this is really just info.
On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote: > Also fix some punctuation, indentation, and capitalization that went > along with the affected lines. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/nand/nand_base.c | 65 ++++++++++++++++++++---------------------- > drivers/mtd/nand/nand_bbt.c | 39 ++++++++++++------------- > 2 files changed, 50 insertions(+), 54 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 019187b..7550cea 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2167,8 +2167,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, > > /* Reject writes, which are not page aligned */ > if (NOTALIGNED(to) || NOTALIGNED(ops->len)) { > - printk(KERN_NOTICE "%s: Attempt to write not " > - "page aligned data\n", __func__); > + pr_notice("%s: attempt to write non page aligned data\n", > + __func__); > return -EINVAL; Wait, and why are we using pr_* while it is better to use dbg_* ? :-)
Brian, would you please send a new series? I suggest you the following
order of things.
1. clean-up of non-DEBUG() messages.
1.1. go through all printks and check if KERN_* is ok, fix. E.g.,
I can see that all KERN_DEBUG should become KERN_INFO
1.2. go through all printks and check if the function name prefix
makes sense there - kill those which do not.
Also, while doing this, keep in mind that messages will be later
turned into dev_* (dev_info(), dev_err(), etc) so they will be
automatically prefixed with the device name which is already some
identification of the source of the message.
1.3. turn messages to dev_*
1.4. make another pass and do all the lines consolidations,
alignments, punctuation, etc etc.
2. clean-up of DEBUG() cruft.
2.1 change DEBUG() with dev_dbg()
Kill all function name prefixes as well. The rationale is: dev_dbg()
already adds function name prefixe (and process id) if dynamic
debugging is enabled. In case of debugging with DEBUG macro, you
anyway have to change the code (adding #define DEBUG) so you might
easily defind pr_fmt at the same time for youself.
How does this sound to you?
FWIW: I've moved the patches I pushed from the master branch to the
"brian" branch of l2-mtd-2.6.git tree. But they will not be useful
for you, I guess.
On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Wait, and why are we using pr_* while it is better to use dbg_* ? :-)
Just so we're on the same page...did you say "dbg_*" when you really
meant "dev_*"?
Anyway, the answer is: I'm mostly trying to do as little shakeup as
possible, so "printk" could translate pretty directly to the "pr_*"
with shorter code, etc. If it makes as much (or more) sense to have
the device name on all the prints, then I can just as well do it that
way.
Brian
On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote: > On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > Wait, and why are we using pr_* while it is better to use dbg_* ? :-) > > Just so we're on the same page...did you say "dbg_*" when you really > meant "dev_*"? Yes, I meant dev_* > Anyway, the answer is: I'm mostly trying to do as little shakeup as > possible, so "printk" could translate pretty directly to the "pr_*" > with shorter code, etc. If it makes as much (or more) sense to have > the device name on all the prints, then I can just as well do it that > way. Yes, I think it makes much more sense to specify for which device (out of possibly many!) this message belongs.
On Thu, Jun 9, 2011 at 1:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > Brian, would you please send a new series? I suggest you the following > order of things. > > 1. clean-up of non-DEBUG() messages. > 1.1. go through all printks and check if KERN_* is ok, fix. E.g., > I can see that all KERN_DEBUG should become KERN_INFO > 1.2. go through all printks and check if the function name prefix > makes sense there - kill those which do not. > > Also, while doing this, keep in mind that messages will be later > turned into dev_* (dev_info(), dev_err(), etc) so they will be > automatically prefixed with the device name which is already some > identification of the source of the message. > > 1.3. turn messages to dev_* > 1.4. make another pass and do all the lines consolidations, > alignments, punctuation, etc etc. > > 2. clean-up of DEBUG() cruft. > 2.1 change DEBUG() with dev_dbg() > > Kill all function name prefixes as well. The rationale is: dev_dbg() > already adds function name prefixe (and process id) if dynamic > debugging is enabled. In case of debugging with DEBUG macro, you > anyway have to change the code (adding #define DEBUG) so you might > easily defind pr_fmt at the same time for youself. > > How does this sound to you? Sounds good. Will send patches later today, I think. > FWIW: I've moved the patches I pushed from the master branch to the > "brian" branch of l2-mtd-2.6.git tree. But they will not be useful > for you, I guess. Thanks, although I will have to rework all of them :) Brian
On Thu, Jun 9, 2011 at 9:22 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Thu, Jun 9, 2011 at 1:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: >> Brian, would you please send a new series? I suggest you the following >> order of things. ... >> How does this sound to you? > > Sounds good. Will send patches later today, I think. Hmm, actually, I'll get to this again on Monday! Have a good weekend. Brian
On Thu, Jun 9, 2011 at 9:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote: >> On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: >> > Wait, and why are we using pr_* while it is better to use dbg_* ? :-) >> >> Just so we're on the same page...did you say "dbg_*" when you really >> meant "dev_*"? > > Yes, I meant dev_* > >> Anyway, the answer is: I'm mostly trying to do as little shakeup as >> possible, so "printk" could translate pretty directly to the "pr_*" >> with shorter code, etc. If it makes as much (or more) sense to have >> the device name on all the prints, then I can just as well do it that >> way. > > Yes, I think it makes much more sense to specify for which device (out > of possibly many!) this message belongs. I've been testing some of the "dev_*" printing, and it seems as if our mtd_info structs never have fully initialized "device" fields (i.e., mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name, etc. are never filled in with anything meaningful). That means that our dev_* messages do not have anything to work from and simply print " (null): " references before our strings instead of device driver/name information, for example: (null): ONFI flash detected I'm a little new to the Linux device model, so I'm not sure if it's safe and sensible to add lines to a driver's nand_probe function, like, for plat_nand.c: data->mtd.dev.driver = pdev->dev.driver; data->mtd.dev.init_name = dev_name(&pdev->dev); Is there some better way to initialize these device values? Or should we scrap the whole dev_* thing since MTD doesn't match the device model well enough? Brian
On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: > On Thu, Jun 9, 2011 at 9:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote: > >> On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > >> > Wait, and why are we using pr_* while it is better to use dbg_* ? :-) > >> > >> Just so we're on the same page...did you say "dbg_*" when you really > >> meant "dev_*"? > > > > Yes, I meant dev_* > > > >> Anyway, the answer is: I'm mostly trying to do as little shakeup as > >> possible, so "printk" could translate pretty directly to the "pr_*" > >> with shorter code, etc. If it makes as much (or more) sense to have > >> the device name on all the prints, then I can just as well do it that > >> way. > > > > Yes, I think it makes much more sense to specify for which device (out > > of possibly many!) this message belongs. > > I've been testing some of the "dev_*" printing, and it seems as if our > mtd_info structs never have fully initialized "device" fields (i.e., > mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name, > etc. are never filled in with anything meaningful). That means that > our dev_* messages do not have anything to work from and simply print > " (null): " references before our strings instead of device > driver/name information, for example: > > (null): ONFI flash detected Ouch, good finding. > I'm a little new to the Linux device model, so I'm not sure if it's > safe and sensible to add lines to a driver's nand_probe function, > like, for plat_nand.c: > data->mtd.dev.driver = pdev->dev.driver; > data->mtd.dev.init_name = dev_name(&pdev->dev); I think so, I believe struct device was added to mtd by some non-MTD guy to just make it complaint with one of interface changes which required struct device. So please, go ahead and improve that a bit. > Is there some better way to initialize these device values? Or should > we scrap the whole dev_* thing since MTD doesn't match the device > model well enough? I think there is no reason why it would not match the model, so of course fixing this is preferable...
Artem Bityutskiy wrote: > On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: >> I'm a little new to the Linux device model, so I'm not sure if it's >> safe and sensible to add lines to a driver's nand_probe function, >> like, for plat_nand.c: >> data->mtd.dev.driver = pdev->dev.driver; IIUC, we should not set driver, but it might be better to consult with more experienced DM hackers (like Greg Kroah-Hartman). >> data->mtd.dev.init_name = dev_name(&pdev->dev); > > I think so, I believe struct device was added to mtd by some non-MTD guy > to just make it complaint with one of interface changes which required > struct device. So please, go ahead and improve that a bit.
On Wed, Jun 22, 2011 at 2:12 AM, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Artem Bityutskiy wrote: > >> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: >>> I'm a little new to the Linux device model, so I'm not sure if it's >>> safe and sensible to add lines to a driver's nand_probe function, >>> like, for plat_nand.c: >>> data->mtd.dev.driver = pdev->dev.driver; > > IIUC, we should not set driver, but it might be better to consult with > more experienced DM hackers (like Greg Kroah-Hartman). > >>> data->mtd.dev.init_name = dev_name(&pdev->dev); >> >> I think so, I believe struct device was added to mtd by some non-MTD guy >> to just make it complaint with one of interface changes which required >> struct device. So please, go ahead and improve that a bit. I've dug a little more on this, especially in the RFC + mailing list conversation that led to adding the "mtd_info.dev" field. See here: http://lists.infradead.org/pipermail/linux-mtd/2009-April/025142.html It seems that, according to the following comment, the master MTD node (which is passed to most of our functions) is never registered and so does not have valid information for printing dev_* information: /* NOTE: we don't arrange MTDs as a tree; it'd be error-prone * to have the same data be in two different partitions. */ So it seems that, by "design," mtd->dev will never be suitable for a dev_* message (at least when "mtd" is the master node), so instead of: dev_info(&mtd->dev, "informational message\n"); we should do: dev_info(mtd->dev.parent, "informational message\n"); This utilizes the physical device instead of the MTD device for debug messages. Unfortunately, this kinda makes some of our code longer, rather than more concise (not a huge deal, I guess). Also, it relies on a specific, potentially-flawed device tree layout. Perhaps there should be a "mtd_to_dev" helper (accompanying "dev_to_mtd") to abstract this a little and leave room for future tree changes? For example: static inline struct device *mtd_to_dev(struct mtd_info *mtd) { return mtd ? mtd->dev.parent : NULL; } That makes the previous example into: dev_info(mtd_to_dev(mtd), "informational message\n"); I'll send out a patch set if any of this makes sense. Brian
On Wed, Jul 6, 2011 at 11:51 AM, Brian Norris <computersforpeace@gmail.com> wrote: > static inline struct device *mtd_to_dev(struct mtd_info *mtd) > { > return mtd ? mtd->dev.parent : NULL; > } Second thought on this function; we may not even need to worry about the "mtd == NULL" case. But we might want to handle the case that mtd->dev actually *is* registered properly: static inline struct device *mtd_to_dev(struct mtd_info *mtd) { if (device_is_registered(&mtd->dev)) return &mtd->dev; return mtd->dev.parent; } Brian
On 7/6/11, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Jul 6, 2011 at 11:51 AM, Brian Norris > <computersforpeace@gmail.com> wrote: >> static inline struct device *mtd_to_dev(struct mtd_info *mtd) >> { >> return mtd ? mtd->dev.parent : NULL; >> } > > Second thought on this function; we may not even need to worry about > the "mtd == NULL" case. But we might want to handle the case that > mtd->dev actually *is* registered properly: > > static inline struct device *mtd_to_dev(struct mtd_info *mtd) > { > if (device_is_registered(&mtd->dev)) > return &mtd->dev; > return mtd->dev.parent; And what if mtd->dev.parent is NULL?
On Wed, Jul 6, 2011 at 12:47 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> And what if mtd->dev.parent is NULL?
Then we're out of luck and simply return NULL.
NULL shouldn't cause an error for debug messages, and if mtd_to_dev is
used for something more serious, then you have a problem you must fix
if mtd->dev.parent is NULL.
On Wed, 2011-07-06 at 11:51 -0700, Brian Norris wrote: > On Wed, Jun 22, 2011 at 2:12 AM, Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com> wrote: > > Artem Bityutskiy wrote: > > > >> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: > >>> I'm a little new to the Linux device model, so I'm not sure if it's > >>> safe and sensible to add lines to a driver's nand_probe function, > >>> like, for plat_nand.c: > >>> data->mtd.dev.driver = pdev->dev.driver; > > > > IIUC, we should not set driver, but it might be better to consult with > > more experienced DM hackers (like Greg Kroah-Hartman). > > > >>> data->mtd.dev.init_name = dev_name(&pdev->dev); > >> > >> I think so, I believe struct device was added to mtd by some non-MTD guy > >> to just make it complaint with one of interface changes which required > >> struct device. So please, go ahead and improve that a bit. > > I've dug a little more on this, especially in the RFC + mailing list > conversation that led to adding the "mtd_info.dev" field. See here: > http://lists.infradead.org/pipermail/linux-mtd/2009-April/025142.html > > It seems that, according to the following comment, the master MTD node > (which is passed to most of our functions) is never registered and so > does not have valid information for printing dev_* information: > /* NOTE: we don't arrange MTDs as a tree; it'd be error-prone > * to have the same data be in two different partitions. > */ Sorry, I still don't get it, why mtd partitions do not have names. The discussion above is about who will be the parent, while we care about making dev_dbg(mtd->dev, ...) print names. if we look at 'add_mtd_device()' in mtdcore.c, we see something like full initialization: /* Caller should have set dev.parent to match the * physical device. */ mtd->dev.type = &mtd_devtype; mtd->dev.class = &mtd_class; mtd->dev.devt = MTD_DEVT(i); dev_set_name(&mtd->dev, "mtd%d", i); dev_set_drvdata(&mtd->dev, mtd); if (device_register(&mtd->dev) != 0) goto fail_added; if (MTD_DEVT(i)) device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL, "mtd%dro", i); And if we look at 'mtd_add_partition()', we see that 'add_mtd_device()' is also called, so mtd->dev should be initialized. What exactly is the problem?
On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: > > I've been testing some of the "dev_*" printing, and it seems as if our > mtd_info structs never have fully initialized "device" fields (i.e., > mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name, > etc. are never filled in with anything meaningful). That means that > our dev_* messages do not have anything to work from and simply print > " (null): " references before our strings instead of device > driver/name information, for example: > > (null): ONFI flash detected You mean that dev_info(&mtd->dev, "ONFI flash detected"); produces this?
Hi, I included some comments along with some of you questions and tried to summarize and answer more at the end. The end comments may explain the middle... On Wed, Jul 6, 2011 at 11:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2011-07-06 at 11:51 -0700, Brian Norris wrote: >> It seems that, according to the following comment, the master MTD node >> (which is passed to most of our functions) is never registered and so >> does not have valid information for printing dev_* information: >> /* NOTE: we don't arrange MTDs as a tree; it'd be error-prone >> * to have the same data be in two different partitions. >> */ > > Sorry, I still don't get it, why mtd partitions do not have names. The > discussion above is about who will be the parent, while we care about > making dev_dbg(mtd->dev, ...) print names. The *partitions* have names, but the master does not. We end up with a device tree that's something like this: <physical_device> |_ mtd0 (slave->dev) |_ mtd0ro |_ mtd1 |_ mtd1ro |_ ... ... master mtd->dev? where each partition (and RO counterpart) in the tree is registered using add_mtd_partitions() and each partition's parent is the physical device. The master mtd->dev is floating in space, since it isn't registered anywhere. I think I meant to copy a different code comment onto the previous e-mail as well: /* ... * We don't register the master, or expect the caller to have done so, * for reasons of data integrity. */ So the master mtd->dev, which is used in much of our MTD initialization code, is never named and registered properly. > if we look at 'add_mtd_device()' in mtdcore.c, we see something like > full initialization: > > /* Caller should have set dev.parent to match the > * physical device. > */ > mtd->dev.type = &mtd_devtype; > mtd->dev.class = &mtd_class; > mtd->dev.devt = MTD_DEVT(i); > dev_set_name(&mtd->dev, "mtd%d", i); > dev_set_drvdata(&mtd->dev, mtd); > if (device_register(&mtd->dev) != 0) > goto fail_added; > > if (MTD_DEVT(i)) > device_create(&mtd_class, mtd->dev.parent, > MTD_DEVT(i) + 1, > NULL, "mtd%dro", i); When called from mtd_add_partitions, this is only called for each partition, not the master. That is the crux of the problem. > And if we look at 'mtd_add_partition()', we see that 'add_mtd_device()' > is also called, so mtd->dev should be initialized. > > What exactly is the problem? I believe the problem is simply that partitions are named and registered, whereas the master is not. During initialization, all debug messages, etc. are run through the master MTD, not the named partitions. This gives the "(null):" dev_* messages I saw on bootup. I'm not sure if the master device causes much trouble after initialization...I think it kinda fades away and leaves the partitions for interaction with the user. FYI, I'm looking mostly at cases where there *is* a partition layout. In cases where there are no partitions, the driver will usually just call "add_mtd_device" on the master MTD, and so this device be named and registered properly. This isn't so much an issue, except that it provides inconsistency between setups that use partitions and those that don't. (Couldn't we just force everyone to use partitions? Users who don't need them could just form a single partition for the whole device...) Brian
On Thu, Jul 7, 2011 at 12:01 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote: >> (null): ONFI flash detected > > You mean that > > dev_info(&mtd->dev, "ONFI flash detected"); > > produces this? Yes, sorry for not being specific.
On Thu, 2011-07-07 at 10:00 -0700, Brian Norris wrote: > I believe the problem is simply that partitions are named and > registered, whereas the master is not. During initialization, all > debug messages, etc. are run through the master MTD, not the named > partitions. This gives the "(null):" dev_* messages I saw on bootup. > I'm not sure if the master device causes much trouble after > initialization...I think it kinda fades away and leaves the partitions > for interaction with the user. > > FYI, I'm looking mostly at cases where there *is* a partition layout. > In cases where there are no partitions, the driver will usually just > call "add_mtd_device" on the master MTD, and so this device be named > and registered properly. This isn't so much an issue, except that it > provides inconsistency between setups that use partitions and those > that don't. (Couldn't we just force everyone to use partitions? Users > who don't need them could just form a single partition for the whole > device...) Well, ok, indeed, in the driver level we have no idea about partitions, we deal with the flash chip, by design... Probably we should better use pr_* series of functions instead, I guess, as dev_* functions seem to not be really suitable for us. That's what you have originally done, sorry for bad suggestion. On the other hand, why we cannot pass the partition struct mtd_info down to the driver instead? Well, ideally, drivers should not use struct mtd_info at all. But this is probably a different story...
Now I'm not sure I entirely understand all your comments :) On Thu, Jul 7, 2011 at 12:56 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > Well, ok, indeed, in the driver level we have no idea about partitions, > we deal with the flash chip, by design... What exactly are you referring to? That the inability for drivers to distinguish between partitions and the master MTD prevents them from handling our device structure correctly? I thought we're talking mostly about the NAND layer, not the driver layer. > Probably we should better use pr_* series of functions instead, I guess, > as dev_* functions seem to not be really suitable for us. That's what > you have originally done, sorry for bad suggestion. OK, I can do that, but first, what about the code I posted earlier for finding the correct device corresponding to mtd_info? static inline struct device *mtd_to_dev(struct mtd_info *mtd) { if (device_is_registered(&mtd->dev)) return &mtd->dev; return mtd->dev.parent; } I think this would work for any NAND or MTD code, so that we can do something like this: dev_info(mtd_to_dev(mtd), "ONFI flash detected\n"); In NAND/MTD code, we basically have two cases for the current mtd_info: 1) corresponds to the master device, which is not registered. Its parent should be set to the physical device, so use mtd->dev.parent. 2) corresponds to a partition, which is registered. Use the partition device &mtd->dev. There's kind of a third case for non-partitioned flash: 3) corresponds to the master device, which is registered. Use &mtd->dev These cases should all be covered, and I think the dev_* could would be just fine. If that's not good enough though, then I'll just go back to the pr_* code. > On the other hand, why we cannot pass the partition struct mtd_info down > to the driver instead? Well, ideally, drivers should not use struct > mtd_info at all. But this is probably a different story... Not quite sure where this is coming from. I don't believe I asked about passing mtd_info to the driver (which happens all the time anyway, AFAICT...). To be clear, what are the exact layers as you're seeing them? It doesn't seem like we strictly follow them, but I see something like this: Filesystems MTD NAND Driver (Hardware) Anyway, this thread has managed to diverge into several different topics by now, and it all came from some simple printk stuff... maybe it's worth wrapping up the printk's and handling any device issues separately. Brian
Hi, sorry, I was very busy so could not reply. On Fri, 2011-07-08 at 09:06 -0700, Brian Norris wrote: > On Thu, Jul 7, 2011 at 12:56 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > Well, ok, indeed, in the driver level we have no idea about partitions, > > we deal with the flash chip, by design... > > What exactly are you referring to? That the inability for drivers to > distinguish between partitions and the master MTD prevents them from > handling our device structure correctly? I thought we're talking > mostly about the NAND layer, not the driver layer. I meant that at mtdparts hides translates requests to partitions into requests to the master MTD device, and in nand_base all requests look like requests to the master MTD device, so it is logical that and nand_base level we cannot easily prefix our messages with "mtdX:", where X is the partition number. > > Probably we should better use pr_* series of functions instead, I guess, > > as dev_* functions seem to not be really suitable for us. That's what > > you have originally done, sorry for bad suggestion. > > OK, I can do that, but first, what about the code I posted earlier for > finding the correct device corresponding to mtd_info? > > static inline struct device *mtd_to_dev(struct mtd_info *mtd) > { > if (device_is_registered(&mtd->dev)) > return &mtd->dev; > return mtd->dev.parent; > } I think I just do not really understand it. At nand_base "struct mtd_info *mtd" will always be the master device, right? Then using it will make messages confusing - they will all start with, say, "mtd0" for all the partitions on this flash chip. So dev_*() become kind of useless, right? Then why using them if we can use pr_*() which do not require the device and will not have confusing prefix? > > I think this would work for any NAND or MTD code, so that we can do > something like this: > dev_info(mtd_to_dev(mtd), "ONFI flash detected\n"); > In NAND/MTD code, we basically have two cases for the current mtd_info: > 1) corresponds to the master device, which is not registered. Its > parent should be set to the physical device, so use mtd->dev.parent. > 2) corresponds to a partition, which is registered. Use the partition > device &mtd->dev. Probably this is the point of confusion. How can mtd->dev in nand_base correspond to an mtd partition if mtdpart is designed so that it translates partition requests into master MTD device requests? E.g., see part_read() in drivers/mtd/mtdpart.c > There's kind of a third case for non-partitioned flash: > 3) corresponds to the master device, which is registered. Use &mtd->dev > > These cases should all be covered, and I think the dev_* could would > be just fine. If that's not good enough though, then I'll just go back > to the pr_* code. IMO, the only advantage of using dev_* is that they prefix messages with the name of device the messages belong to. MTD is designed so that from user perspective partitions and the whole device are not distinguishable, not like in hard drives with with, say, sda,sda1,sda2 - we have mtd0, mtd1, mtd2... So I think pr_*() is just more suitable. > > On the other hand, why we cannot pass the partition struct mtd_info down > > to the driver instead? Well, ideally, drivers should not use struct > > mtd_info at all. But this is probably a different story... > > Not quite sure where this is coming from. I don't believe I asked > about passing mtd_info to the driver (which happens all the time > anyway, AFAICT...). To be clear, what are the exact layers as you're > seeing them? It doesn't seem like we strictly follow them, but I see > something like this: Oh, I just meant that we have struct mtd_info and struct nand_chip. I think struct mtd_info should be only for MTD clients, and when we go down to the nand_base we should not use it at all, we should have all the information in struct nand_chip.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 019187b..7550cea 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2167,8 +2167,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, /* Reject writes, which are not page aligned */ if (NOTALIGNED(to) || NOTALIGNED(ops->len)) { - printk(KERN_NOTICE "%s: Attempt to write not " - "page aligned data\n", __func__); + pr_notice("%s: attempt to write non page aligned data\n", + __func__); return -EINVAL; } @@ -2561,8 +2561,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr, /* Heck if we have a bad block, we do not erase bad blocks! */ if (nand_block_checkbad(mtd, ((loff_t) page) << chip->page_shift, 0, allowbbt)) { - printk(KERN_WARNING "%s: attempt to erase a bad block " - "at page 0x%08x\n", __func__, page); + pr_warning("%s: attempt to erase a bad block at page 0x%08x\n", + __func__, page); instr->state = MTD_ERASE_FAILED; goto erase_exit; } @@ -2735,8 +2735,8 @@ static void nand_resume(struct mtd_info *mtd) if (chip->state == FL_PM_SUSPENDED) nand_release_device(mtd); else - printk(KERN_ERR "%s called for a chip which is not " - "in suspended state\n", __func__); + pr_err("%s called for a chip which is not in suspended state\n", + __func__); } /* Set default functions */ @@ -2827,13 +2827,13 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') return 0; - printk(KERN_INFO "ONFI flash detected\n"); + pr_info("ONFI flash detected\n"); chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); for (i = 0; i < 3; i++) { chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == le16_to_cpu(p->crc)) { - printk(KERN_INFO "ONFI param page %d valid\n", i); + pr_info("ONFI param page %d valid\n", i); break; } } @@ -2857,8 +2857,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, chip->onfi_version = 0; if (!chip->onfi_version) { - printk(KERN_INFO "%s: unsupported ONFI version: %d\n", - __func__, val); + pr_info("%s: unsupported ONFI version: %d\n", __func__, val); return 0; } @@ -2923,7 +2922,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, id_data[i] = chip->read_byte(mtd); if (id_data[0] != *maf_id || id_data[1] != *dev_id) { - printk(KERN_INFO "%s: second ID read did not match " + pr_info("%s: second ID read did not match " "%02x,%02x against %02x,%02x\n", __func__, *maf_id, *dev_id, id_data[0], id_data[1]); return ERR_PTR(-ENODEV); @@ -3068,10 +3067,10 @@ ident_done: * chip correct! */ if (busw != (chip->options & NAND_BUSWIDTH_16)) { - printk(KERN_INFO "NAND device: Manufacturer ID:" + pr_info("NAND device: Manufacturer ID:" " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id, nand_manuf_ids[maf_idx].name, mtd->name); - printk(KERN_WARNING "NAND bus width %d instead %d bit\n", + pr_warning("NAND bus width %d instead %d bit\n", (chip->options & NAND_BUSWIDTH_16) ? 16 : 8, busw ? 16 : 8); return ERR_PTR(-EINVAL); @@ -3129,7 +3128,7 @@ ident_done: if (mtd->writesize > 512 && chip->cmdfunc == nand_command) chip->cmdfunc = nand_command_lp; - printk(KERN_INFO "NAND device: Manufacturer ID:" + pr_info("NAND device: Manufacturer ID:" " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id, nand_manuf_ids[maf_idx].name, chip->onfi_version ? chip->onfi_params.model : type->name); @@ -3166,7 +3165,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips, if (IS_ERR(type)) { if (!(chip->options & NAND_SCAN_SILENT_NODEV)) - printk(KERN_WARNING "No NAND device found.\n"); + pr_warning("No NAND device found\n"); chip->select_chip(mtd, -1); return PTR_ERR(type); } @@ -3184,7 +3183,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips, break; } if (i > 1) - printk(KERN_INFO "%d NAND chips detected\n", i); + pr_info("%d NAND chips detected\n", i); /* Store the number of chips and calc total size for mtd */ chip->numchips = i; @@ -3234,8 +3233,8 @@ int nand_scan_tail(struct mtd_info *mtd) chip->ecc.layout = &nand_oob_128; break; default: - printk(KERN_WARNING "No oob scheme defined for " - "oobsize %d\n", mtd->oobsize); + pr_warning("No oob scheme defined for oobsize %d\n", + mtd->oobsize); BUG(); } } @@ -3253,8 +3252,8 @@ int nand_scan_tail(struct mtd_info *mtd) /* Similar to NAND_ECC_HW, but a separate read_page handle */ if (!chip->ecc.calculate || !chip->ecc.correct || !chip->ecc.hwctl) { - printk(KERN_WARNING "No ECC functions supplied; " - "Hardware ECC not possible\n"); + pr_warning("No ECC functions supplied; " + "hardware ECC not possible\n"); BUG(); } if (!chip->ecc.read_page) @@ -3282,8 +3281,8 @@ int nand_scan_tail(struct mtd_info *mtd) chip->ecc.read_page == nand_read_page_hwecc || !chip->ecc.write_page || chip->ecc.write_page == nand_write_page_hwecc)) { - printk(KERN_WARNING "No ECC functions supplied; " - "Hardware ECC not possible\n"); + pr_warning("No ECC functions supplied; " + "hardware ECC not possible\n"); BUG(); } /* Use standard syndrome read/write page function? */ @@ -3302,9 +3301,9 @@ int nand_scan_tail(struct mtd_info *mtd) if (mtd->writesize >= chip->ecc.size) break; - printk(KERN_WARNING "%d byte HW ECC not possible on " - "%d byte page size, fallback to SW ECC\n", - chip->ecc.size, mtd->writesize); + pr_warning("%d byte HW ECC not possible on " + "%d byte page size, fallback to SW ECC\n", + chip->ecc.size, mtd->writesize); chip->ecc.mode = NAND_ECC_SOFT; case NAND_ECC_SOFT: @@ -3324,7 +3323,7 @@ int nand_scan_tail(struct mtd_info *mtd) case NAND_ECC_SOFT_BCH: if (!mtd_nand_has_bch()) { - printk(KERN_WARNING "CONFIG_MTD_ECC_BCH not enabled\n"); + pr_warning("CONFIG_MTD_ECC_BCH not enabled\n"); BUG(); } chip->ecc.calculate = nand_bch_calculate_ecc; @@ -3351,14 +3350,14 @@ int nand_scan_tail(struct mtd_info *mtd) chip->ecc.bytes, &chip->ecc.layout); if (!chip->ecc.priv) { - printk(KERN_WARNING "BCH ECC initialization failed!\n"); + pr_warning("BCH ECC initialization failed!\n"); BUG(); } break; case NAND_ECC_NONE: - printk(KERN_WARNING "NAND_ECC_NONE selected by board driver. " - "This is not recommended !!\n"); + pr_warning("NAND_ECC_NONE selected by board driver. " + "This is not recommended!\n"); chip->ecc.read_page = nand_read_page_raw; chip->ecc.write_page = nand_write_page_raw; chip->ecc.read_oob = nand_read_oob_std; @@ -3370,8 +3369,7 @@ int nand_scan_tail(struct mtd_info *mtd) break; default: - printk(KERN_WARNING "Invalid NAND_ECC_MODE %d\n", - chip->ecc.mode); + pr_warning("Invalid NAND_ECC_MODE %d\n", chip->ecc.mode); BUG(); } @@ -3392,7 +3390,7 @@ int nand_scan_tail(struct mtd_info *mtd) */ chip->ecc.steps = mtd->writesize / chip->ecc.size; if (chip->ecc.steps * chip->ecc.size != mtd->writesize) { - printk(KERN_WARNING "Invalid ecc parameters\n"); + pr_warning("Invalid ecc parameters\n"); BUG(); } chip->ecc.total = chip->ecc.steps * chip->ecc.bytes; @@ -3483,8 +3481,7 @@ int nand_scan(struct mtd_info *mtd, int maxchips) /* Many callers got this wrong, so check for it for a while... */ if (!mtd->owner && caller_is_module()) { - printk(KERN_CRIT "%s called with NULL mtd->owner!\n", - __func__); + pr_crit("%s called with NULL mtd->owner!\n", __func__); BUG(); } diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c index 8875d6d..d7221df 100644 --- a/drivers/mtd/nand/nand_bbt.c +++ b/drivers/mtd/nand/nand_bbt.c @@ -205,10 +205,10 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num, res = mtd->read(mtd, from, len, &retlen, buf); if (res < 0) { if (retlen != len) { - printk(KERN_INFO "nand_bbt: Error reading bad block table\n"); + pr_info("nand_bbt: Error reading bad block table\n"); return res; } - printk(KERN_WARNING "nand_bbt: ECC error while reading bad block table\n"); + pr_warn("nand_bbt: ECC error while reading bad block table\n"); } /* Analyse data */ @@ -219,7 +219,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num, if (tmp == msk) continue; if (reserved_block_code && (tmp == reserved_block_code)) { - printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%012llx\n", + pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n", (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift); this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06); mtd->ecc_stats.bbtblocks++; @@ -229,7 +229,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num, * Leave it for now, if it's matured we can * move this message to MTD_DEBUG_LEVEL0. */ - printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%012llx\n", + pr_debug("nand_read_bbt: Bad block at 0x%012llx\n", (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift); /* Factory marked bad or worn out? */ if (tmp == 0) @@ -383,7 +383,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf, scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift, mtd->writesize, td); td->version[0] = buf[bbt_get_ver_offs(mtd, td)]; - printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n", + pr_debug("Bad block table at page %d, version 0x%02X\n", td->pages[0], td->version[0]); } @@ -392,7 +392,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf, scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift, mtd->writesize, td); md->version[0] = buf[bbt_get_ver_offs(mtd, md)]; - printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n", + pr_debug("Bad block table at page %d, version 0x%02X\n", md->pages[0], md->version[0]); } return 1; @@ -466,7 +466,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, loff_t from; size_t readlen; - printk(KERN_INFO "Scanning device for bad blocks\n"); + pr_info("Scanning device for bad blocks\n"); if (bd->options & NAND_BBT_SCANALLPAGES) len = 1 << (this->bbt_erase_shift - this->page_shift); @@ -495,7 +495,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, from = 0; } else { if (chip >= this->numchips) { - printk(KERN_WARNING "create_bbt(): chipnr (%d) > available chips (%d)\n", + pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n", chip + 1, this->numchips); return -EINVAL; } @@ -524,7 +524,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, if (ret) { this->bbt[i >> 3] |= 0x03 << (i & 0x6); - printk(KERN_WARNING "Bad eraseblock %d at 0x%012llx\n", + pr_warn("Bad eraseblock %d at 0x%012llx\n", i >> 1, (unsigned long long)from); mtd->ecc_stats.badblocks++; } @@ -607,10 +607,10 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr /* Check, if we found a bbt for each requested chip */ for (i = 0; i < chips; i++) { if (td->pages[i] == -1) - printk(KERN_WARNING "Bad block table not found for chip %d\n", i); + pr_warn("Bad block table not found for chip %d\n", i); else - printk(KERN_DEBUG "Bad block table found at page %d, version 0x%02X\n", td->pages[i], - td->version[i]); + pr_debug("Bad block table found at page %d, version " + "0x%02X\n", td->pages[i], td->version[i]); } return 0; } @@ -723,7 +723,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf, if (!md || md->pages[chip] != page) goto write; } - printk(KERN_ERR "No space left to write bad block table\n"); + pr_err("No space left to write bad block table\n"); return -ENOSPC; write: @@ -758,12 +758,12 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf, res = mtd->read(mtd, to, len, &retlen, buf); if (res < 0) { if (retlen != len) { - printk(KERN_INFO "nand_bbt: Error " + pr_info("nand_bbt: Error " "reading block for writing " "the bad block table\n"); return res; } - printk(KERN_WARNING "nand_bbt: ECC error " + pr_warn("nand_bbt: ECC error " "while reading block for writing " "bad block table\n"); } @@ -840,7 +840,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf, if (res < 0) goto outerr; - printk(KERN_DEBUG "Bad block table written to 0x%012llx, version " + pr_debug("Bad block table written to 0x%012llx, version " "0x%02X\n", (unsigned long long)to, td->version[chip]); /* Mark it as used */ @@ -849,8 +849,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf, return 0; outerr: - printk(KERN_WARNING - "nand_bbt: Error while writing bad block table %d\n", res); + pr_warning("nand_bbt: Error while writing bad block table %d\n", res); return res; } @@ -1130,7 +1129,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd) */ if (!td) { if ((res = nand_memory_bbt(mtd, bd))) { - printk(KERN_ERR "nand_bbt: Can't scan flash and build the RAM-based BBT\n"); + pr_err("nand_bbt: Can't scan flash and build the RAM-based BBT\n"); kfree(this->bbt); this->bbt = NULL; } @@ -1298,7 +1297,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this) { struct nand_bbt_descr *bd; if (this->badblock_pattern) { - printk(KERN_WARNING "BBT descr already allocated; not replacing.\n"); + pr_warn("BBT descr already allocated; not replacing\n"); return -EINVAL; } bd = kzalloc(sizeof(*bd), GFP_KERNEL);
Also fix some punctuation, indentation, and capitalization that went along with the affected lines. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/nand_base.c | 65 ++++++++++++++++++++---------------------- drivers/mtd/nand/nand_bbt.c | 39 ++++++++++++------------- 2 files changed, 50 insertions(+), 54 deletions(-)