From mboxrd@z Thu Jan 1 00:00:00 1970 From: Charles Keepax Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes Date: Tue, 15 Sep 2015 10:13:19 +0100 Message-ID: <20150915091319.GH11200@ck-lbox> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "ath10k" Errors-To: ath10k-bounces+gldad-ath10k=m.gmane.org@lists.infradead.org To: Viresh Kumar Cc: "open list:NETWORKING DRIVERS (WIRELESS)" , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Avri Altman , Stanislaw Gruszka , Jiri Slaby , "open list:DOCUMENTATION" , Peter Zijlstra , Catalin Marinas , Sebastian Andrzej Siewior , Will Deacon , Jaroslav Kysela , "open list:MEMORY MANAGEMENT" , Kalle Valo , Emmanuel Grumbach , Luciano Coelho , Wang Long , Richard Fitzgerald , Ingo Molnar , open list List-Id: alsa-devel@alsa-project.org On Tue, Sep 15, 2015 at 02:04:59PM +0530, Viresh Kumar wrote: > Long back 'bool' type used to be a typecast to 'int', but that changed > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes > just a byte. Anyway, the bool type is implementation defined, and better > we don't assume its size to be 4 bytes or 1. > > 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. > > The debugfs code had this bug since the first time it got introduced, > but was never got caught, strange. Maybe the bool variables (monitored > by debugfs) were followed by an 'int' or something bigger and the pad > bytes made sure, we never see this issue. > > But the OPP (Operating performance points) library have three booleans > allocated to contiguous bytes and this bug got hit quite soon (The > debugfs support for OPP is yet to be merged). It showed up as corruption > of the debugfs boolean symbols, where Y were becoming N and vice versa. > > Fix it properly by changing the last argument of debugfs_create_bool(), > to type 'bool *' instead of 'u32 *', so that it doesn't depend on sizeof > bool at all. > > 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. > > Acked-by: Mark Brown > Signed-off-by: Viresh Kumar > --- For the minor wm_adsp change: Acked-by: Charles Keepax Thanks, Charles From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Date: Tue, 15 Sep 2015 10:13:19 +0100 From: Charles Keepax Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes Message-ID: <20150915091319.GH11200@ck-lbox> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> 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 Cc: "open list:NETWORKING DRIVERS (WIRELESS)" , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Avri Altman , Stanislaw Gruszka , Jiri Slaby , "open list:DOCUMENTATION" , Peter Zijlstra , Catalin Marinas , Sebastian Andrzej Siewior , Will Deacon , Jaroslav Kysela , "open list:MEMORY MANAGEMENT" , Kalle Valo , Emmanuel Grumbach , Luciano Coelho , Wang Long , Richard Fitzgerald , Ingo Molnar , open list , Johan Hedberg , Davidlohr Bueso , Johannes Berg , 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 , Tomas Winkler , 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 , Florian Fainelli , Takashi Iwai , Mel Gorman , "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" , "open list:EDAC-CORE" , Haggai Eran , Narsimhulu Musini , Chaya Rachel Ivgi , "open list:CISCO SCSI HBA DRIVER" , Brian Silverman , "Luis R. Rodriguez" , gregkh@linuxfoundation.org, "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 , "open list:CXGB4 ETHERNET DRIVER (CXGB4)" , Larry Finger On Tue, Sep 15, 2015 at 02:04:59PM +0530, Viresh Kumar wrote: > Long back 'bool' type used to be a typecast to 'int', but that changed > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes > just a byte. Anyway, the bool type is implementation defined, and better > we don't assume its size to be 4 bytes or 1. > > 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. > > The debugfs code had this bug since the first time it got introduced, > but was never got caught, strange. Maybe the bool variables (monitored > by debugfs) were followed by an 'int' or something bigger and the pad > bytes made sure, we never see this issue. > > But the OPP (Operating performance points) library have three booleans > allocated to contiguous bytes and this bug got hit quite soon (The > debugfs support for OPP is yet to be merged). It showed up as corruption > of the debugfs boolean symbols, where Y were becoming N and vice versa. > > Fix it properly by changing the last argument of debugfs_create_bool(), > to type 'bool *' instead of 'u32 *', so that it doesn't depend on sizeof > bool at all. > > 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. > > Acked-by: Mark Brown > Signed-off-by: Viresh Kumar > --- For the minor wm_adsp change: Acked-by: Charles Keepax Thanks, Charles _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k From mboxrd@z Thu Jan 1 00:00:00 1970 From: Charles Keepax Date: Tue, 15 Sep 2015 09:56:59 -0000 Subject: [ath9k-devel] [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes 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> Message-ID: <20150915091319.GH11200@ck-lbox> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ath9k-devel@lists.ath9k.org On Tue, Sep 15, 2015 at 02:04:59PM +0530, Viresh Kumar wrote: > Long back 'bool' type used to be a typecast to 'int', but that changed > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes > just a byte. Anyway, the bool type is implementation defined, and better > we don't assume its size to be 4 bytes or 1. > > 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. > > The debugfs code had this bug since the first time it got introduced, > but was never got caught, strange. Maybe the bool variables (monitored > by debugfs) were followed by an 'int' or something bigger and the pad > bytes made sure, we never see this issue. > > But the OPP (Operating performance points) library have three booleans > allocated to contiguous bytes and this bug got hit quite soon (The > debugfs support for OPP is yet to be merged). It showed up as corruption > of the debugfs boolean symbols, where Y were becoming N and vice versa. > > Fix it properly by changing the last argument of debugfs_create_bool(), > to type 'bool *' instead of 'u32 *', so that it doesn't depend on sizeof > bool at all. > > 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. > > Acked-by: Mark Brown > Signed-off-by: Viresh Kumar > --- For the minor wm_adsp change: Acked-by: Charles Keepax Thanks, Charles From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 15 Sep 2015 10:13:19 +0100 From: Charles Keepax To: Viresh Kumar CC: , , Rafael Wysocki , , , 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" , Avri Altman , "open list:B43 WIRELESS DRIVER" , Borislav Petkov , Brian Silverman , Catalin Marinas , Chaya Rachel Ivgi , Davidlohr Bueso , Dmitry Monakhov , Doug Thompson , Eliad Peller , Emmanuel Grumbach , 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 Berg , 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)" , Luciano Coelho , "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 , Tomas Winkler , Vlastimil Babka , Wang Long , Will Deacon Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes Message-ID: <20150915091319.GH11200@ck-lbox> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" In-Reply-To: <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> List-ID: On Tue, Sep 15, 2015 at 02:04:59PM +0530, Viresh Kumar wrote: > Long back 'bool' type used to be a typecast to 'int', but that changed > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes > just a byte. Anyway, the bool type is implementation defined, and better > we don't assume its size to be 4 bytes or 1. > > 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. > > The debugfs code had this bug since the first time it got introduced, > but was never got caught, strange. Maybe the bool variables (monitored > by debugfs) were followed by an 'int' or something bigger and the pad > bytes made sure, we never see this issue. > > But the OPP (Operating performance points) library have three booleans > allocated to contiguous bytes and this bug got hit quite soon (The > debugfs support for OPP is yet to be merged). It showed up as corruption > of the debugfs boolean symbols, where Y were becoming N and vice versa. > > Fix it properly by changing the last argument of debugfs_create_bool(), > to type 'bool *' instead of 'u32 *', so that it doesn't depend on sizeof > bool at all. > > 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. > > Acked-by: Mark Brown > Signed-off-by: Viresh Kumar > --- For the minor wm_adsp change: Acked-by: Charles Keepax Thanks, Charles From mboxrd@z Thu Jan 1 00:00:00 1970 From: Charles Keepax Subject: Re: [PATCH V3 2/2] debugfs: don't assume sizeof(bool) to be 4 bytes Date: Tue, 15 Sep 2015 10:13:19 +0100 Message-ID: <20150915091319.GH11200@ck-lbox> References: <9b705747a138c96c26faee5218f7b47403195b28.1442305897.git.viresh.kumar@linaro.org> <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "open list:NETWORKING DRIVERS \(WIRELESS\)" , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Avri Altman , Stanislaw Gruszka , Jiri Slaby , "open list:DOCUMENTATION" , Peter Zijlstra , Catalin Marinas , Sebastian Andrzej Siewior , Will Deacon , Jaroslav Kysela , "open list:MEMORY MANAGEMENT" , Kalle Valo , Emmanuel Grumbach , Luciano Coelho , Wang Long , Richard Fitzgerald , Ingo Molnar , open list Return-path: Content-Disposition: inline In-Reply-To: <27d37898b4be6b9b9f31b90135f8206ca079a868.1442305897.git.viresh.kumar@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "ath10k" Errors-To: ath10k-bounces+gldad-ath10k=m.gmane.org@lists.infradead.org List-Id: netdev.vger.kernel.org On Tue, Sep 15, 2015 at 02:04:59PM +0530, Viresh Kumar wrote: > Long back 'bool' type used to be a typecast to 'int', but that changed > in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes > just a byte. Anyway, the bool type is implementation defined, and better > we don't assume its size to be 4 bytes or 1. > > 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. > > The debugfs code had this bug since the first time it got introduced, > but was never got caught, strange. Maybe the bool variables (monitored > by debugfs) were followed by an 'int' or something bigger and the pad > bytes made sure, we never see this issue. > > But the OPP (Operating performance points) library have three booleans > allocated to contiguous bytes and this bug got hit quite soon (The > debugfs support for OPP is yet to be merged). It showed up as corruption > of the debugfs boolean symbols, where Y were becoming N and vice versa. > > Fix it properly by changing the last argument of debugfs_create_bool(), > to type 'bool *' instead of 'u32 *', so that it doesn't depend on sizeof > bool at all. > > 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. > > Acked-by: Mark Brown > Signed-off-by: Viresh Kumar > --- For the minor wm_adsp change: Acked-by: Charles Keepax Thanks, Charles