| Message ID | 20200724212853.11601-1-daniel.gutson@eclypsium.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Module argument to control whether intel-spi-pci attempts to turn the SPI flash chip writeable | expand |
On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote: > Currently, 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. > > That's why I propose this patch to add such argument to intel-spi-pci, > so the user can control whether the driver tries to make the chip > writeable or not, being the default FALSE as is the argument of > intel-spi. > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> > --- > drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 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..77e57450f166 100644 > --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = { > .type = INTEL_SPI_CNL, > }; > > +static bool writeable; > +module_param(writeable, bool, 0); > +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)"); Ick, this isn't the 1990's, please do not add new module parameters, they are a major pain to work with and only work on a global basis, not on a per-device basis. No user will remember how to use this, as it isn't documented anywhere either. Can you make this a sysfs attribute or something, or better yet, make it "just work" depending on the device type? thanks, greg k-h
On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote: > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman < > gregkh@linuxfoundation.org> escribió: > > > On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote: > > > Currently, 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. > > > > > > That's why I propose this patch to add such argument to intel-spi-pci, > > > so the user can control whether the driver tries to make the chip > > > writeable or not, being the default FALSE as is the argument of > > > intel-spi. > > > > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> > > > --- > > > drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++----- > > > 1 file changed, 11 insertions(+), 5 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..77e57450f166 100644 > > > --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > > > @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = { > > > .type = INTEL_SPI_CNL, > > > }; > > > > > > +static bool writeable; > > > +module_param(writeable, bool, 0); > > > +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip > > (default=0)"); > > > > Ick, this isn't the 1990's, please do not add new module parameters, > > they are a major pain to work with and only work on a global basis, not > > on a per-device basis. > > > > No user will remember how to use this, as it isn't documented anywhere > > either. Can you make this a sysfs attribute or something, or better > > yet, make it "just work" depending on the device type? > > > > 1) I just did the same that intel-spi.c does. No need to copy bad examples :) > You need to understand that there's a set of DIDs coming from the > lpc_ich (and then the platform) driver, and another set from > intel-spi-pci. The first set is subject to the check, the second > doesn't. So there is no "just work" as I understand it. I have no idea what "DID" is here, sorry. But why do you want to write it? > 2) this is an initialization argument, I could make it always NOT attempt > to make the chip writable, and a system attribute to turn it writable > post-initialization but the behavior is not the same. In any ways, as I > mentioned before, some DIDs will be covered by the existing argument of > intel-spi and other DIDs by this sysfs attribute, becoming IMHO > inconsistent from the user POV. What sysfs attribute? This is a module parameter :( totally confused, greg k-h
On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote: > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: >> >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote: >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman < >> > gregkh@linuxfoundation.org> escribió: >> > >> > >> > 1) I just did the same that intel-spi.c does. >> >> No need to copy bad examples :) > > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration? I'd say you'd generally want this to be a per-instance setting, which could be a sysfs attribute of the physical device, or an ioctl for an existing user space abstraction. In the changelog, you should also explain what this is used for. Do you actually want to write to a device that is marked read-only, or are you just trying to make the interface more consistent between the two drivers? Arnd
On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > >> > >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote: > >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman < > >> > gregkh@linuxfoundation.org> escribió: > >> > > >> > > >> > 1) I just did the same that intel-spi.c does. > >> > >> No need to copy bad examples :) > > > > > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration? > > I'd say you'd generally want this to be a per-instance setting, which > could be a sysfs attribute of the physical device, or an ioctl for an > existing user space abstraction. But still, they are not equivalent. The initial configuration remains constant for the rest of the life of the driver, whereas the sysfs attribute is set after the initialization and can be re-set over time. I'm not asking module parameters "to come back" if they are now considered obsolete, I'm just trying to understand. > > In the changelog, you should also explain what this is used for. Do > you actually want to write to a device that is marked read-only, or > are you just trying to make the interface more consistent between the > two drivers? The device can either be locked or unlocked. If it is unlocked, it can be set writable depending on the module argument in intel-spi, or straight writable in intel-spi-pci. Which is dangerous. I wanted to make, as you say, the interface consistent. Exposing an interface to the user (via a sysfs attribute) to try to make the driver writable is definitively a bad idea. I'd rather do nothing (no module arg) rather than open that here-be-dragons door. > > Arnd
On Mon, Jul 27, 2020 at 12:31 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > >> > > >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote: > > >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman < > > >> > gregkh@linuxfoundation.org> escribió: > > >> > > > >> > > > >> > 1) I just did the same that intel-spi.c does. > > >> > > >> No need to copy bad examples :) > > > > > > > > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration? > > > > I'd say you'd generally want this to be a per-instance setting, which > > could be a sysfs attribute of the physical device, or an ioctl for an > > existing user space abstraction. > > But still, they are not equivalent. The initial configuration remains > constant for the rest of the life of the driver, whereas the > sysfs attribute is set after the initialization and can be re-set over > time. I'm not asking module parameters "to come back" if > they are now considered obsolete, I'm just trying to understand. > > > > > In the changelog, you should also explain what this is used for. Do > > you actually want to write to a device that is marked read-only, or > > are you just trying to make the interface more consistent between the > > two drivers? > > The device can either be locked or unlocked. If it is unlocked, it can > be set writable depending on the module > argument in intel-spi, or straight writable in intel-spi-pci. Which is > dangerous. > I wanted to make, as you say, the interface consistent. > Exposing an interface to the user (via a sysfs attribute) to try to > make the driver writable is definitively a bad idea. > I'd rather do nothing (no module arg) rather than open that > here-be-dragons door. ping. This is a real existing problem that I'm trying to address. There's currently no way to prevent the kernel to try to turn the SPI flash chip writable for the device IDs handled by intel-spi-pci. And allowing userspace to switch it between writable/non-writable is not an option. What other mechanism can I use other than the module parameter, so - not accessible through user space - ideally set once, ideally at initialization time Thanks, Daniel. > > > > > Arnd > > > > -- > Daniel Gutson > Argentina Site Director > Enginieering Director > Eclypsium > > Below The Surface: Get the latest threat research and insights on > firmware and supply chain threats from the research team at Eclypsium. > https://eclypsium.com/research/#threatreport
On Wed, Jul 29, 2020 at 05:54:35PM -0300, Daniel Gutson wrote: > On Mon, Jul 27, 2020 at 12:31 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > > > On Mon, Jul 27, 2020 at 12:15 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Mon, Jul 27, 2020 at 5:05 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > > > On Sun, Jul 26, 2020 at 4:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > >> > > > >> On Sat, Jul 25, 2020 at 02:20:03PM -0300, Daniel Gutson wrote: > > > >> > El sáb., 25 jul. 2020 2:56 a. m., Greg Kroah-Hartman < > > > >> > gregkh@linuxfoundation.org> escribió: > > > >> > > > > >> > > > > >> > 1) I just did the same that intel-spi.c does. > > > >> > > > >> No need to copy bad examples :) > > > > > > > > > > > > Didn't know it was a bad example. What's is the current modern mechanism that replaces initialization-time configuration? > > > > > > I'd say you'd generally want this to be a per-instance setting, which > > > could be a sysfs attribute of the physical device, or an ioctl for an > > > existing user space abstraction. > > > > But still, they are not equivalent. The initial configuration remains > > constant for the rest of the life of the driver, whereas the > > sysfs attribute is set after the initialization and can be re-set over > > time. I'm not asking module parameters "to come back" if > > they are now considered obsolete, I'm just trying to understand. > > > > > > > > In the changelog, you should also explain what this is used for. Do > > > you actually want to write to a device that is marked read-only, or > > > are you just trying to make the interface more consistent between the > > > two drivers? > > > > The device can either be locked or unlocked. If it is unlocked, it can > > be set writable depending on the module > > argument in intel-spi, or straight writable in intel-spi-pci. Which is > > dangerous. > > I wanted to make, as you say, the interface consistent. > > Exposing an interface to the user (via a sysfs attribute) to try to > > make the driver writable is definitively a bad idea. > > I'd rather do nothing (no module arg) rather than open that > > here-be-dragons door. > > ping. > This is a real existing problem that I'm trying to address. > There's currently no way to prevent the kernel to try to turn > the SPI flash chip writable for the device IDs handled by > intel-spi-pci. And allowing userspace to switch it between > writable/non-writable is not an option. > What other mechanism can I use other than the module parameter, Again, module parameters are working on a per-chunk-of-code basis, while you want to work on a per-device basis, which is why you should not use a module parameter. good luck! greg k-h
On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote: > El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió: >> >> Again, module parameters are working on a per-chunk-of-code basis, while >> you want to work on a per-device basis, > > > I think there is a misunderstanding. What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine. > I assume that the one that doesn't understand is me. > I looked at the source code again and found that the existing module parameter applies to both the platform and pci device front-ends, both of which go through /* Prevent writes if not explicitly enabled */ if (!ispi->writeable || !writeable) ispi->nor.mtd.flags &= ~MTD_WRITEABLE; Setting the PCI device writable in hardware makes it possible to actually write to it *only* if the module parameter is also set to '1'. One might disagree with that design, but I don't think your patch would make it any better, it just means one would have to set two module parameters instead of one. Arnd
On Thu, Jul 30, 2020 at 11:09 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió: > >> > >> Again, module parameters are working on a per-chunk-of-code basis, while > >> you want to work on a per-device basis, > > > > > > I think there is a misunderstanding. What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine. > > I assume that the one that doesn't understand is me. > > > > I looked at the source code again and found that the existing module > parameter applies to both the platform and pci device front-ends, both > of which go through > > /* Prevent writes if not explicitly enabled */ > if (!ispi->writeable || !writeable) > ispi->nor.mtd.flags &= ~MTD_WRITEABLE; > I think you missed https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L44 /* 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); } in the probe function, and is executed always and unconditionally. /* 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); } > Setting the PCI device writable in hardware makes it possible to > actually write to it *only* if the module parameter is also set to '1'. > One might disagree with that design, but I don't think your patch > would make it any better, it just means one would have to set > two module parameters instead of one. > > Arnd
On Thu, Jul 30, 2020 at 11:18 AM Daniel Gutson <daniel@eclypsium.com> wrote: > > On Thu, Jul 30, 2020 at 11:09 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Thu, Jul 30, 2020 at 2:21 PM Daniel Gutson <daniel@eclypsium.com> wrote: > > > El jue., 30 jul. 2020 2:31 a. m., Greg Kroah-Hartman <gregkh@linuxfoundation.org> escribió: > > >> > > >> Again, module parameters are working on a per-chunk-of-code basis, while > > >> you want to work on a per-device basis, > > > > > > > > > I think there is a misunderstanding. What I want is to control (turn on or off) is a very specific code snippet that provides the "functionality" of trying to turn the chip writable. The rest of the device driver is fine. > > > I assume that the one that doesn't understand is me. > > > > > > > I looked at the source code again and found that the existing module > > parameter applies to both the platform and pci device front-ends, both > > of which go through > > > > /* Prevent writes if not explicitly enabled */ > > if (!ispi->writeable || !writeable) > > ispi->nor.mtd.flags &= ~MTD_WRITEABLE; > > > > I think you missed > https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/controllers/intel-spi-pci.c#L44 > > /* 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); > } > > in the probe function, and is executed always and unconditionally. To clarify, this is executed before intel-spi code. > > /* 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); > } > > > Setting the PCI device writable in hardware makes it possible to > > actually write to it *only* if the module parameter is also set to '1'. > > One might disagree with that design, but I don't think your patch > > would make it any better, it just means one would have to set > > two module parameters instead of one. > > > > Arnd > > > > -- > Daniel Gutson > Argentina Site Director > Enginieering Director > Eclypsium > > Below The Surface: Get the latest threat research and insights on > firmware and supply chain threats from the research team at Eclypsium. > https://eclypsium.com/research/#threatreport
Hi, Sorry for the delay, I was on vacation. On Fri, Jul 24, 2020 at 06:28:53PM -0300, Daniel Gutson wrote: > Currently, 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. > > That's why I propose this patch to add such argument to intel-spi-pci, > so the user can control whether the driver tries to make the chip > writeable or not, being the default FALSE as is the argument of > intel-spi. > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> > --- > drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 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..77e57450f166 100644 > --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = { > .type = INTEL_SPI_CNL, > }; > > +static bool writeable; > +module_param(writeable, bool, 0); > +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)"); I think instead of this we should simply make it so that the driver never tries to make the chip writable. > + > static int intel_spi_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *id) > { > @@ -41,12 +45,14 @@ 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); > + if (writeable) { > + /* 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); So here we just read the BCR register and then set info->writeable based on its value. Then it is up to the BIOS to enable this if it allows writing the flash chip from the OS side.
On Mon, 3 Aug 2020 at 10:57, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > I think instead of this we should simply make it so that the driver > never tries to make the chip writable. I think this is a good idea, but I wasn't sure if it was an acceptable behaviour change. Should the driver still try to set BCR_WPD when writing an image (i.e. defer the setting of write enable until later), or just not set the BCR register at all? I think your last comment was the latter, but wanted to check. Richard.
On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > I think instead of this we should simply make it so that the driver > > never tries to make the chip writable. > > I think this is a good idea, but I wasn't sure if it was an acceptable > behaviour change. Should the driver still try to set BCR_WPD when > writing an image (i.e. defer the setting of write enable until later), > or just not set the BCR register at all? I think your last comment was > the latter, but wanted to check. I would say not set it at all. I think it was (my) mistake to set it in the first place.
On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > I think instead of this we should simply make it so that the driver > > > never tries to make the chip writable. > > > > I think this is a good idea, but I wasn't sure if it was an acceptable > > behaviour change. Should the driver still try to set BCR_WPD when > > writing an image (i.e. defer the setting of write enable until later), > > or just not set the BCR register at all? I think your last comment was > > the latter, but wanted to check. > > I would say not set it at all. I think it was (my) mistake to set it in > the first place. Do you want me to remove the module parameter from intel-spi too and do the same?
On Mon, Aug 03, 2020 at 09:58:23AM -0300, Daniel Gutson wrote: > On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > > > <mika.westerberg@linux.intel.com> wrote: > > > > I think instead of this we should simply make it so that the driver > > > > never tries to make the chip writable. > > > > > > I think this is a good idea, but I wasn't sure if it was an acceptable > > > behaviour change. Should the driver still try to set BCR_WPD when > > > writing an image (i.e. defer the setting of write enable until later), > > > or just not set the BCR register at all? I think your last comment was > > > the latter, but wanted to check. > > > > I would say not set it at all. I think it was (my) mistake to set it in > > the first place. > > Do you want me to remove the module parameter from intel-spi too and > do the same? No, I think that should still be left there. Then by default it is read-only and you can only enable writing if the BIOS allows it and that the user actually requested it.
On Mon, Aug 3, 2020 at 10:06 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Mon, Aug 03, 2020 at 09:58:23AM -0300, Daniel Gutson wrote: > > On Mon, Aug 3, 2020 at 7:27 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > > On Mon, Aug 03, 2020 at 11:18:12AM +0100, Richard Hughes wrote: > > > > On Mon, 3 Aug 2020 at 10:57, Mika Westerberg > > > > <mika.westerberg@linux.intel.com> wrote: > > > > > I think instead of this we should simply make it so that the driver > > > > > never tries to make the chip writable. > > > > > > > > I think this is a good idea, but I wasn't sure if it was an acceptable > > > > behaviour change. Should the driver still try to set BCR_WPD when > > > > writing an image (i.e. defer the setting of write enable until later), > > > > or just not set the BCR register at all? I think your last comment was > > > > the latter, but wanted to check. > > > > > > I would say not set it at all. I think it was (my) mistake to set it in > > > the first place. > > > > Do you want me to remove the module parameter from intel-spi too and > > do the same? > > No, I think that should still be left there. Then by default it is > read-only and you can only enable writing if the BIOS allows it and that > the user actually requested it. OK. Patch heading your way in 1h.
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c index 81329f680bec..77e57450f166 100644 --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c @@ -24,6 +24,10 @@ static const struct intel_spi_boardinfo cnl_info = { .type = INTEL_SPI_CNL, }; +static bool writeable; +module_param(writeable, bool, 0); +MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)"); + static int intel_spi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -41,12 +45,14 @@ 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); + if (writeable) { + /* 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);
Currently, 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. That's why I propose this patch to add such argument to intel-spi-pci, so the user can control whether the driver tries to make the chip writeable or not, being the default FALSE as is the argument of intel-spi. Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> --- drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)