Hi Jorge,
On 10/18/22 22:35, Jorge Lopez wrote:
> Version 2 -> Version 3
> ------------------------------------------------------
>
> Introduction of HP-BIOSCFG driver (1/6)
> - No new changes
>
> Update pending_reboot state value (2/6)
> - No new changes
>
> Set current_value permissions appropriate to read-only attributes (3/6)
> - No new changes
>
> Improve friendly display name values (4/6)
> - No new changes
>
> Moving existing HP drivers to a central location (5/6)
>
> The purpose of this patch is to provide a central location where all
> HP related drivers are found. HP drivers will recide under hp
> directory with exception of hp-bioscfg. hp-bioscfg is found under
> hp/hp-bioscfg/ directory.
>
> Introduced changes to Kconfig file to list all HP driver under "HP X86
> Platform Specific Device Drivers" menu option.
>
> Changes needed to relocate hp-bioscfg driver
>
> Relocation of the driver requires minor changes such updating the path
> for a local include file. Additional changes include update MAINTAINERS
> file to indicated support status, reviewer, and maintainer of hp-bioscfg
> driver. Lastly, removal of 'stddef' include file from the source.
>
> Clarify how elements order list impacts priority (6/6)
>
> This patch provides additional clarification and describes how
> priority is determined according to the order. Elements listed first
> are given higher priority to those listed last.
>
> Version 1 -> Version 2
> ------------------------------------------------------
>
> Introduction of HP-BIOSCFG driver (1/4)
> - No new changes
>
> Update pending_reboot state value (2/4)
>
> There is not a reliable mechanism to programmatically determine which
> BIOS settings require a reboot to be updated. The latest changes
> leverages “RequiredPhysicalPresence” reported value to set
> pending_reboot.
>
> Set current_value permissions appropriate to read-only attributes (3/4)
>
> This patch updates ‘current_value’ permissions to match the value
> reported by ‘is_readonly’ value associated with the attribute.
> ‘current_value’ permissions are set to read-only if ‘is_readonly’
> value is 1. ‘current_value’ permissions are set to read-write if
> ‘is_readonly’ value is zero. Other read-only and write-only
> permissions will remain unchanged.
>
> Improve friendly display name values (4/4)
>
> The purpose of this patch is to improve the friendly display name for
> few numbers of attributes associated with ‘Schedule Power-ON.’ BIOS
> assign names such ‘Tuesday’ to an attribute. The name is correct, but
> it is not descriptive enough for the user. It is under those
> conditions a portion of the path data value is append to the attribute
> name to create a user-friendly name.
>
> For instance, the attribute name is ‘Tuesday,’ and the display name
> value is ‘Schedule Power-ON – Tuesday’
>
>
> ------------------------------------------------------
> Version: 1
>
> Introduction of HP-BIOSCFG driver (1/4)
>
> The purpose for this patch is submit HP BIOSCFG driver to be list of
> HP Linux kernel drivers. HP BIOS Configuration driver purpose is to
> provide a driver supporting the latest sysfs class firmware attributes
> framework allowing the user to change BIOS settings and security
> solutions on HP Inc.’s commercial notebooks.
>
> Many features of HP Commercial PC’s can be managed using Windows
> Management Instrumentation (WMI). WMI is an implementation of Web-Based
> Enterprise Management (WBEM) that provides a standards-based interface
> for changing and monitoring system settings. HP BISOCFG driver provides
> a native Linux solution and the exposed features facilitates the
> migration to Linux environments.
>
> The Linux security features to be provided in hp-bioscfg driver enables
> managing the BIOS settings and security solutions via sysfs, a virtual
> filesystem that can be used by user-mode applications. The new
> documentation cover features such Secure Platform Management, Sure
> Admin, and Sure Start. Each section provides security feature
> description and identifies sysfs directories and files exposed by
> the driver.
>
> Many HP Commercial PC’s include a feature called Secure Platform
> Management (SPM), which replaces older password-based BIOS settings
> management with public key cryptography. PC secure product management
> begins when a target system is provisioned with cryptographic keys
> that are used to ensure the integrity of communications between system
> management utilities and the BIOS.
>
> HP Commercial PC’s have several BIOS settings that control its behaviour
> and capabilities, many of which are related to security. To prevent
> unauthorized changes to these settings, the system can be configured
> to use a Sure Admin cryptographic signature-based authorization string
> that the BIOS will use to verify authorization to modify the setting.
>
>
> Jorge Lopez (6):
> Introduction of HP-BIOSCFG driver
> Update pending_reboot state value
> Set current_value permissions appropriate to read-only attributes
> Improve friendly display name values
> Moving existing HP drivers to a central location
> Clarify how elements order list impacts priority
Jorge, thank you for your continued work on this.
I will try to get a detailed review of this done next week (no time this
week, sorry).
But before then can you please restructure the patch series to make
it easier to review ? ATM the series seems to be the original v1
patch + patches on top addressing review comments. That is a somewhat
unusual way to address review comments. Usually new versions include
improvements from review directly in the patch introducing the change
which got commented on.
What I would like to see for v4 of the series is a patch series like
this (with all the hp-bioscfg patches squashed together before doing
this new split):
[PATCH 1/x] Move the existing HP drivers to the new hp subdir (instead of doing this in 5/6)
[PATCH 2/x] Introduce the main bioscfg.h file + *-attributes.c files, yes this won't
compile, but without a Makefile pointing to it that is fine
[PATCH 3/x] Add the main bioscfg.c + Makefile + Kconfig changes
Note feel free to split patch 2/x in maybe 2 or 3 patches adding a few
*-attributes.c files at a time.
The idea here is to make the individual .patch files smaller because
I personally find that reviewing big patch files does not work well
(I tend to loose track of everything which is going on if the patch
is too big / I loose my way because of too much scrolling).
Regards,
Hans
