Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

On 14/06/2019 12:50, Bjorn Helgaas wrote:
> On Fri, Jun 14, 2019 at 10:02:52AM +0100, John Garry wrote:
> > On 13/06/2019 21:09, Bjorn Helgaas wrote:
> > > On Thu, Jun 13, 2019 at 10:39:12AM +0100, John Garry wrote:
> > > > On 13/06/2019 03:39, Bjorn Helgaas wrote:
> > > > > I'm not sure it's even safe, because CONFIG_INDIRECT_PIO depends on
> > > > > ARM64,  but PCI_IOBASE is defined on most arches via asm-generic/io.h,
> > > > > so this potentially affects arches other than ARM64.
> > > >
> > > > It would do. It would affect any arch which defines PCI_IOBASE and
> > > > does not have arch-specific definition of inb et all.
> >
> > > What's the reason for testing PCI_IOBASE instead of
> > > CONFIG_INDIRECT_PIO?  If there's a reason it's needed, that's fine,
> > > but it does make this much more complicated to review.
> >
> > For ARM64, we have PCI_IOBASE defined but may not have
> > CONFIG_INDIRECT_PIO defined. Currently CONFIG_INDIRECT_PIO is only
> > selected by CONFIG_HISILICON_LPC.
> >
> > As such, we should make this change also for when
> > CONFIG_INDIRECT_PIO is not defined.

Hi Bjorn,

> OK.  This is all very important for the commit log -- we need to
> understand what arches are affected and the reason they need it.

Right, and to repeat, this would affect other archs which define 
PCI_IOBASE and don't have custom IO port accessors definitions.

There are a few remaining even after the recent arch clear out . I have 
it at arm64, microblaze, and unicore32. Arch m68k defines PCI_IOBASE but 
seems to have its own IO port accessors. Same again for some arm machines.

At least I should cc those arch maintainers.

> Since the goal of this series is to fix an ARM64-specific issue,

"ARM64" was in the headline banner, but it would apply to other archs, 
as mentioned above. I should have made that clearer.

and
> the typical port I/O model is for each arch to #define its own inb(),
> maybe it would make sense to move the "#define inb logic_inb" from
> linux/logic_pio.h to arm64/include/asm/io.h?

CONFIG_INDIRECT_PIO has been indirectly enabled in ARM64 defconfig for 
some time, so I think that it's ok for ARM64 arch Kconfig to select it 
at this stage. From that, we could make the change suggested.

And, in addition to that, we can make the change in this series just for 
CONFIG_INDIRECT_PIO.

But I think that the other archs, above, could benefit from the changes 
in this series, so it would be shame to omit them.

> The "#ifndef inb" arrangement gets pretty complicated when it occurs
> more than one place (asm-generic/io.h and logic_pio.h) and we have to
> start worrying about the ordering of #includes.

I agree on that.

Cheers,
John

> Bjorn
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel