[alsa-lib] ASoC: hdmi-codec: use unsigned type to structure members with bit-field instead of signed type
diff mbox

Message ID 20161216092654.7773-1-o-takashi@sakamocchi.jp
State Accepted
Commit 9e4d59ada4d602e78eee9fb5f898ce61fdddb446
Headers show

Commit Message

Takashi Sakamoto Dec. 16, 2016, 9:26 a.m. UTC
This is a fix for Linux 4.10-rc1.

In C language specification, a bit-field is interpreted as a signed or
unsigned integer type consisting of the specified number of bits.

In GCC manual, the range of a signed bit field of N bits is from
-(2^N) / 2 to ((2^N) / 2) - 1
https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Bit-Fields

Therefore, when defined as 1 bit-field with signed type, variables can
represents -1 and 0.

The snd-soc-hdmi-codec module includes a structure which has signed type
members with bit-fields. Codes of this module assign 0 and 1 to the
members. This seems to result in implementation-dependent behaviours.

As of v4.10-rc1 merge window, outside of sound subsystem, this structure
is referred by below GPU modules.
 - tda998x
 - sti-drm
 - mediatek-drm-hdmi
 - msm

As long as I review their codes relevant to the structure, the structure
members are used just for condition statements and printk formats.
My proposal of change is a bit intrusive to the printk formats but this
may be acceptable.

Totally, it's reasonable to use unsigned type for the structure members.
This bug is detected by Sparse, static code analyzer with below warnings.

./include/sound/hdmi-codec.h:39:26: error: dubious one-bit signed bitfield
./include/sound/hdmi-codec.h:40:28: error: dubious one-bit signed bitfield
./include/sound/hdmi-codec.h:41:29: error: dubious one-bit signed bitfield
./include/sound/hdmi-codec.h:42:31: error: dubious one-bit signed bitfield

CC: Jyri Sarha <jsarha@ti.com>
CC: Jie Qiu <jie.qiu@mediatek.com>
CC: Arnaud Pouliquen <arnaud.pouliquen@st.com>
CC: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
CC: dri-devel@lists.freedesktop.org
CC: stable@vger.kernel.org
Fixes: 09184118a8ab ("ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders")
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/hdmi-codec.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Arnaud Pouliquen Dec. 16, 2016, 10:27 a.m. UTC | #1
Hi Takashi,

Reviewed and tested for sti machine.

Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Regards
Arnaud

