linux-kernel - Re: [PATCH] parport_pc: Also enable driver for PCI systems

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 linux-hardening linux-cve-announce 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]

Message-ID: <alpine.DEB.2.21.2202142018060.34636@angie.orcam.me.uk>
Date:   Mon, 14 Feb 2022 20:42:05 +0000 (GMT)
From:   "Maciej W. Rozycki" <macro@...am.me.uk>
To:     Niklas Schnelle <schnelle@...ux.ibm.com>
cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Guo Ren <guoren@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Jeff Dike <jdike@...toit.com>,
        Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Chris Zankel <chris@...kel.net>,
        Max Filippov <jcmvbkbc@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-csky@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
        linux-um@...ts.infradead.org, linux-xtensa@...ux-xtensa.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] parport_pc: Also enable driver for PCI systems

On Mon, 14 Feb 2022, Niklas Schnelle wrote:

> > While older versions of the driver did have to be explicitly configured 
> > for MMIO rather than port I/O, a feature added with commit e89a2cfb7d7b 
> > ("[TC] defxx: TURBOchannel support"), the driver has been improved with 
> > commit 795e272e5474 ("FDDI: defxx: Implement dynamic CSR I/O address space 
> > selection") and the selection of the I/O space to use now fully automatic.
> 
> Very interesting and thanks for the input! On s390 we really only have
> very few different PCI devices and I can only test another hand full
> with my private x86 and ARM systems.

 Note that for TURBOchannel support, which is likewise MMIO only, the 
driver has this:

#if defined(CONFIG_EISA) || defined(CONFIG_PCI)
#define dfx_use_mmio bp->mmio
#else
#define dfx_use_mmio true
#endif

so if your proposal to add HAS_IOPORT goes forward it'll be enough if we 
update the condition to:

#if defined(CONFIG_HAS_IOPORT)

or maybe even rewrite the entire piece as:

#define dfx_use_mmio (!IS_ENABLED(CONFIG_HAS_IOPORT) || bp->mmio)

and all the port I/O stuff will be optimised away by the compiler.  The 
only part of the driver that actually cannot do without port I/O is EISA 
support, which uses the EISA slot port I/O space for BAR accesses even 
if the actual CSR block has been set up to be decoded in the MMIO space.

> >  Then what about the other FDDI driver there, SKFP?  It's not marked as
> > LEGACY_PCI, although it's not selectable anyway due to the dependency of 
> > FDDI on LEGACY_PCI.
> > 
> >  Niklas, what was the criterion for placing the LEGACY_PCI dependency?
> 
> Hmm, honestly I haven't really worked on this recently. There were some
> open questions from Bjorn towards Arnd and I was waiting for his reply
> but I guess he missed those. I think what you noticed was the main
> problem, there wasn't really a clear set of criteria for LEGACY_PCI and
> even for HAS_IOPORT we missed some uses if they were not compiled on
> s390's allyesconfig due to other dependencies.

 A dynamic boolean variable might be good having for platforms which may 
or may not have PCI port I/O available depending on the specific system 
model in addition to a compile-time constant of HAS_IOPORT.  I looked 
into it briefly in the context of the POWER9 system when I got it back 
in 2020, but figured out it wasn't straightforward enough and decided I 
could not afford the time for a proper investigation.

> >  Also do you plan to post an updated series anytime soon?  I'm asking 
> > because like with the m68k port also the MIPS one needs a more finegrained 
> > approach and I suspect there may be other corner cases and I'd rather look 
> > at the most recent version of your series.  Otherwise I'll have a look 
> > through your original submission, but it may have to wait until the next 
> > weekend due to my other commitments.
> 
> That sounds like you do see a need for something like HAS_IOPORT too,
> correct? Maybe with some input what you need and possibly stripping the
> LEGACY_PCI option it might make sense to do a new version. Rather than
> possibly getting in your way could directly work in your input.

 Yes, it does seem to me like a good direction, but will surely require 
some coordination from platform and driver maintainers, as it's not 
always easy for someone not familiar with a specific piece what the 
context is (such as with the defxx driver as I noted above).

  Maciej