Return-Path: Message-ID: <1442313464.1914.21.camel@sipsolutions.net> Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes From: Johannes Berg To: Viresh Kumar , "gregkh@linuxfoundation.org" Cc: "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 Date: Tue, 15 Sep 2015 12:37:44 +0200 In-Reply-To: <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: 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_ 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