[PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Javier Martinez Canillas]

Avoid a possible uninitialized use of the crtc_state variable in function
ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:

    drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
    error: uninitialized symbol 'crtc_state'.

Fixes: fdd591e00a9c ("drm/ssd130x: Add support for the SSD132x OLED controller family")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/dri-devel/7dd6ca45-8263-44fe-a318-2fd9d761425d@moroto.mountain/
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/gpu/drm/solomon/ssd130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 32f0857aec9f..e0174f82e353 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -910,7 +910,7 @@ static int ssd132x_primary_plane_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
 	struct drm_crtc *crtc = plane_state->crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state = NULL;
 	const struct drm_format_info *fi;
 	unsigned int pitch;
 	int ret;
-- 
2.41.0

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Jocelyn Falempe]

Hi,

On 21/10/2023 00:52, Javier Martinez Canillas wrote:
> Avoid a possible uninitialized use of the crtc_state variable in function
> ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:
> 
>      drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
>      error: uninitialized symbol 'crtc_state'.

That looks trivial, so you can add:

Acked-by: Jocelyn Falempe <jfalempe@redhat.com>

> 
> Fixes: fdd591e00a9c ("drm/ssd130x: Add support for the SSD132x OLED controller family")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/dri-devel/7dd6ca45-8263-44fe-a318-2fd9d761425d@moroto.mountain/
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 32f0857aec9f..e0174f82e353 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -910,7 +910,7 @@ static int ssd132x_primary_plane_atomic_check(struct drm_plane *plane,
>   	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
>   	struct ssd130x_plane_state *ssd130x_state = to_ssd130x_plane_state(plane_state);
>   	struct drm_crtc *crtc = plane_state->crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state = NULL;
>   	const struct drm_format_info *fi;
>   	unsigned int pitch;
>   	int ret;

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe@redhat.com> writes:

> Hi,
>
> On 21/10/2023 00:52, Javier Martinez Canillas wrote:
>> Avoid a possible uninitialized use of the crtc_state variable in function
>> ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:
>> 
>>      drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
>>      error: uninitialized symbol 'crtc_state'.
>
> That looks trivial, so you can add:
>
> Acked-by: Jocelyn Falempe <jfalempe@redhat.com>
>

Pushed to drm-misc (drm-misc-next). Thanks!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Geert Uytterhoeven]

Hi Javier,

On Fri, Oct 27, 2023 at 11:33 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Jocelyn Falempe <jfalempe@redhat.com> writes:
> > On 21/10/2023 00:52, Javier Martinez Canillas wrote:
> >> Avoid a possible uninitialized use of the crtc_state variable in function
> >> ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:
> >>
> >>      drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
> >>      error: uninitialized symbol 'crtc_state'.
> >
> > That looks trivial, so you can add:
> >
> > Acked-by: Jocelyn Falempe <jfalempe@redhat.com>
> >
>
> Pushed to drm-misc (drm-misc-next). Thanks!

Looks like you introduced an unintended

    (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)

?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert,

> Hi Javier,
>
> On Fri, Oct 27, 2023 at 11:33 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Jocelyn Falempe <jfalempe@redhat.com> writes:
>> > On 21/10/2023 00:52, Javier Martinez Canillas wrote:
>> >> Avoid a possible uninitialized use of the crtc_state variable in function
>> >> ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:
>> >>
>> >>      drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
>> >>      error: uninitialized symbol 'crtc_state'.
>> >
>> > That looks trivial, so you can add:
>> >
>> > Acked-by: Jocelyn Falempe <jfalempe@redhat.com>
>> >
>>
>> Pushed to drm-misc (drm-misc-next). Thanks!
>
> Looks like you introduced an unintended
>
>     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
>
> ?
>

No, that's intended. It's added by the `dim cherry-pick` command, since I
had to cherry-pick to drm-misc-next-fixes the commit that was already in
the drm-misc-next branch.

You will find that message in many drm commits, i.e:

$ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
1708

> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Geert Uytterhoeven]

Hi Javier,

