[PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests

[PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests

[Dan Carpenter]

These if statements are supposed to be true if we ended the
list_for_each_entry() loops without hitting a break statement but they
don't work.

In the first loop, we increment "i" after the "if (i == unit)" condition
so we don't necessarily know that "i" is not equal to unit at the end of
the loop.

In the second loop we exit when mode is not pointing to a valid
drm_display_mode struct so it doesn't make sense to check "mode->type".

Fixes: a278724aa23c ("drm/vmwgfx: Implement fbdev on kms v2")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I reversed the second condition as well, just because I was copy and
pasting the exit condition.  Plus I always feel like error handling is
better than success handling.  If anyone feel strongly, then I can send
a v2.

 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 3c97654b5a43..44168a7d7b44 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2576,7 +2576,7 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
 		++i;
 	}
 
-	if (i != unit) {
+	if (&con->head == &dev_priv->dev->mode_config.connector_list) {
 		DRM_ERROR("Could not find initial display unit.\n");
 		ret = -EINVAL;
 		goto out_unlock;
@@ -2600,13 +2600,13 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
 			break;
 	}
 
-	if (mode->type & DRM_MODE_TYPE_PREFERRED)
-		*p_mode = mode;
-	else {
+	if (&mode->head == &con->modes) {
 		WARN_ONCE(true, "Could not find initial preferred mode.\n");
 		*p_mode = list_first_entry(&con->modes,
 					   struct drm_display_mode,
 					   head);
+	} else {
+		*p_mode = mode;
 	}
 
  out_unlock:
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests

[Roland Scheidegger]

Am 26.06.20 um 12:39 schrieb Dan Carpenter:
> These if statements are supposed to be true if we ended the
> list_for_each_entry() loops without hitting a break statement but they
> don't work.
> 
> In the first loop, we increment "i" after the "if (i == unit)" condition
> so we don't necessarily know that "i" is not equal to unit at the end of
> the loop.
So, if I understand this right, this would only really be a problem if
there's no list entries at all, right? That is i == unit == 0.
Not sure if that can actually happen, but in any case the fix looks correct.


> 
> In the second loop we exit when mode is not pointing to a valid
> drm_display_mode struct so it doesn't make sense to check "mode->type".
Looks good to me too, condition order seems fine to me as well, though I
wouldn't particularly care.

Applied to vmwgfx-next as well, thanks.

Roland


> 
> Fixes: a278724aa23c ("drm/vmwgfx: Implement fbdev on kms v2")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I reversed the second condition as well, just because I was copy and
> pasting the exit condition.  Plus I always feel like error handling is
> better than success handling.  If anyone feel strongly, then I can send
> a v2.
> 
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 3c97654b5a43..44168a7d7b44 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2576,7 +2576,7 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
>  		++i;
>  	}
>  
> -	if (i != unit) {
> +	if (&con->head == &dev_priv->dev->mode_config.connector_list) {
>  		DRM_ERROR("Could not find initial display unit.\n");
>  		ret = -EINVAL;
>  		goto out_unlock;
> @@ -2600,13 +2600,13 @@ int vmw_kms_fbdev_init_data(struct vmw_private *dev_priv,
>  			break;
>  	}
>  
> -	if (mode->type & DRM_MODE_TYPE_PREFERRED)
> -		*p_mode = mode;
> -	else {
> +	if (&mode->head == &con->modes) {
>  		WARN_ONCE(true, "Could not find initial preferred mode.\n");
>  		*p_mode = list_first_entry(&con->modes,
>  					   struct drm_display_mode,
>  					   head);
> +	} else {
> +		*p_mode = mode;
>  	}
>  
>   out_unlock:
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests

[Dan Carpenter]

On Tue, Jul 14, 2020 at 03:39:13AM +0200, Roland Scheidegger wrote:
> Am 26.06.20 um 12:39 schrieb Dan Carpenter:
> > These if statements are supposed to be true if we ended the
> > list_for_each_entry() loops without hitting a break statement but they
> > don't work.
> > 
> > In the first loop, we increment "i" after the "if (i == unit)" condition
> > so we don't necessarily know that "i" is not equal to unit at the end of
> > the loop.
> So, if I understand this right, this would only really be a problem if
> there's no list entries at all, right? That is i == unit == 0.
> Not sure if that can actually happen, but in any case the fix looks correct.

An empty list and there is another potential issue where unit is exactly
off by one.

	list_for_each_entry(con, &dev_priv->dev->mode_config.connector_list,
			    head) {
		if (i == unit)
			break;
		++i;  <-- this is the last iteration and it's off by one
			  so now i == unit but we didn't exit via the
			  break statement.
	}

	if (i != unit) {
            ^^^^^^^^^
Since we didn't exit by the break statement we want this to be true but
it's false instead.

		DRM_ERROR("Could not find initial display unit.\n");

I don't know how *likely* this is, but static checkers complain.
Technically correct is the best kind of correct!  ;)

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vmwgfx: Fix two list_for_each loop exit tests

[Roland Scheidegger]

Am 14.07.20 um 10:25 schrieb Dan Carpenter:
> On Tue, Jul 14, 2020 at 03:39:13AM +0200, Roland Scheidegger wrote:
>> Am 26.06.20 um 12:39 schrieb Dan Carpenter:
>>> These if statements are supposed to be true if we ended the
>>> list_for_each_entry() loops without hitting a break statement but they
>>> don't work.
>>>
>>> In the first loop, we increment "i" after the "if (i == unit)" condition
>>> so we don't necessarily know that "i" is not equal to unit at the end of
>>> the loop.
>> So, if I understand this right, this would only really be a problem if
>> there's no list entries at all, right? That is i == unit == 0.
>> Not sure if that can actually happen, but in any case the fix looks correct.
> 
> An empty list and there is another potential issue where unit is exactly
> off by one.
> 
> 	list_for_each_entry(con, &dev_priv->dev->mode_config.connector_list,
> 			    head) {
> 		if (i == unit)
> 			break;
> 		++i;  <-- this is the last iteration and it's off by one
> 			  so now i == unit but we didn't exit via the
> 			  break statement.
> 	}
> 
> 	if (i != unit) {
>             ^^^^^^^^^
> Since we didn't exit by the break statement we want this to be true but
> it's false instead.
> 
> 		DRM_ERROR("Could not find initial display unit.\n");
> 
> I don't know how *likely* this is, but static checkers complain.
> Technically correct is the best kind of correct!  ;)
Ahh indeed seems obvious now. But kinda difficult to spot :-).

Thanks again,

Roland


> 
> regards,
> dan carpenter
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel