From mboxrd@z Thu Jan 1 00:00:00 1970 From: Viresh Kumar Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes Date: Tue, 15 Sep 2015 16:34:47 +0530 Message-ID: <20150915110447.GI6350@linux> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> <1442313464.1914.21.camel@sipsolutions.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1442313464.1914.21.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Johannes Berg Cc: "open list:NETWORKING DRIVERS (WIRELESS)" , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , "Altman, Avri" , Stanislaw Gruszka , Jiri Slaby , "open list:DOCUMENTATION" , Peter Zijlstra , Catalin Marinas , Sebastian Andrzej Siewior , Will Deacon , Jaroslav Kysela , "open list:MEMORY MANAGEMENT" , Kalle Valo , "Grumbach, Emmanuel" , "Coelho, Luciano" , Wang Long , Richard Fitzgerald , Ingo Molnar , open list List-Id: linux-acpi@vger.kernel.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_ 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