Message-ID: <47744c7bce7b7bb37edee7f249d61dc57ac1fbc5.camel@linux.ibm.com>
Subject: Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Arnd Bergmann <arnd@kernel.org>, John Garry <john.garry@huawei.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        linux-arch <linux-arch@vger.kernel.org>,
        linux-pci <linux-pci@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Date:   Fri, 17 Dec 2021 14:19:17 +0100
In-Reply-To: <CAK8P3a3KTaa-AwCOjhaASMx63B3DUBZCZe6RKWk-=Qu7xr_ijQ@mail.gmail.com>
References: <CAK8P3a2oZ-+qd3Nhpy9VVXCJB3DU5N-y-ta2JpP0t6NHh=GVXw@mail.gmail.com>
         <CAHk-=wg80je=K7madF4e7WrRNp37e3qh6y10Svhdc7O8SZ_-8g@mail.gmail.com>
         <CAK8P3a1D5DzmNGsEPQomkyMCmMrtD6pQ11JRMh78vbY53edp-Q@mail.gmail.com>
         <CAK8P3a0MNbx-iuzW_-=0ab6-TTZzwV-PT_6gAC1Gp5PgYyHcrA@mail.gmail.com>
         <db043b76-880d-5fad-69cf-96abcd9cd34f@huawei.com>
         <CAK8P3a3HHeP+Gw_k2P7Qtig0OmErf0HN30G22+qHic_uZTh11Q@mail.gmail.com>
         <a74dfb1f-befd-92ce-4c30-233cb08e04d3@huawei.com>
         <CAK8P3a3B4FCaPPHhzBdpkv0fsjE0jREwGFCdPeHEDHxxRBEjng@mail.gmail.com>
         <5e8dfbd2-a6c0-6d02-53e9-1f29aebcc44e@huawei.com>
         <CAK8P3a08Zcyx0J4_LGAfU_AtUyEK+XtQJxYBQ52VXfWu8-o8_w@mail.gmail.com>
         <dd2d49ef-3154-3c87-67b9-c134567ba947@huawei.com>
         <CAK8P3a3KTaa-AwCOjhaASMx63B3DUBZCZe6RKWk-=Qu7xr_ijQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Tue, 2021-08-10 at 13:33 +0200, Arnd Bergmann wrote:
> On Tue, Aug 10, 2021 at 11:19 AM John Garry <john.garry@huawei.com> wrote:
> > On 04/08/2021 09:52, Arnd Bergmann wrote:
> > 
> > This seems a reasonable approach. Do you have a plan for this work? Or
> > still waiting for the green light?
> 
> I'm rather busy with other work at the moment, so no particular plans
> for any time soon.
> 
> > I have noticed the kernel test robot reporting the following to me,
> > which seems to be the same issue which was addressed in this series
> > originally:
> > 
> > config: s390-randconfig-r032-20210802 (attached as .config)
> > compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
> > 4f71f59bf3d9914188a11d0c41bedbb339d36ff5)
> > ...
> > All errors (new ones prefixed by >>):
> > 
> >     In file included from drivers/block/null_blk/main.c:12:
> >     In file included from drivers/block/null_blk/null_blk.h:8:
> >     In file included from include/linux/blkdev.h:25:
> >     In file included from include/linux/scatterlist.h:9:
> >     In file included from arch/s390/include/asm/io.h:75:
> >     include/asm-generic/io.h:464:31: warning: performing pointer
> > arithmetic on a null pointer has undefined behavior
> > [-Wnull-pointer-arithmetic]
> >             val = __raw_readb(PCI_IOBASE + addr);
> > 
> > So I imagine lots of people are seeing these.
> 
> Right, this is the original problem that Niklas was trying to solve.
> 
> If Niklas has time to get this fixed, I can probably find a way to work
> with him on finishing up my proposed patch with the changes you
> suggested.
> 
>        Arnd

Hi Arnd, Hi John,

I've had some time to look into this a bit. As a refreshed starting
point I have rebased Arnd's patch to v5.16-rc5. Since I'm not sure how
to handle authorship and it's very early I haven't sent it as RFC but
it's available as a patch from my GitHub here: 
https://gist.github.com/niklas88/a08fe76bdf9f5798500fccea6583e275

I have incorporated the following findings from this thread already:

- Added HAS_IOPORT to arch Kconfigs
- Added "config LEGACY_PCI" to drivers/pci/Kconfig
- Fixed CONFIG_HAS_IOPORT typo in asm-generic/io.h
- Removed LEGACY_PCI dependency of i2c-i801.
  Which is also used in current gen Intel platforms
  and depends on x86 anyway.

I have tested this on s390 with HAS_IOPORT=n and allyesconfig as well
as running it with defconfig. I've also been using it on my Ryzen 3990X
workstation with LEGACY_PCI=n for a few days. I do get about 60 MiB
fewer modules compared with a similar config of v5.15.8. Hard to say
which other systems might miss things of course.

I have not yet worked on the discussed IOPORT_NATIVE flag. Mostly I'm
wondering two things. For one it feels like that could be a separate
change on top since HAS_IOPORT + LEGACY_PCI is already quite big.
Secondly I'm wondering about good ways of identifying such drivers and
how much this overlaps with the ISA config flag.

I'd of course appreciate feedback. If you agree this is still
worthwhile to persue I'd think the next step would be trying to
refactor this into more manageable patches.

Thanks,
Niklas