[PATCH] efi: Avoid sysfs spew on reboot and panic

[PATCH] efi: Avoid sysfs spew on reboot and panic

[Matthew Garrett]

Right now all pstore accesses to efivars will delete or create new sysfs
nodes. This is less than ideal if we've paniced or are rebooting, since
we're in entirely the wrong context to do this kind of thing. Add the
reason to the pstore callback and make sure we only manipulate the files
if we're still going to be alive afterwards.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Reported-by: Seiji Aguchi <saguchi@redhat.com>
---
 drivers/acpi/apei/erst.c   |    6 ++++--
 drivers/firmware/efivars.c |   17 +++++++++++++----
 fs/pstore/platform.c       |    7 ++++---
 include/linux/kmsg_dump.h  |    1 +
 include/linux/pstore.h     |    5 ++++-
 5 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 2ca59dc..a6b5dc1 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -934,7 +934,8 @@ static int erst_close_pstore(struct pstore_info *psi);
 static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
 			   struct timespec *time, struct pstore_info *psi);
 static u64 erst_writer(enum pstore_type_id type, unsigned int part,
-		       size_t size, struct pstore_info *psi);
+		       enum kmsg_dump_reason reason, size_t size,
+		       struct pstore_info *psi);
 static int erst_clearer(enum pstore_type_id type, u64 id,
 			struct pstore_info *psi);
 
@@ -1041,7 +1042,8 @@ out:
 }
 
 static u64 erst_writer(enum pstore_type_id type, unsigned int part,
-		       size_t size, struct pstore_info *psi)
+		       enum kmsg_dump_reason reason, size_t size,
+		       struct pstore_info *psi)
 {
 	struct cper_pstore_record *rcd = (struct cper_pstore_record *)
 					(erst_info.buf - sizeof(*rcd));
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index eb80b54..0510319 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -491,7 +491,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 }
 
 static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
-			    size_t size, struct pstore_info *psi)
+			    enum kmsg_dump_reason reason, size_t size,
+			    struct pstore_info *psi)
 {
 	char name[DUMP_NAME_LEN];
 	char stub_name[DUMP_NAME_LEN];
@@ -544,6 +545,14 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
 
 	spin_unlock(&efivars->lock);
 
+	/*
+	 * If it's more severe than KMSG_DUMP_OOPS then we're already dead.
+	 * Don't bother playing with sysfs.
+	 */
+
+	if (reason > KMSG_DUMP_OOPS)
+		return part;
+
 	if (found)
 		efivar_unregister(found);
 
@@ -552,14 +561,13 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
 					  utf16_strsize(efi_name,
 							DUMP_NAME_LEN * 2),
 					  efi_name, &vendor);
-
 	return part;
 };
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
 			    struct pstore_info *psi)
 {
-	efi_pstore_write(type, id, 0, psi);
+	efi_pstore_write(type, id, KMSG_DUMP_UNKNOWN, 0, psi);
 
 	return 0;
 }
@@ -581,7 +589,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 }
 
 static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
