[PATCH 0/3] acpi: Eliminate misleading erst pstore console message

[PATCH 0/3] acpi: Eliminate misleading erst pstore console message

[Lenny Szubowicz]

On systems that have a valid ACPI ERST table, if the pstore.backend kernel
parameter selects a specific facility other than erst, then during boot the
following console message is displayed:

    ERST: Could not register with persistent store

This message makes it seem that something has gone wrong. In fact the same
message is used when you want ERST to be used by pstore and some real error
precludes registration.

The primary purpose of this 3 patch set is to avoid displaying this console
error message when another facility is explicitly selected as the backend for
pstore. Details are in the individual patches.


Lenny Szubowicz (3):
  pstore: Return unique error if backend registration excluded by kernel
    param
  acpi: Eliminate console msg if pstore.backend excludes ERST
  efivars: If pstore_register fails, free unneeded pstore buffer

 drivers/acpi/apei/erst.c          | 20 ++++++++++++++------
 drivers/firmware/efi/efi-pstore.c |  6 +++++-
 fs/pstore/platform.c              | 11 ++++++-----
 3 files changed, 25 insertions(+), 12 deletions(-)

-- 
1.8.2.1

[PATCH 1/3] pstore: Return unique error if backend registration excluded by kernel param

[Lenny Szubowicz]

This is patch 1/3 of a patch set that avoids what misleadingly appears
to be a error during boot:

ERST: Could not register with persistent store

This message is displayed if the system has a valid ACPI ERST table and the
pstore.backend kernel parameter has been used to disable use of ERST by
pstore. But this same message is used for errors that preclude registration.

As part of fixing this, return a unique error status from pstore_register
if the pstore.backend kernel parameter selects a specific facility other
than the requesting facility and check for this condition before any others.
This allows the caller to distinquish this benign case from the other failure
cases.

Also, print an informational console message about which facility
successfully registered as the pstore backend. Since there are various
kernel parameters, config build options, and boot-time errors that can
influence which facility registers with pstore, it's useful to have a
positive indication.

Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
Reported-by: Naotaka Hamaguchi <n.hamaguchi@jp.fujitsu.com>
---
 fs/pstore/platform.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 86d1038..84f3ca7 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -239,17 +239,15 @@ int pstore_register(struct pstore_info *psi)
 {
 	struct module *owner = psi->owner;
 
+	if (backend && strcmp(backend, psi->name))
+		return -EINVAL;
+
 	spin_lock(&pstore_lock);
 	if (psinfo) {
 		spin_unlock(&pstore_lock);
 		return -EBUSY;
 	}
 
-	if (backend && strcmp(backend, psi->name)) {
-		spin_unlock(&pstore_lock);
-		return -EINVAL;
-	}
-
 	if (!psi->write)
 		psi->write = pstore_write_compat;
 	psinfo = psi;
@@ -274,6 +272,9 @@ int pstore_register(struct pstore_info *psi)
 		add_timer(&pstore_timer);
 	}
 
+	pr_info("pstore: Registered %s as persistent store backend\n",
+		psi->name);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pstore_register);
-- 
1.8.2.1

[PATCH 2/3] acpi: Eliminate console msg if pstore.backend excludes ERST

[Lenny Szubowicz]

This is patch 2/3 of a patch set that avoids what misleadingly appears
to be a error during boot:

ERST: Could not register with persistent store

This message is displayed if the system has a valid ACPI ERST table and the
pstore.backend kernel parameter has been used to disable use of ERST by
pstore. But this same message is used for errors that preclude registration.

In erst_init don't complain if the setting of kernel parameter pstore.backend
precludes use of ACPI ERST for pstore. Routine pstore_register will inform
about the facility that does register.

Also, don't leave a dangling pointer to deallocated mem for the pstore
buffer when registration fails.

Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
Reported-by: Naotaka Hamaguchi <n.hamaguchi@jp.fujitsu.com>
---
 drivers/acpi/apei/erst.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 6d894bf..f7b3b39 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -1180,20 +1180,28 @@ static int __init erst_init(void)
 	if (!erst_erange.vaddr)
 		goto err_release_erange;
 
+	pr_info(ERST_PFX
+	"Error Record Serialization Table (ERST) support is initialized.\n");
+
 	buf = kmalloc(erst_erange.size, GFP_KERNEL);
 	spin_lock_init(&erst_info.buf_lock);
 	if (buf) {
 		erst_info.buf = buf + sizeof(struct cper_pstore_record);
 		erst_info.bufsize = erst_erange.size -
 				    sizeof(struct cper_pstore_record);
-		if (pstore_register(&erst_info)) {
-			pr_info(ERST_PFX "Could not register with persistent store\n");
+		rc = pstore_register(&erst_info);
+		if (rc) {
+			if (rc != -EPERM)
+				pr_info(ERST_PFX
+				"Could not register with persistent store\n");
+			erst_info.buf = NULL;
+			erst_info.bufsize = 0;
 			kfree(buf);
 		}
-	}
-
-	pr_info(ERST_PFX
-	"Error Record Serialization Table (ERST) support is initialized.\n");
+	} else
+		pr_err(ERST_PFX
+		"Failed to allocate %lld bytes for persistent store error log\n",
+		erst_erange.size);
 
 	return 0;
 
