Re: [PATCH RFC 6/9] ALSA: core: pass audio tstamp config from userspace

[Takashi Iwai <tiwai@suse.de>]

At Wed, 10 Dec 2014 11:35:42 -0600,
Pierre-Louis Bossart wrote:
> 
> On 12/10/14, 11:28 AM, Takashi Iwai wrote:
> > At Mon,  8 Dec 2014 16:23:43 -0600,
> > Pierre-Louis Bossart wrote:
> >>
> >> Let userspace select audio timestamp config, ignore and zero all
> >> other fields
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> ---
> >>   sound/core/pcm_native.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 37a7137..7dcd6bd 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
> >>   	struct snd_pcm_runtime *runtime = substream->runtime;
> >>
> >>   	snd_pcm_stream_lock_irq(substream);
> >> +	memset(status, 0, sizeof(*status));
> >
> > This memset() can be outside the lock.
> >
> >>   	status->state = runtime->status->state;
> >>   	status->suspended_state = runtime->status->suspended_state;
> >>   	if (status->state == SNDRV_PCM_STATE_OPEN)
> >> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
> >>   	struct snd_pcm_status status;
> >>   	int res;
> >>   	
> >> -	memset(&status, 0, sizeof(status));
> >> +	if (copy_from_user(&status, _status, sizeof(status)))
> >> +		return -EFAULT;
> >>   	res = snd_pcm_status(substream, &status);
> >
> > You're clearing the data at the beginning of snd_pcm_status(), so it
> > doesn't make sense to do copy_from_user().
> >
> > In anyway, I prefer passing the extra argument for the parameter user
> > may set instead of initializing the whole struct beforehand.
> > snd_pcm_status() is called only internally, so it's no big problem to
> > change the arguments.
> 
> Did you mean something like this, only read the relevant field from the 
> larger structure?
> 
> 	if (copy_from_user(&audio_tstamp_data, _status.audio_tstamp_data, 
> sizeof(int)))
> 		return -EFAULT;
>   	res = snd_pcm_status(substream, &status, audio_tstamp_data);

Yes (although get_user() would be more convenient).


Takashi