-			    size_t size, struct pstore_info *psi)
+			    enum kmsg_dump_reason reason, size_t size,
+			    struct pstore_info *psi)
 {
 	return 0;
 }
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index c5300ec..02117cc 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -51,7 +51,8 @@ void pstore_set_kmsg_bytes(int bytes)
 static int	oopscount;
 
 static char *reason_str[] = {
-	"Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff", "Emergency"
+	"Unknown", "Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff",
+	"Emergency"
 };
 
 /*
@@ -97,7 +98,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		memcpy(dst, s1 + s1_start, l1_cpy);
 		memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);
 
-		id = psinfo->write(PSTORE_TYPE_DMESG, part,
+		id = psinfo->write(PSTORE_TYPE_DMESG, part, reason,
 				   hsize + l1_cpy + l2_cpy, psinfo);
 		if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
 			pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
@@ -207,7 +208,7 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)
 
 	mutex_lock(&psinfo->buf_mutex);
 	memcpy(psinfo->buf, buf, size);
-	id = psinfo->write(type, 0, size, psinfo);
+	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
 	if (pstore_is_mounted())
 		pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
 			      size, CURRENT_TIME, psinfo);
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index ee0c952..b5ffe79 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 
 enum kmsg_dump_reason {
+	KMSG_DUMP_UNKNOWN,
 	KMSG_DUMP_OOPS,
 	KMSG_DUMP_PANIC,
 	KMSG_DUMP_KEXEC,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index cc03bbf..fa049e8 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -22,6 +22,8 @@
 #ifndef _LINUX_PSTORE_H
 #define _LINUX_PSTORE_H
 
+#include <linux/kmsg_dump.h>
+
 /* types */
 enum pstore_type_id {
 	PSTORE_TYPE_DMESG	= 0,
@@ -40,7 +42,8 @@ struct pstore_info {
 	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
 			struct timespec *time, struct pstore_info *psi);
 	u64		(*write)(enum pstore_type_id type, unsigned int part,
-			size_t size, struct pstore_info *psi);
+			enum kmsg_dump_reason reason, size_t size,
+			struct pstore_info *psi);
 	int		(*erase)(enum pstore_type_id type, u64 id,
 			struct pstore_info *psi);
 	void		*data;
-- 
1.7.6.2

Re: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Chen Gong]

于 2011/9/21 5:07, Matthew Garrett 写道:
> Right now all pstore accesses to efivars will delete or create new sysfs
> nodes. This is less than ideal if we've paniced or are rebooting, since
> we're in entirely the wrong context to do this kind of thing. Add the
> reason to the pstore callback and make sure we only manipulate the files
> if we're still going to be alive afterwards.
>
> Signed-off-by: Matthew Garrett<mjg@redhat.com>
> Reported-by: Seiji Aguchi<saguchi@redhat.com>
> ---
>   drivers/acpi/apei/erst.c   |    6 ++++--
>   drivers/firmware/efivars.c |   17 +++++++++++++----
>   fs/pstore/platform.c       |    7 ++++---
>   include/linux/kmsg_dump.h  |    1 +
>   include/linux/pstore.h     |    5 ++++-
>   5 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 2ca59dc..a6b5dc1 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -934,7 +934,8 @@ static int erst_close_pstore(struct pstore_info *psi);
>   static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
>   			   struct timespec *time, struct pstore_info *psi);
>   static u64 erst_writer(enum pstore_type_id type, unsigned int part,
> -		       size_t size, struct pstore_info *psi);
> +		       enum kmsg_dump_reason reason, size_t size,
> +		       struct pstore_info *psi);
>   static int erst_clearer(enum pstore_type_id type, u64 id,
>   			struct pstore_info *psi);
>
> @@ -1041,7 +1042,8 @@ out:
>   }
>
>   static u64 erst_writer(enum pstore_type_id type, unsigned int part,
> -		       size_t size, struct pstore_info *psi)
> +		       enum kmsg_dump_reason reason, size_t size,
> +		       struct pstore_info *psi)
>   {
>   	struct cper_pstore_record *rcd = (struct cper_pstore_record *)
>   					(erst_info.buf - sizeof(*rcd));
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index eb80b54..0510319 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -491,7 +491,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>   }
>
>   static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
> -			    size_t size, struct pstore_info *psi)
> +			    enum kmsg_dump_reason reason, size_t size,
> +			    struct pstore_info *psi)
>   {
>   	char name[DUMP_NAME_LEN];
>   	char stub_name[DUMP_NAME_LEN];
> @@ -544,6 +545,14 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
>
>   	spin_unlock(&efivars->lock);
>
> +	/*
> +	 * If it's more severe than KMSG_DUMP_OOPS then we're already dead.
> +	 * Don't bother playing with sysfs.
> +	 */
> +
> +	if (reason>  KMSG_DUMP_OOPS)
> +		return part;
> +
>   	if (found)
>   		efivar_unregister(found);
>
> @@ -552,14 +561,13 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
>   					  utf16_strsize(efi_name,
>   							DUMP_NAME_LEN * 2),
>   					  efi_name,&vendor);
> -
>   	return part;
>   };
>
>   static int efi_pstore_erase(enum pstore_type_id type, u64 id,
>   			    struct pstore_info *psi)
>   {
> -	efi_pstore_write(type, id, 0, psi);
> +	efi_pstore_write(type, id, KMSG_DUMP_UNKNOWN, 0, psi);
>
>   	return 0;
>   }
> @@ -581,7 +589,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>   }
>
>   static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
> -			    size_t size, struct pstore_info *psi)
> +			    enum kmsg_dump_reason reason, size_t size,
> +			    struct pstore_info *psi)
>   {
>   	return 0;
>   }
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index c5300ec..02117cc 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -51,7 +51,8 @@ void pstore_set_kmsg_bytes(int bytes)
>   static int	oopscount;
>
>   static char *reason_str[] = {
> -	"Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff", "Emergency"
> +	"Unknown", "Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff",
> +	"Emergency"
>   };
>
>   /*
> @@ -97,7 +98,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>   		memcpy(dst, s1 + s1_start, l1_cpy);
>   		memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);
>
> -		id = psinfo->write(PSTORE_TYPE_DMESG, part,
> +		id = psinfo->write(PSTORE_TYPE_DMESG, part, reason,
>   				   hsize + l1_cpy + l2_cpy, psinfo);
>   		if (reason == KMSG_DUMP_OOPS&&  pstore_is_mounted())
>   			pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
> @@ -207,7 +208,7 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)
>
>   	mutex_lock(&psinfo->buf_mutex);
>   	memcpy(psinfo->buf, buf, size);
> -	id = psinfo->write(type, 0, size, psinfo);
> +	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);

I can't say it is wrong because no real caller for this function, but I can't
say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
any reason to use this type here ?

>   	if (pstore_is_mounted())
>   		pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
>   			      size, CURRENT_TIME, psinfo);
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index ee0c952..b5ffe79 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -16,6 +16,7 @@
>   #include<linux/list.h>
>
>   enum kmsg_dump_reason {
> +	KMSG_DUMP_UNKNOWN,
>   	KMSG_DUMP_OOPS,
>   	KMSG_DUMP_PANIC,
>   	KMSG_DUMP_KEXEC,
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index cc03bbf..fa049e8 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -22,6 +22,8 @@
>   #ifndef _LINUX_PSTORE_H
>   #define _LINUX_PSTORE_H
>
> +#include<linux/kmsg_dump.h>
> +
>   /* types */
>   enum pstore_type_id {
>   	PSTORE_TYPE_DMESG	= 0,
> @@ -40,7 +42,8 @@ struct pstore_info {
>   	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
>   			struct timespec *time, struct pstore_info *psi);
>   	u64		(*write)(enum pstore_type_id type, unsigned int part,
> -			size_t size, struct pstore_info *psi);
> +			enum kmsg_dump_reason reason, size_t size,
> +			struct pstore_info *psi);
>   	int		(*erase)(enum pstore_type_id type, u64 id,
>   			struct pstore_info *psi);
>   	void		*data;

