[PATCH V5 0/2] ASoC: Add core API to register DMI names to card

[PATCH V5 0/2] ASoC: Add core API to register DMI names to card

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

 Share more product information, for user space utils such as PA and UCM to
 distinguish different products.
  1. Add a core API to register DMI names to card.
  2. Apply the API to bytcr-rt5640 driver.
 
 changes on V5:
  1. Use independent space to store card long_name, to avoid irrelavant
  info sharing from card component
  2. Use letter ';' instead of ':' to separate strings in long name, in
  case name strings may also contain ':' and confusing user
  3. Fix error that vendor name and firmware name were not optional 

 changes on V4:
  1. Replace kmalloc() and snprintf() with ksaprintf() to simplify code
 
 changes on V3:
  1. Split the core API and the API call to two patches
  2. Replace misused strcat() with snprintf()
  3. Code and comment fix 

Lu, Han (2):
  ASoC: core: add API for registering DMI card names
  ASoC: bytcr-rt5640: register DMI names for card

 include/sound/soc.h                   |  3 ++
 sound/soc/intel/boards/bytcr_rt5640.c | 13 +++++++
 sound/soc/soc-core.c                  | 67 +++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)

-- 
2.5.0

[PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Add core API for registering DMI card names, so user space utils such
as PA and UCM can distinguish various products.
Previously on ASoC, the card short name, driver name and long name are
all the same as the machine driver name.
The patch adds more board information:
  card driver name --->  machine driver name
  card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
  card long name and
  card component   --->  short name;driver name;(DMI_SYS_VENDOR,
                         optional);(the firmware name, optional)

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 02b4a21..4e80444 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream);
 int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 	unsigned int dai_fmt);
 
+int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
+		const char *vendor, const char *firmware);
+
 /* Utility functions to get clock rates from various things */
 int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
 int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index d2e62b15..a06b9b9 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -48,6 +48,9 @@
 
 #define NAME_SIZE	32
 
+#define LONGNAME_SIZE	80	/* size of longname[] in struct snd_card */
+static char card_longname[LONGNAME_SIZE];
+
 #ifdef CONFIG_DEBUG_FS
 struct dentry *snd_soc_debugfs_root;
 EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
@@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 }
 EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
 
