[PATCH v1] drm/i915/bdb: Fix version check

[PATCH v1] drm/i915/bdb: Fix version check

[Lukasz Majczak]

With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
the size of bdb_lfp_backlight_data structure has been increased,
causing if-statement in the parse_lfp_backlight function
that comapres this structure size to the one retrieved from BDB,
always to fail for older revisions.
This patch fixes it by comparing a total size of all fileds from
the structure (present before the change) with the value gathered from BDB.
Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)

Cc: <stable@vger.kernel.org> # 5.4+
Tested-by: Lukasz Majczak <lma@semihalf.com>
Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c     | 4 +++-
 drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 3c25926092de..052a19b455d1 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
 
 	i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
 	if (bdb->version >= 191 &&
-	    get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
+	    get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
+					      sizeof(backlight_data->data) +
+					      sizeof(backlight_data->level))) {
 		const struct lfp_backlight_control_method *method;
 
 		method = &backlight_data->backlight_control[panel_type];
diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
index 330077c2e588..fff456bf8783 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -814,6 +814,11 @@ struct lfp_brightness_level {
 	u16 reserved;
 } __packed;
 
+/*
+ * Changing struct bdb_lfp_backlight_data might affect its
+ * size comparation to the value hold in BDB.
+ * (e.g. in parse_lfp_backlight())
+ */
 struct bdb_lfp_backlight_data {
 	u8 entry_size;
 	struct lfp_backlight_data_entry data[16];
-- 
2.33.0.464.g1972c5931b-goog

Re: [PATCH v1] drm/i915/bdb: Fix version check

[Jani Nikula]

On Mon, 20 Sep 2021, Lukasz Majczak <lma@semihalf.com> wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
>
> Cc: <stable@vger.kernel.org> # 5.4+

Not a review of the patch, I'll leave that to José, but 5.4 is not
right.

The commit is d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT
234+") which was merged in v5.11 and AFAICT has not been backported to
any stable kernel.

So this would be:

Fixes: d381baad29b4 ("drm/i915/vbt: Fix backlight parsing for VBT 234+")
Cc: <stable@vger.kernel.org> # v5.11+

BR,
Jani.




> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     | 4 +++-
>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>  
>  	i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
>  	if (bdb->version >= 191 &&
> -	    get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> +	    get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> +					      sizeof(backlight_data->data) +
> +					      sizeof(backlight_data->level))) {
>  		const struct lfp_backlight_control_method *method;
>  
>  		method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
>  	u16 reserved;
>  } __packed;
>  
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */
>  struct bdb_lfp_backlight_data {
>  	u8 entry_size;
>  	struct lfp_backlight_data_entry data[16];

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v1] drm/i915/bdb: Fix version check

[Souza, Jose]

On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> the size of bdb_lfp_backlight_data structure has been increased,
> causing if-statement in the parse_lfp_backlight function
> that comapres this structure size to the one retrieved from BDB,
> always to fail for older revisions.
> This patch fixes it by comparing a total size of all fileds from
> the structure (present before the change) with the value gathered from BDB.
> Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> 
> Cc: <stable@vger.kernel.org> # 5.4+
> Tested-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     | 4 +++-
>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 3c25926092de..052a19b455d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
>  
>  	i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
>  	if (bdb->version >= 191 &&
> -	    get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> +	    get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> +					      sizeof(backlight_data->data) +
> +					      sizeof(backlight_data->level))) {

Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
Would be better have a expected size variable set each version set in the beginning of this function.

something like:
switch (bdb->version) {
case 191:
	expected_size = x;
	break;
case 234:
	expected_size = x;
	break;
case 236:
default:
	expected_size = x;
}
	

>  		const struct lfp_backlight_control_method *method;
>  
>  		method = &backlight_data->backlight_control[panel_type];
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 330077c2e588..fff456bf8783 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -814,6 +814,11 @@ struct lfp_brightness_level {
>  	u16 reserved;
>  } __packed;
>  
> +/*
> + * Changing struct bdb_lfp_backlight_data might affect its
> + * size comparation to the value hold in BDB.
> + * (e.g. in parse_lfp_backlight())
> + */

This is true for all the blocks so I don't think we need this comment.

>  struct bdb_lfp_backlight_data {
>  	u8 entry_size;
>  	struct lfp_backlight_data_entry data[16];

Re: [PATCH v1] drm/i915/bdb: Fix version check

[Lukasz Majczak]

pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@intel.com> napisał(a):
>
> On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> > the size of bdb_lfp_backlight_data structure has been increased,
> > causing if-statement in the parse_lfp_backlight function
> > that comapres this structure size to the one retrieved from BDB,
> > always to fail for older revisions.
> > This patch fixes it by comparing a total size of all fileds from
> > the structure (present before the change) with the value gathered from BDB.
> > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> >
> > Cc: <stable@vger.kernel.org> # 5.4+
> > Tested-by: Lukasz Majczak <lma@semihalf.com>
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c     | 4 +++-
> >  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 3c25926092de..052a19b455d1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
> >
> >       i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> >       if (bdb->version >= 191 &&
> > -         get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> > +         get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> > +                                           sizeof(backlight_data->data) +
> > +                                           sizeof(backlight_data->level))) {
>
> Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
> Would be better have a expected size variable set each version set in the beginning of this function.
>
> something like:
> switch (bdb->version) {
> case 191:
>         expected_size = x;
>         break;
> case 234:
>         expected_size = x;
>         break;
> case 236:
> default:
>         expected_size = x;
> }
>
>
> >               const struct lfp_backlight_control_method *method;
> >
> >               method = &backlight_data->backlight_control[panel_type];
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> >       u16 reserved;
> >  } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
>
> >  struct bdb_lfp_backlight_data {
> >       u8 entry_size;
> >       struct lfp_backlight_data_entry data[16];
>
Hi Jose, Jani

Jani - you are right - I was working on 5.4 with a backported patch  -
I'm sorry for this confusion.

Jose,

Regarding expected_size, I couldn't find documentation that could
described this structure size changes among revisions, so all I could
do is to do an educated guess, basing on comments at this structure,
like:

(gdb) ptype /o  struct bdb_lfp_backlight_data
/* offset    |  size */  type = struct bdb_lfp_backlight_data {
/*    0      |     1 */    u8 entry_size;
/*    1      |    96 */    struct lfp_backlight_data_entry data[16];
/*   97      |    16 */    u8 level[16];
/*  113      |    16 */    struct lfp_backlight_control_method
backlight_control[16];
/*  129      |    64 */    struct lfp_brightness_level
brightness_level[16]; /* 234+ */
/*  193      |    64 */    struct lfp_brightness_level
brightness_min_level[16]; /* 234+ */
/*  257      |    16 */    u8 brightness_precision_bits[16]; /* 236+ */

                           /* total size (bytes):  273 */
                         }

if (revision <= 234)
   expected_size = 129;
else if (revision > 234 && revision <=236)
  expected_size = 257;
else /* revision > 236 */
   expected_size = 273;

Is this approach ok? Otherwise I think I would need help from you to
get exact numbers for each revision...

Best regards,
Lukasz