| Message ID | 20200803134449.72502-1-daniel.gutson@eclypsium.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Remove attempt by intel-spi-pci to turn the SPI flash chip writeable | expand |
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
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
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 --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);
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(-)