+/**
+ * snd_soc_set_card_names() - Register DMI names to card
+ * @card: The card to register DMI names
+ * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
+ * @vendor: DMI_SYS_VENDOR, optional
+ * @firmware: The firmware name, optional
+ *
+ * This function registers DMI names to card for the userspace to distinguish
+ * different boards/products:
+ *   card driver name --->  machine driver name
+ *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
+ *   card long name and
+ *   card component   --->  short name;driver name;(DMI_SYS_VENDOR, optional)
+ *                          ;(firmware name, optional)
+ *
+ * Returns 0 on success, otherwise a negative error code.
+ */
+int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
+		const char *vendor, const char *firmware)
+{
+	int ret = 0;
+	size_t name_size;
+
+	if (!board) {
+		dev_err(card->dev, "ASoC: the board/product name is empty!\n");
+		return -EINVAL;
+	}
+
+	/* card driver name */
+	card->driver_name = card->name;
+
+	/* card short name */
+	card->name = board;
+
+	/* card long name */
+	name_size = strlen(card->name) + strlen(card->driver_name) + 4;
+	if (vendor)
+		name_size += strlen(vendor);
+	if (firmware)
+		name_size += strlen(firmware);
+	if (name_size > LONGNAME_SIZE)
+		return -ENOMEM;
+
+	snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
+			card->name, card->driver_name);
+	if (vendor)
+		strlcat(card_longname, vendor, LONGNAME_SIZE);
+	strlcat(card_longname, ";", LONGNAME_SIZE);
+	if (firmware)
+		strlcat(card_longname, firmware, LONGNAME_SIZE);
+
+	card->long_name = card_longname;
+
+	/* card component */
+	if (sizeof(card->snd_card->components) < name_size
+			+ strlen(card->snd_card->components))
+		return -ENOMEM;
+
+	ret = snd_component_add(card->snd_card, card->long_name);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
+
 static int snd_soc_instantiate_card(struct snd_soc_card *card)
 {
 	struct snd_soc_codec *codec;
-- 
2.5.0

[PATCH V5 2/2] ASoC: bytcr-rt5640: register DMI names for card

[han.lu]

From: "Lu, Han" <han.lu@intel.com>

Register DMI names for products using bytcr-rt5640 driver, such as ASUS
T100TA and Dell Venue 8 Pro 5830, for user space utils to distinguish
different products.

For example, previously on T100TA:
  $ cat /proc/asound/cards
    0 [bytcrrt5640 ]: bytcr-rt5640 -  bytcr-rt5640
                      bytcr-rt5640
  $ amixer -c0 info
  Card hw:0 'bytcrrt5640'/'bytcr-rt5640'
    Mixer name    : ''
    Components    : ''
    Controls      : 256
    Simple ctrls  : 228

After apply the patch:
  $ cat /proc/asound/cards
    0 [T100TA      ]: bytcr-rt5640 -  T100TA
                      T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_
  sst_0f28.bin
  $ amixer -c0 info
  Card hw:0 'T100TA'/'T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_
  sst_0f28.bin'
    Mixer name    : ''
    Components    : 'T100TA;bytcr-rt5640;ASUSTek COMPUTER INC.;intel/fw_s
  st_0f28.bin'
    Controls      : 256
    Simple ctrls  : 228

Signed-off-by: Lu, Han <han.lu@intel.com>

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 032a2e7..2e083a7 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -152,6 +152,8 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = {
 	{}
 };
 
+static struct snd_soc_card byt_rt5640_card;
+
 static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
@@ -159,6 +161,17 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 	struct snd_soc_card *card = runtime->card;
 	const struct snd_soc_dapm_route *custom_map;
 	int num_routes;
+	const char *board, *vendor;
+	struct sst_acpi_mach *mach = byt_rt5640_card.dev->platform_data;
+
+	/* Add board name, vendor name and firmware name for the userspace */
+	board = dmi_get_system_info(DMI_PRODUCT_NAME);
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	ret = snd_soc_set_card_names(card, board, vendor, mach->fw_filename);
+	if (ret < 0) {
+		dev_err(card->dev, "unable to register card names\n");
+		return ret;
+	}
 
 	card->dapm.idle_bias_off = true;
 
-- 
2.5.0

Re: [PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[Takashi Sakamoto]

Hi,

On Apr 5 2016 13:26, han.lu@intel.com wrote:
> From: "Lu, Han" <han.lu@intel.com>
>
> Add core API for registering DMI card names, so user space utils such
> as PA and UCM can distinguish various products.
> Previously on ASoC, the card short name, driver name and long name are
> all the same as the machine driver name.
> The patch adds more board information:
>    card driver name --->  machine driver name
>    card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>    card long name and
>    card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>                           optional);(the firmware name, optional)
>
> Signed-off-by: Lu, Han <han.lu@intel.com>
>
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 02b4a21..4e80444 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream);
>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>   	unsigned int dai_fmt);
>
> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
> +		const char *vendor, const char *firmware);
> +
>   /* Utility functions to get clock rates from various things */
>   int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index d2e62b15..a06b9b9 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -48,6 +48,9 @@
>
>   #define NAME_SIZE	32
>
> +#define LONGNAME_SIZE	80	/* size of longname[] in struct snd_card */
> +static char card_longname[LONGNAME_SIZE];
> +

I'm not a developer for SoC sound interfaces or Skylake something, 
however I wonder the reason to add fiile local variable for this 
purpose. Could I request your intension about it?

Here, I assume a platform with some sound intefaces (I don't know we 
rarely have the situation or not). If several drivers are probed for 
these interfaces, the 'snd_soc_set_card_names()' might cause a race 
condition between the drivers against the local variable. However, 
there's no usage of lock primitive.
Additionally, when probing processing finishes for the second driver, 
the local variable changes its contents. Then, ALSA core returns the 
renewed string as the component of the first driver. This is not good.

If it's too rare that a platform with several interfaces, please inform 
it to me.

>   #ifdef CONFIG_DEBUG_FS
>   struct dentry *snd_soc_debugfs_root;
>   EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
> @@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>   }
>   EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
>
> +/**
> + * snd_soc_set_card_names() - Register DMI names to card
> + * @card: The card to register DMI names
> + * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
> + * @vendor: DMI_SYS_VENDOR, optional
> + * @firmware: The firmware name, optional
> + *
> + * This function registers DMI names to card for the userspace to distinguish
> + * different boards/products:
> + *   card driver name --->  machine driver name
> + *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
> + *   card long name and
> + *   card component   --->  short name;driver name;(DMI_SYS_VENDOR, optional)
> + *                          ;(firmware name, optional)
> + *
> + * Returns 0 on success, otherwise a negative error code.
> + */
> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
> +		const char *vendor, const char *firmware)
> +{
> +	int ret = 0;
> +	size_t name_size;
> +
> +	if (!board) {
> +		dev_err(card->dev, "ASoC: the board/product name is empty!\n");
> +		return -EINVAL;
> +	}
> +
> +	/* card driver name */
> +	card->driver_name = card->name;
> +
> +	/* card short name */
> +	card->name = board;
> +
> +	/* card long name */
> +	name_size = strlen(card->name) + strlen(card->driver_name) + 4;
> +	if (vendor)
> +		name_size += strlen(vendor);
> +	if (firmware)
> +		name_size += strlen(firmware);
> +	if (name_size > LONGNAME_SIZE)
> +		return -ENOMEM;
> +
> +	snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
> +			card->name, card->driver_name);
> +	if (vendor)
> +		strlcat(card_longname, vendor, LONGNAME_SIZE);
> +	strlcat(card_longname, ";", LONGNAME_SIZE);
> +	if (firmware)
> +		strlcat(card_longname, firmware, LONGNAME_SIZE);
> +
> +	card->long_name = card_longname;
> +
> +	/* card component */
> +	if (sizeof(card->snd_card->components) < name_size
> +			+ strlen(card->snd_card->components))
> +		return -ENOMEM;
> +
> +	ret = snd_component_add(card->snd_card, card->long_name);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
> +
>   static int snd_soc_instantiate_card(struct snd_soc_card *card)
>   {
>   	struct snd_soc_codec *codec;

Regards


Takashi Sakamoto

Re: [PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[Takashi Sakamoto]

On Apt 5 2016 14:38, Takashi Sakamoto wrote:
> Hi,
>
> On Apr 5 2016 13:26, han.lu@intel.com wrote:
>> From: "Lu, Han" <han.lu@intel.com>
>>
>> Add core API for registering DMI card names, so user space utils such
>> as PA and UCM can distinguish various products.
>> Previously on ASoC, the card short name, driver name and long name are
>> all the same as the machine driver name.
>> The patch adds more board information:
>>    card driver name --->  machine driver name
>>    card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>    card long name and
>>    card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>                           optional);(the firmware name, optional)
>>
>> Signed-off-by: Lu, Han <han.lu@intel.com>
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index 02b4a21..4e80444 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct
>> snd_soc_pcm_runtime *rtd, int stream);
>>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>       unsigned int dai_fmt);
>>
>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
>> +        const char *vendor, const char *firmware);
>> +
>>   /* Utility functions to get clock rates from various things */
>>   int snd_soc_calc_frame_size(int sample_size, int channels, int
>> tdm_slots);
>>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index d2e62b15..a06b9b9 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -48,6 +48,9 @@
>>
>>   #define NAME_SIZE    32
>>
>> +#define LONGNAME_SIZE    80    /* size of longname[] in struct
>> snd_card */
>> +static char card_longname[LONGNAME_SIZE];
>> +
>
> I'm not a developer for SoC sound interfaces or Skylake something,
> however I wonder the reason to add fiile local variable for this
> purpose. Could I request your intension about it?
>
> Here, I assume a platform with some sound intefaces (I don't know we
> rarely have the situation or not). If several drivers are probed for
> these interfaces, the 'snd_soc_set_card_names()' might cause a race
> condition between the drivers against the local variable. However,
> there's no usage of lock primitive.
> Additionally, when probing processing finishes for the second driver,
> the local variable changes its contents. Then, ALSA core returns the
> renewed string as the component of the first driver. This is not good.

Oops. The additional case comes from my misunderstanding. It never occurs...

So the point is the race condition.

