linux-kernel - Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFE3TVxqpt4-lmYP@cyber-t14sg4>
Date: Tue, 17 Jun 2025 11:37:17 +0200
From: Michal Gorlas <michal.gorlas@...ements.com>
To: Brian Norris <briannorris@...omium.org>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>,
	Julius Werner <jwerner@...omium.org>, marcello.bauer@...ements.com,
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM
 related informations from coreboot tables

On Mon, Jun 16, 2025 at 11:16:18AM -0700, Brian Norris wrote:
> On Sat, Jun 14, 2025 at 02:53:18PM +0200, Michal Gorlas wrote:
> > On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> > > > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> > > >   * boilerplate.  Each module may only use this macro once, and
> > > >   * calling it replaces module_init() and module_exit()
> > > >   */
> > > > -#define module_coreboot_driver(__coreboot_driver) \
> > > > +#define module_coreboot_driver(__coreboot_driver)                  \
> > > >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > > > -			coreboot_driver_unregister)
> > > > +		      coreboot_driver_unregister)
> > > 
> > > You're making arbitrary whitespace changes in this hunk. Try to avoid
> > > that, please.
> > > 
> > 
> > Sure, will do. It came from a style warning when running
> > scripts/checkpatch.pl. I thought it could be useful to fix it on the
> > same go.
> 
> That's odd, I don't see any such warning. Anyway, typically I'd expect
> such things not to be lumped together under the "separate your changes"
> guidance of Documentation/process/submitting-patches.rst (if they're
> worth changing at all), although that may not be a hard and fast rule.

My bad, the whitespace were not a consequence of checkpatch.pl
complaints, but rather running clang-format on coreboot_table.h
on a clean tree:

 * $ clang-format -i drivers/firmware/google/coreboot_table.h
 * $ git diff
 * diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
 * index bb6f0f7299b4..3d933c657535 100644
 * --- a/drivers/firmware/google/coreboot_table.h
 * +++ b/drivers/firmware/google/coreboot_table.h
 * @@ -61,15 +61,15 @@ struct lb_framebuffer {
 *         u32 x_resolution;
 *         u32 y_resolution;
 *         u32 bytes_per_line;
 * -       u8  bits_per_pixel;
 * -       u8  red_mask_pos;
 * -       u8  red_mask_size;
 * -       u8  green_mask_pos;
 * -       u8  green_mask_size;
 * -       u8  blue_mask_pos;
 * -       u8  blue_mask_size;
 * -       u8  reserved_mask_pos;
 * -       u8  reserved_mask_size;
 * +       u8 bits_per_pixel;
 * +       u8 red_mask_pos;
 * +       u8 red_mask_size;
 * +       u8 green_mask_pos;
 * +       u8 green_mask_size;
 * +       u8 blue_mask_pos;
 * +       u8 blue_mask_size;
 * +       u8 reserved_mask_pos;
 * +       u8 reserved_mask_size;
 *  };
 * 
 *  /* A device, additionally with information from coreboot. */
 * @@ -112,8 +112,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
 *   * boilerplate.  Each module may only use this macro once, and
 *   * calling it replaces module_init() and module_exit()
 *   */
 * -#define module_coreboot_driver(__coreboot_driver) \
 * +#define module_coreboot_driver(__coreboot_driver)                  \
 *         module_driver(__coreboot_driver, coreboot_driver_register, \
 * -                       coreboot_driver_unregister)
 * +                     coreboot_driver_unregister)
 * 
 *  #endif /* __COREBOOT_TABLE_H */

Anyway, I am not sure if it is even worth changing, its rather cosmetic. I
removed these changes in v2. 

Best,
Michal