diff mbox series

drm/dp: Actually read Adjust Request Post Cursor2 register

Message ID 20211203084354.3105253-1-keescook@chromium.org (mailing list archive)
State Superseded
Headers show
Series drm/dp: Actually read Adjust Request Post Cursor2 register | expand

Commit Message

Kees Cook Dec. 3, 2021, 8:43 a.m. UTC
The link_status array was not large enough to read the Adjust Request
Post Cursor2 register. Adjust the size to include it. Found with a
-Warray-bounds build:

drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
   59 |         return link_status[r - DP_LANE0_1_STATUS];
      |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
  147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
      |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/drm/drm_dp_helper.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Jani Nikula Dec. 8, 2021, 11:19 a.m. UTC | #1
On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> The link_status array was not large enough to read the Adjust Request
> Post Cursor2 register. Adjust the size to include it. Found with a
> -Warray-bounds build:
>
> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
>    59 |         return link_status[r - DP_LANE0_1_STATUS];
>       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
>   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
>       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
published in 2014, and Tegra is the only user of
drm_dp_get_adjust_request_post_cursor().

Instead of bumping the link status read size from 6 to 11 for all
drivers I'd much rather see some other (maybe Tegra specific) solution
to this.


BR,
Jani.


> ---
>  include/drm/drm_dp_helper.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 472dac376284..277643d2fe2c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1517,7 +1517,15 @@ enum drm_dp_phy {
>  #define DP_MST_LOGICAL_PORT_0 8
>  
>  #define DP_LINK_CONSTANT_N_VALUE 0x8000
> -#define DP_LINK_STATUS_SIZE	   6
> +/*
> + * DPCD registers in link_status:
> + * Link Status:		0x202 through 0x204
> + * Sink Status:		0x205
> + * Adjust Request:	0x206 through 0x207
> + * Training Score:	0x208 through 0x20b
> + * AR Post Cursor2:	0x20c
> + */
> +#define DP_LINK_STATUS_SIZE	   11
>  bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
>  			  int lane_count);
>  bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
Kees Cook Dec. 9, 2021, 5:54 a.m. UTC | #2
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >    59 |         return link_status[r - DP_LANE0_1_STATUS];
> >       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().
> 
> Instead of bumping the link status read size from 6 to 11 for all
> drivers I'd much rather see some other (maybe Tegra specific) solution
> to this.

Hmmm... Well given this is currently non-functional on Tegra (and is an
OOB memory read), how about just removing it entirely?

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
 }
 EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
 
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
-					 unsigned int lane)
-{
-	unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
-	u8 value = dp_link_status(link_status, offset);
-
-	return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
 static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
 {
 	if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..bb5bfa93950f 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -559,8 +559,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
 			drm_dp_get_adjust_request_pre_emphasis(status, i) >>
 				DP_TRAIN_PRE_EMPHASIS_SHIFT;
 
-		adjust->post_cursor[i] =
-			drm_dp_get_adjust_request_post_cursor(status, i);
+		adjust->post_cursor[i] = 0;
 	}
 }
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
 					  int lane);
 u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
 				   int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
-					 unsigned int lane);
 
 #define DP_BRANCH_OUI_HEADER_SIZE	0xc
 #define DP_RECEIVER_CAP_SIZE		0xf


Or maybe do a long link status read in Tegra only?
Kees Cook Dec. 9, 2021, 6:23 a.m. UTC | #3
On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > The link_status array was not large enough to read the Adjust Request
> > Post Cursor2 register. Adjust the size to include it. Found with a
> > -Warray-bounds build:
> >
> > drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> > drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >    59 |         return link_status[r - DP_LANE0_1_STATUS];
> >       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> published in 2014, and Tegra is the only user of
> drm_dp_get_adjust_request_post_cursor().

I see POST_CURSOR2 is used here too:

drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c

Here's a version of that for tegra (untested):


diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 23f9073bc473..c9528aa62c9c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
 }
 EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
 
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
-					 unsigned int lane)
-{
-	unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
-	u8 value = dp_link_status(link_status, offset);
-
-	return (value >> (lane << 1)) & 0x3;
-}
-EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
-
 static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
 {
 	if (rd_interval > 4)
diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
index 70dfb7d1dec5..f5535eb04c6b 100644
--- a/drivers/gpu/drm/tegra/dp.c
+++ b/drivers/gpu/drm/tegra/dp.c
@@ -549,6 +549,15 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
 {
 	struct drm_dp_link_train_set *adjust = &link->train.adjust;
 	unsigned int i;
+	u8 post_cursor;
+	int err;
+
+	err = drm_dp_dpcd_read(link->aux, DP_ADJUST_REQUEST_POST_CURSOR2,
+			       &post_cursor, sizeof(post_cursor));
+	if (err < 0) {
+		DRM_ERROR("failed to read post_cursor2: %d\n", err);
+		post_cursor = 0;
+	}
 
 	for (i = 0; i < link->lanes; i++) {
 		adjust->voltage_swing[i] =
@@ -560,7 +569,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
 				DP_TRAIN_PRE_EMPHASIS_SHIFT;
 
 		adjust->post_cursor[i] =
-			drm_dp_get_adjust_request_post_cursor(status, i);
+			(post_cursor >> (i << 1)) & 0x3;
 	}
 }
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 30359e434c3f..28378db676c8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
 					  int lane);
 u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
 				   int lane);
-u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
-					 unsigned int lane);
 
 #define DP_BRANCH_OUI_HEADER_SIZE	0xc
 #define DP_RECEIVER_CAP_SIZE		0xf


Is that the right way to go?
Harry Wentland Dec. 9, 2021, 10:20 p.m. UTC | #4
On 2021-12-09 01:23, Kees Cook wrote:
> On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
>> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
>>> The link_status array was not large enough to read the Adjust Request
>>> Post Cursor2 register. Adjust the size to include it. Found with a
>>> -Warray-bounds build:
>>>
>>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
>>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
>>>    59 |         return link_status[r - DP_LANE0_1_STATUS];
>>>       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
>>>   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
>>>       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
>> published in 2014, and Tegra is the only user of
>> drm_dp_get_adjust_request_post_cursor().
> 
> I see POST_CURSOR2 is used here too:
> 
> drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> 

Looks like we read and parse that in the admgpu driver without
using drm_dp_get_adjust_request_post_cursor.

I don't have a strong feeling but I liked your original
patch a bit better. I'm not sure what it means when part
of a spec is deprecated. Once a spec is written display
vendors might implement it. We should make sure that
displays like that are always handled in a sane manner.

Harry

> Here's a version of that for tegra (untested):
> 
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 23f9073bc473..c9528aa62c9c 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -144,16 +144,6 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
>  }
>  EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
>  
> -u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> -					 unsigned int lane)
> -{
> -	unsigned int offset = DP_ADJUST_REQUEST_POST_CURSOR2;
> -	u8 value = dp_link_status(link_status, offset);
> -
> -	return (value >> (lane << 1)) & 0x3;
> -}
> -EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
> -
>  static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval)
>  {
>  	if (rd_interval > 4)
> diff --git a/drivers/gpu/drm/tegra/dp.c b/drivers/gpu/drm/tegra/dp.c
> index 70dfb7d1dec5..f5535eb04c6b 100644
> --- a/drivers/gpu/drm/tegra/dp.c
> +++ b/drivers/gpu/drm/tegra/dp.c
> @@ -549,6 +549,15 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
>  {
>  	struct drm_dp_link_train_set *adjust = &link->train.adjust;
>  	unsigned int i;
> +	u8 post_cursor;
> +	int err;
> +
> +	err = drm_dp_dpcd_read(link->aux, DP_ADJUST_REQUEST_POST_CURSOR2,
> +			       &post_cursor, sizeof(post_cursor));
> +	if (err < 0) {
> +		DRM_ERROR("failed to read post_cursor2: %d\n", err);
> +		post_cursor = 0;
> +	}
>  
>  	for (i = 0; i < link->lanes; i++) {
>  		adjust->voltage_swing[i] =
> @@ -560,7 +569,7 @@ static void drm_dp_link_get_adjustments(struct drm_dp_link *link,
>  				DP_TRAIN_PRE_EMPHASIS_SHIFT;
>  
>  		adjust->post_cursor[i] =
> -			drm_dp_get_adjust_request_post_cursor(status, i);
> +			(post_cursor >> (i << 1)) & 0x3;
>  	}
>  }
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 30359e434c3f..28378db676c8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1528,8 +1528,6 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI
>  					  int lane);
>  u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE],
>  				   int lane);
> -u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> -					 unsigned int lane);
>  
>  #define DP_BRANCH_OUI_HEADER_SIZE	0xc
>  #define DP_RECEIVER_CAP_SIZE		0xf
> 
> 
> Is that the right way to go?
>
Kees Cook Dec. 9, 2021, 11:41 p.m. UTC | #5
On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
> 
> 
> On 2021-12-09 01:23, Kees Cook wrote:
> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >>> The link_status array was not large enough to read the Adjust Request
> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >>> -Warray-bounds build:
> >>>
> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >>>    59 |         return link_status[r - DP_LANE0_1_STATUS];
> >>>       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >>>   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >>>       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >>
> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> published in 2014, and Tegra is the only user of
> >> drm_dp_get_adjust_request_post_cursor().
> > 
> > I see POST_CURSOR2 is used here too:
> > 
> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> > 
> 
> Looks like we read and parse that in the admgpu driver without
> using drm_dp_get_adjust_request_post_cursor.

Right, and probably that could be switched to use it, but I'm not sure
what the impact of the larger link_status read is.

> 
> I don't have a strong feeling but I liked your original
> patch a bit better. I'm not sure what it means when part
> of a spec is deprecated. Once a spec is written display
> vendors might implement it. We should make sure that
> displays like that are always handled in a sane manner.

Jani, Dave, any guidance here? I'm fine with whatever, but the current
code is for sure broken. ;)

-Kees
Jani Nikula Dec. 10, 2021, 10:06 a.m. UTC | #6
On Thu, 09 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
>> 
>> 
>> On 2021-12-09 01:23, Kees Cook wrote:
>> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
>> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
>> >>> The link_status array was not large enough to read the Adjust Request
>> >>> Post Cursor2 register. Adjust the size to include it. Found with a
>> >>> -Warray-bounds build:
>> >>>
>> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
>> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
>> >>>    59 |         return link_status[r - DP_LANE0_1_STATUS];
>> >>>       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
>> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
>> >>>   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
>> >>>       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >>>
>> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
>> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >>
>> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
>> >> published in 2014, and Tegra is the only user of
>> >> drm_dp_get_adjust_request_post_cursor().
>> > 
>> > I see POST_CURSOR2 is used here too:
>> > 
>> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
>> > 
>> 
>> Looks like we read and parse that in the admgpu driver without
>> using drm_dp_get_adjust_request_post_cursor.
>
> Right, and probably that could be switched to use it, but I'm not sure
> what the impact of the larger link_status read is.
>
>> 
>> I don't have a strong feeling but I liked your original
>> patch a bit better. I'm not sure what it means when part
>> of a spec is deprecated. Once a spec is written display
>> vendors might implement it. We should make sure that
>> displays like that are always handled in a sane manner.
>
> Jani, Dave, any guidance here? I'm fine with whatever, but the current
> code is for sure broken. ;)

Post Cursor2 was completely optional for the transmitter even before it
was deprecated.

And now we'd be adding 5 bytes extra to all link status reads. To fix
the only user of drm_dp_get_adjust_request_post_cursor() that apparently
has never worked as intended. I'm just not convinced.

I was trying to look through the implications of DP_LINK_STATUS_SIZE
increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
as something probably needing attention.


