Re: [PATCH v2 10/14] staging: most: allow speculative configuration

[<Christian.Gromm@microchip.com>]

On Do, 2019-03-28 at 17:12 +0300, Dan Carpenter wrote:
> External E-Mail
> 
> 
> On Thu, Mar 28, 2019 at 02:17:38PM +0100, Christian Gromm wrote:
> > 
> > This patch makes the driver accept a link confiiguration eventhough
> > no
> > device is attached to the bus. Instead the configuration is being
> > applied
> > as soon as a device is being registered with the core.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > ---
> > v2:
> > 	- follow-up adaptions due to changes introduced w/ [PATCH v2
> > 01/14]
> > 
> >  drivers/staging/most/configfs.c    | 60
> > ++++++++++++++++++++++++++++----------
> >  drivers/staging/most/core.c        | 16 +++-------
> >  drivers/staging/most/core.h        |  1 +
> >  drivers/staging/most/sound/sound.c |  6 ++--
> >  4 files changed, 53 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/staging/most/configfs.c
> > b/drivers/staging/most/configfs.c
> > index 6968b299..3a7c54d 100644
> > --- a/drivers/staging/most/configfs.c
> > +++ b/drivers/staging/most/configfs.c
> > @@ -14,6 +14,7 @@
> >  
> >  struct mdev_link {
> >  	struct config_item item;
> > +	struct list_head list;
> >  	bool create;
> >  	u16 num_buffers;
> >  	u16 buffer_size;
> > @@ -29,6 +30,8 @@ struct mdev_link {
> >  	char param[PAGE_SIZE];
> >  };
> >  
> > +struct list_head mdev_link_list;
> > +
> >  static int set_cfg_buffer_size(struct mdev_link *link)
> >  {
> >  	if (!link->buffer_size)
> > @@ -105,33 +108,41 @@ static ssize_t mdev_link_create_show(struct
> > config_item *item, char *page)
> >  	return snprintf(page, PAGE_SIZE, "%d\n",
> > to_mdev_link(item)->create);
> >  }
> >  
> > +static int set_config_and_add_link(struct mdev_link *mdev_link)
> > +{
> > +	int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> > +		ret = set_config_val[i](mdev_link);
> > +		if (ret == -ENODATA) {
> I've read the commit description but I don't really understand the
> error codes.  Here only -ENODATA is an error.  But later -ENODEV
> is success.  Why not "if (ret) {" here?
> 

The most_set_cfg_* functions return success, ENODEV or ENODATA.
We want to stop only in case there is something wrong with the
provided data. A missing device is not an issue here. In this
case we want to keep the configuration as is and complete stuff
once a device is being attached.
 
> 
> > 
> > +			pr_err("Config failed\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return most_add_link(mdev_link->mdev, mdev_link->channel,
> > +			     mdev_link->comp, mdev_link->name,
> > +			     mdev_link->param);
> > +}
> > +
> >  static ssize_t mdev_link_create_store(struct config_item *item,
> >  				      const char *page, size_t
> > count)
> >  {
> >  	struct mdev_link *mdev_link = to_mdev_link(item);
> >  	bool tmp;
> >  	int ret;
> > -	int i;
> >  
> >  	ret = kstrtobool(page, &tmp);
> >  	if (ret)
> >  		return ret;
> > -
> > -	for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> > -		ret = set_config_val[i](mdev_link);
> > -		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> > -	if (tmp)
> > -		ret = most_add_link(mdev_link->mdev, mdev_link-
> > >channel,
> > -				    mdev_link->comp, mdev_link-
> > >name,
> > -				    mdev_link->param);
> > -	else
> > -		ret = most_remove_link(mdev_link->mdev, mdev_link-
> > >channel,
> > -				       mdev_link->comp);
> > -	if (ret)
> > +	if (!tmp)
> > +		return most_remove_link(mdev_link->mdev,
> > mdev_link->channel,
> > +					mdev_link->comp);
> > +	ret = set_config_and_add_link(mdev_link);
> > +	if (ret && ret != -ENODEV)
> ENODEV is success.  I feel like this needs to be explained in
> comments
> in the code.

ENODEV is not a show-stopper. It only postpones the configuration
process until the driver is being notified about a new device.

> 
> > 
> >  		return ret;
> > +	list_add_tail(&mdev_link->list, &mdev_link_list);
> >  	mdev_link->create = tmp;
> >  	return count;
> >  }
> > @@ -590,6 +601,22 @@ int most_register_configfs_subsys(struct
> > core_component *c)
> >  }
> >  EXPORT_SYMBOL_GPL(most_register_configfs_subsys);
> >  
> > +void most_interface_register_notify(const char *mdev)
> > +{
> > +	bool register_snd_card = false;
> > +	struct mdev_link *mdev_link;
> > +
> > +	list_for_each_entry(mdev_link, &mdev_link_list, list) {
> > +		if (!strcmp(mdev_link->mdev, mdev)) {
> > +			set_config_and_add_link(mdev_link);
> Here the errors are not checked.

We are in the notify function. That means a device is present
now. And if the list isn't empty, there is also a valid configuration
available. So, set_config_and_add_link should not fail.

> 
> > 
> > +			if (!strcmp(mdev_link->comp, "sound"))
> > +				register_snd_card = true;
> > +		}
> > +	}
> > +	if (register_snd_card)
> > +		most_cfg_complete("sound");
> > +}
> > +
> [ snip ]
> 
> > 
> > diff --git a/drivers/staging/most/sound/sound.c
> > b/drivers/staging/most/sound/sound.c
> > index fd089e6..44c9146 100644
> > --- a/drivers/staging/most/sound/sound.c
> > +++ b/drivers/staging/most/sound/sound.c
> > @@ -20,6 +20,7 @@
> >  #include <most/core.h>
> >  
> >  #define DRIVER_NAME "sound"
> > +#define STRING_SIZE	80
> >  
> >  static struct core_component comp;
> >  
> > @@ -582,6 +583,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  	int direction;
> >  	u16 ch_num;
> >  	char *sample_res;
> > +	char arg_list_cpy[STRING_SIZE];
> >  
> >  	if (!iface)
> >  		return -EINVAL;
> > @@ -590,8 +592,8 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >  		pr_err("Incompatible channel type\n");
> >  		return -EINVAL;
> >  	}
> > -
> > -	ret = split_arg_list(arg_list, &ch_num, &sample_res);
> > +	strlcpy(arg_list_cpy, arg_list, STRING_SIZE);
> > +	ret = split_arg_list(arg_list_cpy, &ch_num, &sample_res);
> 
> I don't understand why we need a copy of arg_list or how this relates
> to the rest of the patch.

The function split_arg_list modifies the argument. So if we want to
read it back later in a show function, we need to have a clean copy.

> 
> > 
> >  	if (ret < 0)
> >  		return ret;
> >  
> regards,
> dan carpenter
> 
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel