Re: ftrace use of pci_resource_to_user()

[Steven Rostedt]

On Wed, 4 May 2016 14:17:13 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> 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