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 <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Sui Jingfeng <[email protected]>
---
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;
--
2.34.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 <[email protected]>
> Closes: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Sui Jingfeng <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
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)
On Mon, 10 Jul 2023, suijingfeng <[email protected]> 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.
--
Jani Nikula, Intel Open Source Graphics Center
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 <[email protected]>
>> Closes: https://lore.kernel.org/r/[email protected]/
>> Signed-off-by: Sui Jingfeng <[email protected]>
>
> Acked-by: Thomas Zimmermann <[email protected]>
>
> 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;
>
Hi
Am 10.07.23 um 13:46 schrieb Jani Nikula:
> On Mon, 10 Jul 2023, suijingfeng <[email protected]> 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)
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 <[email protected]>
>>> Closes: https://lore.kernel.org/r/[email protected]/
>>> Signed-off-by: Sui Jingfeng <[email protected]>
>>
>> Acked-by: Thomas Zimmermann <[email protected]>
>>
>> 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)
On Mon, 10 Jul 2023, Thomas Zimmermann <[email protected]> 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.
--
Jani Nikula, Intel Open Source Graphics Center