| Message ID | 20190906055524.7393-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2ec42f3147e1610716f184b02e65d7f493eed925 |
| Headers | show |
| Series | ASoC: dmaengine: Make the pcm->name equal to pcm->id if the name is not set | expand |
On Fri, 06 Sep 2019 07:55:24 +0200, Peter Ujfalusi wrote: > > Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for > other purposes. > > Currently it is left empty with the dmaengine-pcm, in this case copy the > pcm->id string as pcm->name. > > For example IGT is using this to find the HDMI PCM for testing audio on it. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > Reported-by: Arthur She <arthur.she@linaro.org> > --- > Hi, > > this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm > in v4.18), since then we only use the generic dmaengine PCM but the same issue > applies today. > > Regards, > Peter > > sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > index 748f5f641002..d93db2c2b527 100644 > --- a/sound/soc/soc-generic-dmaengine-pcm.c > +++ b/sound/soc/soc-generic-dmaengine-pcm.c > @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) > > if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) > pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; > + > + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { > + strncpy(rtd->pcm->streams[i].pcm->name, > + rtd->pcm->streams[i].pcm->id, > + sizeof(rtd->pcm->streams[i].pcm->name)); > + } Any reason to use strncpy() instead of strscpy()? After merging Mark's branch, I got a compile warning like: sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at offset [9223372036854775807, -9223372036854775808] [-Wrestrict] Takashi > > return 0; > -- > Peter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On 10/09/2019 15.07, Takashi Iwai wrote: > On Fri, 06 Sep 2019 07:55:24 +0200, > Peter Ujfalusi wrote: >> >> Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for >> other purposes. >> >> Currently it is left empty with the dmaengine-pcm, in this case copy the >> pcm->id string as pcm->name. >> >> For example IGT is using this to find the HDMI PCM for testing audio on it. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Reported-by: Arthur She <arthur.she@linaro.org> >> --- >> Hi, >> >> this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm >> in v4.18), since then we only use the generic dmaengine PCM but the same issue >> applies today. >> >> Regards, >> Peter >> >> sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c >> index 748f5f641002..d93db2c2b527 100644 >> --- a/sound/soc/soc-generic-dmaengine-pcm.c >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c >> @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) >> >> if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) >> pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; >> + >> + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { >> + strncpy(rtd->pcm->streams[i].pcm->name, >> + rtd->pcm->streams[i].pcm->id, >> + sizeof(rtd->pcm->streams[i].pcm->name)); >> + } > > Any reason to use strncpy() instead of strscpy()? > After merging Mark's branch, I got a compile warning like: > sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' > accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at > offset [9223372036854775807, -9223372036854775808] [-Wrestrict] I have not seen such a warning. 'may overlap up to 0 bytes' ? snd_pcm_info { ... unsigned char id[64]; /* ID (user selectable) */ unsigned char name[80]; /* name of this device */ unsigned char subname[32]; /* subdevice name */ ... }; and strncpy() supposed to be something like this: char * strncpy(char *dest, const char *src, size_t n) { size_t i; for (i = 0; i < n && src[i] != '\0'; i++) dest[i] = src[i]; for ( ; i < n; i++) dest[i] = '\0'; return dest; } I can see if I can get my compilers to show the warning and try strscpy() if it helps on it. > > > Takashi > >> >> return 0; >> -- >> Peter >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >> - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, 10 Sep 2019 16:23:59 +0200, Peter Ujfalusi wrote: > > > > On 10/09/2019 15.07, Takashi Iwai wrote: > > On Fri, 06 Sep 2019 07:55:24 +0200, > > Peter Ujfalusi wrote: > >> > >> Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for > >> other purposes. > >> > >> Currently it is left empty with the dmaengine-pcm, in this case copy the > >> pcm->id string as pcm->name. > >> > >> For example IGT is using this to find the HDMI PCM for testing audio on it. > >> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> Reported-by: Arthur She <arthur.she@linaro.org> > >> --- > >> Hi, > >> > >> this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm > >> in v4.18), since then we only use the generic dmaengine PCM but the same issue > >> applies today. > >> > >> Regards, > >> Peter > >> > >> sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > >> index 748f5f641002..d93db2c2b527 100644 > >> --- a/sound/soc/soc-generic-dmaengine-pcm.c > >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c > >> @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) > >> > >> if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) > >> pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; > >> + > >> + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { > >> + strncpy(rtd->pcm->streams[i].pcm->name, > >> + rtd->pcm->streams[i].pcm->id, > >> + sizeof(rtd->pcm->streams[i].pcm->name)); > >> + } > > > > Any reason to use strncpy() instead of strscpy()? > > After merging Mark's branch, I got a compile warning like: > > sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' > > accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at > > offset [9223372036854775807, -9223372036854775808] [-Wrestrict] > > I have not seen such a warning. > 'may overlap up to 0 bytes' ? > snd_pcm_info { > ... > unsigned char id[64]; /* ID (user selectable) */ > unsigned char name[80]; /* name of this device */ > unsigned char subname[32]; /* subdevice name */ > ... > }; > > and strncpy() supposed to be something like this: > char * strncpy(char *dest, const char *src, size_t n) > { > size_t i; > > for (i = 0; i < n && src[i] != '\0'; i++) > dest[i] = src[i]; > for ( ; i < n; i++) > dest[i] = '\0'; > > return dest; > } > > I can see if I can get my compilers to show the warning and try > strscpy() if it helps on it. strncpy() doesn't guarantee the string termination if you pass the exact buffer size. Better to use strscpy() in such a case. thanks, Takashi
On Tue, 10 Sep 2019 16:46:04 +0200, Takashi Iwai wrote: > > On Tue, 10 Sep 2019 16:23:59 +0200, > Peter Ujfalusi wrote: > > > > > > > > On 10/09/2019 15.07, Takashi Iwai wrote: > > > On Fri, 06 Sep 2019 07:55:24 +0200, > > > Peter Ujfalusi wrote: > > >> > > >> Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for > > >> other purposes. > > >> > > >> Currently it is left empty with the dmaengine-pcm, in this case copy the > > >> pcm->id string as pcm->name. > > >> > > >> For example IGT is using this to find the HDMI PCM for testing audio on it. > > >> > > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > >> Reported-by: Arthur She <arthur.she@linaro.org> > > >> --- > > >> Hi, > > >> > > >> this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm > > >> in v4.18), since then we only use the generic dmaengine PCM but the same issue > > >> applies today. > > >> > > >> Regards, > > >> Peter > > >> > > >> sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ > > >> 1 file changed, 6 insertions(+) > > >> > > >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > > >> index 748f5f641002..d93db2c2b527 100644 > > >> --- a/sound/soc/soc-generic-dmaengine-pcm.c > > >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c > > >> @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) > > >> > > >> if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) > > >> pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; > > >> + > > >> + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { > > >> + strncpy(rtd->pcm->streams[i].pcm->name, > > >> + rtd->pcm->streams[i].pcm->id, > > >> + sizeof(rtd->pcm->streams[i].pcm->name)); > > >> + } > > > > > > Any reason to use strncpy() instead of strscpy()? > > > After merging Mark's branch, I got a compile warning like: > > > sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' > > > accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at > > > offset [9223372036854775807, -9223372036854775808] [-Wrestrict] > > > > I have not seen such a warning. > > 'may overlap up to 0 bytes' ? > > snd_pcm_info { > > ... > > unsigned char id[64]; /* ID (user selectable) */ > > unsigned char name[80]; /* name of this device */ > > unsigned char subname[32]; /* subdevice name */ > > ... > > }; > > > > and strncpy() supposed to be something like this: > > char * strncpy(char *dest, const char *src, size_t n) > > { > > size_t i; > > > > for (i = 0; i < n && src[i] != '\0'; i++) > > dest[i] = src[i]; > > for ( ; i < n; i++) > > dest[i] = '\0'; > > > > return dest; > > } > > > > I can see if I can get my compilers to show the warning and try > > strscpy() if it helps on it. > > strncpy() doesn't guarantee the string termination if you pass the > exact buffer size. Better to use strscpy() in such a case. On the second thought, if pcm->id isn't NULL-terminated, it would result in OOB access. So, if strncpy() usage is just to guarantee for the zero-fill, that's OK, to ignore as a possibly spurious old gcc warning... Takashi
On 10/09/2019 15.07, Takashi Iwai wrote: > On Fri, 06 Sep 2019 07:55:24 +0200, > Peter Ujfalusi wrote: >> >> Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for >> other purposes. >> >> Currently it is left empty with the dmaengine-pcm, in this case copy the >> pcm->id string as pcm->name. >> >> For example IGT is using this to find the HDMI PCM for testing audio on it. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Reported-by: Arthur She <arthur.she@linaro.org> >> --- >> Hi, >> >> this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm >> in v4.18), since then we only use the generic dmaengine PCM but the same issue >> applies today. >> >> Regards, >> Peter >> >> sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c >> index 748f5f641002..d93db2c2b527 100644 >> --- a/sound/soc/soc-generic-dmaengine-pcm.c >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c >> @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) >> >> if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) >> pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; >> + >> + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { >> + strncpy(rtd->pcm->streams[i].pcm->name, >> + rtd->pcm->streams[i].pcm->id, >> + sizeof(rtd->pcm->streams[i].pcm->name)); >> + } > > Any reason to use strncpy() instead of strscpy()? > After merging Mark's branch, I got a compile warning like: > sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' > accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at > offset [9223372036854775807, -9223372036854775808] [-Wrestrict] fwiw I run it again with sparse and it only reports these: CHECK sound/soc/soc-generic-dmaengine-pcm.c sound/soc/soc-generic-dmaengine-pcm.c:177:42: warning: restricted snd_pcm_format_t degrades to integer sound/soc/soc-generic-dmaengine-pcm.c:177:47: warning: restricted snd_pcm_format_t degrades to integer sound/soc/soc-generic-dmaengine-pcm.c:177:71: warning: restricted snd_pcm_format_t degrades to integer CC sound/soc/soc-generic-dmaengine-pcm.o gcc 6.4.0...9.2.0 sparse 0.6.0 > > > Takashi > >> >> return 0; >> -- >> Peter >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >> - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Wed, 11 Sep 2019 09:22:53 +0200, Peter Ujfalusi wrote: > > > > On 10/09/2019 15.07, Takashi Iwai wrote: > > On Fri, 06 Sep 2019 07:55:24 +0200, > > Peter Ujfalusi wrote: > >> > >> Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for > >> other purposes. > >> > >> Currently it is left empty with the dmaengine-pcm, in this case copy the > >> pcm->id string as pcm->name. > >> > >> For example IGT is using this to find the HDMI PCM for testing audio on it. > >> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> Reported-by: Arthur She <arthur.she@linaro.org> > >> --- > >> Hi, > >> > >> this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm > >> in v4.18), since then we only use the generic dmaengine PCM but the same issue > >> applies today. > >> > >> Regards, > >> Peter > >> > >> sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c > >> index 748f5f641002..d93db2c2b527 100644 > >> --- a/sound/soc/soc-generic-dmaengine-pcm.c > >> +++ b/sound/soc/soc-generic-dmaengine-pcm.c > >> @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) > >> > >> if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) > >> pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; > >> + > >> + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { > >> + strncpy(rtd->pcm->streams[i].pcm->name, > >> + rtd->pcm->streams[i].pcm->id, > >> + sizeof(rtd->pcm->streams[i].pcm->name)); > >> + } > > > > Any reason to use strncpy() instead of strscpy()? > > After merging Mark's branch, I got a compile warning like: > > sound/soc/soc-generic-dmaengine-pcm.c:311:4: warning: 'strncpy' > > accessing 80 bytes at offsets 88 and 24 may overlap up to 0 bytes at > > offset [9223372036854775807, -9223372036854775808] [-Wrestrict] > > fwiw I run it again with sparse and it only reports these: > CHECK sound/soc/soc-generic-dmaengine-pcm.c > sound/soc/soc-generic-dmaengine-pcm.c:177:42: warning: restricted snd_pcm_format_t degrades to integer > sound/soc/soc-generic-dmaengine-pcm.c:177:47: warning: restricted snd_pcm_format_t degrades to integer > sound/soc/soc-generic-dmaengine-pcm.c:177:71: warning: restricted snd_pcm_format_t degrades to integer > CC sound/soc/soc-generic-dmaengine-pcm.o > > gcc 6.4.0...9.2.0 > sparse 0.6.0 That happens only with some cross-build on my build tests, so it must be a pretty minor issue. But in general, almost all strncpy() use case can be replaced gracefully with a safer function, either strscpy() or strscpy_pad(). IOW, if you see strncpy(), the patch looks doubtful :) thanks, Takashi > > > > > > > Takashi > > > >> > >> return 0; > >> -- > >> Peter > >> > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > >> > >> _______________________________________________ > >> Alsa-devel mailing list > >> Alsa-devel@alsa-project.org > >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > >> > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 748f5f641002..d93db2c2b527 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -306,6 +306,12 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) if (!dmaengine_pcm_can_report_residue(dev, pcm->chan[i])) pcm->flags |= SND_DMAENGINE_PCM_FLAG_NO_RESIDUE; + + if (rtd->pcm->streams[i].pcm->name[0] == '\0') { + strncpy(rtd->pcm->streams[i].pcm->name, + rtd->pcm->streams[i].pcm->id, + sizeof(rtd->pcm->streams[i].pcm->name)); + } } return 0;
Some tools use the snd_pcm_info_get_name() to try to identify PCMs or for other purposes. Currently it is left empty with the dmaengine-pcm, in this case copy the pcm->id string as pcm->name. For example IGT is using this to find the HDMI PCM for testing audio on it. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Reported-by: Arthur She <arthur.she@linaro.org> --- Hi, this was actually reported for 4.14 kernel with omap-pcm (replaced by sdma-pcm in v4.18), since then we only use the generic dmaengine PCM but the same issue applies today. Regards, Peter sound/soc/soc-generic-dmaengine-pcm.c | 6 ++++++ 1 file changed, 6 insertions(+)