Skip to contentSkip to conversation

drm/i915: Remove unused IRQ chip data of HDMI LPE audio

PATCH In review Augustine.Chen 6 participants 18 messages Dec 8, 2017 – Feb 20, 2018

Subsystems (3)
DRM drivers x86 Intel i915
lore
Share & tools
lore

Augustine.Chen

author
#0 lore raw

PATCH 1/1 drm/i915: Remove unused IRQ chip data of HDMI LPE audio

The chip data of HDMI LPE audio is set to drm_i915_private which is not consistent with the expectation by x86 APIC driver. 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 <[email protected]>
Cc Jerome Anand <[email protected]>
Cc Takashi Iwai <[email protected]>
Signed-off-by Augustine.Chen <[email protected]>
---
 drivers/gpu/drm/i915/intel_lpe_audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_lpe_audio.c +1 -1
index 3bf6528..56176f9 100644
@@ -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)
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#1 lore raw

✓ Fi.CI.BAT: success for drm/i915: Remove unused IRQ chip data of HDMI LPE audio

== Series Details ==

== Series Details ==

Series: drm/i915: Remove unused IRQ chip data of HDMI LPE audio
URL : https://patchwork.freedesktop.org/series/35076/
State : success

== Summary ==

Series 35076v1 drm/i915: Remove unused IRQ chip data of HDMI LPE audio
https://patchwork.freedesktop.org/api/1.0/series/35076/revisions/1/mbox/

Test debugfs_test:

        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989
                dmesg-warn -> PASS       (fi-bdw-gvtdvm) fdo#103938 +1

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:444s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:519s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:280s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:487s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:476s
fi-elk-e7500     total:224  pass:163  dwarn:14  dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:269s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:368s
fi-hsw-4770r     total:288  pass:224  dwarn:0   dfail:0   fail:0   skip:64  time:260s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:390s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-ivb-3770      total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:446s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:536s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:534s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:587s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:543s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-skl-6700k     total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:518s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:498s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:443s
fi-snb-2520m     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:545s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:414s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:607s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:646s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:489s

7f090d915d4f2f0f5788d40eba402ae9bf878d43 drm-tip: 2017y-12m-08d-10h-31m-00s UTC integration manifest 0364141225bc drm/i915: Remove unused IRQ chip data of HDMI LPE audio

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7450/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#2 lore raw
In reply to Augustine.Chen
Quoted 2 lines · Augustine.Chen

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.

Quoted 36 lines

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 <[email protected]>
Cc Jerome Anand <[email protected]>
Cc Takashi Iwai <[email protected]>
Signed-off-by Augustine.Chen <[email protected]>
---
 drivers/gpu/drm/i915/intel_lpe_audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_lpe_audio.c +1 -1
index 3bf6528..56176f9 100644
@@ -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)
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#17 lore raw

✗ Fi.CI.IGT: warning for drm/i915: Remove unused IRQ chip data of HDMI LPE audio

In reply to Augustine.Chen
== Series Details ==

== Series Details ==

Series: drm/i915: Remove unused IRQ chip data of HDMI LPE audio
URL : https://patchwork.freedesktop.org/series/35076/
State : warning

== Summary ==

Test kms_cursor_legacy:

        Subgroup flip-vs-cursor-toggle:
                skip       -> PASS       (shard-snb)
        Subgroup basic-flip-before-cursor-legacy:
                skip       -> PASS       (shard-hsw)
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-b:
                skip       -> PASS       (shard-snb)
Test drv_suspend:
        Subgroup fence-restore-tiled2untiled-hibernate:
                skip       -> FAIL       (shard-hsw) fdo#103375 +1
        Subgroup forcewake:
                pass       -> SKIP       (shard-snb)
Test pm_rpm:
        Subgroup fences:
                skip       -> PASS       (shard-hsw)
Test kms_chv_cursor_fail:
        Subgroup pipe-c-128x128-right-edge:
                skip       -> PASS       (shard-hsw)
Test kms_atomic_transition:
        Subgroup 1x-modeset-transitions-nonblocking-fencing:
                skip       -> PASS       (shard-hsw)
