| Message ID | 20231020225338.1686974-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | drm/ssd130x: Fix possible uninitialized usage of crtc_state variable | expand |
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;
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!
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 <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, >
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 <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 >
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
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
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
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;
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(-)