Re: [PATCH] PCI: Also set up legacy files only after sysfs init
On Wed, Feb 10, 2021 at 10:40 PM Bjorn Helgaas wrote:
>
> On Fri, Feb 05, 2021 at 02:36:32PM +0100, Daniel Vetter wrote:
> > We are already doing this for all the regular sysfs files on PCI
> > devices, but not yet on the legacy io files on the PCI buses. Thus far
> > no problem, but in the next patch I want to wire up iomem revoke
> > support. That needs the vfs up and running already to make sure that
> > iomem_get_mapping() works.
> >
> > Wire it up exactly like the existing code in
> > pci_create_sysfs_dev_files(). Note that pci_remove_legacy_files()
> > doesn't need a check since the one for pci_bus->legacy_io is
> > sufficient.
> >
> > An alternative solution would be to implement a callback in sysfs to
> > set up the address space from iomem_get_mapping() when userspace calls
> > mmap(). This also works, but Greg didn't really like that just to work
> > around an ordering issue when the kernel loads initially.
> >
> > v2: Improve commit message (Bjorn)
> >
> > Signed-off-by: Daniel Vetter
>
> Acked-by: Bjorn Helgaas
>
> I wish we weren't extending a known-racy mechanism to do this, but at
> least we're not *adding* a brand new race.
Yeah it's not great. Thanks for looking at both again, I'll fix up the
typos on the 2nd one and merge them both.
Cheers, Daniel
>
> > Cc: Stephen Rothwell
> > Cc: Jason Gunthorpe
> > Cc: Kees Cook
> > Cc: Dan Williams
> > Cc: Andrew Morton
> > Cc: John Hubbard
> > Cc: Jérôme Glisse
> > Cc: Jan Kara
> > Cc: Dan Williams
> > Cc: Greg Kroah-Hartman
> > Cc: linux...@kvack.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Cc: linux-me...@vger.kernel.org
> > Cc: Bjorn Helgaas
> > Cc: linux-...@vger.kernel.org
> > ---
> > drivers/pci/pci-sysfs.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index fb072f4b3176..0c45b4f7b214 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -927,6 +927,9 @@ void pci_create_legacy_files(struct pci_bus *b)
> > {
> > int error;
> >
> > + if (!sysfs_initialized)
> > + return;
> > +
> > b->legacy_io = kcalloc(2, sizeof(struct bin_attribute),
> > GFP_ATOMIC);
> > if (!b->legacy_io)
> > @@ -1448,6 +1451,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> > static int __init pci_sysfs_init(void)
> > {
> > struct pci_dev *pdev = NULL;
> > + struct pci_bus *pbus = NULL;
> > int retval;
> >
> > sysfs_initialized = 1;
> > @@ -1459,6 +1463,9 @@ static int __init pci_sysfs_init(void)
> > }
> > }
> >
> > + while ((pbus = pci_find_next_bus(pbus)))
> > + pci_create_legacy_files(pbus);
> > +
> > return 0;
> > }
> > late_initcall(pci_sysfs_init);
> > --
> > 2.30.0
> >
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] PCI: Also set up legacy files only after sysfs init
On Fri, Feb 05, 2021 at 02:36:32PM +0100, Daniel Vetter wrote:
> We are already doing this for all the regular sysfs files on PCI
> devices, but not yet on the legacy io files on the PCI buses. Thus far
> no problem, but in the next patch I want to wire up iomem revoke
> support. That needs the vfs up and running already to make sure that
> iomem_get_mapping() works.
>
> Wire it up exactly like the existing code in
> pci_create_sysfs_dev_files(). Note that pci_remove_legacy_files()
> doesn't need a check since the one for pci_bus->legacy_io is
> sufficient.
>
> An alternative solution would be to implement a callback in sysfs to
> set up the address space from iomem_get_mapping() when userspace calls
> mmap(). This also works, but Greg didn't really like that just to work
> around an ordering issue when the kernel loads initially.
>
> v2: Improve commit message (Bjorn)
>
> Signed-off-by: Daniel Vetter
Acked-by: Bjorn Helgaas
I wish we weren't extending a known-racy mechanism to do this, but at
least we're not *adding* a brand new race.
> Cc: Stephen Rothwell
> Cc: Jason Gunthorpe
> Cc: Kees Cook
> Cc: Dan Williams
> Cc: Andrew Morton
> Cc: John Hubbard
> Cc: Jérôme Glisse
> Cc: Jan Kara
> Cc: Dan Williams
> Cc: Greg Kroah-Hartman
> Cc: linux...@kvack.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-me...@vger.kernel.org
> Cc: Bjorn Helgaas
> Cc: linux-...@vger.kernel.org
> ---
> drivers/pci/pci-sysfs.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fb072f4b3176..0c45b4f7b214 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -927,6 +927,9 @@ void pci_create_legacy_files(struct pci_bus *b)
> {
> int error;
>
> + if (!sysfs_initialized)
> + return;
> +
> b->legacy_io = kcalloc(2, sizeof(struct bin_attribute),
> GFP_ATOMIC);
> if (!b->legacy_io)
> @@ -1448,6 +1451,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> static int __init pci_sysfs_init(void)
> {
> struct pci_dev *pdev = NULL;
> + struct pci_bus *pbus = NULL;
> int retval;
>
> sysfs_initialized = 1;
> @@ -1459,6 +1463,9 @@ static int __init pci_sysfs_init(void)
> }
> }
>
> + while ((pbus = pci_find_next_bus(pbus)))
> + pci_create_legacy_files(pbus);
> +
> return 0;
> }
> late_initcall(pci_sysfs_init);
> --
> 2.30.0
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH] PCI: Also set up legacy files only after sysfs init
Hi Bjorn,
Can you ack this for merging through my topic branch with the other
follow_pfn/iomem revoke fixes for 5.12?
If not, what's the plan for getting this (or equivalent functionality)
in for 5.13? I have more of these follow_pfn/iomem revoke patches on
top, so I'd like to get the first cut merged sooner than later if
possible. And the other prep work has been in -next since -rc1.
Thanks, Daniel
On Fri, Feb 5, 2021 at 2:36 PM Daniel Vetter wrote:
>
> We are already doing this for all the regular sysfs files on PCI
> devices, but not yet on the legacy io files on the PCI buses. Thus far
> no problem, but in the next patch I want to wire up iomem revoke
> support. That needs the vfs up and running already to make sure that
> iomem_get_mapping() works.
>
> Wire it up exactly like the existing code in
> pci_create_sysfs_dev_files(). Note that pci_remove_legacy_files()
> doesn't need a check since the one for pci_bus->legacy_io is
> sufficient.
>
> An alternative solution would be to implement a callback in sysfs to
> set up the address space from iomem_get_mapping() when userspace calls
> mmap(). This also works, but Greg didn't really like that just to work
> around an ordering issue when the kernel loads initially.
>
> v2: Improve commit message (Bjorn)
>
> Signed-off-by: Daniel Vetter
> Cc: Stephen Rothwell
> Cc: Jason Gunthorpe
> Cc: Kees Cook
> Cc: Dan Williams
> Cc: Andrew Morton
> Cc: John Hubbard
> Cc: Jérôme Glisse
> Cc: Jan Kara
> Cc: Dan Williams
> Cc: Greg Kroah-Hartman
> Cc: linux...@kvack.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-me...@vger.kernel.org
> Cc: Bjorn Helgaas
> Cc: linux-...@vger.kernel.org
> ---
> drivers/pci/pci-sysfs.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fb072f4b3176..0c45b4f7b214 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -927,6 +927,9 @@ void pci_create_legacy_files(struct pci_bus *b)
> {
> int error;
>
> + if (!sysfs_initialized)
> + return;
> +
> b->legacy_io = kcalloc(2, sizeof(struct bin_attribute),
>GFP_ATOMIC);
> if (!b->legacy_io)
> @@ -1448,6 +1451,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
> static int __init pci_sysfs_init(void)
> {
> struct pci_dev *pdev = NULL;
> + struct pci_bus *pbus = NULL;
> int retval;
>
> sysfs_initialized = 1;
> @@ -1459,6 +1463,9 @@ static int __init pci_sysfs_init(void)
> }
> }
>
> + while ((pbus = pci_find_next_bus(pbus)))
> + pci_create_legacy_files(pbus);
> +
> return 0;
> }
> late_initcall(pci_sysfs_init);
> --
> 2.30.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
