On 2020-01-31 17:39, Pierre-Louis Bossart wrote:
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?
All addressed in v5, thanks for review!