[flashrom] [PATCH] Undo all PCI writes on shutdown

[David Hendricks]

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Undo all PCI config space writes on shutdown.
> This means all chipset enables etc. will be undone on shutdown.
> Any writes which are one-shot should use the permanent ppci_write_*
> variants.
>

Nack. I like the idea of the rpci_* functions, but I think rpci_* should be
opt-in and pci_* functions should remain as they are. As Michael pointed
out, I don't think it's good to redefine this behavior, and will probably be
a source of errors in the future for those used to pci_* functions as
defined in libpci.

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c
> ===================================================================
> --- flashrom-pci_configspace_shutdown_restore/drkaiser.c        (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/drkaiser.c
>  (Arbeitskopie)
> @@ -61,8 +61,7 @@
>
>  int drkaiser_shutdown(void)
>  {
> -       /* Write protect the flash again. */
> -       pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0);
> +       /* Flash write is disabled automatically by PCI restore. */
>

It's fine to nuke this function. However, I'd rather see corresponding
pci_write_word in drkaiser_init() changed to rpci_write_word().

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> +#define NO_PCI_REDIRECT 1
>

Should this be "NO_PCI_RESTORE"? Maybe I misunderstand the context of this
particular change...

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> +#define register_undo_pci_write(a, b, c)                               \
> +{                                                                      \
> +       struct undo_pci_write_data *undo_pci_write_data;                \
> +       undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \
> +       undo_pci_write_data->dev = *a;                                  \
> +       undo_pci_write_data->pos = b;                                   \
> +       undo_pci_write_data->type = pci_write_type_##c;                 \
> +       undo_pci_write_data->c##data = pci_read_##c(dev, pos);          \
> +       register_shutdown(undo_pci_write, undo_pci_write_data);         \
> +}
> +
> +int rpci_write_byte(struct pci_dev *dev, int pos, uint8_t data)
> +{
> +       register_undo_pci_write(dev, pos, byte);
> +       return pci_write_byte(dev, pos, data);
> +}
> +
> +int rpci_write_word(struct pci_dev *dev, int pos, uint16_t data)
> +{
> +       register_undo_pci_write(dev, pos, word);
> +       return pci_write_word(dev, pos, data);
> +}
> +
> +int rpci_write_long(struct pci_dev *dev, int pos, uint32_t data)
> +{
> +       register_undo_pci_write(dev, pos, long);
> +       return pci_write_long(dev, pos, data);
> +}
> +
> +int ppci_write_byte(struct pci_dev *dev, int pos, u8 data)
> +{
> +       return pci_write_byte(dev, pos, data);
> +}
> +
> +int ppci_write_word(struct pci_dev *dev, int pos, u16 data)
> +{
> +       return pci_write_word(dev, pos, data);
> +}
> +
> +int ppci_write_long(struct pci_dev *dev, int pos, u32 data)
> +{
> +       return pci_write_long(dev, pos, data);
> +}
> +
>

Dumb question -- What exactly is "pos" in this context? It looks like
register offset, in which case "reg" or something would be a better name.

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
> ===================================================================
> --- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c       (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
> (Arbeitskopie)
> @@ -89,13 +89,9 @@
>
>  int gfxnvidia_shutdown(void)
>  {
> -       uint32_t reg32;
> -
> -       /* Disallow access to flash interface (and re-enable screen). */
> -       reg32 = pci_read_long(pcidev_dev, 0x50);
> -       reg32 |= (1 << 0);
> -       pci_write_long(pcidev_dev, 0x50, reg32);
> -
> +       /* Flash interface access is disabled (and screen enabled)
> automatically
> +        * by PCI restore.
> +        */
>        pci_cleanup(pacc);
>        release_io_perms();
>        return 0;
> Index: flashrom-pci_configspace_shutdown_restore/atahpt.c
> ===================================================================
> --- flashrom-pci_configspace_shutdown_restore/atahpt.c  (Revision 1225)
> +++ flashrom-pci_configspace_shutdown_restore/atahpt.c  (Arbeitskopie)
> @@ -59,13 +59,7 @@
>
>  int atahpt_shutdown(void)
>  {
> -       uint32_t reg32;
> -
> -       /* Disable flash access again. */
> -       reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
> -       reg32 &= ~(1 << 24);
> -       pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
> -
> +       /* Flash access is disabled automatically by PCI restore. */
>        pci_cleanup(pacc);
>        release_io_perms();
>        return 0;
> Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c
> ===================================================================
> --- flashrom-pci_configspace_shutdown_restore/chipset_enable.c  (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c
>  (Arbeitskopie)
> @@ -498,17 +498,6 @@
>        return enable_flash_ich_dc_spi(dev, name, 10);
>  }
>
> -static void via_do_byte_merge(void * arg)
> -{
> -       struct pci_dev * dev = arg;
> -       uint8_t val;
> -
> -       msg_pdbg("Re-enabling byte merging\n");
> -       val = pci_read_byte(dev, 0x71);
> -       val |= 0x40;
> -       pci_write_byte(dev, 0x71, val);
> -}
> -
>  static int via_no_byte_merge(struct pci_dev *dev, const char *name)
>  {
>        uint8_t val;
> @@ -519,7 +508,6 @@
>                msg_pdbg("Disabling byte merging\n");
>                val &= ~0x40;
>                pci_write_byte(dev, 0x71, val);
> -               register_shutdown(via_do_byte_merge, dev);
>        }
>        return NOT_DONE_YET;    /* need to find south bridge, too */
>  }
>

Good stuff!

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Index: flashrom-pci_configspace_shutdown_restore/flashrom.c
> +#if NO_PCI_REDIRECT
> +/* Don't touch pci_write_* definitions. */
> +#else
> +#define pci_write_byte rpci_write_byte
> +#define pci_write_word rpci_write_word
> +#define pci_write_long rpci_write_long
>  #endif
> +#endif


This is really the only part of the patch that I dislike...

I think the added work of changing a couple pci_write_* calls to rpci_* in
the via, atahpt, drkaiser, and gfxnvidia functions is better than changing
the behavior of pci_write_* all together.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20101107/2aeb3e37/attachment.html>