Re: [PATCH] stagin: atomisp: Fix oops by unbalanced clk enable/disable call

[Sakari Ailus]

On Tue, Oct 17, 2017 at 08:10:15AM +1100, Tobin C. Harding wrote:
> On Mon, Oct 16, 2017 at 02:34:48PM +0200, Hans de Goede wrote:
> > diff --git 
> > a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
> > b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > index 828fe5abd832..6671ebe4ecc9 100644
> > --- 
> > a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > +++ 
> > b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> > @@ -29,6 +29,7 @@ struct gmin_subdev {
> > struct v4l2_subdev *subdev;
> > int clock_num;
> > int clock_src;
> > +   bool clock_on;
> > struct clk *pmc_clk;
> > struct gpio_desc *gpio0;
> > struct gpio_desc *gpio1;
> > @@ -583,6 +584,9 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev 
> > *subdev, int on)
> > struct gmin_subdev *gs = find_gmin_subdev(subdev);
> > struct i2c_client *client = v4l2_get_subdevdata(subdev);
> >  
> > +   if (gs->clock_on == !!on)
> > +   return 0;
> > +
> > if (on) {
> > ret = clk_set_rate(gs->pmc_clk, gs->clock_src);
> 
> Which tree [and branch] are you working off please? In the staging-next 
> branch of Greg's staging
> tree this function does not appear as it is in this patch.

Media tree master.



-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH] stagin: atomisp: Fix oops by unbalanced clk enable/disable call

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 02:34:48PM +0200, Hans de Goede wrote:
> The common-clk core expects clk consumers to always call enable/disable
> in a balanced manner. The atomisp driver does not call gmin_flisclk_ctrl()
> in a balanced manner, so add a clock_on bool and skip redundant calls.
> 
> This fixes kernel oops like this one:
> 
> [   19.811613] gc0310_s_config S
> [   19.811655] [ cut here ]
> [   19.811664] WARNING: CPU: 1 PID: 720 at drivers/clk/clk.c:594 
> clk_core_disabl
> [   19.811666] Modules linked in: tpm_crb(+) snd_soc_sst_atom_hifi2_platform 
> tpm
> [   19.811744] CPU: 1 PID: 720 Comm: systemd-udevd Tainted: G C OE   
> 4.1
> [   19.811746] Hardware name: Insyde T701/T701, BIOS BYT70A.YNCHENG.WIN.007 
> 08/2
> [   19.811749] task: 988df7ab2500 task.stack: ac1400474000
> [   19.811752] RIP: 0010:clk_core_disable+0xc0/0x130
> ...
> [   19.811775] Call Trace:
> [   19.811783]  clk_core_disable_lock+0x1f/0x30
> [   19.811788]  clk_disable+0x1f/0x30
> [   19.811794]  gmin_flisclk_ctrl+0x87/0xf0
> [   19.811801]  0xc0528512
> [   19.811805]  0xc05295e2
> [   19.811811]  ? acpi_device_wakeup_disable+0x50/0x60
> [   19.811815]  ? acpi_dev_pm_attach+0x8e/0xd0
> [   19.811818]  ? 0xc05294d0
> [   19.811823]  i2c_device_probe+0x1cd/0x280
> [   19.811828]  driver_probe_device+0x2ff/0x450
> 
> Fixes: "staging: atomisp: use clock framework for camera clocks"
> Signed-off-by: Hans de Goede 
> ---
>  .../media/atomisp/platform/intel-mid/atomisp_gmin_platform.c   | 7 
> +++
>  1 file changed, 7 insertions(+)
> 
> diff --git 
> a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
> b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> index 828fe5abd832..6671ebe4ecc9 100644
> --- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> @@ -29,6 +29,7 @@ struct gmin_subdev {
>   struct v4l2_subdev *subdev;
>   int clock_num;
>   int clock_src;
> + bool clock_on;
>   struct clk *pmc_clk;
>   struct gpio_desc *gpio0;
>   struct gpio_desc *gpio1;
> @@ -583,6 +584,9 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, 
> int on)
>   struct gmin_subdev *gs = find_gmin_subdev(subdev);
>   struct i2c_client *client = v4l2_get_subdevdata(subdev);
>  
> + if (gs->clock_on == !!on)
> + return 0;
> +
>   if (on) {
>   ret = clk_set_rate(gs->pmc_clk, gs->clock_src);

Which tree [and branch] are you working off please? In the staging-next branch 
of Greg's staging
tree this function does not appear as it is in this patch.

thanks,
Tobin.

[PATCH] stagin: atomisp: Fix oops by unbalanced clk enable/disable call

[Hans de Goede]

The common-clk core expects clk consumers to always call enable/disable
in a balanced manner. The atomisp driver does not call gmin_flisclk_ctrl()
in a balanced manner, so add a clock_on bool and skip redundant calls.

This fixes kernel oops like this one:

[   19.811613] gc0310_s_config S
[   19.811655] [ cut here ]
[   19.811664] WARNING: CPU: 1 PID: 720 at drivers/clk/clk.c:594 clk_core_disabl
[   19.811666] Modules linked in: tpm_crb(+) snd_soc_sst_atom_hifi2_platform tpm
[   19.811744] CPU: 1 PID: 720 Comm: systemd-udevd Tainted: G C OE   4.1
[   19.811746] Hardware name: Insyde T701/T701, BIOS BYT70A.YNCHENG.WIN.007 08/2
[   19.811749] task: 988df7ab2500 task.stack: ac1400474000
[   19.811752] RIP: 0010:clk_core_disable+0xc0/0x130
...
[   19.811775] Call Trace:
[   19.811783]  clk_core_disable_lock+0x1f/0x30
[   19.811788]  clk_disable+0x1f/0x30
[   19.811794]  gmin_flisclk_ctrl+0x87/0xf0
[   19.811801]  0xc0528512
[   19.811805]  0xc05295e2
[   19.811811]  ? acpi_device_wakeup_disable+0x50/0x60
[   19.811815]  ? acpi_dev_pm_attach+0x8e/0xd0
[   19.811818]  ? 0xc05294d0
[   19.811823]  i2c_device_probe+0x1cd/0x280
[   19.811828]  driver_probe_device+0x2ff/0x450

Fixes: "staging: atomisp: use clock framework for camera clocks"
Signed-off-by: Hans de Goede 
---
 .../media/atomisp/platform/intel-mid/atomisp_gmin_platform.c   | 7 +++
 1 file changed, 7 insertions(+)

diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 828fe5abd832..6671ebe4ecc9 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -29,6 +29,7 @@ struct gmin_subdev {
struct v4l2_subdev *subdev;
int clock_num;
int clock_src;
+   bool clock_on;
struct clk *pmc_clk;
struct gpio_desc *gpio0;
struct gpio_desc *gpio1;
@@ -583,6 +584,9 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, 
int on)
struct gmin_subdev *gs = find_gmin_subdev(subdev);
struct i2c_client *client = v4l2_get_subdevdata(subdev);
 
+   if (gs->clock_on == !!on)
+   return 0;
+
if (on) {
ret = clk_set_rate(gs->pmc_clk, gs->clock_src);
 
@@ -591,8 +595,11 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, 
int on)
gs->clock_src);
 
ret = clk_prepare_enable(gs->pmc_clk);
+   if (ret == 0)
+   gs->clock_on = true;
} else {
clk_disable_unprepare(gs->pmc_clk);
+   gs->clock_on = false;
}
 
return ret;
-- 
2.14.2