[PATCH 0/6] staging: most: sound: change sound card layout

[PATCH 0/6] staging: most: sound: change sound card layout

[Christian Gromm]

Currently, for every synchronous channel allocated on MOST an extra sound
card is being created and registered with ALSA. From a logical point of
view this fails to reflect the actual condition, as all channels originate
in the same physical device. This patch series introduces a way to map this
layout and creates only one sound card per registered MOST device that has
multiple PCM devices to access the individual MOST channels. 

Christian Gromm (6):
  staging: most: sound: create one sound card w/ multiple PCM devices
    per MOST device
  staging: most: sound: correct label name
  staging: most: sound: rename variable
  staging: most: sound: use static name for ALSA card
  staging: most: sound: remove channel number from ALSA card's long name
  staging: most: Documentation: add information to driver_usage file

 .../staging/most/Documentation/driver_usage.txt    |  16 ++-
 drivers/staging/most/sound/sound.c                 | 142 ++++++++++++++-------
 2 files changed, 107 insertions(+), 51 deletions(-)

-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Christian Gromm]

This patch avoids that a sound card is created and registered with ALSA
every time a channel is being linked. Instead the channels are hooked on
the same card, which is registered not until the final link has been added
to the component.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/sound/sound.c | 127 +++++++++++++++++++++++++------------
 1 file changed, 87 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 89b02fc..41bcd2c 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/printk.h>
 #include <linux/kernel.h>
+#include <linux/slab.h>
 #include <linux/init.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -20,7 +21,6 @@
 
 #define DRIVER_NAME "sound"
 
