Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

On Wed, 13 May 2020 at 08:08, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> I think this one should contain KernelVersion as well, see
> Documentation/ABI/README.

Thanks, I'll fix that up.

> I think you can always include this header without #ifs

Thanks.

> >  static struct resource wdt_ich_res[] = {
> > @@ -221,6 +236,16 @@ enum lpc_chipsets {
> >       LPC_APL,        /* Apollo Lake SoC */
> >       LPC_GLK,        /* Gemini Lake SoC */
> >       LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
> > +     LPC_SPT,        /* Sunrise Point */
> > +     LPC_KLK,        /* Kaby Lake */
> KBL for Kaby Lake

I can fix up all those, but out of interest how did you "know" the
right three digit identifier to use?

> This is not PCH, Cactus Ridge is Thunderbolt host controller AFAIK.

This was suggested from someone testing the original spi_lpc.c code on
a macbook, I can remove it for now and work out if it's incorrect
later.

> For example these PCI IDs are for the SPI-NOR controller (not LPC
> controller) so this causes this driver to try to bind to a completely
> different device which it cannot handle.

I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
this kind of "just expose one byte of PCI config space" functionality.
Certainly drivers/platform/x86/intel_spi_lpc.c is much simpler, and
would also allow me to do some of the chipsec tests in the future --
for instance if BIOSWE is unset but BLE is set, try setting BIOSWE and
check that SMM clears it back.

> > +     char tmp[2];
>
> Wouldn't this need to account the '\0' as well?

It's one char ('1' or '0') plus '`\0` -- no?

> I think "spi" is bit too general name here. I would expect "spi" to
> actually refer to something connected to spi bus and possibly coming
> from drivers/spi/*.
> Perhaps "bios_protections" or something like that.

Sure, that's a good idea. I know BIOS is a badword now, so how about
just "firmware"? so /sys/kernel/security/firmware/bioswe

> > +     securityfs_remove(priv->spi_dir);
> > +     return -1;
> I don't know securityfs well enought but I think -1 is not correct here
> and if you want that then maybe -EPERM instead.

I will look, I don't think the actual value is terribly important. The
only time I can trigger this is forgetting to remove the securityfs
file in module unload, and then trying to re-insert the module --
which failed with -EEXIST from memory.

> I wonder if you can simply call
>         securityfs_remove(priv->spi_dir);
> and that removes the children automatically? I'm do not know securityfs
> so it may not be the case.

No, that doesn't work.

> >  struct intel_spi_boardinfo {
> >       enum intel_spi_type type;
> >       bool writeable;
> > +     bool ble;
> > +     bool smm_bwp;
>
> I don't think these belong here. They should be part of the lpc private
> structure instead (lpc_ich_priv).

Can fix, thanks.

Richard.