Re: [PATCH 1/1] pci: Block config access during BIST (resend)

Matthew Wilcox
2005-02-01T15:44:38Z

On Tue, Feb 01, 2005 at 09:12:56AM -0600, Brian King wrote:
> Greg KH wrote:
> >On Fri, Jan 28, 2005 at 08:35:46AM -0600, Brian King wrote:
> >>+void pci_block_user_cfg_access(struct pci_dev *dev)
> >>+{
> >>+ unsigned long flags;
> >>+
> >>+ pci_save_state(dev);
> >>+ spin_lock_irqsave(&pci_lock, flags);
> >>+ dev->block_ucfg_access = 1;
> >>+ spin_unlock_irqrestore(&pci_lock, flags);
> >>+}
> >>+EXPORT_SYMBOL(pci_block_user_cfg_access);
> >
> >
> >EXPORT_SYMBOL_GPL() please?
>
> Ok.

I'm not entirely convinced these should be GPL-only exports. Basically,
this is saying that any driver for a device that has this problem must be
GPLd. I think that's a firmer stance than Linus had in mind originally.

> +++ linux-2.6.11-rc2-bk9-bjking1/drivers/pci/access.c 2005-02-01 08:39:57.000000000 -0600
> @@ -60,3 +60,78 @@ EXPORT_SYMBOL(pci_bus_read_config_dword)
> EXPORT_SYMBOL(pci_bus_write_config_byte);
> EXPORT_SYMBOL(pci_bus_write_config_word);
> EXPORT_SYMBOL(pci_bus_write_config_dword);
> +
> +#define PCI_USER_READ_CONFIG(size,type) \
> +int pci_user_read_config_##size \
> + (struct pci_dev *dev, int pos, type *val) \
{ \
unsigned long flags; \

Could you line up the \ so they're uniform like the above PCI_OP_READ?

> + int ret = 0; \
> + u32 data = -1; \
> + if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
> + spin_lock_irqsave(&pci_lock, flags); \
> + if (likely(!dev->block_ucfg_access)) \
> + ret = dev->bus->ops->read(dev->bus, dev->devfn, pos, sizeof(type), &data); \
> + else if (pos < sizeof(dev->saved_config_space)) \
> + data = dev->saved_config_space[pos/sizeof(dev->saved_config_space[0])]; \
> + spin_unlock_irqrestore(&pci_lock, flags); \
> + *val = (type)data; \

Does this actually work? Surely if you're reading byte 14, you get the
byte that was at address 12 or 15 in the config space, depending whether
you're on a big or little endian machine?

> +void pci_unblock_user_cfg_access(struct pci_dev *dev)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pci_lock, flags);
> + dev->block_ucfg_access = 0;
> + spin_unlock_irqrestore(&pci_lock, flags);
> +}

If we've done a write to config space while the adapter was blocked,
shouldn't we replay those accesses at this point?

--
"Next the statesmen will invent cheap lies, putting the blame upon
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince
himself that the war is just, and will thank God for the better sleep
he enjoys after this process of grotesque self-deception." -- Mark Twain