lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 14 Jun 2019 13:22:31 +0100
From:   John Garry <john.garry@...wei.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     <rjw@...ysocki.net>, <wangkefeng.wang@...wei.com>,
        <lorenzo.pieralisi@....com>, <arnd@...db.de>,
        <linux-pci@...r.kernel.org>, <will.deacon@....com>,
        <linuxarm@...wei.com>, <linux-kernel@...r.kernel.org>,
        <catalin.marinas@....com>, <andriy.shevchenko@...ux.intel.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 1/3] lib: logic_pio: Use logical PIO low-level
 accessors for !CONFIG_INDIRECT_PIO

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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>


Powered by blists - more mailing lists