From mboxrd@z Thu Jan 1 00:00:00 1970 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 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> Mime-Version: 1.0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Viresh Kumar , "gregkh@linuxfoundation.org" 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 , Johan Hedberg , Davidlohr Bueso , Joonsoo Kim , Jonathan Corbet , Joerg Roedel , "open list:WOLFSON MICROELECTRONICS DRIVERS" , Sebastian Ott , "open list:QUALCOMM ATHEROS ATH10K WIRELESS DRIVER" , Intel Linux Wireless , "open list:ACPI" , Dmitry Monakhov , Nick Kossifidis , "open list:B43 WIRELESS DRIVER" , Doug Thompson , Gustavo Padovan , Sasha Levin , "Winkler, Tomas" , "sboyd@codeaurora.org" , Len Brown , "linaro-kernel@lists.linaro.org" , Hariprasad S , "arnd@arndb.de" , Mauro Carvalho Chehab , Vlastimil Babka , Arik Nemtsov , Marcel Holtmann , "James E.J. Bottomley" , Michal Hocko , Akinobu Mita , QCA ath9k Development , Michael Kerrisk , Tejun Heo , Mark Brown , Borislav Petkov , Steven Rostedt , Takashi Iwai , Florian Fainelli , Charles Keepax , Mel Gorman , "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" , "open list:EDAC-CORE" , Haggai Eran , Narsimhulu Musini , "Ivgi, Chaya Rachel" , "open list:CISCO SCSI HBA DRIVER" , Brian Silverman , "Luis R. Rodriguez" , "open list:CXGB4 ETHERNET DRIVER (CXGB4)" , "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" , Rafael Wysocki , Liam Girdwood , Sesidhar Baddela , Andy Lutomirski , "open list:BLUETOOTH DRIVERS" , "open list:AMD IOMMU (AMD-VI)" , "open list:QUALCOMM ATHEROS ATH9K WIRELESS DRIVER" , Thomas Gleixner , Johannes Weiner , Joe Perches , Eliad Peller , Andrew Morton , Alexander Duyck , Larry Finger 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 _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k