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

[Adrian Hunter]

On 3/06/24 14:26, Heiko Stübner wrote:
> Am Montag, 3. Juni 2024, 10:01:23 CEST schrieb Adrian Hunter:
>> On 31/05/24 00:55, Heiko Stuebner wrote:
>>> From: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>> 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.
> 
> though here we are in the very limited scope of only the dwcmshc sdhci
> controller.

I am afraid that does not justify it.  It can still create
problems in the future if that very limited scope changes
i.e. someone wants to use the ->irq() callback in
sdhci-of-dwcmshc.c for something else.

> 
> And at this point, any controller using the generic sdhci_dwcmshc_ops
> will always get the CQE irq handler, while _all other_ controllers
> will need to define one to handle the CQE irqs.
> 
> So any variant trying to enable CQE needs to define an irq handler, hence
> the check simply was meant to not allow CQE enablement without any irq
> handler, because that will always cause those unhandled irq issues.
> 
>> If necessary, it would be better to flag which variants support
>> CQE in their platform data.
> 
> I guess we can assume that all of the dwcmshc IP variants support
> CQE, it's just that the implementation is slightly strange in that
> a DT can enable CQE support and this will cause the driver to enable the
> CQE interrupt, even if nothing is there to handle it.

Isn't the problem that the IP may support CQE but the driver does
not for the given variant, but switches it on anyway.

> 
> 
> For my own case I can also just live with the Rockchip irq handler
> patch going in (ideally as a fix) and I just wanted to try to fix this
> will definitly fail if irq-handler not present case.
> 
> 
> Heiko
> 
> 
>> 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@xxxxxxxxx/
>>
>>
>>> +			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)
>>
>>
> 
> 
> 
>