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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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