Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2af933f1-1662-2c91-b4da-6e067a9a9389@redhat.com>
Date:   Wed, 26 Apr 2023 15:04:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v11 01/14] HP BIOSCFG driver - Documentation
Content-Language: en-US, nl
To:     Jorge Lopez <jorgealtxwork@gmail.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        thomas@t-8ch.de
References: <20230420165454.9517-1-jorge.lopez2@hp.com>
 <20230420165454.9517-2-jorge.lopez2@hp.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230420165454.9517-2-jorge.lopez2@hp.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jorge, Thomas,

Thank you both so much for all your work on this!

The userspace API of this looks like it is pretty much
done now (after the discussed changes for
the "Sure_Start" attribute), which is great.

I have one small remark below (inline).

On 4/20/23 18:54, Jorge Lopez wrote:

<snip>

> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index 4cdba3477176..73d7b8fbc0b2 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -22,6 +22,12 @@ Description:
>  			- integer: a range of numerical values
>  			- string
>  
> +		HP specific types
> +		-----------------
> +			- ordered-list - a set of ordered list valid values
> +			- sure-start - report audit logs read from BIOS
> +
> +
>  		All attribute types support the following values:
>  
>  		current_value:
> @@ -126,6 +132,44 @@ Description:
>  					value will not be effective through sysfs until this rule is
>  					met.
>  
> +		HP specific class extensions
> +		------------------------------
> +
> +		On HP systems the following additional attributes are available:
> +
> +		"ordered-list"-type specific properties:
> +
> +		elements:
> +					A file that can be read to obtain the possible
> +					list of values of the <attr>. Values are separated using
> +					semi-colon (``;``). The order individual elements are listed
> +					according to their priority.  An Element listed first has the
> +					highest priority. Writing the list in a different order to
> +					current_value alters the priority order for the particular
> +					attribute.
> +
> +		"sure-start"-type specific properties:
> +
> +		audit_log_entries:
> +					A read-only file that returns the events in the log.
> +					Values are separated using semi-colon (``;``)

Looking at the documented format which seems to be 128 raw bytes per entry, I think
that the "Values are separated using semi-colon (``;``)" line is not correct here
and that line should not removed here ?

But maybe I'm misunderstanding things here. Do you have an example
of what catting (or "hexdump -C"-ing if binary)
the "audit_log_entries" sysfs file looks like ? 


> +
> +					Audit log entry format
> +
> +					Byte 0-15:   Requested Audit Log entry  (Each Audit log is 16 bytes)
> +					Byte 16-127: Unused
> +
> +		audit_log_entry_count:
> +					A read-only file that returns the number of existing audit log events available to be read.
> +					Values are separated using comma (``,``)
> +
> +					[No of entries],[log entry size],[Max number of entries supported]
> +
> +					log entry size identifies audit log size for the current BIOS version.
> +					The current size is 16 bytes but it can be to up to 128 bytes long
> +					in future BIOS versions.
> +
> +
>  What:		/sys/class/firmware-attributes/*/authentication/
>  Date:		February 2021
>  KernelVersion:	5.11

<snip>

> @@ -311,7 +364,7 @@ Description:
>  			==	=========================================
>  			0	All BIOS attributes setting are current
>  			1	A reboot is necessary to get pending BIOS
> -			        attribute changes applied
> +				attribute changes applied
>  			==	=========================================
>  
>  		Note, userspace applications need to follow below steps for efficient

This seems like an unrelated whitespace change which
has accidentally ended up in this patch.

Regards,

Hans


p.s.

I'll also read / catch up with all the comments on the actual implementation
(patches 2-14) and I'll let you know if I have any remarks there.