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

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

[Ville Syrjälä]

On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> The chip data of HDMI LPE audio is set to drm_i915_private which is not
> consistent with the expectation by x86 APIC driver.

Hmm. Why is the apic code looking at data for an irq chip it
hasn't created?

Do we need something like
- dev_priv->lpe_audio.irq = irq_alloc_desc(0);
+ dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
?

That *looks* more correct to me based on a cursory glance at the x86
code, but I didn't trawl very deeply.

> In the case of not
> enabling CONFIG_CPUMASK_OFFSTACK, this would cause kernel panic while doing
> CPU hotplug. Since the dependency of IRQ chip data was removed from HDMI
> LPE audio by Commit 9bd9590997b92fbd79fd028f704f6c584b4439d7 ("drm/i915:
> Stop pretending to mask/unmask LPE audio interrupts"), remove the
> code of setting IRQ chip data to resolve this issue.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103731
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> Cc: Jerome Anand <jerome.anand@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Augustine.Chen <augustine.chen@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index 3bf6528..56176f9 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -176,7 +176,7 @@ static int lpe_audio_irq_init(struct drm_i915_private *dev_priv)
>  				handle_simple_irq,
>  				"hdmi_lpe_audio_irq_handler");
>  
> -	return irq_set_chip_data(irq, dev_priv);
> +	return 0;
>  }
>  
>  static bool lpe_audio_detect(struct drm_i915_private *dev_priv)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Fri, 8 Dec 2017, Ville Syrjälä wrote:

> On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > The chip data of HDMI LPE audio is set to drm_i915_private which is not
> > consistent with the expectation by x86 APIC driver.
> 
> Hmm. Why is the apic code looking at data for an irq chip it
> hasn't created?
> 
> Do we need something like
> - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);

#define irq_alloc_desc(node)

So instead of handing in node 0 you hand in node -1 which is NUMA_NO_NODE

> 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.

Curing the symptom is never a good approach.

Thanks,

	tglx

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

[Anand, Jerome]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, December 9, 2017 4:22 AM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chen, Augustine <augustine.chen@intel.com>; intel-
> gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Anand, Jerome
> <jerome.anand@intel.com>; Bossart, Pierre-louis <pierre-
> louis.bossart@intel.com>; tiwai@suse.de; Ingo Molnar <mingo@redhat.com>;
> H. Peter Anvin <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen
> Gross <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> kernel@vger.kernel.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> HDMI LPE audio
> 
> On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> 
> > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > The chip data of HDMI LPE audio is set to drm_i915_private which is
> > > not consistent with the expectation by x86 APIC driver.
> >
> > Hmm. Why is the apic code looking at data for an irq chip it hasn't
> > created?
> >

apic code expects an irq domain to be place as generic approach.

> > Do we need something like
> > - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
> 
> #define irq_alloc_desc(node)
> 
> So instead of handing in node 0 you hand in node -1 which is NUMA_NO_NODE
> 

Agree - am not sure whether it will make any difference.

> > 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.
 
> Curing the symptom is never a good approach.
> 
> Thanks,
> 
> 	tglx

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

[Ville Syrjälä]

On Mon, Dec 11, 2017 at 08:33:33AM +0000, Anand, Jerome wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Saturday, December 9, 2017 4:22 AM
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chen, Augustine <augustine.chen@intel.com>; intel-
> > gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Anand, Jerome
> > <jerome.anand@intel.com>; Bossart, Pierre-louis <pierre-
> > louis.bossart@intel.com>; tiwai@suse.de; Ingo Molnar <mingo@redhat.com>;
> > H. Peter Anvin <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen
> > Gross <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> > HDMI LPE audio
> > 
> > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > 
> > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > The chip data of HDMI LPE audio is set to drm_i915_private which is
> > > > not consistent with the expectation by x86 APIC driver.
> > >
> > > Hmm. Why is the apic code looking at data for an irq chip it hasn't
> > > created?
> > >
> 
> apic code expects an irq domain to be place as generic approach.
> 
> > > Do we need something like
> > > - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > > + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
> > 
> > #define irq_alloc_desc(node)
> > 
> > So instead of handing in node 0 you hand in node -1 which is NUMA_NO_NODE
> > 

