diff mbox series

drm/loongson: Remove a useless check in cursor_plane_atomic_async_check()

Message ID 20230710102411.257970-1-suijingfeng@loongson.cn
State New
Headers
Series drm/loongson: Remove a useless check in cursor_plane_atomic_async_check() |

Commit Message

Sui Jingfeng July 10, 2023, 10:24 a.m. UTC 
  Because smatch warnings:

drivers/gpu/drm/loongson/lsdc_plane.c:199
lsdc_cursor_plane_atomic_async_check()
warn: variable dereferenced before check 'state' (see line 180)

vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c

174  static int
     lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
175                                       struct drm_atomic_state *state)
176  {
177          struct drm_plane_state *new_state;
178          struct drm_crtc_state *crtc_state;
179
180          new_state = drm_atomic_get_new_plane_state(state, plane);
                                                        ^^^^^
state is dereferenced inside this function

181
182  if (!plane->state || !plane->state->fb) {
183          drm_dbg(plane->dev, "%s: state is NULL\n", plane->name);
184                  return -EINVAL;
185  }
186
187  if (new_state->crtc_w != new_state->crtc_h) {
188          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
189                  new_state->crtc_w, new_state->crtc_h);
190          return -EINVAL;
191  }
192
193  if (new_state->crtc_w != 64 && new_state->crtc_w != 32) {
194          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
195                  new_state->crtc_w, new_state->crtc_h);
196          return -EINVAL;
197  }
198
199  if (state) {
         ^^^^^
Checked too late!

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202307100423.rV7D05Uq-lkp@intel.com/
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/loongson/lsdc_plane.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Comments

Thomas Zimmermann July 10, 2023, 10:39 a.m. UTC | #1 
Am 10.07.23 um 12:24 schrieb Sui Jingfeng:
> Because smatch warnings:
> 
> drivers/gpu/drm/loongson/lsdc_plane.c:199
> lsdc_cursor_plane_atomic_async_check()
> warn: variable dereferenced before check 'state' (see line 180)
> 
> vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
> 
> 174  static int
>       lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
> 175                                       struct drm_atomic_state *state)
> 176  {
> 177          struct drm_plane_state *new_state;
> 178          struct drm_crtc_state *crtc_state;
> 179
> 180          new_state = drm_atomic_get_new_plane_state(state, plane);
>                                                          ^^^^^
> state is dereferenced inside this function
> 
> 181
> 182  if (!plane->state || !plane->state->fb) {
> 183          drm_dbg(plane->dev, "%s: state is NULL\n", plane->name);
> 184                  return -EINVAL;
> 185  }
> 186
> 187  if (new_state->crtc_w != new_state->crtc_h) {
> 188          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
> 189                  new_state->crtc_w, new_state->crtc_h);
> 190          return -EINVAL;
> 191  }
> 192
> 193  if (new_state->crtc_w != 64 && new_state->crtc_w != 32) {
> 194          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
> 195                  new_state->crtc_w, new_state->crtc_h);
> 196          return -EINVAL;
> 197  }
> 198
> 199  if (state) {
>           ^^^^^
> Checked too late!
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202307100423.rV7D05Uq-lkp@intel.com/
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

BTW, you're posting these patches for loongson, but that driver is not 
yet in our tree?

Best regards
Thomas


> ---
>   drivers/gpu/drm/loongson/lsdc_plane.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/loongson/lsdc_plane.c b/drivers/gpu/drm/loongson/lsdc_plane.c
> index 2ab3db982aa3..0d5094633222 100644
> --- a/drivers/gpu/drm/loongson/lsdc_plane.c
> +++ b/drivers/gpu/drm/loongson/lsdc_plane.c
> @@ -196,13 +196,7 @@ static int lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
>   		return -EINVAL;
>   	}
>   
> -	if (state) {
> -		crtc_state = drm_atomic_get_existing_crtc_state(state, new_state->crtc);
> -	} else {
> -		crtc_state = plane->crtc->state;
> -		drm_dbg(plane->dev, "%s: atomic state is NULL\n", plane->name);
> -	}
> -
> +	crtc_state = drm_atomic_get_existing_crtc_state(state, new_state->crtc);
>   	if (!crtc_state->active)
>   		return -EINVAL;
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Sui Jingfeng July 10, 2023, 11:34 a.m. UTC | #2 
Hi,

