[alsa-devel] Question about the right fix for a sparse warning
Hi Takashi,
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Thanks, Ranjani
On Wed, 29 Jan 2020 02:11:38 +0100, Sridharan, Ranjani wrote:
Hi Takashi,
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
Takashi
On 1/29/20 1:55 AM, Takashi Iwai wrote:
On Wed, 29 Jan 2020 02:11:38 +0100, Sridharan, Ranjani wrote:
Hi Takashi,
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
It's been merged in the SOF tree (topic/sof-dev) and it'll be in the next batch I send. It's not upstream just yet because we want to remove the warning to make the patch nice and shiny :-)
On Wed, 29 Jan 2020 15:39:50 +0100, Pierre-Louis Bossart wrote:
On 1/29/20 1:55 AM, Takashi Iwai wrote:
On Wed, 29 Jan 2020 02:11:38 +0100, Sridharan, Ranjani wrote:
Hi Takashi,
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
It's been merged in the SOF tree (topic/sof-dev) and it'll be in the next batch I send. It's not upstream just yet because we want to remove the warning to make the patch nice and shiny :-)
I still wonder why I couldn't get it on my tree. I guess the code around that hasn't changed, right?
Wait... Is it a test on 32bit arch? If so, it might be a new thing for y2038 support. Then we may fix the uapi definition instead.
Takashi
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
It's been merged in the SOF tree (topic/sof-dev) and it'll be in the next batch I send. It's not upstream just yet because we want to remove the warning to make the patch nice and shiny :-)
I still wonder why I couldn't get it on my tree. I guess the code around that hasn't changed, right?
Wait... Is it a test on 32bit arch? If so, it might be a new thing for y2038 support. Then we may fix the uapi definition instead.
It's actually in your tree already, my mistake, see:
ee1e79b72e3cf ("ASoC: SOF: partition audio-related parts from SOF core")
It's with plain-vanilla x86_64, I can share the config.
On Wed, 29 Jan 2020 18:01:00 +0100, Pierre-Louis Bossart wrote:
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
It's been merged in the SOF tree (topic/sof-dev) and it'll be in the next batch I send. It's not upstream just yet because we want to remove the warning to make the patch nice and shiny :-)
I still wonder why I couldn't get it on my tree. I guess the code around that hasn't changed, right?
Wait... Is it a test on 32bit arch? If so, it might be a new thing for y2038 support. Then we may fix the uapi definition instead.
It's actually in your tree already, my mistake, see:
ee1e79b72e3cf ("ASoC: SOF: partition audio-related parts from SOF core")
It's with plain-vanilla x86_64, I can share the config.
Sorry, my bad, it seems that my sparse program was too old. After upgrading to the latest version, I could see the warning, too.
Takashi
On Wed, 29 Jan 2020 18:07:20 +0100, Takashi Iwai wrote:
On Wed, 29 Jan 2020 18:01:00 +0100, Pierre-Louis Bossart wrote:
We're seeing the following sparse warning in the SOF code:
sound/soc/sof/sof-audio.c:86:31: warning: incorrect type in assignment (different base types) sound/soc/sof/sof-audio.c:86:31: expected restricted snd_pcm_state_t [usertype] state sound/soc/sof/sof-audio.c:86:31: got signed int [signed] [usertype] [explicitly-signed] state
The line under scrutiny where we assign "state" is as follows:
state = substream->runtime->status->state;
and it is defined as snd_pcm_state_t state;
There are other places (ex pcm_oss.c) where a similar assignment has been used as well.
What fixes the issue is a forced cast to snd_pcm_state_t as below before assigning: state = (__force snd_pcm_state_t)substream->runtime->status->state;
Do you think this is acceptable? If not, could you please suggest an alternative?
Hm, I don't see the warning in my code. Did you merge all upstream stuff and still get it?
It's been merged in the SOF tree (topic/sof-dev) and it'll be in the next batch I send. It's not upstream just yet because we want to remove the warning to make the patch nice and shiny :-)
I still wonder why I couldn't get it on my tree. I guess the code around that hasn't changed, right?
Wait... Is it a test on 32bit arch? If so, it might be a new thing for y2038 support. Then we may fix the uapi definition instead.
It's actually in your tree already, my mistake, see:
ee1e79b72e3cf ("ASoC: SOF: partition audio-related parts from SOF core")
It's with plain-vanilla x86_64, I can share the config.
Sorry, my bad, it seems that my sparse program was too old. After upgrading to the latest version, I could see the warning, too.
... and the cause was what I suspected: it's a 64bit compat type that defines the fields explicitly with __s32. I overlooked that it's always used for __KERNEL__. The tentative fix is below.
thanks,
Takashi
--- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -564,13 +564,13 @@ typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; #endif
struct __snd_pcm_mmap_status64 { - __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */ + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ __u32 pad1; /* Needed for 64 bit alignment */ __pad_before_uframe __pad1; snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ __pad_after_uframe __pad2; struct __snd_timespec64 tstamp; /* Timestamp */ - __s32 suspended_state; /* RO: suspended stream state */ + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ __u32 pad3; /* Needed for 64 bit alignment */ struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */ };
Sorry, my bad, it seems that my sparse program was too old. After upgrading to the latest version, I could see the warning, too.
... and the cause was what I suspected: it's a 64bit compat type that defines the fields explicitly with __s32. I overlooked that it's always used for __KERNEL__. The tentative fix is below.
thanks,
Takashi
--- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -564,13 +564,13 @@ typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; #endif
struct __snd_pcm_mmap_status64 {
__s32 state; /* RO: state -SNDRV_PCM_STATE_XXXX */
snd_pcm_state_t state; /* RO: state -SNDRV_PCM_STATE_XXXX */ __u32 pad1; /* Needed for 64 bit alignment */ __pad_before_uframe __pad1; snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ __pad_after_uframe __pad2; struct __snd_timespec64 tstamp; /* Timestamp */
__s32 suspended_state; /* RO: suspended stream state */
snd_pcm_state_t suspended_state; /* RO: suspended streamstate */ __u32 pad3; /* Needed for 64 bit alignment */ struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */ };
Thanks, Takashi. I will prepare a patch with the fix.
Thanks, Ranjani
On 1/29/20 11:16 AM, Sridharan, Ranjani wrote:
Sorry, my bad, it seems that my sparse program was too old. After upgrading to the latest version, I could see the warning, too.
... and the cause was what I suspected: it's a 64bit compat type that defines the fields explicitly with __s32. I overlooked that it's always used for __KERNEL__. The tentative fix is below.
Nice, thanks Takashi, Ranjani and I were lost on this one.
thanks,
Takashi
--- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -564,13 +564,13 @@ typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; #endif
struct __snd_pcm_mmap_status64 {
__s32 state; /* RO: state -SNDRV_PCM_STATE_XXXX */
snd_pcm_state_t state; /* RO: state -SNDRV_PCM_STATE_XXXX */ __u32 pad1; /* Needed for 64 bit alignment */ __pad_before_uframe __pad1; snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ __pad_after_uframe __pad2; struct __snd_timespec64 tstamp; /* Timestamp */
__s32 suspended_state; /* RO: suspended stream state */
snd_pcm_state_t suspended_state; /* RO: suspended streamstate */ __u32 pad3; /* Needed for 64 bit alignment */ struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */ };
Thanks, Takashi. I will prepare a patch with the fix.
Thanks, Ranjani _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (3)
-
Pierre-Louis Bossart -
Sridharan, Ranjani -
Takashi Iwai