| Message ID | 20210416071147.2149109-2-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 343e55e71877415a23372388b3e0c59a9bba42f6 |
| Headers | show |
| Series | [1/2] ASoC: simple-card-utils: Propagate errors on too many links | expand |
On 16/04/2021 08:11, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > On Tegra186 and later, the number of links can go up to 72, so bump the > maximum number of links to the next power of two (128). > > Fixes: f2138aed231c ("ASoC: simple-card-utils: enable flexible CPU/Codec/Platform") > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > include/sound/simple_card_utils.h | 4 +++- > sound/soc/generic/audio-graph-card.c | 4 ++-- > sound/soc/generic/simple-card.c | 4 ++-- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h > index fac3b832d982..e318a2d4ac44 100644 > --- a/include/sound/simple_card_utils.h > +++ b/include/sound/simple_card_utils.h > @@ -115,10 +115,12 @@ struct asoc_simple_priv { > ((codec) = simple_props_to_dai_codec(props, i)); \ > (i)++) > > +#define SNDRV_MAX_LINKS 128 > + > struct link_info { > int link; /* number of link */ > int cpu; /* turn for CPU / Codec */ > - struct prop_nums num[SNDRV_MINOR_DEVICES]; > + struct prop_nums num[SNDRV_MAX_LINKS]; > }; > > int asoc_simple_parse_daifmt(struct device *dev, > diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c > index 2401212281c2..d6d90285967c 100644 > --- a/sound/soc/generic/audio-graph-card.c > +++ b/sound/soc/generic/audio-graph-card.c > @@ -616,7 +616,7 @@ static int graph_count_noml(struct asoc_simple_priv *priv, > { > struct device *dev = simple_priv_to_dev(priv); > > - if (li->link >= SNDRV_MINOR_DEVICES) { > + if (li->link >= SNDRV_MAX_LINKS) { > dev_err(dev, "too many links\n"); > return -EINVAL; > } > @@ -639,7 +639,7 @@ static int graph_count_dpcm(struct asoc_simple_priv *priv, > { > struct device *dev = simple_priv_to_dev(priv); > > - if (li->link >= SNDRV_MINOR_DEVICES) { > + if (li->link >= SNDRV_MAX_LINKS) { > dev_err(dev, "too many links\n"); > return -EINVAL; > } > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > index 1d1c4309b582..59b41019c65f 100644 > --- a/sound/soc/generic/simple-card.c > +++ b/sound/soc/generic/simple-card.c > @@ -484,7 +484,7 @@ static int simple_count_noml(struct asoc_simple_priv *priv, > struct device_node *codec, > struct link_info *li, bool is_top) > { > - if (li->link >= SNDRV_MINOR_DEVICES) { > + if (li->link >= SNDRV_MAX_LINKS) { > struct device *dev = simple_priv_to_dev(priv); > > dev_err(dev, "too many links\n"); > @@ -505,7 +505,7 @@ static int simple_count_dpcm(struct asoc_simple_priv *priv, > struct device_node *codec, > struct link_info *li, bool is_top) > { > - if (li->link >= SNDRV_MINOR_DEVICES) { > + if (li->link >= SNDRV_MAX_LINKS) { > struct device *dev = simple_priv_to_dev(priv); > > dev_err(dev, "too many links\n"); > Reviewed-by: Jon Hunter <jonathanh@nvidia.com> Tested-by: Jon Hunter <jonathanh@nvidia.com> Thanks! Jon
Hi Thierry > From: Thierry Reding <treding@nvidia.com> > > On Tegra186 and later, the number of links can go up to 72, so bump the > maximum number of links to the next power of two (128). > > Fixes: f2138aed231c ("ASoC: simple-card-utils: enable flexible CPU/Codec/Platform") > Signed-off-by: Thierry Reding <treding@nvidia.com> (snip) > +#define SNDRV_MAX_LINKS 128 > + > struct link_info { > int link; /* number of link */ > int cpu; /* turn for CPU / Codec */ > - struct prop_nums num[SNDRV_MINOR_DEVICES]; > + struct prop_nums num[SNDRV_MAX_LINKS]; > }; How many numbers do you really need ? Because simple-card needs many parameters, thus I will get below warning. # It is not yet happen on upstream kernel, but will be warning: the frame size of 2280 bytes is larger than 2048 bytes [-Wframe-larger-than=] This warning doesn't appear if SNDRV_MAX_LINKS was 64. Can we reduce it ? Thank you for your help !! Best regards --- Kuninori Morimoto
On Mon, Apr 19, 2021 at 10:42:55AM +0900, Kuninori Morimoto wrote: > > Hi Thierry > > > From: Thierry Reding <treding@nvidia.com> > > > > On Tegra186 and later, the number of links can go up to 72, so bump the > > maximum number of links to the next power of two (128). > > > > Fixes: f2138aed231c ("ASoC: simple-card-utils: enable flexible CPU/Codec/Platform") > > Signed-off-by: Thierry Reding <treding@nvidia.com> > (snip) > > +#define SNDRV_MAX_LINKS 128 > > + > > struct link_info { > > int link; /* number of link */ > > int cpu; /* turn for CPU / Codec */ > > - struct prop_nums num[SNDRV_MINOR_DEVICES]; > > + struct prop_nums num[SNDRV_MAX_LINKS]; > > }; > > > How many numbers do you really need ? As I mentioned in the commit message, the maximum I've seen is 72. Rounding up to the next power of two seemed like a good idea to give a bit of extra headroom. > Because simple-card needs many parameters, > thus I will get below warning. > # It is not yet happen on upstream kernel, but will be > > warning: the frame size of 2280 bytes is larger than 2048 bytes [-Wframe-larger-than=] > > This warning doesn't appear if SNDRV_MAX_LINKS was 64. > Can we reduce it ? Reducing from 128 to, say, 80 should eliminate that warning, but I wonder if perhaps a better solution for the longer term would be to allocate this structure on the heap rather than on the stack? That way we don't risk triggering this warning again in the future. The data structure seems to be only used during initialization, so something like a kzalloc()/kfree() pair doesn't seem like it would hurt much performance-wise. Add in the devm_ variants and the code complexity should also remain moderately low. Thierry
On Mon, Apr 19, 2021 at 04:51:42PM +0200, Thierry Reding wrote: > Reducing from 128 to, say, 80 should eliminate that warning, but I > wonder if perhaps a better solution for the longer term would be to > allocate this structure on the heap rather than on the stack? That > way we don't risk triggering this warning again in the future. > The data structure seems to be only used during initialization, so > something like a kzalloc()/kfree() pair doesn't seem like it would > hurt much performance-wise. Add in the devm_ variants and the code > complexity should also remain moderately low. Yes, that'd be much better - ideally we wouldn't have a fixed limit of any kind but that's a more involved change and quite possibly more trouble than it's worth.
Hi Thierry, Mark > > The data structure seems to be only used during initialization, so > > something like a kzalloc()/kfree() pair doesn't seem like it would > > hurt much performance-wise. Add in the devm_ variants and the code > > complexity should also remain moderately low. > > Yes, that'd be much better - ideally we wouldn't have a fixed limit of > any kind but that's a more involved change and quite possibly more > trouble than it's worth. Using "fixed array style" was very easy for me (*^o^*). OK, let's use kzalloc()/kfree() style. I can try it. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index fac3b832d982..e318a2d4ac44 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -115,10 +115,12 @@ struct asoc_simple_priv { ((codec) = simple_props_to_dai_codec(props, i)); \ (i)++) +#define SNDRV_MAX_LINKS 128 + struct link_info { int link; /* number of link */ int cpu; /* turn for CPU / Codec */ - struct prop_nums num[SNDRV_MINOR_DEVICES]; + struct prop_nums num[SNDRV_MAX_LINKS]; }; int asoc_simple_parse_daifmt(struct device *dev, diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 2401212281c2..d6d90285967c 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -616,7 +616,7 @@ static int graph_count_noml(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { dev_err(dev, "too many links\n"); return -EINVAL; } @@ -639,7 +639,7 @@ static int graph_count_dpcm(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { dev_err(dev, "too many links\n"); return -EINVAL; } diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 1d1c4309b582..59b41019c65f 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -484,7 +484,7 @@ static int simple_count_noml(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { struct device *dev = simple_priv_to_dev(priv); dev_err(dev, "too many links\n"); @@ -505,7 +505,7 @@ static int simple_count_dpcm(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { struct device *dev = simple_priv_to_dev(priv); dev_err(dev, "too many links\n");