Re: [mm Patch] isdn4linux: Gigaset driver: fix __must_check warning

Re: [mm Patch] isdn4linux: Gigaset driver: fix __must_check warning

[Andrew Morton]

Tilman Schmidt <tilman@imap.cc> wrote:
>
> This patch to the Siemens Gigaset driver fixes the compile warning
> "ignoring return value of 'class_device_create_file', declared with
> attribute warn_unused_result" appearing with CONFIG_ENABLE_MUST_CHECK=y
> in release 2.6.18-rc1-mm1.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Acked-by: Hansjoerg Lipp <hjlipp@web.de>
> ---
> 
>  proc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.18-rc1-orig/drivers/isdn/gigaset/proc.c	2006-07-09 17:19:49.000000000 +0200
> +++ linux-2.6.18-rc1-mm1-work/drivers/isdn/gigaset/proc.c	2006-07-09 18:31:15.000000000 +0200
> @@ -83,5 +83,6 @@ void gigaset_init_dev_sysfs(struct cards
>  		return;
>  
>  	gig_dbg(DEBUG_INIT, "setting up sysfs");
> -	class_device_create_file(cs->class, &class_device_attr_cidmode);
> +	if (class_device_create_file(cs->class, &class_device_attr_cidmode))
> +		dev_warn(cs->dev, "could not create sysfs attribute\n");
>  }

hm.

With this change we'll emit a warning (actually it's an error - I'll make
it dev_err(), OK?) and then we'll continue execution, pretending that the
sysfs file actually got registered.  Later, we'll try to unregister a
not-registered sysfs file.

So it's all a bit flakey when you look at it in a dumb fashion.

But I think the patch is OK - if that class_device_create_file() fails,
then there's some other bug somewhere, and the warning you've added is
sufficient - it tells the developers what the initial failure was, when it
happens.  So later, if someone reports a crash, we'll see that warning in
their logs and it'll lead us to the real bug.  We certainly couldn't
justify adding additional code which attempts to "continue working" if the
class_device_create_file() fails, because it just shouldn't fail.


It's probable that the message will never come out ever, so it's not worth
adding a ton of code to support this.

It'd be better if we had a class_device_create_file_warn() which does the
warning for you: its semantics are "this is expected to succeed".  But if
we do that to class_device_create_file() then we'd need to do it to 200
other things too.

Re: [mm Patch] isdn4linux: Gigaset driver: fix __must_check warning

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On 11.07.2006 23:51, Andrew Morton wrote:

> Tilman Schmidt <tilman@imap.cc> wrote:
> 
>>-	class_device_create_file(cs->class, &class_device_attr_cidmode);
>>+	if (class_device_create_file(cs->class, &class_device_attr_cidmode))
>>+		dev_warn(cs->dev, "could not create sysfs attribute\n");
> 
> With this change we'll emit a warning (actually it's an error - I'll make
> it dev_err(), OK?)

Fine with me. It's not fatal to the driver which is quite capable of
operating without that sysfs file, but ok, it *is* an error, and in
fact, if class_device_create_file() fails there must be something
seriously wrong.

> and then we'll continue execution, pretending that the
> sysfs file actually got registered.  Later, we'll try to unregister a
> not-registered sysfs file.

Well, from my reading of the source, class_device_remove_file() should
be able to cope with that. The alternative would be to save off the
fact that the original creation failed somewhere in the driver data,
for the sole purpose of avoiding calling class_device_remove_file()
for it later.

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]