On 2023/7/10 18:39, Thomas Zimmermann wrote:
>
>
> Am 10.07.23 um 12:24 schrieb Sui Jingfeng:
>> Because smatch warnings:
>>
>> drivers/gpu/drm/loongson/lsdc_plane.c:199
>> lsdc_cursor_plane_atomic_async_check()
>> warn: variable dereferenced before check 'state' (see line 180)
>>
>> vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
>>
>> 174  static int
>>       lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
>> 175                                       struct drm_atomic_state 
>> *state)
>> 176  {
>> 177          struct drm_plane_state *new_state;
>> 178          struct drm_crtc_state *crtc_state;
>> 179
>> 180          new_state = drm_atomic_get_new_plane_state(state, plane);
>>                                                          ^^^^^
>> state is dereferenced inside this function
>>
>> 181
>> 182  if (!plane->state || !plane->state->fb) {
>> 183          drm_dbg(plane->dev, "%s: state is NULL\n", plane->name);
>> 184                  return -EINVAL;
>> 185  }
>> 186
>> 187  if (new_state->crtc_w != new_state->crtc_h) {
>> 188          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
>> 189                  new_state->crtc_w, new_state->crtc_h);
>> 190          return -EINVAL;
>> 191  }
>> 192
>> 193  if (new_state->crtc_w != 64 && new_state->crtc_w != 32) {
>> 194          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
>> 195                  new_state->crtc_w, new_state->crtc_h);
>> 196          return -EINVAL;
>> 197  }
>> 198
>> 199  if (state) {
>>           ^^^^^
>> Checked too late!
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202307100423.rV7D05Uq-lkp@intel.com/
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> BTW, you're posting these patches for loongson, 

I'm posting these patches for the drm/loongson driver in drm-misc and/or 
drm-tip branch,

what do you means for *loongson*,

> but that driver is not yet in our tree?
>

I already applied(push) drm/loongson driver to drm-misc-next branch,

What do you means that by "not yet in our tree", linux kernel side?

Am I missing something ?


> Best regards
> Thomas
>
>
>> ---
>>   drivers/gpu/drm/loongson/lsdc_plane.c | 8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/loongson/lsdc_plane.c 
>> b/drivers/gpu/drm/loongson/lsdc_plane.c
>> index 2ab3db982aa3..0d5094633222 100644
>> --- a/drivers/gpu/drm/loongson/lsdc_plane.c
>> +++ b/drivers/gpu/drm/loongson/lsdc_plane.c
>> @@ -196,13 +196,7 @@ static int 
>> lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
>>           return -EINVAL;
>>       }
>>   -    if (state) {
>> -        crtc_state = drm_atomic_get_existing_crtc_state(state, 
>> new_state->crtc);
>> -    } else {
>> -        crtc_state = plane->crtc->state;
>> -        drm_dbg(plane->dev, "%s: atomic state is NULL\n", plane->name);
>> -    }
>> -
>> +    crtc_state = drm_atomic_get_existing_crtc_state(state, 
>> new_state->crtc);
>>       if (!crtc_state->active)
>>           return -EINVAL;
>
Jani Nikula July 10, 2023, 11:46 a.m. UTC | #3 
On Mon, 10 Jul 2023, suijingfeng <suijingfeng@loongson.cn> wrote:
> On 2023/7/10 18:39, Thomas Zimmermann wrote:
>> but that driver is not yet in our tree?
>>
>
> I already applied(push) drm/loongson driver to drm-misc-next branch,
>
> What do you means that by "not yet in our tree", linux kernel side?
>
> Am I missing something ?

Hmm, indeed there's commit f39db26c5428 ("drm: Add kms driver for
loongson display controller")... but it only has one Acked-by from
Thomas that I can't find on any lists, and zero Reviewed-by.

Usually, the bigger the change, the more acks/reviews you need. Did
Thomas really ack this to be merged without recorder reviews? :o


BR,
Jani.
Thomas Zimmermann July 10, 2023, 11:51 a.m. UTC | #4 
Hi

