Skip to contentSkip to conversation

drm/i915: add audio_ptr pointer check

PATCH In review [email protected] 5 participants 7 messages Mar 2, 2016 – Mar 3, 2016

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 drm/i915: add audio_ptr pointer check

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

Signed-off-by Libin Yang <[email protected]>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_audio.c +4 -2
index 31f6d21..667596d 100644
@@ -529,7 +529,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = connector;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
 

@@ -556,7 +557,8 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = NULL;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw
Quoted 5 lines · On Wed, 02 Mar 2016 03:24:31 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

I don't think non-NULL is mandatory. If any invalid access is seen, it should be fixed rather in the audio side.

thanks,

Takashi

Quoted 33 lines
Signed-off-by Libin Yang <[email protected]>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_audio.c +4 -2
index 31f6d21..667596d 100644
@@ -529,7 +529,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = connector;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
 

@@ -556,7 +557,8 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = NULL;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
Signature · 1 line
1.9.1

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

maintainer
#2 lore raw
Quoted 10 lines · Takashi Iwai
Quoted 5 lines · On Wed, 02 Mar 2016 03:24:31 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

I don't think non-NULL is mandatory. If any invalid access is seen, it should be fixed rather in the audio side.

Agreed.

BR,
Jani

Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#3 lore raw
In reply to Takashi Iwai

Hi Takashi,

Quoted 12 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Wednesday, March 02, 2016 3:44 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Vetter, Daniel; [email protected]; Yang, Libin
Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check

Quoted 1 line · On Wed, 02 Mar 2016 03:24:31 +0100, [email protected] wrote:
Quoted 7 lines
Quoted 4 lines

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

I don't think non-NULL is mandatory. If any invalid access is seen, it should be fixed rather in the audio side.

When I enable MST with other patches (not submitted so far), I found system may be random hang when boot up.

After apply this patch and another patch (I sent the patch in ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier), the hang issue is fixed.

I make the patch because in sound driver side, setting audio_ptr and audio_ops is not in the lock. I'm wondering that the compiler or CPU may optimize and change the setting order (the gfx code and alsa code may run parallelly?).

Regards,
Libin

Quoted 45 lines

thanks,

Takashi

Review

Quoted 7 lines
Signed-off-by Libin Yang <[email protected]>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_audio.c

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_enable(struct

Quoted 4 lines

index 31f6d21..667596d 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -529,7 +529,8 @@ void intel_audio_codec_enable(struct

intel_encoder *intel_encoder)

drivers/gpu/drm/i915/intel_audio.c void intel_audio_codec_disable(struct

Quoted 13 lines
drivers/gpu/drm/i915/intel_audio.c +1 -1
 	intel_dig_port->audio_connector = connector;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops-
 pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops-

pin_eld_notify &&

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-

audio_ptr, (int) port);
}

drivers/gpu/drm/i915/intel_audio.c +0 -0
@@ -556,7 +557,8 @@ void intel_audio_codec_disable(struct

intel_encoder *intel_encoder)

Quoted 12 lines
drivers/gpu/drm/i915/intel_audio.c +1 -1
 	intel_dig_port->audio_connector = NULL;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops-
 pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops-

pin_eld_notify &&

drivers/gpu/drm/i915/intel_audio.c +1 -0
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-

audio_ptr, (int) port);
}

Quoted 3 lines
Quoted 3 lines

--
1.9.1

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

#4 lore raw
Quoted 26 lines · Libin

Hi Takashi,

Quoted 12 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Wednesday, March 02, 2016 3:44 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Vetter, Daniel; [email protected]; Yang, Libin
Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check

Quoted 1 line · On Wed, 02 Mar 2016 03:24:31 +0100, [email protected] wrote:
Quoted 7 lines
Quoted 4 lines

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

I don't think non-NULL is mandatory. If any invalid access is seen, it should be fixed rather in the audio side.

When I enable MST with other patches (not submitted so far), I found system may be random hang when boot up.

Quoted 8 lines

After apply this patch and another patch (I sent the patch in ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier), the hang issue is fixed.

I make the patch because in sound driver side, setting audio_ptr and audio_ops is not in the lock. I'm wondering that the compiler or CPU may optimize and change the setting order (the gfx code and alsa code may run parallelly?).

There are several ways to deal with such a thing. An easy one would be the memory barrier.

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

#5 lore raw

Hi Takashi,

Quoted 42 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Wednesday, March 02, 2016 4:57 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Vetter, Daniel
Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check

Quoted 9 lines · Libin

Hi Takashi,

Quoted 6 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Wednesday, March 02, 2016 3:44 PM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Vetter,

Daniel;

Quoted 21 lines
Quoted 6 lines

[email protected]; Yang, Libin
Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check

Quoted 1 line · On Wed, 02 Mar 2016 03:24:31 +0100, [email protected] wrote:
Quoted 7 lines
Quoted 4 lines

From: Libin Yang <[email protected]>

check to make sure audio_ptr is not NULL before
using it.

I don't think non-NULL is mandatory. If any invalid access is seen, it should be fixed rather in the audio side.

When I enable MST with other patches (not submitted so far), I found system may be random hang when boot up.

After apply this patch and another patch (I sent the patch in ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier), the hang issue is fixed.

Quoted 7 lines
Quoted 4 lines

I make the patch because in sound driver side, setting audio_ptr and audio_ops is not in the lock. I'm wondering that the compiler or CPU may optimize and change the setting order (the gfx code and alsa code may run parallelly?).

There are several ways to deal with such a thing. An easy one would be the memory barrier.

Yes, agree to use mb. It will be easier and safer.

I think wmb() should be enough for this case, right?

I will do the stress test on mb.

Regards,
Libin

Quoted 3 lines

Takashi

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

CI
#6 lore raw

✗ Fi.CI.BAT: warning for drm/i915: add audio_ptr pointer check

In reply to [email protected]
== Series Details ==

== Series Details ==

Series: drm/i915: add audio_ptr pointer check
URL : https://patchwork.freedesktop.org/series/4002/
State : warning

== Summary ==

Series 4002v1 drm/i915: add audio_ptr pointer check
http://patchwork.freedesktop.org/api/1.0/series/4002/revisions/1/mbox/

Test kms_flip:

        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (bdw-ultra)
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                fail       -> SKIP       (ilk-hp8440p)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (ilk-hp8440p)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-fail -> FAIL       (snb-x220t)
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (snb-x220t)
bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:169  pass:154  dwarn:1   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:169  pass:137  dwarn:1   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25 
hsw-brixbox      total:169  pass:152  dwarn:2   dfail:0   fail:0   skip:15 
hsw-gt2          total:169  pass:156  dwarn:2   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:169  pass:115  dwarn:2   dfail:0   fail:0   skip:52 
ivb-t430s        total:169  pass:153  dwarn:0   dfail:0   fail:1   skip:15 
skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
snb-dellxps      total:169  pass:143  dwarn:2   dfail:0   fail:1   skip:23 
snb-x220t        total:169  pass:144  dwarn:1   dfail:0   fail:2   skip:22

Results at /archive/results/CI_IGT_test/Patchwork_1510/

f9cadb616ff17d482312fba07db772b6604ce799 drm-intel-nightly: 2016y-03m-01d-17h-16m-32s UTC integration manifest fe32b47cd9d0b86c2f46da80b4e02cbbccc9cbee drm/i915: add audio_ptr pointer check

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