Date:   Thu, 9 Feb 2023 07:39:43 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mauro Lima <mauro.lima@eclypsium.com>
Cc:     broonie@kernel.org, michael@walle.cc, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] spi: intel: Update help text of PCI driver
Message-ID: <Y+SHH2+VqLoQ+6Ss@black.fi.intel.com>
References: <20230208175253.117714-1-mauro.lima@eclypsium.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230208175253.117714-1-mauro.lima@eclypsium.com>
Precedence: bulk

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 <mauro.lima@eclypsium.com>
> ---
>  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/Y1d9glOgHsQlZe2L@black.fi.intel.com/
>  [2] https://lore.kernel.org/linux-spi/20230201205455.550308-1-mauro.lima@eclypsium.com/
> 
>  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/20230206183143.75274-1-mauro.lima@eclypsium.com/
> 
>  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