Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API

[Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>]

Hi,

On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 628d46a0da92..c458172b32d5 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image {
>   *                                                                           *
>   *****************************************************************************/
>  
> -#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 17)
> +#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 18)
>  
>  typedef unsigned long snd_pcm_uframes_t;
>  typedef signed long snd_pcm_sframes_t;
> @@ -330,12 +330,6 @@ enum {
>  #endif
>  };
>  
> -union snd_pcm_sync_id {
> -	unsigned char id[16];
> -	unsigned short id16[8];
> -	unsigned int id32[4];
> -};

It can bring FTBFS for any userspace application which uses the
structure. If getting rid of such public structure, we should have the
term to deprecate it for a while.

> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 6f73b3c2c205..57ed4ffc891a 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -5,6 +5,7 @@
>   *                   Abramo Bagnara <abramo@xxxxxxxxxxxxxxxx>
>   */
>  
> +#include <asm/unaligned.h>
>  #include <linux/slab.h>
>  #include <linux/sched/signal.h>
>  #include <linux/time.h>
> @@ -525,10 +526,8 @@ void snd_pcm_set_sync(struct snd_pcm_substream *substream)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	
> -	runtime->sync.id32[0] = substream->pcm->card->number;
> -	runtime->sync.id32[1] = -1;
> -	runtime->sync.id32[2] = -1;
> -	runtime->sync.id32[3] = -1;
> +	*(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
> +	memset(runtime->sync + 4, 0xff, sizeof(runtime->sync) - 4);
>  }
>  EXPORT_SYMBOL(snd_pcm_set_sync);
>  
> @@ -1810,6 +1809,24 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
>  	return 0;
>  }
>  
> +/**
> + * is sync id (clock id) empty?
> + */
> +static inline bool pcm_sync_empty(const unsigned char *sync)
> +{
> +	return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
> +}

This kind of usage of C inline qualifier is not useless, since the modern
compilers can optimize it into the local code.

> +static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
> +				     void *arg)
> +{
> +	struct snd_pcm_hw_params *params = arg;
> +
> +	if (pcm_sync_empty(params->sync))
> +		memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
> +	return 0;
> +}

Furthermore, it does not work as expected in the case that any of driver sets
zeros to char[16] intentionally for its own purpose at PCM .open callback.

I think it is a kind of 'Separation of mechanism and policy' argument,
and a kind of designs which should be avoided.

>  /**
>   * snd_pcm_lib_ioctl - a generic PCM ioctl callback
>   * @substream: the pcm substream instance
> @@ -1831,6 +1848,8 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  		return snd_pcm_lib_ioctl_channel_info(substream, arg);
>  	case SNDRV_PCM_IOCTL1_FIFO_SIZE:
>  		return snd_pcm_lib_ioctl_fifo_size(substream, arg);
> +	case SNDRV_PCM_IOCTL1_SYNC_ID:
> +		return snd_pcm_lib_ioctl_sync_id(substream, arg);
>  	}
>  	return -ENXIO;
>  }
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 0b76e76823d2..63fcb08ee93d 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -533,6 +533,12 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
>  					  SNDRV_PCM_INFO_MMAP_VALID);
>  	}
>  
> +	err = snd_pcm_ops_ioctl(substream,
> +				SNDRV_PCM_IOCTL1_SYNC_ID,
> +				params);
> +	if (err < 0)
> +		return err;
> +
>  	return 0;
>  }
>  
> diff --git a/sound/pci/emu10k1/p16v.c b/sound/pci/emu10k1/p16v.c
> index e7f097cae574..773725dbdfbd 100644
> --- a/sound/pci/emu10k1/p16v.c
> +++ b/sound/pci/emu10k1/p16v.c
> @@ -174,10 +174,9 @@ static int snd_p16v_pcm_open_playback_channel(struct snd_pcm_substream *substrea
>  	if (err < 0)
>                  return err;
>  
> -	runtime->sync.id32[0] = substream->pcm->card->number;
> -	runtime->sync.id32[1] = 'P';
> -	runtime->sync.id32[2] = 16;
> -	runtime->sync.id32[3] = 'V';
> +	*(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
> +	memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
> +	strncpy(runtime->sync + 4, "P16V", 4);

I'm sorry if I overlooked your reply to my point, however the above is
not equivalent.


Regards

Takashi Sakamoto