spi: intel: Update help text of PCI driver

Commit Message

Mauro Lima Feb. 8, 2023, 5:52 p.m. UTC

Modern intel hardware uses controllers that work in hardware
sequencing mode. In this mode, the controller exposes a subset
of operations, like read, write and erase, making it easier
and less error-prone for use.
On the other hand, most of the controllers handled by the
platform driver use software sequencing that exposes the
entire set of opcodes i.e. include the low-level operations
available from the controller.

Since the PCI driver works with modern controllers, remove the
DANGEROUS tag from it.
Update the driver's help text and leave the DANGEROUS tag of
the platform driver.

Signed-off-by: Mauro Lima <[e-mail]>
---
 For the record of the base commit:

 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/[e-mail]/
 [2] https://lore.kernel.org/linux-spi/[e-mail]/

 This patch continues the work addressing the comments in the previous
 patch adding information about hardware and software sequencing.
 Discussion: https://lore.kernel.org/r/[e-mail]/

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


base-commit: 7db738b5fea4533fa217dfb05c506c15bd0964d9

Comments

Mika Westerberg Feb. 9, 2023, 5:39 a.m. UTC | #1

Hi,

On Wed, Feb 08, 2023 at 02:52:53PM -0300, Mauro Lima wrote:
> Modern intel hardware uses controllers that work in hardware
> sequencing mode. In this mode, the controller exposes a subset
> of operations, like read, write and erase, making it easier
> and less error-prone for use.
> On the other hand, most of the controllers handled by the
> platform driver use software sequencing that exposes the
> entire set of opcodes i.e. include the low-level operations
> available from the controller.
> 
> Since the PCI driver works with modern controllers, remove the
> DANGEROUS tag from it.
> Update the driver's help text and leave the DANGEROUS tag of
> the platform driver.

This is not done in this commit. You just update the help texts, right?

> Signed-off-by: Mauro Lima <[e-mail]>
> ---
>  For the record of the base commit:
> 
>  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/[e-mail]/
>  [2] https://lore.kernel.org/linux-spi/[e-mail]/
> 
>  This patch continues the work addressing the comments in the previous
>  patch adding information about hardware and software sequencing.
>  Discussion: https://lore.kernel.org/r/[e-mail]/
> 
>  drivers/spi/Kconfig | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 3a362c450cb6..9eb3c72d7cd8 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -454,13 +454,16 @@ config SPI_INTEL_PCI
>  	select SPI_INTEL
>  	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. This
> -	  driver only supports hardware sequencing mode. Using this
> -	  driver it is possible to upgrade BIOS directly from Linux.
> +	  master mode. This controller is used to hold BIOS and other
> +	  persistent settings. Controllers present in modern Intel hardware
> +	  only work in hardware sequencing mode, this means that the
> +	  controller exposes a subset of operations that makes it easier
> +	  and safer to use. Using this driver it is possible to upgrade BIOS

I would remove the "easier" part because from user perspective there is
really no difference.

> +	  directly from Linux.
>  
> -	  Say N here unless you know what you are doing. Overwriting the
> -	  SPI flash may render the system unbootable.
> +	  Say N here unless you want to overwrite the flash memory and

Putting it like this surely scares all distro folks from ever enabling
this ;-)

  "Say N here unless you want to overwrite the flash memory.."

At least to me this means that if you enable this option your flash
memory will be overwritten.

> +	  know what you are doing or you want to read the memory's content.
> +	  Overwriting the SPI flash may render the system unbootable.
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called spi-intel-pci.
> @@ -473,8 +476,10 @@ config SPI_INTEL_PLATFORM
>  	help
>  	  This enables platform support for the Intel PCH/PCU SPI
>  	  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
> +	  persistent settings. Most of these controllers work in
> +	  software sequencing mode, which means that the controller
> +	  exposes the full set of operations that supports, making it
> +	  more complex for use. Using this driver it is possible to

Here,

exposes the low level SPI-NOR opcodes to the software

I think is better. Also here too drop the "complex" as it looks similar
from user perspective.

>  	  upgrade BIOS directly from Linux.
>  
>  	  Say N here unless you know what you are doing. Overwriting the
> 
> base-commit: 7db738b5fea4533fa217dfb05c506c15bd0964d9
> -- 
> 2.39.1

Mauro Lima Feb. 9, 2023, 2:16 p.m. UTC | #2

Hi Mika,

On Thu, Feb 9, 2023 at 2:39 AM Mika Westerberg
<[e-mail]> wrote:
>
> Hi,
>
> On Wed, Feb 08, 2023 at 02:52:53PM -0300, Mauro Lima wrote:
> > Modern intel hardware uses controllers that work in hardware
> > sequencing mode. In this mode, the controller exposes a subset
> > of operations, like read, write and erase, making it easier
> > and less error-prone for use.
> > On the other hand, most of the controllers handled by the
> > platform driver use software sequencing that exposes the
> > entire set of opcodes i.e. include the low-level operations
> > available from the controller.
> >
> > Since the PCI driver works with modern controllers, remove the
> > DANGEROUS tag from it.
> > Update the driver's help text and leave the DANGEROUS tag of
> > the platform driver.
>
> This is not done in this commit. You just update the help texts, right?
Yes, you are right, I will update it.