Test kms_flip:
        Subgroup flip-vs-panning:
                pass       -> INCOMPLETE (shard-hsw) fdo#103540
Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> PASS       (shard-hsw) fdo#102707
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-hsw) fdo#101623

fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-hsw        total:2671 pass:1528 dwarn:1   dfail:0   fail:10  skip:1131 time:9106s
shard-snb        total:2679 pass:1308 dwarn:1   dfail:0   fail:11  skip:1359 time:8081s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7450/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#3 lore raw
In reply to Ville Syrjälä
Quoted 10 lines · Ville Syrjälä
Quoted 2 lines · Augustine.Chen

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

Quoted 2 lines

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.

Quoted 2 lines
Quoted 2 lines

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

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#4 lore raw
Quoted 16 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, December 9, 2017 4:22 AM
To: Ville Syrjälä <[email protected]>
Cc: Chen, Augustine <[email protected]>; intel- [email protected]; [email protected]; Anand, Jerome <[email protected]>; Bossart, Pierre-louis <pierre- [email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 9 lines
Quoted 7 lines · Ville Syrjälä
Quoted 2 lines · Augustine.Chen

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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 5 lines

Curing the symptom is never a good approach.

Thanks,

tglx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw
Quoted 19 lines · Jerome
Quoted 19 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, December 9, 2017 4:22 AM
To: Ville Syrjälä <[email protected]>
Cc: Chen, Augustine <[email protected]>; intel- [email protected]; [email protected]; Anand, Jerome <[email protected]>; Bossart, Pierre-louis <pierre- [email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 1 line · Ville Syrjälä

On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:

Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 13 lines

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

Quoted 10 lines
Quoted 2 lines

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.

Quoted 2 lines
Quoted 2 lines

In the case of not enabling CONFIG_CPUMASK_OFFSTACK, this would cause kernel panic while doing CPU hotplug.

Quoted 8 lines
Quoted 3 lines

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.

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#6 lore raw
Quoted 63 lines · Ville Syrjälä
Quoted 19 lines · Jerome
Quoted 19 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, December 9, 2017 4:22 AM
To: Ville Syrjälä <[email protected]>
Cc: Chen, Augustine <[email protected]>; intel- [email protected]; [email protected]; Anand, Jerome <[email protected]>; Bossart, Pierre-louis <pierre- [email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 1 line · Ville Syrjälä

On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:

Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 13 lines

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

Quoted 10 lines
Quoted 2 lines

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.

Quoted 2 lines
Quoted 2 lines

In the case of not enabling CONFIG_CPUMASK_OFFSTACK, this would cause kernel panic while doing CPU hotplug.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 5 lines

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

author
#7 lore raw
Quoted 15 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Monday, December 11, 2017 9:23 PM
To: Ville Syrjälä <[email protected]>
Cc: Anand, Jerome <[email protected]>; Thomas Gleixner <[email protected]>; Chen, Augustine <[email protected]>; intel- [email protected]; [email protected]; Bossart, Pierre-louis <[email protected]>; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 43 lines
Quoted 41 lines · Ville Syrjälä
Quoted 19 lines · Jerome
Quoted 19 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Saturday, December 9, 2017 4:22 AM
To: Ville Syrjälä <[email protected]>
Cc: Chen, Augustine <[email protected]>; intel- [email protected]; [email protected]; Anand, Jerome <[email protected]>; Bossart, Pierre-louis <pierre- [email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 1 line · Ville Syrjälä

On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:

Quoted 18 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

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.

Quoted 9 lines
Quoted 3 lines

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.

Quoted 30 lines
Quoted 11 lines
Quoted 11 lines

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

Quoted 7 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 9 lines
Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

thanks,

Takashi

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#8 lore raw
Quoted 26 lines · Augustine
Quoted 26 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 9 lines
Quoted 3 lines

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.

Quoted 5 lines
Quoted 3 lines

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 :)

Quoted 3 lines

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

#10 lore raw
In reply to Anand, Jerome
Quoted 11 lines · Jerome
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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.

Quoted 11 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#11 lore raw
Quoted 23 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Tuesday, December 12, 2017 10:37 PM
To: Anand, Jerome <[email protected]>
Cc: Ville Syrjälä <[email protected]>; Chen, Augustine <[email protected]>; [email protected]; alsa-devel@alsa- project.org; Bossart, Pierre-louis <[email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 6 lines · Jerome
Quoted 6 lines · Ville Syrjälä
Quoted 5 lines · Augustine.Chen
Quoted 2 lines

The chip data of HDMI LPE audio is set to drm_i915_private which is not consistent with the expectation by x86 APIC driver.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 4 lines

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

Quoted 3 lines

Thanks,

tglx

author
#9 lore raw
In reply to Takashi Iwai
Quoted 59 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Tuesday, December 12, 2017 5:45 PM
To: Chen, Augustine <[email protected]>
Cc: Ville Syrjälä <[email protected]>; Anand, Jerome <[email protected]>; Thomas Gleixner <[email protected]>; intel- [email protected]; [email protected]; Bossart, Pierre-louis <[email protected]>; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

Quoted 7 lines · Augustine
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines

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.

Quoted 21 lines
Quoted 21 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines

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.

Quoted 9 lines
Quoted 3 lines

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.

Quoted 5 lines
Quoted 3 lines

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 :)

Quoted 3 lines

And besides, there
are other drivers with similar implementation which doesn't set chip data at all.

Quoted 7 lines

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.

Quoted 2 lines

Takashi

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#12 lore raw
In reply to Anand, Jerome
Quoted 5 lines · Jerome
Quoted 5 lines

-----Original Message-----
From: Thomas Gleixner [mailto:[email protected]]
Sent: Tuesday, December 12, 2017 10:37 PM
To: Anand, Jerome <[email protected]>
Cc: Ville Syrjälä <[email protected]>; Chen, Augustine

Quoted 8 lines
Quoted 8 lines

<[email protected]>; [email protected]; alsa-devel@alsa- project.org; Bossart, Pierre-louis <[email protected]>; [email protected]; Ingo Molnar <[email protected]>; H. Peter Anvin <[email protected]>; Jiang Liu <[email protected]>; Juergen Gross <[email protected]>; Dou Liyang <[email protected]>; linux- [email protected]
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.

Quoted 11 lines
Quoted 11 lines · Jerome
Quoted 10 lines
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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?

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

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

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#13 lore raw
Quoted 14 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines · Jerome
Quoted 11 lines
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#14 lore raw
Quoted 52 lines · Takashi Iwai
Quoted 14 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines · Jerome
Quoted 11 lines
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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;
		....
Quoted 2 lines

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

Quoted 6 lines

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

arch/x86/kernel/apic/vector.c +3 -0
index f3557a1eb562..02e6a3cc0d74 100644
@@ -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;
 }

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#15 lore raw
Quoted 80 lines · Thomas Gleixner
Quoted 52 lines · Takashi Iwai
Quoted 14 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines · Jerome
Quoted 11 lines
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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;
		....
Quoted 2 lines

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

Quoted 6 lines

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

arch/x86/kernel/apic/vector.c +0 -0
index f3557a1eb562..02e6a3cc0d74 100644

arch/x86/kernel/apic/vector.c static struct apic_chip_data *apic_chip_data(struct irq_data *irq_data) · line 66

Quoted 9 lines
arch/x86/kernel/apic/vector.c +3 -0
@@ -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?

Signature · 2 lines
Ville Syrjälä
Intel OTC
#16 lore raw
Quoted 83 lines · Ville Syrjälä
Quoted 82 lines · Thomas Gleixner
Quoted 52 lines · Takashi Iwai
Quoted 14 lines · Thomas Gleixner
Quoted 14 lines
Quoted 14 lines · Jerome
Quoted 11 lines
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Augustine.Chen
Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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;
		....
Quoted 2 lines

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

Quoted 6 lines

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

arch/x86/kernel/apic/vector.c +0 -0
index f3557a1eb562..02e6a3cc0d74 100644
@@ -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;
Quoted 9 lines

arch/x86/kernel/apic/vector.c static struct apic_chip_data *apic_chip_data(struct irq_data *irq_data) · line 66

Quoted 7 lines
arch/x86/kernel/apic/vector.c +3 -0
+	if (irq_data->domain != x86_vector_domain)
+		return NULL;
+
 	return irq_data->chip_data;
 }

Did this patch resolve the issue?

Ping.

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx