linux-kernel - Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity

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: <87sg91ik9e.wl-maz@kernel.org>
Date:   Sun, 22 Nov 2020 13:38:53 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     John Garry <john.garry@...wei.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <martin.petersen@...cle.com>,
        <jejb@...ux.ibm.com>, <linuxarm@...wei.com>,
        <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc()

On Fri, 20 Nov 2020 11:52:09 +0000,
John Garry <john.garry@...wei.com> wrote:
> 
> Hi Thomas,
> 
> >> Just mentioning a couple of things here, which could be a clue to what
> >> is going on:
> >> - the device is behind mbigen secondary irq controller
> >> - the flow in the LLDD is to allocate all 128 interrupts during probe,
> >> but we only register handlers for a subset with device managed API
> > Right, but if the driver is removed then the interrupts should be
> > deallocated, right?
> > 
> 
> When removing the driver we just call free_irq(), which removes the
> handler and disables the interrupt.
> 
> But about the irq_desc, this is created when the mapping is created in
> irq_create_fwspec_mapping(), and I don't see this being torn down in
> the driver removal, so persistent in that regard.

If the irq_descs are created via the platform_get_irq() calls in
platform_get_irqs_affinity(), I'd expect some equivalent helper to
tear things down as a result, calling irq_dispose_mapping() behind the
scenes.

> So for pci msi I can see that we free the irq_desc in
> pci_disable_msi() -> free_msi_irqs() -> msi_domain_free_irqs() ...
> 
> So what I am missing here?

I'm not sure the paths are strictly equivalent. On the PCI side, we
can have something that completely driver agnostic, as it is all
architectural. In your case, only the endpoint driver knows about what
happens, and needs to free things accordingly.

Finally, there is the issue in your driver that everything is
requested using devm_request_irq, which cannot play nicely with an
explicit irq_desc teardown. You'll probably need to provide the
equivalent devm helpers for your driver to safely be taken down.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.