diff mbox series

[5/8] drm/edid: Document why we don't bounds check the DispID CEA block start/end

Message ID 20200124200231.10517-5-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/8] drm/edid: Check the number of detailed timing descriptors in the CEA ext block | expand

Commit Message

Ville Syrjälä Jan. 24, 2020, 8:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

After much head scratching I managed to convince myself that
for_each_displayid_db() has already done the bounds checks for
the DispID CEA data block. Which is why we don't need to repeat
them in cea_db_offsets(). To avoid having to go through that
pain again in the future add a comment which explains this fact.

Cc: Andres Rodriguez <andresx7@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Alex Deucher Jan. 27, 2020, 10:30 p.m. UTC | #1 
On Fri, Jan 24, 2020 at 3:03 PM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> After much head scratching I managed to convince myself that
> for_each_displayid_db() has already done the bounds checks for
> the DispID CEA data block. Which is why we don't need to repeat
> them in cea_db_offsets(). To avoid having to go through that
> pain again in the future add a comment which explains this fact.
>
> Cc: Andres Rodriguez <andresx7@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3df5744026b0..0369a54e3d32 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4001,6 +4001,10 @@ cea_db_offsets(const u8 *cea, int *start, int *end)
>          *   no non-DTD data.
>          */
>         if (cea[0] == DATA_BLOCK_CTA) {
> +               /*
> +                * for_each_displayid_db() has already verified
> +                * that these stay within expected bounds.
> +                */

I think the preferred format is to have the start of the comment be on
the first line after the /* with that fixed:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

>                 *start = 3;
>                 *end = *start + cea[2];
>         } else if (cea[0] == CEA_EXT) {
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä Jan. 28, 2020, 11:44 a.m. UTC | #2 
On Mon, Jan 27, 2020 at 05:30:42PM -0500, Alex Deucher wrote:
> On Fri, Jan 24, 2020 at 3:03 PM Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > After much head scratching I managed to convince myself that
> > for_each_displayid_db() has already done the bounds checks for
> > the DispID CEA data block. Which is why we don't need to repeat
> > them in cea_db_offsets(). To avoid having to go through that
> > pain again in the future add a comment which explains this fact.
> >
> > Cc: Andres Rodriguez <andresx7@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 3df5744026b0..0369a54e3d32 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4001,6 +4001,10 @@ cea_db_offsets(const u8 *cea, int *start, int *end)
> >          *   no non-DTD data.
> >          */
> >         if (cea[0] == DATA_BLOCK_CTA) {
> > +               /*
> > +                * for_each_displayid_db() has already verified
> > +                * that these stay within expected bounds.
> > +                */
> 
> I think the preferred format is to have the start of the comment be on
> the first line after the /* with that fixed:

Nope.

> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> 
> >                 *start = 3;
> >                 *end = *start + cea[2];
> >         } else if (cea[0] == CEA_EXT) {
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shankar, Uma Feb. 3, 2020, 7:58 p.m. UTC | #3 
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> Sent: Tuesday, January 28, 2020 5:14 PM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>; Andres Rodriguez
> <andresx7@gmail.com>; Maling list - DRI developers <dri-
> devel@lists.freedesktop.org>
> Subject: Re: [PATCH 5/8] drm/edid: Document why we don't bounds check the
> DispID CEA block start/end
> 
> On Mon, Jan 27, 2020 at 05:30:42PM -0500, Alex Deucher wrote:
> > On Fri, Jan 24, 2020 at 3:03 PM Ville Syrjala
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > After much head scratching I managed to convince myself that
> > > for_each_displayid_db() has already done the bounds checks for the
> > > DispID CEA data block. Which is why we don't need to repeat them in
> > > cea_db_offsets(). To avoid having to go through that pain again in
> > > the future add a comment which explains this fact.
> > >
> > > Cc: Andres Rodriguez <andresx7@gmail.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_edid.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 3df5744026b0..0369a54e3d32 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4001,6 +4001,10 @@ cea_db_offsets(const u8 *cea, int *start, int *end)
> > >          *   no non-DTD data.
> > >          */
> > >         if (cea[0] == DATA_BLOCK_CTA) {
> > > +               /*
> > > +                * for_each_displayid_db() has already verified
> > > +                * that these stay within expected bounds.
> > > +                */
> >
> > I think the preferred format is to have the start of the comment be on
> > the first line after the /* with that fixed:
> 
> Nope.

Yes the style is correct here, comment is apt as well.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> > Acked-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > >                 *start = 3;
> > >                 *end = *start + cea[2];
> > >         } else if (cea[0] == CEA_EXT) {
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3df5744026b0..0369a54e3d32 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4001,6 +4001,10 @@  cea_db_offsets(const u8 *cea, int *start, int *end)
 	 *   no non-DTD data.
 	 */
 	if (cea[0] == DATA_BLOCK_CTA) {
+		/*
+		 * for_each_displayid_db() has already verified
+		 * that these stay within expected bounds.
+		 */
 		*start = 3;
 		*end = *start + cea[2];
 	} else if (cea[0] == CEA_EXT) {