Re: PCI IO resource question.

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Mar 18, 2016 at 02:12:51PM -0400, Murali Karicheri wrote:
> On 03/18/2016 11:28 AM, Bjorn Helgaas wrote:
> > On Fri, Mar 18, 2016 at 11:09:27AM -0400, Murali Karicheri wrote:
> >> On 03/18/2016 07:28 AM, Lorenzo Pieralisi wrote:
> >>> On Thu, Mar 17, 2016 at 05:28:31PM -0400, Murali Karicheri wrote:
> >>>
> >>> [...]
> >>>
> >>>>> The only ways I see that PCI_PROBE_ONLY can be set on ARM are if you have
> >>>>> "linux,pci-probe-only" in your DT or you boot with "pci=firmware".
> >>>>>
> >>>>> I expect you're in this path:
> >>>>>
> >>>>>   ahci_init_one
> >>>>>     pcim_enable_device
> >>>>>       pci_enable_device
> >>>>>         pci_enable_device_flags(dev, IORESOURCE_MEM | IORESOURCE_IO)
> >>>>>           # build "bars" mask
> >>>>>           do_pci_enable_device(dev, bars)
> >>>>>             pcibios_enable_device
> >>>>>               if (pci_has_flag(PCI_PROBE_ONLY))
> >>>>>                 return 0;
> >>>>>               pci_enable_resources
> >>>>>
> >>>>> Can you add a little debug code like this to verify that we're in this
> >>>>> path?
> >>>>
> >>>> Yes we are in the path.
> >>>>
> >>>>
> >>>> [    1.557561] ahci_init_one
> >>>> [    1.560214] ahci 0000:01:00.0: version 3.0
> >>>> [    1.564302] pcim_enable_device
> >>>> [    1.567349] pci_enable_device
> >>>> [    1.570340] pci_enable_device_flags
> >>>> [    1.573824] do_pci_enable_device
> >>>> [    1.577042] pcibios_enable_device
> >>>> [    1.580380] pci_enable_resources
> >>>
> >>> So resources are actually enabled (ie PCI_PROBE_ONLY is not set)
> >>> and that makes sense otherwise you would not be able to use the
> >>> MEM resources anyway (ie they would not be enabled).
> >>>
> >>> I suspect the PCI dev IO resources were reset in reset_resource() in
> >>> assign_requested_resource_sorted(), hence the bar mask that is built
> >>> in pci_enable_device_flags() does not contain the IO resources,
> >>> it would be helpful if you can print the bar mask passed to
> >>> pcibios_enable_device() (ie the mask parameter).
> >>
> >> Here it is
> >>
> >> [    1.556507] ahci_init_one
> >> [    1.559124] ahci 0000:01:00.0: version 3.0
> >> [    1.563246] pcim_enable_device
> >> [    1.566294] pci_enable_device
> >> [    1.569252] pci_enable_device_flags
> >> [    1.572766] do_pci_enable_device
> >> [    1.575985] pcibios_enable_device 60
> >> [    1.579551] pci_enable_resources
> >>
> >> I know that some of our customers use PCIe SATA from u-boot and would
> >> like to honor the assignment in Linux space.. I believe they use PCI_PROBE_ONLY
> >> by setting the bootarg. So Keystone PCI should work in both cases.
> > 
> > We're only getting little pieces of the story here.  Can you apply the
> > following patch and collect the entire dmesg log?  I want to see:
> > 
> >   - the root bus resources (which presumably include no I/O space)
> >   - all the SATA resources during enumeration (which should include an
> >     I/O BAR)
> >   - the reset_resource() call that clears the I/O BAR flags
> >   - all the SATA resources in pci_enable_resources() (the I/O BAR
> >     should be cleared out)
> >   - the PCI_COMMAND register values before and after
> >     pci_enable_resources()
> > 
> > Bjorn
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 55641a3..83e8d42 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -211,6 +211,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
> >  
> >  static inline void reset_resource(struct resource *res)
> >  {
> > +	printk("%s: %pR\n", __func__, res);
> >  	res->start = 0;
> >  	res->end = 0;
> >  	res->flags = 0;
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index 66c4d8f..c2c45f9 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> > @@ -364,11 +364,14 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
> >  	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> >  	old_cmd = cmd;
> >  
> > +	dev_info(&dev->dev, "%s: mask %#x old_cmd %#x\n", __func__, mask, old_cmd);
> > +
> >  	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> >  		if (!(mask & (1 << i)))
> >  			continue;
> >  
> >  		r = &dev->resource[i];
> > +		dev_info(&dev->dev, "  BAR %d %pR parent %p\n", i, r, r->parent);
> >  
> >  		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> >  			continue;
> > @@ -394,6 +397,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
> >  			cmd |= PCI_COMMAND_MEMORY;
> >  	}
> >  
> > +	dev_info(&dev->dev, "%s: cmd %#x\n", __func__, cmd);
> >  	if (cmd != old_cmd) {
> >  		dev_info(&dev->dev, "enabling device (%04x -> %04x)\n",
> >  			 old_cmd, cmd);
> > 
> You can see complete bootlog with above at
> http://pastebin.ubuntu.com/15416575/

Here are the interesting parts:

  keystone-pcie 21021000.pcie: PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [bus 00-ff]
  pci_bus 0000:00: root bus resource [mem 0x60000000-0x6fffffff]

No I/O space, as we expected.

  pci 0000:01:00.0: [1b4b:9182] type 00 class 0x010601
  pci 0000:01:00.0: reg 0x10: [io  0x8000-0x8007]
  pci 0000:01:00.0: reg 0x14: [io  0x8040-0x8043]
  pci 0000:01:00.0: reg 0x18: [io  0x8100-0x8107]
  pci 0000:01:00.0: reg 0x1c: [io  0x8140-0x8143]
  pci 0000:01:00.0: reg 0x20: [io  0x800000-0x80000f]
  pci 0000:01:00.0: reg 0x24: [mem 0x00900000-0x009001ff]
  pci 0000:01:00.0: reg 0x30: [mem 0xd0000000-0xd000ffff pref]

Several I/O BARs shown above.

  pci 0000:01:00.0: BAR 6: assigned [mem 0x60100000-0x6010ffff pref]
  pci 0000:01:00.0: BAR 5: assigned [mem 0x60000000-0x600001ff]
  pci 0000:01:00.0: BAR 4: failed to assign [io  size 0x0010]
  reset_resource: [io  size 0x0010]
  pci 0000:01:00.0: BAR 0: failed to assign [io  size 0x0008]
  reset_resource: [io  size 0x0008]
  pci 0000:01:00.0: BAR 2: failed to assign [io  size 0x0008]
  reset_resource: [io  size 0x0008]
  pci 0000:01:00.0: BAR 1: failed to assign [io  size 0x0004]
  reset_resource: [io  size 0x0004]
  pci 0000:01:00.0: BAR 3: failed to assign [io  size 0x0004]
  reset_resource: [io  size 0x0004]

reset_resource() shows "size 0x...." instead of the address because we
set the IORESOURCE_UNSET bit when we failed to assign space.  That
part is fine, but then reset_resource() goes on to clear res->flags,
which is not fine.

  ahci 0000:01:00.0: ahci_init_one:
  ahci 0000:01:00.0: version 3.0
  ahci 0000:01:00.0: pcim_enable_device:
  ahci 0000:01:00.0: pci_enable_device:
  ahci 0000:01:00.0: pci_enable_device_flags:
  ahci 0000:01:00.0: do_pci_enable_device:
  ahci 0000:01:00.0: pcibios_enable_device: 60
  ahci 0000:01:00.0: pci_enable_resources: mask 0x60 old_cmd 0x143
  ahci 0000:01:00.0:   BAR 5 [mem 0x60000000-0x600001ff] parent eb149b10
  ahci 0000:01:00.0:   BAR 6 [mem 0x60100000-0x6010ffff pref] parent eb149b38
  ahci 0000:01:00.0: pci_enable_resources: cmd 0x143

pci_enable_device() requests all resources of type
IORESOURCE_MEM | IORESOURCE_IO.  pci_enable_device_flags() builds
"mask" (0x60 here) based on which resources match that type.  For the
I/O resources, res->flags has been cleared out by reset_resource(), so
only the MMIO resources (BARs 5 & 6) match, hence we have bits 5 and 6
set in "mask".

So pci_enable_resources() only looks at the MMIO resources, which are
both fine.  It thinks no IORESOURCE_IO resources are needed, so it
doesn't turn on PCI_COMMAND_IO.  Somebody (maybe firmware) had
previously enabled PCI_COMMAND_IO, and we leave it enabled.  This is a
potential problem because those I/O BARs are still enabled and the
device will respond if it receives an I/O access to those regions.
This isn't a problem on your particular system because there's no way
to generate I/O accesses, but it *is* a problem in general.

There are lots of things I think we should fix here.  They're all in
the PCI core and in drivers, not in anything Keystone-related:

  - reset_resource() shouldn't clear the IORESOURCE_TYPE_BITS.  This
    probably has implications in the rest of resource assignment.
  - pci_enable_resources() probably should clear PCI_COMMAND_IO if any
    I/O resources are unset.
  - There should be a pcim_enable_device_mem().
  - ahci_init_one() and similar drivers that don't need I/O space
    should use pcim_enable_device_mem().

Bjorn