Re: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Matthew Garrett]

On Wed, Sep 21, 2011 at 11:31:40AM +0800, Chen Gong wrote:
> >  	mutex_lock(&psinfo->buf_mutex);
> >  	memcpy(psinfo->buf, buf, size);
> >-	id = psinfo->write(type, 0, size, psinfo);
> >+	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
> 
> I can't say it is wrong because no real caller for this function, but I can't
> say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
> any reason to use this type here ?

If a function calls pstore_write() directly then we have no type to 
associate with it. It seems worth making this explicit.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Seiji Aguchi]

Matthew

>        Reported-by: Seiji Aguchi <saguchi@redhat.com>

Could you please change it to seiji.aguchi@hds.com?

Seiji

________________________________________
Right now all pstore accesses to efivars will delete or create new sysfs
nodes. This is less than ideal if we've paniced or are rebooting, since
we're in entirely the wrong context to do this kind of thing. Add the
reason to the pstore callback and make sure we only manipulate the files
if we're still going to be alive afterwards.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Reported-by: Seiji Aguchi <saguchi@redhat.com>
---
 drivers/acpi/apei/erst.c   |    6 ++++--
 drivers/firmware/efivars.c |   17 +++++++++++++----
 fs/pstore/platform.c       |    7 ++++---
 include/linux/kmsg_dump.h  |    1 +
 include/linux/pstore.h     |    5 ++++-
 5 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 2ca59dc..a6b5dc1 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -934,7 +934,8 @@ static int erst_close_pstore(struct pstore_info *psi);
 static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
                           struct timespec *time, struct pstore_info *psi);
 static u64 erst_writer(enum pstore_type_id type, unsigned int part,
-                      size_t size, struct pstore_info *psi);
+                      enum kmsg_dump_reason reason, size_t size,
+                      struct pstore_info *psi);
 static int erst_clearer(enum pstore_type_id type, u64 id,
                        struct pstore_info *psi);

