PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Daniel Palmer]

Hi,

I posted on linux-ide a few days ago about PATA_ARTOP causing an oops in 
2.6.38.
Anyhow, it seems this issue has existed for a while but something has 
changed in the SH code since 2.6.35 that exposes the problem.
Mikael Pettersson replied to my original post and said that the driver 
works on his board. I suspect his board isn't reading the byte from the 
PCI io port (different controller that doesn't enter that code path) or 
on his board it maps to the right place (or some place that doesn't 
cause an oops) without out asking for it to be mapped.

I would write a patch, but I don't understand the linux PCI stuff enough 
to know what is right.
I have something that works on my machine (or at least it doesn't oops 
anymore) but it's probably not the right solution.

Anyhow;-


unsigned long io = pci_resource_start(pdev, 4);           // This 
returns 0x1400 on my machine
                 u8 reg;

                 ppi[0] = &info_628x;
                 if (inb(io) & 0x10)                                    
         // This reads from 0x1400, which isn't were the port actually 
is in the processors address                                             
                                                      // space and thus 
an oops happens.
                         ppi[0] = &info_628x_fast;
                 /* Mac systems come up with some registers not set as we
                    will need them */


IO port mapping for the pata controller;

    0.040000] pci 0000:00:01.0: BAR 4: set to [io  0x1400-0x140f] (PCI 
address [0x1400-0x140f])
[    0.040000] pci 0000:00:01.0: BAR 0: assigned [io  0x1410-0x1417]


I guess io should be calculated with pci_iomap(pdev, 4, 0) or something? 
I don't understand how the PCI stuff works though..
Looks like the BAR is hardcoded to 4 in the existing code. I have no 
idea what a BAR is. ;)
So, that's what I have on my machine.. and I don't get an oops anymore,
pata seems to be working too. I haven't checked what value is actually 
being read. So it's possible that it's just reading somewhere that 
doesn't cause an oops and the issue isn't actually fixed.
The main issue is the use of inb I guess. I noticed that there was some 
activity to remove all uses of it in drivers some time back.

Anyhow, that seems to be the problem. If someone who isn't just guessing 
this stuff could come up with a fix that would be nice.
I can get my board to boot now anyhow. :)

Regards,

Daniel

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Alan Cox]

> unsigned long io = pci_resource_start(pdev, 4);           // This 
> returns 0x1400 on my machine

That is the PCI port address.

>                  u8 reg;
> 
>                  ppi[0] = &info_628x;
>                  if (inb(io) & 0x10)                                    
>          // This reads from 0x1400, which isn't were the port actually 
> is in the processors address                                             

This reads from I/O port 0x1400 - your inb logic and I/O port mapping on
the platform is responsible for making that work.

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Linus Torvalds]

On Wed, Mar 30, 2011 at 12:26 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> unsigned long io = pci_resource_start(pdev, 4);           // This
>> returns 0x1400 on my machine
>
> That is the PCI port address.
>
>>                  u8 reg;
>>
>>                  ppi[0] = &info_628x;
>>                  if (inb(io) & 0x10)
>>          // This reads from 0x1400, which isn't were the port actually
>> is in the processors address
>
> This reads from I/O port 0x1400 - your inb logic and I/O port mapping on
> the platform is responsible for making that work.

Indeed. We do _not_ expect inb/outb to do the equivalent of ioremap()
etc, since the IO port range is just 64kB. The arch code is supposed
to do whatever mapping it needs statically at bootup, and just make
inb/outb honor that mapping.

                          Linus

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Daniel Palmer]

>  That is the PCI port address.

I don't pretend to understand most of this fully.
On the board the PCI address space/PCI controller is apparently at 
0xfe240000
So I guess that when that byte is read it should read from 0xfe240000 + 
0x1400?
That doesn't happen and a read happens at 0x1400 and an oops results.
I don't know enough to point the finger really.

There are probably only 2 people using this board still too. So I doubt 
it really matters. :)

Cheers,

Daniel

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Arnd Bergmann]

On Wednesday 30 March 2011 21:48:26 Daniel Palmer wrote:
> >  That is the PCI port address.
> 
> I don't pretend to understand most of this fully.
> On the board the PCI address space/PCI controller is apparently at 
> 0xfe240000
> So I guess that when that byte is read it should read from 0xfe240000 + 
> 0x1400?
> That doesn't happen and a read happens at 0x1400 and an oops results.
> I don't know enough to point the finger really.
> 
> There are probably only 2 people using this board still too. So I doubt 
> it really matters. :)

Is this an ARM machine? The platform specific mach/io.h header file
in that case defines an __io() macro that is responsible for mapping
port numbers to virtual addresses.

You have to ioremap() the physical address 0xfe240000 to a virtual
address, and then add the base address to the number inside of the
__io macro, e.g.

/* in mach/io.h: */
extern void __iomem *this_board_pci_iospace;
#define __io(x) ((x & 0xffff) + this_board_pci_iospace)

/* in pci controller setup code: */
this_board_pci_iospace = ioremap(0xfe240000, 0x10000);

	Arnd

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Daniel Palmer]

> Is this an ARM machine? The platform specific mach/io.h header

Sorry, I should have mentioned it more explicitly.
The machine is an SH4 based (The processor model is SH7751R, board is 
landisk.. in /arch/sh/include/mach-landisk/).

> You have to ioremap() the physical address 0xfe240000 to a virtual
> address, and then add the base address to the number inside of the
> __io macro, e.g.
>
>
>    
-- snip --

Ok, that makes a lot of sense.  This is the first time I've ever touched 
PCI.
I've looked around and I can't right now see where that matches up with 
the SH/board specific code but I'll have a better look later.

We've been trying to work this out for a few days.. we were fudging the 
io port base address in arch/sh/kernel/ioport.c to the start of the PCI 
space. But we didn't think that was the right thing to be doing.
We have another board with the same processor and e100 NIC that uses the 
same mapping to 0x1400 and uses io ports but worked fine. So we guessed 
it was pata_artops use of inb() .. the person that did a lot of changes 
to this board's code recently also wrote that he thought it was 
pata_artops fault when I asked on the linux-sh list.

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Arnd Bergmann]

On Wednesday 30 March 2011 22:24:31 Daniel Palmer wrote:
> > Is this an ARM machine? The platform specific mach/io.h header
> 
> Sorry, I should have mentioned it more explicitly.
> The machine is an SH4 based (The processor model is SH7751R, board is 
> landisk.. in /arch/sh/include/mach-landisk/).
> 

Ok, in that case, you should look at how arch/sh/kernel/ioport.c
handles this. You probably just need to call __set_io_port_base().

	Arnd

Re: PATA_ARTOP reads byte from PCI IO port without mapping it to the right address.

[Alan Cox]

On Wed, 30 Mar 2011 20:48:26 +0100
Daniel Palmer <me@danielpalmer.co.uk> wrote:

> 
> >  That is the PCI port address.
> 
> I don't pretend to understand most of this fully.
> On the board the PCI address space/PCI controller is apparently at 
> 0xfe240000
> So I guess that when that byte is read it should read from 0xfe240000 + 
> 0x1400?
> That doesn't happen and a read happens at 0x1400 and an oops results.
> I don't know enough to point the finger really.
> 
> There are probably only 2 people using this board still too. So I doubt 
> it really matters. :)

That sounds like the inb() function has been broken by a change perhaps
by some related board change - if your I/O is mapped as you describe I'd
expect inb() to be implemented as something like


	return *(volatile u8 *)(0xfe2400 + addr)

Might be worth bisecting between a good and bad release as it ought to be
easy to find

Alan