[PATCH] media: saa7134 fix media_dev alloc error path to not free when alloc fails

[PATCH] media: saa7134 fix media_dev alloc error path to not free when alloc fails

[Shuah Khan]

media_dev alloc error path does kfree when alloc fails. Fix it to not call
kfree when media_dev alloc fails.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/pci/saa7134/saa7134-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
index c0e1780..eab2684 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -1046,7 +1046,7 @@ static int saa7134_initdev(struct pci_dev *pci_dev,
 	dev->media_dev = kzalloc(sizeof(*dev->media_dev), GFP_KERNEL);
 	if (!dev->media_dev) {
 		err = -ENOMEM;
-		goto fail0;
+		goto media_dev_alloc_fail;
 	}
 	media_device_pci_init(dev->media_dev, pci_dev, dev->name);
 	dev->v4l2_dev.mdev = dev->media_dev;
@@ -1309,6 +1309,7 @@ static int saa7134_initdev(struct pci_dev *pci_dev,
  fail0:
 #ifdef CONFIG_MEDIA_CONTROLLER
 	kfree(dev->media_dev);
+ media_dev_alloc_fail:
 #endif
 	kfree(dev);
 	return err;
-- 
2.5.0

Re: [PATCH] media: saa7134 fix media_dev alloc error path to not free when alloc fails

[Mauro Carvalho Chehab]

Em Thu, 14 Apr 2016 10:31:20 -0600
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> media_dev alloc error path does kfree when alloc fails. Fix it to not call
> kfree when media_dev alloc fails.

No need. kfree(NULL) is OK.

Adding a label inside a conditional block is ugly.

> 

> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/pci/saa7134/saa7134-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-core.c b/drivers/media/pci/saa7134/saa7134-core.c
> index c0e1780..eab2684 100644
> --- a/drivers/media/pci/saa7134/saa7134-core.c
> +++ b/drivers/media/pci/saa7134/saa7134-core.c
> @@ -1046,7 +1046,7 @@ static int saa7134_initdev(struct pci_dev *pci_dev,
>  	dev->media_dev = kzalloc(sizeof(*dev->media_dev), GFP_KERNEL);
>  	if (!dev->media_dev) {
>  		err = -ENOMEM;
> -		goto fail0;
> +		goto media_dev_alloc_fail;
>  	}
>  	media_device_pci_init(dev->media_dev, pci_dev, dev->name);
>  	dev->v4l2_dev.mdev = dev->media_dev;
> @@ -1309,6 +1309,7 @@ static int saa7134_initdev(struct pci_dev *pci_dev,
>   fail0:
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	kfree(dev->media_dev);
> + media_dev_alloc_fail:
>  #endif
>  	kfree(dev);
>  	return err;


-- 
Thanks,
Mauro

Re: [PATCH] media: saa7134 fix media_dev alloc error path to not free when alloc fails

[Shuah Khan]

On 04/14/2016 03:08 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 14 Apr 2016 10:31:20 -0600
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> media_dev alloc error path does kfree when alloc fails. Fix it to not call
>> kfree when media_dev alloc fails.
> 
> No need. kfree(NULL) is OK.

Agreed.

> 
> Adding a label inside a conditional block is ugly.

In this case, if label is in normal path, we will see defined, but not
used warnings when condition isn't defined. We seem to have many such
cases for CONFIG_MEDIA_CONTROLLER :(

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America(Silicon Valley)
shuah.kh@samsung.com | (970) 217-8978

Re: [PATCH] media: saa7134 fix media_dev alloc error path to not free when alloc fails

[Mauro Carvalho Chehab]

Em Thu, 14 Apr 2016 16:18:17 -0600
Shuah Khan <shuah.kh@samsung.com> escreveu:

> On 04/14/2016 03:08 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 14 Apr 2016 10:31:20 -0600
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >   
> >> media_dev alloc error path does kfree when alloc fails. Fix it to not call
> >> kfree when media_dev alloc fails.  
> > 
> > No need. kfree(NULL) is OK.  
> 
> Agreed.
> 
> > 
> > Adding a label inside a conditional block is ugly.  
> 
> In this case, if label is in normal path, we will see defined, but not
> used warnings when condition isn't defined. 

True, but we don't need a label here, as kfree() can be called with a null
pointer.

> We seem to have many such
> cases for CONFIG_MEDIA_CONTROLLER :(

We may try to address those media-controller dependent code latter on.

I have some ideas of adding some macros and helper functions to allow
getting rid of those ifdefs and not add extra code if !MEDIA_CONTROLLER,
but the better seems to first add MC support to ALSA and make the
enable/disable functions generic, and then cleanup the code to remove
those ifdefs.

> 
> thanks,
> -- Shuah
> 
> 


-- 
Thanks,
Mauro