> If it's too rare that a platform with several interfaces, please inform
> it to me.
>
>>   #ifdef CONFIG_DEBUG_FS
>>   struct dentry *snd_soc_debugfs_root;
>>   EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
>> @@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct
>> snd_soc_pcm_runtime *rtd,
>>   }
>>   EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
>>
>> +/**
>> + * snd_soc_set_card_names() - Register DMI names to card
>> + * @card: The card to register DMI names
>> + * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
>> + * @vendor: DMI_SYS_VENDOR, optional
>> + * @firmware: The firmware name, optional
>> + *
>> + * This function registers DMI names to card for the userspace to
>> distinguish
>> + * different boards/products:
>> + *   card driver name --->  machine driver name
>> + *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>> + *   card long name and
>> + *   card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>> optional)
>> + *                          ;(firmware name, optional)
>> + *
>> + * Returns 0 on success, otherwise a negative error code.
>> + */
>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char *board,
>> +        const char *vendor, const char *firmware)
>> +{
>> +    int ret = 0;
>> +    size_t name_size;
>> +
>> +    if (!board) {
>> +        dev_err(card->dev, "ASoC: the board/product name is empty!\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* card driver name */
>> +    card->driver_name = card->name;
>> +
>> +    /* card short name */
>> +    card->name = board;
>> +
>> +    /* card long name */
>> +    name_size = strlen(card->name) + strlen(card->driver_name) + 4;
>> +    if (vendor)
>> +        name_size += strlen(vendor);
>> +    if (firmware)
>> +        name_size += strlen(firmware);
>> +    if (name_size > LONGNAME_SIZE)
>> +        return -ENOMEM;
>> +
>> +    snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
>> +            card->name, card->driver_name);
>> +    if (vendor)
>> +        strlcat(card_longname, vendor, LONGNAME_SIZE);
>> +    strlcat(card_longname, ";", LONGNAME_SIZE);
>> +    if (firmware)
>> +        strlcat(card_longname, firmware, LONGNAME_SIZE);
>> +
>> +    card->long_name = card_longname;
>> +
>> +    /* card component */
>> +    if (sizeof(card->snd_card->components) < name_size
>> +            + strlen(card->snd_card->components))
>> +        return -ENOMEM;
>> +
>> +    ret = snd_component_add(card->snd_card, card->long_name);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
>> +
>>   static int snd_soc_instantiate_card(struct snd_soc_card *card)
>>   {
>>       struct snd_soc_codec *codec;
>
> Regards
>
>
> Takashi Sakamoto

Re: [PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[Han Lu]

On 04/05/2016 01:38 PM, Takashi Sakamoto wrote:
> Hi,
>
> On Apr 5 2016 13:26, han.lu@intel.com wrote:
>> From: "Lu, Han" <han.lu@intel.com>
>>
>> Add core API for registering DMI card names, so user space utils such
>> as PA and UCM can distinguish various products.
>> Previously on ASoC, the card short name, driver name and long name are
>> all the same as the machine driver name.
>> The patch adds more board information:
>>    card driver name --->  machine driver name
>>    card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>    card long name and
>>    card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>                           optional);(the firmware name, optional)
>>
>> Signed-off-by: Lu, Han <han.lu@intel.com>
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index 02b4a21..4e80444 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct 
>> snd_soc_pcm_runtime *rtd, int stream);
>>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>       unsigned int dai_fmt);
>>
>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char 
>> *board,
>> +        const char *vendor, const char *firmware);
>> +
>>   /* Utility functions to get clock rates from various things */
>>   int snd_soc_calc_frame_size(int sample_size, int channels, int 
>> tdm_slots);
>>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index d2e62b15..a06b9b9 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -48,6 +48,9 @@
>>
>>   #define NAME_SIZE    32
>>
>> +#define LONGNAME_SIZE    80    /* size of longname[] in struct 
>> snd_card */
>> +static char card_longname[LONGNAME_SIZE];
>> +
>
> I'm not a developer for SoC sound interfaces or Skylake something, 
> however I wonder the reason to add fiile local variable for this 
> purpose. Could I request your intension about it?
>
> Here, I assume a platform with some sound intefaces (I don't know we 
> rarely have the situation or not). If several drivers are probed for 
> these interfaces, the 'snd_soc_set_card_names()' might cause a race 
> condition between the drivers against the local variable. However, 
> there's no usage of lock primitive.
> Additionally, when probing processing finishes for the second driver, 
> the local variable changes its contents. Then, ALSA core returns the 
> renewed string as the component of the first driver. This is not good.
>
> If it's too rare that a platform with several interfaces, please 
> inform it to me.
Thanks for reviewing.
I allocated an array here just intending for drivers to store 
card->long_name, since I
assumed (by mistake) that one platform has only one interface.
I'll use dynamic allocate and cleanup instead.

