linux-kernel - Re: [PATCH] mmc: sdhci-of-dwcmshc: don't enable CQE without a suitable irq handler

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: <55cea03b-e685-4f7b-a93b-cb464417d364@intel.com>
Date: Mon, 3 Jun 2024 11:01:23 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Heiko Stuebner <heiko@...ech.de>, ulf.hansson@...aro.org
Cc: serghox@...il.com, linux-mmc@...r.kernel.org,
 linux-kernel@...r.kernel.org, quentin.schulz@...rry.de,
 Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH] mmc: sdhci-of-dwcmshc: don't enable CQE without a
 suitable irq handler

On 31/05/24 00:55, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@...rry.de>
> 
> supports-cqe is an established dt property so can appear in devicetrees
> at any time. CQE support in the sdhci-of-dwcmshc driver does require a
> special irq handler in the platform-specific ops, to handle the CQE
> interrupt.
> 
> Without this special handler we end up with a spew of unhandled interrupt
> messages on devices with supports-cqe property but without irq handler:
> 
> [   11.624143] mmc0: Unexpected interrupt 0x00004000.
> [   11.629504] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> [   11.636711] mmc0: sdhci: Sys addr:  0x00000008 | Version:  0x00000005
> [   11.643919] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000000
> [   11.651128] mmc0: sdhci: Argument:  0x00018000 | Trn mode: 0x00000033
> [   11.658336] mmc0: sdhci: Present:   0x13f700f0 | Host ctl: 0x00000034
> [   11.665545] mmc0: sdhci: Power:     0x00000001 | Blk gap:  0x00000000
> [   11.672753] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000407
> [   11.679961] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00004000
> [   11.687169] mmc0: sdhci: Int enab:  0x02ff4000 | Sig enab: 0x02ff4000
> [   11.694378] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [   11.701586] mmc0: sdhci: Caps:      0x226dc881 | Caps_1:   0x08000007
> [   11.708794] mmc0: sdhci: Cmd:       0x00000d1e | Max curr: 0x00000000
> [   11.716003] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0x00000000
> [   11.723211] mmc0: sdhci: Resp[2]:   0x328f5903 | Resp[3]:  0x000007cd
> [   11.730419] mmc0: sdhci: Host ctl2: 0x0000000f
> [   11.735392] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xee28f008
> [   11.742600] mmc0: sdhci: ============================================
> 
> So don't enable CQE if a usable interrupt handler is not defined and warn
> instead about this fact.
> 
> Fixes: 53ab7f7fe412 ("mmc: sdhci-of-dwcmshc: Implement SDHCI CQE support")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@...rry.de>
> ---
> My rk3588-tiger and rk3588-jaguar devicetrees had an accidential
> supports-cqe in their devicetree, which made me run into this problem
> with 6.10-rc1 .
> 
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 39edf04fedcf7..4410d4523728d 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -1254,10 +1254,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  
>  	/* Setup Command Queue Engine if enabled */
>  	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
> -		priv->vendor_specific_area2 =
> -			sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
> +		if (pltfm_data && pltfm_data->ops && pltfm_data->ops->irq) {

->irq() could be used for other things, so checking it for CQE
support is not appropriate.

If necessary, it would be better to flag which variants support
CQE in their platform data.

However that would probably mean introducing something
like struct dwcmshc_pltfm_data as described here:

https://lore.kernel.org/linux-mmc/ed900af1-f090-49a9-bc7e-363a28a4ac2b@intel.com/


> +			priv->vendor_specific_area2 =
> +				sdhci_readw(host, DWCMSHC_P_VENDOR_AREA2);
>  
> -		dwcmshc_cqhci_init(host, pdev);
> +			dwcmshc_cqhci_init(host, pdev);
> +		} else {
> +			dev_warn(&pdev->dev, "can't enable cqe support without irq handler\n");
> +		}
>  	}
>  
>  	if (rk_priv)