[PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Daniel Gutson]

This patch separates the writing part of the intel-spi drivers
so the 'dangerous' part can be set/unset independently.
This way, the kernel can be configured to include the reading
parts of the driver which can be used without
the dangerous write operations that can turn the system
unbootable.

Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
---
 drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
 drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
index 5c0e0ec2e6d1..491c755fea49 100644
--- a/drivers/mtd/spi-nor/controllers/Kconfig
+++ b/drivers/mtd/spi-nor/controllers/Kconfig
@@ -31,34 +31,41 @@ config SPI_INTEL_SPI
 	tristate
 
 config SPI_INTEL_SPI_PCI
-	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
+	tristate "Intel PCH/PCU SPI flash PCI driver"
 	depends on X86 && PCI
 	select SPI_INTEL_SPI
 	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. 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.
+	  This enables read only 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.
+	  Using this driver it is possible to read the SPI chip directly
+	  from Linux.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel-spi-pci.
 
 config SPI_INTEL_SPI_PLATFORM
-	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
+	tristate "Intel PCH/PCU SPI flash platform driver"
 	depends on X86
 	select SPI_INTEL_SPI
 	help
-	  This enables platform support for the Intel PCH/PCU SPI
+	  This enables read only platform 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. Using this driver it is possible to upgrade BIOS
-	  directly from Linux.
+	  Intel hardware and is used to hold BIOS and other persistent settings.
+	  Using this driver it is possible to read the SPI chip directly
+	  from Linux.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called intel-spi-pci.
+
+config SPI_INTEL_SPI_WRITE
+	bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
+	depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
+	help
+	  This enables full read/write support for the Intel PCH/PCU SPI
+	  controller.
+	  Using this option it may be 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.
-
-	  To compile this driver as a module, choose M here: the module
-	  will be called intel-spi-platform.
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
index b54a56a68100..8d8053395c3d 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
 	return 0;
 }
 
+#ifdef CONFIG_SPI_INTEL_SPI_WRITE
 /* Writes max INTEL_SPI_FIFO_SZ bytes to the device fifo */
 static int intel_spi_write_block(struct intel_spi *ispi, const void *buf,
 				 size_t size)
@@ -286,6 +287,7 @@ static int intel_spi_write_block(struct intel_spi *ispi, const void *buf,
 
 	return 0;
 }
+#endif /* CONFIG_SPI_INTEL_SPI_WRITE */
 
 static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
 {
@@ -576,6 +578,7 @@ static int intel_spi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
 	return intel_spi_read_block(ispi, buf, len);
 }
 
+#ifdef CONFIG_SPI_INTEL_SPI_WRITE
 static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
 			       size_t len)
 {
@@ -633,6 +636,7 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8 opcode, const u8 *buf,
 					  OPTYPE_WRITE_NO_ADDR);
 	return intel_spi_hw_cycle(ispi, opcode, len);
 }
+#endif /* CONFIG_SPI_INTEL_SPI_WRITE */
 
 static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 			      u_char *read_buf)
@@ -705,6 +709,7 @@ static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
 	return retlen;
 }
 
+#ifdef CONFIG_SPI_INTEL_SPI_WRITE
 static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
 			       const u_char *write_buf)
 {
@@ -829,6 +834,7 @@ static int intel_spi_erase(struct spi_nor *nor, loff_t offs)
 
 	return 0;
 }
+#endif /* CONFIG_SPI_INTEL_SPI_WRITE */
 
 static bool intel_spi_is_protected(const struct intel_spi *ispi,
 				   unsigned int base, unsigned int limit)
@@ -897,10 +903,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
 
 static const struct spi_nor_controller_ops intel_spi_controller_ops = {
 	.read_reg = intel_spi_read_reg,
-	.write_reg = intel_spi_write_reg,
 	.read = intel_spi_read,
+#ifdef CONFIG_SPI_INTEL_SPI_WRITE
+	.write_reg = intel_spi_write_reg,
 	.write = intel_spi_write,
-	.erase = intel_spi_erase,
+	.erase = intel_spi_erase
+#endif
 };
 
 struct intel_spi *intel_spi_probe(struct device *dev,
-- 
2.25.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Greg Kroah-Hartman]

On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> This patch separates the writing part of the intel-spi drivers
> so the 'dangerous' part can be set/unset independently.
> This way, the kernel can be configured to include the reading
> parts of the driver which can be used without
> the dangerous write operations that can turn the system
> unbootable.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
>  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
>  2 files changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> index 5c0e0ec2e6d1..491c755fea49 100644
> --- a/drivers/mtd/spi-nor/controllers/Kconfig
> +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
>  	tristate
>  
>  config SPI_INTEL_SPI_PCI
> -	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> +	tristate "Intel PCH/PCU SPI flash PCI driver"
>  	depends on X86 && PCI
>  	select SPI_INTEL_SPI
>  	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. 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.
> +	  This enables read only 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.
> +	  Using this driver it is possible to read the SPI chip directly
> +	  from Linux.
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called intel-spi-pci.
>  
>  config SPI_INTEL_SPI_PLATFORM
> -	tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> +	tristate "Intel PCH/PCU SPI flash platform driver"
>  	depends on X86
>  	select SPI_INTEL_SPI
>  	help
> -	  This enables platform support for the Intel PCH/PCU SPI
> +	  This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> -	  directly from Linux.
> +	  Intel hardware and is used to hold BIOS and other persistent settings.
> +	  Using this driver it is possible to read the SPI chip directly
> +	  from Linux.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called intel-spi-pci.
> +
> +config SPI_INTEL_SPI_WRITE
> +	bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> +	depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> +	help
> +	  This enables full read/write support for the Intel PCH/PCU SPI
> +	  controller.
> +	  Using this option it may be 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.
> -
> -	  To compile this driver as a module, choose M here: the module
> -	  will be called intel-spi-platform.
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> index b54a56a68100..8d8053395c3d 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SPI_INTEL_SPI_WRITE

<snip>

Please do not add #ifdef to .c files, that's not the proper kernel
coding style at all, and just makes maintaining this file much much
harder over time.

Split things out into two different files if you really need to do this.

thanks,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Daniel Gutson]

On Thu, Oct 29, 2020 at 2:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> > This patch separates the writing part of the intel-spi drivers
> > so the 'dangerous' part can be set/unset independently.
> > This way, the kernel can be configured to include the reading
> > parts of the driver which can be used without
> > the dangerous write operations that can turn the system
> > unbootable.
> >
> > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > ---
> >  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
> >  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
> >  2 files changed, 33 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> > index 5c0e0ec2e6d1..491c755fea49 100644
> > --- a/drivers/mtd/spi-nor/controllers/Kconfig
> > +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> > @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
> >       tristate
> >
> >  config SPI_INTEL_SPI_PCI
> > -     tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > +     tristate "Intel PCH/PCU SPI flash PCI driver"
> >       depends on X86 && PCI
> >       select SPI_INTEL_SPI
> >       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. 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.
> > +       This enables read only 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.
> > +       Using this driver it is possible to read the SPI chip directly
> > +       from Linux.
> >
> >         To compile this driver as a module, choose M here: the module
> >         will be called intel-spi-pci.
> >
> >  config SPI_INTEL_SPI_PLATFORM
> > -     tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > +     tristate "Intel PCH/PCU SPI flash platform driver"
> >       depends on X86
> >       select SPI_INTEL_SPI
> >       help
> > -       This enables platform support for the Intel PCH/PCU SPI
> > +       This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> > -       directly from Linux.
> > +       Intel hardware and is used to hold BIOS and other persistent settings.
> > +       Using this driver it is possible to read the SPI chip directly
> > +       from Linux.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called intel-spi-pci.
> > +
> > +config SPI_INTEL_SPI_WRITE
> > +     bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> > +     depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> > +     help
> > +       This enables full read/write support for the Intel PCH/PCU SPI
> > +       controller.
> > +       Using this option it may be 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.
> > -
> > -       To compile this driver as a module, choose M here: the module
> > -       will be called intel-spi-platform.
> > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > index b54a56a68100..8d8053395c3d 100644
> > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_SPI_INTEL_SPI_WRITE
>
> <snip>
>
> Please do not add #ifdef to .c files, that's not the proper kernel
> coding style at all, and just makes maintaining this file much much
> harder over time.
>
> Split things out into two different files if you really need to do this.