> > Signed-off-by: Mauro Lima <[e-mail]>
> > ---
> >  For the record of the base commit:
> >
> >  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/[e-mail]/
> >  [2] https://lore.kernel.org/linux-spi/[e-mail]/
> >
> >  This patch continues the work addressing the comments in the previous
> >  patch adding information about hardware and software sequencing.
> >  Discussion: https://lore.kernel.org/r/[e-mail]/
> >
> >  drivers/spi/Kconfig | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 3a362c450cb6..9eb3c72d7cd8 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -454,13 +454,16 @@ config SPI_INTEL_PCI
> >       select SPI_INTEL
> >       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. This
> > -       driver only supports hardware sequencing mode. Using this
> > -       driver it is possible to upgrade BIOS directly from Linux.
> > +       master mode. This controller is used to hold BIOS and other
> > +       persistent settings. Controllers present in modern Intel hardware
> > +       only work in hardware sequencing mode, this means that the
> > +       controller exposes a subset of operations that makes it easier
> > +       and safer to use. Using this driver it is possible to upgrade BIOS
>
> I would remove the "easier" part because from user perspective there is
> really no difference.
Will do, thanks.

> > +       directly from Linux.
> >
> > -       Say N here unless you know what you are doing. Overwriting the
> > -       SPI flash may render the system unbootable.
> > +       Say N here unless you want to overwrite the flash memory and
>
> Putting it like this surely scares all distro folks from ever enabling
> this ;-)
>
>   "Say N here unless you want to overwrite the flash memory.."
>
> At least to me this means that if you enable this option your flash
> memory will be overwritten.
Do you have a suggestion for the "Say N here" text? Maybe remove it
since this is safe for use now? I found it difficult to rephrase it

> > +       know what you are doing or you want to read the memory's content.
> > +       Overwriting the SPI flash may render the system unbootable.
> >
> >         To compile this driver as a module, choose M here: the module
> >         will be called spi-intel-pci.
> > @@ -473,8 +476,10 @@ config SPI_INTEL_PLATFORM
> >       help
> >         This enables platform support for the Intel PCH/PCU SPI
> >         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
> > +       persistent settings. Most of these controllers work in
> > +       software sequencing mode, which means that the controller
> > +       exposes the full set of operations that supports, making it
> > +       more complex for use. Using this driver it is possible to
>
> Here,
>
> exposes the low level SPI-NOR opcodes to the software
Ok
>
> I think is better. Also here too drop the "complex" as it looks similar
> from user perspective.
I agree
> >         upgrade BIOS directly from Linux.
> >
> >         Say N here unless you know what you are doing. Overwriting the
> >
> > base-commit: 7db738b5fea4533fa217dfb05c506c15bd0964d9
> > --
> > 2.39.1

Thanks

Mika Westerberg Feb. 9, 2023, 3:57 p.m. UTC | #3

Hi,

On Thu, Feb 09, 2023 at 11:16:36AM -0300, Mauro Lima wrote:
> Hi Mika,
> >   "Say N here unless you want to overwrite the flash memory.."
> >
> > At least to me this means that if you enable this option your flash
> > memory will be overwritten.
> Do you have a suggestion for the "Say N here" text? Maybe remove it
> since this is safe for use now? I found it difficult to rephrase it

Agree, just remove it.

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 3a362c450cb6..9eb3c72d7cd8 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -454,13 +454,16 @@  config SPI_INTEL_PCI
 	select SPI_INTEL
 	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. This
-	  driver only supports hardware sequencing mode. Using this
-	  driver it is possible to upgrade BIOS directly from Linux.
+	  master mode. This controller is used to hold BIOS and other
+	  persistent settings. Controllers present in modern Intel hardware
+	  only work in hardware sequencing mode, this means that the
+	  controller exposes a subset of operations that makes it easier
+	  and safer to use. 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.
+	  Say N here unless you want to overwrite the flash memory and
+	  know what you are doing or you want to read the memory's content.
+	  Overwriting the SPI flash may render the system unbootable.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called spi-intel-pci.
@@ -473,8 +476,10 @@  config SPI_INTEL_PLATFORM
 	help
 	  This enables platform support for the Intel PCH/PCU SPI
 	  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
+	  persistent settings. Most of these controllers work in
+	  software sequencing mode, which means that the controller
+	  exposes the full set of operations that supports, making it
+	  more complex for use. Using this driver it is possible to
 	  upgrade BIOS directly from Linux.
 
 	  Say N here unless you know what you are doing. Overwriting the