* [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 10:50 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 10:50 UTC (permalink / raw)
To: Rob Clark, Archit Taneja
Cc: Rob Herring, David Airlie, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Wei Yongjun,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Hai Li
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]);
return ret;
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 10:50 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 10:50 UTC (permalink / raw)
To: Rob Clark, Archit Taneja
Cc: Rob Herring, David Airlie, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Wei Yongjun,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Hai Li
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]);
return ret;
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
2017-02-16 10:50 ` Dan Carpenter
@ 2017-02-16 11:27 ` Jani Nikula
-1 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 11:27 UTC (permalink / raw)
To: Dan Carpenter, Rob Clark, Archit Taneja
Cc: linux-arm-msm, Wei Yongjun, freedreno, kernel-janitors, dri-devel
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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 11:27 ` Jani Nikula
0 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 11:27 UTC (permalink / raw)
To: Dan Carpenter, Rob Clark, Archit Taneja
Cc: linux-arm-msm, Wei Yongjun, freedreno, kernel-janitors, dri-devel
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
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
[not found] ` <87a89mnznw.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-02-16 11:53 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 11:53 UTC (permalink / raw)
To: Jani Nikula
Cc: Archit Taneja, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Wei Yongjun,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Clark,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 11:53 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 11:53 UTC (permalink / raw)
To: Jani Nikula
Cc: Archit Taneja, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Wei Yongjun,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Clark,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [patch v2] drm/msm/dsi: free first element on error
2017-02-16 11:27 ` Jani Nikula
@ 2017-02-16 12:00 ` Dan Carpenter
-1 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 12:00 UTC (permalink / raw)
To: Rob Clark, Archit Taneja
Cc: David Airlie, Jani Nikula, Hai Li, Rob Herring, Wei Yongjun,
linux-arm-msm, dri-devel, freedreno, kernel-janitors
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>
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;
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-16 12:00 ` Dan Carpenter
0 siblings, 0 replies; 26+ messages in thread
From: Dan Carpenter @ 2017-02-16 12:00 UTC (permalink / raw)
To: Rob Clark, Archit Taneja
Cc: David Airlie, Jani Nikula, Hai Li, Rob Herring, Wei Yongjun,
linux-arm-msm, dri-devel, freedreno, kernel-janitors
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>
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;
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
2017-02-16 11:53 ` Dan Carpenter
@ 2017-02-16 12:03 ` walter harms
-1 siblings, 0 replies; 26+ messages in thread
From: walter harms @ 2017-02-16 12:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jani Nikula, Rob Clark, Archit Taneja, linux-arm-msm,
kernel-janitors, dri-devel, Wei Yongjun, freedreno
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 12:03 ` walter harms
0 siblings, 0 replies; 26+ messages in thread
From: walter harms @ 2017-02-16 12:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jani Nikula, Rob Clark, Archit Taneja, linux-arm-msm,
kernel-janitors, dri-devel, Wei Yongjun, freedreno
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
2017-02-16 12:03 ` walter harms
@ 2017-02-16 12:15 ` Rob Clark
-1 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-16 12:15 UTC (permalink / raw)
To: walter harms
Cc: Dan Carpenter, Jani Nikula, Archit Taneja, linux-arm-msm,
kernel-janitors, dri-devel, Wei Yongjun, freedreno
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 12:15 ` Rob Clark
0 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-16 12:15 UTC (permalink / raw)
To: walter harms
Cc: Dan Carpenter, Jani Nikula, Archit Taneja, linux-arm-msm,
kernel-janitors, dri-devel, Wei Yongjun, freedreno
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
2017-02-16 12:00 ` Dan Carpenter
@ 2017-02-16 12:16 ` Rob Clark
-1 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-16 12:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Archit Taneja, David Airlie, Jani Nikula, Hai Li, Rob Herring,
Wei Yongjun, linux-arm-msm, dri-devel, freedreno,
kernel-janitors
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;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-16 12:16 ` Rob Clark
0 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-16 12:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Archit Taneja, David Airlie, Jani Nikula, Hai Li, Rob Herring,
Wei Yongjun, linux-arm-msm, dri-devel, freedreno,
kernel-janitors
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;
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
2017-02-16 12:03 ` walter harms
@ 2017-02-16 12:25 ` Jani Nikula
-1 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 12:25 UTC (permalink / raw)
To: wharms, Dan Carpenter
Cc: Rob Clark, Archit Taneja, linux-arm-msm, kernel-janitors,
dri-devel, Wei Yongjun, freedreno
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
>
>
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch] drm/msm/dsi: free first element on error
@ 2017-02-16 12:25 ` Jani Nikula
0 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 12:25 UTC (permalink / raw)
To: wharms, Dan Carpenter
Cc: Rob Clark, Archit Taneja, linux-arm-msm, kernel-janitors,
dri-devel, Wei Yongjun, freedreno
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
>
>
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
2017-02-16 12:16 ` Rob Clark
@ 2017-02-16 12:27 ` Jani Nikula
-1 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 12:27 UTC (permalink / raw)
To: Rob Clark, Dan Carpenter
Cc: Archit Taneja, David Airlie, Hai Li, Rob Herring, Wei Yongjun,
linux-arm-msm, dri-devel, freedreno, kernel-janitors
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;
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-16 12:27 ` Jani Nikula
0 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-16 12:27 UTC (permalink / raw)
To: Rob Clark, Dan Carpenter
Cc: Archit Taneja, David Airlie, Hai Li, Rob Herring, Wei Yongjun,
linux-arm-msm, dri-devel, freedreno, kernel-janitors
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;
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
2017-02-16 12:27 ` Jani Nikula
@ 2017-02-26 20:52 ` Daniel Vetter
-1 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2017-02-26 20:52 UTC (permalink / raw)
To: Jani Nikula
Cc: Rob Clark, Dan Carpenter, linux-arm-msm, kernel-janitors,
dri-devel, Wei Yongjun, freedreno
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-26 20:52 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2017-02-26 20:52 UTC (permalink / raw)
To: Jani Nikula
Cc: Rob Clark, Dan Carpenter, linux-arm-msm, kernel-janitors,
dri-devel, Wei Yongjun, freedreno
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
2017-02-26 20:52 ` Daniel Vetter
@ 2017-02-27 10:18 ` Jani Nikula
-1 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-27 10:18 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-arm-msm, kernel-janitors, Wei Yongjun, dri-devel,
freedreno, Dan Carpenter
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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-27 10:18 ` Jani Nikula
0 siblings, 0 replies; 26+ messages in thread
From: Jani Nikula @ 2017-02-27 10:18 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-arm-msm, kernel-janitors, Wei Yongjun, dri-devel,
freedreno, Dan Carpenter
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
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
[not found] ` <87h93ggcnn.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
@ 2017-02-27 10:25 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2017-02-27 10:25 UTC (permalink / raw)
To: Jani Nikula
Cc: linux-arm-msm, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
Wei Yongjun, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Rob Clark, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Dan Carpenter
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-27 10:25 ` Daniel Vetter
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Vetter @ 2017-02-27 10:25 UTC (permalink / raw)
To: Jani Nikula
Cc: linux-arm-msm, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
Wei Yongjun, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Rob Clark, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Dan Carpenter
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
[not found] ` <CAKMK7uGLn3m8eEzYEmPFDPr090B0Cq9Xycg_bJdX7SkRJYtNyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-02-27 11:25 ` Rob Clark
0 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-27 11:25 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-arm-msm, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Wei Yongjun,
Jani Nikula, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Dan Carpenter
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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [patch v2] drm/msm/dsi: free first element on error
@ 2017-02-27 11:25 ` Rob Clark
0 siblings, 0 replies; 26+ messages in thread
From: Rob Clark @ 2017-02-27 11:25 UTC (permalink / raw)
To: Daniel Vetter
Cc: linux-arm-msm, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Wei Yongjun,
Jani Nikula, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-02-27 11:25 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 10:50 [patch] drm/msm/dsi: free first element on error Dan Carpenter
2017-02-16 10:50 ` Dan Carpenter
2017-02-16 11:27 ` Jani Nikula
2017-02-16 11:27 ` Jani Nikula
[not found] ` <87a89mnznw.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-02-16 11:53 ` Dan Carpenter
2017-02-16 11:53 ` Dan Carpenter
2017-02-16 12:03 ` walter harms
2017-02-16 12:03 ` walter harms
2017-02-16 12:15 ` Rob Clark
2017-02-16 12:15 ` Rob Clark
2017-02-16 12:25 ` Jani Nikula
2017-02-16 12:25 ` Jani Nikula
2017-02-16 12:00 ` [patch v2] " Dan Carpenter
2017-02-16 12:00 ` Dan Carpenter
2017-02-16 12:16 ` Rob Clark
2017-02-16 12:16 ` Rob Clark
2017-02-16 12:27 ` Jani Nikula
2017-02-16 12:27 ` Jani Nikula
2017-02-26 20:52 ` Daniel Vetter
2017-02-26 20:52 ` Daniel Vetter
2017-02-27 10:18 ` Jani Nikula
2017-02-27 10:18 ` Jani Nikula
[not found] ` <87h93ggcnn.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-02-27 10:25 ` Daniel Vetter
2017-02-27 10:25 ` Daniel Vetter
[not found] ` <CAKMK7uGLn3m8eEzYEmPFDPr090B0Cq9Xycg_bJdX7SkRJYtNyg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-02-27 11:25 ` Rob Clark
2017-02-27 11:25 ` Rob Clark
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.