diff mbox series

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

Message ID 20230206183143.75274-2-mauro.lima@eclypsium.com (mailing list archive)
State Accepted
Commit 7db738b5fea4533fa217dfb05c506c15bd0964d9
Headers show
Series spi: intel: Remove DANGEROUS tag from pci driver | expand

Commit Message

Mauro Lima Feb. 6, 2023, 6:31 p.m. UTC
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(-)

Comments

Mika Westerberg Feb. 7, 2023, 5:56 a.m. UTC | #1
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>
Michael Walle Feb. 7, 2023, 1:52 p.m. UTC | #2
> 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
Mika Westerberg Feb. 7, 2023, 2:03 p.m. UTC | #3
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.
Michael Walle Feb. 7, 2023, 2:11 p.m. UTC | #4
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
Mika Westerberg Feb. 7, 2023, 2:45 p.m. UTC | #5
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
Mauro Lima Feb. 7, 2023, 8:44 p.m. UTC | #6
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.
diff mbox series

Patch

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.