BR,
Han
>
>>   #ifdef CONFIG_DEBUG_FS
>>   struct dentry *snd_soc_debugfs_root;
>>   EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
>> @@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct 
>> snd_soc_pcm_runtime *rtd,
>>   }
>>   EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
>>
>> +/**
>> + * snd_soc_set_card_names() - Register DMI names to card
>> + * @card: The card to register DMI names
>> + * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
>> + * @vendor: DMI_SYS_VENDOR, optional
>> + * @firmware: The firmware name, optional
>> + *
>> + * This function registers DMI names to card for the userspace to 
>> distinguish
>> + * different boards/products:
>> + *   card driver name --->  machine driver name
>> + *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>> + *   card long name and
>> + *   card component   --->  short name;driver name;(DMI_SYS_VENDOR, 
>> optional)
>> + *                          ;(firmware name, optional)
>> + *
>> + * Returns 0 on success, otherwise a negative error code.
>> + */
>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char 
>> *board,
>> +        const char *vendor, const char *firmware)
>> +{
>> +    int ret = 0;
>> +    size_t name_size;
>> +
>> +    if (!board) {
>> +        dev_err(card->dev, "ASoC: the board/product name is empty!\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* card driver name */
>> +    card->driver_name = card->name;
>> +
>> +    /* card short name */
>> +    card->name = board;
>> +
>> +    /* card long name */
>> +    name_size = strlen(card->name) + strlen(card->driver_name) + 4;
>> +    if (vendor)
>> +        name_size += strlen(vendor);
>> +    if (firmware)
>> +        name_size += strlen(firmware);
>> +    if (name_size > LONGNAME_SIZE)
>> +        return -ENOMEM;
>> +
>> +    snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
>> +            card->name, card->driver_name);
>> +    if (vendor)
>> +        strlcat(card_longname, vendor, LONGNAME_SIZE);
>> +    strlcat(card_longname, ";", LONGNAME_SIZE);
>> +    if (firmware)
>> +        strlcat(card_longname, firmware, LONGNAME_SIZE);
>> +
>> +    card->long_name = card_longname;
>> +
>> +    /* card component */
>> +    if (sizeof(card->snd_card->components) < name_size
>> +            + strlen(card->snd_card->components))
>> +        return -ENOMEM;
>> +
>> +    ret = snd_component_add(card->snd_card, card->long_name);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
>> +
>>   static int snd_soc_instantiate_card(struct snd_soc_card *card)
>>   {
>>       struct snd_soc_codec *codec;
>
> Regards
>
>
> Takashi Sakamoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[Takashi Sakamoto]

Hi,

On Apr 6 2016 13:23, Han Lu wrote:
>
>
> On 04/05/2016 01:38 PM, Takashi Sakamoto wrote:
>> Hi,
>>
>> On Apr 5 2016 13:26, han.lu@intel.com wrote:
>>> From: "Lu, Han" <han.lu@intel.com>
>>>
>>> Add core API for registering DMI card names, so user space utils such
>>> as PA and UCM can distinguish various products.
>>> Previously on ASoC, the card short name, driver name and long name are
>>> all the same as the machine driver name.
>>> The patch adds more board information:
>>>    card driver name --->  machine driver name
>>>    card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>>    card long name and
>>>    card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>>                           optional);(the firmware name, optional)
>>>
>>> Signed-off-by: Lu, Han <han.lu@intel.com>
>>>
>>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>>> index 02b4a21..4e80444 100644
>>> --- a/include/sound/soc.h
>>> +++ b/include/sound/soc.h
>>> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct
>>> snd_soc_pcm_runtime *rtd, int stream);
>>>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>>       unsigned int dai_fmt);
>>>
>>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char
>>> *board,
>>> +        const char *vendor, const char *firmware);
>>> +
>>>   /* Utility functions to get clock rates from various things */
>>>   int snd_soc_calc_frame_size(int sample_size, int channels, int
>>> tdm_slots);
>>>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
>>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>>> index d2e62b15..a06b9b9 100644
>>> --- a/sound/soc/soc-core.c
>>> +++ b/sound/soc/soc-core.c
>>> @@ -48,6 +48,9 @@
>>>
>>>   #define NAME_SIZE    32
>>>
>>> +#define LONGNAME_SIZE    80    /* size of longname[] in struct
>>> snd_card */
>>> +static char card_longname[LONGNAME_SIZE];
>>> +
>>
>> I'm not a developer for SoC sound interfaces or Skylake something,
>> however I wonder the reason to add fiile local variable for this
>> purpose. Could I request your intension about it?
>>
>> Here, I assume a platform with some sound intefaces (I don't know we
>> rarely have the situation or not). If several drivers are probed for
>> these interfaces, the 'snd_soc_set_card_names()' might cause a race
>> condition between the drivers against the local variable. However,
>> there's no usage of lock primitive.
>> Additionally, when probing processing finishes for the second driver,
>> the local variable changes its contents. Then, ALSA core returns the
>> renewed string as the component of the first driver. This is not good.
>>
>> If it's too rare that a platform with several interfaces, please
>> inform it to me.
> Thanks for reviewing.
> I allocated an array here just intending for drivers to store
> card->long_name, since I
> assumed (by mistake) that one platform has only one interface.
> I'll use dynamic allocate and cleanup instead.

