On 29/03/2017 13:34, Marc Gonzalez wrote:
> + /*
> + * QUIRK #1
> + * Reads in configuration space outside devfn 0 return garbage.
> + */
> + if (devfn != 0) {
> + *val = ~0; /* Is this required even if we return an error? */
> + return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */
> + }
AFAICT, the relevant caller is:
bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
int crs_timeout)
if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
return false;
Therefore, I believe updating *val is unnecessary.
What matters is returning an error when appropriate.
PCIBIOS_FUNC_NOT_SUPPORTED fits my purpose.
> +
> + /*
> + * QUIRK #2
> + * The root complex advertizes a fake BAR, which is used to filter
> + * bus-to-system requests. Hide it from Linux.
> + */
> + if (where == PCI_BASE_ADDRESS_0 && bus->number == 0) {
> + *val = 0; /* 0 or ~0 to hide the BAR from Linux? */
> + return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */
> + }
AFAICT, the relevant caller is:
int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
struct resource *res, unsigned int pos)
u32 l, sz, mask;
pci_read_config_dword(dev, pos, &l);
pci_write_config_dword(dev, pos, l | mask);
pci_read_config_dword(dev, pos, &sz);
pci_write_config_dword(dev, pos, l);
Several things are note-worthy:
1) __pci_read_base() ignores the config accessors return value.
Of the existing PCIBIOS errors, none seem to be a good fit for
my use-case (hiding a non-standard BAR).
Tangent:
Maybe I should set dev->non_compliant_bars = true; instead
of messing around in the accessor...
I would likely set the flag in a PCI_FIXUP_EARLY function.
/* Early fixups, before probing the BARs */
1b) Perhaps a generic error could be added to the PCIBIOS_*
error family, to signal that the requested operation was not
completed, and *val remains unchanged.
=> Maybe PCIBIOS_FAILURE or PCIBIOS_NOP ?
2) Some drivers are not aware that *val needs to be updated
for BAR accesses.
e.g. drivers/pci/host/pcie-altera.c
if altera_pcie_hide_rc_bar() is true, 'l' and 'sz' are not updated,
and therefore contain garbage (uninitialized stack variables).
I think we should apply the following trivial patch.
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -175,7 +175,7 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar)
int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
struct resource *res, unsigned int pos)
{
- u32 l, sz, mask;
+ u32 l = 0, sz = 0, mask;
u64 l64, sz64, mask64;
u16 orig_cmd;
struct pci_bus_region region, inverted_region;
Regards.