-- 
1.8.2.1

[PATCH 3/3] efivars: If pstore_register fails, free unneeded pstore buffer

[Lenny Szubowicz]

This is patch 3/3 of a patch set that cleans up pstore_register failure paths.

If efivars fails to register with pstore, there is no point to keeping
the 4 KB buffer around. It's only used by the pstore read/write routines.

Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
Reported-by: Naotaka Hamaguchi <n.hamaguchi@jp.fujitsu.com>
---
 drivers/firmware/efi/efi-pstore.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 202d2c8..66c0c76 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -236,7 +236,11 @@ static __init int efivars_pstore_init(void)
 	efi_pstore_info.bufsize = 1024;
 	spin_lock_init(&efi_pstore_info.buf_lock);
 
-	pstore_register(&efi_pstore_info);
+	if (pstore_register(&efi_pstore_info)) {
+		kfree(efi_pstore_info.buf);
+		efi_pstore_info.buf = NULL;
+		efi_pstore_info.bufsize = 0;
+	}
 
 	return 0;
 }
-- 
1.8.2.1

Re: [PATCH 2/3] acpi: Eliminate console msg if pstore.backend excludes ERST

[Tony Luck]

On Fri, Jun 28, 2013 at 1:14 PM, Lenny Szubowicz <lszubowi@redhat.com> wrote:

> -               if (pstore_register(&erst_info)) {
> -                       pr_info(ERST_PFX "Could not register with persistent store\n");
> +               rc = pstore_register(&erst_info);
> +               if (rc) {
> +                       if (rc != -EPERM)
> +                               pr_info(ERST_PFX
> +                               "Could not register with persistent store\n");
> +                       erst_info.buf = NULL;
> +                       erst_info.bufsize = 0;

Mismatch between part 1 and part 2 here ... we return -EINVAL if
our name doesn't match the desired backend ... but you only suppress
the "Could not register" message for -EPERM

Or am I confused while just looking at patch fragments?

-Tony

Re: [PATCH 2/3] acpi: Eliminate console msg if pstore.backend excludes ERST

[Lenny Szubowicz]

----- Original Message -----
> From: "Tony Luck" <tony.luck@gmail.com>
> To: "Lenny Szubowicz" <lszubowi@redhat.com>
> Cc: "Anton Vorontsov" <cbouatmailru@gmail.com>, "Matt Fleming" <matt.fleming@intel.com>, "Linux Kernel Mailing List"
> <linux-kernel@vger.kernel.org>, "n hamaguchi" <n.hamaguchi@jp.fujitsu.com>, "Don Zickus" <dzickus@redhat.com>
> Sent: Friday, June 28, 2013 4:44:51 PM
> Subject: Re: [PATCH 2/3] acpi: Eliminate console msg if pstore.backend excludes ERST
> 
> On Fri, Jun 28, 2013 at 1:14 PM, Lenny Szubowicz <lszubowi@redhat.com> wrote:
> 
> > -               if (pstore_register(&erst_info)) {
> > -                       pr_info(ERST_PFX "Could not register with
> > persistent store\n");
> > +               rc = pstore_register(&erst_info);
> > +               if (rc) {
> > +                       if (rc != -EPERM)
> > +                               pr_info(ERST_PFX
> > +                               "Could not register with persistent
> > store\n");
> > +                       erst_info.buf = NULL;
> > +                       erst_info.bufsize = 0;
> 
> Mismatch between part 1 and part 2 here ... we return -EINVAL if
> our name doesn't match the desired backend ... but you only suppress
> the "Could not register" message for -EPERM
> 
> Or am I confused while just looking at patch fragments?
> 
> -Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Yes, you are absolutely correct. My [PATCH 1/3] is not what I intended. Thanks!

                         -Lenny.

Re: [PATCH 0/3] acpi: Eliminate misleading erst pstore console message

[Tony Luck]

On Fri, Jun 28, 2013 at 1:14 PM, Lenny Szubowicz <lszubowi@redhat.com> wrote:
> On systems that have a valid ACPI ERST table, if the pstore.backend kernel
> parameter selects a specific facility other than erst, then during boot the
> following console message is displayed:
>
>     ERST: Could not register with persistent store

Applied (using revised version of part 1).

Thanks

-Tony