For example, we can see Allwinner A31 has two interfaces for audio 
functionality. See clause '8.9. DIGITAL AUDIO INTERFACE' and '7.5.2. 
HDMI BLOCK DIAGRAM' in this user manual:

http://dl.linux-sunxi.org/A31/A3x_release_document/A31/IC/A31%20user%20manual%20V1.1%2020130630.pdf

The data for these interfaces is transferred by different DMA contexts, 
thus I can assume a different driver is developed for each interface. 
Depending on the driver implementations, your code might cause a bug for 
them.

(I'm not a developer for SoC interface, so have no detail about it, of 
cource.)


As another advice, when you add static variable to keep it in ELF .data 
section and the variable is just referred to by one function, it's 
better to constrain the scope of the variable by adding it inner the 
function. For example, in your case, this is better practice:

int snd_soc_set_card_names(struct snd_soc_card *card,
                            const char *board,
                            const char *vendor, const char *firmware)
{
     static char card_longname[LONGNAME_SIZE];
     ...

Please notice that callers are still under the race condition against 
the variable.


Regards

Takashi Sakamoto

> BR,
> Han
>>
>>>   #ifdef CONFIG_DEBUG_FS
>>>   struct dentry *snd_soc_debugfs_root;
>>>   EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
>>> @@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct
>>> snd_soc_pcm_runtime *rtd,
>>>   }
>>>   EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
>>>
>>> +/**
>>> + * snd_soc_set_card_names() - Register DMI names to card
>>> + * @card: The card to register DMI names
>>> + * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>> + * @vendor: DMI_SYS_VENDOR, optional
>>> + * @firmware: The firmware name, optional
>>> + *
>>> + * This function registers DMI names to card for the userspace to
>>> distinguish
>>> + * different boards/products:
>>> + *   card driver name --->  machine driver name
>>> + *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>> + *   card long name and
>>> + *   card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>> optional)
>>> + *                          ;(firmware name, optional)
>>> + *
>>> + * Returns 0 on success, otherwise a negative error code.
>>> + */
>>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char
>>> *board,
>>> +        const char *vendor, const char *firmware)
>>> +{
>>> +    int ret = 0;
>>> +    size_t name_size;
>>> +
>>> +    if (!board) {
>>> +        dev_err(card->dev, "ASoC: the board/product name is empty!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* card driver name */
>>> +    card->driver_name = card->name;
>>> +
>>> +    /* card short name */
>>> +    card->name = board;
>>> +
>>> +    /* card long name */
>>> +    name_size = strlen(card->name) + strlen(card->driver_name) + 4;
>>> +    if (vendor)
>>> +        name_size += strlen(vendor);
>>> +    if (firmware)
>>> +        name_size += strlen(firmware);
>>> +    if (name_size > LONGNAME_SIZE)
>>> +        return -ENOMEM;
>>> +
>>> +    snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
>>> +            card->name, card->driver_name);
>>> +    if (vendor)
>>> +        strlcat(card_longname, vendor, LONGNAME_SIZE);
>>> +    strlcat(card_longname, ";", LONGNAME_SIZE);
>>> +    if (firmware)
>>> +        strlcat(card_longname, firmware, LONGNAME_SIZE);
>>> +
>>> +    card->long_name = card_longname;
>>> +
>>> +    /* card component */
>>> +    if (sizeof(card->snd_card->components) < name_size
>>> +            + strlen(card->snd_card->components))
>>> +        return -ENOMEM;
>>> +
>>> +    ret = snd_component_add(card->snd_card, card->long_name);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
>>> +
>>>   static int snd_soc_instantiate_card(struct snd_soc_card *card)
>>>   {
>>>       struct snd_soc_codec *codec;
>>
>> Regards
>>
>>
>> Takashi Sakamoto

Re: [PATCH V5 1/2] ASoC: core: add API for registering DMI card names

[Han Lu]

Hi,

On 04/06/2016 01:47 PM, Takashi Sakamoto wrote:
> Hi,
>
> On Apr 6 2016 13:23, Han Lu wrote:
>>
>>
>> On 04/05/2016 01:38 PM, Takashi Sakamoto wrote:
>>> Hi,
>>>
>>> On Apr 5 2016 13:26, han.lu@intel.com wrote:
>>>> From: "Lu, Han" <han.lu@intel.com>
>>>>
>>>> Add core API for registering DMI card names, so user space utils such
>>>> as PA and UCM can distinguish various products.
>>>> Previously on ASoC, the card short name, driver name and long name are
>>>> all the same as the machine driver name.
>>>> The patch adds more board information:
>>>>    card driver name --->  machine driver name
>>>>    card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>>>    card long name and
>>>>    card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>>>                           optional);(the firmware name, optional)
>>>>
>>>> Signed-off-by: Lu, Han <han.lu@intel.com>
>>>>
>>>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>>>> index 02b4a21..4e80444 100644
>>>> --- a/include/sound/soc.h
>>>> +++ b/include/sound/soc.h
>>>> @@ -486,6 +486,9 @@ void snd_soc_runtime_deactivate(struct
>>>> snd_soc_pcm_runtime *rtd, int stream);
>>>>   int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
>>>>       unsigned int dai_fmt);
>>>>
>>>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char
>>>> *board,
>>>> +        const char *vendor, const char *firmware);
>>>> +
>>>>   /* Utility functions to get clock rates from various things */
>>>>   int snd_soc_calc_frame_size(int sample_size, int channels, int
>>>> tdm_slots);
>>>>   int snd_soc_params_to_frame_size(struct snd_pcm_hw_params *params);
>>>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>>>> index d2e62b15..a06b9b9 100644
>>>> --- a/sound/soc/soc-core.c
>>>> +++ b/sound/soc/soc-core.c
>>>> @@ -48,6 +48,9 @@
>>>>
>>>>   #define NAME_SIZE    32
>>>>
>>>> +#define LONGNAME_SIZE    80    /* size of longname[] in struct
>>>> snd_card */
>>>> +static char card_longname[LONGNAME_SIZE];
>>>> +
>>>
>>> I'm not a developer for SoC sound interfaces or Skylake something,
>>> however I wonder the reason to add fiile local variable for this
>>> purpose. Could I request your intension about it?
>>>
>>> Here, I assume a platform with some sound intefaces (I don't know we
>>> rarely have the situation or not). If several drivers are probed for
>>> these interfaces, the 'snd_soc_set_card_names()' might cause a race
>>> condition between the drivers against the local variable. However,
>>> there's no usage of lock primitive.
>>> Additionally, when probing processing finishes for the second driver,
>>> the local variable changes its contents. Then, ALSA core returns the
>>> renewed string as the component of the first driver. This is not good.
>>>
>>> If it's too rare that a platform with several interfaces, please
>>> inform it to me.
>> Thanks for reviewing.
>> I allocated an array here just intending for drivers to store
>> card->long_name, since I
>> assumed (by mistake) that one platform has only one interface.
>> I'll use dynamic allocate and cleanup instead.
>
> For example, we can see Allwinner A31 has two interfaces for audio 
> functionality. See clause '8.9. DIGITAL AUDIO INTERFACE' and '7.5.2. 
> HDMI BLOCK DIAGRAM' in this user manual:
>
> http://dl.linux-sunxi.org/A31/A3x_release_document/A31/IC/A31%20user%20manual%20V1.1%2020130630.pdf 
>
>
> The data for these interfaces is transferred by different DMA 
> contexts, thus I can assume a different driver is developed for each 
> interface. Depending on the driver implementations, your code might 
> cause a bug for them.
>
> (I'm not a developer for SoC interface, so have no detail about it, of 
> cource.)
>
>
> As another advice, when you add static variable to keep it in ELF 
> .data section and the variable is just referred to by one function, 
> it's better to constrain the scope of the variable by adding it inner 
> the function. For example, in your case, this is better practice:
>
> int snd_soc_set_card_names(struct snd_soc_card *card,
>                            const char *board,
>                            const char *vendor, const char *firmware)
> {
>     static char card_longname[LONGNAME_SIZE];
>     ...
>
> Please notice that callers are still under the race condition against 
> the variable.
Thanks for comment. I agree that it's not a good idea to use static 
array here anyway.
So I used kmalloc() and kfree() in patch V6.

BR,
Han
>
>
> Regards
>
> Takashi Sakamoto
>
>> BR,
>> Han
>>>
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>   struct dentry *snd_soc_debugfs_root;
>>>>   EXPORT_SYMBOL_GPL(snd_soc_debugfs_root);
>>>> @@ -1828,6 +1831,70 @@ int snd_soc_runtime_set_dai_fmt(struct
>>>> snd_soc_pcm_runtime *rtd,
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
>>>>
>>>> +/**
>>>> + * snd_soc_set_card_names() - Register DMI names to card
>>>> + * @card: The card to register DMI names
>>>> + * @board: DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>>> + * @vendor: DMI_SYS_VENDOR, optional
>>>> + * @firmware: The firmware name, optional
>>>> + *
>>>> + * This function registers DMI names to card for the userspace to
>>>> distinguish
>>>> + * different boards/products:
>>>> + *   card driver name --->  machine driver name
>>>> + *   card short name  --->  DMI_BOARD_NAME or DMI_PRODUCT_NAME
>>>> + *   card long name and
>>>> + *   card component   --->  short name;driver name;(DMI_SYS_VENDOR,
>>>> optional)
>>>> + *                          ;(firmware name, optional)
>>>> + *
>>>> + * Returns 0 on success, otherwise a negative error code.
>>>> + */
>>>> +int snd_soc_set_card_names(struct snd_soc_card *card, const char
>>>> *board,
>>>> +        const char *vendor, const char *firmware)
>>>> +{
>>>> +    int ret = 0;
>>>> +    size_t name_size;
>>>> +
>>>> +    if (!board) {
>>>> +        dev_err(card->dev, "ASoC: the board/product name is 
>>>> empty!\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    /* card driver name */
>>>> +    card->driver_name = card->name;
>>>> +
>>>> +    /* card short name */
>>>> +    card->name = board;
>>>> +
>>>> +    /* card long name */
>>>> +    name_size = strlen(card->name) + strlen(card->driver_name) + 4;
>>>> +    if (vendor)
>>>> +        name_size += strlen(vendor);
>>>> +    if (firmware)
>>>> +        name_size += strlen(firmware);
>>>> +    if (name_size > LONGNAME_SIZE)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    snprintf(card_longname, LONGNAME_SIZE, "%s;%s;",
>>>> +            card->name, card->driver_name);
>>>> +    if (vendor)
>>>> +        strlcat(card_longname, vendor, LONGNAME_SIZE);
>>>> +    strlcat(card_longname, ";", LONGNAME_SIZE);
>>>> +    if (firmware)
>>>> +        strlcat(card_longname, firmware, LONGNAME_SIZE);
>>>> +
>>>> +    card->long_name = card_longname;
>>>> +
>>>> +    /* card component */
>>>> +    if (sizeof(card->snd_card->components) < name_size
>>>> +            + strlen(card->snd_card->components))
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = snd_component_add(card->snd_card, card->long_name);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(snd_soc_set_card_names);
>>>> +
>>>>   static int snd_soc_instantiate_card(struct snd_soc_card *card)
>>>>   {
>>>>       struct snd_soc_codec *codec;
>>>
>>> Regards
>>>
>>>
>>> Takashi Sakamoto
>