[alsa-devel] [PATCH v4 06/11] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

[Pierre-Louis Bossart]

Maybe be a paranoid check but the types used in this patch seem to need 
additional work:

> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 9a8bf1eb7d69..9a24d57f0cf2 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -496,6 +496,7 @@ struct hdac_stream {
>   	bool locked:1;
>   	bool stripe:1;			/* apply stripe control */
>   
> +	unsigned long curr_pos;

'long' is an error-prone definition...

>   	/* timestamp */
>   	unsigned long start_wallclk;	/* start + minimum wallclk */
>   	unsigned long period_wallclk;	/* wallclk for period */
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index c0ab9bb2a797..c8920a60e346 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
>   	return ret;
>   }
>   
> +static void hda_dsp_set_bytes_transferred(struct hdac_stream *hstream,
> +					  u64 buffer_size)
> +{
> +	unsigned int prev_pos;
> +	int pos, num_bytes;
> +
> +	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);

... here you use it as a u64 value so I guess the intent was to use more 
than 32 bits?

But then the u64 for the 'buffer size' argument is also not consistent 
with the definition of div_u64_rem, it's got to be u32, or you wanted to 
use div64_64_rem?

static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)

prev_pos should also be declared as u32 to avoid any ambiguity.

> +	pos = snd_hdac_stream_get_pos_posbuf(hstream);
> +
> +	if (pos < prev_pos)
> +		num_bytes = (buffer_size - prev_pos) +  pos;
> +	else
> +		num_bytes = pos - prev_pos;
> +
> +	hstream->curr_pos += num_bytes;

... and here it's a never ending-increment that is likely to hit a 
32-bit ceiling.

I remember we made a mistake some time back on compressed stuff and 
updated the counters to rely on 64 bits. Unless this is a hardware 
counter (and then we should use u32 and deal with overflow), we should 
use u64, no?