| Message ID | 20190122220525.5211-1-elliott@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libnvdimm: normalize dev_debug failed and succeeded wording | expand |
Looks ok, just some formatting nits... On Tue, Jan 22, 2019 at 2:14 PM Robert Elliott <elliott@hpe.com> wrote: > > Make dev_dbg prints use consistent wording for failed and succeeded, and > include the return code when reporting something failed. > > Signed-off-by: Robert Elliott <elliott@hpe.com> > --- > drivers/nvdimm/dimm.c | 2 +- > drivers/nvdimm/label.c | 7 ++++--- > drivers/nvdimm/security.c | 53 ++++++++++++++++++++++++++++++++++------------- > 3 files changed, 44 insertions(+), 18 deletions(-) This insertions > 2x deletions is concerning, some suggestions below. > > diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c > index 0cf58cabc9ed..3ff104f7ccaa 100644 > --- a/drivers/nvdimm/dimm.c > +++ b/drivers/nvdimm/dimm.c > @@ -62,7 +62,7 @@ static int nvdimm_probe(struct device *dev) > */ > rc = nvdimm_security_unlock(dev); > if (rc < 0) > - dev_dbg(dev, "failed to unlock dimm: %d\n", rc); > + dev_dbg(dev, "unlock failed: %d\n", rc); > > > /* > diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c > index a11bf4e6b451..235833a4881b 100644 > --- a/drivers/nvdimm/label.c > +++ b/drivers/nvdimm/label.c > @@ -149,7 +149,7 @@ static int __nd_label_validate(struct nvdimm_drvdata *ndd) > labelsize = 128; > > if (labelsize != sizeof_namespace_label(ndd)) { > - dev_dbg(dev, "nsindex%d labelsize %d invalid\n", > + dev_dbg(dev, "nsindex%d labelsize: %d invalid\n", > i, nsindex[i]->labelsize); > continue; > } > @@ -373,7 +373,8 @@ static bool slot_valid(struct nvdimm_drvdata *ndd, > sum = nd_fletcher64(nd_label, sizeof_namespace_label(ndd), 1); > nd_label->checksum = __cpu_to_le64(sum_save); > if (sum != sum_save) { > - dev_dbg(ndd->dev, "fail checksum. slot: %d expect: %#llx\n", > + dev_dbg(ndd->dev, > + "checksum failed. slot: %d expected: %#llx\n", It's ok to keep the format string on the same line as dev_dbg(), even if it violates 80 columns. In general it helps grep'ability if the format string is on the same line as print call. > slot, sum); > return false; > } > @@ -535,7 +536,7 @@ int nd_label_data_init(struct nvdimm_drvdata *ndd) > read_size += label_read_size; > } > > - dev_dbg(ndd->dev, "len: %zu rc: %d\n", offset, rc); > + dev_dbg(ndd->dev, "init config data area len: %zu succeeded\n", offset); > out_err: > return rc; > } > diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c > index f8bb746a549f..b7f86846015a 100644 > --- a/drivers/nvdimm/security.c > +++ b/drivers/nvdimm/security.c > @@ -58,7 +58,8 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm) > if (PTR_ERR(key) == -ENOKEY) > dev_dbg(dev, "request_key() found no key\n"); > else > - dev_dbg(dev, "request_key() upcall failed\n"); > + dev_dbg(dev, "request_key() upcall failed: %ld\n", > + PTR_ERR(key)); > key = NULL; > } else { > struct encrypted_key_payload *epayload; > @@ -107,6 +108,7 @@ static struct key *nvdimm_lookup_user_key(struct nvdimm *nvdimm, > > static struct key *nvdimm_key_revalidate(struct nvdimm *nvdimm) > { > + struct device *dev = &nvdimm->dev; > struct key *key; > int rc; > > @@ -126,7 +128,11 @@ static struct key *nvdimm_key_revalidate(struct nvdimm *nvdimm) > if (rc < 0) { > nvdimm_put_key(key); > key = NULL; > - } > + dev_dbg(dev, "key: %d revalidation failed: %d\n", > + key_serial(key), rc); > + } else > + dev_dbg(dev, "key: %d revalidation succeeded\n", > + key_serial(key)); For all of these success vs fail cases I would prefer to continue to use a ternary: dev_dbg(dev, "key: %d revalidation %s: %d\n", key_serial(key), rc == 0 ? "succeeded" : "failed", rc); ...as to not clutter the code with extra debug statements.
> -----Original Message----- > From: Dan Williams [mailto:dan.j.williams@intel.com] > Sent: Tuesday, January 22, 2019 4:24 PM > To: Elliott, Robert (Persistent Memory) <elliott@hpe.com> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org> > Subject: Re: [PATCH] libnvdimm: normalize dev_debug failed and succeeded wording > > Looks ok, just some formatting nits... > > On Tue, Jan 22, 2019 at 2:14 PM Robert Elliott <elliott@hpe.com> wrote: > > > > if (rc < 0) { > > nvdimm_put_key(key); > > key = NULL; > > - } > > + dev_dbg(dev, "key: %d revalidation failed: %d\n", > > + key_serial(key), rc); > > + } else > > + dev_dbg(dev, "key: %d revalidation succeeded\n", > > + key_serial(key)); > > For all of these success vs fail cases I would prefer to continue to > use a ternary: > > dev_dbg(dev, "key: %d revalidation %s: %d\n", > key_serial(key), rc == 0 ? "succeeded" : "failed", rc); > > ...as to not clutter the code with extra debug statements. That'll result in printing the return code for successful cases: ...succeeded: 0 I guess that's fine for the dev_dbg level.
On Tue, Jan 22, 2019 at 3:13 PM Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote: > > > > > -----Original Message----- > > From: Dan Williams [mailto:dan.j.williams@intel.com] > > Sent: Tuesday, January 22, 2019 4:24 PM > > To: Elliott, Robert (Persistent Memory) <elliott@hpe.com> > > Cc: linux-nvdimm <linux-nvdimm@lists.01.org> > > Subject: Re: [PATCH] libnvdimm: normalize dev_debug failed and succeeded wording > > > > Looks ok, just some formatting nits... > > > > On Tue, Jan 22, 2019 at 2:14 PM Robert Elliott <elliott@hpe.com> wrote: > > > > > > if (rc < 0) { > > > nvdimm_put_key(key); > > > key = NULL; > > > - } > > > + dev_dbg(dev, "key: %d revalidation failed: %d\n", > > > + key_serial(key), rc); > > > + } else > > > + dev_dbg(dev, "key: %d revalidation succeeded\n", > > > + key_serial(key)); > > > > For all of these success vs fail cases I would prefer to continue to > > use a ternary: > > > > dev_dbg(dev, "key: %d revalidation %s: %d\n", > > key_serial(key), rc == 0 ? "succeeded" : "failed", rc); > > > > ...as to not clutter the code with extra debug statements. > > That'll result in printing the return code for successful cases: > ...succeeded: 0 > > I guess that's fine for the dev_dbg level. Yeah, I'm ok with that price for slightly more compact code.
diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c index 0cf58cabc9ed..3ff104f7ccaa 100644 --- a/drivers/nvdimm/dimm.c +++ b/drivers/nvdimm/dimm.c @@ -62,7 +62,7 @@ static int nvdimm_probe(struct device *dev) */ rc = nvdimm_security_unlock(dev); if (rc < 0) - dev_dbg(dev, "failed to unlock dimm: %d\n", rc); + dev_dbg(dev, "unlock failed: %d\n", rc); /* diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c index a11bf4e6b451..235833a4881b 100644 --- a/drivers/nvdimm/label.c +++ b/drivers/nvdimm/label.c @@ -149,7 +149,7 @@ static int __nd_label_validate(struct nvdimm_drvdata *ndd) labelsize = 128; if (labelsize != sizeof_namespace_label(ndd)) { - dev_dbg(dev, "nsindex%d labelsize %d invalid\n", + dev_dbg(dev, "nsindex%d labelsize: %d invalid\n", i, nsindex[i]->labelsize); continue; } @@ -373,7 +373,8 @@ static bool slot_valid(struct nvdimm_drvdata *ndd, sum = nd_fletcher64(nd_label, sizeof_namespace_label(ndd), 1); nd_label->checksum = __cpu_to_le64(sum_save); if (sum != sum_save) { - dev_dbg(ndd->dev, "fail checksum. slot: %d expect: %#llx\n", + dev_dbg(ndd->dev, + "checksum failed. slot: %d expected: %#llx\n", slot, sum); return false; } @@ -535,7 +536,7 @@ int nd_label_data_init(struct nvdimm_drvdata *ndd) read_size += label_read_size; } - dev_dbg(ndd->dev, "len: %zu rc: %d\n", offset, rc); + dev_dbg(ndd->dev, "init config data area len: %zu succeeded\n", offset); out_err: return rc; } diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index f8bb746a549f..b7f86846015a 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -58,7 +58,8 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm) if (PTR_ERR(key) == -ENOKEY) dev_dbg(dev, "request_key() found no key\n"); else - dev_dbg(dev, "request_key() upcall failed\n"); + dev_dbg(dev, "request_key() upcall failed: %ld\n", + PTR_ERR(key)); key = NULL; } else { struct encrypted_key_payload *epayload; @@ -107,6 +108,7 @@ static struct key *nvdimm_lookup_user_key(struct nvdimm *nvdimm, static struct key *nvdimm_key_revalidate(struct nvdimm *nvdimm) { + struct device *dev = &nvdimm->dev; struct key *key; int rc; @@ -126,7 +128,11 @@ static struct key *nvdimm_key_revalidate(struct nvdimm *nvdimm) if (rc < 0) { nvdimm_put_key(key); key = NULL; - } + dev_dbg(dev, "key: %d revalidation failed: %d\n", + key_serial(key), rc); + } else + dev_dbg(dev, "key: %d revalidation succeeded\n", + key_serial(key)); return key; } @@ -170,8 +176,11 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm) return -ENOKEY; rc = nvdimm->sec.ops->unlock(nvdimm, key_data(key)); - dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key), - rc == 0 ? "success" : "fail"); + if (rc) + dev_dbg(dev, "key: %d unlock failed: %d\n", + key_serial(key), rc); + else + dev_dbg(dev, "key: %d unlock succeeded\n", key_serial(key)); nvdimm_put_key(key); nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER); @@ -219,8 +228,11 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid) return -ENOKEY; rc = nvdimm->sec.ops->disable(nvdimm, key_data(key)); - dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), - rc == 0 ? "success" : "fail"); + if (rc) + dev_dbg(dev, "key: %d disable failed: %d\n", + key_serial(key), rc); + else + dev_dbg(dev, "key: %d disable succeeded\n", key_serial(key)); nvdimm_put_key(key); nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER); @@ -265,10 +277,15 @@ int nvdimm_security_update(struct nvdimm *nvdimm, unsigned int keyid, rc = nvdimm->sec.ops->change_key(nvdimm, key ? key_data(key) : NULL, key_data(newkey), pass_type); - dev_dbg(dev, "key: %d %d update%s: %s\n", + if (rc) + dev_dbg(dev, "key: %d %d update%s failed: %d\n", key_serial(key), key_serial(newkey), pass_type == NVDIMM_MASTER ? "(master)" : "(user)", - rc == 0 ? "success" : "fail"); + rc); + else + dev_dbg(dev, "key: %d %d update%s succeeded\n", + key_serial(key), key_serial(newkey), + pass_type == NVDIMM_MASTER ? "(master)" : "(user)"); nvdimm_put_key(newkey); nvdimm_put_key(key); @@ -324,9 +341,13 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, return -ENOKEY; rc = nvdimm->sec.ops->erase(nvdimm, key_data(key), pass_type); - dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key), + if (rc) + dev_dbg(dev, "key: %d erase%s failed: %d\n", key_serial(key), pass_type == NVDIMM_MASTER ? "(master)" : "(user)", - rc == 0 ? "success" : "fail"); + rc); + else + dev_dbg(dev, "key: %d erase%s succeeded\n", key_serial(key), + pass_type == NVDIMM_MASTER ? "(master)" : "(user)"); nvdimm_put_key(key); nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER); @@ -377,8 +398,12 @@ int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid) } rc = nvdimm->sec.ops->overwrite(nvdimm, key ? key_data(key) : NULL); - dev_dbg(dev, "key: %d overwrite submission: %s\n", key_serial(key), - rc == 0 ? "success" : "fail"); + if (rc) + dev_dbg(dev, "key: %d overwrite submission failed: %d\n", + key_serial(key), rc); + else + dev_dbg(dev, "key: %d overwrite submission succeeded\n", + key_serial(key)); nvdimm_put_key(key); if (rc == 0) { @@ -429,9 +454,9 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm) } if (rc < 0) - dev_dbg(&nvdimm->dev, "overwrite failed\n"); + dev_dbg(&nvdimm->dev, "overwrite completion failed: %d\n", rc); else - dev_dbg(&nvdimm->dev, "overwrite completed\n"); + dev_dbg(&nvdimm->dev, "overwrite completion succeeded\n"); if (nvdimm->sec.overwrite_state) sysfs_notify_dirent(nvdimm->sec.overwrite_state);
Make dev_dbg prints use consistent wording for failed and succeeded, and include the return code when reporting something failed. Signed-off-by: Robert Elliott <elliott@hpe.com> --- drivers/nvdimm/dimm.c | 2 +- drivers/nvdimm/label.c | 7 ++++--- drivers/nvdimm/security.c | 53 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 44 insertions(+), 18 deletions(-)