From: Bjorn Helgaas <helgaas@kernel.org> To: Brian Norris <briannorris@chromium.org> Cc: Bjorn Helgaas <bhelgaas@google.com>, linux-kernel@vger.kernel.org, Shawn Lin <shawn.lin@rock-chips.com>, Jeffy Chen <jeffy.chen@rock-chips.com>, Wenrui Li <wenrui.li@rock-chips.com>, linux-pci@vger.kernel.org, linux-rockchip@lists.infradead.org, Ray Jui <rjui@broadcom.com> Subject: Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Date: Thu, 30 Mar 2017 18:28:25 -0500 [thread overview] Message-ID: <20170330232825.GB3912@bhelgaas-glaptop.roam.corp.google.com> (raw) In-Reply-To: <20170324172218.GA119093@google.com> 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/
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> To: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Cc: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>, Wenrui Li <wenrui.li-TNX95d0MmH7DzftRWevZcw@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>, Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Subject: Re: [PATCH v2 3/5] PCI: rockchip: add remove() support Date: Thu, 30 Mar 2017 18:28:25 -0500 [thread overview] Message-ID: <20170330232825.GB3912@bhelgaas-glaptop.roam.corp.google.com> (raw) In-Reply-To: <20170324172218.GA119093-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> 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/
next prev parent reply other threads:[~2017-03-30 23:29 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-10 2:46 [PATCH v2 1/5] PCI: rockchip: fix sign issues for current limits Brian Norris 2017-03-10 2:46 ` Brian Norris 2017-03-10 2:46 ` [PATCH v2 2/5] PCI: rockchip: make 'return 0' more obvious in probe() Brian Norris 2017-03-10 2:46 ` Brian Norris 2017-03-10 2:46 ` [PATCH v2 3/5] PCI: rockchip: add remove() support Brian Norris 2017-03-10 2:46 ` Brian Norris 2017-03-10 3:22 ` Shawn Lin 2017-03-10 4:20 ` Shawn Lin 2017-03-10 19:40 ` Brian Norris 2017-03-10 19:40 ` Brian Norris 2017-03-13 2:26 ` Shawn Lin 2017-03-20 22:29 ` Brian Norris 2017-03-20 22:29 ` Brian Norris 2017-03-24 14:25 ` Bjorn Helgaas 2017-03-24 17:22 ` Brian Norris 2017-03-30 23:28 ` Bjorn Helgaas [this message] 2017-03-30 23:28 ` Bjorn Helgaas 2017-03-31 0:26 ` Brian Norris 2017-03-31 5:17 ` Bjorn Helgaas 2017-03-31 5:17 ` Bjorn Helgaas 2017-03-31 16:40 ` Brian Norris 2017-04-11 18:18 ` Brian Norris 2017-03-10 2:46 ` [PATCH v2 4/5] PCI: export pci_remap_iospace() and pci_unmap_iospace() Brian Norris 2017-03-10 2:46 ` Brian Norris 2017-03-10 2:46 ` [PATCH v2 5/5] PCI: rockchip: modularize Brian Norris 2017-03-10 2:46 ` Brian Norris 2017-03-23 22:27 ` [PATCH v2 1/5] PCI: rockchip: fix sign issues for current limits Bjorn Helgaas 2017-03-23 22:33 ` Brian Norris 2017-03-24 1:24 ` Shawn Lin 2017-04-21 19:03 ` Bjorn Helgaas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170330232825.GB3912@bhelgaas-glaptop.roam.corp.google.com \ --to=helgaas@kernel.org \ --cc=bhelgaas@google.com \ --cc=briannorris@chromium.org \ --cc=jeffy.chen@rock-chips.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=rjui@broadcom.com \ --cc=shawn.lin@rock-chips.com \ --cc=wenrui.li@rock-chips.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.