| Message ID | 1513324742-21469-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote: > In case of fused or unused pipes, return early with a warning when reading > information for encoder. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > drivers/gpu/drm/i915/intel_audio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct > drm_i915_private *dev_priv, { > struct intel_encoder *encoder; > > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) > return NULL; I think we should remove the WARN_ON() and just return NULL. The error return and the debug message we print in the caller is good enough IMO. > > /* MST */
On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote: > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote: >> In case of fused or unused pipes, return early with a warning when reading >> information for encoder. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 >> Reported-by: Thomas Gleixner <tglx@linutronix.de> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> --- >> drivers/gpu/drm/i915/intel_audio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644 >> --- a/drivers/gpu/drm/i915/intel_audio.c >> +++ b/drivers/gpu/drm/i915/intel_audio.c >> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct >> drm_i915_private *dev_priv, { >> struct intel_encoder *encoder; >> >> - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) >> + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) >> return NULL; > > I think we should remove the WARN_ON() and just return NULL. The error return > and the debug message we print in the caller is good enough IMO. On the contrary, I think this one is fine with WARN_ON. The commit message is inaccurate. This will only become a bounds check with the change, and I think it's important to catch the totally out of bounds values loudly. In case of fused or unused pipes, the code will happily pass that check, and check the av_enc_map for whether the pipe is available or not. BR, Jani. > >> >> /* MST */ > >
On Fri, 2017-12-15 at 11:27 +0200, Jani Nikula wrote: > On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.c > om> wrote: > > > > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote: > > > > > > In case of fused or unused pipes, return early with a warning > > > when reading > > > information for encoder. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_audio.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > > > b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f > > > 100644 > > > --- a/drivers/gpu/drm/i915/intel_audio.c > > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > > @@ -779,7 +779,7 @@ static struct intel_encoder > > > *get_saved_enc(struct > > > drm_i915_private *dev_priv, { > > > struct intel_encoder *encoder; > > > > > > - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > > > + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) > > > return NULL; > > I think we should remove the WARN_ON() and just return NULL. The > > error return > > and the debug message we print in the caller is good enough IMO. > On the contrary, I think this one is fine with WARN_ON. The commit > message is inaccurate. This will only become a bounds check with the > change, and I think it's important to catch the totally out of bounds > values loudly. Well, I need to rephrase that commit message. > > In case of fused or unused pipes, the code will happily pass that > check, > and check the av_enc_map for whether the pipe is available or not. > > BR, > Jani. > > > > > > > > > > > > > > > /* MST */ > >
On Friday, December 15, 2017 11:27:02 AM PST Jani Nikula wrote: > On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote: > > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote: > >> In case of fused or unused pipes, return early with a warning when > >> reading > >> information for encoder. > >> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 > >> Reported-by: Thomas Gleixner <tglx@linutronix.de> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> Signed-off-by: Mika Kahola <mika.kahola@intel.com> > >> --- > >> > >> drivers/gpu/drm/i915/intel_audio.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_audio.c > >> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644 > >> --- a/drivers/gpu/drm/i915/intel_audio.c > >> +++ b/drivers/gpu/drm/i915/intel_audio.c > >> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct > >> drm_i915_private *dev_priv, { > >> > >> struct intel_encoder *encoder; > >> > >> - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) > >> + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) > >> > >> return NULL; > > > > I think we should remove the WARN_ON() and just return NULL. The error > > return and the debug message we print in the caller is good enough IMO. > > On the contrary, I think this one is fine with WARN_ON. The commit > message is inaccurate. This will only become a bounds check with the > change, and I think it's important to catch the totally out of bounds > values loudly. > My concern was that we don't expose any interface to tell the audio driver how many pipes are present, so a WARN_ON would be unreasonable. But on second thoughts, audio should have ways to find that information from the hardware. > In case of fused or unused pipes, the code will happily pass that check, > and check the av_enc_map for whether the pipe is available or not. > > BR, > Jani. > > >> /* MST */
On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote: > On Friday, December 15, 2017 11:27:02 AM PST Jani Nikula wrote: >> On Fri, 15 Dec 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> > wrote: >> > On Friday, December 15, 2017 9:59:02 AM PST Mika Kahola wrote: >> >> In case of fused or unused pipes, return early with a warning when >> >> reading >> >> information for encoder. >> >> >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103206 >> >> Reported-by: Thomas Gleixner <tglx@linutronix.de> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com> >> >> --- >> >> >> >> drivers/gpu/drm/i915/intel_audio.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> >> b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644 >> >> --- a/drivers/gpu/drm/i915/intel_audio.c >> >> +++ b/drivers/gpu/drm/i915/intel_audio.c >> >> @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct >> >> drm_i915_private *dev_priv, { >> >> >> >> struct intel_encoder *encoder; >> >> >> >> - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) >> >> + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) >> >> >> >> return NULL; >> > >> > I think we should remove the WARN_ON() and just return NULL. The error >> > return and the debug message we print in the caller is good enough IMO. >> >> On the contrary, I think this one is fine with WARN_ON. The commit >> message is inaccurate. This will only become a bounds check with the >> change, and I think it's important to catch the totally out of bounds >> values loudly. >> > > My concern was that we don't expose any interface to tell the audio driver how > many pipes are present, so a WARN_ON would be unreasonable. But on second > thoughts, audio should have ways to find that information from the hardware. Yeah. And if it gets that wrong, we'd like to know. It just doesn't know about fused stuff, which is why checking against num_pipes fails. BR, Jani. > >> In case of fused or unused pipes, the code will happily pass that check, >> and check the av_enc_map for whether the pipe is available or not. >> >> BR, >> Jani. >> >> >> /* MST */ > >
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index f1502a0..522d54f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -779,7 +779,7 @@ static struct intel_encoder *get_saved_enc(struct drm_i915_private *dev_priv, { struct intel_encoder *encoder; - if (WARN_ON(pipe >= INTEL_INFO(dev_priv)->num_pipes)) + if (WARN_ON(pipe >= ARRAY_SIZE(dev_priv->av_enc_map))) return NULL; /* MST */