What about the static functions that I'll need to turn non-static and
in a header file?
I mean, the functions that the functions in the new file will have to call.
Should I do that, turn static functions into non-static and declared
in a header file?


>
> thanks,
>
> greg k-h



-- 


Daniel Gutson
Engineering Director
Eclypsium, Inc.


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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Greg Kroah-Hartman]

On Thu, Oct 29, 2020 at 12:39:08PM -0300, Daniel Gutson wrote:
> On Thu, Oct 29, 2020 at 2:40 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> > > This patch separates the writing part of the intel-spi drivers
> > > so the 'dangerous' part can be set/unset independently.
> > > This way, the kernel can be configured to include the reading
> > > parts of the driver which can be used without
> > > the dangerous write operations that can turn the system
> > > unbootable.
> > >
> > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > ---
> > >  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
> > >  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
> > >  2 files changed, 33 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> > > index 5c0e0ec2e6d1..491c755fea49 100644
> > > --- a/drivers/mtd/spi-nor/controllers/Kconfig
> > > +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> > > @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
> > >       tristate
> > >
> > >  config SPI_INTEL_SPI_PCI
> > > -     tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > > +     tristate "Intel PCH/PCU SPI flash PCI driver"
> > >       depends on X86 && PCI
> > >       select SPI_INTEL_SPI
> > >       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. 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.
> > > +       This enables read only 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.
> > > +       Using this driver it is possible to read the SPI chip directly
> > > +       from Linux.
> > >
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called intel-spi-pci.
> > >
> > >  config SPI_INTEL_SPI_PLATFORM
> > > -     tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > > +     tristate "Intel PCH/PCU SPI flash platform driver"
> > >       depends on X86
> > >       select SPI_INTEL_SPI
> > >       help
> > > -       This enables platform support for the Intel PCH/PCU SPI
> > > +       This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> > > -       directly from Linux.
> > > +       Intel hardware and is used to hold BIOS and other persistent settings.
> > > +       Using this driver it is possible to read the SPI chip directly
> > > +       from Linux.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called intel-spi-pci.
> > > +
> > > +config SPI_INTEL_SPI_WRITE
> > > +     bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> > > +     depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> > > +     help
> > > +       This enables full read/write support for the Intel PCH/PCU SPI
> > > +       controller.
> > > +       Using this option it may be 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.
> > > -
> > > -       To compile this driver as a module, choose M here: the module
> > > -       will be called intel-spi-platform.
> > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > index b54a56a68100..8d8053395c3d 100644
> > > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> > >       return 0;
> > >  }
> > >
> > > +#ifdef CONFIG_SPI_INTEL_SPI_WRITE
> >
> > <snip>
> >
> > Please do not add #ifdef to .c files, that's not the proper kernel
> > coding style at all, and just makes maintaining this file much much
> > harder over time.
> >
> > Split things out into two different files if you really need to do this.
> 
> What about the static functions that I'll need to turn non-static and
> in a header file?
> I mean, the functions that the functions in the new file will have to call.
> Should I do that, turn static functions into non-static and declared
> in a header file?

No idea, but again, no #ifdefs in .c files like this, that is not the
proper kernel coding style as it is not maintainable for the lifespan
that we have to maintain code.

thanks,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Daniel Gutson]

