diff mbox series

Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

Message ID 20200803134449.72502-1-daniel.gutson@eclypsium.com
State New
Delegated to: Ambarus Tudor
Headers show
Series Remove attempt by intel-spi-pci to turn the SPI flash chip writeable | expand

Commit Message

Daniel Gutson Aug. 3, 2020, 1:44 p.m. UTC
Currently, the intel-spi-pci driver tries to unconditionally set
the SPI chip writeable. After discussing in the LKML, the original
author decided that it was better  to remove the attempt.

Context, the intel-spi has a module argument that controls
whether the driver attempts to turn the SPI flash chip writeable.
The default value is FALSE (don't try to make it writeable).
However, this flag applies only for a number of devices, coming from the
platform driver, whereas the devices detected through the PCI driver
(intel-spi-pci) are not subject to this check since the configuration
takes place in intel-spi-pci which doesn't have an argument.

This patch removes the code that attempts to turn the SPI chip writeable.

Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
---
 drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Arnd Bergmann Aug. 3, 2020, 1:54 p.m. UTC | #1
On Mon, Aug 3, 2020 at 3:45 PM Daniel Gutson
<daniel.gutson@eclypsium.com> wrote:

> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.

This part of the description sounds wrong: the current behavior is that
the BIOS setting is ignored for PCI devices and it only uses the module
parameter, the same way as it does for the platform driver.

With your patch, both the BIOS setting and the module parameter
have to explicitly allow writing on PCI devices, while at least for Bay
Trail platform devices the BIOS write protection is still ignored.

It sounds like this is what you want, but you should update the description
accordingly.

      Arnd
Daniel Gutson Aug. 3, 2020, 2:21 p.m. UTC | #2
On Mon, Aug 3, 2020 at 10:55 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Aug 3, 2020 at 3:45 PM Daniel Gutson
> <daniel.gutson@eclypsium.com> wrote:
>
> > However, this flag applies only for a number of devices, coming from the
> > platform driver, whereas the devices detected through the PCI driver
> > (intel-spi-pci) are not subject to this check since the configuration
> > takes place in intel-spi-pci which doesn't have an argument.
>
> This part of the description sounds wrong: the current behavior is that
> the BIOS setting is ignored for PCI devices and it only uses the module
> parameter, the same way as it does for the platform driver.

Actually, the BIOS setting is not ignored, since it is not bypassable.
There is a lock in the BIOS setting, that, if enabled no matter what the
driver does, it will be still read only. However, if that lock is not set,
the SPI chip will be writable because of the driver. That's why
I say 'attempts'.
The intel-spi-pci driver doesn't have a module parameter, and that's
why it unconditionally attempts to turn the chip writable (it will succeed
if it is not locked).
What I did was just left the intel-spi-pci driver without any module parameter,
as it currently is, but removed the part where it attempts to turn the chip
writable (just in case the BIOS is not locked).

>
> With your patch, both the BIOS setting and the module parameter
> have to explicitly allow writing on PCI devices, while at least for Bay
> Trail platform devices the BIOS write protection is still ignored.
>
> It sounds like this is what you want, but you should update the description
> accordingly.
>
>       Arnd
Mika Westerberg Aug. 4, 2020, 11:52 a.m. UTC | #3
Hi,

On Mon, Aug 03, 2020 at 10:44:49AM -0300, Daniel Gutson wrote:
> Currently, the intel-spi-pci driver tries to unconditionally set
> the SPI chip writeable. After discussing in the LKML, the original
> author decided that it was better  to remove the attempt.
> 
> Context, the intel-spi has a module argument that controls
> whether the driver attempts to turn the SPI flash chip writeable.
> The default value is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.
> 
> This patch removes the code that attempts to turn the SPI chip writeable.

I think you should make the $subject to follow the convention used in
the SPI-NOR subsystem. You can see it running following command:

  $ git log --oneline drivers/mtd/spi-nor/controllers/intel-spi-pci.c

In this case I think it should be:

  mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable

or something like that.

The patch itself looks good, one minor comment below.

> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 81329f680bec..d721ba4e8b41 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -41,13 +41,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>  	if (!info)
>  		return -ENOMEM;
>  
> -	/* Try to make the chip read/write */
>  	pci_read_config_dword(pdev, BCR, &bcr);
> -	if (!(bcr & BCR_WPD)) {
> -		bcr |= BCR_WPD;
> -		pci_write_config_dword(pdev, BCR, bcr);
> -		pci_read_config_dword(pdev, BCR, &bcr);
> -	}
>  	info->writeable = !!(bcr & BCR_WPD);

Perhaps we should log this in debug level (dev_dbg()) so when debugging
possible issues we can see that the chip is write protected by the BIOS.

>  
>  	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
> -- 
> 2.25.1
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
index 81329f680bec..d721ba4e8b41 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
@@ -41,13 +41,7 @@  static int intel_spi_pci_probe(struct pci_dev *pdev,
 	if (!info)
 		return -ENOMEM;
 
-	/* Try to make the chip read/write */
 	pci_read_config_dword(pdev, BCR, &bcr);
-	if (!(bcr & BCR_WPD)) {
-		bcr |= BCR_WPD;
-		pci_write_config_dword(pdev, BCR, bcr);
-		pci_read_config_dword(pdev, BCR, &bcr);
-	}
 	info->writeable = !!(bcr & BCR_WPD);
 
 	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);