2023-03-09 21:30:26

by Hamza Mahfooz

Subject: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()

We should be checking if drm_dp_dpcd_read() returns the size that we are
asking it to read instead of just checking if it is greater than zero.
Also, we should WARN_ON() here since this condition is only ever met, if
there is an issue worth investigating. So, compare the return value of
drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.

Signed-off-by: Hamza Mahfooz <[email protected]>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 8d598b322e5b..ed2ed7b1d869 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
return false;
}

- return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
- data, size) > 0;
+ return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
+ data, size) != size);
}

bool dm_helpers_dp_write_dpcd(
--
2.39.2

2023-03-10 17:45:24

by Rodrigo Siqueira Jordao

Subject: Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()

On 3/9/23 14:30, Hamza Mahfooz wrote:
> We should be checking if drm_dp_dpcd_read() returns the size that we are
> asking it to read instead of just checking if it is greater than zero.
> Also, we should WARN_ON() here since this condition is only ever met, if
> there is an issue worth investigating. So, compare the return value of
> drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.
>
> Signed-off-by: Hamza Mahfooz <[email protected]>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 8d598b322e5b..ed2ed7b1d869 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
> return false;
> }
>
> - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> - data, size) > 0;
> + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> + data, size) != size);
> }
>
> bool dm_helpers_dp_write_dpcd(


Reviewed-by: Rodrigo Siqueira <[email protected]>

and pushed to amd-staging-drm-next.

Thanks
Siqueira

2023-03-10 17:49:00

by Ville Syrjälä

Subject: Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()

On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote:
> We should be checking if drm_dp_dpcd_read() returns the size that we are
> asking it to read instead of just checking if it is greater than zero.
> Also, we should WARN_ON() here since this condition is only ever met, if
> there is an issue worth investigating. So, compare the return value of
> drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.
>
> Signed-off-by: Hamza Mahfooz <[email protected]>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 8d598b322e5b..ed2ed7b1d869 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
> return false;
> }
>
> - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> - data, size) > 0;
> + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> + data, size) != size);

Just FYI there are devices out there that violate the DP spec and reads
from specific DPCD registers simply fail instead of returning the
expected 0.

> }
>
> bool dm_helpers_dp_write_dpcd(
> --
> 2.39.2

--
Ville Syrj?l?
Intel

2023-03-10 17:52:01

by Ville Syrjälä

Subject: Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()

On Fri, Mar 10, 2023 at 07:48:04PM +0200, Ville Syrj?l? wrote:
> On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote:
> > We should be checking if drm_dp_dpcd_read() returns the size that we are
> > asking it to read instead of just checking if it is greater than zero.
> > Also, we should WARN_ON() here since this condition is only ever met, if
> > there is an issue worth investigating. So, compare the return value of
> > drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.
> >
> > Signed-off-by: Hamza Mahfooz <[email protected]>
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > index 8d598b322e5b..ed2ed7b1d869 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
> > return false;
> > }
> >
> > - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> > - data, size) > 0;
> > + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
> > + data, size) != size);
>
> Just FYI there are devices out there that violate the DP spec and reads
> from specific DPCD registers simply fail instead of returning the
> expected 0.

And of course anyone can yank the cable anytime, so in
fact pretty much any DPCD read can fail.

--
Ville Syrj?l?
Intel

2023-03-20 20:26:28

by Harry Wentland

Subject: Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()

On 3/10/23 12:51, Ville Syrjälä wrote:
> On Fri, Mar 10, 2023 at 07:48:04PM +0200, Ville Syrjälä wrote:
>> On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote:
>>> We should be checking if drm_dp_dpcd_read() returns the size that we are
>>> asking it to read instead of just checking if it is greater than zero.
>>> Also, we should WARN_ON() here since this condition is only ever met, if
>>> there is an issue worth investigating. So, compare the return value of
>>> drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.
>>>
>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> index 8d598b322e5b..ed2ed7b1d869 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
>>> return false;
>>> }
>>>
>>> - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
>>> - data, size) > 0;
>>> + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
>>> + data, size) != size);
>>
>> Just FYI there are devices out there that violate the DP spec and reads
>> from specific DPCD registers simply fail instead of returning the
>> expected 0.
>
> And of course anyone can yank the cable anytime, so in
> fact pretty much any DPCD read can fail.
>

Thanks for making this very important point. It seems like drm_dp_dpcd_access
checks for that, though, and returns -EPROTO if !(ret == size). So I don't
expect this patch to change any behavior.

Harry