linux-kernel - Re: [RFC 3/3] PCI: tegra: Support driver unbinding

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130819201623.GD5191@mithrandir>
Date:	Mon, 19 Aug 2013 22:16:24 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Russell King <linux@....linux.org.uk>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Jason Cooper <jason@...edaemon.net>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	Thierry Reding <treding@...dia.com>
Subject: Re: [RFC 3/3] PCI: tegra: Support driver unbinding

On Thu, Aug 15, 2013 at 09:21:53AM -0600, Stephen Warren wrote:
> On 08/15/2013 04:34 AM, Thierry Reding wrote:
> > On Wed, Aug 14, 2013 at 03:43:40PM -0600, Stephen Warren wrote:
> >> On 08/13/2013 05:12 AM, Thierry Reding wrote:
> >>> Implement the platform driver's .remove() callback to free all
> >>> resources allocated during driver setup and call
> >>> pci_common_exit() to cleanup ARM specific datastructures. Unmap
> >>> the fixed PCI I/O mapping by calling the new pci_iounmap_io()
> >>> function in the new .teardown() callback.
> >>> 
> >>> Finally, no longer set the .suppress_bind_attrs field to true
> >>> to allow the driver to unbind from a device.
> >> 
> >>> +static int tegra_pcie_remove(struct platform_device *pdev) +{ 
> >>> +	struct tegra_pcie *pcie = platform_get_drvdata(pdev); +
> >>> struct tegra_pcie_bus *bus, *tmp; +	int err; + +
> >>> pci_common_exit(&pcie->sys); + +	list_for_each_entry_safe(bus,
> >>> tmp, &pcie->busses, list) { +		vunmap(bus->area->addr); +
> >>> kfree(bus); +	} + +	if (IS_ENABLED(CONFIG_PCI_MSI)) { +		err =
> >>> tegra_pcie_disable_msi(pcie); +		if (err < 0) +			return err; +
> >>> }
> >> 
> >> Wouldn't it make sense to do that as early as possible in the
> >> function, to make sure that no MSI accidentally fires after some
> >> of the cleanup has already happened?
> > 
> > I don't think that's strictly necessary in this case. After the
> > call to pci_common_exit() there are no PCI devices left, there's
> > not even a bus left. All MSI users should have cleaned up after
> > themselves.
> > 
> > Given that I thought it more useful to mirror the setup done in
> > .probe() to make it clearer what's being undone (and potentially
> > what's missing).
> 
> That makes sense SW-wise, but what about mis-behaving HW that triggers
> an MSI even when it's been told not to? I assume that
> tegra_pcie_disable_msi() unrequests the IRQ, hence solves that
> problem, if done early enough.

To be honest, I'm not sure about the side-effects that this will have.
tegra_pcie_disable_msi() does quite a bit more than just masking the
interrupts. It also completely removes the IRQ domain that provides the
MSI interrupts. While I haven't tried it yet I can imagine that it will
cause crashes at a later point when drivers want to disable MSI on a
device and the IRQ domain having vanished from underneath.

Thierry

Content of type "application/pgp-signature" skipped