[PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[S.j. Wang]

There is a constraint for the channel number setting on the
asrc of older version (e.g. imx35), the channel number should
be even, odd number isn't valid.

So add protection when the asrc of older version is used.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 sound/soc/fsl/fsl_asrc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 528e8b108422..b3b3c5e15ef1 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
 	if (index == ASRC_INVALID_PAIR) {
 		dev_err(dev, "all pairs are busy now\n");
 		ret = -EBUSY;
-	} else if (asrc_priv->channel_avail < channels) {
+	} else if (asrc_priv->channel_avail < channels ||
+		(asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
 		dev_err(dev, "can't afford required channels: %d\n", channels);
 		ret = -EINVAL;
 	} else {
-- 
1.9.1

Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[Nicolin Chen]

Hi Shengjiu,

On Fri, Mar 01, 2019 at 02:32:38AM +0000, S.j. Wang wrote:
> There is a constraint for the channel number setting on the
> asrc of older version (e.g. imx35), the channel number should
> be even, odd number isn't valid.
> 
> So add protection when the asrc of older version is used.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  sound/soc/fsl/fsl_asrc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 528e8b108422..b3b3c5e15ef1 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
>  	if (index == ASRC_INVALID_PAIR) {
>  		dev_err(dev, "all pairs are busy now\n");
>  		ret = -EBUSY;
> -	} else if (asrc_priv->channel_avail < channels) {
> +	} else if (asrc_priv->channel_avail < channels ||
> +		(asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
>  		dev_err(dev, "can't afford required channels: %d\n", channels);

I feel it'd be better to have another else-if, since the existing
error message is against something else.

+	} else if (asrc_priv->channel_bits < 4 && channels & 1) {
+		/* old version of ASRC has channel_bits = 3 */
+  		dev_err(dev, "does not support odd channel number\n");
+  		ret = -EINVAL;

Alternatively, I feel instead of error-out at here, should we add
a HW constraint or at least fence it off at the beginning of the
hw_params()? This is actually nothing specific to the pair-request
function but a hardware constraint.

RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[S.j. Wang]

Hi
> 
> Hi Shengjiu,
> 
> On Fri, Mar 01, 2019 at 02:32:38AM +0000, S.j. Wang wrote:
> > There is a constraint for the channel number setting on the asrc of
> > older version (e.g. imx35), the channel number should be even, odd
> > number isn't valid.
> >
> > So add protection when the asrc of older version is used.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  sound/soc/fsl/fsl_asrc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 528e8b108422..b3b3c5e15ef1 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -109,7 +109,8 @@ static int fsl_asrc_request_pair(int channels, struct
> fsl_asrc_pair *pair)
> >  	if (index == ASRC_INVALID_PAIR) {
> >  		dev_err(dev, "all pairs are busy now\n");
> >  		ret = -EBUSY;
> > -	} else if (asrc_priv->channel_avail < channels) {
> > +	} else if (asrc_priv->channel_avail < channels ||
> > +		(asrc_priv->channel_bits < 4 && channels % 2 != 0)) {
> >  		dev_err(dev, "can't afford required channels: %d\n",
> channels);
> 
> I feel it'd be better to have another else-if, since the existing error message
> is against something else.
> 
> +	} else if (asrc_priv->channel_bits < 4 && channels & 1) {
> +		/* old version of ASRC has channel_bits = 3 */
> +  		dev_err(dev, "does not support odd channel number\n");
> +  		ret = -EINVAL;
> 
> Alternatively, I feel instead of error-out at here, should we add a HW
> constraint or at least fence it off at the beginning of the hw_params()? This
> is actually nothing specific to the pair-request function but a hardware
> constraint.

How about add constraint in startup?
static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
                               struct snd_soc_dai *dai)
{
       struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);

       if (asrc_priv->channel_bits == 3) {
               snd_pcm_hw_constraint_step(substream->runtime, 0,
                                          SNDRV_PCM_HW_PARAM_CHANNELS, 2);
       }

       return 0;
}

Best regards
Wang shengjiu

Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[Nicolin Chen]

On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:

> > Alternatively, I feel instead of error-out at here, should we add a HW
> > constraint or at least fence it off at the beginning of the hw_params()? This
> > is actually nothing specific to the pair-request function but a hardware
> > constraint.
> 
> How about add constraint in startup?
> static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
>                                struct snd_soc_dai *dai)
> {
>        struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> 
>        if (asrc_priv->channel_bits == 3) {
>                snd_pcm_hw_constraint_step(substream->runtime, 0,
>                                           SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>        }
> 
>        return 0;
> }

Yea, that looks good to me. Better to have a line of comments
to tell that "bits==3" means older version -- maybe we should
have something much more clear than using channel_bits but it
is fine for now since they only differ here.

RE: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[S.j. Wang]

Hi

> 
> On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:
> 
> > > Alternatively, I feel instead of error-out at here, should we add a
> > > HW constraint or at least fence it off at the beginning of the
> > > hw_params()? This is actually nothing specific to the pair-request
> > > function but a hardware constraint.
> >
> > How about add constraint in startup?
> > static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
> >                                struct snd_soc_dai *dai) {
> >        struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> >
> >        if (asrc_priv->channel_bits == 3) {
> >                snd_pcm_hw_constraint_step(substream->runtime, 0,
> >                                           SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> >        }
> >
> >        return 0;
> > }
> 
> Yea, that looks good to me. Better to have a line of comments to tell that
> "bits==3" means older version -- maybe we should have something much
> more clear than using channel_bits but it is fine for now since they only
> differ here.

Ok , will send v2.

Re: [PATCH] ASoC: fsl_asrc: add protection for the asrc of older version

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Fri, Mar 01, 2019 at 06:55:25AM +0000, S.j. Wang wrote:

> > Alternatively, I feel instead of error-out at here, should we add a HW
> > constraint or at least fence it off at the beginning of the hw_params()? This
> > is actually nothing specific to the pair-request function but a hardware
> > constraint.

> How about add constraint in startup?
> static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
>                                struct snd_soc_dai *dai)
> {
>        struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> 
>        if (asrc_priv->channel_bits == 3) {
>                snd_pcm_hw_constraint_step(substream->runtime, 0,
>                                           SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>        }
> 
>        return 0;
> }

Yes, that's definitely good - the general idea is that we should never
need to return an error from hw_params() as the constraints code will
have filtered out any invalid configurations before they get that far.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]