Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Mikael Pettersson]

On Mon, 19 Mar 2007 18:42:22 +0100, Jesper Juhl wrote:
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
>  		if (err)
>  			goto out_flush_work;
>  
> -		device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
> +		err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
> +		if (err)
> +			goto out_flush_work;
> +
>  		/* to be cleaned up... */
>  		disks[drive]->private_data = (void *)(long)drive;
>  		disks[drive]->queue = floppy_queue;

The floppy driver's sysfs file just provides some auxiliary
information to user-space, none of which matters for most of
its users. It is IMO totally inappropriate to fail floppy
driver init in this case.

/Mikael

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Jesper Juhl]

On 20/03/07, Mikael Pettersson <mikpe@it.uu.se> wrote:
> On Mon, 19 Mar 2007 18:42:22 +0100, Jesper Juhl wrote:
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
> >               if (err)
> >                       goto out_flush_work;
> >
> > -             device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
> > +             err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
> > +             if (err)
> > +                     goto out_flush_work;
> > +
> >               /* to be cleaned up... */
> >               disks[drive]->private_data = (void *)(long)drive;
> >               disks[drive]->queue = floppy_queue;
>
> The floppy driver's sysfs file just provides some auxiliary
> information to user-space, none of which matters for most of
> its users. It is IMO totally inappropriate to fail floppy
> driver init in this case.
>

Which is why my original patch just output a warning to let the user
know that creating the file had failed.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Dmitriy Monakhov]

"Jesper Juhl" <jesper.juhl@gmail.com> writes:

> On 20/03/07, Mikael Pettersson <mikpe@it.uu.se> wrote:
>> On Mon, 19 Mar 2007 18:42:22 +0100, Jesper Juhl wrote:
>> > --- a/drivers/block/floppy.c
>> > +++ b/drivers/block/floppy.c
>> > @@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
>> >               if (err)
>> >                       goto out_flush_work;
>> >
>> > -             device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
>> > +             err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
>> > +             if (err)
>> > +                     goto out_flush_work;
>> > +
>> >               /* to be cleaned up... */
>> >               disks[drive]->private_data = (void *)(long)drive;
>> >               disks[drive]->queue = floppy_queue;
>>
>> The floppy driver's sysfs file just provides some auxiliary
>> information to user-space, none of which matters for most of
>> its users. It is IMO totally inappropriate to fail floppy
>> driver init in this case.
>>
>
> Which is why my original patch just output a warning to let the user
> know that creating the file had failed.
Ohh please... First of all you have to make you patch correct, and only
after this think about tiny shiny error massages.
Lets look at original code:
              err = platform_device_register(&floppy_device[drive]);
              if (err)
                        goto out_flush_work;

              device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
<<<<<<<<< You propose  "goto out_flush_work" here if device_create_file() failed
<<<<<<<<< ,as it was done if platform_device_register() failed. But at the 
<<<<<<<<< moment we call device_create_file() platform_device_register()
<<<<<<<<< succeed, this means we have to unregister it first and only after
<<<<<<<<< this jumpto out_flush_work.
>
> -- 
> Jesper Juhl <jesper.juhl@gmail.com>
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please      http://www.expita.com/nomime.html
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Andreas Kleen]

> The floppy driver's sysfs file just provides some auxiliary
> information to user-space, none of which matters for most of
> its users. It is IMO totally inappropriate to fail floppy
> driver init in this case.

I thought it was for udev to create the device nodes? But
I might be wrong on that.

If it's ok to ignore such failures there sometimes then the warning
attribute should
be dropped. But just shutting the warning up is the wrong change.

Beides if floppy doesn't need sysfs for anything why does it
initialize sysfs? IMHO sysfs should be only used if there is a
good reason, otherwise it is just wasted memory.

-Andi

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Mikael Pettersson]

On Tue, 20 Mar 2007 12:29:49 +0100 (CET), Andreas Kleen <ak@suse.de> wrote:
> > The floppy driver's sysfs file just provides some auxiliary
> > information to user-space, none of which matters for most of
> > its users. It is IMO totally inappropriate to fail floppy
> > driver init in this case.
> 
> I thought it was for udev to create the device nodes? But
> I might be wrong on that.

It's a file called "cmos" containing the ASCII
representation of UDP->cmos, which appears to be some
kind of "type" indicator. See floppy_cmos_show().

The file is only readable, not even root can write to it.

On one of my machines it contains the value "4", but the
device nodes udev created are still just /dev/fd0 and
/dev/floppy -> /dev/fd0, so I don't think it affects udev.

