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

[Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Hi Johannes,

On 15-09-15, 12:37, Johannes Berg wrote:
> 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.

Hmm, I know the list is too long and yes its blocked for Admin's
approval on most of the lists. But that's what was generated by
get_maintainers and I didn't wanted to miss cc'ing anybody who might
be able to catch a bug in there.

> > 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.

Yeah, that's right. Its just a trivial cleanup rather. What about this
simple changelog instead?

--
viresh

-------------------------8<-------------------------

Subject: [PATCH] debugfs: Pass bool pointer to debugfs_create_bool()

Its a bit odd that debugfs_create_bool() takes 'u32 *' as an argument,
when all it needs is a boolean pointer.

It would be better to update this API to make it accept 'bool *'
instead, as that will make it more consistent and often more convenient.
Over that bool takes just a byte.

That required updates to all user sites as well in a single commit.
regmap core was also using debugfs_{read|write}_file_bool(), directly
and variable types were updated for that to be bool as well.

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>