Return-Path: Date: Tue, 15 Sep 2015 16:34:47 +0530 From: Viresh Kumar To: Johannes Berg Cc: "gregkh@linuxfoundation.org" , "linaro-kernel@lists.linaro.org" , Rafael Wysocki , "sboyd@codeaurora.org" , "arnd@arndb.de" , Mark Brown , Akinobu Mita , Alexander Duyck , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Andrew Morton , Andy Lutomirski , Arik Nemtsov , "open list:QUALCOMM ATHEROS ATH10K WIRELESS DRIVER" , "open list:QUALCOMM ATHEROS ATH9K WIRELESS DRIVER" , "Altman, Avri" , "open list:B43 WIRELESS DRIVER" , Borislav Petkov , Brian Silverman , Catalin Marinas , Charles Keepax , "Ivgi, Chaya Rachel" , Davidlohr Bueso , Dmitry Monakhov , Doug Thompson , Eliad Peller , "Grumbach, Emmanuel" , Florian Fainelli , Gustavo Padovan , Haggai Eran , Hariprasad S , Ingo Molnar , Intel Linux Wireless , "open list:AMD IOMMU (AMD-VI)" , "James E.J. Bottomley" , Jaroslav Kysela , Jiri Slaby , Joe Perches , Joerg Roedel , Johan Hedberg , Johannes Weiner , Jonathan Corbet , Joonsoo Kim , Kalle Valo , Larry Finger , Len Brown , Liam Girdwood , "open list:ACPI" , "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" , "open list:BLUETOOTH DRIVERS" , "open list:DOCUMENTATION" , "open list:EDAC-CORE" , open list , "open list:MEMORY MANAGEMENT" , "open list:CISCO SCSI HBA DRIVER" , "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" , "open list:NETWORKING DRIVERS (WIRELESS)" , "Coelho, Luciano" , "Luis R. Rodriguez" , Marcel Holtmann , Mauro Carvalho Chehab , Mel Gorman , Michael Kerrisk , Michal Hocko , Narsimhulu Musini , "open list:CXGB4 ETHERNET DRIVER (CXGB4)" , Nick Kossifidis , "open list:WOLFSON MICROELECTRONICS DRIVERS" , Peter Zijlstra , QCA ath9k Development , Richard Fitzgerald , Sasha Levin , Sebastian Andrzej Siewior , Sebastian Ott , Sesidhar Baddela , Stanislaw Gruszka , Steven Rostedt , Takashi Iwai , Tejun Heo , Thomas Gleixner , "Winkler, Tomas" , Vlastimil Babka , Wang Long , Will Deacon Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes 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 In-Reply-To: <1442313464.1914.21.camel@sipsolutions.net> List-ID: 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