Re: [PATCH] libnvdimm: remove redundant __func__ in dev_dbg
On Tue, Mar 06, 2018 at 08:59:11AM -0800, Dan Williams wrote: > >> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c > >> index e068d72b4357..df17f1cd696d 100644 > >> --- a/drivers/nvdimm/badrange.c > >> +++ b/drivers/nvdimm/badrange.c > >> @@ -176,8 +176,7 @@ static void set_badblock(struct badblocks *bb, > >> sector_t s, int num) > >> (u64) s * 512, (u64) num * 512); > >> /* this isn't an error as the hardware will still throw an exception > >> */ > >> if (badblocks_set(bb, s, num, 1)) > >> - dev_info_once(bb->dev, "%s: failed for sector %llx\n", > >> - __func__, (u64) s); > >> + dev_info_once(bb->dev, "failed for sector %llx\n", (u64) s); > > > > I don't think you should remove this one. dev_info_once() is just a > > printk(), > > and doesn't inherit the +f flag from the dynamic debugging code. The > > __func__ > > here does add value. > > > > The rest of these look correct, though I think you missed one in each of > > nvdimm_map_release() > > This one is now fixed. With the above fixed, Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] libnvdimm: remove redundant __func__ in dev_dbg
On Tue, Mar 06, 2018 at 08:59:11AM -0800, Dan Williams wrote: > On Mon, Mar 5, 2018 at 7:54 PM, Ross Zwisler > wrote: > > On Mon, Mar 05, 2018 at 05:09:21PM -0800, Dan Williams wrote: > >> Dynamic debug can be instructed to add the function name to the debug > >> output using the +f switch, so there is no need for the libnvdimm > >> modules to do it again. If a user decides to add the +f switch for > >> libnvdimm's dynamic debug this results in double prints of the function > >> name. > >> > >> Reported-by: Johannes Thumshirn > >> Reported-by: Ross Zwisler > >> Signed-off-by: Dan Williams > >> --- > >> drivers/nvdimm/badrange.c |3 +- > >> drivers/nvdimm/btt_devs.c | 21 > >> drivers/nvdimm/bus.c| 13 +- > >> drivers/nvdimm/claim.c |2 +- > >> drivers/nvdimm/core.c |4 ++- > >> drivers/nvdimm/dax_devs.c |5 ++-- > >> drivers/nvdimm/dimm_devs.c |7 ++--- > >> drivers/nvdimm/label.c | 51 > >> ++- > >> drivers/nvdimm/namespace_devs.c | 38 - > >> drivers/nvdimm/pfn_devs.c | 25 +-- > >> drivers/nvdimm/pmem.c |2 +- > >> 11 files changed, 77 insertions(+), 94 deletions(-) > >> > >> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c > >> index e068d72b4357..df17f1cd696d 100644 > >> --- a/drivers/nvdimm/badrange.c > >> +++ b/drivers/nvdimm/badrange.c > >> @@ -176,8 +176,7 @@ static void set_badblock(struct badblocks *bb, > >> sector_t s, int num) > >> (u64) s * 512, (u64) num * 512); > >> /* this isn't an error as the hardware will still throw an exception > >> */ > >> if (badblocks_set(bb, s, num, 1)) > >> - dev_info_once(bb->dev, "%s: failed for sector %llx\n", > >> - __func__, (u64) s); > >> + dev_info_once(bb->dev, "failed for sector %llx\n", (u64) s); > > > > I don't think you should remove this one. dev_info_once() is just a > > printk(), > > and doesn't inherit the +f flag from the dynamic debugging code. The > > __func__ > > here does add value. > > > > The rest of these look correct, though I think you missed one in each of > > nvdimm_map_release() > > This one is now fixed. > > > and validate_dimm(). (I made these changes as well, but > > you sent out your patch first. :) > > The validate_dimm() one is printing the callee function one level up > in the call chain, so it's fine as is. Sounds good. you can add: Reviewed-by: Ross Zwisler
Re: [PATCH] libnvdimm: remove redundant __func__ in dev_dbg
On Mon, Mar 5, 2018 at 7:54 PM, Ross Zwisler wrote: > On Mon, Mar 05, 2018 at 05:09:21PM -0800, Dan Williams wrote: >> Dynamic debug can be instructed to add the function name to the debug >> output using the +f switch, so there is no need for the libnvdimm >> modules to do it again. If a user decides to add the +f switch for >> libnvdimm's dynamic debug this results in double prints of the function >> name. >> >> Reported-by: Johannes Thumshirn >> Reported-by: Ross Zwisler >> Signed-off-by: Dan Williams >> --- >> drivers/nvdimm/badrange.c |3 +- >> drivers/nvdimm/btt_devs.c | 21 >> drivers/nvdimm/bus.c| 13 +- >> drivers/nvdimm/claim.c |2 +- >> drivers/nvdimm/core.c |4 ++- >> drivers/nvdimm/dax_devs.c |5 ++-- >> drivers/nvdimm/dimm_devs.c |7 ++--- >> drivers/nvdimm/label.c | 51 >> ++- >> drivers/nvdimm/namespace_devs.c | 38 - >> drivers/nvdimm/pfn_devs.c | 25 +-- >> drivers/nvdimm/pmem.c |2 +- >> 11 files changed, 77 insertions(+), 94 deletions(-) >> >> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c >> index e068d72b4357..df17f1cd696d 100644 >> --- a/drivers/nvdimm/badrange.c >> +++ b/drivers/nvdimm/badrange.c >> @@ -176,8 +176,7 @@ static void set_badblock(struct badblocks *bb, sector_t >> s, int num) >> (u64) s * 512, (u64) num * 512); >> /* this isn't an error as the hardware will still throw an exception */ >> if (badblocks_set(bb, s, num, 1)) >> - dev_info_once(bb->dev, "%s: failed for sector %llx\n", >> - __func__, (u64) s); >> + dev_info_once(bb->dev, "failed for sector %llx\n", (u64) s); > > I don't think you should remove this one. dev_info_once() is just a printk(), > and doesn't inherit the +f flag from the dynamic debugging code. The __func__ > here does add value. > > The rest of these look correct, though I think you missed one in each of > nvdimm_map_release() This one is now fixed. > and validate_dimm(). (I made these changes as well, but > you sent out your patch first. :) The validate_dimm() one is printing the callee function one level up in the call chain, so it's fine as is.
Re: [PATCH] libnvdimm: remove redundant __func__ in dev_dbg
On Mon, Mar 05, 2018 at 05:09:21PM -0800, Dan Williams wrote: > Dynamic debug can be instructed to add the function name to the debug > output using the +f switch, so there is no need for the libnvdimm > modules to do it again. If a user decides to add the +f switch for > libnvdimm's dynamic debug this results in double prints of the function > name. > > Reported-by: Johannes Thumshirn > Reported-by: Ross Zwisler > Signed-off-by: Dan Williams > --- > drivers/nvdimm/badrange.c |3 +- > drivers/nvdimm/btt_devs.c | 21 > drivers/nvdimm/bus.c| 13 +- > drivers/nvdimm/claim.c |2 +- > drivers/nvdimm/core.c |4 ++- > drivers/nvdimm/dax_devs.c |5 ++-- > drivers/nvdimm/dimm_devs.c |7 ++--- > drivers/nvdimm/label.c | 51 > ++- > drivers/nvdimm/namespace_devs.c | 38 - > drivers/nvdimm/pfn_devs.c | 25 +-- > drivers/nvdimm/pmem.c |2 +- > 11 files changed, 77 insertions(+), 94 deletions(-) > > diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c > index e068d72b4357..df17f1cd696d 100644 > --- a/drivers/nvdimm/badrange.c > +++ b/drivers/nvdimm/badrange.c > @@ -176,8 +176,7 @@ static void set_badblock(struct badblocks *bb, sector_t > s, int num) > (u64) s * 512, (u64) num * 512); > /* this isn't an error as the hardware will still throw an exception */ > if (badblocks_set(bb, s, num, 1)) > - dev_info_once(bb->dev, "%s: failed for sector %llx\n", > - __func__, (u64) s); > + dev_info_once(bb->dev, "failed for sector %llx\n", (u64) s); I don't think you should remove this one. dev_info_once() is just a printk(), and doesn't inherit the +f flag from the dynamic debugging code. The __func__ here does add value. The rest of these look correct, though I think you missed one in each of nvdimm_map_release() and validate_dimm(). (I made these changes as well, but you sent out your patch first. :)