Ah. I misread the macros. So we already pass irq=-1.

> 
> Agree - am not sure whether it will make any difference.
> 
> > > 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.

-- 
Ville Syrjälä
Intel OTC

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

[Takashi Iwai]

On Mon, 11 Dec 2017 14:20:23 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Dec 11, 2017 at 08:33:33AM +0000, Anand, Jerome wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > Sent: Saturday, December 9, 2017 4:22 AM
> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chen, Augustine <augustine.chen@intel.com>; intel-
> > > gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Anand, Jerome
> > > <jerome.anand@intel.com>; Bossart, Pierre-louis <pierre-
> > > louis.bossart@intel.com>; tiwai@suse.de; Ingo Molnar <mingo@redhat.com>;
> > > H. Peter Anvin <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen
> > > Gross <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> > > HDMI LPE audio
> > > 
> > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > 
> > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > The chip data of HDMI LPE audio is set to drm_i915_private which is
> > > > > not consistent with the expectation by x86 APIC driver.
> > > >
> > > > Hmm. Why is the apic code looking at data for an irq chip it hasn't
> > > > created?
> > > >
> > 
> > apic code expects an irq domain to be place as generic approach.
> > 
> > > > Do we need something like
> > > > - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > > > + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
> > > 
> > > #define irq_alloc_desc(node)
> > > 
> > > So instead of handing in node 0 you hand in node -1 which is NUMA_NO_NODE
> > > 
> 
> Ah. I misread the macros. So we already pass irq=-1.
> 
> > 
> > Agree - am not sure whether it will make any difference.
> > 
> > > > 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.


thanks,

Takashi

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

[Chen, Augustine]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, December 11, 2017 9:23 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Anand, Jerome <jerome.anand@intel.com>; Thomas Gleixner
> <tglx@linutronix.de>; Chen, Augustine <augustine.chen@intel.com>; intel-
> gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Bossart, Pierre-louis
> <pierre-louis.bossart@intel.com>; Ingo Molnar <mingo@redhat.com>; H. Peter
> Anvin <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen Gross
> <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> kernel@vger.kernel.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> HDMI LPE audio
> 
> On Mon, 11 Dec 2017 14:20:23 +0100,
> Ville Syrjälä wrote:
> >
> > On Mon, Dec 11, 2017 at 08:33:33AM +0000, Anand, Jerome wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > > Sent: Saturday, December 9, 2017 4:22 AM
> > > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Chen, Augustine <augustine.chen@intel.com>; intel-
> > > > gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Anand,
> > > > Jerome <jerome.anand@intel.com>; Bossart, Pierre-louis <pierre-
> > > > louis.bossart@intel.com>; tiwai@suse.de; Ingo Molnar
> > > > <mingo@redhat.com>; H. Peter Anvin <hpa@zytor.com>; Jiang Liu
> > > > <jiang.liu@linux.intel.com>; Juergen Gross <jgross@suse.com>; Dou
> > > > Liyang <douly.fnst@cn.fujitsu.com>; linux- kernel@vger.kernel.org
> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip
> > > > data of HDMI LPE audio
> > > >
> > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > >
> > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > The chip data of HDMI LPE audio is set to drm_i915_private
> > > > > > which is not consistent with the expectation by x86 APIC driver.
> > > > >
> > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > hasn't created?
> > > > >
> > >
> > > apic code expects an irq domain to be place as generic approach.
> > >
> > > > > Do we need something like
> > > > > - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > > > > + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
> > > >
> > > > #define irq_alloc_desc(node)
> > > >
> > > > So instead of handing in node 0 you hand in node -1 which is
> > > > NUMA_NO_NODE
> > > >
> >
> > Ah. I misread the macros. So we already pass irq=-1.
No matter from the code perspective or from the real test result, this change doesn't make any difference in terms of the issue symptom.

> >
> > >
> > > Agree - am not sure whether it will make any difference.
> > >
> > > > > 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. And besides, there are other drivers with similar implementation which doesn't set chip data at all.


> 
> 
> thanks,
> 
> Takashi

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

[Takashi Iwai]

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.


Takashi

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

[Chen, Augustine]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, December 12, 2017 5:45 PM
> To: Chen, Augustine <augustine.chen@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Anand, Jerome
> <jerome.anand@intel.com>; Thomas Gleixner <tglx@linutronix.de>; intel-
> gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; Bossart, Pierre-louis
> <pierre-louis.bossart@intel.com>; Ingo Molnar <mingo@redhat.com>; H. Peter
> Anvin <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen Gross
> <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> kernel@vger.kernel.org
> 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

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > 
> > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > The chip data of HDMI LPE audio is set to drm_i915_private which is
> > > > not consistent with the expectation by x86 APIC driver.
> > >
> > > Hmm. Why is the apic code looking at data for an irq chip it hasn't
> > > created?
> > >
> 
> apic code expects an irq domain to be place as generic approach.

APIC code does not even see that interrupt at all. It's completely
disconnected.

> > > > 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.

That still does not explain why this is broken with
CONFIG_CPUMASK_OFFSTACK=n and cpu hotplug.

Thanks,

	tglx

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

[Anand, Jerome]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Tuesday, December 12, 2017 10:37 PM
> To: Anand, Jerome <jerome.anand@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Chen, Augustine
> <augustine.chen@intel.com>; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-
> project.org; Bossart, Pierre-louis <pierre-louis.bossart@intel.com>;
> tiwai@suse.de; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen Gross
> <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> kernel@vger.kernel.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> HDMI LPE audio
> 
> On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > >
> > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > is not consistent with the expectation by x86 APIC driver.
> > > >
> > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > hasn't created?
> > > >
> >
> > apic code expects an irq domain to be place as generic approach.
> 
> APIC code does not even see that interrupt at all. It's completely disconnected.
> 

That's the problem - APIC just converts the chip data to its internal format and fails.

> > > > > 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.
> 
> That still does not explain why this is broken with
> CONFIG_CPUMASK_OFFSTACK=n and cpu hotplug.
> 

If the config option is enabled, the address accessed by APIC is valid as shown below
else it tries to access locations which might or might not be valid based on the chip data set.
This is the reason why some drivers work and some don't.
I feel this needs to be fixed in APIC - not sure though !

#ifdef CONFIG_CPUMASK_OFFSTACK
typedef struct cpumask *cpumask_var_t;
...
#else
typedef struct cpumask cpumask_var_t[1];
...
#endif

> Thanks,
> 
> 	tglx

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

On Wed, 13 Dec 2017, Anand, Jerome wrote:
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Tuesday, December 12, 2017 10:37 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Chen, Augustine
> > <augustine.chen@intel.com>; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-
> > project.org; Bossart, Pierre-louis <pierre-louis.bossart@intel.com>;
> > tiwai@suse.de; Ingo Molnar <mingo@redhat.com>; H. Peter Anvin
> > <hpa@zytor.com>; Jiang Liu <jiang.liu@linux.intel.com>; Juergen Gross
> > <jgross@suse.com>; Dou Liyang <douly.fnst@cn.fujitsu.com>; linux-
> > kernel@vger.kernel.org
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> > HDMI LPE audio

Can you please fix your mail client NOT to replicate the full mail
header. That's just annoying.

> > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > >
> > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > >
> > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > hasn't created?
> > > > >
> > >
> > > apic code expects an irq domain to be place as generic approach.
> > 
> > APIC code does not even see that interrupt at all. It's completely disconnected.
> > 
> 
> That's the problem - APIC just converts the chip data to its internal
> format and fails.

How does APIC code end up to touch that interrupt at all? Call stack please.

And please enable CONFIG_GENERIC_IRQ_DEBUGFS and provide the output of

cat /sys/kernel/debug/irq/irqs/$N

where N is the interrupt number of that thing.

Thanks,

	tglx

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

