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@xxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
