Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1613548 > unrolled thread

Re: [PATCH v2 3/5] PCI: rockchip: add remove() support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Bjorn Helgaas <helgaas@kernel.org> - 2017-03-31 01:30 +0200
    Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Brian Norris <briannorris@chromium.org> - 2017-03-31 02:30 +0200
      Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Bjorn Helgaas <helgaas@kernel.org> - 2017-03-31 07:20 +0200
        Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Brian Norris <briannorris@chromium.org> - 2017-03-31 18:50 +0200

#1613548 — Re: [PATCH v2 3/5] PCI: rockchip: add remove() support

On Fri, Mar 24, 2017 at 10:22:19AM -0700, Brian Norris wrote:
> Hi Bjorn,
> 
> On Fri, Mar 24, 2017 at 09:25:41AM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 09, 2017 at 06:46:15PM -0800, Brian Norris wrote:
> > > Currently, if we try to unbind the platform device, the remove will
> > > succeed, but the removal won't undo most of the registration, leaving
> > > partially-configured PCI devices in the system.
> > > 
> > > This allows, for example, a simple 'lspci' to crash the system, as it
> > > will try to touch the freed (via devm_*) driver structures.
> > > 
> > > So let's implement device remove().
> > 
> > How exactly do you reproduce this problem?
> 
> On RK3399:
> 
>   # echo f8000000.pcie > /sys/bus/platform/drivers/rockchip-pcie/unbind
>   # lspci
> 
> > There are several other drivers that are superficially similar, e.g.,
> > they define a struct platform_driver without a .remove method.  Do
> > they all have this problem?  Some of them do set .suppress_bind_attrs
> > = true; is that relevant to this scenario?
> 
> Yes, I think .suppress_bind_attrs would be enough to prevent this,
> according to my reading of the code and comments:
> 
>  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> 
> > In fact, the only other callers of pci_remove_root_bus() are
> > iproc_pcie_remove(), hv_pci_remove(), and vmd_remove().
> 
> Then iProc would suffer from the same memory leak in
> of_pci_get_host_bridge_resources() [1]. It *would* suffer from the same
> domain allocation issues in of_pci_bus_find_domain_nr() ->
> pci_get_new_domain_nr() [2], except that all iProc device trees (in
> mainline at least) use the 'linux,pci-domain' property to avoid it.
> 
> HyperV and VMD drivers use ACPI, which uses neither
> pci_get_new_domain_nr() nor of_pci_get_host_bridge_resources().
> 
> > These don't have .remove:
> > 
> >   imx6_pcie_driver
> >   ls_pcie_driver
> >   armada8k_pcie_driver
> >   artpec6_pcie_driver
> >   dw_plat_pcie_driver
> >   hisi_pcie_driver
> >   hisi_pcie_almost_ecam_driver
> >   spear13xx_pcie_driver
> >   gen_pci_driver
> 
> I think these are all technically broken.

Can we fix them all at the same time as you fix Rockchip?  Maybe we
should have a series that adds ".suppress_bind_attrs = true" to all
these drivers, including Rockchip.  Then you could have this current 
series to make Rockchip modular on top, if there's still value in it.

If we find a common problem, I'd like to fix it everywhere we know
about so it doesn't get forgotten or copied to even more places.

> > These don't have .remove but do set .suppress_bind_attrs = true:
> > 
> >   dra7xx_pcie_driver
> >   qcom_pcie_driver
> >   advk_pcie_driver
> >   mvebu_pcie_driver
> >   rcar_pci_driver
> >   rcar_pcie_driver
> >   tegra_pcie_driver
> >   altera_pcie_driver
> >   nwl_pcie_driver
> >   xilinx_pcie_driver
> 
> Those are fine then, I suppose.
> 
> Brian
> 
> [1] PCI: return resource_entry in pci_add_resource helpers
>     https://patchwork.kernel.org/patch/9642229/
>     of/pci: Fix memory leak in of_pci_get_host_bridge_resources
>     https://patchwork.kernel.org/patch/9642231/
> 
> [2] PCI: use IDA to manage domain number if not getting it from DT
>     https://patchwork.kernel.org/patch/9638353/

[toc] | [next] | [standalone]

#1613563

