On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@xxxxxxxxxxxxx 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@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.
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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
