[alsa-devel] Question about the right fix for a sparse warning

[Takashi Iwai]

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 */
 };