BR,
Jani.
Kees Cook Dec. 10, 2021, 4:53 p.m. UTC | #7
On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
> On Thu, 09 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> > On Thu, Dec 09, 2021 at 05:20:45PM -0500, Harry Wentland wrote:
> >> 
> >> 
> >> On 2021-12-09 01:23, Kees Cook wrote:
> >> > On Wed, Dec 08, 2021 at 01:19:28PM +0200, Jani Nikula wrote:
> >> >> On Fri, 03 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> >> >>> The link_status array was not large enough to read the Adjust Request
> >> >>> Post Cursor2 register. Adjust the size to include it. Found with a
> >> >>> -Warray-bounds build:
> >> >>>
> >> >>> drivers/gpu/drm/drm_dp_helper.c: In function 'drm_dp_get_adjust_request_post_cursor':
> >> >>> drivers/gpu/drm/drm_dp_helper.c:59:27: error: array subscript 10 is outside array bounds of 'const u8[6]' {aka 'const unsigned char[6]'} [-Werror=array-bounds]
> >> >>>    59 |         return link_status[r - DP_LANE0_1_STATUS];
> >> >>>       |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> >> >>> drivers/gpu/drm/drm_dp_helper.c:147:51: note: while referencing 'link_status'
> >> >>>   147 | u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE],
> >> >>>       |                                          ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> >>>
> >> >>> Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
> >> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> >>
> >> >> Using DP_ADJUST_REQUEST_POST_CURSOR2 has been deprecated since DP 1.3
> >> >> published in 2014, and Tegra is the only user of
> >> >> drm_dp_get_adjust_request_post_cursor().
> >> > 
> >> > I see POST_CURSOR2 is used here too:
> >> > 
> >> > drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> >> > 
> >> 
> >> Looks like we read and parse that in the admgpu driver without
> >> using drm_dp_get_adjust_request_post_cursor.
> >
> > Right, and probably that could be switched to use it, but I'm not sure
> > what the impact of the larger link_status read is.
> >
> >> 
> >> I don't have a strong feeling but I liked your original
> >> patch a bit better. I'm not sure what it means when part
> >> of a spec is deprecated. Once a spec is written display
> >> vendors might implement it. We should make sure that
> >> displays like that are always handled in a sane manner.
> >
> > Jani, Dave, any guidance here? I'm fine with whatever, but the current
> > code is for sure broken. ;)
> 
> Post Cursor2 was completely optional for the transmitter even before it
> was deprecated.
> 
> And now we'd be adding 5 bytes extra to all link status reads. To fix
> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
> has never worked as intended. I'm just not convinced.
> 
> I was trying to look through the implications of DP_LINK_STATUS_SIZE
> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
> as something probably needing attention.

Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
proposed. I will work that up as a proper patch and send it.

Thanks!
Jani Nikula Dec. 13, 2021, 9:34 a.m. UTC | #8
On Fri, 10 Dec 2021, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 10, 2021 at 12:06:20PM +0200, Jani Nikula wrote:
>> Post Cursor2 was completely optional for the transmitter even before it
>> was deprecated.
>> 
>> And now we'd be adding 5 bytes extra to all link status reads. To fix
>> the only user of drm_dp_get_adjust_request_post_cursor() that apparently
>> has never worked as intended. I'm just not convinced.
>> 
>> I was trying to look through the implications of DP_LINK_STATUS_SIZE
>> increase, and at least drm_dp_dpcd_read_phy_link_status() comes across
>> as something probably needing attention.
>
> Okay, it sounds like you'd prefer the "make it tegra-specific" patch I
> proposed. I will work that up as a proper patch and send it.

Yes. Of course, I'd like to hear Thierry's view as well.

BR,
Jani.
diff mbox series

Patch

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 472dac376284..277643d2fe2c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1517,7 +1517,15 @@  enum drm_dp_phy {
 #define DP_MST_LOGICAL_PORT_0 8
 
 #define DP_LINK_CONSTANT_N_VALUE 0x8000
-#define DP_LINK_STATUS_SIZE	   6
+/*
+ * DPCD registers in link_status:
+ * Link Status:		0x202 through 0x204
+ * Sink Status:		0x205
+ * Adjust Request:	0x206 through 0x207
+ * Training Score:	0x208 through 0x20b
+ * AR Post Cursor2:	0x20c
+ */
+#define DP_LINK_STATUS_SIZE	   11
 bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
 			  int lane_count);
 bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],