atmel_isi: allocate memory to store the isi platform data.
Subsystems (2)
PATCH 1/1 atmel_isi: allocate memory to store the isi platform data.
This patch fix the bug: ISI driver's platform data became invalid when isi platform data's attribution is __initdata.
If the isi platform data is passed as __initdata. Then we need store it in driver allocated memory. otherwise when we use it out of the probe() function, then the isi platform data is invalid.
--- drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/media/platform/soc_camera/atmel-isi.c +11 -1
@@ -926,6 +926,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)clk_put (isi->mck);clk_unprepare (isi->pclk);clk_put (isi->pclk); +kfree (isi->pdata);kfree (isi);return 0 ;
@@ -968,8 +969,15 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_alloc_isi; } + isi->pdata =kzalloc (sizeof (struct isi_platform_data ),GFP_KERNEL ); +if (!isi->pdata) { + ret = -ENOMEM ; +dev_err (&pdev->dev,"Can't allocate isi platform data!\n" ); + goto err_alloc_isi_pdata; + } +memcpy (isi->pdata, pdata,sizeof (struct isi_platform_data )); + isi->pclk = pclk; - isi->pdata = pdata; isi->active =NULL ;spin_lock_init (&isi->lock);init_waitqueue_head (&isi->vsync_wq);
@@ -1073,6 +1081,8 @@ err_set_mck_rate: err_clk_prepare_mck:clk_put (isi->mck); err_clk_get: +kfree (isi->pdata); +err_alloc_isi_pdata:kfree (isi); err_alloc_isi:clk_unprepare (pclk);
Signature · 1 line
1.7.9.5
PATCH 1/1 atmel_isi: allocate memory to store the isi platform data.
Hello,
Quoted 14 lines · Josh Wu
This patch fix the bug: ISI driver's platform data became invalid when isi platform data's attribution is
__initdata.If the isi platform data is passed as
__initdata. Then we need store it in driver allocated memory. otherwise when we use it out of theprobe()function, then the isi platform data is invalid.Signed-off-by Josh Wu <[email protected]>--- drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/media/platform/soc_camera/atmel-isi.c +0
-0
drivers/media/platform/soc_camera/atmel-isi.c static int __devexit atmel_isi_remove(struct platform_device *pdev) · line 929
Quoted 4 lines
drivers/media/platform/soc_camera/atmel-isi.c +1
-0@@ -926,6 +926,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)clk_put (isi->mck);clk_unprepare (isi->pclk);clk_put (isi->pclk); +kfree (isi->pdata);
Not needed if you use devm_kzalloc(). See below.
Quoted 5 lines
drivers/media/platform/soc_camera/atmel-isi.c +0
-0kfree (isi);return 0 ;@@ -968,8 +969,15 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_alloc_isi;
drivers/media/platform/soc_camera/atmel-isi.c static int __devinit atmel_isi_probe(struct platform_device *pdev) · line 979
Quoted 10 lines
drivers/media/platform/soc_camera/atmel-isi.c +8
-0} + isi->pdata =kzalloc (sizeof (struct isi_platform_data ),GFP_KERNEL ); +if (!isi->pdata) { + ret = -ENOMEM ; +dev_err (&pdev->dev,"Can't allocate isi platform data!\n" ); + goto err_alloc_isi_pdata; + } +memcpy (isi->pdata, pdata,sizeof (struct isi_platform_data )); +
It is more idiomatic to use sizeof(*isi->pdata) in kzalloc() and memcpy() calls to be resilient to future type changes.
You may also want to use dev_kzalloc() which frees memory automagically on driver detach.
Quoted 7 lines
drivers/media/platform/soc_camera/atmel-isi.c +0
-1isi->pclk = pclk; - isi->pdata = pdata; isi->active =NULL ;spin_lock_init (&isi->lock);init_waitqueue_head (&isi->vsync_wq);
drivers/media/platform/soc_camera/atmel-isi.c err_set_mck_rate: · line 1088
Quoted 8 lines
drivers/media/platform/soc_camera/atmel-isi.c +2
-0@@ -1073,6 +1081,8 @@ err_set_mck_rate: err_clk_prepare_mck:clk_put (isi->mck); err_clk_get: +kfree (isi->pdata); +err_alloc_isi_pdata:kfree (isi); err_alloc_isi:clk_unprepare (pclk);
Best regards,
Nicolas
PATCH 1/1 atmel_isi: allocate memory to store the isi platform data.
In reply to Josh WuHi,
Quoted 26 lines · Josh Wu
This patch fix the bug: ISI driver's platform data became invalid when isi platform data's attribution is
__initdata.If the isi platform data is passed as
__initdata. Then we need store it in driver allocated memory. otherwise when we use it out of theprobe()function, then the isi platform data is invalid.Signed-off-by Josh Wu <[email protected]>--- drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/media/platform/soc_camera/atmel-isi.c +1
-0@@ -926,6 +926,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)clk_put (isi->mck);clk_unprepare (isi->pclk);clk_put (isi->pclk); +kfree (isi->pdata);kfree (isi);return 0 ;@@ -968,8 +969,15 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_alloc_isi;
drivers/media/platform/soc_camera/atmel-isi.c static int __devinit atmel_isi_probe(struct platform_device *pdev) · line 979
Quoted 10 lines
drivers/media/platform/soc_camera/atmel-isi.c +8
-0} + isi->pdata =kzalloc (sizeof (struct isi_platform_data ),GFP_KERNEL ); +if (!isi->pdata) { + ret = -ENOMEM ; +dev_err (&pdev->dev,"Can't allocate isi platform data!\n" ); + goto err_alloc_isi_pdata; + } +memcpy (isi->pdata, pdata,sizeof (struct isi_platform_data )); +
Why not just embed struct isi_platform_data in struct atmel_isi and drop this another kzalloc() ?
Then you could simply do isi->pdata = *pdata.
Also, is this going to work when this driver is build and as a module and its loading is deferred past system booting ? At that time the driver's platform data may be well discarded. You may wan't to duplicate it on the running boards in board code with kmemdup() or something.
--
Regards,
Sylwester
PATCH 1/1 atmel_isi: allocate memory to store the isi platform data.
Quoted 44 lines · Sylwester Nawrocki
Hi,
Quoted 26 lines · Josh Wu
This patch fix the bug: ISI driver's platform data became invalid when isi platform data's attribution is
__initdata.If the isi platform data is passed as
__initdata. Then we need store it in driver allocated memory. otherwise when we use it out of theprobe()function, then the isi platform data is invalid.Signed-off-by Josh Wu <[email protected]>--- drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/media/platform/soc_camera/atmel-isi.c +1
-0@@ -926,6 +926,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)clk_put (isi->mck);clk_unprepare (isi->pclk);clk_put (isi->pclk); +kfree (isi->pdata);kfree (isi);return 0 ;@@ -968,8 +969,15 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_alloc_isi;drivers/media/platform/soc_camera/atmel-isi.c static int __devinit atmel_isi_probe(struct platform_device *pdev) · line 979
Quoted 10 lines
drivers/media/platform/soc_camera/atmel-isi.c +8
-0} + isi->pdata =kzalloc (sizeof (struct isi_platform_data ),GFP_KERNEL ); +if (!isi->pdata) { + ret = -ENOMEM ; +dev_err (&pdev->dev,"Can't allocate isi platform data!\n" ); + goto err_alloc_isi_pdata; + } +memcpy (isi->pdata, pdata,sizeof (struct isi_platform_data )); +Why not just embed struct
isi_platform_datain structatmel_isiand drop this anotherkzalloc()?
Then you could simply doisi->pdata= *pdata.
Quoted 3 lines
Also, is this going to work when this driver is build and as a module and its loading is deferred past system booting ? At that time the driver's platform data may be well discarded.
Right, it will be gone, I think.
Quoted 2 lines
You may wan't to duplicate it on the
running boards in board code withkmemdup()or something.
How about removing __initdata from board code?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
PATCH 1/1 atmel_isi: allocate memory to store the isi platform data.
Hi, all
Sorry, My mistake here. After checking the code, this ISI bug doesn't exist in current mainline code. So I will *cancel* this patch.
Since current mainline will copy this __initdata isi platform data to one static structure in function at91_add_device_isi(...). Then pass this static structure to the driver.
So the ISI driver has no bug that isi platform became invalid. I meet this is because I'm not call the at91_add_device_isi(...) since I try in the DT support board.
At last, even no above bug in the code, This isi_platform_data is still need to stored in ISI driver. Since if we support DT then we need this isi platform data and the function at91_add_device_isi(...) will not to be called (it is in device file).
So I think after soc-camera DT support is merged. Then I will send a DT support patch for ISI driver which will embed the isi_platform_data into atmel_isi.
Thank you all for the replies. That helps a lot even in this small patch. :)
Quoted 54 lines · Guennadi Liakhovetski
Quoted 43 lines · Sylwester Nawrocki
Hi,
Quoted 26 lines · Josh Wu
This patch fix the bug: ISI driver's platform data became invalid when isi platform data's attribution is
__initdata.If the isi platform data is passed as
__initdata. Then we need store it in driver allocated memory. otherwise when we use it out of theprobe()function, then the isi platform data is invalid.Signed-off-by Josh Wu <[email protected]>--- drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/media/platform/soc_camera/atmel-isi.c +1
-0@@ -926,6 +926,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)clk_put (isi->mck);clk_unprepare (isi->pclk);clk_put (isi->pclk); +kfree (isi->pdata);kfree (isi);return 0 ;@@ -968,8 +969,15 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_alloc_isi;drivers/media/platform/soc_camera/atmel-isi.c static int __devinit atmel_isi_probe(struct platform_device *pdev) · line 979
Quoted 10 lines
drivers/media/platform/soc_camera/atmel-isi.c +8
-0} + isi->pdata =kzalloc (sizeof (struct isi_platform_data ),GFP_KERNEL ); +if (!isi->pdata) { + ret = -ENOMEM ; +dev_err (&pdev->dev,"Can't allocate isi platform data!\n" ); + goto err_alloc_isi_pdata; + } +memcpy (isi->pdata, pdata,sizeof (struct isi_platform_data )); +Why not just embed struct
isi_platform_datain structatmel_isiand drop this anotherkzalloc()?
Then you could simply doisi->pdata= *pdata.Quoted 3 lines
Also, is this going to work when this driver is build and as a module and its loading is deferred past system booting ? At that time the driver's platform data may be well discarded.
Right, it will be gone, I think.
Quoted 2 lines
You may wan't to duplicate it on the
running boards in board code withkmemdup()or something.How about removing
__initdatafrom board code?
Quoted 10 lines
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Best Regards,
Josh Wu