-static struct list_head dev_list;
 static struct core_component comp;
 
 /**
@@ -56,6 +56,17 @@ struct channel {
 	void (*copy_fn)(void *alsa, void *most, unsigned int bytes);
 };
 
+struct sound_adapter {
+	struct list_head dev_list;
+	struct most_interface *iface;
+	struct snd_card *card;
+	struct list_head list;
+	bool registered;
+	int pcm_dev_idx;
+};
+
+static struct list_head adpt_list;
+
 #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
 		       SNDRV_PCM_INFO_MMAP_VALID | \
 		       SNDRV_PCM_INFO_BATCH | \
@@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
 static struct channel *get_channel(struct most_interface *iface,
 				   int channel_id)
 {
+	struct sound_adapter *adpt = iface->priv;
 	struct channel *channel, *tmp;
 
-	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
+	list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
 		if ((channel->iface == iface) && (channel->id == channel_id))
 			return channel;
 	}
@@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
 };
 
 static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
-			  char **sample_res)
+			  char **sample_res, u8 *create)
 {
 	char *num;
 	int ret;
@@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
 	*sample_res = strsep(&buf, ".\n");
 	if (!*sample_res)
 		goto err;
+
+	if (buf && !strcmp(buf, "create"))
+		*create = 1;
 	return 0;
 
 err:
@@ -536,6 +551,19 @@ static int audio_set_hw_params(struct snd_pcm_hardware *pcm_hw,
 	return 0;
 }
 
+static void release_adapter(struct sound_adapter *adpt)
+{
+	struct channel *channel, *tmp;
+
+	list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
+		list_del(&channel->list);
+		kfree(channel);
+	}
+	snd_card_free(adpt->card);
+	list_del(&adpt->list);
+	kfree(adpt);
+}
+
 /**
  * audio_probe_channel - probe function of the driver module
  * @iface: pointer to interface instance
@@ -553,7 +581,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 			       char *arg_list)
 {
 	struct channel *channel;
-	struct snd_card *card;
+	struct sound_adapter *adpt;
 	struct snd_pcm *pcm;
 	int playback_count = 0;
 	int capture_count = 0;
@@ -561,6 +589,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	int direction;
 	char *card_name;
 	u16 ch_num;
+	u8 create = 0;
 	char *sample_res;
 
 	if (!iface)
@@ -571,6 +600,40 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 		return -EINVAL;
 	}
 
+	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
+			     &create);
+	if (ret < 0)
+		return ret;
+
+	list_for_each_entry(adpt, &adpt_list, list) {
+		if (adpt->iface == iface && adpt->registered)
+			return -ENOSPC;
+		if (!adpt->registered) {
+			adpt->pcm_dev_idx++;
+			goto skip_adpt_alloc;
+		}
+	}
+	adpt = kzalloc(sizeof(*adpt), GFP_KERNEL);
+	if (!adpt)
+		return -ENOMEM;
+
+	adpt->iface = iface;
+	adpt->registered = false;
+	adpt->pcm_dev_idx = 0;
+	INIT_LIST_HEAD(&adpt->dev_list);
+	iface->priv = adpt;
+	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
+			   sizeof(*channel), &adpt->card);
+	if (ret < 0)
+		return ret;
+	snprintf(adpt->card->driver, sizeof(adpt->card->driver),
+		 "%s", DRIVER_NAME);
+	snprintf(adpt->card->shortname, sizeof(adpt->card->shortname),
+		 "Microchip MOST:%d", adpt->card->number);
+	snprintf(adpt->card->longname, sizeof(adpt->card->longname),
+		 "%s at %s, ch %d", adpt->card->shortname, iface->description,
+		 channel_id);
+skip_adpt_alloc:
 	if (get_channel(iface, channel_id)) {
 		pr_err("channel (%s:%d) is already linked\n",
 		       iface->description, channel_id);
@@ -584,36 +647,25 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 		capture_count = 1;
 		direction = SNDRV_PCM_STREAM_CAPTURE;
 	}
-
-	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res);
-	if (ret < 0)
-		return ret;
-
-	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
-			   sizeof(*channel), &card);
-	if (ret < 0)
-		return ret;
-
-	channel = card->private_data;
-	channel->card = card;
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		goto err_free_card;
+	channel->card = adpt->card;
 	channel->cfg = cfg;
 	channel->iface = iface;
 	channel->id = channel_id;
 	init_waitqueue_head(&channel->playback_waitq);
+	list_add_tail(&channel->list, &adpt->dev_list);
+	list_add_tail(&adpt->list, &adpt_list);
 
 	ret = audio_set_hw_params(&channel->pcm_hardware, ch_num, sample_res,
 				  cfg);
 	if (ret)
 		goto err_free_card;
 
-	snprintf(card->driver, sizeof(card->driver), "%s", DRIVER_NAME);
-	snprintf(card->shortname, sizeof(card->shortname), "Microchip MOST:%d",
-		 card->number);
-	snprintf(card->longname, sizeof(card->longname), "%s at %s, ch %d",
-		 card->shortname, iface->description, channel_id);
+	ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
+			  playback_count, capture_count, &pcm);
 
-	ret = snd_pcm_new(card, card_name, 0, playback_count,
-			  capture_count, &pcm);
 	if (ret < 0)
 		goto err_free_card;
 
@@ -621,16 +673,16 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 
 	snd_pcm_set_ops(pcm, direction, &pcm_ops);
 
-	ret = snd_card_register(card);
-	if (ret < 0)
-		goto err_free_card;
-
-	list_add_tail(&channel->list, &dev_list);
-
+	if (create) {
+		ret = snd_card_register(adpt->card);
+		if (ret < 0)
+			goto err_free_card;
+		adpt->registered = true;
+	}
 	return 0;
 
 err_free_card:
-	snd_card_free(card);
+	release_adapter(adpt);
 	return ret;
 }
 
@@ -647,6 +699,7 @@ static int audio_disconnect_channel(struct most_interface *iface,
 				    int channel_id)
 {
 	struct channel *channel;
+	struct sound_adapter *adpt = iface->priv;
 
 	channel = get_channel(iface, channel_id);
 	if (!channel) {
@@ -656,8 +709,10 @@ static int audio_disconnect_channel(struct most_interface *iface,
 	}
 
 	list_del(&channel->list);
-	snd_card_free(channel->card);
 
+	kfree(channel);
+	if (list_empty(&adpt->dev_list))
+		release_adapter(adpt);
 	return 0;
 }
 
@@ -733,22 +788,14 @@ static int __init audio_init(void)
 {
 	pr_info("init()\n");
 
-	INIT_LIST_HEAD(&dev_list);
+	INIT_LIST_HEAD(&adpt_list);
 
 	return most_register_component(&comp);
 }
 
 static void __exit audio_exit(void)
 {
-	struct channel *channel, *tmp;
-
 	pr_info("exit()\n");
-
-	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
-		list_del(&channel->list);
-		snd_card_free(channel->card);
-	}
-
 	most_deregister_component(&comp);
 }
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/6] staging: most: sound: correct label name

[Christian Gromm]

This patch fixes the lable name that is used to jump to error
handling section of function audio_probe_channel() in case
something went wrong.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/sound/sound.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 41bcd2c..6db726b 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -649,7 +649,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	}
 	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
 	if (!channel)
-		goto err_free_card;
+		goto err_free_adpt;
 	channel->card = adpt->card;
 	channel->cfg = cfg;
 	channel->iface = iface;
@@ -661,13 +661,13 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	ret = audio_set_hw_params(&channel->pcm_hardware, ch_num, sample_res,
 				  cfg);
 	if (ret)
-		goto err_free_card;
+		goto err_free_adpt;
 
 	ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
 			  playback_count, capture_count, &pcm);
 
 	if (ret < 0)
-		goto err_free_card;
+		goto err_free_adpt;
 
 	pcm->private_data = channel;
 
@@ -676,12 +676,12 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	if (create) {
 		ret = snd_card_register(adpt->card);
 		if (ret < 0)
-			goto err_free_card;
+			goto err_free_adpt;
 		adpt->registered = true;
 	}
 	return 0;
 
-err_free_card:
+err_free_adpt:
 	release_adapter(adpt);
 	return ret;
 }
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/6] staging: most: sound: rename variable

[Christian Gromm]

Since the channels of a MOST device are now being represented as
individual PCM devices of one sound card, the variable card_name is not
suitable anymore to describe them. Therefore, this patch renames the
variable to device_name.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/sound/sound.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 6db726b..988cc55 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -471,14 +471,14 @@ static const struct snd_pcm_ops pcm_ops = {
 	.page       = snd_pcm_lib_get_vmalloc_page,
 };
 
-static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
+static int split_arg_list(char *buf, char **device_name, u16 *ch_num,
 			  char **sample_res, u8 *create)
 {
 	char *num;
 	int ret;
 
-	*card_name = strsep(&buf, ".");
-	if (!*card_name) {
+	*device_name = strsep(&buf, ".");
+	if (!*device_name) {
 		pr_err("Missing sound card name\n");
 		return -EIO;
 	}
@@ -587,7 +587,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	int capture_count = 0;
 	int ret;
 	int direction;
-	char *card_name;
+	char *device_name;
 	u16 ch_num;
 	u8 create = 0;
 	char *sample_res;
@@ -600,7 +600,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 		return -EINVAL;
 	}
 
-	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
+	ret = split_arg_list(arg_list, &device_name, &ch_num, &sample_res,
 			     &create);
 	if (ret < 0)
 		return ret;
@@ -622,7 +622,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	adpt->pcm_dev_idx = 0;
 	INIT_LIST_HEAD(&adpt->dev_list);
 	iface->priv = adpt;
-	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
+	ret = snd_card_new(&iface->dev, -1, device_name, THIS_MODULE,
 			   sizeof(*channel), &adpt->card);
 	if (ret < 0)
 		return ret;
@@ -663,14 +663,14 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	if (ret)
 		goto err_free_adpt;
 
-	ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
+	ret = snd_pcm_new(adpt->card, device_name, adpt->pcm_dev_idx,
 			  playback_count, capture_count, &pcm);
 
 	if (ret < 0)
 		goto err_free_adpt;
 
 	pcm->private_data = channel;
-
+	snprintf(pcm->name, sizeof(device_name), device_name);
 	snd_pcm_set_ops(pcm, direction, &pcm_ops);
 
 	if (create) {
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/6] staging: most: sound: use static name for ALSA card

[Christian Gromm]

This patch uses a static name for the sound card's short name and
long name. Having the card names configurable doesn't make sense
anymore, as the card represents the same physical hardware.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/sound/sound.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index 988cc55..bdf870f 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -622,14 +622,14 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	adpt->pcm_dev_idx = 0;
 	INIT_LIST_HEAD(&adpt->dev_list);
 	iface->priv = adpt;
-	ret = snd_card_new(&iface->dev, -1, device_name, THIS_MODULE,
+	ret = snd_card_new(&iface->dev, -1, "INIC", THIS_MODULE,
 			   sizeof(*channel), &adpt->card);
 	if (ret < 0)
 		return ret;
 	snprintf(adpt->card->driver, sizeof(adpt->card->driver),
 		 "%s", DRIVER_NAME);
 	snprintf(adpt->card->shortname, sizeof(adpt->card->shortname),
-		 "Microchip MOST:%d", adpt->card->number);
+		 "Microchip INIC");
 	snprintf(adpt->card->longname, sizeof(adpt->card->longname),
 		 "%s at %s, ch %d", adpt->card->shortname, iface->description,
 		 channel_id);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/6] staging: most: sound: remove channel number from ALSA card's long name

[Christian Gromm]

Adding the channel number to the name of the sound card is wrong,
as the card does not represent a single streaming channel of the
MOST device.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/sound/sound.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
index bdf870f..fbfdbc1 100644
--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -631,8 +631,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
 	snprintf(adpt->card->shortname, sizeof(adpt->card->shortname),
 		 "Microchip INIC");
 	snprintf(adpt->card->longname, sizeof(adpt->card->longname),
-		 "%s at %s, ch %d", adpt->card->shortname, iface->description,
-		 channel_id);
+		 "%s at %s", adpt->card->shortname, iface->description);
 skip_adpt_alloc:
 	if (get_channel(iface, channel_id)) {
 		pr_err("channel (%s:%d) is already linked\n",
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian Gromm]

This patch updates driver_usage.txt file to reflect the latest changes
that this patch set introduces.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/Documentation/driver_usage.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/most/Documentation/driver_usage.txt b/drivers/staging/most/Documentation/driver_usage.txt
index bb9b4e8..da7a8f4 100644
--- a/drivers/staging/most/Documentation/driver_usage.txt
+++ b/drivers/staging/most/Documentation/driver_usage.txt
@@ -142,8 +142,9 @@ Cdev component example:
 
 Sound component example:
 
-The sound component needs an additional parameter to determine the audio
-resolution that is going to be used. The following formats are available:
+The sound component needs additional parameters to determine the audio
+resolution that is going to be used and to trigger the registration of a
+sound card with ALSA. The following audio formats are available:
 
 	- "1x8" (Mono)
 	- "2x16" (16-bit stereo)
@@ -151,9 +152,18 @@ resolution that is going to be used. The following formats are available:
 	- "2x32" (32-bit stereo)
 	- "6x16" (16-bit surround 5.1)
 
-        $ echo "mdev0:ep_81:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
+To make the sound module create a sound card and register it with ALSA the
+string "create" needs to be attached to the module parameter section of the
+configuration string. To create a sound card with with two playback devices
+(linked to channel ep01 and ep02) and one capture device (linked to channel
+ep83) the following is written to the add_link file:
 
+        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
+        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
+        $ echo "mdev0:ep83:sound:most_capture.2x16.create" >$(DRV_DIR)/add_link
 
+The link names (most51_playback, most_playback and most_capture) will
+become the names of the PCM devices of the sound card.
 
 		Section 2.3 USB Padding
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> This patch avoids that a sound card is created and registered with ALSA
> every time a channel is being linked. Instead the channels are hooked on
> the same card, which is registered not until the final link has been added
> to the component.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>  drivers/staging/most/sound/sound.c | 127 +++++++++++++++++++++++++------------
>  1 file changed, 87 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
> index 89b02fc..41bcd2c 100644
> --- a/drivers/staging/most/sound/sound.c
> +++ b/drivers/staging/most/sound/sound.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/printk.h>
>  #include <linux/kernel.h>
> +#include <linux/slab.h>
>  #include <linux/init.h>
>  #include <sound/core.h>
>  #include <sound/pcm.h>
> @@ -20,7 +21,6 @@
>  
>  #define DRIVER_NAME "sound"
>  
> -static struct list_head dev_list;
>  static struct core_component comp;
>  
>  /**
> @@ -56,6 +56,17 @@ struct channel {
>  	void (*copy_fn)(void *alsa, void *most, unsigned int bytes);
>  };
>  
> +struct sound_adapter {
> +	struct list_head dev_list;
> +	struct most_interface *iface;
> +	struct snd_card *card;
> +	struct list_head list;
> +	bool registered;
> +	int pcm_dev_idx;
> +};
> +
> +static struct list_head adpt_list;
> +
>  #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
>  		       SNDRV_PCM_INFO_MMAP_VALID | \
>  		       SNDRV_PCM_INFO_BATCH | \
> @@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
>  static struct channel *get_channel(struct most_interface *iface,
>  				   int channel_id)
>  {
> +	struct sound_adapter *adpt = iface->priv;
>  	struct channel *channel, *tmp;
>  
> -	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> +	list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
>  		if ((channel->iface == iface) && (channel->id == channel_id))
>  			return channel;
>  	}
> @@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
>  };
>  
>  static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
> -			  char **sample_res)
> +			  char **sample_res, u8 *create)
>  {
>  	char *num;
>  	int ret;
> @@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
>  	*sample_res = strsep(&buf, ".\n");
>  	if (!*sample_res)
>  		goto err;
> +
> +	if (buf && !strcmp(buf, "create"))
> +		*create = 1;

This comes from userspace, right?  So we're adding a new API?

>  	return 0;
>  
>  err:
> @@ -536,6 +551,19 @@ static int audio_set_hw_params(struct snd_pcm_hardware *pcm_hw,
>  	return 0;
>  }
>  
> +static void release_adapter(struct sound_adapter *adpt)
> +{
> +	struct channel *channel, *tmp;
> +
> +	list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
> +		list_del(&channel->list);
> +		kfree(channel);
> +	}
> +	snd_card_free(adpt->card);
> +	list_del(&adpt->list);
> +	kfree(adpt);
> +}
> +
>  /**
>   * audio_probe_channel - probe function of the driver module
>   * @iface: pointer to interface instance
> @@ -553,7 +581,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  			       char *arg_list)
>  {
>  	struct channel *channel;
> -	struct snd_card *card;
> +	struct sound_adapter *adpt;
>  	struct snd_pcm *pcm;
>  	int playback_count = 0;
>  	int capture_count = 0;
> @@ -561,6 +589,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  	int direction;
>  	char *card_name;
>  	u16 ch_num;
> +	u8 create = 0;
>  	char *sample_res;
>  
>  	if (!iface)
> @@ -571,6 +600,40 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  		return -EINVAL;
>  	}
>  
> +	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
> +			     &create);
> +	if (ret < 0)
> +		return ret;
> +
> +	list_for_each_entry(adpt, &adpt_list, list) {
> +		if (adpt->iface == iface && adpt->registered)
> +			return -ENOSPC;
> +		if (!adpt->registered) {

This is very confusing and I'm sorry but I don't think it even works...

:(

We add new allocations to the &adpt_list, but then if audio_probe_channel()
fails, then we free "adpt" in the error handling.  But we don't remove
the adpt from the list so now if we iterate through the list again it's
a use after free.

It would be easy enought to remove the item from the list, but really my
issue is that I don't understand what we're trying to do here.  It looks
like this patch changes the user space interface and adds a "create"
argument?  I just think that's not a good API design.

> +			adpt->pcm_dev_idx++;
> +			goto skip_adpt_alloc;
> +		}
> +	}
> +	adpt = kzalloc(sizeof(*adpt), GFP_KERNEL);
> +	if (!adpt)
> +		return -ENOMEM;
> +
> +	adpt->iface = iface;
> +	adpt->registered = false;

No need for this.  adpt was allocated with kzalloc().

> +	adpt->pcm_dev_idx = 0;

Same.

> +	INIT_LIST_HEAD(&adpt->dev_list);
> +	iface->priv = adpt;
> +	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> +			   sizeof(*channel), &adpt->card);
> +	if (ret < 0)
> +		return ret;
> +	snprintf(adpt->card->driver, sizeof(adpt->card->driver),
> +		 "%s", DRIVER_NAME);
> +	snprintf(adpt->card->shortname, sizeof(adpt->card->shortname),
> +		 "Microchip MOST:%d", adpt->card->number);
> +	snprintf(adpt->card->longname, sizeof(adpt->card->longname),
> +		 "%s at %s, ch %d", adpt->card->shortname, iface->description,
> +		 channel_id);
> +skip_adpt_alloc:
>  	if (get_channel(iface, channel_id)) {
>  		pr_err("channel (%s:%d) is already linked\n",
>  		       iface->description, channel_id);
> @@ -584,36 +647,25 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  		capture_count = 1;
>  		direction = SNDRV_PCM_STREAM_CAPTURE;
>  	}
> -
> -	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> -			   sizeof(*channel), &card);
> -	if (ret < 0)
> -		return ret;
> -
> -	channel = card->private_data;
> -	channel->card = card;
> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> +	if (!channel)
> +		goto err_free_card;

We need to set "ret = -ENOMEM;" before the goto.


> +	channel->card = adpt->card;
>  	channel->cfg = cfg;
>  	channel->iface = iface;
>  	channel->id = channel_id;
>  	init_waitqueue_head(&channel->playback_waitq);
> +	list_add_tail(&channel->list, &adpt->dev_list);
> +	list_add_tail(&adpt->list, &adpt_list);
>  
>  	ret = audio_set_hw_params(&channel->pcm_hardware, ch_num, sample_res,
>  				  cfg);
>  	if (ret)
>  		goto err_free_card;
>  
> -	snprintf(card->driver, sizeof(card->driver), "%s", DRIVER_NAME);
> -	snprintf(card->shortname, sizeof(card->shortname), "Microchip MOST:%d",
> -		 card->number);
> -	snprintf(card->longname, sizeof(card->longname), "%s at %s, ch %d",
> -		 card->shortname, iface->description, channel_id);
> +	ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
> +			  playback_count, capture_count, &pcm);
>  
> -	ret = snd_pcm_new(card, card_name, 0, playback_count,
> -			  capture_count, &pcm);
>  	if (ret < 0)
>  		goto err_free_card;
>  
> @@ -621,16 +673,16 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  
>  	snd_pcm_set_ops(pcm, direction, &pcm_ops);
>  
> -	ret = snd_card_register(card);
> -	if (ret < 0)
> -		goto err_free_card;
> -
> -	list_add_tail(&channel->list, &dev_list);
> -
> +	if (create) {
> +		ret = snd_card_register(adpt->card);
> +		if (ret < 0)
> +			goto err_free_card;
> +		adpt->registered = true;
> +	}
>  	return 0;
>  
>  err_free_card:
> -	snd_card_free(card);
> +	release_adapter(adpt);
>  	return ret;
>  }
>  
> @@ -647,6 +699,7 @@ static int audio_disconnect_channel(struct most_interface *iface,
>  				    int channel_id)
>  {
>  	struct channel *channel;
> +	struct sound_adapter *adpt = iface->priv;
>  
>  	channel = get_channel(iface, channel_id);
>  	if (!channel) {
> @@ -656,8 +709,10 @@ static int audio_disconnect_channel(struct most_interface *iface,
>  	}
>  
>  	list_del(&channel->list);
> -	snd_card_free(channel->card);
>  
> +	kfree(channel);
> +	if (list_empty(&adpt->dev_list))
> +		release_adapter(adpt);

This is racy.  If we connect a new device at the same time then it leads
to a use after free.  And the thing is say a device gets bumped then it
can trigger a disconnect followed by an immediate reconnect so I could
easily imagine hitting this bug.

I don't really know this driver well (or at all) so can you maybe
explain the problem a bit and we can figure out a better solution?  Do
we not know which channels are available at probe time?  Do they change?

It feels like we should just take the list of channels and register them
all in a for loop and then register the sound card.  But obviously if it
were that simple, you would have done that so there is a problem here
which I have not seen...

regards,
dan carpenter

>  	return 0;
>  }
>  
> @@ -733,22 +788,14 @@ static int __init audio_init(void)
>  {
>  	pr_info("init()\n");
>  
> -	INIT_LIST_HEAD(&dev_list);
> +	INIT_LIST_HEAD(&adpt_list);
>  
>  	return most_register_component(&comp);
>  }
>  
>  static void __exit audio_exit(void)
>  {
> -	struct channel *channel, *tmp;
> -
>  	pr_info("exit()\n");
> -
> -	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> -		list_del(&channel->list);
> -		snd_card_free(channel->card);
> -	}
> -
>  	most_deregister_component(&comp);
>  }
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/6] staging: most: sound: rename variable

[Dan Carpenter]

On Wed, Dec 12, 2018 at 01:15:28PM +0100, Christian Gromm wrote:
> @@ -587,7 +587,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  	int capture_count = 0;
>  	int ret;
>  	int direction;
> -	char *card_name;
> +	char *device_name;
        ^^^^^^^^^^^^^^^^^

>  	u16 ch_num;
>  	u8 create = 0;
>  	char *sample_res;
> @@ -600,7 +600,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  		return -EINVAL;
>  	}
>  
> -	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
> +	ret = split_arg_list(arg_list, &device_name, &ch_num, &sample_res,
>  			     &create);
>  	if (ret < 0)
>  		return ret;
> @@ -622,7 +622,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  	adpt->pcm_dev_idx = 0;
>  	INIT_LIST_HEAD(&adpt->dev_list);
>  	iface->priv = adpt;
> -	ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> +	ret = snd_card_new(&iface->dev, -1, device_name, THIS_MODULE,
>  			   sizeof(*channel), &adpt->card);
>  	if (ret < 0)
>  		return ret;
> @@ -663,14 +663,14 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  	if (ret)
>  		goto err_free_adpt;
>  
> -	ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
> +	ret = snd_pcm_new(adpt->card, device_name, adpt->pcm_dev_idx,
>  			  playback_count, capture_count, &pcm);
>  
>  	if (ret < 0)
>  		goto err_free_adpt;
>  
>  	pcm->private_data = channel;
> -
> +	snprintf(pcm->name, sizeof(device_name), device_name);
                            ^^^^^^^^^^^^^^^^^^^

This change was not described in the commit message and it's not
correct.  sizeof(device_name) is sizeof(long).  We should be taking
sizeof(pcm->name) which is 80. 

>  	snd_pcm_set_ops(pcm, direction, &pcm_ops);
>  
>  	if (create) {

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Dan Carpenter]

On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> diff --git a/drivers/staging/most/Documentation/driver_usage.txt b/drivers/staging/most/Documentation/driver_usage.txt
> index bb9b4e8..da7a8f4 100644
> --- a/drivers/staging/most/Documentation/driver_usage.txt
> +++ b/drivers/staging/most/Documentation/driver_usage.txt
> @@ -142,8 +142,9 @@ Cdev component example:
>  
>  Sound component example:
>  
> -The sound component needs an additional parameter to determine the audio
> -resolution that is going to be used. The following formats are available:
> +The sound component needs additional parameters to determine the audio
> +resolution that is going to be used and to trigger the registration of a
> +sound card with ALSA. The following audio formats are available:
>  
>  	- "1x8" (Mono)
>  	- "2x16" (16-bit stereo)
> @@ -151,9 +152,18 @@ resolution that is going to be used. The following formats are available:
>  	- "2x32" (32-bit stereo)
>  	- "6x16" (16-bit surround 5.1)
>  
> -        $ echo "mdev0:ep_81:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> +To make the sound module create a sound card and register it with ALSA the
> +string "create" needs to be attached to the module parameter section of the
> +configuration string. To create a sound card with with two playback devices
> +(linked to channel ep01 and ep02) and one capture device (linked to channel
> +ep83) the following is written to the add_link file:
>  
> +        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep83:sound:most_capture.2x16.create" >$(DRV_DIR)/add_link
>  
> +The link names (most51_playback, most_playback and most_capture) will
> +become the names of the PCM devices of the sound card.

So this patchset does break userspace...  Which is allowed sometimes in
staging, but it's better to point it out in the original commit which
causes the breakage.

But really I don't like this API at all...  It feels like something
from decades ago.  There has to be a better way than this.

Unfortunately, I'm not clever enough to give you useful suggestions...

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Christian.Gromm]

On Mi, 2018-12-12 at 17:21 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> > 
> > This patch avoids that a sound card is created and registered with
> > ALSA
> > every time a channel is being linked. Instead the channels are
> > hooked on
> > the same card, which is registered not until the final link has
> > been added
> > to the component.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > ---
> >  drivers/staging/most/sound/sound.c | 127
> > +++++++++++++++++++++++++------------
> >  1 file changed, 87 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/staging/most/sound/sound.c
> > b/drivers/staging/most/sound/sound.c
> > index 89b02fc..41bcd2c 100644
> > --- a/drivers/staging/most/sound/sound.c
> > +++ b/drivers/staging/most/sound/sound.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/module.h>
> >  #include <linux/printk.h>
> >  #include <linux/kernel.h>
> > +#include <linux/slab.h>
> >  #include <linux/init.h>
> >  #include <sound/core.h>
> >  #include <sound/pcm.h>
> > @@ -20,7 +21,6 @@
> >  
> >  #define DRIVER_NAME "sound"
> >  
> > -static struct list_head dev_list;
> >  static struct core_component comp;
> >  
> >  /**
> > @@ -56,6 +56,17 @@ struct channel {
> >  	void (*copy_fn)(void *alsa, void *most, unsigned int
> > bytes);
> >  };
> >  
> > +struct sound_adapter {
> > +	struct list_head dev_list;
> > +	struct most_interface *iface;
> > +	struct snd_card *card;
> > +	struct list_head list;
> > +	bool registered;
> > +	int pcm_dev_idx;
> > +};
> > +
> > +static struct list_head adpt_list;
> > +
> >  #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> >  		       SNDRV_PCM_INFO_MMAP_VALID | \
> >  		       SNDRV_PCM_INFO_BATCH | \
> > @@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa,
> > void *most, unsigned int bytes)
> >  static struct channel *get_channel(struct most_interface *iface,
> >  				   int channel_id)
> >  {
> > +	struct sound_adapter *adpt = iface->priv;
> >  	struct channel *channel, *tmp;
> >  
> > -	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> > +	list_for_each_entry_safe(channel, tmp, &adpt->dev_list,
> > list) {
> >  		if ((channel->iface == iface) && (channel->id ==
> > channel_id))
> >  			return channel;
> >  	}
> > @@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
> >  };
> >  
> >  static int split_arg_list(char *buf, char **card_name, u16
> > *ch_num,
> > -			  char **sample_res)
> > +			  char **sample_res, u8 *create)
> >  {
> >  	char *num;
> >  	int ret;
> > @@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char
> > **card_name, u16 *ch_num,
> >  	*sample_res = strsep(&buf, ".\n");
> >  	if (!*sample_res)
> >  		goto err;
> > +
> > +	if (buf && !strcmp(buf, "create"))
> > +		*create = 1;
> This comes from userspace, right?  So we're adding a new API?
> 

An additional field is added to the configuration parameter,
which is provided by user space.
This seemed to be less painful than adding a new sysfs
file and make the configuration even more complicated. 

> > 
> >  	return 0;
> >  
> >  err:
> > @@ -536,6 +551,19 @@ static int audio_set_hw_params(struct
> > snd_pcm_hardware *pcm_hw,
> >  	return 0;
> >  }
> >  
> > +static void release_adapter(struct sound_adapter *adpt)
> > +{
> > +	struct channel *channel, *tmp;
> > +
> > +	list_for_each_entry_safe(channel, tmp, &adpt->dev_list,
> > list) {
> > +		list_del(&channel->list);
> > +		kfree(channel);
> > +	}
> > +	snd_card_free(adpt->card);
> > +	list_del(&adpt->list);
> > +	kfree(adpt);
> > +}
> > +
> >  /**
> >   * audio_probe_channel - probe function of the driver module
> >   * @iface: pointer to interface instance
> > @@ -553,7 +581,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  			       char *arg_list)
> >  {
> >  	struct channel *channel;
> > -	struct snd_card *card;
> > +	struct sound_adapter *adpt;
> >  	struct snd_pcm *pcm;
> >  	int playback_count = 0;
> >  	int capture_count = 0;
> > @@ -561,6 +589,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  	int direction;
> >  	char *card_name;
> >  	u16 ch_num;
> > +	u8 create = 0;
> >  	char *sample_res;
> >  
> >  	if (!iface)
> > @@ -571,6 +600,40 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  		return -EINVAL;
> >  	}
> >  
> > +	ret = split_arg_list(arg_list, &card_name, &ch_num,
> > &sample_res,
> > +			     &create);
> > +	if (ret < 0)
> > j+		return ret;
> > +
> > +	list_for_each_entry(adpt, &adpt_list, list) {
> > +		if (adpt->iface == iface && adpt->registered)
> > +			return -ENOSPC;
> > +		if (!adpt->registered) {
> This is very confusing and I'm sorry but I don't think it even
> works...
> 
> :(
> 
> We add new allocations to the &adpt_list, but then if
> audio_probe_channel()
> fails, then we free "adpt" in the error handling.  But we don't
> remove
> the adpt from the list so now if we iterate through the list again
> it's
> a use after free.
> 

The only location where "adpt" is being freed is inside of function
release_adapter(). And there it gets removed from the list right before
the call to kfree. Am I missing something?

> It would be easy enought to remove the item from the list, but really
> my
> issue is that I don't understand what we're trying to do here.  It
> looks
> like this patch changes the user space interface and adds a "create"
> argument?  I just think that's not a good API design.
> 
Maybe, but we need a trigger to notify the driver that all desired
channels have been linked and that the sound card should be
registered with ALSA. I tried creating a new dedicated sysfs file,
but this didn't really make the configuration pretty.

> > 
> > +			adpt->pcm_dev_idx++;
> > +			goto skip_adpt_alloc;
> > +		}
> > +	}
> > +	adpt = kzalloc(sizeof(*adpt), GFP_KERNEL);
> > +	if (!adpt)
> > +		return -ENOMEM;
> > +
> > +	adpt->iface = iface;
> > +	adpt->registered = false;
> No need for this.  adpt was allocated with kzalloc().

Right.

> 
> > 
> > +	adpt->pcm_dev_idx = 0;
> Same.
> 
> > 
> > +	INIT_LIST_HEAD(&adpt->dev_list);
> > +	iface->priv = adpt;
> > +	ret = snd_card_new(&iface->dev, -1, card_name,
> > THIS_MODULE,
> > +			   sizeof(*channel), &adpt->card);
> > +	if (ret < 0)
> > +		return ret;
> > +	snprintf(adpt->card->driver, sizeof(adpt->card->driver),
> > +		 "%s", DRIVER_NAME);
> > +	snprintf(adpt->card->shortname, sizeof(adpt->card-
> > >shortname),
> > +		 "Microchip MOST:%d", adpt->card->number);
> > +	snprintf(adpt->card->longname, sizeof(adpt->card-
> > >longname),
> > +		 "%s at %s, ch %d", adpt->card->shortname, iface-
> > >description,
> > +		 channel_id);
> > +skip_adpt_alloc:
> >  	if (get_channel(iface, channel_id)) {
> >  		pr_err("channel (%s:%d) is already linked\n",
> >  		       iface->description, channel_id);
> > @@ -584,36 +647,25 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  		capture_count = 1;
> >  		direction = SNDRV_PCM_STREAM_CAPTURE;
> >  	}
> > -
> > -	ret = split_arg_list(arg_list, &card_name, &ch_num,
> > &sample_res);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = snd_card_new(&iface->dev, -1, card_name,
> > THIS_MODULE,
> > -			   sizeof(*channel), &card);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	channel = card->private_data;
> > -	channel->card = card;
> > +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > +	if (!channel)
> > +		goto err_free_card;
> We need to set "ret = -ENOMEM;" before the goto.
> 
> 
> > 
> > +	channel->card = adpt->card;
> >  	channel->cfg = cfg;
> >  	channel->iface = iface;
> >  	channel->id = channel_id;
> >  	init_waitqueue_head(&channel->playback_waitq);
> > +	list_add_tail(&channel->list, &adpt->dev_list);
> > +	list_add_tail(&adpt->list, &adpt_list);
> >  
> >  	ret = audio_set_hw_params(&channel->pcm_hardware, ch_num,
> > sample_res,
> >  				  cfg);
> >  	if (ret)
> >  		goto err_free_card;
> >  
> > -	snprintf(card->driver, sizeof(card->driver), "%s",
> > DRIVER_NAME);
> > -	snprintf(card->shortname, sizeof(card->shortname),
> > "Microchip MOST:%d",
> > -		 card->number);
> > -	snprintf(card->longname, sizeof(card->longname), "%s at
> > %s, ch %d",
> > -		 card->shortname, iface->description, channel_id);
> > +	ret = snd_pcm_new(adpt->card, card_name, adpt-
> > >pcm_dev_idx,
> > +			  playback_count, capture_count, &pcm);
> >  
> > -	ret = snd_pcm_new(card, card_name, 0, playback_count,
> > -			  capture_count, &pcm);
> >  	if (ret < 0)
> >  		goto err_free_card;
> >  
> > @@ -621,16 +673,16 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  
> >  	snd_pcm_set_ops(pcm, direction, &pcm_ops);
> >  
> > -	ret = snd_card_register(card);
> > -	if (ret < 0)
> > -		goto err_free_card;
> > -
> > -	list_add_tail(&channel->list, &dev_list);
> > -
> > +	if (create) {
> > +		ret = snd_card_register(adpt->card);
> > +		if (ret < 0)
> > +			goto err_free_card;
> > +		adpt->registered = true;
> > +	}
> >  	return 0;
> >  
> >  err_free_card:
> > -	snd_card_free(card);
> > +	release_adapter(adpt);
> >  	return ret;
> >  }
> >  
> > @@ -647,6 +699,7 @@ static int audio_disconnect_channel(struct
> > most_interface *iface,
> >  				    int channel_id)
> >  {
> >  	struct channel *channel;
> > +	struct sound_adapter *adpt = iface->priv;
> >  
> >  	channel = get_channel(iface, channel_id);
> >  	if (!channel) {
> > @@ -656,8 +709,10 @@ static int audio_disconnect_channel(struct
> > most_interface *iface,
> >  	}
> >  
> >  	list_del(&channel->list);
> > -	snd_card_free(channel->card);
> >  
> > +	kfree(channel);
> > +	if (list_empty(&adpt->dev_list))
> > +		release_adapter(adpt);
> This is racy.  If we connect a new device at the same time then it
> leads
> to a use after free.  And the thing is say a device gets bumped then
> it
> can trigger a disconnect followed by an immediate reconnect so I
> could
> easily imagine hitting this bug.
> 

Connecting another device will result in a new "adpt" being allocated,
which will not interfere with the current one in question, will it?

> I don't really know this driver well (or at all) so can you maybe
> explain the problem a bit and we can figure out a better
> solution?  Do
> we not know which channels are available at probe time?  Do t
> hey change?
> 

No we don't.

The channels of a MOST device that are going to be used with ALSA
are not predetermined. We could have way more streaming channels
available within an interface than will actually be used to make up
an ALSA device.

> It feels like we should just take the list of channels and register
> them
> all in a for loop and then register the sound card.  But obviously if
> it
> were that simple, you would have done that so there is a problem here
> which I have not seen...
> 
After this change, the sound component will be the only one that
sinks more than one channel into one user entity. I tried to keep
the way how channels are being linked generic for all components.

thanks,
Chris

Re: [PATCH 3/6] staging: most: sound: rename variable

[Christian.Gromm]

On Mi, 2018-12-12 at 17:26 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:28PM +0100, Christian Gromm wrote:
> > 
> > @@ -587,7 +587,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  	int capture_count = 0;
> >  	int ret;
> >  	int direction;
> > -	char *card_name;
> > +	char *device_name;
>         ^^^^^^^^^^^^^^^^^
> 
> > 
> >  	u16 ch_num;
> >  	u8 create = 0;
> >  	char *sample_res;
> > @@ -600,7 +600,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = split_arg_list(arg_list, &card_name, &ch_num,
> > &sample_res,
> > +	ret = split_arg_list(arg_list, &device_name, &ch_num,
> > &sample_res,
> >  			     &create);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -622,7 +622,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  	adpt->pcm_dev_idx = 0;
> >  	INIT_LIST_HEAD(&adpt->dev_list);
> >  	iface->priv = adpt;
> > -	ret = snd_card_new(&iface->dev, -1, card_name,
> > THIS_MODULE,
> > +	ret = snd_card_new(&iface->dev, -1, device_name,
> > THIS_MODULE,
> >  			   sizeof(*channel), &adpt->card);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -663,14 +663,14 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  	if (ret)
> >  		goto err_free_adpt;
> >  
> > -	ret = snd_pcm_new(adpt->card, card_name, adpt-
> > >pcm_dev_idx,
> > +	ret = snd_pcm_new(adpt->card, device_name, adpt-
> > >pcm_dev_idx,
> >  			  playback_count, capture_count, &pcm);
> >  
> >  	if (ret < 0)
> >  		goto err_free_adpt;
> >  
> >  	pcm->private_data = channel;
> > -
> > +	snprintf(pcm->name, sizeof(device_name), device_name);
>                             ^^^^^^^^^^^^^^^^^^^
> 
> This change was not described in the commit message and it's not
> correct.  sizeof(device_name) is sizeof(long).  We should be taking
> sizeof(pcm->name) which is 80. 
> 
Right, this needs to be a separate patch.
Missed that one while interactively staging the individual changes. 

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian.Gromm]

On Mi, 2018-12-12 at 17:31 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> > 
> > diff --git a/drivers/staging/most/Documentation/driver_usage.txt
> > b/drivers/staging/most/Documentation/driver_usage.txt
> > index bb9b4e8..da7a8f4 100644
> > --- a/drivers/staging/most/Documentation/driver_usage.txt
> > +++ b/drivers/staging/most/Documentation/driver_usage.txt
> > @@ -142,8 +142,9 @@ Cdev component example:
> >  
> >  Sound component example:
> >  
> > -The sound component needs an additional parameter to determine the
> > audio
> > -resolution that is going to be used. The following formats are
> > available:
> > +The sound component needs additional parameters to determine the
> > audio
> > +resolution that is going to be used and to trigger the
> > registration of a
> > +sound card with ALSA. The following audio formats are available:
> >  
> >  	- "1x8" (Mono)
> >  	- "2x16" (16-bit stereo)
> > @@ -151,9 +152,18 @@ resolution that is going to be used. The
> > following formats are available:
> >  	- "2x32" (32-bit stereo)
> >  	- "6x16" (16-bit surround 5.1)
> >  
> > -        $ echo "mdev0:ep_81:sound:most51_playback.6x16"
> > >$(DRV_DIR)/add_link
> > +To make the sound module create a sound card and register it with
> > ALSA the
> > +string "create" needs to be attached to the module parameter
> > section of the
> > +configuration string. To create a sound card with with two
> > playback devices
> > +(linked to channel ep01 and ep02) and one capture device (linked
> > to channel
> > +ep83) the following is written to the add_link file:
> >  
> > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep83:sound:most_capture.2x16.create"
> > >$(DRV_DIR)/add_link
> >  
> > +The link names (most51_playback, most_playback and most_capture)
> > will
> > +become the names of the PCM devices of the sound card.
> So this patchset does break userspace...  Which is allowed sometimes
> in
> staging, but it's better to point it out in the original commit which
> causes the breakage.
> 

The sound card layout that we have in mind is, from a system 
architecture point of view, the better one. Unfortunately, it does
come with the cost of changing the configuration in one way or the 
other. Which is not really great, I agree on that.

What I don't see is, why enhancing the way how user space interferes 
wi
th the  driver is such a terrible thing to do in staging. This 
should
be the place to be to develop and change things (unless I 
totally
misunderstood the concept behind it).

> But really I don't like this API at all...  It feels like something

Fair enough. Is it just latest change that made you feel this way,
or didn't you like the process in the first place?

> from decades ago.  There has to be a better way than this.
> 
 
What if I would find a better way and change the API into something
you like. Wouldn't that break user space too?

Is this a dead end? How am I supposed to go on?

> Unfortunately, I'm not clever enough to give you useful
> suggestions...
> 

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Dan Carpenter]

I'm not really complaining about breaking userspace, I'm complaining
that I had to discover it by reading the code.  It should have been
mentioned in the commit message.  We should probably just fold
patch 1 & 6 together.

I'm also not really complaining about this patch in particular when it
comes to the API.  The whole thing is a bit weird to me.  I wish we
could re-think the configuration from square one...

It could be done in a later patch.  I'm not going to NAK this patch if
you resend it with the other small issues fixed.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Dan Carpenter]

On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> This patch updates driver_usage.txt file to reflect the latest changes
> that this patch set introduces.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>  drivers/staging/most/Documentation/driver_usage.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/most/Documentation/driver_usage.txt b/drivers/staging/most/Documentation/driver_usage.txt
> index bb9b4e8..da7a8f4 100644
> --- a/drivers/staging/most/Documentation/driver_usage.txt
> +++ b/drivers/staging/most/Documentation/driver_usage.txt
> @@ -142,8 +142,9 @@ Cdev component example:
>  
>  Sound component example:
>  
> -The sound component needs an additional parameter to determine the audio
> -resolution that is going to be used. The following formats are available:
> +The sound component needs additional parameters to determine the audio
> +resolution that is going to be used and to trigger the registration of a
> +sound card with ALSA. The following audio formats are available:
>  
>  	- "1x8" (Mono)
>  	- "2x16" (16-bit stereo)
> @@ -151,9 +152,18 @@ resolution that is going to be used. The following formats are available:
>  	- "2x32" (32-bit stereo)
>  	- "6x16" (16-bit surround 5.1)
>  
> -        $ echo "mdev0:ep_81:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> +To make the sound module create a sound card and register it with ALSA the
> +string "create" needs to be attached to the module parameter section of the
> +configuration string. To create a sound card with with two playback devices
> +(linked to channel ep01 and ep02) and one capture device (linked to channel
> +ep83) the following is written to the add_link file:
>  
> +        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep83:sound:most_capture.2x16.create" >$(DRV_DIR)/add_link

I would maybe prefer if the "create" command were on a separate line.
Something like:

+        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
+        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
+        $ echo "mdev0:ep83:sound:most_capture.2x16" >$(DRV_DIR)/add_link
+        $ echo "mdev0:ep83:sound:create" >$(DRV_DIR)/sound_card

It's sort of a separate command in a way?

I'm not sure...

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@microchip.com wrote:
> On Mi, 2018-12-12 at 17:21 +0300, Dan Carpenter wrote:
> > On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> > > 
> > > This patch avoids that a sound card is created and registered with
> > > ALSA
> > > every time a channel is being linked. Instead the channels are
> > > hooked on
> > > the same card, which is registered not until the final link has
> > > been added
> > > to the component.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > ---
> > >  drivers/staging/most/sound/sound.c | 127
> > > +++++++++++++++++++++++++------------
> > >  1 file changed, 87 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/staging/most/sound/sound.c
> > > b/drivers/staging/most/sound/sound.c
> > > index 89b02fc..41bcd2c 100644
> > > --- a/drivers/staging/most/sound/sound.c
> > > +++ b/drivers/staging/most/sound/sound.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/printk.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/slab.h>
> > >  #include <linux/init.h>
> > >  #include <sound/core.h>
> > >  #include <sound/pcm.h>
> > > @@ -20,7 +21,6 @@
> > >  
> > >  #define DRIVER_NAME "sound"
> > >  
> > > -static struct list_head dev_list;
> > >  static struct core_component comp;
> > >  
> > >  /**
> > > @@ -56,6 +56,17 @@ struct channel {
> > >  	void (*copy_fn)(void *alsa, void *most, unsigned int
> > > bytes);
> > >  };
> > >  
> > > +struct sound_adapter {
> > > +	struct list_head dev_list;
> > > +	struct most_interface *iface;
> > > +	struct snd_card *card;
> > > +	struct list_head list;
> > > +	bool registered;
> > > +	int pcm_dev_idx;
> > > +};
> > > +
> > > +static struct list_head adpt_list;
> > > +
> > >  #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > >  		       SNDRV_PCM_INFO_MMAP_VALID | \
> > >  		       SNDRV_PCM_INFO_BATCH | \
> > > @@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa,
> > > void *most, unsigned int bytes)
> > >  static struct channel *get_channel(struct most_interface *iface,
> > >  				   int channel_id)
> > >  {
> > > +	struct sound_adapter *adpt = iface->priv;
> > >  	struct channel *channel, *tmp;
> > >  
> > > -	list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> > > +	list_for_each_entry_safe(channel, tmp, &adpt->dev_list,
> > > list) {
> > >  		if ((channel->iface == iface) && (channel->id ==
> > > channel_id))
> > >  			return channel;
> > >  	}
> > > @@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
> > >  };
> > >  
> > >  static int split_arg_list(char *buf, char **card_name, u16
> > > *ch_num,
> > > -			  char **sample_res)
> > > +			  char **sample_res, u8 *create)
> > >  {
> > >  	char *num;
> > >  	int ret;
> > > @@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char
> > > **card_name, u16 *ch_num,
> > >  	*sample_res = strsep(&buf, ".\n");
> > >  	if (!*sample_res)
> > >  		goto err;
> > > +
> > > +	if (buf && !strcmp(buf, "create"))
> > > +		*create = 1;
> > This comes from userspace, right?  So we're adding a new API?
> > 
> 
> An additional field is added to the configuration parameter,
> which is provided by user space.
> This seemed to be less painful than adding a new sysfs
> file and make the configuration even more complicated. 

Using sysfs to configure it might actually be the right thing...

Four commands:  Add link, Add link, Add link, create.

> > We add new allocations to the &adpt_list, but then if
> > audio_probe_channel()
> > fails, then we free "adpt" in the error handling.  But we don't
> > remove
> > the adpt from the list so now if we iterate through the list again
> > it's
> > a use after free.
> > 
> 
> The only location where "adpt" is being freed is inside of function
> release_adapter(). And there it gets removed from the list right before
> the call to kfree. Am I missing something?

My bad, I didn't read the code properly.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> @@ -571,6 +600,40 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>  		return -EINVAL;
>  	}
>  
> +	ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
> +			     &create);
> +	if (ret < 0)
> +		return ret;
> +
> +	list_for_each_entry(adpt, &adpt_list, list) {
> +		if (adpt->iface == iface && adpt->registered)
> +			return -ENOSPC;
> +		if (!adpt->registered) {
> +			adpt->pcm_dev_idx++;
> +			goto skip_adpt_alloc;

We haven't ensured the adpt->iface == iface.

> +		}
> +	}

Probably you want to say:

	list_for_each_entry(adpt, &adpt_list, list) {
		if (adpt->iface != iface)
			continue;
		if (adpt->registered)
			return -ENOSPC;
		adpt->pcm_dev_idx++;
		goto skip_adpt_alloc;
	}

But here again, I think I might prefer if allocating a new "adpt" were
and explicit command from user space as opposed to just a side effect of
registering a new iface.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Greg KH]

On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> > This patch updates driver_usage.txt file to reflect the latest changes
> > that this patch set introduces.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > ---
> >  drivers/staging/most/Documentation/driver_usage.txt | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/most/Documentation/driver_usage.txt b/drivers/staging/most/Documentation/driver_usage.txt
> > index bb9b4e8..da7a8f4 100644
> > --- a/drivers/staging/most/Documentation/driver_usage.txt
> > +++ b/drivers/staging/most/Documentation/driver_usage.txt
> > @@ -142,8 +142,9 @@ Cdev component example:
> >  
> >  Sound component example:
> >  
> > -The sound component needs an additional parameter to determine the audio
> > -resolution that is going to be used. The following formats are available:
> > +The sound component needs additional parameters to determine the audio
> > +resolution that is going to be used and to trigger the registration of a
> > +sound card with ALSA. The following audio formats are available:
> >  
> >  	- "1x8" (Mono)
> >  	- "2x16" (16-bit stereo)
> > @@ -151,9 +152,18 @@ resolution that is going to be used. The following formats are available:
> >  	- "2x32" (32-bit stereo)
> >  	- "6x16" (16-bit surround 5.1)
> >  
> > -        $ echo "mdev0:ep_81:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> > +To make the sound module create a sound card and register it with ALSA the
> > +string "create" needs to be attached to the module parameter section of the
> > +configuration string. To create a sound card with with two playback devices
> > +(linked to channel ep01 and ep02) and one capture device (linked to channel
> > +ep83) the following is written to the add_link file:
> >  
> > +        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep83:sound:most_capture.2x16.create" >$(DRV_DIR)/add_link
> 
> I would maybe prefer if the "create" command were on a separate line.
> Something like:
> 
> +        $ echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep02:sound:most_playback.2x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep83:sound:most_capture.2x16" >$(DRV_DIR)/add_link
> +        $ echo "mdev0:ep83:sound:create" >$(DRV_DIR)/sound_card
> 
> It's sort of a separate command in a way?

Why is sysfs being used for configuring things?  Isn't that what
configfs was created for?  :)

thanks,

greg k-h

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@microchip.com wrote:
> An additional field is added to the configuration parameter,
> which is provided by user space.
> This seemed to be less painful than adding a new sysfs
> file and make the configuration even more complicated. 

I think that adding a bunch of new sysfs files *and directories* is the
way to go actually.

echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link

I've never used the most driver but my guess is that $DRV_DIR is
/sys/module/most/.  In theory, you're supposed to write one word or
number to sysfs files so this is sort of a misuse of the API.

There should be an mdev0/ep01/sound/most51_playback/ directory.  Maybe
every ep01 channel will have a sound/ subdirectory or maybe we have to
create it manually, I don't know.

There should be a command to create an new adpt, and another command to
add some commponents to it and a last command to register it with the
kernel.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Thu, Dec 13, 2018 at 03:38:03PM +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@microchip.com wrote:
> > An additional field is added to the configuration parameter,
> > which is provided by user space.
> > This seemed to be less painful than adding a new sysfs
> > file and make the configuration even more complicated. 
> 
> I think that adding a bunch of new sysfs files *and directories* is the
> way to go actually.

Or configfs.  But I think generally my email still applies in that we
should break things up into separate create, add link, and register
commands.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Christian.Gromm]

On Do, 2018-12-13 at 15:16 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> > 
> > @@ -571,6 +600,40 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  		return -EINVAL;
> >  	}
> >  
> > +	ret = split_arg_list(arg_list, &card_name, &ch_num,
> > &sample_res,
> > +			     &create);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	list_for_each_entry(adpt, &adpt_list, list) {
> > +		if (adpt->iface == iface && adpt->registered)
> > +			return -ENOSPC;
> > +		if (!adpt->registered) {
> > +			adpt->pcm_dev_idx++;
> > +			goto skip_adpt_alloc;
> We haven't ensured the adpt->iface == iface.
> 
> > 
> > +		}
> > +	}
> Probably you want to say:
> 
> 	list_for_each_entry(adpt, &adpt_list, list) {
> 		if (adpt->iface != iface)
> 			continue;
> 		if (adpt->registered)
> 			return -ENOSPC;
> 		adpt->pcm_dev_idx++;
> 		goto skip_adpt_alloc;
> 	}
> 

I do.

> But here again, I think I might prefer if allocating a new "adpt"
> were
> and explicit command from user space as opposed to just a side effect
> of
> registering a new iface.
> 

Sounds reasonable. But, problem here is that we want the
process of how channels are linked to be independent from the 
component that is being used. That's when things start to
become complicated. 
In other words, I don't want the sound module to be configured
in a different way than the cdev module, networking module or the
v4l2 module is. The goal was to have only sysfs files that
are generic (->default attrs of most_core mod) and apply to all 
components.

The latest change of the sound module creates the need of
signaling that the user is done with the config of the ALSA card.
The quickest way to achieve this, was to expand the "module
parameter" field (which we already have) with the "create" flag
that is passed to the module when a channel is being probed as a 
trigger.

Because no other module needs such a trigger, creating a dedicated 
sysfs file for this purpose seemed kind of weird, especially when
the sound module isn't loaded at all.

Maybe the sound module should create its own file, but since it
has no struct device embedded, I'm not sure how to achieve that :/

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Christian.Gromm]

On Do, 2018-12-13 at 15:38 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@microchip.c
> om wrote:
> > 
> > An additional field is added to the configuration parameter,
> > which is provided by user space.
> > This seemed to be less painful than adding a new sysfs
> > file and make the configuration even more complicated. 
> I think that adding a bunch of new sysfs files *and directories* is
> the
> way to go actually.
> 
> echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link
> 
> I've never used the most driver but my guess is that $DRV_DIR is
> /sys/module/most/.  In theory, you're supposed to write one word or
> number to sysfs files so this is sort of a misuse of the API.
> 

Well, you have to have a MOST or INICnet Infotainment network on
your desk to use the driver.

Actually, the $DRV_DIR is /sys/bus/most/drivers/most_core.

And if the driver is loaded and a MOST device is
attached that features two channels we get:

/sys/bus/most/
├── devices
│   └── mdev0 -> ../../../devices/most_bus/mdev0
├── drivers
│   └── most_core
│       ├── add_link
│       ├── bind
│       ├── components
│       ├── links
│       ├── mdev0 -> ../../../../devices/most_bus/mdev0
│       ├── remove_link
│       ├── uevent
│       └── unbind
├── drivers_autoprobe
├── drivers_probe
└── uevent

Having a new default attr under $DRV_DIR that is exclusively
used for the sound module is not really nice, unless it appears
only in case the sound module itself is loaded. But as I
said, I'm not sure if we can establish that with the current
struct core_component as it lacks the struct device.  


thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian.Gromm]

On Do, 2018-12-13 at 14:54 +0300, Dan Carpenter wrote:
> I'm not really complaining about breaking userspace, I'm complaining
> that I had to discover it by reading the code.  It should have been
> mentioned in the commit message.  We should probably just fold
> patch 1 & 6 together.
> 
Ok, I probably should have mentioned this in the cover letter.
Thought the usage file addition is enough. Sorry for the
inconvenience. 

> I'm also not really complaining about this patch in particular when
> it
> comes to the API.  The whole thing is a bit weird to me.  I wish we
> could re-think the configuration from square one...
> 
> It could be done in a later patch.  I'm not going to NAK this patch
> if
> you resend it with the other small issues fixed.
> 

Greg put configfs on the table. But this is like a total
redesign of the
driver interface which will take some
time.

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian.Gromm]

On Do, 2018-12-13 at 13:32 +0100, Greg KH wrote:
> On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote:
> > 
> > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> > > 
> > > This patch updates driver_usage.txt file to reflect the latest
> > > changes
> > > that this patch set introduces.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > ---
> > >  drivers/staging/most/Documentation/driver_usage.txt | 16
> > > +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/most/Documentation/driver_usage.txt
> > > b/drivers/staging/most/Documentation/driver_usage.txt
> > > index bb9b4e8..da7a8f4 100644
> > > --- a/drivers/staging/most/Documentation/driver_usage.txt
> > > +++ b/drivers/staging/most/Documentation/driver_usage.txt
> > > @@ -142,8 +142,9 @@ Cdev component example:
> > >  
> > >  Sound component example:
> > >  
> > > -The sound component needs an additional parameter to determine
> > > the audio
> > > -resolution that is going to be used. The following formats are
> > > available:
> > > +The sound component needs additional parameters to determine the
> > > audio
> > > +resolution that is going to be used and to trigger the
> > > registration of a
> > > +sound card with ALSA. The following audio formats are available:
> > >  
> > >  	- "1x8" (Mono)
> > >  	- "2x16" (16-bit stereo)
> > > @@ -151,9 +152,18 @@ resolution that is going to be used. The
> > > following formats are available:
> > >  	- "2x32" (32-bit stereo)
> > >  	- "6x16" (16-bit surround 5.1)
> > >  
> > > -        $ echo "mdev0:ep_81:sound:most51_playback.6x16"
> > > >$(DRV_DIR)/add_link
> > > +To make the sound module create a sound card and register it
> > > with ALSA the
> > > +string "create" needs to be attached to the module parameter
> > > section of the
> > > +configuration string. To create a sound card with with two
> > > playback devices
> > > +(linked to channel ep01 and ep02) and one capture device (linked
> > > to channel
> > > +ep83) the following is written to the add_link file:
> > >  
> > > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > > >$(DRV_DIR)/add_link
> > > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > > >$(DRV_DIR)/add_link
> > > +        $ echo "mdev0:ep83:sound:most_capture.2x16.create"
> > > >$(DRV_DIR)/add_link
> > I would maybe prefer if the "create" command were on a separate
> > line.
> > Something like:
> > 
> > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep83:sound:most_capture.2x16"
> > >$(DRV_DIR)/add_link
> > +        $ echo "mdev0:ep83:sound:create" >$(DRV_DIR)/sound_card
> > 
> > It's sort of a separate command in a way?
> Why is sysfs being used for configuring things?  Isn't that what
> configfs was created for?  :)
> 

Omg, that would be one API change!

I need to dig a little deeper into configfs to recognize
its beauty and the possible benefit for our driver.

Do you see this as a prerequisite?  

thanks,
Chris

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Greg KH]

On Thu, Dec 13, 2018 at 04:32:25PM +0000, Christian.Gromm@microchip.com wrote:
> On Do, 2018-12-13 at 13:32 +0100, Greg KH wrote:
> > On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote:
> > > 
> > > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote:
> > > > 
> > > > This patch updates driver_usage.txt file to reflect the latest
> > > > changes
> > > > that this patch set introduces.
> > > > 
> > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > > ---
> > > >  drivers/staging/most/Documentation/driver_usage.txt | 16
> > > > +++++++++++++---
> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/most/Documentation/driver_usage.txt
> > > > b/drivers/staging/most/Documentation/driver_usage.txt
> > > > index bb9b4e8..da7a8f4 100644
> > > > --- a/drivers/staging/most/Documentation/driver_usage.txt
> > > > +++ b/drivers/staging/most/Documentation/driver_usage.txt
> > > > @@ -142,8 +142,9 @@ Cdev component example:
> > > >  
> > > >  Sound component example:
> > > >  
> > > > -The sound component needs an additional parameter to determine
> > > > the audio
> > > > -resolution that is going to be used. The following formats are
> > > > available:
> > > > +The sound component needs additional parameters to determine the
> > > > audio
> > > > +resolution that is going to be used and to trigger the
> > > > registration of a
> > > > +sound card with ALSA. The following audio formats are available:
> > > >  
> > > >  	- "1x8" (Mono)
> > > >  	- "2x16" (16-bit stereo)
> > > > @@ -151,9 +152,18 @@ resolution that is going to be used. The
> > > > following formats are available:
> > > >  	- "2x32" (32-bit stereo)
> > > >  	- "6x16" (16-bit surround 5.1)
> > > >  
> > > > -        $ echo "mdev0:ep_81:sound:most51_playback.6x16"
> > > > >$(DRV_DIR)/add_link
> > > > +To make the sound module create a sound card and register it
> > > > with ALSA the
> > > > +string "create" needs to be attached to the module parameter
> > > > section of the
> > > > +configuration string. To create a sound card with with two
> > > > playback devices
> > > > +(linked to channel ep01 and ep02) and one capture device (linked
> > > > to channel
> > > > +ep83) the following is written to the add_link file:
> > > >  
> > > > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > > > >$(DRV_DIR)/add_link
> > > > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > > > >$(DRV_DIR)/add_link
> > > > +        $ echo "mdev0:ep83:sound:most_capture.2x16.create"
> > > > >$(DRV_DIR)/add_link
> > > I would maybe prefer if the "create" command were on a separate
> > > line.
> > > Something like:
> > > 
> > > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > > >$(DRV_DIR)/add_link
> > > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > > >$(DRV_DIR)/add_link
> > > +        $ echo "mdev0:ep83:sound:most_capture.2x16"
> > > >$(DRV_DIR)/add_link
> > > +        $ echo "mdev0:ep83:sound:create" >$(DRV_DIR)/sound_card
> > > 
> > > It's sort of a separate command in a way?
> > Why is sysfs being used for configuring things?  Isn't that what
> > configfs was created for?  :)
> > 
> 
> Omg, that would be one API change!
> 
> I need to dig a little deeper into configfs to recognize
> its beauty and the possible benefit for our driver.
> 
> Do you see this as a prerequisite?  

Writing random strings to a random sysfs file to configure things is not
a "normal" user/kernel api as sysfs files are supposed to just be "one
value" and not parsed like what you are doing here.

So I would strongly suggest looking at configfs, that is what it was
designed for.

thanks,

greg k-h

Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device

[Dan Carpenter]

On Thu, Dec 13, 2018 at 02:15:59PM +0000, Christian.Gromm@microchip.com wrote:
> Sounds reasonable. But, problem here is that we want the
> process of how channels are linked to be independent from the 
> component that is being used. That's when things start to
> become complicated. 

I'm not going NAK the patchset because of the API, but I do think we
need to rethink it before we move this out of staging.  Greg has
suggested configfs, but you might also want to consult with the
linux-media or ALSA devs because there might be some subsystem specific
patterns to consider.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian.Gromm]

On Do, 2018-12-13 at 18:55 +0100, Greg KH wrote:
> On Thu, Dec 13, 2018 at 04:32:25PM +0000, Christian.Gromm@microchip.c
> om wrote:
> > 
> > On Do, 2018-12-13 at 13:32 +0100, Greg KH wrote:
> > > 
> > > On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote:
> > > > 
> > > > 
> > > > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm
> > > > wrote:
> > > > > 
> > > > > 
> > > > > This patch updates driver_usage.txt file to reflect the
> > > > > latest
> > > > > changes
> > > > > that this patch set introduces.
> > > > > 
> > > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com
> > > > > >
> > > > > ---
> > > > >  drivers/staging/most/Documentation/driver_usage.txt | 16
> > > > > +++++++++++++---
> > > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/staging/most/Documentation/driver_usage.txt
> > > > > b/drivers/staging/most/Documentation/driver_usage.txt
> > > > > index bb9b4e8..da7a8f4 100644
> > > > > --- a/drivers/staging/most/Documentation/driver_usage.txt
> > > > > +++ b/drivers/staging/most/Documentation/driver_usage.txt
> > > > > @@ -142,8 +142,9 @@ Cdev component example:
> > > > >  
> > > > >  Sound component example:
> > > > >  
> > > > > -The sound component needs an additional parameter to
> > > > > determine
> > > > > the audio
> > > > > -resolution that is going to be used. The following formats
> > > > > are
> > > > > available:
> > > > > +The sound component needs additional parameters to determine
> > > > > the
> > > > > audio
> > > > > +resolution that is going to be used and to trigger the
> > > > > registration of a
> > > > > +sound card with ALSA. The following audio formats are
> > > > > available:
> > > > >  
> > > > >  	- "1x8" (Mono)
> > > > >  	- "2x16" (16-bit stereo)
> > > > > @@ -151,9 +152,18 @@ resolution that is going to be used. The
> > > > > following formats are available:
> > > > >  	- "2x32" (32-bit stereo)
> > > > >  	- "6x16" (16-bit surround 5.1)
> > > > >  
> > > > > -        $ echo "mdev0:ep_81:sound:most51_playback.6x16"
> > > > > > 
> > > > > > $(DRV_DIR)/add_link
> > > > > +To make the sound module create a sound card and register it
> > > > > with ALSA the
> > > > > +string "create" needs to be attached to the module parameter
> > > > > section of the
> > > > > +configuration string. To create a sound card with with two
> > > > > playback devices
> > > > > +(linked to channel ep01 and ep02) and one capture device
> > > > > (linked
> > > > > to channel
> > > > > +ep83) the following is written to the add_link file:
> > > > >  
> > > > > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > > > > > 
> > > > > > $(DRV_DIR)/add_link
> > > > > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > > > > > 
> > > > > > $(DRV_DIR)/add_link
> > > > > +        $ echo "mdev0:ep83:sound:most_capture.2x16.create"
> > > > > > 
> > > > > > $(DRV_DIR)/add_link
> > > > I would maybe prefer if the "create" command were on a separate
> > > > line.
> > > > Something like:
> > > > 
> > > > +        $ echo "mdev0:ep01:sound:most51_playback.6x16"
> > > > > 
> > > > > $(DRV_DIR)/add_link
> > > > +        $ echo "mdev0:ep02:sound:most_playback.2x16"
> > > > > 
> > > > > $(DRV_DIR)/add_link
> > > > +        $ echo "mdev0:ep83:sound:most_capture.2x16"
> > > > > 
> > > > > $(DRV_DIR)/add_link
> > > > +        $ echo "mdev0:ep83:sound:create"
> > > > >$(DRV_DIR)/sound_card
> > > > 
> > > > It's sort of a separate command in a way?
> > > Why is sysfs being used for configuring things?  Isn't that what
> > > configfs was created for?  :)
> > > 
> > Omg, that would be one API change!
> > 
> > I need to dig a little deeper into configfs to recognize
> > its beauty and the possible benefit for our driver.
> > 
> > Do you see this as a prerequisite?  
> Writing random strings to a random sysfs file to configure things is
> not
> a "normal" user/kernel api as sysfs files are supposed to just be
> "one
> value" and not parsed like what you are doing here.
> 
> So I would strongly suggest looking at configfs, that is what it was
> designed for.
> 

Message received. Anyway, I would like to resent the current
patch set with the things Dan found fixed. And then looking
into Configfs. Would this be ok for you guys?

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Dan Carpenter]

On Fri, Dec 14, 2018 at 09:06:01AM +0000, Christian.Gromm@microchip.com wrote:
> Message received. Anyway, I would like to resent the current
> patch set with the things Dan found fixed. And then looking
> into Configfs. Would this be ok for you guys?
> 

Sure.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Christian.Gromm]

On Fr, 2018-12-14 at 12:10 +0300, Dan Carpenter wrote:
> On Fri, Dec 14, 2018 at 09:06:01AM +0000, Christian.Gromm@microchip.c
> om wrote:
> > 
> > Message received. Anyway, I would like to resent the current
> > patch set with the things Dan found fixed. And then looking
> > into Configfs. Would this be ok for you guys?
> > 
> Sure.
> 

I'm not sure what is the best way to resend this.

Can you skip the series and I'm going to send an entire
new one. Or should I create a v2 of some of the patches
of the series and only resend those and you keep the others?
 

thanks,
Chris
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Dan Carpenter]

On Fri, Dec 14, 2018 at 09:33:50AM +0000, Christian.Gromm@microchip.com wrote:
> On Fr, 2018-12-14 at 12:10 +0300, Dan Carpenter wrote:
> > On Fri, Dec 14, 2018 at 09:06:01AM +0000, Christian.Gromm@microchip.c
> > om wrote:
> > > 
> > > Message received. Anyway, I would like to resent the current
> > > patch set with the things Dan found fixed. And then looking
> > > into Configfs. Would this be ok for you guys?
> > > 
> > Sure.
> > 
> 
> I'm not sure what is the best way to resend this.
> 
> Can you skip the series and I'm going to send an entire
> new one. Or should I create a v2 of some of the patches
> of the series and only resend those and you keep the others?
>  

Please resend the whole patchset.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file

[Greg KH]

On Fri, Dec 14, 2018 at 09:33:50AM +0000, Christian.Gromm@microchip.com wrote:
> On Fr, 2018-12-14 at 12:10 +0300, Dan Carpenter wrote:
> > On Fri, Dec 14, 2018 at 09:06:01AM +0000, Christian.Gromm@microchip.c
> > om wrote:
> > > 
> > > Message received. Anyway, I would like to resent the current
> > > patch set with the things Dan found fixed. And then looking
> > > into Configfs. Would this be ok for you guys?
> > > 
> > Sure.
> > 
> 
> I'm not sure what is the best way to resend this.
> 
> Can you skip the series and I'm going to send an entire
> new one. Or should I create a v2 of some of the patches
> of the series and only resend those and you keep the others?

Send a whole new v2 of this series, I'll drop the first one.

thanks,

greg k-h

Re: [PATCH 3/6] staging: most: sound: rename variable

[kbuild test robot]

Hi Christian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-Gromm/staging-most-sound-change-sound-card-layout/20181213-063731


coccinelle warnings: (new ones prefixed by >>)

>> drivers/staging/most/sound/sound.c:673:21-27: ERROR: application of sizeof to pointer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: most: sound: fix noderef.cocci warnings

[kbuild test robot]

From: kbuild test robot <fengguang.wu@intel.com>

drivers/staging/most/sound/sound.c:673:21-27: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: scripts/coccinelle/misc/noderef.cocci

Fixes: 58bce1efd0e8 ("staging: most: sound: rename variable")
CC: Christian Gromm <christian.gromm@microchip.com>
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Christian-Gromm/staging-most-sound-change-sound-card-layout/20181213-063731

 sound.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/staging/most/sound/sound.c
+++ b/drivers/staging/most/sound/sound.c
@@ -670,7 +670,7 @@ skip_adpt_alloc:
 		goto err_free_adpt;
 
 	pcm->private_data = channel;
-	snprintf(pcm->name, sizeof(device_name), device_name);
+	snprintf(pcm->name, sizeof(*device_name), device_name);
 	snd_pcm_set_ops(pcm, direction, &pcm_ops);
 
 	if (create) {
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel