linux-kernel - Re: [PATCH v2 1/2] ALSA: hda: cs35l41: Safety-guard against capped SPI speed

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: <8170413d-07a8-4e77-b43d-78cd9e4ea76f@opensource.cirrus.com>
Date: Wed, 20 Dec 2023 13:43:55 +0000
From: Stefan Binding <sbinding@...nsource.cirrus.com>
To: Aleksandrs Vinarskis <alex.vinarskis@...il.com>, <tiwai@...e.de>
CC: <alsa-devel@...a-project.org>, <david.rhodes@...rus.com>,
        <james.schulman@...rus.com>, <josbeir@...il.com>,
        <linux-kernel@...r.kernel.org>, <patches@...nsource.cirrus.com>,
        <perex@...ex.cz>, <stuarth@...nsource.cirrus.com>, <tiwai@...e.com>
Subject: Re: [PATCH v2 1/2] ALSA: hda: cs35l41: Safety-guard against capped
 SPI speed

Hi,

On 20/12/2023 07:38, Aleksandrs Vinarskis wrote:
> Some devices with intel-lpss based SPI controllers may have misconfigured
> clock divider due to firmware bug. This would result in capped SPI speeds,
> which leads to longer DSP firmware loading times.
> This safety guards against possible hangs during wake-up by not
> initializing the device if lpss was not patched/fixed UEFI was not
> installed
>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
> ---
>   sound/pci/hda/cs35l41_hda_property.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
> index c9eb70290973..cb305b093311 100644
> --- a/sound/pci/hda/cs35l41_hda_property.c
> +++ b/sound/pci/hda/cs35l41_hda_property.c
> @@ -210,6 +210,19 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
>   
>   	if (cfg->bus == SPI) {
>   		cs35l41->index = id;
> +		/*
> +		 * Some devices with intel-lpss based SPI controllers may have misconfigured
> +		 * clock divider due to firmware bug. This would result in capped SPI speeds,
> +		 * which leads to longer DSP firmware loading times.
> +		 * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
> +		 */
> +		spi = to_spi_device(cs35l41->dev);
> +		if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ/2) {
> +			dev_err(cs35l41->dev,
> +				"SPI's max_speed_hz is capped at %u Hz, will not continue to avoid hanging\n",
> +				spi->max_speed_hz);
> +			return -EINVAL;
> +		}

Not sure I agree with completely disabling the CS35L41 Speaker Driver if 
the SPI speed is low (for laptops without _DSD).
With a slow speed the driver does not hang - it just takes a long time 
(~80s per amp) to load the firmware.
Instead I would prefer that we instead disable the loading of the 
firmware in this case.
Without loading firmware, the volume is much lower, but at least you 
still have audio.
I have a patch to do that, which I was planning on pushing up 
(hopefully) today.

Thanks,

Stefan

>   		/*
>   		 * Manually set the Chip Select for the second amp <cs_gpio_index> in the node.
>   		 * This is only supported for systems with 2 amps, since we cannot expand the
> @@ -219,8 +232,6 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
>   		 * first.
>   		 */
>   		if (cfg->cs_gpio_index >= 0) {
> -			spi = to_spi_device(cs35l41->dev);
> -
>   			if (cfg->num_amps != 2) {
>   				dev_warn(cs35l41->dev,
>   					 "Cannot update SPI CS, Number of Amps (%d) != 2\n",