@@ -1041,7 +1042,8 @@ out:
 }

 static u64 erst_writer(enum pstore_type_id type, unsigned int part,
-                      size_t size, struct pstore_info *psi)
+                      enum kmsg_dump_reason reason, size_t size,
+                      struct pstore_info *psi)
 {
        struct cper_pstore_record *rcd = (struct cper_pstore_record *)
                                        (erst_info.buf - sizeof(*rcd));
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index eb80b54..0510319 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -491,7 +491,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 }

 static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
-                           size_t size, struct pstore_info *psi)
+                           enum kmsg_dump_reason reason, size_t size,
+                           struct pstore_info *psi)
 {
        char name[DUMP_NAME_LEN];
        char stub_name[DUMP_NAME_LEN];
@@ -544,6 +545,14 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,

        spin_unlock(&efivars->lock);

+       /*
+        * If it's more severe than KMSG_DUMP_OOPS then we're already dead.
+        * Don't bother playing with sysfs.
+        */
+
+       if (reason > KMSG_DUMP_OOPS)
+               return part;
+
        if (found)
                efivar_unregister(found);

@@ -552,14 +561,13 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
                                          utf16_strsize(efi_name,
                                                        DUMP_NAME_LEN * 2),
                                          efi_name, &vendor);
-
        return part;
 };

 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
                            struct pstore_info *psi)
 {
-       efi_pstore_write(type, id, 0, psi);
+       efi_pstore_write(type, id, KMSG_DUMP_UNKNOWN, 0, psi);

        return 0;
 }
@@ -581,7 +589,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 }

 static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
-                           size_t size, struct pstore_info *psi)
+                           enum kmsg_dump_reason reason, size_t size,
+                           struct pstore_info *psi)
 {
        return 0;
 }
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index c5300ec..02117cc 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -51,7 +51,8 @@ void pstore_set_kmsg_bytes(int bytes)
 static int     oopscount;

 static char *reason_str[] = {
-       "Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff", "Emergency"
+       "Unknown", "Oops", "Panic", "Kexec", "Restart", "Halt", "Poweroff",
+       "Emergency"
 };

 /*
@@ -97,7 +98,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
                memcpy(dst, s1 + s1_start, l1_cpy);
                memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);

-               id = psinfo->write(PSTORE_TYPE_DMESG, part,
+               id = psinfo->write(PSTORE_TYPE_DMESG, part, reason,
                                   hsize + l1_cpy + l2_cpy, psinfo);
                if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
                        pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
@@ -207,7 +208,7 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)

        mutex_lock(&psinfo->buf_mutex);
        memcpy(psinfo->buf, buf, size);
-       id = psinfo->write(type, 0, size, psinfo);
+       id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
        if (pstore_is_mounted())
                pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
                              size, CURRENT_TIME, psinfo);
diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index ee0c952..b5ffe79 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -16,6 +16,7 @@
 #include <linux/list.h>

 enum kmsg_dump_reason {
+       KMSG_DUMP_UNKNOWN,
        KMSG_DUMP_OOPS,
        KMSG_DUMP_PANIC,
        KMSG_DUMP_KEXEC,
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index cc03bbf..fa049e8 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -22,6 +22,8 @@
 #ifndef _LINUX_PSTORE_H
 #define _LINUX_PSTORE_H

+#include <linux/kmsg_dump.h>
+
 /* types */
 enum pstore_type_id {
        PSTORE_TYPE_DMESG       = 0,
@@ -40,7 +42,8 @@ struct pstore_info {
        ssize_t         (*read)(u64 *id, enum pstore_type_id *type,
                        struct timespec *time, struct pstore_info *psi);
        u64             (*write)(enum pstore_type_id type, unsigned int part,
-                       size_t size, struct pstore_info *psi);
+                       enum kmsg_dump_reason reason, size_t size,
+                       struct pstore_info *psi);
        int             (*erase)(enum pstore_type_id type, u64 id,
                        struct pstore_info *psi);
        void            *data;
--
1.7.6.2

--
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/

Re: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Chen Gong]