On Thu, Oct 29, 2020 at 4:14 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 29, 2020 at 12:39:08PM -0300, Daniel Gutson wrote:
> > On Thu, Oct 29, 2020 at 2:40 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> > > > This patch separates the writing part of the intel-spi drivers
> > > > so the 'dangerous' part can be set/unset independently.
> > > > This way, the kernel can be configured to include the reading
> > > > parts of the driver which can be used without
> > > > the dangerous write operations that can turn the system
> > > > unbootable.
> > > >
> > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > > ---
> > > >  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
> > > >  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
> > > >  2 files changed, 33 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > index 5c0e0ec2e6d1..491c755fea49 100644
> > > > --- a/drivers/mtd/spi-nor/controllers/Kconfig
> > > > +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
> > > >       tristate
> > > >
> > > >  config SPI_INTEL_SPI_PCI
> > > > -     tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > > > +     tristate "Intel PCH/PCU SPI flash PCI driver"
> > > >       depends on X86 && PCI
> > > >       select SPI_INTEL_SPI
> > > >       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. 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.
> > > > +       This enables read only 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.
> > > > +       Using this driver it is possible to read the SPI chip directly
> > > > +       from Linux.
> > > >
> > > >         To compile this driver as a module, choose M here: the module
> > > >         will be called intel-spi-pci.
> > > >
> > > >  config SPI_INTEL_SPI_PLATFORM
> > > > -     tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > > > +     tristate "Intel PCH/PCU SPI flash platform driver"
> > > >       depends on X86
> > > >       select SPI_INTEL_SPI
> > > >       help
> > > > -       This enables platform support for the Intel PCH/PCU SPI
> > > > +       This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> > > > -       directly from Linux.
> > > > +       Intel hardware and is used to hold BIOS and other persistent settings.
> > > > +       Using this driver it is possible to read the SPI chip directly
> > > > +       from Linux.
> > > > +
> > > > +       To compile this driver as a module, choose M here: the module
> > > > +       will be called intel-spi-pci.
> > > > +
> > > > +config SPI_INTEL_SPI_WRITE
> > > > +     bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> > > > +     depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> > > > +     help
> > > > +       This enables full read/write support for the Intel PCH/PCU SPI
> > > > +       controller.
> > > > +       Using this option it may be 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.
> > > > -
> > > > -       To compile this driver as a module, choose M here: the module
> > > > -       will be called intel-spi-platform.
> > > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > index b54a56a68100..8d8053395c3d 100644
> > > > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> > > >       return 0;
> > > >  }
> > > >
> > > > +#ifdef CONFIG_SPI_INTEL_SPI_WRITE
> > >
> > > <snip>
> > >
> > > Please do not add #ifdef to .c files, that's not the proper kernel
> > > coding style at all, and just makes maintaining this file much much
> > > harder over time.
> > >
> > > Split things out into two different files if you really need to do this.
> >
> > What about the static functions that I'll need to turn non-static and
> > in a header file?
> > I mean, the functions that the functions in the new file will have to call.
> > Should I do that, turn static functions into non-static and declared
> > in a header file?
>
> No idea, but again, no #ifdefs in .c files like this, that is not the
> proper kernel coding style as it is not maintainable for the lifespan
> that we have to maintain code.

Is it acceptable to leave static functions unused and let the optimizer
remove them as DCE? This way I would just leave the writing functions
unused and just apply the macro to the struct of the callbacks.
Otherwise, I need to move a lot of static and private macros to the
.h to allow the new file use them.

>
> thanks,
>
> greg k-h



-- 


Daniel Gutson
Engineering Director
Eclypsium, Inc.


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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Greg Kroah-Hartman]

