| 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
| ||
|
Message-ID: <87il96cfnb.wl-tiwai@suse.de>
Date: Wed, 23 Aug 2023 10:43:52 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Luke Jones <luke@...nes.dev>
Cc: Takashi Iwai <tiwai@...e.de>, tiwai@...e.com,
james.schulman@...rus.com, david.rhodes@...rus.com,
rf@...nsource.cirrus.com, linux-kernel@...r.kernel.org,
sbinding@...nsource.cirrus.com, Jonathan LoBue <jlobue10@...il.com>
Subject: Re: [PATCH] ALSA: hda: cs35l41: Support ASUS 2023 laptops with missing DSD
On Wed, 23 Aug 2023 10:02:11 +0200,
Luke Jones wrote:
>
>
>
> On Wed, Aug 23 2023 at 09:37:08 +02:00:00, Takashi Iwai
> <tiwai@...e.de> wrote:
> > On Wed, 23 Aug 2023 09:28:39 +0200,
> > Luke Jones wrote:
> >>
> >>
> >>
> >> On Wed, Aug 23 2023 at 08:24:51 +02:00:00, Takashi Iwai
> >> <tiwai@...e.de> wrote:
> >> > On Wed, 23 Aug 2023 03:10:08 +0200,
> >> > Luke D. Jones wrote:
> >> >>
> >> >> Support adding the missing DSD properties required for ASUS ROG
> >> >> 2023
> >> >> laptops and other ASUS laptops to properly utilise the cs35l41.
> >> >>
> >> >> This support includes both I2C and SPI connected amps.
> >> >>
> >> >> The SPI connected amps may be required to use an external DSD
> >> patch
> >> >> to fix or add the "cs-gpios" property.
> >> >>
> >> >> Co-developed-by: Jonathan LoBue <jlobue10@...il.com>
> >> >> Signed-off-by: Jonathan LoBue <jlobue10@...il.com>
> >> >> Co-developed-by: Luke D. Jones <luke@...nes.dev>
> >> >> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> >> >> ---
> >> >> sound/pci/hda/cs35l41_hda_property.c | 26
> >> >> ++++++++++++++++++++++++++
> >> >> 1 file changed, 26 insertions(+)
> >> >>
> >> >> diff --git a/sound/pci/hda/cs35l41_hda_property.c
> >> >> b/sound/pci/hda/cs35l41_hda_property.c
> >> >> index 673f23257a09..69879ab57918 100644
> >> >> --- a/sound/pci/hda/cs35l41_hda_property.c
> >> >> +++ b/sound/pci/hda/cs35l41_hda_property.c
> >> >> @@ -43,6 +43,31 @@ static int lenovo_legion_no_acpi(struct
> >> >> cs35l41_hda *cs35l41, struct device *phy
> >> >> return 0;
> >> >> }
> >> >>
> >> >> +/*
> >> >> + * The CSC3551 is used in almost the entire ASUS ROG laptop
> >> range
> >> >> in 2023, this is likely to
> >> >> + * also include many non ROG labelled laptops. It is also used
> >> >> with either I2C connection or
> >> >> + * SPI connection. The SPI connected versions may be missing a
> >> >> chip select GPIO and require
> >> >> + * an DSD table patch.
> >> >> + */
> >> >> +static int asus_rog_2023_no_acpi(struct cs35l41_hda *cs35l41,
> >> >> struct device *physdev, int id,
> >> >> + const char *hid)
> >> >> +{
> >> >> + struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
> >> >> +
> >> >> + /* check SPI or I2C address to assign the index */
> >> >> + cs35l41->index = (id == 0 || id == 0x40) ? 0 : 1;
> >> >> + cs35l41->channel_index = 0;
> >> >> + cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0,
> >> >> GPIOD_OUT_HIGH);
> >> >> + cs35l41->speaker_id = cs35l41_get_speaker_id(physdev,
> >> 0, 0, 2);
> >> >> + hw_cfg->spk_pos = cs35l41->index;
> >> >> + hw_cfg->gpio2.func = CS35L41_INTERRUPT;
> >> >> + hw_cfg->gpio2.valid = true;
> >> >> + hw_cfg->bst_type = CS35L41_EXT_BOOST_NO_VSPK_SWITCH;
> >> >> + hw_cfg->valid = true;
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> struct cs35l41_prop_model {
> >> >> const char *hid;
> >> >> const char *ssid;
> >> >> @@ -53,6 +78,7 @@ struct cs35l41_prop_model {
> >> >> const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
> >> >> { "CLSA0100", NULL, lenovo_legion_no_acpi },
> >> >> { "CLSA0101", NULL, lenovo_legion_no_acpi },
> >> >> + { "CSC3551", NULL, asus_rog_2023_no_acpi },
> >> >
> >> > I believe this breaks things badly.
> >> cs35l41_add_dsd_properties() is
> >> > always called no matter whether _DSD is found or not. So this
> >> will
> >> > override the setup of all machines with CSC3551 even if it has a
> >> > proper _DSD.
> >>
> >> These are the entries I know of so far since they definitely had
> >> to be
> >> added and have a DSD patch:
> >>
> >> - SPI_2
> >> - 0x1043, 0x1473, "ASUS GU604V"
> >> - 0x1043, 0x1483, "ASUS GU603V"
> >> - 0x1043, 0x1493, "ASUS GV601V"
> >> - 0x1043, 0x1573, "ASUS GZ301V"
> >> - 0x1043, 0x1c9f, "ASUS G614JI"
> >> - SPI_4
> >> - 0x1043, 0x1caf, "ASUS G634JYR/JZR"
> >> - I2C_2
> >> - 0x1043, 0x1d1f, "ASUS ROG Strix G17
> >> - 0x1043, 0x1463, "Asus GA402X"
> >> - 0x1043, 0x1433, "ASUS GX650P"
> >> - ROG ALLY
> >>
> >> You can see the variants are V, J, X, and P. A grep through the DSL
> >> dumps I have collected reveals that these machines are all indeed
> >> missing DSD entries. These are a mix of I2C and SPI.
> >>
> >> The patch I submitted was based on Stefan's work only, and tested
> >> on 3
> >> SPI machines plus 2 I2C machines with no issues except the SPI
> >> machines needing a chipselect DSD patch.
> >>
> >> It's worth stating that the DSD patches people were using all
> >> followed
> >> the exact same template except for the SPI number, or speaker ID.
> >>
> >> I'd wager it being a safe bet to assume that every one of the ASUS
> >> laptops this year using the CSC3551 will be missing the required DSD
> >> entries given the trend so far.
> >
> > Yes, I know that there are tons of such devices that need the
> > workaround for missing _DSD. My point is, however, that your patch
> > breaks the devices that *do have* _DSD, because the function always
> > overrides.
>
> Ah, because CSC3551 is not unique at all. I see.
>
> >
> >> > The existing entries of CLSA0100 and CLSA0101 are OK since
> >> > (supposedly) those never had _DSD. The current code is a bit
> >> > misleading as if it's applicable easily, though.
> >> >
> >> > That said, we have to apply the setup only conditionally for each
> >> > specific device. One easy thing would be to move the function
> >> call
> >> > after _DSD check. But, I *guess* that Stefan applied the
> >> function at
> >> > the top so that it may cover the all cases including incorrect
> >> _DSD
> >> > properties.
> >>
> >> Given the trend of what I've seen this seems like a reasonable
> >> assumption and desired.
> >
> > ... and it's not enough. That's another point. The parameters aren't
> > always same for all devices but may vary. That is, it must have some
> > check of devices and models to identify which parameters may be used.
> >
> >> > And, the question is how to be specific to each device. This
> >> can be
> >> > messy, as the sub-codec driver is probed independently from
> >> Realtek
> >> > codec driver, hence you can't pass any flag from Realtek to CS
> >> driver
> >> > at the probe time. In the end, we might need to keep another
> >> table of
> >> > IDs (either the same SSID or DMI) to distinguish which machine
> >> needs
> >> > which properties.
> >>
> >> Is there some other ID we can use? I see:
> >> [ 13.569242] cs35l41-hda spi0-CSC3551:00-cs35l41-hda.1: Cirrus
> >> Logic
> >> CS35L41 (35a40), Revision: B2
> >> and I'd assume that 35a40 is unique? A bit of searching on my
> >> discord
> >> reveals that all the machines I listed that require the same patch
> >> also have this identifier (including a ProArt laptop).
> >
> > That's not unique to each *device* (not the chip model). Say, we want
> > to distinguish ASUS GU604V and ASUS G64JYR. They may have different
> > setups.
> >
> > I guess only DMI is suitable at the time of probing this driver.
>
> Oh, I'm sorry I should have paid more attention the first time. I can
> match against acpi_subsystem_id which would be for example 10431caf
> yes?
I believe many other drivers match with vendor name, product name, or
whatever strings.
> I'll begin working on a patch series for each.
Thanks!
Takashi
Powered by blists - more mailing lists