Hi Bjorn,

On Thu, Mar 30, 2017 at 06:28:25PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 24, 2017 at 10:22:19AM -0700, Brian Norris wrote:
> > On Fri, Mar 24, 2017 at 09:25:41AM -0500, Bjorn Helgaas wrote:
> > > These don't have .remove:
> > > 
> > >   imx6_pcie_driver
> > >   ls_pcie_driver
> > >   armada8k_pcie_driver
> > >   artpec6_pcie_driver
> > >   dw_plat_pcie_driver
> > >   hisi_pcie_driver
> > >   hisi_pcie_almost_ecam_driver
> > >   spear13xx_pcie_driver
> > >   gen_pci_driver
> > 
> > I think these are all technically broken.
> 
> Can we fix them all at the same time as you fix Rockchip?  Maybe we
> should have a series that adds ".suppress_bind_attrs = true" to all
> these drivers,

Sure, I can do that.

> including Rockchip.

Huh? Why? So I can revert that in the next patch?

> Then you could have this current 
> series to make Rockchip modular on top, if there's still value in it.

I do see value in it. That's the whole reason I wrote this patchset.
It's useful for stressing out certain behaviors that will happen all the
time (i.e., boot-time initialization, from platform probe, to bus init,
to client/EP init), via repeated bind/unbind (or modprobe/rmmod). It's
much faster than reboot testing.

Personally, I'd rather just patch the other drivers, and you can wait
until I follow through on that promise before applying my existing work
for the Rockchip driver, if that's what you'd prefer.

> If we find a common problem, I'd like to fix it everywhere we know
> about so it doesn't get forgotten or copied to even more places.

Sure. But you only just pointed out how broken several drivers were; I
didn't really notice :)

Brian

[toc] | [prev] | [next] | [standalone]

#1613640

On Thu, Mar 30, 2017 at 05:26:09PM -0700, Brian Norris wrote:
> Hi Bjorn,
> 
> On Thu, Mar 30, 2017 at 06:28:25PM -0500, Bjorn Helgaas wrote:
> > On Fri, Mar 24, 2017 at 10:22:19AM -0700, Brian Norris wrote:
> > > On Fri, Mar 24, 2017 at 09:25:41AM -0500, Bjorn Helgaas wrote:
> > > > These don't have .remove:
> > > > 
> > > >   imx6_pcie_driver
> > > >   ls_pcie_driver
> > > >   armada8k_pcie_driver
> > > >   artpec6_pcie_driver
> > > >   dw_plat_pcie_driver
> > > >   hisi_pcie_driver
> > > >   hisi_pcie_almost_ecam_driver
> > > >   spear13xx_pcie_driver
> > > >   gen_pci_driver
> > > 
> > > I think these are all technically broken.
> > 
> > Can we fix them all at the same time as you fix Rockchip?  Maybe we
> > should have a series that adds ".suppress_bind_attrs = true" to all
> > these drivers,
> 
> Sure, I can do that.
> 
> > including Rockchip.
> 
> Huh? Why? So I can revert that in the next patch?
> 
> > Then you could have this current 
> > series to make Rockchip modular on top, if there's still value in it.
> 
> I do see value in it. That's the whole reason I wrote this patchset.
> It's useful for stressing out certain behaviors that will happen all the
> time (i.e., boot-time initialization, from platform probe, to bus init,
> to client/EP init), via repeated bind/unbind (or modprobe/rmmod). It's
> much faster than reboot testing.

I didn't phrase that very well.  There's certainly value in stressing
the bind/unbind paths, but I thought the primary reason you wrote this
was to fix the fact that you could crash the system like this:

  # echo f8000000.pcie > /sys/bus/platform/drivers/rockchip-pcie/unbind
  # lspci

From my point of view, that's the issue that *has* to be fixed.
Better test coverage is icing.

It sounds like several drivers have that same issue, and the simplest
possible fix is to set .suppress_bind_attrs, so I suggested doing that 
so it's easy to analyze the tree as a whole and say "these drivers
all have the same problem, and all the fixes look the same."

