| Message ID | 20190725234032.21152-7-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
On 2019-07-26 01:39, Pierre-Louis Bossart wrote: > This should not happen in production systems but we should test for > all callback arguments before invoking the config_stream callback. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/intel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 68832e613b1e..497879dd9c0d 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw, > struct snd_soc_dai *dai, > struct snd_pcm_hw_params *hw_params, int link_id) > { > - if (sdw->res->ops && sdw->res->ops->config_stream) > + if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg) > return sdw->res->ops->config_stream(sdw->res->arg, > substream, dai, hw_params, link_id); > > Hmm, declaring local for sdw->res should prove useful here after addition of 4th sdw->res dereference.
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c >> index 68832e613b1e..497879dd9c0d 100644 >> --- a/drivers/soundwire/intel.c >> +++ b/drivers/soundwire/intel.c >> @@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw, >> struct snd_soc_dai *dai, >> struct snd_pcm_hw_params *hw_params, int link_id) >> { >> - if (sdw->res->ops && sdw->res->ops->config_stream) >> + if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg) >> return sdw->res->ops->config_stream(sdw->res->arg, >> substream, dai, hw_params, link_id); >> > > Hmm, declaring local for sdw->res should prove useful here after > addition of 4th sdw->res dereference. yes, it's an eyesore. I added this to quickly fix a kernel oops while debugging, will simplify. thanks for the note.
On 25-07-19, 18:39, Pierre-Louis Bossart wrote: > This should not happen in production systems but we should test for > all callback arguments before invoking the config_stream callback. so you are saying callback arg is mandatory, if so please document that assumption > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/intel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 68832e613b1e..497879dd9c0d 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw, > struct snd_soc_dai *dai, > struct snd_pcm_hw_params *hw_params, int link_id) > { > - if (sdw->res->ops && sdw->res->ops->config_stream) > + if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg) > return sdw->res->ops->config_stream(sdw->res->arg, > substream, dai, hw_params, link_id); > > -- > 2.20.1
On 8/2/19 6:55 AM, Vinod Koul wrote: > On 25-07-19, 18:39, Pierre-Louis Bossart wrote: >> This should not happen in production systems but we should test for >> all callback arguments before invoking the config_stream callback. > > so you are saying callback arg is mandatory, if so please document that > assumption no, what this says is that if a config_stream is provided then it needs to have a valid argument. I am not sure what you mean by "document that assumption", comment in the code (where?) or SoundWire documentation? > >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> drivers/soundwire/intel.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c >> index 68832e613b1e..497879dd9c0d 100644 >> --- a/drivers/soundwire/intel.c >> +++ b/drivers/soundwire/intel.c >> @@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw, >> struct snd_soc_dai *dai, >> struct snd_pcm_hw_params *hw_params, int link_id) >> { >> - if (sdw->res->ops && sdw->res->ops->config_stream) >> + if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg) >> return sdw->res->ops->config_stream(sdw->res->arg, >> substream, dai, hw_params, link_id); >> >> -- >> 2.20.1 >
On 02-08-19, 10:16, Pierre-Louis Bossart wrote: > > > On 8/2/19 6:55 AM, Vinod Koul wrote: > > On 25-07-19, 18:39, Pierre-Louis Bossart wrote: > > > This should not happen in production systems but we should test for > > > all callback arguments before invoking the config_stream callback. > > > > so you are saying callback arg is mandatory, if so please document that > > assumption > > no, what this says is that if a config_stream is provided then it needs to > have a valid argument. well typically args are not mandatory.. > I am not sure what you mean by "document that assumption", comment in the > code (where?) or SoundWire documentation? The callback documentation which in this is in include/linux/soundwire/sdw_intel.h
On 8/2/19 10:57 AM, Vinod Koul wrote: > On 02-08-19, 10:16, Pierre-Louis Bossart wrote: >> >> >> On 8/2/19 6:55 AM, Vinod Koul wrote: >>> On 25-07-19, 18:39, Pierre-Louis Bossart wrote: >>>> This should not happen in production systems but we should test for >>>> all callback arguments before invoking the config_stream callback. >>> >>> so you are saying callback arg is mandatory, if so please document that >>> assumption >> >> no, what this says is that if a config_stream is provided then it needs to >> have a valid argument. > > well typically args are not mandatory.. > >> I am not sure what you mean by "document that assumption", comment in the >> code (where?) or SoundWire documentation? > > The callback documentation which in this is in include/linux/soundwire/sdw_intel.h > /** * struct sdw_intel_ops: Intel audio driver callback ops * * @config_stream: configure the stream with the hw_params */ struct sdw_intel_ops { int (*config_stream)(void *arg, void *substream, void *dai, void *hw_params, int stream_num); }; all parameters are mandatory really, not sure what you are trying to get at.
On 02-08-19, 11:52, Pierre-Louis Bossart wrote: > > > On 8/2/19 10:57 AM, Vinod Koul wrote: > > On 02-08-19, 10:16, Pierre-Louis Bossart wrote: > > > > > > > > > On 8/2/19 6:55 AM, Vinod Koul wrote: > > > > On 25-07-19, 18:39, Pierre-Louis Bossart wrote: > > > > > This should not happen in production systems but we should test for > > > > > all callback arguments before invoking the config_stream callback. > > > > > > > > so you are saying callback arg is mandatory, if so please document that > > > > assumption > > > > > > no, what this says is that if a config_stream is provided then it needs to > > > have a valid argument. > > > > well typically args are not mandatory.. > > > > > I am not sure what you mean by "document that assumption", comment in the > > > code (where?) or SoundWire documentation? > > > > The callback documentation which in this is in include/linux/soundwire/sdw_intel.h > > > > /** > * struct sdw_intel_ops: Intel audio driver callback ops > * > * @config_stream: configure the stream with the hw_params > */ > struct sdw_intel_ops { > int (*config_stream)(void *arg, void *substream, > void *dai, void *hw_params, int stream_num); > }; > > all parameters are mandatory really, not sure what you are trying to get at. It would be good to make a note that argument is mandatory! Thanks
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 68832e613b1e..497879dd9c0d 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw, struct snd_soc_dai *dai, struct snd_pcm_hw_params *hw_params, int link_id) { - if (sdw->res->ops && sdw->res->ops->config_stream) + if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg) return sdw->res->ops->config_stream(sdw->res->arg, substream, dai, hw_params, link_id);
This should not happen in production systems but we should test for all callback arguments before invoking the config_stream callback. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/intel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)