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

[Michal Gorlas <michal.gorlas@9elements.com>]

Hi,
Thanks for taking your time to go over the patches.

On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> 
> This all looks highly X86-specific, so you probably need 'depends on
> X86'.
> 

Indeed, this was also why CI build was failed.

> > @@ -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.

> >  
> >  #endif /* __COREBOOT_TABLE_H */
> > diff --git a/drivers/firmware/google/mm_info.c b/drivers/firmware/google/mm_info.c
> > new file mode 100644
> > index 000000000000..55bcdc8b8d53
> > --- /dev/null
> > +++ b/drivers/firmware/google/mm_info.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * mm_info.c
> > + *
> > + * Driver for exporting MM payload information from coreboot table.
> > + *
> > + * Copyright 2025 9elements gmbh
> > + * Copyright 2025 Michal Gorlas <michal.gorlas@9elements.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "coreboot_table.h"
> > +#include "mm_payload.h"
> > +
> > +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> > +struct mm_info *mm_info;
> > +
> > +static int mm_driver_probe(struct coreboot_device *dev)
> > +{
> > +	mm_cbtable_info = &dev->mm_info;
> > +	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
> > +		return -ENXIO;
> > +
> > +	mm_info = kmalloc(sizeof(*mm_info), GFP_KERNEL);
> 
> Error handling? (Needs a NULL check.)
> 
> And might as well use devm_*() (e.g., devm_kzalloc()); then you can drop
> mm_driver_remove().
> 
> > +	mm_info->revision = mm_cbtable_info->revision;
> > +	mm_info->requires_long_mode_call =
> > +		mm_cbtable_info->requires_long_mode_call;
> > +	mm_info->register_mm_entry_command =
> > +		mm_cbtable_info->register_mm_entry_command;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mm_info);
> 
> Why non-GPL? IIUC, EXPORT_SYMBOL_GPL() is the usual default.
> 
> Or, I suspect you don't actually need two separate modules, so you
> probably don't need this EXPORT at all.
> 

Yep these two definetely can be merged into one module. Will do that in
v2 series.

Best,
Michal