| Message ID | 20170216105042.GA25544@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 16 Feb 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: > 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 <dan.carpenter@oracle.com> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > index 1fc07ce24686..239e79b39a45 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -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. > > return ret; > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote: > On Thu, 16 Feb 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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 <dan.carpenter@oracle.com> > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index 1fc07ce24686..239e79b39a45 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -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. > Ah yeah. You're right. I'll resend. regards, dan carpenter
Am 16.02.2017 12:53, schrieb Dan Carpenter: > On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote: >> On Thu, 16 Feb 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>> 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 <dan.carpenter@oracle.com> >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> index 1fc07ce24686..239e79b39a45 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>> @@ -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. >> > We already know that programmers are bad in counting backwards ... any chance to make that into a forward loop ? re, wh
On Thu, Feb 16, 2017 at 7:03 AM, walter harms <wharms@bfs.de> wrote: > > > Am 16.02.2017 12:53, schrieb Dan Carpenter: >> On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote: >>> On Thu, 16 Feb 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>> 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 <dan.carpenter@oracle.com> >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> index 1fc07ce24686..239e79b39a45 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> @@ -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. >>> >> > > 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
On Thu, 16 Feb 2017, walter harms <wharms@bfs.de> wrote: > Am 16.02.2017 12:53, schrieb Dan Carpenter: >> On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote: >>> On Thu, 16 Feb 2017, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>> 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 <dan.carpenter@oracle.com> >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> index 1fc07ce24686..239e79b39a45 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> @@ -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. >>> >> > > 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. > > re, > wh > >
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 1fc07ce24686..239e79b39a45 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -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;
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 <dan.carpenter@oracle.com>