On Tue, Oct 31, 2023 at 11:11 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > On Fri, Oct 27, 2023 at 11:33 AM Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
> >> Jocelyn Falempe <jfalempe@redhat.com> writes:
> >> > On 21/10/2023 00:52, Javier Martinez Canillas wrote:
> >> >> Avoid a possible uninitialized use of the crtc_state variable in function
> >> >> ssd132x_primary_plane_atomic_check() and avoid the following Smatch warn:
> >> >>
> >> >>      drivers/gpu/drm/solomon/ssd130x.c:921 ssd132x_primary_plane_atomic_check()
> >> >>      error: uninitialized symbol 'crtc_state'.
> >> >
> >> > That looks trivial, so you can add:
> >> >
> >> > Acked-by: Jocelyn Falempe <jfalempe@redhat.com>
> >> >
> >>
> >> Pushed to drm-misc (drm-misc-next). Thanks!
> >
> > Looks like you introduced an unintended
> >
> >     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
> >
> > ?
> >
>
> No, that's intended. It's added by the `dim cherry-pick` command, since I
> had to cherry-pick to drm-misc-next-fixes the commit that was already in
> the drm-misc-next branch.
>
> You will find that message in many drm commits, i.e:
>
> $ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
> 1708

Ah, so that's why it's (way too) common to have merge conflicts between
the fixes and non-fixes drm branches :-(

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Javier Martinez Canillas]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Javier,

[...]

>> >> Pushed to drm-misc (drm-misc-next). Thanks!
>> >
>> > Looks like you introduced an unintended
>> >
>> >     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
>> >
>> > ?
>> >
>>
>> No, that's intended. It's added by the `dim cherry-pick` command, since I
>> had to cherry-pick to drm-misc-next-fixes the commit that was already in
>> the drm-misc-next branch.
>>
>> You will find that message in many drm commits, i.e:
>>
>> $ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
>> 1708
>
> Ah, so that's why it's (way too) common to have merge conflicts between
> the fixes and non-fixes drm branches :-(
>

I guess so. In this particular case it was my fault because I pushed to
drm-misc-next with the expectation that there would be a last PR before
the drm-next tree was sent to Torvalds but I missed for a few hours...

So then I had the option for the fixes to miss 6.7 and wait to land in
6.8, or cherry-pick them to the drm-misc-next-fixes branch and pollute
the git history log :(

> Gr{oetje,eeting}s,
>
>                         Geert
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On Tue, Oct 31, 2023 at 12:27:05PM +0100, Javier Martinez Canillas wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> 
> > Hi Javier,
> 
> [...]
> 
> >> >> Pushed to drm-misc (drm-misc-next). Thanks!
> >> >
> >> > Looks like you introduced an unintended
> >> >
> >> >     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
> >> >
> >> > ?
> >> >
> >>
> >> No, that's intended. It's added by the `dim cherry-pick` command, since I
> >> had to cherry-pick to drm-misc-next-fixes the commit that was already in
> >> the drm-misc-next branch.
> >>
> >> You will find that message in many drm commits, i.e:
> >>
> >> $ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
> >> 1708
> >
> > Ah, so that's why it's (way too) common to have merge conflicts between
> > the fixes and non-fixes drm branches :-(
> >
> 
> I guess so. In this particular case it was my fault because I pushed to
> drm-misc-next with the expectation that there would be a last PR before
> the drm-next tree was sent to Torvalds but I missed for a few hours...
>
> So then I had the option for the fixes to miss 6.7 and wait to land in
> 6.8, or cherry-pick them to the drm-misc-next-fixes branch and pollute
> the git history log :(

Yeah, it's the downside of having so many committers, we have to expect
that people are going to make small mistakes every now and then and
that's ok.

That's also not as bad as Geert put it: merging two branches with the
exact same commit applied won't create conflict. If the two commits
aren't exactly the same then we can indeed create conflicts, but that
would have been the case anyway with or without the "double-commits"

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Geert Uytterhoeven]

Hi Maxime,

On Tue, Oct 31, 2023 at 12:53 PM Maxime Ripard <mripard@kernel.org> wrote:
> On Tue, Oct 31, 2023 at 12:27:05PM +0100, Javier Martinez Canillas wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > >> >> Pushed to drm-misc (drm-misc-next). Thanks!
> > >> >
> > >> > Looks like you introduced an unintended
> > >> >
> > >> >     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
> > >> >
> > >> > ?
> > >>
> > >> No, that's intended. It's added by the `dim cherry-pick` command, since I
> > >> had to cherry-pick to drm-misc-next-fixes the commit that was already in
> > >> the drm-misc-next branch.
> > >>
> > >> You will find that message in many drm commits, i.e:
> > >>
> > >> $ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
> > >> 1708
> > >
> > > Ah, so that's why it's (way too) common to have merge conflicts between
> > > the fixes and non-fixes drm branches :-(

> That's also not as bad as Geert put it: merging two branches with the
> exact same commit applied won't create conflict. If the two commits
> aren't exactly the same then we can indeed create conflicts, but that
> would have been the case anyway with or without the "double-commits"

Oh it is, as soon as one branch receives more commits that make changes
to the same location.  Which is fairly common, too, to the point
that I am surprised when merging a drm for-next branch does not trigger
a conflict...

Cfr. the conflict I had to resolve this morning between commit
64ffd2f1d00c6235 ("drm/amd: Disable ASPM for VI w/ all Intel systems")
already upstream, and commits e5f52a84bf0a8170 ("drm/amd: Disable ASPM
for VI w/ all Intel systems") and follow-up 2757a848cb0f1848
("drm/amd: Explicitly disable ASPM when dynamic switching disabled")
in drm/drm-next.

Note that none of 64ffd2f1d00c6235 and 2757a848cb0f1848 has a "cherry
picked from commit" line in the commit description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/ssd130x: Fix possible uninitialized usage of crtc_state variable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Tue, Oct 31, 2023 at 02:00:06PM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Tue, Oct 31, 2023 at 12:53 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, Oct 31, 2023 at 12:27:05PM +0100, Javier Martinez Canillas wrote:
> > > Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > > >> >> Pushed to drm-misc (drm-misc-next). Thanks!
> > > >> >
> > > >> > Looks like you introduced an unintended
> > > >> >
> > > >> >     (cherry picked from commit 9e4db199e66d427c50458f4d72734cc4f0b92948)
> > > >> >
> > > >> > ?
> > > >>
> > > >> No, that's intended. It's added by the `dim cherry-pick` command, since I
> > > >> had to cherry-pick to drm-misc-next-fixes the commit that was already in
> > > >> the drm-misc-next branch.
> > > >>
> > > >> You will find that message in many drm commits, i.e:
> > > >>
> > > >> $ git log --oneline --grep="(cherry picked from commit" drivers/gpu/drm/ | wc -l
> > > >> 1708
> > > >
> > > > Ah, so that's why it's (way too) common to have merge conflicts between
> > > > the fixes and non-fixes drm branches :-(
> 
> > That's also not as bad as Geert put it: merging two branches with the
> > exact same commit applied won't create conflict. If the two commits
> > aren't exactly the same then we can indeed create conflicts, but that
> > would have been the case anyway with or without the "double-commits"
> 
> Oh it is, as soon as one branch receives more commits that make changes
> to the same location.  Which is fairly common, too, to the point
> that I am surprised when merging a drm for-next branch does not trigger
> a conflict...
> 
> Cfr. the conflict I had to resolve this morning between commit
> 64ffd2f1d00c6235 ("drm/amd: Disable ASPM for VI w/ all Intel systems")
> already upstream, and commits e5f52a84bf0a8170 ("drm/amd: Disable ASPM
> for VI w/ all Intel systems") and follow-up 2757a848cb0f1848
> ("drm/amd: Explicitly disable ASPM when dynamic switching disabled")
> in drm/drm-next.

I probably don't get what you're saying, sorry, but those two commits
would have conflicted anyway when merging the two branches, with or
without the cherry-pick.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]