diff mbox

[v2] drm/msm/dsi: free first element on error

Message ID 20170216120028.GA29698@mwanda (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Dan Carpenter Feb. 16, 2017, noon UTC 
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 <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rob Clark Feb. 16, 2017, 12:16 p.m. UTC | #1 
On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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 <dan.carpenter@oracle.com>

Thanks

Reviewed-by: Rob Clark <robdclark@gmail.com>

>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1fc07ce24686..4fa32296162e 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--)
> +       while (i--)
>                 clk_disable_unprepare(msm_host->bus_clks[i]);
>
>         return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jani Nikula Feb. 16, 2017, 12:27 p.m. UTC | #2 
On Thu, 16 Feb 2017, Rob Clark <robdclark@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> 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 <dan.carpenter@oracle.com>
>
> Thanks
>
> Reviewed-by: Rob Clark <robdclark@gmail.com>

And for good measure,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 1fc07ce24686..4fa32296162e 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--)
>> +       while (i--)
>>                 clk_disable_unprepare(msm_host->bus_clks[i]);
>>
>>         return ret;
Daniel Vetter Feb. 26, 2017, 8:52 p.m. UTC | #3 
On Thu, Feb 16, 2017 at 02:27:08PM +0200, Jani Nikula wrote:
> On Thu, 16 Feb 2017, Rob Clark <robdclark@gmail.com> wrote:
> > On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> 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 <dan.carpenter@oracle.com>
> >
> > Thanks
> >
> > Reviewed-by: Rob Clark <robdclark@gmail.com>
> 
> And for good measure,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

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

> 
> 
> >
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index 1fc07ce24686..4fa32296162e 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--)
> >> +       while (i--)
> >>                 clk_disable_unprepare(msm_host->bus_clks[i]);
> >>
> >>         return ret;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jani Nikula Feb. 27, 2017, 10:18 a.m. UTC | #4 
On Sun, 26 Feb 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 16, 2017 at 02:27:08PM +0200, Jani Nikula wrote:
>> On Thu, 16 Feb 2017, Rob Clark <robdclark@gmail.com> wrote:
>> > On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >> 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 <dan.carpenter@oracle.com>
>> >
>> > Thanks
>> >
>> > Reviewed-by: Rob Clark <robdclark@gmail.com>
>> 
>> And for good measure,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> 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.

> -Daniel
>
>> 
>> 
>> >
>> >>
>> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> >> index 1fc07ce24686..4fa32296162e 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--)
>> >> +       while (i--)
>> >>                 clk_disable_unprepare(msm_host->bus_clks[i]);
>> >>
>> >>         return ret;
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Feb. 27, 2017, 10:25 a.m. UTC | #5 
On Mon, Feb 27, 2017 at 11:18 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Sun, 26 Feb 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Feb 16, 2017 at 02:27:08PM +0200, Jani Nikula wrote:
>>> On Thu, 16 Feb 2017, Rob Clark <robdclark@gmail.com> wrote:
>>> > On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>> >> 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 <dan.carpenter@oracle.com>
>>> >
>>> > Thanks
>>> >
>>> > Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>
>>> And for good measure,
>>>
>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>
>> 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
Rob Clark Feb. 27, 2017, 11:25 a.m. UTC | #6 
On Mon, Feb 27, 2017 at 5:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Feb 27, 2017 at 11:18 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>> On Sun, 26 Feb 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Thu, Feb 16, 2017 at 02:27:08PM +0200, Jani Nikula wrote:
>>>> On Thu, 16 Feb 2017, Rob Clark <robdclark@gmail.com> wrote:
>>>> > On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>> >> 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 <dan.carpenter@oracle.com>
>>>> >
>>>> > Thanks
>>>> >
>>>> > Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>>
>>>> And for good measure,
>>>>
>>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>>
>>> 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.

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

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1fc07ce24686..4fa32296162e 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--)
+	while (i--)
 		clk_disable_unprepare(msm_host->bus_clks[i]);
 
 	return ret;