On Tue, Nov 03, 2020 at 12:18:01PM -0300, Daniel Gutson wrote:
> On Thu, Oct 29, 2020 at 4:14 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Oct 29, 2020 at 12:39:08PM -0300, Daniel Gutson wrote:
> > > On Thu, Oct 29, 2020 at 2:40 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> > > > > This patch separates the writing part of the intel-spi drivers
> > > > > so the 'dangerous' part can be set/unset independently.
> > > > > This way, the kernel can be configured to include the reading
> > > > > parts of the driver which can be used without
> > > > > the dangerous write operations that can turn the system
> > > > > unbootable.
> > > > >
> > > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > > > ---
> > > > >  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
> > > > >  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
> > > > >  2 files changed, 33 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > index 5c0e0ec2e6d1..491c755fea49 100644
> > > > > --- a/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
> > > > >       tristate
> > > > >
> > > > >  config SPI_INTEL_SPI_PCI
> > > > > -     tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > > > > +     tristate "Intel PCH/PCU SPI flash PCI driver"
> > > > >       depends on X86 && PCI
> > > > >       select SPI_INTEL_SPI
> > > > >       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. 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.
> > > > > +       This enables read only 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.
> > > > > +       Using this driver it is possible to read the SPI chip directly
> > > > > +       from Linux.
> > > > >
> > > > >         To compile this driver as a module, choose M here: the module
> > > > >         will be called intel-spi-pci.
> > > > >
> > > > >  config SPI_INTEL_SPI_PLATFORM
> > > > > -     tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > > > > +     tristate "Intel PCH/PCU SPI flash platform driver"
> > > > >       depends on X86
> > > > >       select SPI_INTEL_SPI
> > > > >       help
> > > > > -       This enables platform support for the Intel PCH/PCU SPI
> > > > > +       This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> > > > > -       directly from Linux.
> > > > > +       Intel hardware and is used to hold BIOS and other persistent settings.
> > > > > +       Using this driver it is possible to read the SPI chip directly
> > > > > +       from Linux.
> > > > > +
> > > > > +       To compile this driver as a module, choose M here: the module
> > > > > +       will be called intel-spi-pci.
> > > > > +
> > > > > +config SPI_INTEL_SPI_WRITE
> > > > > +     bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> > > > > +     depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> > > > > +     help
> > > > > +       This enables full read/write support for the Intel PCH/PCU SPI
> > > > > +       controller.
> > > > > +       Using this option it may be 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.
> > > > > -
> > > > > -       To compile this driver as a module, choose M here: the module
> > > > > -       will be called intel-spi-platform.
> > > > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > index b54a56a68100..8d8053395c3d 100644
> > > > > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > +#ifdef CONFIG_SPI_INTEL_SPI_WRITE
> > > >
> > > > <snip>
> > > >
> > > > Please do not add #ifdef to .c files, that's not the proper kernel
> > > > coding style at all, and just makes maintaining this file much much
> > > > harder over time.
> > > >
> > > > Split things out into two different files if you really need to do this.
> > >
> > > What about the static functions that I'll need to turn non-static and
> > > in a header file?
> > > I mean, the functions that the functions in the new file will have to call.
> > > Should I do that, turn static functions into non-static and declared
> > > in a header file?
> >
> > No idea, but again, no #ifdefs in .c files like this, that is not the
> > proper kernel coding style as it is not maintainable for the lifespan
> > that we have to maintain code.
> 
> Is it acceptable to leave static functions unused and let the optimizer
> remove them as DCE?

You will get build warnings if you do that, right?

You have thousands of examples of how to do this correct, it shouldn't
be that tough :)

thanks,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: intel-spi: Split intel-spi reading from writing

[Daniel Gutson]

