linux-kernel - Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

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]

Date:   Mon, 20 Nov 2023 14:40:36 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Maciej Strozek <mstrozek@...nsource.cirrus.com>
Cc:     James Schulman <james.schulman@...rus.com>,
        David Rhodes <david.rhodes@...rus.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        alsa-devel@...a-project.org, patches@...nsource.cirrus.com,
        linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ
 connection

On Mon, Nov 20, 2023 at 02:17:34PM +0000, Maciej Strozek wrote:

> +	if (cs43130->has_irq_line) {
> +		ret = wait_for_completion_timeout(to_poll, msecs_to_jiffies(time));
> +	} else {

If you just put a return here then you don't need the else and can
reduce the intentation level of the rest of the function, making it more
legible.

> +		if (to_poll == &cs43130->xtal_rdy) {
> +			offset = 0;
> +			flag = CS43130_XTAL_RDY_INT;
> +		} else if (to_poll == &cs43130->pll_rdy) {
> +			offset = 0;
> +			flag = CS43130_PLL_RDY_INT;
> +		} else if (to_poll == &cs43130->hpload_evt) {
> +			offset = 3;
> +			flag = CS43130_HPLOAD_NO_DC_INT | CS43130_HPLOAD_UNPLUG_INT |
> +				CS43130_HPLOAD_OOR_INT | CS43130_HPLOAD_AC_INT |
> +				CS43130_HPLOAD_DC_INT | CS43130_HPLOAD_ON_INT |
> +				CS43130_HPLOAD_OFF_INT;
> +		} else {
> +			return 0;
> +		}

Is it a bug to call this function without to_poll set to something
known?  This will just silently ignore it which seems wrong and is
inconsitent with the handling in the interrupt case which will wait for
the the completion to be signalled and report a timeout on error.

> @@ -2545,8 +2579,10 @@ static int cs43130_i2c_probe(struct i2c_client *client)
>  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>  					"cs43130", cs43130);
>  	if (ret != 0) {
> -		dev_err(cs43130->dev, "Failed to request IRQ: %d\n", ret);
> -		goto err;
> +		dev_dbg(cs43130->dev, "Failed to request IRQ: %d, will poll instead\n", ret);
> +		cs43130->has_irq_line = 0;
> +	} else {
> +		cs43130->has_irq_line = 1;

This will treat probe deferral as the interrupt not being supplied, and
will squash even real errors silently - it should probably check for
both the specific error the clock API returns when no clock is provided
by the firmware and probe deferral and handle both specifically.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)