diff mbox series

[v2,3/5] efi/cper: Add a new helper function to print bitmasks

Message ID 5bb5f806a763b295401972fdff17bb455bee2e82.1720679234.git.mchehab+huawei@kernel.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [v2,1/5] RAS: Report all ARM processor CPER information to userspace | expand

Commit Message

Mauro Carvalho Chehab July 11, 2024, 6:28 a.m. UTC
Sometimes it is desired to produce a single log line for errors.
Add a new helper function for such purpose.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  2 ++
 2 files changed, 45 insertions(+)

Comments

Jonathan Cameron July 11, 2024, 3:24 p.m. UTC | #1
On Thu, 11 Jul 2024 08:28:54 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Sometimes it is desired to produce a single log line for errors.
> Add a new helper function for such purpose.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
LGTM

Reviewed-by; Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 7d2cdd9e2227..462d739e8dd1 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  		printk("%s\n", buf);
>  }
>  
> +/**
> + * cper_bits_to_str - return a string for set bits
> + * @buf: buffer to store the output string
> + * @buf_size: size of the output string buffer
> + * @bits: bit mask
> + * @strs: string array, indexed by bit position
> + * @strs_size: size of the string array: @strs
> + *
> + * Add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits,
> + * add the corresponding string describing the bit in @strs to @buf.
> + *
> + * Return: number of bytes stored or an error code if lower than zero.
> + */
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size)
> +{
> +	int len = buf_size;
> +	char *str = buf;
> +	int i, size;
> +
> +	*buf = '\0';
> +
> +	for_each_set_bit(i, &bits, strs_size) {
> +		if (!(bits & (1U << (i))))
> +			continue;
> +
> +		if (*buf && len > 0) {
> +			*str = '|';
> +			len--;
> +			str++;
> +		}
> +
> +		size = strscpy(str, strs[i], len);
> +		if (size < 0)
> +			return size;
> +
> +		len -= size;
> +		str += size;
> +	}
> +	return len - buf_size;
> +}
> +EXPORT_SYMBOL_GPL(cper_bits_to_str);
> +
>  static const char * const proc_type_strs[] = {
>  	"IA32/X64",
>  	"IA64",
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 265b0f8fc0b3..25858a7608b7 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -584,6 +584,8 @@ const char *cper_mem_err_type_str(unsigned int);
>  const char *cper_mem_err_status_str(u64 status);
>  void cper_print_bits(const char *prefix, unsigned int bits,
>  		     const char * const strs[], unsigned int strs_size);
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size);
>  void cper_mem_err_pack(const struct cper_sec_mem_err *,
>  		       struct cper_mem_err_compact *);
>  const char *cper_mem_err_unpack(struct trace_seq *,
Borislav Petkov Sept. 2, 2024, 11:24 a.m. UTC | #2
On Thu, Jul 11, 2024 at 08:28:54AM +0200, Mauro Carvalho Chehab wrote:
> Sometimes it is desired to produce a single log line for errors.
> Add a new helper function for such purpose.

How does this have anything to do with the below function?

Example?

Why isn't anything in lib/bitmap-str.c not useful for this?

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 7d2cdd9e2227..462d739e8dd1 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  		printk("%s\n", buf);
>  }
>  
> +/**
> + * cper_bits_to_str - return a string for set bits
> + * @buf: buffer to store the output string
> + * @buf_size: size of the output string buffer
> + * @bits: bit mask
> + * @strs: string array, indexed by bit position
> + * @strs_size: size of the string array: @strs
> + *
> + * Add to @buf the bitmask in hexadecimal. 

Where does it do that?

> Then, for each set bit in @bits,
> + * add the corresponding string describing the bit in @strs to @buf.
> + *
> + * Return: number of bytes stored or an error code if lower than zero.
> + */
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size)
> +{
> +	int len = buf_size;
> +	char *str = buf;
> +	int i, size;
> +
> +	*buf = '\0';
> +
> +	for_each_set_bit(i, &bits, strs_size) {
> +		if (!(bits & (1U << (i))))

BIT_UL()

> +			continue;
> +
> +		if (*buf && len > 0) {

Uff, this is testing the first char in buf and it gets copied in below in
strscpy() through the str pointer.

So this function converts a set of set bits to their corresponding *names*
from strs[].

This name doesn't even begin to explain what this function does - it converts
a bitmap to the corresponding names of the bits in @strs. If anything, the
above comment needs an example and the function needs to be named properly.
Mauro Carvalho Chehab Sept. 4, 2024, 5:24 a.m. UTC | #3
Em Mon, 2 Sep 2024 13:24:29 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Jul 11, 2024 at 08:28:54AM +0200, Mauro Carvalho Chehab wrote:
> > Sometimes it is desired to produce a single log line for errors.
> > Add a new helper function for such purpose.  
> 
> How does this have anything to do with the below function?

There is a variant at cper.c that creates a multi-line output
for bitmaps.

> Example?
> 
> Why isn't anything in lib/bitmap-str.c not useful for this?

I took a look there. I wasn't able to find anything remotely
close to convert a bitmap into their correspondent bit names.

See, the intended usage for this function is to convert ACPI
bitmasks into the field names. On ARM Processor Error, this is
used to properly parse the type field, as described at:

	https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information

The definition for this specific bitmask starts on bit 1,
so the logic to parse it uses a FIELD_GET to apply the
proper bitmask. This is how it is used (see patch 4/5):

	const char * const cper_proc_error_type_strs[] = {
		"cache error",
		"TLB error",
		"bus error",
		"micro-architectural error",
	};

	#define CPER_ARM_ERR_TYPE_MASK                     GENMASK(4,1)

	char error_type[120];

	cper_bits_to_str(error_type, sizeof(error_type),
			 FIELD_GET(CPER_ARM_ERR_TYPE_MASK, err_info->type),
			 cper_proc_error_type_strs,
			 ARRAY_SIZE(cper_proc_error_type_strs));

I'll add an example similar to it to kernel-doc comment.

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
> >  include/linux/cper.h        |  2 ++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > index 7d2cdd9e2227..462d739e8dd1 100644
> > --- a/drivers/firmware/efi/cper.c
> > +++ b/drivers/firmware/efi/cper.c
> > @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
> >  		printk("%s\n", buf);
> >  }
> >  
> > +/**
> > + * cper_bits_to_str - return a string for set bits
> > + * @buf: buffer to store the output string
> > + * @buf_size: size of the output string buffer
> > + * @bits: bit mask
> > + * @strs: string array, indexed by bit position
> > + * @strs_size: size of the string array: @strs
> > + *

> > + * Add to @buf the bitmask in hexadecimal.   
> 
> Where does it do that?

Legacy comment from the past version. Will drop it.

> > Then, for each set bit in @bits,
> > + * add the corresponding string describing the bit in @strs to @buf.
> > + *
> > + * Return: number of bytes stored or an error code if lower than zero.
> > + */
> > +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> > +		     const char * const strs[], unsigned int strs_size)
> > +{
> > +	int len = buf_size;
> > +	char *str = buf;
> > +	int i, size;
> > +
> > +	*buf = '\0';
> > +
> > +	for_each_set_bit(i, &bits, strs_size) {
> > +		if (!(bits & (1U << (i))))  
> 
> BIT_UL()
> 
> > +			continue;
> > +
> > +		if (*buf && len > 0) {  
> 
> Uff, this is testing the first char in buf and it gets copied in below in
> strscpy() through the str pointer.
> 
> So this function converts a set of set bits to their corresponding *names*
> from strs[].

Yes.

> This name doesn't even begin to explain what this function does - it converts
> a bitmap to the corresponding names of the bits in @strs. If anything, the
> above comment needs an example and the function needs to be named properly.

I'll add an example.

Thanks,
Mauro
diff mbox series

Patch

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 7d2cdd9e2227..462d739e8dd1 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -106,6 +106,49 @@  void cper_print_bits(const char *pfx, unsigned int bits,
 		printk("%s\n", buf);
 }
 
+/**
+ * cper_bits_to_str - return a string for set bits
+ * @buf: buffer to store the output string
+ * @buf_size: size of the output string buffer
+ * @bits: bit mask
+ * @strs: string array, indexed by bit position
+ * @strs_size: size of the string array: @strs
+ *
+ * Add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits,
+ * add the corresponding string describing the bit in @strs to @buf.
+ *
+ * Return: number of bytes stored or an error code if lower than zero.
+ */
+int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
+		     const char * const strs[], unsigned int strs_size)
+{
+	int len = buf_size;
+	char *str = buf;
+	int i, size;
+
+	*buf = '\0';
+
+	for_each_set_bit(i, &bits, strs_size) {
+		if (!(bits & (1U << (i))))
+			continue;
+
+		if (*buf && len > 0) {
+			*str = '|';
+			len--;
+			str++;
+		}
+
+		size = strscpy(str, strs[i], len);
+		if (size < 0)
+			return size;
+
+		len -= size;
+		str += size;
+	}
+	return len - buf_size;
+}
+EXPORT_SYMBOL_GPL(cper_bits_to_str);
+
 static const char * const proc_type_strs[] = {
 	"IA32/X64",
 	"IA64",
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 265b0f8fc0b3..25858a7608b7 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -584,6 +584,8 @@  const char *cper_mem_err_type_str(unsigned int);
 const char *cper_mem_err_status_str(u64 status);
 void cper_print_bits(const char *prefix, unsigned int bits,
 		     const char * const strs[], unsigned int strs_size);
+int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
+		     const char * const strs[], unsigned int strs_size);
 void cper_mem_err_pack(const struct cper_sec_mem_err *,
 		       struct cper_mem_err_compact *);
 const char *cper_mem_err_unpack(struct trace_seq *,