Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

This email has far too many people Cc'ed on it - I don't think vger is
even accepting it for that reason. You should probably restrict it to
just a few lists when you resubmit.

> The problem with current code is that it reads/writes 4 bytes for a
> boolean, which will read/update 3 excess bytes following the boolean
> variable (when sizeof(bool) is 1 byte). And that can lead to hard to 
> fix bugs. It was a nightmare cracking this one.

Unless you're ignoring (or worse, casting away) type warnings, there's
no problem/bug at all, you just have to define all the variables used
with debugfs_create_bool() as actual u32 variables.

It sounds like you are/were doing something like the following:

bool a, b, c;
...
debugfs_create_bool("a", 0600, dir, (u32 *)&a);

which is quite clearly invalid.

Had you properly defined them as u32, as everyone (except for the ACPI
case) does, there wouldn't have been any problem:

u32 a, b, c;
...
debugfs_create_bool("a", 0600, dir, &a);


As far as I can tell, there's no bug in the API. It might be a bit
strange to have a set of functions called debugfs_create_<type> and
then one of them doesn't actually use the type from the name, but
that's only a problem if you blindly add casts or ignore the compiler
warnings you'd get without casts.

In other words, I think your commit log is extremely misleading. The
API perhaps has some inconsistent naming, but all this talk about the
sizeof(bool) etc. is simply completely irrelevant since "bool" is not
the type used here at all. There's nothing to fix in any of the code
you're changing (again, apart from ACPI.)

That said, I don't actually object to this change itself, being able to
actually use bool variables with debugfs_create_bool would be nice.
However, that shouldn't be documented as a bugfix or anything like
that, merely as a cleanup to make the API naming more consistent and to
be able to use the (smaller and often more convenient) bool type.

Clearly, it would also lead to less confusion, as we see in ACPI and
hear from your OPP code. Note that ACPI is even more confused though
since it uses "unsigned long", so it's entirely possible that somebody
actually thought about that case and decided not to worry about 64-bit
big-endian platforms.

Of course this also means that only the ACPI patch is a candidate for s
table.

johannes

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>