Re: [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi all!

On Thu, Sep 30, 2021 at 7:07 AM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>
> Currently the driver tries to disable the BIOS write protection
> automatically even if this is not what the user wants. For this reason
> modify the driver so that by default it does not touch the write
> protection. Only if specifically asked by the user (setting writeable=1
> command line parameter) the driver tries to disable the BIOS write
> protection.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>  drivers/mfd/lpc_ich.c                         | 59 +++++++++++++++++--
>  .../mtd/spi-nor/controllers/intel-spi-pci.c   | 29 +++++----
>  drivers/mtd/spi-nor/controllers/intel-spi.c   | 41 ++++++-------
>  include/linux/platform_data/x86/intel-spi.h   |  6 +-
>  4 files changed, 96 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index f10e53187f67..9ffab9aafd81 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -63,6 +63,8 @@
>  #define SPIBASE_BYT            0x54
>  #define SPIBASE_BYT_SZ         512
>  #define SPIBASE_BYT_EN         BIT(1)
> +#define BYT_BCR                        0xfc
> +#define BYT_BCR_WPD            BIT(0)
>
>  #define SPIBASE_LPT            0x3800
>  #define SPIBASE_LPT_SZ         512
> @@ -1084,12 +1086,57 @@ static int lpc_ich_init_wdt(struct pci_dev *dev)
>         return ret;
>  }
>
> +static bool lpc_ich_byt_set_writeable(void __iomem *base, void *data)
> +{
> +       u32 val;
> +
> +       val = readl(base + BYT_BCR);
> +       if (!(val & BYT_BCR_WPD)) {
> +               val |= BYT_BCR_WPD;
> +               writel(val, base + BYT_BCR);
> +               val = readl(base + BYT_BCR);
> +       }
> +
> +       return val & BYT_BCR_WPD;
> +}
> +
> +static bool lpc_ich_lpt_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       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);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
> +static bool lpc_ich_bxt_set_writeable(void __iomem *base, void *data)
> +{
> +       unsigned int spi = PCI_DEVFN(13, 2);
> +       struct pci_bus *bus = data;
> +       u32 bcr;
> +
> +       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       if (!(bcr & BCR_WPD)) {
> +               bcr |= BCR_WPD;
> +               pci_bus_write_config_dword(bus, spi, BCR, bcr);
> +               pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static int lpc_ich_init_spi(struct pci_dev *dev)
>  {
>         struct lpc_ich_priv *priv = pci_get_drvdata(dev);
>         struct resource *res = &intel_spi_res[0];
>         struct intel_spi_boardinfo *info;
> -       u32 spi_base, rcba, bcr;
> +       u32 spi_base, rcba;
>
>         info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
>         if (!info)
> @@ -1103,6 +1150,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                 if (spi_base & SPIBASE_BYT_EN) {
>                         res->start = spi_base & ~(SPIBASE_BYT_SZ - 1);
>                         res->end = res->start + SPIBASE_BYT_SZ - 1;
> +
> +                       info->set_writeable = lpc_ich_byt_set_writeable;
>                 }
>                 break;
>
> @@ -1113,8 +1162,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base + SPIBASE_LPT;
>                         res->end = res->start + SPIBASE_LPT_SZ - 1;
>
> -                       pci_read_config_dword(dev, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_lpt_set_writeable;
> +                       info->data = dev;
>                 }
>                 break;
>
> @@ -1135,8 +1184,8 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>                         res->start = spi_base & 0xfffffff0;
>                         res->end = res->start + SPIBASE_APL_SZ - 1;
>
> -                       pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> -                       info->writeable = !!(bcr & BCR_WPD);
> +                       info->set_writeable = lpc_ich_bxt_set_writeable;
> +                       info->data = bus;
>                 }
>
>                 pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 1bc53b8bb88a..508f7ca098ef 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -16,12 +16,30 @@
>  #define BCR            0xdc
>  #define BCR_WPD                BIT(0)
>
> +static bool intel_spi_pci_set_writeable(void __iomem *base, void *data)
> +{
> +       struct pci_dev *pdev = data;
> +       u32 bcr;
> +
> +       /* 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);
> +       }
> +
> +       return bcr & BCR_WPD;
> +}
> +
>  static const struct intel_spi_boardinfo bxt_info = {
>         .type = INTEL_SPI_BXT,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static const struct intel_spi_boardinfo cnl_info = {
>         .type = INTEL_SPI_CNL,
> +       .set_writeable = intel_spi_pci_set_writeable,
>  };
>
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
> @@ -29,7 +47,6 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>  {
>         struct intel_spi_boardinfo *info;
>         struct intel_spi *ispi;
> -       u32 bcr;
>         int ret;
>
>         ret = pcim_enable_device(pdev);
> @@ -41,15 +58,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);
> -
> +       info->data = pdev;
>         ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
>         if (IS_ERR(ispi))
>                 return PTR_ERR(ispi);
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> index a413892ff449..f35597cbea0c 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> @@ -131,7 +131,6 @@
>   * @sregs: Start of software sequencer registers
>   * @nregions: Maximum number of regions
>   * @pr_num: Maximum number of protected range registers
> - * @writeable: Is the chip writeable
>   * @locked: Is SPI setting locked
>   * @swseq_reg: Use SW sequencer in register reads/writes
>   * @swseq_erase: Use SW sequencer in erase operation
> @@ -149,7 +148,6 @@ struct intel_spi {
>         void __iomem *sregs;
>         size_t nregions;
>         size_t pr_num;
> -       bool writeable;
>         bool locked;
>         bool swseq_reg;
>         bool swseq_erase;
> @@ -304,6 +302,14 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
>                                   INTEL_SPI_TIMEOUT * 1000);
>  }
>
> +static bool intel_spi_set_writeable(struct intel_spi *ispi)
> +{
> +       if (!ispi->info->set_writeable)
> +               return false;
> +
> +       return ispi->info->set_writeable(ispi->base, ispi->info->data);
> +}
> +
>  static int intel_spi_init(struct intel_spi *ispi)
>  {
>         u32 opmenu0, opmenu1, lvscc, uvscc, val;
> @@ -316,19 +322,6 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 ispi->nregions = BYT_FREG_NUM;
>                 ispi->pr_num = BYT_PR_NUM;
>                 ispi->swseq_reg = true;
> -
> -               if (writeable) {
> -                       /* Disable write protection */
> -                       val = readl(ispi->base + BYT_BCR);
> -                       if (!(val & BYT_BCR_WPD)) {
> -                               val |= BYT_BCR_WPD;
> -                               writel(val, ispi->base + BYT_BCR);
> -                               val = readl(ispi->base + BYT_BCR);
> -                       }
> -
> -                       ispi->writeable = !!(val & BYT_BCR_WPD);
> -               }
> -
>                 break;
>
>         case INTEL_SPI_LPT:
> @@ -358,6 +351,12 @@ static int intel_spi_init(struct intel_spi *ispi)
>                 return -EINVAL;
>         }
>
> +       /* Try to disable write protection if user asked to do so */
> +       if (writeable && !intel_spi_set_writeable(ispi)) {
> +               dev_warn(ispi->dev, "can't disable chip write protection\n");
> +               writeable = false;
> +       }
> +
>         /* Disable #SMI generation from HW sequencer */
>         val = readl(ispi->base + HSFSTS_CTL);
>         val &= ~HSFSTS_CTL_FSMIE;
> @@ -884,9 +883,12 @@ static void intel_spi_fill_partition(struct intel_spi *ispi,
>                 /*
>                  * If any of the regions have protection bits set, make the
>                  * whole partition read-only to be on the safe side.
> +                *
> +                * Also if the user did not ask the chip to be writeable
> +                * mask the bit too.
>                  */
> -               if (intel_spi_is_protected(ispi, base, limit))
> -                       ispi->writeable = false;
> +               if (!writeable || intel_spi_is_protected(ispi, base, limit))
> +                       part->mask_flags |= MTD_WRITEABLE;
>
>                 end = (limit << 12) + 4096;
>                 if (end > part->size)
> @@ -927,7 +929,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         ispi->dev = dev;
>         ispi->info = info;
> -       ispi->writeable = info->writeable;
>
>         ret = intel_spi_init(ispi);
>         if (ret)
> @@ -945,10 +946,6 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>
>         intel_spi_fill_partition(ispi, &part);
>
> -       /* Prevent writes if not explicitly enabled */
> -       if (!ispi->writeable || !writeable)
> -               ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> -
>         ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
>         if (ret)
>                 return ERR_PTR(ret);
> diff --git a/include/linux/platform_data/x86/intel-spi.h b/include/linux/platform_data/x86/intel-spi.h
> index 7f53a5c6f35e..7dda3f690465 100644
> --- a/include/linux/platform_data/x86/intel-spi.h
> +++ b/include/linux/platform_data/x86/intel-spi.h
> @@ -19,11 +19,13 @@ enum intel_spi_type {
>  /**
>   * struct intel_spi_boardinfo - Board specific data for Intel SPI driver
>   * @type: Type which this controller is compatible with
> - * @writeable: The chip is writeable
> + * @set_writeable: Try to make the chip writeable (optional)
> + * @data: Data to be passed to @set_writeable can be %NULL
>   */
>  struct intel_spi_boardinfo {
>         enum intel_spi_type type;
> -       bool writeable;
> +       bool (*set_writeable)(void __iomem *base, void *data);
> +       void *data;
>  };
>
>  #endif /* INTEL_SPI_PDATA_H */
> --
> 2.33.0
>
Question for maintainers: With these changes is it safe to remove the
*(DANGEROUS)* tag from menuconfig?
Thanks, Mauro.



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux