[v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

[PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

Posted by Baojun Xu 1 year ago

Driver will return error if no SUBSYS_ID found in BIOS(acpi).
It will cause error in tas2563 projects, which have no SUBSYS_ID.
Change strncmp to strcmp to avoid warning for weird length.

Signed-off-by: Baojun Xu <baojun.xu@ti.com>
---
 sound/pci/hda/tas2781_hda_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index 0e42b87dadb8..61aec92f6536 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -143,7 +143,7 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
 	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
 	if (IS_ERR(sub)) {
 		/* No subsys id in older tas2563 projects. */
-		if (!strncmp(hid, "INT8866", sizeof("INT8866")))
+		if (!strcmp(hid, "INT8866"))
 			goto end_2563;
 		dev_err(p->dev, "Failed to get SUBSYS ID.\n");
 		ret = PTR_ERR(sub);
-- 
2.43.0

Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

Posted by Takashi Iwai 1 year ago

On Sat, 11 Jan 2025 10:57:28 +0100,
Baojun Xu wrote:
> 
> Driver will return error if no SUBSYS_ID found in BIOS(acpi).
> It will cause error in tas2563 projects, which have no SUBSYS_ID.
> Change strncmp to strcmp to avoid warning for weird length.

I don't understand the logic.
The use of strncmp() there already matches only with the exact string
"INT8866", not the substring, because you use sizeof("INT8866") which
is 8 including the NUL-terminator.
The only merit of strncmp() in this case is that it can be used for an
unterminated char array.

In which situation do you see the problem and how does your patch
improve / fix it?  Please give a more concrete example.


thanks,

Takashi

> 
> Signed-off-by: Baojun Xu <baojun.xu@ti.com>
> ---
>  sound/pci/hda/tas2781_hda_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> index 0e42b87dadb8..61aec92f6536 100644
> --- a/sound/pci/hda/tas2781_hda_i2c.c
> +++ b/sound/pci/hda/tas2781_hda_i2c.c
> @@ -143,7 +143,7 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
>  	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
>  	if (IS_ERR(sub)) {
>  		/* No subsys id in older tas2563 projects. */
> -		if (!strncmp(hid, "INT8866", sizeof("INT8866")))
> +		if (!strcmp(hid, "INT8866"))
>  			goto end_2563;
>  		dev_err(p->dev, "Failed to get SUBSYS ID.\n");
>  		ret = PTR_ERR(sub);
> -- 
> 2.43.0
> 
>

Re: [EXTERNAL] Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

Posted by Xu, Baojun 1 year ago

> ________________________________________
> From: Takashi Iwai <tiwai@suse.de>
> Sent: 12 January 2025 16:26
> To: Xu, Baojun
> Cc: tiwai@suse.de; robh+dt@kernel.org; andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz; Ding, Shenghao; Navada Kanyana, Mukund; 13916275206@139.com; Hampiholi, Vallabha; P O, Vijeth; linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; liam.r.girdwood@intel.com; yung-chuan.liao@linux.intel.com; broonie@kernel.org; antheas.dk@gmail.com; stuart.a.hayhurst@gmail.com; dan.carpenter@linaro.org
> Subject: [EXTERNAL] Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects
> 
> On Sat, 11 Jan 2025 10: 57: 28 +0100, Baojun Xu wrote: > > Driver will return error if no SUBSYS_ID found in BIOS(acpi). > It will cause error in tas2563 projects, which have no SUBSYS_ID. > Change strncmp to strcmp to avoid warning
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source of this email and know the content is safe.
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!vxdrHfwF_qfzogYE_xtoX7lTozH70XOKA0HRu-ioBvUvCv8QNkmAsR1WurvR9Zlh7GmU7A$>
> Report Suspicious
> 
> ZjQcmQRYFpfptBannerEnd
> 
> On Sat, 11 Jan 2025 10:57:28 +0100,
> Baojun Xu wrote:
> >
> > Driver will return error if no SUBSYS_ID found in BIOS(acpi).
> > It will cause error in tas2563 projects, which have no SUBSYS_ID.
> > Change strncmp to strcmp to avoid warning for weird length.
> 
> I don't understand the logic.
> The use of strncmp() there already matches only with the exact string
> "INT8866", not the substring, because you use sizeof("INT8866") which
> is 8 including the NUL-terminator.
> The only merit of strncmp() in this case is that it can be used for an
> unterminated char array.
> 
> In which situation do you see the problem and how does your patch
> improve / fix it?  Please give a more concrete example.
> 

Because it will have Smatch Static Checker warning:

	sound/pci/hda/tas2781_hda_i2c.c:146 tas2781_read_acpi()
	warn: strncmp() with weird length: 7 vs 8

sound/pci/hda/tas2781_hda_i2c.c
    144         if (IS_ERR(sub)) {
    145                 /* No subsys id in older tas2563 projects. */
--> 146                 if (!strncmp(hid, "INT8866", sizeof("INT8866")))

This patch will avoid this warning, and this hid will be a null terminated
"INT8866", so have no different when this code was go through.

> 
> thanks,
> 
> Takashi
> 
> >
> > Signed-off-by: Baojun Xu <baojun.xu@ti.com>
> > ---
> >  sound/pci/hda/tas2781_hda_i2c.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> > index 0e42b87dadb8..61aec92f6536 100644
> > --- a/sound/pci/hda/tas2781_hda_i2c.c
> > +++ b/sound/pci/hda/tas2781_hda_i2c.c
> > @@ -143,7 +143,7 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
> >       sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
> >       if (IS_ERR(sub)) {
> >               /* No subsys id in older tas2563 projects. */
> > -             if (!strncmp(hid, "INT8866", sizeof("INT8866")))
> > +             if (!strcmp(hid, "INT8866"))
> >                       goto end_2563;
> >               dev_err(p->dev, "Failed to get SUBSYS ID.\n");
> >               ret = PTR_ERR(sub);
> > --
> > 2.43.0
> >
> >
> 
>

Re: [EXTERNAL] Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

Posted by Takashi Iwai 1 year ago

On Mon, 13 Jan 2025 07:41:02 +0100,
Xu, Baojun wrote:
> 
> > ________________________________________
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: 12 January 2025 16:26
> > To: Xu, Baojun
> > Cc: tiwai@suse.de; robh+dt@kernel.org; andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz; Ding, Shenghao; Navada Kanyana, Mukund; 13916275206@139.com; Hampiholi, Vallabha; P O, Vijeth; linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; liam.r.girdwood@intel.com; yung-chuan.liao@linux.intel.com; broonie@kernel.org; antheas.dk@gmail.com; stuart.a.hayhurst@gmail.com; dan.carpenter@linaro.org
> > Subject: [EXTERNAL] Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects
> > 
> > On Sat, 11 Jan 2025 10: 57: 28 +0100, Baojun Xu wrote: > > Driver will return error if no SUBSYS_ID found in BIOS(acpi). > It will cause error in tas2563 projects, which have no SUBSYS_ID. > Change strncmp to strcmp to avoid warning
> > ZjQcmQRYFpfptBannerStart
> > This message was sent from outside of Texas Instruments.
> > Do not click links or open attachments unless you recognize the source of this email and know the content is safe.
> > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!vxdrHfwF_qfzogYE_xtoX7lTozH70XOKA0HRu-ioBvUvCv8QNkmAsR1WurvR9Zlh7GmU7A$>
> > Report Suspicious
> > 
> > ZjQcmQRYFpfptBannerEnd
> > 
> > On Sat, 11 Jan 2025 10:57:28 +0100,
> > Baojun Xu wrote:
> > >
> > > Driver will return error if no SUBSYS_ID found in BIOS(acpi).
> > > It will cause error in tas2563 projects, which have no SUBSYS_ID.
> > > Change strncmp to strcmp to avoid warning for weird length.
> > 
> > I don't understand the logic.
> > The use of strncmp() there already matches only with the exact string
> > "INT8866", not the substring, because you use sizeof("INT8866") which
> > is 8 including the NUL-terminator.
> > The only merit of strncmp() in this case is that it can be used for an
> > unterminated char array.
> > 
> > In which situation do you see the problem and how does your patch
> > improve / fix it?  Please give a more concrete example.
> > 
> 
> Because it will have Smatch Static Checker warning:
> 
> 	sound/pci/hda/tas2781_hda_i2c.c:146 tas2781_read_acpi()
> 	warn: strncmp() with weird length: 7 vs 8
> 
> sound/pci/hda/tas2781_hda_i2c.c
>     144         if (IS_ERR(sub)) {
>     145                 /* No subsys id in older tas2563 projects. */
> --> 146                 if (!strncmp(hid, "INT8866", sizeof("INT8866")))
> 
> This patch will avoid this warning, and this hid will be a null terminated
> "INT8866", so have no different when this code was go through.

Ah, so your change is only about the last line in the sentence.
The former two lines describe what the current driver already does.

As the driver behavior won't change, your current patch subject and
description are way too confusing.  Please rephrase them and
resubmit.  It's nothing but just a code refactoring that eventually
shuts up the Smatch warning, after all.


thanks,

Takashi

Re: [PATCH v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects

Posted by Dan Carpenter 1 year ago

On Sun, Jan 12, 2025 at 09:26:37AM +0100, Takashi Iwai wrote:
> On Sat, 11 Jan 2025 10:57:28 +0100,
> Baojun Xu wrote:
> > 
> > Driver will return error if no SUBSYS_ID found in BIOS(acpi).
> > It will cause error in tas2563 projects, which have no SUBSYS_ID.
> > Change strncmp to strcmp to avoid warning for weird length.
> 
> I don't understand the logic.
> The use of strncmp() there already matches only with the exact string
> "INT8866", not the substring, because you use sizeof("INT8866") which
> is 8 including the NUL-terminator.
> The only merit of strncmp() in this case is that it can be used for an
> unterminated char array.
> 
> In which situation do you see the problem and how does your patch
> improve / fix it?  Please give a more concrete example.
> 

I was just confused by the initial code and wondered if it was
intended to be: if (!strncmp(hid, "INT8866", sizeof("INT8866") - 1)).
This change doesn't affect anything, it's just a clarification.

But the subject "Ignore SUBSYS_ID not found for tas2563 projects" implies
that it's a change.

regards,
dan carpenter