[PATCH 0/1] spi: intel: Remove DANGEROUS tag from pci driver

[PATCH 0/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mauro Lima]

Given that the PCI driver handles controllers that only work with
hardware sequencing, we can remove the dangerous tag.
This patch is the second part of Mika's suggestion [1].
The first part was accepted in [2].

[1] https://lore.kernel.org/r/Y1d9glOgHsQlZe2L@black.fi.intel.com/
[2] https://lore.kernel.org/linux-spi/20230201205455.550308-1-mauro.lima@eclypsium.com/

Mauro Lima (1):
  spi: intel: Remove DANGEROUS tag from pci driver

 drivers/spi/Kconfig | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)


base-commit: 6437d7e4505debf3e2ea6cf1d04e9f8afd834445
-- 
2.39.1

[PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mauro Lima]

Modern CPUs exposes this controller as PCI device that only uses
hardware sequencing capabilities which is safer than software
sequencing.
Leave the platform driver as *DANGEROUS* and update help text since
most of these controllers are using software sequencing.

Signed-off-by: Mauro Lima <mauro.lima@eclypsium.com>
---
 drivers/spi/Kconfig | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 87fc2bd16b72..3a362c450cb6 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -447,7 +447,7 @@ config SPI_INTEL
 	tristate
 
 config SPI_INTEL_PCI
-	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
+	tristate "Intel PCH/PCU SPI flash PCI driver"
 	depends on PCI
 	depends on X86 || COMPILE_TEST
 	depends on SPI_MEM
@@ -455,8 +455,9 @@ config SPI_INTEL_PCI
 	help
 	  This enables PCI support for the Intel PCH/PCU SPI controller in
 	  master mode. This controller is present in modern Intel hardware
-	  and is used to hold BIOS and other persistent settings. Using
-	  this driver it is possible to upgrade BIOS directly from Linux.
+	  and is used to hold BIOS and other persistent settings. This
+	  driver only supports hardware sequencing mode. Using this
+	  driver it is possible to upgrade BIOS directly from Linux.
 
 	  Say N here unless you know what you are doing. Overwriting the
 	  SPI flash may render the system unbootable.
@@ -471,10 +472,10 @@ config SPI_INTEL_PLATFORM
 	select SPI_INTEL
 	help
 	  This enables platform support for the Intel PCH/PCU SPI
-	  controller in master mode. This controller is present in modern
-	  Intel hardware and is used to hold BIOS and other persistent
-	  settings. Using this driver it is possible to upgrade BIOS
-	  directly from Linux.
+	  controller in master mode that is used to hold BIOS and other
+	  persistent settings. Most of these controllers are using
+	  software sequencing mode. Using this driver it is possible to
+	  upgrade BIOS directly from Linux.
 
 	  Say N here unless you know what you are doing. Overwriting the
 	  SPI flash may render the system unbootable.
-- 
2.39.1

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mika Westerberg]

Hi,

One nit: In $subject please use PCI instead of pci.

On Mon, Feb 06, 2023 at 03:31:43PM -0300, Mauro Lima wrote:
> Modern CPUs exposes this controller as PCI device that only uses
> hardware sequencing capabilities which is safer than software
> sequencing.
> Leave the platform driver as *DANGEROUS* and update help text since
> most of these controllers are using software sequencing.
> 
> Signed-off-by: Mauro Lima <mauro.lima@eclypsium.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Michael Walle]

> Modern CPUs exposes this controller as PCI device that only uses
> hardware sequencing capabilities which is safer than software
> sequencing.
> Leave the platform driver as *DANGEROUS* and update help text since
> most of these controllers are using software sequencing.

Out of curiosity, what is hardware sequencing? Maybe this should
be explained a bit more in the Kconfig help text. Looks like the
dangerous was there because you can update the bios and that
could eventually lead to a bricked mainboard. So hardware
sequencing helps there? how?

-michael

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mika Westerberg]

Hi,

On Tue, Feb 07, 2023 at 02:52:54PM +0100, Michael Walle wrote:
> > Modern CPUs exposes this controller as PCI device that only uses
> > hardware sequencing capabilities which is safer than software
> > sequencing.
> > Leave the platform driver as *DANGEROUS* and update help text since
> > most of these controllers are using software sequencing.
> 
> Out of curiosity, what is hardware sequencing? Maybe this should
> be explained a bit more in the Kconfig help text. Looks like the
> dangerous was there because you can update the bios and that
> could eventually lead to a bricked mainboard. So hardware
> sequencing helps there? how?

Hardware sequencing means the controller exposes just a bunch of "high
level" operations to the software. Such as read, write, erase and so on
but does not allow running the actual "low level" SPI-NOR opcodes.
Software sequencing on the other hand allows running pretty much any
opcode and this is what caused problems for certain Lenovo laptops few
years back that then resulted adding DANGEROUS to the Kconfig.

Typically the flash is locked by the BIOS so ordinary users cannot
really overwrite it, even by accident.

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Michael Walle]

Hi Mika,

Am 2023-02-07 15:03, schrieb Mika Westerberg:
> On Tue, Feb 07, 2023 at 02:52:54PM +0100, Michael Walle wrote:
>> > Modern CPUs exposes this controller as PCI device that only uses
>> > hardware sequencing capabilities which is safer than software
>> > sequencing.
>> > Leave the platform driver as *DANGEROUS* and update help text since
>> > most of these controllers are using software sequencing.
>> 
>> Out of curiosity, what is hardware sequencing? Maybe this should
>> be explained a bit more in the Kconfig help text. Looks like the
>> dangerous was there because you can update the bios and that
>> could eventually lead to a bricked mainboard. So hardware
>> sequencing helps there? how?
> 
> Hardware sequencing means the controller exposes just a bunch of "high
> level" operations to the software.

Ok, I figured it would have been something to do with the SPI driver
just supporting these high level ops. But even with that background
it was hard to connect that to the "hardware sequencing". The help
text should be somewhat understandable to the user/distro 
people/whoever,
right? So I'd suggest to explain that a bit more in detail, or don't
use the term hardware sequencing at all. I'm not sure.

> Such as read, write, erase and so on
> but does not allow running the actual "low level" SPI-NOR opcodes.
> Software sequencing on the other hand allows running pretty much any
> opcode and this is what caused problems for certain Lenovo laptops few
> years back that then resulted adding DANGEROUS to the Kconfig.

That information should go into the commit message.

> Typically the flash is locked by the BIOS so ordinary users cannot
> really overwrite it, even by accident.

I see, thanks for the explanation!

-michael

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mika Westerberg]

Hi,

On Tue, Feb 07, 2023 at 03:11:26PM +0100, Michael Walle wrote:
> Hi Mika,
> 
> Am 2023-02-07 15:03, schrieb Mika Westerberg:
> > On Tue, Feb 07, 2023 at 02:52:54PM +0100, Michael Walle wrote:
> > > > Modern CPUs exposes this controller as PCI device that only uses
> > > > hardware sequencing capabilities which is safer than software
> > > > sequencing.
> > > > Leave the platform driver as *DANGEROUS* and update help text since
> > > > most of these controllers are using software sequencing.
> > > 
> > > Out of curiosity, what is hardware sequencing? Maybe this should
> > > be explained a bit more in the Kconfig help text. Looks like the
> > > dangerous was there because you can update the bios and that
> > > could eventually lead to a bricked mainboard. So hardware
> > > sequencing helps there? how?
> > 
> > Hardware sequencing means the controller exposes just a bunch of "high
> > level" operations to the software.
> 
> Ok, I figured it would have been something to do with the SPI driver
> just supporting these high level ops. But even with that background
> it was hard to connect that to the "hardware sequencing". The help
> text should be somewhat understandable to the user/distro people/whoever,
> right? So I'd suggest to explain that a bit more in detail, or don't
> use the term hardware sequencing at all. I'm not sure.

I agree it should be made more understandable for the distro folks. At
least add some explanation why it is OK to select this.

Mauro, can you do that in the next version?

> > Such as read, write, erase and so on
> > but does not allow running the actual "low level" SPI-NOR opcodes.
> > Software sequencing on the other hand allows running pretty much any
> > opcode and this is what caused problems for certain Lenovo laptops few
> > years back that then resulted adding DANGEROUS to the Kconfig.
> 
> That information should go into the commit message.

+1

Re: [PATCH 1/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mauro Lima]

Hi all,

On Tue, Feb 7, 2023 at 2:25 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Tue, Feb 07, 2023 at 03:11:26PM +0100, Michael Walle wrote:
> > Hi Mika,
> >
> > Am 2023-02-07 15:03, schrieb Mika Westerberg:
> > > On Tue, Feb 07, 2023 at 02:52:54PM +0100, Michael Walle wrote:
> > > > > Modern CPUs exposes this controller as PCI device that only uses
> > > > > hardware sequencing capabilities which is safer than software
> > > > > sequencing.
> > > > > Leave the platform driver as *DANGEROUS* and update help text since
> > > > > most of these controllers are using software sequencing.
> > > >
> > > > Out of curiosity, what is hardware sequencing? Maybe this should
> > > > be explained a bit more in the Kconfig help text. Looks like the
> > > > dangerous was there because you can update the bios and that
> > > > could eventually lead to a bricked mainboard. So hardware
> > > > sequencing helps there? how?
> > >
> > > Hardware sequencing means the controller exposes just a bunch of "high
> > > level" operations to the software.
> >
> > Ok, I figured it would have been something to do with the SPI driver
> > just supporting these high level ops. But even with that background
> > it was hard to connect that to the "hardware sequencing". The help
> > text should be somewhat understandable to the user/distro people/whoever,
> > right? So I'd suggest to explain that a bit more in detail, or don't
> > use the term hardware sequencing at all. I'm not sure.
>
> I agree it should be made more understandable for the distro folks. At
> least add some explanation why it is OK to select this.
I agree with this.
> Mauro, can you do that in the next version?
Sure thing.
> > > Such as read, write, erase and so on
> > > but does not allow running the actual "low level" SPI-NOR opcodes.
> > > Software sequencing on the other hand allows running pretty much any
> > > opcode and this is what caused problems for certain Lenovo laptops few
> > > years back that then resulted adding DANGEROUS to the Kconfig.
> >
> > That information should go into the commit message.
>
> +1
Sorry about this, still learning :)

Thanks all for your comments and time.

Re: [PATCH 0/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

On Mon, Feb 06, 2023 at 03:31:42PM -0300, Mauro Lima wrote:
> Given that the PCI driver handles controllers that only work with
> hardware sequencing, we can remove the dangerous tag.
> This patch is the second part of Mika's suggestion [1].
> The first part was accepted in [2].

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/1] spi: intel: Remove DANGEROUS tag from pci driver

[Mark Brown]

On Mon, 06 Feb 2023 15:31:42 -0300, Mauro Lima wrote:
> Given that the PCI driver handles controllers that only work with
> hardware sequencing, we can remove the dangerous tag.
> This patch is the second part of Mika's suggestion [1].
> The first part was accepted in [2].
> 
> [1] https://lore.kernel.org/r/Y1d9glOgHsQlZe2L@black.fi.intel.com/
> [2] https://lore.kernel.org/linux-spi/20230201205455.550308-1-mauro.lima@eclypsium.com/
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: intel: Remove DANGEROUS tag from pci driver
      commit: 7db738b5fea4533fa217dfb05c506c15bd0964d9

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark