* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-11 8:33 ` Anand, Jerome
0 siblings, 0 replies; 30+ messages in thread
From: Anand, Jerome @ 2017-12-11 8:33 UTC (permalink / raw)
To: Thomas Gleixner, Ville Syrjälä
Cc: Juergen Gross, Dou Liyang, alsa-devel, tiwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen,
Augustine, Bossart, Pierre-louis
> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-11 8:33 ` Anand, Jerome
@ 2017-12-11 13:20 ` Ville Syrjälä
-1 siblings, 0 replies; 30+ messages in thread
From: Ville Syrjälä @ 2017-12-11 13:20 UTC (permalink / raw)
To: Anand, Jerome
Cc: Thomas Gleixner, Chen, Augustine, intel-gfx, alsa-devel, Bossart,
Pierre-louis, tiwai, Ingo Molnar, H. Peter Anvin, Jiang Liu,
Juergen Gross, Dou Liyang, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-11 13:20 ` Ville Syrjälä
0 siblings, 0 replies; 30+ messages in thread
From: Ville Syrjälä @ 2017-12-11 13:20 UTC (permalink / raw)
To: Anand, Jerome
Cc: Juergen Gross, Dou Liyang, alsa-devel, tiwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Thomas Gleixner,
Jiang Liu, Chen, Augustine, Bossart, Pierre-louis
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-11 13:20 ` Ville Syrjälä
(?)
@ 2017-12-11 13:23 ` Takashi Iwai
2017-12-12 9:26 ` Chen, Augustine
-1 siblings, 1 reply; 30+ messages in thread
From: Takashi Iwai @ 2017-12-11 13:23 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Anand, Jerome, Thomas Gleixner, Chen, Augustine, intel-gfx,
alsa-devel, Bossart, Pierre-louis, Ingo Molnar, H. Peter Anvin,
Jiang Liu, Juergen Gross, Dou Liyang, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-11 13:23 ` [Intel-gfx] " Takashi Iwai
@ 2017-12-12 9:26 ` Chen, Augustine
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Augustine @ 2017-12-12 9:26 UTC (permalink / raw)
To: Takashi Iwai, Ville Syrjälä
Cc: Anand, Jerome, Thomas Gleixner, intel-gfx, alsa-devel, Bossart,
Pierre-louis, Ingo Molnar, H. Peter Anvin, Jiang Liu,
Juergen Gross, Dou Liyang, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-12 9:26 ` Chen, Augustine
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Augustine @ 2017-12-12 9:26 UTC (permalink / raw)
To: Takashi Iwai, Ville Syrjälä
Cc: Juergen Gross, Dou Liyang, alsa-devel, intel-gfx, linux-kernel,
Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Jiang Liu, Bossart,
Pierre-louis
> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-12 9:26 ` Chen, Augustine
@ 2017-12-12 9:45 ` Takashi Iwai
-1 siblings, 0 replies; 30+ messages in thread
From: Takashi Iwai @ 2017-12-12 9:45 UTC (permalink / raw)
To: Chen, Augustine
Cc: Ville Syrjälä,
Anand, Jerome, Thomas Gleixner, intel-gfx, alsa-devel, Bossart,
Pierre-louis, Ingo Molnar, H. Peter Anvin, Jiang Liu,
Juergen Gross, Dou Liyang, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-12 9:45 ` Takashi Iwai
0 siblings, 0 replies; 30+ messages in thread
From: Takashi Iwai @ 2017-12-12 9:45 UTC (permalink / raw)
To: Chen, Augustine
Cc: Juergen Gross, Dou Liyang, alsa-devel, intel-gfx, linux-kernel,
Ingo Molnar, Anand, Jerome, H. Peter Anvin, Thomas Gleixner,
Jiang Liu, Ville Syrjälä,
Bossart, Pierre-louis
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-12 9:45 ` Takashi Iwai
@ 2017-12-13 9:25 ` Chen, Augustine
-1 siblings, 0 replies; 30+ messages in thread
From: Chen, Augustine @ 2017-12-13 9:25 UTC (permalink / raw)
To: Takashi Iwai
Cc: Ville Syrjälä,
Anand, Jerome, Thomas Gleixner, intel-gfx, alsa-devel, Bossart,
Pierre-louis, Ingo Molnar, H. Peter Anvin, Jiang Liu,
Juergen Gross, Dou Liyang, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-13 9:25 ` Chen, Augustine
0 siblings, 0 replies; 30+ messages in thread
From: Chen, Augustine @ 2017-12-13 9:25 UTC (permalink / raw)
To: Takashi Iwai
Cc: Juergen Gross, Dou Liyang, alsa-devel, intel-gfx, linux-kernel,
Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Jiang Liu, Bossart,
Pierre-louis
> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-11 8:33 ` Anand, Jerome
@ 2017-12-12 17:06 ` Thomas Gleixner
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-12 17:06 UTC (permalink / raw)
To: Anand, Jerome
Cc: Ville Syrjälä,
Chen, Augustine, intel-gfx, alsa-devel, Bossart, Pierre-louis,
tiwai, Ingo Molnar, H. Peter Anvin, Jiang Liu, Juergen Gross,
Dou Liyang, linux-kernel
[-- 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-12 17:06 ` Thomas Gleixner
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-12 17:06 UTC (permalink / raw)
To: Anand, Jerome
Cc: Juergen Gross, Dou Liyang, alsa-devel, tiwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen,
Augustine, Bossart, Pierre-louis
[-- 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
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-12 17:06 ` Thomas Gleixner
(?)
@ 2017-12-13 2:15 ` Anand, Jerome
2017-12-13 11:35 ` Thomas Gleixner
-1 siblings, 1 reply; 30+ messages in thread
From: Anand, Jerome @ 2017-12-13 2:15 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ville Syrjälä,
Chen, Augustine, intel-gfx, alsa-devel, Bossart, Pierre-louis,
tiwai, Ingo Molnar, H. Peter Anvin, Jiang Liu, Juergen Gross,
Dou Liyang, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-13 2:15 ` [Intel-gfx] " Anand, Jerome
@ 2017-12-13 11:35 ` Thomas Gleixner
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-13 11:35 UTC (permalink / raw)
To: Anand, Jerome
Cc: Ville Syrjälä,
Chen, Augustine, intel-gfx, alsa-devel, Bossart, Pierre-louis,
tiwai, Ingo Molnar, H. Peter Anvin, Jiang Liu, Juergen Gross,
Dou Liyang, linux-kernel
[-- 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-13 11:35 ` Thomas Gleixner
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-13 11:35 UTC (permalink / raw)
To: Anand, Jerome
Cc: Juergen Gross, Dou Liyang, alsa-devel, tiwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen,
Augustine, Bossart, Pierre-louis
[-- 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
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-13 11:35 ` Thomas Gleixner
@ 2017-12-13 11:52 ` Takashi Iwai
-1 siblings, 0 replies; 30+ messages in thread
From: Takashi Iwai @ 2017-12-13 11:52 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Anand, Jerome, Ville Syrjälä,
Chen, Augustine, intel-gfx, alsa-devel, Bossart, Pierre-louis,
Ingo Molnar, H. Peter Anvin, Jiang Liu, Juergen Gross,
Dou Liyang, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-13 11:52 ` Takashi Iwai
0 siblings, 0 replies; 30+ messages in thread
From: Takashi Iwai @ 2017-12-13 11:52 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Juergen Gross, Dou Liyang, alsa-devel, intel-gfx, linux-kernel,
Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen, Augustine, Bossart,
Pierre-louis
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-13 11:52 ` Takashi Iwai
@ 2017-12-13 14:06 ` Thomas Gleixner
-1 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-13 14:06 UTC (permalink / raw)
To: Takashi Iwai
Cc: Anand, Jerome, Ville Syrjälä,
Chen, Augustine, intel-gfx, alsa-devel, Bossart, Pierre-louis,
Ingo Molnar, H. Peter Anvin, Jiang Liu, Juergen Gross,
Dou Liyang, linux-kernel
[-- 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;
}
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2017-12-13 14:06 ` Thomas Gleixner
0 siblings, 0 replies; 30+ messages in thread
From: Thomas Gleixner @ 2017-12-13 14:06 UTC (permalink / raw)
To: Takashi Iwai
Cc: Juergen Gross, Dou Liyang, alsa-devel, intel-gfx, linux-kernel,
Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen, Augustine, Bossart,
Pierre-louis
[-- 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;
}
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2017-12-13 14:06 ` Thomas Gleixner
(?)
@ 2018-01-29 17:09 ` Ville Syrjälä
2018-02-20 18:49 ` Ville Syrjälä
-1 siblings, 1 reply; 30+ messages in thread
From: Ville Syrjälä @ 2018-01-29 17:09 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Takashi Iwai, Anand, Jerome, Chen, Augustine, intel-gfx,
alsa-devel, Bossart, Pierre-louis, Ingo Molnar, H. Peter Anvin,
Jiang Liu, Juergen Gross, Dou Liyang, linux-kernel
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
2018-01-29 17:09 ` [Intel-gfx] " Ville Syrjälä
@ 2018-02-20 18:49 ` Ville Syrjälä
0 siblings, 0 replies; 30+ messages in thread
From: Ville Syrjälä @ 2018-02-20 18:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Juergen Gross, Dou Liyang, alsa-devel, Takashi Iwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen,
Augustine, Bossart, Pierre-louis
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio
@ 2018-02-20 18:49 ` Ville Syrjälä
0 siblings, 0 replies; 30+ messages in thread
From: Ville Syrjälä @ 2018-02-20 18:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Juergen Gross, Dou Liyang, alsa-devel, Takashi Iwai, intel-gfx,
linux-kernel, Ingo Molnar, H. Peter Anvin, Jiang Liu, Chen,
Augustine, Bossart, Pierre-louis
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 30+ messages in thread