[Takashi Iwai]

On Wed, 13 Dec 2017 12:35:54 +0100,
Thomas Gleixner wrote:
> 
> > > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > > >
> > > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > > >
> > > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > > hasn't created?
> > > > > >
> > > >
> > > > apic code expects an irq domain to be place as generic approach.
> > > 
> > > APIC code does not even see that interrupt at all. It's completely disconnected.
> > > 
> > 
> > That's the problem - APIC just converts the chip data to its internal
> > format and fails.
> 
> How does APIC code end up to touch that interrupt at all? Call stack please.

It's found in the bugzilla referred in the patch:
  https://bugs.freedesktop.org/show_bug.cgi?id=103731

[   87.353072] irq 298 idata->chip->name hdmi_lpe_audio_irqchip
[   87.353072] irq 298 apic_chip_data
[   87.353073] irq 298 data->domain is NULL
[   87.353120] BUG: unable to handle kernel NULL pointer dereference at (null)
[   87.353132] IP: setup_vector_irq+0x1ba/0x230
[   87.353133] PGD 0

If my understanding is correct, it happens only with 4.14 and earlier
kernels where __setup_vector_irq() loops over the all irqs:

static void __setup_vector_irq(int cpu)
{
	struct apic_chip_data *data;
	struct irq_desc *desc;
	int irq, vector;

	/* Mark the inuse vectors */
	for_each_irq_desc(irq, desc) {
		struct irq_data *idata = irq_desc_get_irq_data(desc);

		data = apic_chip_data(idata);
		if (!data || !cpumask_test_cpu(cpu, data->domain))
			continue;
		....

And since we have assigned a non-APIC chip data in the driver, the
code above refers to a wrong object, leading to Oops.

As a further note, the setup_vector_irq() code has been changed in
4.15, and such a reference won't happen any longer.  So the patch
isn't necessary for now, although it's not bad to take as a cleanup.
And we can eventually put Cc to stable there since it actually works
around the issue above for the older kernels -- of course, with more
detailed descriptions about the background.


thanks,

Takashi

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 3167 bytes --]

On Wed, 13 Dec 2017, Takashi Iwai wrote:
> On Wed, 13 Dec 2017 12:35:54 +0100,
> Thomas Gleixner wrote:
> > 
> > > > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > > > >
> > > > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > > > >
> > > > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > > > hasn't created?
> > > > > > >
> > > > >
> > > > > apic code expects an irq domain to be place as generic approach.
> > > > 
> > > > APIC code does not even see that interrupt at all. It's completely disconnected.
> > > > 
> > > 
> > > That's the problem - APIC just converts the chip data to its internal
> > > format and fails.
> > 
> > How does APIC code end up to touch that interrupt at all? Call stack please.
> 
> It's found in the bugzilla referred in the patch:
>   https://bugs.freedesktop.org/show_bug.cgi?id=103731
> 
> [   87.353072] irq 298 idata->chip->name hdmi_lpe_audio_irqchip
> [   87.353072] irq 298 apic_chip_data
> [   87.353073] irq 298 data->domain is NULL
> [   87.353120] BUG: unable to handle kernel NULL pointer dereference at (null)
> [   87.353132] IP: setup_vector_irq+0x1ba/0x230
> [   87.353133] PGD 0
> 
> If my understanding is correct, it happens only with 4.14 and earlier
> kernels where __setup_vector_irq() loops over the all irqs:
> 
> static void __setup_vector_irq(int cpu)
> {
> 	struct apic_chip_data *data;
> 	struct irq_desc *desc;
> 	int irq, vector;
> 
> 	/* Mark the inuse vectors */
> 	for_each_irq_desc(irq, desc) {
> 		struct irq_data *idata = irq_desc_get_irq_data(desc);
> 
> 		data = apic_chip_data(idata);
> 		if (!data || !cpumask_test_cpu(cpu, data->domain))
> 			continue;
> 		....
> 
> And since we have assigned a non-APIC chip data in the driver, the
> code above refers to a wrong object, leading to Oops.

Bah crap. This information should have been provided earlier instead of
handwavy 'doesnt work with CONFIG_FOO and hotplug'.

> As a further note, the setup_vector_irq() code has been changed in
> 4.15, and such a reference won't happen any longer.  So the patch
> isn't necessary for now, although it's not bad to take as a cleanup.
> And we can eventually put Cc to stable there since it actually works
> around the issue above for the older kernels -- of course, with more
> detailed descriptions about the background.

No, that's just tinkering. The proper fix is to make that code robust.

Something like the completely untested patch below should do the trick.

Thanks,

	tglx
---
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index f3557a1eb562..02e6a3cc0d74 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -58,6 +58,9 @@ static struct apic_chip_data *apic_chip_data(struct irq_data *irq_data)
 	while (irq_data->parent_data)
 		irq_data = irq_data->parent_data;
 
+	if (irq_data->domain != x86_vector_domain)
+		return NULL;
+
 	return irq_data->chip_data;
 }
 

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

[Ville Syrjälä]

On Wed, Dec 13, 2017 at 03:06:55PM +0100, Thomas Gleixner wrote:
> On Wed, 13 Dec 2017, Takashi Iwai wrote:
> > On Wed, 13 Dec 2017 12:35:54 +0100,
> > Thomas Gleixner wrote:
> > > 
> > > > > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > > > > >
> > > > > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > > > > >
> > > > > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > > > > hasn't created?
> > > > > > > >
> > > > > >
> > > > > > apic code expects an irq domain to be place as generic approach.
> > > > > 
> > > > > APIC code does not even see that interrupt at all. It's completely disconnected.
> > > > > 
> > > > 
> > > > That's the problem - APIC just converts the chip data to its internal
> > > > format and fails.
> > > 
> > > How does APIC code end up to touch that interrupt at all? Call stack please.
> > 
> > It's found in the bugzilla referred in the patch:
> >   https://bugs.freedesktop.org/show_bug.cgi?id=103731
> > 
> > [   87.353072] irq 298 idata->chip->name hdmi_lpe_audio_irqchip
> > [   87.353072] irq 298 apic_chip_data
> > [   87.353073] irq 298 data->domain is NULL
> > [   87.353120] BUG: unable to handle kernel NULL pointer dereference at (null)
> > [   87.353132] IP: setup_vector_irq+0x1ba/0x230
> > [   87.353133] PGD 0
> > 
> > If my understanding is correct, it happens only with 4.14 and earlier
> > kernels where __setup_vector_irq() loops over the all irqs:
> > 
> > static void __setup_vector_irq(int cpu)
> > {
> > 	struct apic_chip_data *data;
> > 	struct irq_desc *desc;
> > 	int irq, vector;
> > 
> > 	/* Mark the inuse vectors */
> > 	for_each_irq_desc(irq, desc) {
> > 		struct irq_data *idata = irq_desc_get_irq_data(desc);
> > 
> > 		data = apic_chip_data(idata);
> > 		if (!data || !cpumask_test_cpu(cpu, data->domain))
> > 			continue;
> > 		....
> > 
> > And since we have assigned a non-APIC chip data in the driver, the
> > code above refers to a wrong object, leading to Oops.
> 
> Bah crap. This information should have been provided earlier instead of
> handwavy 'doesnt work with CONFIG_FOO and hotplug'.
> 
> > As a further note, the setup_vector_irq() code has been changed in
> > 4.15, and such a reference won't happen any longer.  So the patch
> > isn't necessary for now, although it's not bad to take as a cleanup.
> > And we can eventually put Cc to stable there since it actually works
> > around the issue above for the older kernels -- of course, with more
> > detailed descriptions about the background.
> 
> No, that's just tinkering. The proper fix is to make that code robust.
> 
> Something like the completely untested patch below should do the trick.
> 
> Thanks,
> 
> 	tglx
> ---
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index f3557a1eb562..02e6a3cc0d74 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -58,6 +58,9 @@ static struct apic_chip_data *apic_chip_data(struct irq_data *irq_data)
>  	while (irq_data->parent_data)
>  		irq_data = irq_data->parent_data;
>  
> +	if (irq_data->domain != x86_vector_domain)
> +		return NULL;
> +
>  	return irq_data->chip_data;
>  }
>  
> 

Did this patch resolve the issue?

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Mon, Jan 29, 2018 at 07:09:22PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2017 at 03:06:55PM +0100, Thomas Gleixner wrote:
> > On Wed, 13 Dec 2017, Takashi Iwai wrote:
> > > On Wed, 13 Dec 2017 12:35:54 +0100,
> > > Thomas Gleixner wrote:
> > > > 
> > > > > > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > > > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > > > > > >
> > > > > > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > > > > > >
> > > > > > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > > > > > hasn't created?
> > > > > > > > >
> > > > > > >
> > > > > > > apic code expects an irq domain to be place as generic approach.
> > > > > > 
> > > > > > APIC code does not even see that interrupt at all. It's completely disconnected.
> > > > > > 
> > > > > 
> > > > > That's the problem - APIC just converts the chip data to its internal
> > > > > format and fails.
> > > > 
> > > > How does APIC code end up to touch that interrupt at all? Call stack please.
> > > 
> > > It's found in the bugzilla referred in the patch:
> > >   https://bugs.freedesktop.org/show_bug.cgi?id=103731
> > > 
> > > [   87.353072] irq 298 idata->chip->name hdmi_lpe_audio_irqchip
> > > [   87.353072] irq 298 apic_chip_data
> > > [   87.353073] irq 298 data->domain is NULL
> > > [   87.353120] BUG: unable to handle kernel NULL pointer dereference at (null)
> > > [   87.353132] IP: setup_vector_irq+0x1ba/0x230
> > > [   87.353133] PGD 0
> > > 
> > > If my understanding is correct, it happens only with 4.14 and earlier
> > > kernels where __setup_vector_irq() loops over the all irqs:
> > > 
> > > static void __setup_vector_irq(int cpu)
> > > {
> > > 	struct apic_chip_data *data;
> > > 	struct irq_desc *desc;
> > > 	int irq, vector;
> > > 
> > > 	/* Mark the inuse vectors */
> > > 	for_each_irq_desc(irq, desc) {
> > > 		struct irq_data *idata = irq_desc_get_irq_data(desc);
> > > 
> > > 		data = apic_chip_data(idata);
> > > 		if (!data || !cpumask_test_cpu(cpu, data->domain))
> > > 			continue;
> > > 		....
> > > 
> > > And since we have assigned a non-APIC chip data in the driver, the
> > > code above refers to a wrong object, leading to Oops.
> > 
> > Bah crap. This information should have been provided earlier instead of
> > handwavy 'doesnt work with CONFIG_FOO and hotplug'.
> > 
> > > As a further note, the setup_vector_irq() code has been changed in
> > > 4.15, and such a reference won't happen any longer.  So the patch
> > > isn't necessary for now, although it's not bad to take as a cleanup.
> > > And we can eventually put Cc to stable there since it actually works
> > > around the issue above for the older kernels -- of course, with more
> > > detailed descriptions about the background.
> > 
> > No, that's just tinkering. The proper fix is to make that code robust.
> > 
> > Something like the completely untested patch below should do the trick.
> > 
> > Thanks,
> > 
> > 	tglx
> > ---
> > diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> > index f3557a1eb562..02e6a3cc0d74 100644
> > --- a/arch/x86/kernel/apic/vector.c
> > +++ b/arch/x86/kernel/apic/vector.c
> > @@ -58,6 +58,9 @@ static struct apic_chip_data *apic_chip_data(struct irq_data *irq_data)
> >  	while (irq_data->parent_data)
> >  		irq_data = irq_data->parent_data;
> >  
> > +	if (irq_data->domain != x86_vector_domain)
> > +		return NULL;
> > +
> >  	return irq_data->chip_data;
> >  }
> >  
> > 
> 
> Did this patch resolve the issue?

Ping.

-- 
Ville Syrjälä
Intel OTC