I guess if you'd rather skip that for Rockchip and apply a more
complicated fix there, I could go along with that.  But I don't think
it would hurt anything to set .suppress_bind_attrs, then remove it
when you add module support.  The concepts of .suppress_bind_attrs and
modularity are related, and doing this in a separate patch would make
it a nice example to follow if somebody wants to make other drivers
modular as well.

> Personally, I'd rather just patch the other drivers, and you can wait
> until I follow through on that promise before applying my existing work
> for the Rockchip driver, if that's what you'd prefer.

It's not so much a question of using the Rockchip change as a stick.
I'm just thinking that it makes a more logical progression to fix the
more important issue globally first.

> > If we find a common problem, I'd like to fix it everywhere we know
> > about so it doesn't get forgotten or copied to even more places.
> 
> Sure. But you only just pointed out how broken several drivers were; I
> didn't really notice :)

Yeah, you're right, I had in my head the idea that if we've identified
the same problem in several drivers, we should fix them all, but I
neglected to turn that into words.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1614151

Hi Bjorn,

On Fri, Mar 31, 2017 at 12:17:02AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 30, 2017 at 05:26:09PM -0700, Brian Norris wrote:
> > On Thu, Mar 30, 2017 at 06:28:25PM -0500, Bjorn Helgaas wrote:
> > > Can we fix them all at the same time as you fix Rockchip?  Maybe we
> > > should have a series that adds ".suppress_bind_attrs = true" to all
> > > these drivers,
> > 
> > Sure, I can do that.
> > 
> > > including Rockchip.
> > 
> > Huh? Why? So I can revert that in the next patch?
> > 
> > > Then you could have this current 
> > > series to make Rockchip modular on top, if there's still value in it.
> > 
> > I do see value in it. That's the whole reason I wrote this patchset.
> > It's useful for stressing out certain behaviors that will happen all the
> > time (i.e., boot-time initialization, from platform probe, to bus init,
> > to client/EP init), via repeated bind/unbind (or modprobe/rmmod). It's
> > much faster than reboot testing.
> 
> I didn't phrase that very well.  There's certainly value in stressing
> the bind/unbind paths, but I thought the primary reason you wrote this
> was to fix the fact that you could crash the system like this:
> 
>   # echo f8000000.pcie > /sys/bus/platform/drivers/rockchip-pcie/unbind
>   # lspci

Well, they're kinda two sides of the same coin; I was wanting to test
the bind path, and when I tried this, I noticed that I could trivially
crash the system. The crash seemed like a more important thing to
document (because otherwise, it just looks like I'm adding a feature).

> From my point of view, that's the issue that *has* to be fixed.
> Better test coverage is icing.

I didn't really view messing with /sys/.../unbind as a big issue,
outside of development and testing (there's a lot of damage a malicious
actor can do with unconstrained access to /sys/), so I guess I didn't
put that aspect as super-high priority. If you'd like to prioritize
that, then I'm OK with that.

> It sounds like several drivers have that same issue, and the simplest
> possible fix is to set .suppress_bind_attrs, so I suggested doing that 
> so it's easy to analyze the tree as a whole and say "these drivers
> all have the same problem, and all the fixes look the same."

Sure, that is the simplest approach.

> I guess if you'd rather skip that for Rockchip and apply a more
> complicated fix there, I could go along with that.  But I don't think
> it would hurt anything to set .suppress_bind_attrs, then remove it
> when you add module support.  The concepts of .suppress_bind_attrs and
> modularity are related, and doing this in a separate patch would make
> it a nice example to follow if somebody wants to make other drivers
> modular as well.

I'll leave that up to you, and I can resubmit things if desired. As you
have since noticed, I already sent a patch to add .suppress_bind_attrs
to all the other drivers. If you'd like, feel free to add
pcie-rockchip.c into that mix, it's not hard -- or I can redo it myself.
Then I can modify and resend (or you can do the trivial modification
required to) the current patch set.

Just let me know.

> > Personally, I'd rather just patch the other drivers, and you can wait
> > until I follow through on that promise before applying my existing work
> > for the Rockchip driver, if that's what you'd prefer.
> 
> It's not so much a question of using the Rockchip change as a stick.
> I'm just thinking that it makes a more logical progression to fix the
> more important issue globally first.

Sure, I can grok that. Just let me know if you want any more action from
me.

Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web