On Tue, Nov 3, 2020 at 1:07 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 03, 2020 at 12:18:01PM -0300, Daniel Gutson wrote:
> > On Thu, Oct 29, 2020 at 4:14 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Oct 29, 2020 at 12:39:08PM -0300, Daniel Gutson wrote:
> > > > On Thu, Oct 29, 2020 at 2:40 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Oct 28, 2020 at 06:43:59PM -0300, Daniel Gutson wrote:
> > > > > > This patch separates the writing part of the intel-spi drivers
> > > > > > so the 'dangerous' part can be set/unset independently.
> > > > > > This way, the kernel can be configured to include the reading
> > > > > > parts of the driver which can be used without
> > > > > > the dangerous write operations that can turn the system
> > > > > > unbootable.
> > > > > >
> > > > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> > > > > > ---
> > > > > >  drivers/mtd/spi-nor/controllers/Kconfig     | 39 ++++++++++++---------
> > > > > >  drivers/mtd/spi-nor/controllers/intel-spi.c | 12 +++++--
> > > > > >  2 files changed, 33 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > > index 5c0e0ec2e6d1..491c755fea49 100644
> > > > > > --- a/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > > +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> > > > > > @@ -31,34 +31,41 @@ config SPI_INTEL_SPI
> > > > > >       tristate
> > > > > >
> > > > > >  config SPI_INTEL_SPI_PCI
> > > > > > -     tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > > > > > +     tristate "Intel PCH/PCU SPI flash PCI driver"
> > > > > >       depends on X86 && PCI
> > > > > >       select SPI_INTEL_SPI
> > > > > >       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. 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.
> > > > > > +       This enables read only 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.
> > > > > > +       Using this driver it is possible to read the SPI chip directly
> > > > > > +       from Linux.
> > > > > >
> > > > > >         To compile this driver as a module, choose M here: the module
> > > > > >         will be called intel-spi-pci.
> > > > > >
> > > > > >  config SPI_INTEL_SPI_PLATFORM
> > > > > > -     tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > > > > > +     tristate "Intel PCH/PCU SPI flash platform driver"
> > > > > >       depends on X86
> > > > > >       select SPI_INTEL_SPI
> > > > > >       help
> > > > > > -       This enables platform support for the Intel PCH/PCU SPI
> > > > > > +       This enables read only platform 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. Using this driver it is possible to upgrade BIOS
> > > > > > -       directly from Linux.
> > > > > > +       Intel hardware and is used to hold BIOS and other persistent settings.
> > > > > > +       Using this driver it is possible to read the SPI chip directly
> > > > > > +       from Linux.
> > > > > > +
> > > > > > +       To compile this driver as a module, choose M here: the module
> > > > > > +       will be called intel-spi-pci.
> > > > > > +
> > > > > > +config SPI_INTEL_SPI_WRITE
> > > > > > +     bool "Intel PCH/PCU SPI flash drivers write operations (DANGEROUS)"
> > > > > > +     depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
> > > > > > +     help
> > > > > > +       This enables full read/write support for the Intel PCH/PCU SPI
> > > > > > +       controller.
> > > > > > +       Using this option it may be 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.
> > > > > > -
> > > > > > -       To compile this driver as a module, choose M here: the module
> > > > > > -       will be called intel-spi-platform.
> > > > > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > > index b54a56a68100..8d8053395c3d 100644
> > > > > > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > > > > > @@ -266,6 +266,7 @@ static int intel_spi_read_block(struct intel_spi *ispi, void *buf, size_t size)
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > +#ifdef CONFIG_SPI_INTEL_SPI_WRITE
> > > > >
> > > > > <snip>
> > > > >
> > > > > Please do not add #ifdef to .c files, that's not the proper kernel
> > > > > coding style at all, and just makes maintaining this file much much
> > > > > harder over time.
> > > > >
> > > > > Split things out into two different files if you really need to do this.
> > > >
> > > > What about the static functions that I'll need to turn non-static and
> > > > in a header file?
> > > > I mean, the functions that the functions in the new file will have to call.
> > > > Should I do that, turn static functions into non-static and declared
> > > > in a header file?
> > >
> > > No idea, but again, no #ifdefs in .c files like this, that is not the
> > > proper kernel coding style as it is not maintainable for the lifespan
> > > that we have to maintain code.
> >
> > Is it acceptable to leave static functions unused and let the optimizer
> > remove them as DCE?
>
> You will get build warnings if you do that, right?

I don't know why I didn't get any unused function warning, but OK. I'm using
a decently recent gcc.

>
> You have thousands of examples of how to do this correct, it shouldn't
> be that tough :)

The job is not that tough, you are to accept changes :)


>
> thanks,
>
> greg k-h



-- 


Daniel Gutson
Engineering Director
Eclypsium, Inc.


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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/