| 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 |
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 *,
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.
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 --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 *,
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(+)