Re: [PATCH 1/5] irqchip/gic-pm: add driver remove support

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3253 bytes --]

On Wed, Mar 13, 2019 at 02:20:41PM +0000, Marc Zyngier wrote:
> On 13/03/2019 13:50, Sameer Pujar wrote:
> > 
> > On 3/13/2019 4:52 PM, Marc Zyngier wrote:
> >> First things first:
> >>
> >> - Where is the cover letter?
> >> - This series should be flagged as v2, as it not the same as the one you
> >> sent last week.
> > I had the dilemma whether to name this series as v2 or not, thought the 
> > commits
> > in the series are different and v2 may not be necessary.
> 
> This is an iteration on the same theme. Please always bump up the
> counter. Better do it more often than not.
> 
> > Also felt commit messages are descriptive enough and all belong to 
> > irq-gic-pm,
> > hence did not send cover letter.
> > If you suggest so, I will send a cover letter next patch version(v2)
> 
> You should always send a cover letter if you have more than a single patch.
> 
> >>
> >> On 13/03/2019 11:02, Sameer Pujar wrote:
> >>> This is a preparatory patch for using irq-gic-pm driver as module and thus
> >>> implement remove() call for the driver. Details of remove() are as below,
> >>>
> >>>   * pm_runtime_force_suspend() is added to balance runtime PM, otherwise
> >>>     following is seen: "agic-controller: Unbalanced pm_runtime_enable!"
> >>>   * Function gic_teardown() is exported from gic driver and called in remove
> >>>     to perform io unmap.
> >>>   * pm_clk_destroy() to free clock resources
> >>>   * irq is unmapped and freed with irq_dispose_mapping()
> >>>
> >> Let's be clear, I have no desire to export any GIC symbol at all. Why
> >> should we do this? This "driver" is the tiniest thing, and making it
> >> modular doesn't get us anything.
> >>
> >> So what's the rational for doing so?
> > Reason for this was, the driver gets used for AGIC block and audio is not
> > boot critical and hence module option was preferred.
> 
> Sure, but look at the result:
> 
> - you remove your gic-pm module
> - the MMIO mapping disappears
> - the GIC data structures *are still live*
> - a driver does a disable_irq() on an interrupt routed to this block
> (because nothing has taken the interrupts away, as far as the kernel is
> concerned)
> - ...
> - profit! (or kernel panic, your choice)

I suppose we could use device links to model the dependency, but perhaps
it's not worth it for something like the GIC. The AGIC is somewhat of an
outlier because it serves a fairly encapsulated part of the system, so
it would be manageable to make it unloadable.

> Even better if something else in the system has mapped anything that
> ends up in the same vmalloc range. Congratulations, you have now
> corrupted unsuspecting memory. This reminds me of the e1000 corruption
> bug. Great stuff.

Can you elaborate on this point? How would unloading a driver cause
memory corruption in another driver's mapped memory? I've never heard of
this before, so I want to make sure I understand what to look out for in
the future.

> So for the whole thing, NAK. You don't pull an irqchip from under the
> kernel's feet.

Maybe we should also set the driver's .suppress_bind_attrs = true while
at it, to prevent anyone from trying to force unbind the driver via
sysfs?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel