drm/msm/dsi: free first element on error
Subsystems (4)
PATCH 1/1 drm/msm/dsi: free first element on error
We want to free msm_host->bus_clks[0] so the > should be >=.
drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1
@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
Quoted 10 lines · Dan Carpenter
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0
drivers/gpu/drm/msm/dsi/dsi_host.c static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) · line 441
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);
By the looks of it this is also wrong. I didn't look at the functions, but you probably don't want to unprepare something where prepare failed, i.e. you want to -1 both the start and end offsets. Perhaps the right fix is
while (i--) clk_disable_unprepare(msm_host->bus_clks[i]);
which also seems to be widely used on error paths.
BR,
Jani.
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0return ret;_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 23 lines · Jani Nikula
Quoted 10 lines · Dan Carpenter
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0drivers/gpu/drm/msm/dsi/dsi_host.c static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) · line 441
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);By the looks of it this is also wrong. I didn't look at the functions, but you probably don't want to unprepare something where prepare failed, i.e. you want to -1 both the start and end offsets. Perhaps the right fix is
Quoted 5 lines
while (i--) clk_disable_unprepare(msm_host->bus_clks[i]);which also seems to be widely used on error paths.
Ah yeah. You're right. I'll resend.
regards,
dan carpenter
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
PATCH v2 1/1 drm/msm/dsi: free first element on error
In reply to Jani NikulaWe're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
drivers/gpu/drm/msm/dsi/dsi_host.c +1 -1
@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +while (i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;
Am 16.02.2017 12:53, schrieb Dan Carpenter:
Quoted 24 lines · Jani Nikula
Quoted 23 lines · Dan Carpenter
Quoted 10 lines
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0drivers/gpu/drm/msm/dsi/dsi_host.c static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) · line 441
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);By the looks of it this is also wrong. I didn't look at the functions, but you probably don't want to unprepare something where prepare failed, i.e. you want to -1 both the start and end offsets. Perhaps the right fix is
Quoted 6 lines
Quoted 5 lines
while (i--) clk_disable_unprepare(msm_host->bus_clks[i]);which also seems to be widely used on error paths.
We already know that programmers are bad in counting backwards ...
any chance to make that into a forward loop ?
re,
wh
Quoted 30 lines · walter harms
Am 16.02.2017 12:53, schrieb Dan Carpenter:
Quoted 27 lines · Jani Nikula
Quoted 26 lines · Dan Carpenter
Quoted 10 lines
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0drivers/gpu/drm/msm/dsi/dsi_host.c static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) · line 441
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);By the looks of it this is also wrong. I didn't look at the functions, but you probably don't want to unprepare something where prepare failed, i.e. you want to -1 both the start and end offsets. Perhaps the right fix is
while (i--) clk_disable_unprepare(msm_host->bus_clks[i]);
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
which also seems to be widely used on error paths.
We already know that programmers are bad in counting backwards ...
any chance to make that into a forward loop ?
well, this *is* a common pattern. And in some cases you actually do need to disable clks in reverse order. So meh, I think while (i--) approach is fine
BR,
-R
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>
Thanks
Quoted 14 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +while (i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;
Quoted 28 lines · walter harms
Am 16.02.2017 12:53, schrieb Dan Carpenter:
Quoted 27 lines · Jani Nikula
Quoted 26 lines · Dan Carpenter
Quoted 10 lines
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>drivers/gpu/drm/msm/dsi/dsi_host.c +0
-0drivers/gpu/drm/msm/dsi/dsi_host.c static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) · line 441
Quoted 6 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +for (; i >=0 ; i--)clk_disable_unprepare (msm_host->bus_clks[i]);By the looks of it this is also wrong. I didn't look at the functions, but you probably don't want to unprepare something where prepare failed, i.e. you want to -1 both the start and end offsets. Perhaps the right fix is
while (i--) clk_disable_unprepare(msm_host->bus_clks[i]);
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
which also seems to be widely used on error paths.
We already know that programmers are bad in counting backwards ...
any chance to make that into a forward loop ?
In most cases I'd agree with you. But I see that this while (i--) is becoming somewhat of a pattern for error paths (grep for it), and I think following patterns like this is more important. After a while, you don't have to think about counting when you see it.
Besides, it's generally preferred to cleanup in the reverse order of init, so a forward counting loop would require two variables here.
BR,
Jani.
Quoted 5 lines
re,
wh
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
Jani Nikula
Reviewed-byQuoted 10 lines · Rob Clark
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>Thanks
Reviewed-by Rob Clark <[email protected]>
And for good measure,
Quoted 15 lines
Quoted 15 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +while (i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
Quoted 8 lines · Jani Nikula
Quoted 7 lines · Rob Clark
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>
Quoted 7 lines
Quoted 3 lines
Thanks
Reviewed-by Rob Clark <[email protected]>And for good measure,
Reviewed-by Jani Nikula <[email protected]>
So 2 r-b from maintainers, no one said he'll apply. This isn't really great coordination :-) Note that drm-misc-next is always open, so you could always smash it in there, irrespective of merge window state. hint, hint ...
-Daniel
Quoted 24 lines
Quoted 17 lines
Quoted 17 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +while (i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Quoted 13 lines · Daniel Vetter
Quoted 12 lines · Jani Nikula
Quoted 10 lines · Rob Clark
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>Thanks
Reviewed-by Rob Clark <[email protected]>
Quoted 8 lines
Quoted 3 lines
And for good measure,
Reviewed-by Jani Nikula <[email protected]>So 2 r-b from maintainers, no one said he'll apply. This isn't really great coordination :-) Note that drm-misc-next is always open, so you could always smash it in there, irrespective of merge window state. hint, hint ...
Admittedly I shied away from touching drm/msm.
BR,
Jani.
Quoted 26 lines
-Daniel
Quoted 24 lines
Quoted 17 lines
Quoted 17 lines
drivers/gpu/drm/msm/dsi/dsi_host.c +1
-1@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)return 0 ; err: -for (; i >0 ; i--) +while (i--)clk_disable_unprepare (msm_host->bus_clks[i]);return ret;Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 16 lines · Jani Nikula
Quoted 15 lines · Daniel Vetter
Quoted 14 lines · Jani Nikula
Quoted 10 lines · Rob Clark
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>Thanks
Reviewed-by Rob Clark <[email protected]>And for good measure,
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Reviewed-by Jani Nikula <[email protected]>So 2 r-b from maintainers, no one said he'll apply. This isn't really great coordination :-) Note that drm-misc-next is always open, so you could always smash it in there, irrespective of merge window state. hint, hint ...
Admittedly I shied away from touching drm/msm.
Well that's kinda my point, we have a pile of maintainers who could push this, which means if no one says they do, the patch will likely get lost. Especially if the main maintainer (Rob here) smashes an r-b onto a patch it's super confusing, at least to me.
I guess this is a downside to having lots of committers, and I started to stumble over this in a bunch of places. I think as a rule we should always state when we plan to or have merged a patch, and if it's just an r-b assume it's lost ... At least that's how I deal with core refactorings touching drivers, otherwise we'd probably never get them all landed.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Quoted 25 lines · Daniel Vetter
Quoted 23 lines · Jani Nikula
Quoted 21 lines · Daniel Vetter
Quoted 8 lines · Jani Nikula
Quoted 7 lines · Rob Clark
Quoted 5 lines · Dan Carpenter
We're off by one here. We free something that wasn't allocated and we don't free msm_host->bus_clks[].
Fixes 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")Signed-off-by Dan Carpenter <[email protected]>Quoted 7 lines
Quoted 3 lines
Thanks
Reviewed-by Rob Clark <[email protected]>And for good measure,
Reviewed-by Jani Nikula <[email protected]>So 2 r-b from maintainers, no one said he'll apply. This isn't really great coordination :-) Note that drm-misc-next is always open, so you could always smash it in there, irrespective of merge window state. hint, hint ...
Quoted 6 lines
Quoted 1 line
Admittedly I shied away from touching drm/msm.
Well that's kinda my point, we have a pile of maintainers who could push this, which means if no one says they do, the patch will likely get lost. Especially if the main maintainer (Rob here) smashes an r-b onto a patch it's super confusing, at least to me.
the r-b was in case you wanted to pick it up in drm-misc (with the usual backup plan of sending it in an msm-fixes pull req after the first rc or two).
I don't mind pushing small fixes to drm-misc instead, since I generally don't have a lot of 'em
BR,
-R
Quoted 11 lines
I guess this is a downside to having lots of committers, and I started to stumble over this in a bunch of places. I think as a rule we should always state when we plan to or have merged a patch, and if it's just an r-b assume it's lost ... At least that's how I deal with core refactorings touching drivers, otherwise we'd probably never get them all landed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno