[PATCH] drivers/video/aty/radeon_base.c: compilation warning fix

[PATCH] drivers/video/aty/radeon_base.c: compilation warning fix

[Leonardo Potenza]

From: Leonardo Potenza <lpotenza@inwind.it>

Added a check for the sysfs_create_bin_file() return value

Signed-off-by: Leonardo Potenza <lpotenza@inwind.it>
---

The aim of this patch is to remove the following warning messages:
drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result

--- linux-2.6.orig/drivers/video/aty/radeon_base.c
+++ linux-2.6/drivers/video/aty/radeon_base.c
@@ -2329,11 +2329,23 @@ static int __devinit radeonfb_pci_regist
 	/* Build mode list, check out panel native model */
 	radeon_check_modes(rinfo, mode_option);
 
-	/* Register some sysfs stuff (should be done better) */
-	if (rinfo->mon1_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
-	if (rinfo->mon2_EDID)
-		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+	/* Register some sysfs stuff */
+	if (rinfo->mon1_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
+		if (ret != 0) {
+			printk( KERN_ERR "radeonfb (%s): cannot create edid1 bin file\n",
+				pci_name(rinfo->pdev));
+			goto err_unmap_fb;
+		}
+	}
+	if (rinfo->mon2_EDID) {
+		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+		if (ret != 0) {
+			printk( KERN_ERR "radeonfb (%s): cannot create edid2 bin file\n",
+				pci_name(rinfo->pdev));
+			goto err_remove_edid1_file;
+		}
+	}
 
 	/* save current mode regs before we switch into the new one
 	 * so we can restore this upon __exit
@@ -2357,7 +2369,7 @@ static int __devinit radeonfb_pci_regist
 	if (ret < 0) {
 		printk (KERN_ERR "radeonfb (%s): could not register framebuffer\n",
 			pci_name(rinfo->pdev));
-		goto err_unmap_fb;
+		goto err_remove_edid2_file;
 	}
 
 #ifdef CONFIG_MTRR
@@ -2376,6 +2388,12 @@ static int __devinit radeonfb_pci_regist
 	RTRACE("radeonfb_pci_register END\n");
 
 	return 0;
+err_remove_edid2_file:
+	if (rinfo->mon2_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
+err_remove_edid1_file:
+	if (rinfo->mon1_EDID)
+		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
 err_unmap_fb:
 	iounmap(rinfo->fb_base);
 err_unmap_rom:

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning

[Benjamin Herrenschmidt]

On Wed, 2008-01-02 at 19:01 +0100, Leonardo Potenza wrote:
> From: Leonardo Potenza <lpotenza@inwind.it>
> 
> Added a check for the sysfs_create_bin_file() return value
> 
> Signed-off-by: Leonardo Potenza <lpotenza@inwind.it>

NACK.

The warnings are stupid, this is a long argument we had with akpm among
others, I totally refuse to fail creating the framebuffer because
something went bonkers creating some sysfs file that are in no way
mandatory for the good operations of the fb.

There is a _shitload_ of cases where testing the result of those sysfs
calls is pure bloat.

Ben.

> 
> The aim of this patch is to remove the following warning messages:
> drivers/video/aty/radeon_base.c: In function 'radeonfb_pci_register':
> drivers/video/aty/radeon_base.c:2334: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> drivers/video/aty/radeon_base.c:2336: warning: ignoring return value of 'sysfs_create_bin_file', declared with attribute warn_unused_result
> 
> --- linux-2.6.orig/drivers/video/aty/radeon_base.c
> +++ linux-2.6/drivers/video/aty/radeon_base.c
> @@ -2329,11 +2329,23 @@ static int __devinit radeonfb_pci_regist
>  	/* Build mode list, check out panel native model */
>  	radeon_check_modes(rinfo, mode_option);
>  
> -	/* Register some sysfs stuff (should be done better) */
> -	if (rinfo->mon1_EDID)
> -		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
> -	if (rinfo->mon2_EDID)
> -		sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
> +	/* Register some sysfs stuff */
> +	if (rinfo->mon1_EDID) {
> +		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
> +		if (ret != 0) {
> +			printk( KERN_ERR "radeonfb (%s): cannot create edid1 bin file\n",
> +				pci_name(rinfo->pdev));
> +			goto err_unmap_fb;
> +		}
> +	}
> +	if (rinfo->mon2_EDID) {
> +		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
> +		if (ret != 0) {
> +			printk( KERN_ERR "radeonfb (%s): cannot create edid2 bin file\n",
> +				pci_name(rinfo->pdev));
> +			goto err_remove_edid1_file;
> +		}
> +	}
>  
>  	/* save current mode regs before we switch into the new one
>  	 * so we can restore this upon __exit
> @@ -2357,7 +2369,7 @@ static int __devinit radeonfb_pci_regist
>  	if (ret < 0) {
>  		printk (KERN_ERR "radeonfb (%s): could not register framebuffer\n",
>  			pci_name(rinfo->pdev));
> -		goto err_unmap_fb;
> +		goto err_remove_edid2_file;
>  	}
>  
>  #ifdef CONFIG_MTRR
> @@ -2376,6 +2388,12 @@ static int __devinit radeonfb_pci_regist
>  	RTRACE("radeonfb_pci_register END\n");
>  
>  	return 0;
> +err_remove_edid2_file:
> +	if (rinfo->mon2_EDID)
> +		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid2_attr);
> +err_remove_edid1_file:
> +	if (rinfo->mon1_EDID)
> +		sysfs_remove_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
>  err_unmap_fb:
>  	iounmap(rinfo->fb_base);
>  err_unmap_rom:

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning fix

[Leonardo Potenza]

On Wednesday 02 January 2008 21:19:33 Benjamin Herrenschmidt wrote:
> 
> On Wed, 2008-01-02 at 19:01 +0100, Leonardo Potenza wrote:
> > From: Leonardo Potenza <lpotenza@inwind.it>
> > 
> > Added a check for the sysfs_create_bin_file() return value
> > 
> > Signed-off-by: Leonardo Potenza <lpotenza@inwind.it>
> 
> NACK.
> 
> The warnings are stupid, this is a long argument we had with akpm among
> others, I totally refuse to fail creating the framebuffer because
> something went bonkers creating some sysfs file that are in no way
> mandatory for the good operations of the fb.
> 
> There is a _shitload_ of cases where testing the result of those sysfs
> calls is pure bloat.
> 
> Ben.

Sorry, I missed the former discussion.

Do you think that a solution like:

[...]
	if (rinfo->mon1_EDID) {
		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
		if (ret != 0)
			printk( KERN_WARNING "radeonfb (%s): cannot create edid1 bin file\n",
				pci_name(rinfo->pdev));
	}
[...]

would be acceptable in this case?

If you agree, I could re-submit the patch modified.

Thanks.

Leonardo

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning

[Benjamin Herrenschmidt]

> Do you think that a solution like:
> 
> [...]
> 	if (rinfo->mon1_EDID) {
> 		ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
> 		if (ret != 0)
> 			printk( KERN_WARNING "radeonfb (%s): cannot create edid1 bin file\n",
> 				pci_name(rinfo->pdev));
> 	}
> [...]
> 
> would be acceptable in this case?
> 
> If you agree, I could re-submit the patch modified.

Bloat bloat bloat... 

Ben.

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 03 Jan 2008 07:19:33 +1100

> 
> On Wed, 2008-01-02 at 19:01 +0100, Leonardo Potenza wrote:
> > From: Leonardo Potenza <lpotenza@inwind.it>
> > 
> > Added a check for the sysfs_create_bin_file() return value
> > 
> > Signed-off-by: Leonardo Potenza <lpotenza@inwind.it>
> 
> NACK.
> 
> The warnings are stupid, this is a long argument we had with akpm among
> others, I totally refuse to fail creating the framebuffer because
> something went bonkers creating some sysfs file that are in no way
> mandatory for the good operations of the fb.
> 
> There is a _shitload_ of cases where testing the result of those sysfs
> calls is pure bloat.

I agree.

In fact my first reaction to this patch is "Damn, I better test
to make sure I don't lose my console on my workstation because
of this stupid patch."

Just put (void)'s there or whatever, or we should get rid of
the mustcheck annotations.  Either way is fine with me.

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning fix

[Leonardo Potenza]

On Thursday 03 January 2008 01:32:13 Benjamin Herrenschmidt wrote:
> > Do you think that a solution like:
> > 
> > [...]
> >       if (rinfo->mon1_EDID) {
> >               ret = sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr);
> >               if (ret != 0)
> >                       printk( KERN_WARNING "radeonfb (%s): cannot create edid1 bin file\n",
> >                               pci_name(rinfo->pdev));
> >       }
> > [...]
> > 
> > would be acceptable in this case?
> > 
> > If you agree, I could re-submit the patch modified.
> 
> Bloat bloat bloat... 
> 
> Ben.
> 

I have to agree with you that my first proposal (avoid creating 
the framebuffer in case of a problem in the creation of a sysfs file) 
was a little bit "drastic" measure.

I thought that at least the user should be aware that something went
wrong in the driver initialization and that not all the features 
will be available at run-time. 
That's why I tried to add a specific warning message.

Anyway, I didn't intend to waste so much of your time with my patch
submission.

Sorry for that.

Leonardo

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning

[Benjamin Herrenschmidt]

On Thu, 2008-01-03 at 18:12 +0100, Leonardo Potenza wrote:
> I thought that at least the user should be aware that something went
> wrong in the driver initialization and that not all the features 
> will be available at run-time. 
> That's why I tried to add a specific warning message.
> 
> Anyway, I didn't intend to waste so much of your time with my patch
> submission.

The warning is annoying... see if you can find the shortest way in term
of code bloat to get rid of it...

Cheers,
Ben.

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning fix

[Leonardo Potenza]

On Thursday 03 January 2008 21:24:07 Benjamin Herrenschmidt wrote:
> 
> On Thu, 2008-01-03 at 18:12 +0100, Leonardo Potenza wrote:
> > I thought that at least the user should be aware that something went
> > wrong in the driver initialization and that not all the features 
> > will be available at run-time. 
> > That's why I tried to add a specific warning message.
> > 
> > Anyway, I didn't intend to waste so much of your time with my patch
> > submission.
> 
> The warning is annoying... see if you can find the shortest way in term
> of code bloat to get rid of it...
> 
> Cheers,
> Ben.
> 

Maybe something like:

	if (rinfo->mon1_EDID)
		WARN_ON(sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr));

could suppress the compilation warning while keeping the generation of a warning 
message in case of error.

I wonder if this solution could be acceptable for you.

It doesn't sound too bloat to me, but maybe you were thinking about something 
completely different...

Leonardo

Re: [PATCH] drivers/video/aty/radeon_base.c: compilation warning

[Benjamin Herrenschmidt]

> 	if (rinfo->mon1_EDID)
> 		WARN_ON(sysfs_create_bin_file(&rinfo->pdev->dev.kobj, &edid1_attr));
> 
> could suppress the compilation warning while keeping the generation of a warning 
> message in case of error.
> 
> I wonder if this solution could be acceptable for you.
> 
> It doesn't sound too bloat to me, but maybe you were thinking about something 
> completely different...

That would be allright I suppose.

Ben.