于 2011/9/21 20:40, Matthew Garrett 写道:
> On Wed, Sep 21, 2011 at 11:31:40AM +0800, Chen Gong wrote:
>>>   	mutex_lock(&psinfo->buf_mutex);
>>>   	memcpy(psinfo->buf, buf, size);
>>> -	id = psinfo->write(type, 0, size, psinfo);
>>> +	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
>>
>> I can't say it is wrong because no real caller for this function, but I can't
>> say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
>> any reason to use this type here ?
>
> If a function calls pstore_write() directly then we have no type to
> associate with it. It seems worth making this explicit.
>

Yep, that's the point. We hope to get a more reasonable method to do it, not
any assumption.

Re: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Matthew Garrett]

On Thu, Sep 22, 2011 at 10:21:58AM +0800, Chen Gong wrote:
> 于 2011/9/21 20:40, Matthew Garrett 写道:
> >On Wed, Sep 21, 2011 at 11:31:40AM +0800, Chen Gong wrote:
> >>>  	mutex_lock(&psinfo->buf_mutex);
> >>>  	memcpy(psinfo->buf, buf, size);
> >>>-	id = psinfo->write(type, 0, size, psinfo);
> >>>+	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
> >>
> >>I can't say it is wrong because no real caller for this function, but I can't
> >>say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
> >>any reason to use this type here ?
> >
> >If a function calls pstore_write() directly then we have no type to
> >associate with it. It seems worth making this explicit.
> 
> Yep, that's the point. We hope to get a more reasonable method to do it, not
> any assumption.

I'm afraid I don't understand. Could you give an example of how you 
think this should look?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Chen Gong]

于 2011/9/22 21:15, Matthew Garrett 写道:
> On Thu, Sep 22, 2011 at 10:21:58AM +0800, Chen Gong wrote:
>> 于 2011/9/21 20:40, Matthew Garrett 写道:
>>> On Wed, Sep 21, 2011 at 11:31:40AM +0800, Chen Gong wrote:
>>>>>   	mutex_lock(&psinfo->buf_mutex);
>>>>>   	memcpy(psinfo->buf, buf, size);
>>>>> -	id = psinfo->write(type, 0, size, psinfo);
>>>>> +	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
>>>>
>>>> I can't say it is wrong because no real caller for this function, but I can't
>>>> say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
>>>> any reason to use this type here ?
>>>
>>> If a function calls pstore_write() directly then we have no type to
>>> associate with it. It seems worth making this explicit.
>>
>> Yep, that's the point. We hope to get a more reasonable method to do it, not
>> any assumption.
>
> I'm afraid I don't understand. Could you give an example of how you
> think this should look?
>
If you insist on your design, I prefer deleting the function pstore_write before 
applying your patch. We all know no real users to call this function,
every backend will register Its own callback, so this function is useless at all.

RE: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 857 bytes --]

> If you insist on your design, I prefer deleting the function pstore_write before 
> applying your patch. We all know no real users to call this function,
> every backend will register Its own callback, so this function is useless at all.

We could do that. I threw in pstore_write without any particular use case in mind.
Since there hasn't been a rush of people adding calls to it (and since it seems
that pstore back ends have very limited amounts of space at their disposal, any
use of pstore_write would probably cause problems for the dmesg dump model)
removing it sounds like a good idea.

If someone does want to use it - they will have to figure out how to solve
priority issues.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Seiji Aguchi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1313 bytes --]


>>>>>>   	mutex_lock(&psinfo->buf_mutex);
>>>>>>   	memcpy(psinfo->buf, buf, size);
>>>>>> -	id = psinfo->write(type, 0, size, psinfo);
>>>>>> +	id = psinfo->write(type, 0, KMSG_DUMP_UNKNOWN, size, psinfo);
>>>>>
>>>>> I can't say it is wrong because no real caller for this function, but I can't
>>>>> say it is right, yet. KMSG_DUMP_UNKNOWN here looks too arbirary. Do you have
>>>>> any reason to use this type here ?
>>>>
>>>> If a function calls pstore_write() directly then we have no type to
>>>> associate with it. It seems worth making this explicit.
>>>
>>> Yep, that's the point. We hope to get a more reasonable method to do it, not
>>> any assumption.
>>
>> I'm afraid I don't understand. Could you give an example of how you
>> think this should look?
>>
>If you insist on your design, I prefer deleting the function pstore_write before
>applying your patch. We all know no real users to call this function,
>every backend will register Its own callback, so this function is useless at all.

Matthew,

Do you plan to resend this patch?
Your patch will be accepted if you resend it,
because pstore_write() has been deleted by Kees.

Seiji
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] efi: Avoid sysfs spew on reboot and panic

[Seiji Aguchi]

Matthew,

Do you have any comment on this patch?

Seiji

> -----Original Message-----
> From: Seiji Aguchi
> Sent: Wednesday, March 07, 2012 5:50 PM
> To: linux-kernel@vger.kernel.org; Luck, Tony (tony.luck@intel.com); Chen Gong (gong.chen@linux.intel.com); Matthew Garrett
> (mjg@redhat.com); dzickus@redhat.com
> Cc: dle-develop@lists.sourceforge.net; Satoru Moriya
> Subject: [PATCH] efi: Avoid sysfs spew on reboot and panic
> 
> Hi,
> 
> This patch just modified Matthew's patch which has not included in upstream to fit current upstream code.
> 
> https://lkml.org/lkml/2011/9/20/468
> 
>  Right now all pstore accesses to efivars will delete or create new sysfs  nodes. This is less than ideal if we've panicked or rebooting for
> following reasons.
>   - efi_pstore may not work if kernel panics in interrupt context, since
>     cpu can sleep while creating sysfs files.
>   - we don't need to create sysfs if we've panicked or rebooting, because
>     no one can access to it.
> 
> 
> Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/firmware/efivars.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index d25599f..34c8890 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -550,6 +550,16 @@ static int efi_pstore_write(enum pstore_type_id type,
> 
>  	spin_unlock(&efivars->lock);
> 
> +	/*
> +	 * If it's more severe than KMSG_DUMP_OOPS then we're already dead.
> +	 * Don't bother playing with sysfs.
> +	 */
> +
> +	if (reason != KMSG_DUMP_OOPS) {
> +		*id = part;
> +		return ret;
> +	}
> +
>  	if (found)
>  		efivar_unregister(found);
> 
> -- 1.7.1

[PATCH] efi: Avoid sysfs spew on reboot and panic

[Seiji Aguchi]

Hi,

This patch just modified Matthew's patch which has not included in upstream to fit current upstream code.

https://lkml.org/lkml/2011/9/20/468

 Right now all pstore accesses to efivars will delete or create new sysfs  nodes. This is less than ideal if we've panicked or rebooting for  following reasons.
  - efi_pstore may not work if kernel panics in interrupt context, since
    cpu can sleep while creating sysfs files.
  - we don't need to create sysfs if we've panicked or rebooting, because
    no one can access to it.


Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/firmware/efivars.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d25599f..34c8890 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -550,6 +550,16 @@ static int efi_pstore_write(enum pstore_type_id type,
 
 	spin_unlock(&efivars->lock);
 
+	/*
+	 * If it's more severe than KMSG_DUMP_OOPS then we're already dead.
+	 * Don't bother playing with sysfs.
+	 */
+
+	if (reason != KMSG_DUMP_OOPS) {
+		*id = part;
+		return ret;
+	}
+
 	if (found)
 		efivar_unregister(found);
 
-- 1.7.1