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

[Mika Westerberg]

On Wed, May 13, 2020 at 09:48:55AM +0100, Richard Hughes wrote:
> 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?

I work for Intel ;-)

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

It is definitely incorrect. They are completely different things.

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

Ideally there is one driver per device. Otherwise we end up issues when
the device appears and there are several to choose from, which one to
pick.

If this is touching the 00:1f.5 PCI device (SPI-NOR controller) then the
right place is the intel-spi-pci.c as that's the driver for this
controller. We can put this there so that it does not enable the SPI-NOR
functionality itself and the mark the SPI-NOR functionality only as
being dangerous or something like that.

> 
> > > +     char tmp[2];
> >
> > Wouldn't this need to account the '\0' as well?
> 
> It's one char ('1' or '0') plus '`\0` -- no?

You sprint() there "%d\n", so that includes a number, '\n' and '\0' unless
I'm missing something.

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

Yup, sounds good :)

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

OK