The "cmos" file seems to be a relatively new addition,
as another machine here running an RHEL4 2.6.9 kernel
doesn't have it.

Unless someone can prove that it's useful I think it
should be removed.

/Mikael

[PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Jesper Juhl]

    This fixes the warning
      warning: ignoring return value of `device_create_file', declared with attribute warn_unused_result in function `floppy_init'.
    It does this by checking the return value and printing a warning message in case of no success.
    
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Acked-by: Alan Cox <alan@redhat.com>
Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 577c621..9d543f3 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
 		if (err)
 			goto out_flush_work;
 
-		device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
+		err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
+		if (err)
+			printk(KERN_WARNING "Unable to create sysfs attribute "
+				"file for floppy device: %s\n", 
+				floppy_device[drive].name);
+
 		/* to be cleaned up... */
 		disks[drive]->private_data = (void *)(long)drive;
 		disks[drive]->queue = floppy_queue;

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Andi Kleen]

>  
> -		device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
> +		err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
> +		if (err)
> +			printk(KERN_WARNING "Unable to create sysfs attribute "
> +				"file for floppy device: %s\n", 
> +				floppy_device[drive].name);

That change looks pretty useless. Either the error should be handled correctly
by bailing out or the warn_unused_results should be dropped.

-Andi

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Jesper Juhl]

On 19/03/07, Andi Kleen <ak@suse.de> wrote:
>
> >
> > -             device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
> > +             err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
> > +             if (err)
> > +                     printk(KERN_WARNING "Unable to create sysfs attribute "
> > +                             "file for floppy device: %s\n",
> > +                             floppy_device[drive].name);
>
> That change looks pretty useless. Either the error should be handled correctly
> by bailing out or the warn_unused_results should be dropped.
>
At least letting the user know that something failed is better than
the current situation of just failing silently I'd say.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Andi Kleen]

On Monday 19 March 2007 16:16, Jesper Juhl wrote:
> On 19/03/07, Andi Kleen <ak@suse.de> wrote:
> >
> > >
> > > -             device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
> > > +             err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
> > > +             if (err)
> > > +                     printk(KERN_WARNING "Unable to create sysfs attribute "
> > > +                             "file for floppy device: %s\n",
> > > +                             floppy_device[drive].name);
> >
> > That change looks pretty useless. Either the error should be handled correctly
> > by bailing out or the warn_unused_results should be dropped.
> >
> At least letting the user know that something failed is better than
> the current situation of just failing silently I'd say.

I don't think so. That's just bloat.

-Andi

Re: [PATCH][5/5][resend] floppy.c: Fix device_create_file() warning

[Jesper Juhl]

On Monday 19 March 2007 16:20:00 Andi Kleen wrote:
> On Monday 19 March 2007 16:16, Jesper Juhl wrote:
> > On 19/03/07, Andi Kleen <ak@suse.de> wrote:
> > > > -            
> > > > device_create_file(&floppy_device[drive].dev,&dev_attr_cmos); +      
> > > >       err = device_create_file(&floppy_device[drive].dev,
> > > > &dev_attr_cmos); +             if (err)
> > > > +                     printk(KERN_WARNING "Unable to create sysfs
> > > > attribute " +                             "file for floppy device:
> > > > %s\n", +                             floppy_device[drive].name);
> > >
> > > That change looks pretty useless. Either the error should be handled
> > > correctly by bailing out or the warn_unused_results should be dropped.
> >
> > At least letting the user know that something failed is better than
> > the current situation of just failing silently I'd say.
>
> I don't think so. That's just bloat.
>

Ok, how about one of the following then?  
Personally I'd be ok with either one.


Either this : 

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 577c621..9d543f3 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
 		if (err)
 			goto out_flush_work;
 
-		device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
+		err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
+		if (err) {
+			printk(KERN_WARNING "Unable to create sysfs attribute "
+				"file for floppy device: %s\n", 
+				floppy_device[drive].name);
+			goto out_flush_work;
+		}
+
 		/* to be cleaned up... */
 		disks[drive]->private_data = (void *)(long)drive;
 		disks[drive]->queue = floppy_queue;


or this : 


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 577c621..9d543f3 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4302,7 +4302,12 @@ static int __init floppy_init(void)
 		if (err)
 			goto out_flush_work;
 
-		device_create_file(&floppy_device[drive].dev,&dev_attr_cmos);
+		err = device_create_file(&floppy_device[drive].dev, &dev_attr_cmos);
+		if (err)
+			goto out_flush_work;
+
 		/* to be cleaned up... */
 		disks[drive]->private_data = (void *)(long)drive;
 		disks[drive]->queue = floppy_queue;