Re: [PATCH] powerpc/pci: Fix IO space breakage after of_pci_range_to_resource() change

(hope my email makes it everywhere, using a webmail client at the moment as=
 I'm
at plumbersconf

Michael Ellerman [off-list ref] hat am 16. Oktober 2014 um 05:22
geschrieben:

Commit 0b0b0893d49b "of/pci: Fix the conversion of IO ranges into IO
resources" changed the behaviour of of_pci_range_to_resource().

I just looked at this after benh mentioned the problem on IRC, here's a log=
 dump

:26 AM   <benh> argh =20
9:27 AM   <benh> the whole ARM OF PCI rework seems to completely break PIO =
on
powerpc =20
9:30 AM   =E2=86=92 willy <willy> joined (^willy@62.156.150.204) =20
9:35 AM   <benh> and reverting it would mean reverting all of ARM new PCI s=
tuff
=20
9:35 AM   <benh> crap =20
9:35 AM   <benh> that business with IO space allocation taking over our cod=
e
without understanding what it does =20
9:35 AM   <benh> yuck =20
9:41 AM     =E2=86=92 markf <markf> , olaf <olaf> , sarnold <sarnold> , gos=
po <gospo> ,
cmarinas <cmarinas> , Mahesh1 <Mahesh1> , joern <joern> , clark_ <clark_> a=
nd
benhjoined  =E2=87=90 gcl <gcl> and clark <clark> quit  =E2=86=94 willy <wi=
lly> , jbarnes
<jbarnes> , jbrandeb_ <jbrandeb_> and Mahesh <Mahesh> popped in  =E2=86=94 =
sameo <sameo>
, jbrandeb <jbrandeb> , steved <steved> and jj <jj> nipped out  =E2=80=A2  =
srikar =E2=86=92
srikar_away <srikar_away> , raghu =E2=86=92 raghu_away <raghu_away> =20
Thursday, October 16th, 2014 =20
12:06 AM     =E2=86=92 fweisbec <fweisbec> , Mahesh <Mahesh> , kamalesh <ka=
malesh> ,
heiko <heiko> , olaf <olaf> , riel <riel> and willy <willy> joined  =E2=87=
=90 shaggy
<shaggy> , sammj <sammj> , sameo <sameo> , clark_ <clark_> , lenb <lenb> an=
d jj
<jj> quit  =E2=86=94 jbrandeb <jbrandeb> , cdub <cdub> , Mahesh1 <Mahesh1> =
and gcl <gcl>
popped in  =E2=86=94 jbrandeb_ <jbrandeb_> , benh, joern <joern> and BenC <=
BenC> nipped
out  =E2=80=A2  mpe|away =E2=86=92 mpe|away <mpe%7Caway> , raghu_away =E2=
=86=92 raghu <raghu> ,
srikar_away =E2=86=92 srikar <srikar> =20
10:16 AM   <arnd_> benh: is it the of_pci_range_to_resource change? =20
10:17 AM   <arnd_> the new pci_ioremap_iospace logic should not get used on
powerpc at all, so I didn't expect any breakage =20
10:18 AM   <arnd_> I wasn't too happy with all the details of Liviu's serie=
s,
bit in the end it seemed reasonable enough =20
10:20 AM   <arnd_> he really wanted to use the pci_address_to_pio code from
powerpc and in the end I stopped complaining =20
10:21 AM   <arnd_> the new code can do a few things that simpler versions c=
ould
not, e.g. handling multiple host bridges getting registered when they have =
the
same I/O space window =20
10:23 AM   =E2=86=92 jj <jj> joined (^jj@static-50-53-60-87.bvtn.or.frontie=
rnet.net) =20
10:33 AM   <arnd_> benh: I can see how it breaks your
pci_process_bridge_OF_ranges, we had the same problem in some of the ARM
platforms and Liviu fixed those but apparently didn't realize he had to cha=
nge
the ppc implementation (and get your ack) too =20
10:35 AM   <arnd_> the good news is that it should in fact simplify your co=
de to
fix it, but the fact that this bug got into the kernel in the first place i=
s
extremely annoying =20
10:39 AM   <arnd_> benh: the fixup that is done in your
pcibios_reserve_legacy_regions is now already performed in
of_pci_range_to_resource =20
10:39 AM   <arnd_> we had duplicated the same thing in each pci host driver=
 (and
they all got it wrong), so the intent was to move it into a common place =
=20
10:40 AM   <arnd_> but of course it's a bug to do it twice =20
10:43 AM   <arnd_> pci_register_io_range is trying to do a more generalized
version of how you assign hose->io_base_virt, you should probably override =
that
to keep the current behavior =20
10:45 AM   <arnd_> pcibios_map_phb_io_space I mean, for ppc64 =20
10:52 AM   =E2=86=92 cmarinas <cmarinas> joined (~cmarinas@fw-tnat.cambridg=
e.arm.com) =20
10:53 AM   <arnd_> benh: for 3.18, the best approach is likely to #ifdef
<%23ifdef> PCI_IOBASE the changes in of_pci_range_to_resource =20
10:54 AM   <arnd_> I suspect you are fine with effectively reverting Liviu'=
s
changes that way, and you can decide whether or not you want to later make =
the
powerpc code use the common logic =20
11:01 AM     =E2=86=92 cdub <cdub> and sarnold <sarnold> joined  =E2=87=90 =
cmarinas <cmarinas>
quit =20
11:28 AM   <benh> arnd_: can you shoot the above in an email CCed to mpe ? =
=20
11:28 AM   <benh> arnd_: he did a band aid that works =20
11:28 AM   <benh> arnd_: and see the comment I made today about using his s=
tuff
if I can specify where I want the IO ranges =20
11:28 AM   <benh> arnd_: I want to keep the way I do the layout on ppc64 =
=20

Previously it simply populated the resource based on the arguments. Now
it calls pci_register_io_range() and pci_address_to_pio(). These both
have two implementations depending on whether PCI_IOBASE is defined,
which it is not for powerpc.

Further complicating matters, both routines are weak, and powerpc
implements it's own version of one - pci_address_to_pio(). However
powerpc's implementation depends on other initialisations which are done
later in boot.

Right, sorry for missing this during the last review of the broken patches.

The end result is incorrectly initialised IO space. Often we can get
away with that, because we don't make much use of IO space. However
virtio requires it, so we see eg:

pci_bus 0000:00: root bus resource [io 0xffff] (bus address
[0xffffffffffffffff-0xffffffffffffffff])
PCI: Cannot allocate resource region 0 of device 0000:00:01.0, will remap
virtio-pci 0000:00:01.0: can't enable device: BAR 0 [io size 0x0020] not
assigned

The simplest fix for now is to just stop using of_pci_range_to_resource()=

,

and open-code the original imp`lementation, that's all we want it to do.

The same bug is likely to be present on microblaze and mips, which may or m=
ay
not
care about it. I'll ask Michal about whether microblaze actually has any
I/O space, otherwise we have to fix it too for 3.18.

I believe for 3.19, we should probably migrate microblaze over to use the s=
ame
code
as ARM.

ler

*hose,
break;
}
if (res !=3D NULL) {
- of_pci_range_to_resource(&range, dev, res);
+ res->name =3D dev->full_name;
+ res->flags =3D range.flags;
+ res->start =3D range.cpu_addr;
+ res->end =3D range.cpu_addr + range.size - 1;
+ res->parent =3D res->child =3D res->sibling =3D NULL;
}
}

This looks reasonable to me as a hack to work around the breakage. It would=
 be
good
to work together on this for 3.19 to move on to the a common implementation=
 that
works on both ARM and PowerPC. This might be possibly by removing a lot of =
code
for PowerPC (at least 64-bit) that is now present in common code, but it wi=
ll
change the structure of the powerpc implementation significantly, since the
returned numbers are now in different memory spaces (logical I/O space rath=
er
than physical). The PowerPC _IO_BASE is the equivalent of the now generic
PCI_IOBASE,
but it's used slightly differently. If you want to use the generic code,=20
you should probably change host->io_base_virt to host->io_base in logical
space (i.e. removing the _IO_BASE offset), or using hose->io_resource.start
instead.

       Arnd