On 12/16/2016 10:26 AM, Takashi Sakamoto wrote:
> This is a fix for Linux 4.10-rc1.
> 
> In C language specification, a bit-field is interpreted as a signed or
> unsigned integer type consisting of the specified number of bits.
> 
> In GCC manual, the range of a signed bit field of N bits is from
> -(2^N) / 2 to ((2^N) / 2) - 1
> https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Bit-Fields
> 
> Therefore, when defined as 1 bit-field with signed type, variables can
> represents -1 and 0.
> 
> The snd-soc-hdmi-codec module includes a structure which has signed type
> members with bit-fields. Codes of this module assign 0 and 1 to the
> members. This seems to result in implementation-dependent behaviours.
> 
> As of v4.10-rc1 merge window, outside of sound subsystem, this structure
> is referred by below GPU modules.
>  - tda998x
>  - sti-drm
>  - mediatek-drm-hdmi
>  - msm
> 
> As long as I review their codes relevant to the structure, the structure
> members are used just for condition statements and printk formats.
> My proposal of change is a bit intrusive to the printk formats but this
> may be acceptable.
> 
> Totally, it's reasonable to use unsigned type for the structure members.
> This bug is detected by Sparse, static code analyzer with below warnings.
> 
> ./include/sound/hdmi-codec.h:39:26: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:40:28: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:41:29: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:42:31: error: dubious one-bit signed bitfield
> 
> CC: Jyri Sarha <jsarha@ti.com>
> CC: Jie Qiu <jie.qiu@mediatek.com>
> CC: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> CC: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> CC: dri-devel@lists.freedesktop.org
> CC: stable@vger.kernel.org
> Fixes: 09184118a8ab ("ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/sound/hdmi-codec.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> index 530c57b..915c435 100644
> --- a/include/sound/hdmi-codec.h
> +++ b/include/sound/hdmi-codec.h
> @@ -36,10 +36,10 @@ struct hdmi_codec_daifmt {
>  		HDMI_AC97,
>  		HDMI_SPDIF,
>  	} fmt;
> -	int bit_clk_inv:1;
> -	int frame_clk_inv:1;
> -	int bit_clk_master:1;
> -	int frame_clk_master:1;
> +	unsigned int bit_clk_inv:1;
> +	unsigned int frame_clk_inv:1;
> +	unsigned int bit_clk_master:1;
> +	unsigned int frame_clk_master:1;
>  };
>  
>  /*
>
Takashi Sakamoto Dec. 16, 2016, 10:32 a.m. UTC | #2
Oops. This is not a patch for alsa-lib, but for Linux kernel.
I'm sorry to confuse you...

On Dec 16 2016 18:26, Takashi Sakamoto wrote:
> This is a fix for Linux 4.10-rc1.
>
> In C language specification, a bit-field is interpreted as a signed or
> unsigned integer type consisting of the specified number of bits.
>
> In GCC manual, the range of a signed bit field of N bits is from
> -(2^N) / 2 to ((2^N) / 2) - 1
> https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Bit-Fields
>
> Therefore, when defined as 1 bit-field with signed type, variables can
> represents -1 and 0.
>
> The snd-soc-hdmi-codec module includes a structure which has signed type
> members with bit-fields. Codes of this module assign 0 and 1 to the
> members. This seems to result in implementation-dependent behaviours.
>
> As of v4.10-rc1 merge window, outside of sound subsystem, this structure
> is referred by below GPU modules.
>  - tda998x
>  - sti-drm
>  - mediatek-drm-hdmi
>  - msm
>
> As long as I review their codes relevant to the structure, the structure
> members are used just for condition statements and printk formats.
> My proposal of change is a bit intrusive to the printk formats but this
> may be acceptable.
>
> Totally, it's reasonable to use unsigned type for the structure members.
> This bug is detected by Sparse, static code analyzer with below warnings.
>
> ./include/sound/hdmi-codec.h:39:26: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:40:28: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:41:29: error: dubious one-bit signed bitfield
> ./include/sound/hdmi-codec.h:42:31: error: dubious one-bit signed bitfield
>
> CC: Jyri Sarha <jsarha@ti.com>
> CC: Jie Qiu <jie.qiu@mediatek.com>
> CC: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> CC: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> CC: dri-devel@lists.freedesktop.org
> CC: stable@vger.kernel.org
> Fixes: 09184118a8ab ("ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders")
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/sound/hdmi-codec.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> index 530c57b..915c435 100644
> --- a/include/sound/hdmi-codec.h
> +++ b/include/sound/hdmi-codec.h
> @@ -36,10 +36,10 @@ struct hdmi_codec_daifmt {
>  		HDMI_AC97,
>  		HDMI_SPDIF,
>  	} fmt;
> -	int bit_clk_inv:1;
> -	int frame_clk_inv:1;
> -	int bit_clk_master:1;
> -	int frame_clk_master:1;
> +	unsigned int bit_clk_inv:1;
> +	unsigned int frame_clk_inv:1;
> +	unsigned int bit_clk_master:1;
> +	unsigned int frame_clk_master:1;
>  };
>
>  /*


Regards

Takashi Sakamoto
Mark Brown Dec. 16, 2016, 12:27 p.m. UTC | #3
On Fri, Dec 16, 2016 at 06:26:54PM +0900, Takashi Sakamoto wrote:

> This is a fix for Linux 4.10-rc1.

This...

> CC: stable@vger.kernel.org
> Fixes: 09184118a8ab ("ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders")

...and this don't add up (though it is something that should go to
stable, good catch).  Please also try to keep your subject lines much
shorter.

Patch
diff mbox

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 530c57b..915c435 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -36,10 +36,10 @@  struct hdmi_codec_daifmt {
 		HDMI_AC97,
 		HDMI_SPDIF,
 	} fmt;
-	int bit_clk_inv:1;
-	int frame_clk_inv:1;
-	int bit_clk_master:1;
-	int frame_clk_master:1;
+	unsigned int bit_clk_inv:1;
+	unsigned int frame_clk_inv:1;
+	unsigned int bit_clk_master:1;
+	unsigned int frame_clk_master:1;
 };
 
 /*