RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

[Chen, Augustine]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@xxxxxxx]
> Sent: Tuesday, December 12, 2017 5:45 PM
> To: Chen, Augustine <augustine.chen@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Anand, Jerome
> <jerome.anand@xxxxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Bossart, Pierre-louis
> <pierre-louis.bossart@xxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; H. Peter
> Anvin <hpa@xxxxxxxxx>; Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx>; Juergen Gross
> <jgross@xxxxxxxx>; Dou Liyang <douly.fnst@xxxxxxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> HDMI LPE audio
> 
> On Tue, 12 Dec 2017 10:26:08 +0100,
> Chen, Augustine wrote:
> > > > > > > That *looks* more correct to me based on a cursory glance at
> > > > > > > the
> > > > > > > x86 code, but I didn't trawl very deeply.
> > > > > >
> > > > > > The x86 core cares not at all about interrupt chips which are
> > > > > > created in a driver and not connected to an actual
> > > > > > apic/ioapic/msi interrupt. This interrupt chip is its own thing as we have
> others in GPIO etc.
> > > > > >
> > > > > > > > In the case of not enabling CONFIG_CPUMASK_OFFSTACK, this
> > > > > > > > would cause kernel panic while doing CPU hotplug.
> > > > > >
> > > > > > And why so? Surely not because you set irq_chip_data. That's
> > > > > > really no explanation at all.
> > > > > >
> > > > >
> > > > > Ideally, I feel there needs to be an irq domain for mapping the
> > > > > irq numbers
> > > with hwirq.
> > > > > It is not created as part of the hdmi lpe audio bridge.
> > > > > Since the logic to mask/unmask lpe audio interrupts is removed,
> > > > > there is no need of the Chip data or creation of the domain now.
> > > >
> > > > There is no need right now. But there might be a need in the future.
> > > > LPE audio isn't even the only piece of hardware whose irq goes
> > > > through the i915 display engine (there's also the ISP and VED). So
> > > > I would much prefer a proper solution instead of sweeping the
> > > > problem under the rug.
> > >
> > > IMO, the primary question is whether the usage of irq chip without
> > > irq domain is valid or not.  If an irq domain is mandatory, that's
> > > the thing to be fixed in i915 side.
> > In terms of functionality, the interrupt and hdmi audio work fine
> > without irq domain according to the validation.
> 
> Sure, otherwise the patch never landed to mainline :)
> 
> > And besides, there
> > are other drivers with similar implementation which doesn't set chip
> > data at all.
> 
> Yes, dropping the chip data should be OK in the driver, but the only question is
> whether it is the right fix.
> 
> Did you check whether the issue happens with 4.15-rc, too?  This was never
> answered in bugzilla.  If I understand correctly, the code triggering Oops has
> been changed largely there and it should prune the non-legacy irqs.
> 
I tested it with 4.15-RC1 today and couldn't observed the same symptom. And yes, the code running into invalid pointer was modified dramatically.
Feels like it's available to keep this driver intact.


> 
> Takashi