linux-kernel - Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZN+j3LmfUW2vB+QS@smile.fi.intel.com>
Date:   Fri, 18 Aug 2023 20:01:16 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Shenghao Ding <shenghao-ding@...com>, tiwai@...e.de,
        robh+dt@...nel.org, lgirdwood@...il.com, perex@...ex.cz,
        kevin-lu@...com, 13916275206@....com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, liam.r.girdwood@...el.com,
        mengdong.lin@...el.com, baojun.xu@...com,
        thomas.gfeller@...rop.com, peeyush@...com, navada@...com,
        broonie@...nel.org, gentuser@...il.com
Subject: Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:

...

> > +static int comp_match_tas2781_dev_name(struct device *dev,
> > +	void *data)
> > +{
> > +	struct scodec_dev_name *p = data;
> > +	const char *d = dev_name(dev);
> > +	int n = strlen(p->bus);
> > +	char tmp[32];
> > +
> > +	/* check the bus name */
> > +	if (strncmp(d, p->bus, n))
> > +		return 0;

> > +	/* skip the bus number */
> > +	if (isdigit(d[n]))
> > +		n++;

Why do you think it can't be two or more digits?

> > +	/* the rest must be exact matching */
> > +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> 
> ACPI can sometimes add :01 suffixes, this looks like the re-invention of
> an ACPI helper?
> 
> Adding Andy for the ACPI review.
> 
> > +	return !strcmp(d + n, tmp);
> > +}

Yes, this looks like reinventing a wheel.
Just compare dev_name() against what is in p->....

...

> > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > +	const struct hda_fixup *fix, int action)
> > +{
> > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> 
> TI ACPI ID is TXNW
> 
> https://uefi.org/ACPI_ID_List?search=TEXAS
> 
> There's also a PNP ID PXN
> 
> https://uefi.org/PNP_ID_List?search=TEXAS
> 
> "TIAS" looks like an invented identifier. It's not uncommon but should
> be recorded with a comment if I am not mistaken.
> 
> > +}

Thank you, but actually it's a strong NAK to this even with the comment.
We have to teach people to follow the specification (may be even hard way).

So where did you get the ill-formed ACPI ID?
Is Texas Instrument aware of this?
Can we have a confirmation letter from TI for this ID, please?

-- 
With Best Regards,
Andy Shevchenko