On Wed, 4 May 2016 14:17:13 -0500
Bjorn Helgaas <[email protected]> wrote:
> 138295373ccf ("ftrace: mmiotrace update, #2") added this use of
> pci_resource_to_user():
>
> +static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev
> *dev)
> +{
> ...
> + /*
> + * XXX: is pci_resource_to_user() appropriate, since we are
> + * supposed to interpret the __ioremap() phys_addr argument based on
> + * these printed values?
> + */
> + for (i = 0; i < 7; i++) {
> + pci_resource_to_user(dev, i, &dev->resource[i], &start,
> &end);
> + ret += trace_seq_printf(s, " %llx",
> + (unsigned long long)(start |
> + (dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
> + }
>
> I think it was a mistake to use pci_resource_to_user() here because it
> adds unnecessary arch dependencies in whatever consumes this output.
>
> On most arches, pci_resource_to_user() is a no-op and the result is
> normal resource addresses, i.e., CPU physical addresses that match
> things in /proc/iomem and /sys/devices/pci.../resource.
>
> On microblaze, mips, powerpc, and sparc, the result of
> pci_resource_to_user() is something else, usually a PCI bus address (a
> raw BAR value). These values are only useful for using mmap on
> files like /proc/bus/pci/....
>
> I don't know what, if anything, consumes this output. If things parse
> it, we shouldn't break them. But those things likely would need
> special cases for microblaze, mips, powerpc, and sparc.
>
> If it's only for human consumption, I think we should consider
> removing the use of pci_resource_to_user() and printing
> dev->resource[i].start instead.
Currently this code requires the arch to define HAVE_MMIOTRACE_SUPPORT,
and so far as I can tell, only x86 does this.
-- Steve
