Question about PCI I/O space in ARM64

Question about PCI I/O space in ARM64

[Kefeng Wang]

Hi all?

If no pci, the PCI I/O space(16M) is mapped into an irrelevant mem space(right ?), not a right IO space,
that is, no one call pci_remap_iospace() to remap the memory mapped I/O space, once driver
like f71805f loaded, write value to IO space(see f71805f_init->f71805f_find->superio_enter->outb),
we met following oops,
------------------------
Unable to handle kernel paging request at virtual address ffffffbffee0002e
pgd = ffffffc1d68d4000
[ffffffbffee0002e] *pgd=0000000000000000, *pud=0000000000000000
Internal error: Oops: 94000046 [#1] PREEMPT SMP
Modules linked in: f71805f(+) hwmon
CPU: 3 PID: 1659 Comm: insmod Not tainted 4.5.0+ #88
Hardware name: linux,dummy-virt (DT)
task: ffffffc1f6665400 ti: ffffffc1d6418000 task.ti: ffffffc1d6418000
PC is at f71805f_find+0x6c/0x358 [f71805f]
------------------------

I am not clear about PCI I/O, but if this is indeed a bug, how to solve this issue,
any advice will be appreciated.

Thanks
Kefeng

Question about PCI I/O space in ARM64

[Arnd Bergmann]

On Wednesday 23 March 2016 10:18:40 Catalin Marinas wrote:
> On Wed, Mar 23, 2016 at 11:12:41AM +0800, Kefeng Wang wrote:
> > If no pci, the PCI I/O space(16M) is mapped into an irrelevant mem space(right ?),
> 
> No. It is not mapped at all.
> 
> > not a right IO space,
> > that is, no one call pci_remap_iospace() to remap the memory mapped I/O space, once driver
> > like f71805f loaded, write value to IO space(see f71805f_init->f71805f_find->superio_enter->outb),
> > we met following oops,
> > ------------------------
> > Unable to handle kernel paging request at virtual address ffffffbffee0002e
> > pgd = ffffffc1d68d4000
> > [ffffffbffee0002e] *pgd=0000000000000000, *pud=0000000000000000
> > Internal error: Oops: 94000046 [#1] PREEMPT SMP
> > Modules linked in: f71805f(+) hwmon
> > CPU: 3 PID: 1659 Comm: insmod Not tainted 4.5.0+ #88
> > Hardware name: linux,dummy-virt (DT)
> > task: ffffffc1f6665400 ti: ffffffc1d6418000 task.ti: ffffffc1d6418000
> > PC is at f71805f_find+0x6c/0x358 [f71805f]
> > ------------------------
> 
> That's caused by not having a mapped PCI I/O space.
> 
> > I am not clear about PCI I/O, but if this is indeed a bug, how to solve this issue,
> > any advice will be appreciated.
> 
> You need a PCI host controller driver (e.g.
> drivers/pci/host/pci-host-generic.c) and corresponding bindings in DT or
> ACPI.

I think getting an Oops is not the best behavior though, it would be
nice if that could be improved in some way.

Ideally, each driver that accesses PCI I/O space would call request_region()
before doing so, and it would be good if that call could be made to
return an error when asked about an address that has not been mapped.

I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
range. If we could change it so that pci_remap_iospace() hooks up
to ioport_resource and extends it whenever something gets mapped
there up to IO_SPACE_LIMIT, we can change the default range to
{0,0}, which would fail for any request_region call before the
first pci_remap_iospace.

This won't help for the specific f71805f driver example, because that
does not call request_region(), but we can treat that as a driver bug
and fix it.

	Arnd

Question about PCI I/O space in ARM64

[Kefeng Wang]

On 2016/3/23 20:26, Arnd Bergmann wrote:
> On Wednesday 23 March 2016 10:18:40 Catalin Marinas wrote:
>> On Wed, Mar 23, 2016 at 11:12:41AM +0800, Kefeng Wang wrote:
>>> If no pci, the PCI I/O space(16M) is mapped into an irrelevant mem space(right ?),
>>
>> No. It is not mapped at all.
>>
>>> not a right IO space,
>>> that is, no one call pci_remap_iospace() to remap the memory mapped I/O space, once driver
>>> like f71805f loaded, write value to IO space(see f71805f_init->f71805f_find->superio_enter->outb),
>>> we met following oops,
>>> ------------------------
>>> Unable to handle kernel paging request at virtual address ffffffbffee0002e
>>> pgd = ffffffc1d68d4000
>>> [ffffffbffee0002e] *pgd=0000000000000000, *pud=0000000000000000
>>> Internal error: Oops: 94000046 [#1] PREEMPT SMP
>>> Modules linked in: f71805f(+) hwmon
>>> CPU: 3 PID: 1659 Comm: insmod Not tainted 4.5.0+ #88
>>> Hardware name: linux,dummy-virt (DT)
>>> task: ffffffc1f6665400 ti: ffffffc1d6418000 task.ti: ffffffc1d6418000
>>> PC is at f71805f_find+0x6c/0x358 [f71805f]
>>> ------------------------
>>
>> That's caused by not having a mapped PCI I/O space.

Clear.

>>
>>> I am not clear about PCI I/O, but if this is indeed a bug, how to solve this issue,
>>> any advice will be appreciated.
>>
>> You need a PCI host controller driver (e.g.
>> drivers/pci/host/pci-host-generic.c) and corresponding bindings in DT or
>> ACPI.

In our inner test, there are some board without pcie host driver(even without
pci host controller).

> 
> I think getting an Oops is not the best behavior though, it would be
> nice if that could be improved in some way.
> 
> Ideally, each driver that accesses PCI I/O space would call request_region()
> before doing so, and it would be good if that call could be made to
> return an error when asked about an address that has not been mapped.
> 
> I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
> range. If we could change it so that pci_remap_iospace() hooks up
> to ioport_resource and extends it whenever something gets mapped
> there up to IO_SPACE_LIMIT, we can change the default range to
> {0,0}, which would fail for any request_region call before the
> first pci_remap_iospace.
> 
> This won't help for the specific f71805f driver example, because that
> does not call request_region(), but we can treat that as a driver bug
> and fix it.

Yes, we met same error with several modules(at least 8+), so it's better to find
a good way to avoid it, change modules one by one maybe not a good choice.

Define some arch in/out func instead of generic ops? when in/out vals, check
whether or not the pci_iobase is mapped.

Thanks catalin and arnd for your reply.
> 
> 	Arnd
> 
> .
> 

Question about PCI I/O space in ARM64

[Lorenzo Pieralisi]

[+ Jean]

On Thu, Mar 24, 2016 at 11:14:50AM +0800, Kefeng Wang wrote:

[...]

> >> You need a PCI host controller driver (e.g.
> >> drivers/pci/host/pci-host-generic.c) and corresponding bindings in DT or
> >> ACPI.
> 
> In our inner test, there are some board without pcie host driver(even
> without pci host controller).

So I guess those drivers are for devices that are attached to an LPC
controller that is not part of a PCI host controller, right ?

BTW, what happened to this (not that I particularly like this patchset) ?

https://lkml.org/lkml/2015/12/29/154

> > I think getting an Oops is not the best behavior though, it would be
> > nice if that could be improved in some way.
> > 
> > Ideally, each driver that accesses PCI I/O space would call request_region()
> > before doing so, and it would be good if that call could be made to
> > return an error when asked about an address that has not been mapped.
> > 
> > I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
> > range. If we could change it so that pci_remap_iospace() hooks up
> > to ioport_resource and extends it whenever something gets mapped
> > there up to IO_SPACE_LIMIT, we can change the default range to
> > {0,0}, which would fail for any request_region call before the
> > first pci_remap_iospace.
> > 
> > This won't help for the specific f71805f driver example, because that
> > does not call request_region(), but we can treat that as a driver bug
> > and fix it.
> 
> Yes, we met same error with several modules(at least 8+), so it's
> better to find a good way to avoid it, change modules one by one maybe
> not a good choice.

But that's the right thing to do :)

The problem with those drivers is that IIUC they use ISA IO port space to
detect if the devices they manage are actually present, it does not
look that correct to me, at least not on !X86 (IA64 ?) systems.

Those drivers can't be built on PPC for a reason (?), I found these threads
which might be telling:

http://www.spinics.net/lists/lm-sensors/msg43868.html
http://comments.gmane.org/gmane.linux.drivers.sensors/17290

I agree with Arnd on this and we need to patch the eg f71805f driver too
(or we just do not build on ARM) to make it request the IO port region it
needs to actually probe the device, it is not correct to assume IO space
is available and mapped, I think that's a driver bug rather than anything
else, Jean can certainly shed some light here.

> Define some arch in/out func instead of generic ops? when in/out vals, check
> whether or not the pci_iobase is mapped.

I do not think the problem should be solved in the IO accessors
implementation, see above.

Lorenzo

Question about PCI I/O space in ARM64

[Kefeng Wang]

On 2016/3/25 1:57, Lorenzo Pieralisi wrote:
> [+ Jean]
> 
> On Thu, Mar 24, 2016 at 11:14:50AM +0800, Kefeng Wang wrote:
> 
> [...]
> 
>>>> You need a PCI host controller driver (e.g.
>>>> drivers/pci/host/pci-host-generic.c) and corresponding bindings in DT or
>>>> ACPI.
>>
>> In our inner test, there are some board without pcie host driver(even
>> without pci host controller).
> 
> So I guess those drivers are for devices that are attached to an LPC
> controller that is not part of a PCI host controller, right ?
> 
> BTW, what happened to this (not that I particularly like this patchset) ?

They are working on it, and will upstream again.

> 
> https://lkml.org/lkml/2015/12/29/154
> 
[...]
> 
> I agree with Arnd on this and we need to patch the eg f71805f driver too
> (or we just do not build on ARM) to make it request the IO port region it
> needs to actually probe the device, it is not correct to assume IO space
> is available and mapped, I think that's a driver bug rather than anything
> else, Jean can certainly shed some light here.

Ok, so Jean, would you take a look this? Thanks.

Kefeng
> 
>> Define some arch in/out func instead of generic ops? when in/out vals, check
>> whether or not the pci_iobase is mapped.
> 
> I do not think the problem should be solved in the IO accessors
> implementation, see above.
> 
> Lorenzo
> 
> .
>