| Message ID | 20250111095728.1232-1-baojun.xu@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] ALSA: hda/tas2781: Ignore SUBSYS_ID not found for tas2563 projects | expand |
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 > >
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
> ________________________________________ > 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 > > > > > >
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
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);
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(-)