Am 10.07.23 um 13:34 schrieb suijingfeng:
> Hi,
> 
> On 2023/7/10 18:39, Thomas Zimmermann wrote:
>>
>>
>> Am 10.07.23 um 12:24 schrieb Sui Jingfeng:
>>> Because smatch warnings:
>>>
>>> drivers/gpu/drm/loongson/lsdc_plane.c:199
>>> lsdc_cursor_plane_atomic_async_check()
>>> warn: variable dereferenced before check 'state' (see line 180)
>>>
>>> vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
>>>
>>> 174  static int
>>>       lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
>>> 175                                       struct drm_atomic_state 
>>> *state)
>>> 176  {
>>> 177          struct drm_plane_state *new_state;
>>> 178          struct drm_crtc_state *crtc_state;
>>> 179
>>> 180          new_state = drm_atomic_get_new_plane_state(state, plane);
>>>                                                          ^^^^^
>>> state is dereferenced inside this function
>>>
>>> 181
>>> 182  if (!plane->state || !plane->state->fb) {
>>> 183          drm_dbg(plane->dev, "%s: state is NULL\n", plane->name);
>>> 184                  return -EINVAL;
>>> 185  }
>>> 186
>>> 187  if (new_state->crtc_w != new_state->crtc_h) {
>>> 188          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
>>> 189                  new_state->crtc_w, new_state->crtc_h);
>>> 190          return -EINVAL;
>>> 191  }
>>> 192
>>> 193  if (new_state->crtc_w != 64 && new_state->crtc_w != 32) {
>>> 194          drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
>>> 195                  new_state->crtc_w, new_state->crtc_h);
>>> 196          return -EINVAL;
>>> 197  }
>>> 198
>>> 199  if (state) {
>>>           ^^^^^
>>> Checked too late!
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://lore.kernel.org/r/202307100423.rV7D05Uq-lkp@intel.com/
>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> BTW, you're posting these patches for loongson, 
> 
> I'm posting these patches for the drm/loongson driver in drm-misc and/or 
> drm-tip branch,
> 
> what do you means for *loongson*,
> 
>> but that driver is not yet in our tree?
>>
> 
> I already applied(push) drm/loongson driver to drm-misc-next branch,
> 
> What do you means that by "not yet in our tree", linux kernel side?
> 
> Am I missing something ?

No, it's my fault. I didn't update my branches correctly. :)

Best regards
Thomas

> 
> 
>> Best regards
>> Thomas
>>
>>
>>> ---
>>>   drivers/gpu/drm/loongson/lsdc_plane.c | 8 +-------
>>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/loongson/lsdc_plane.c 
>>> b/drivers/gpu/drm/loongson/lsdc_plane.c
>>> index 2ab3db982aa3..0d5094633222 100644
>>> --- a/drivers/gpu/drm/loongson/lsdc_plane.c
>>> +++ b/drivers/gpu/drm/loongson/lsdc_plane.c
>>> @@ -196,13 +196,7 @@ static int 
>>> lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
>>>           return -EINVAL;
>>>       }
>>>   -    if (state) {
>>> -        crtc_state = drm_atomic_get_existing_crtc_state(state, 
>>> new_state->crtc);
>>> -    } else {
>>> -        crtc_state = plane->crtc->state;
>>> -        drm_dbg(plane->dev, "%s: atomic state is NULL\n", plane->name);
>>> -    }
>>> -
>>> +    crtc_state = drm_atomic_get_existing_crtc_state(state, 
>>> new_state->crtc);
>>>       if (!crtc_state->active)
>>>           return -EINVAL;
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Thomas Zimmermann July 10, 2023, 11:59 a.m. UTC | #5 
Hi

Am 10.07.23 um 13:46 schrieb Jani Nikula:
> On Mon, 10 Jul 2023, suijingfeng <suijingfeng@loongson.cn> wrote:
>> On 2023/7/10 18:39, Thomas Zimmermann wrote:
>>> but that driver is not yet in our tree?
>>>
>>
>> I already applied(push) drm/loongson driver to drm-misc-next branch,
>>
>> What do you means that by "not yet in our tree", linux kernel side?
>>
>> Am I missing something ?
> 
> Hmm, indeed there's commit f39db26c5428 ("drm: Add kms driver for
> loongson display controller")... but it only has one Acked-by from
> Thomas that I can't find on any lists, and zero Reviewed-by.
> 
> Usually, the bigger the change, the more acks/reviews you need. Did
> Thomas really ack this to be merged without recorder reviews? :o

Should be OK; it's an isolated driver. I did a review early on and after 
~13 revisions I asked for it to be merged. It's actually a bit hard to 
find reviewers.

Best regards
Thomas

> 
> 
> BR,
> Jani.
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Jani Nikula July 10, 2023, 12:07 p.m. UTC | #6 
On Mon, 10 Jul 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Should be OK; it's an isolated driver. I did a review early on and after 
> ~13 revisions I asked for it to be merged. It's actually a bit hard to 
> find reviewers.

Fair enough.

BR,
Jani.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/loongson/lsdc_plane.c b/drivers/gpu/drm/loongson/lsdc_plane.c
index 2ab3db982aa3..0d5094633222 100644
--- a/drivers/gpu/drm/loongson/lsdc_plane.c
+++ b/drivers/gpu/drm/loongson/lsdc_plane.c
@@ -196,13 +196,7 @@  static int lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
-	if (state) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state, new_state->crtc);
-	} else {
-		crtc_state = plane->crtc->state;
-		drm_dbg(plane->dev, "%s: atomic state is NULL\n", plane->name);
-	}
-
+	crtc_state = drm_atomic_get_existing_crtc_state(state, new_state->crtc);
 	if (!crtc_state->active)
 		return -EINVAL;