(RFC) sdhci_generic_intr: only handle bits that are enabled in slot->intmask

[kevans]

Summary

sdhci drivers may have internal handling (e.g. for external DMA) for transfers. In these cases, they may turn off interrupts for SPACE_AVAIL | DATA_AVAIL | DATA_END while keeping them enabled in SDHCI_INT_ENABLE so that they can still determine and handle controller status in a non-SDHCI interrupt. The intention is that sdhci_generic_intr() would not touch these, but there's no way to stop sdhci_generic_intr() from nabbing them before the specific driver can if another interrupt comes in.

A cursory glance through other SDHCI stuff leads me to believe that we can simply AND the status with slot->intmask and make sure we always observe SDHCI_INT_DMA_END and SDHCI_INT_RESPONSE (in case interrupt aggregation is doing funky things with slot->intmask) and this will still be correct.

[kevans]

Try to sucker a couple more people into the review- I don't recall now which !(DATA_AVAIL|SPACE_AVAIL) interrupt was being processed causing us to re-enter the DATA_AVAIL|SPACE_AVAIL path, though. With the current bcm2835_sdhci driver, it generally results in "DMA in use" noise that seems rather redundant because we don't want these interrupts to be processed outside of our DMA interrupt.

[marius]

I'm inclined to say that this change is okay, I'm having problems to understand its motivation, though. First off, bcm_sdhci_intr() already wraps around sdhci_generic_intr() so I don't see why sdhci_bcm(4) shouldn't actually be able to nab SDHCI interrupts before sdhci(4) does. Granted, there's the case of sdhci(4) calling sdhci_generic_intr() directly when dumping. However, when dumping in case of a panic, the scheduler is stopped so the interrupt thread for bcm_sdhci_dma_intr() shouldn't get to run either. Thus, it's not obvious to me how panic dumping currently can work at all in the first place if sdhci_bcm(4) uses DMA for these requests.
Second, "platform-driven transfer [...] keeping the bits enabled in SDHCI_INT_ENABLE for internal handling" sounds like using SDHCI_INT_ENABLE as a mere scratchpad register in the platform-specific code. If that's what you mean, it generally would be better implemented using e. g. a variable or a member in the front-end-specific softc.

[kevans]

In D22430#491639, @marius wrote:

I'm inclined to say that this change is okay, I'm having problems to understand its motivation, though. First off, bcm_sdhci_intr() already wraps around sdhci_generic_intr() so I don't see why sdhci_bcm(4) shouldn't actually be able to nab SDHCI interrupts before sdhci(4) does. Granted, there's the case of sdhci(4) calling sdhci_generic_intr() directly when dumping. However, when dumping in case of a panic, the scheduler is stopped so the interrupt thread for bcm_sdhci_dma_intr() shouldn't get to run either. Thus, it's not obvious to me how panic dumping currently can work at all in the first place if sdhci_bcm(4) uses DMA for these requests.

The issue with trying to nab them beforehand is mainly that we can't/shouldn't ack them in the sdhci interrupt handler, so we'd need to re-architect it a little bit so that bcm_sdhci_intr can read the register, mask these bits out if DMA is in progress, then pass it back up to sdhci_generic_intr_locked() -- that would take the already-read interrupt status bits and process them itself. sdhci_generic_intr would then take the slot-lock, read the status register, and do the needful. This isn't terrible looking, but it might be excessive.

Dumping <-> Platform interactions are kind of interesting, though, and I'm not sure how this works. If we panic mid-transfer, I would think SDHCI_PLATFORM_FINISH_TRANSFER() should be invoked to kill off the transfer, which would re-enable these interrupts as expected -- or at least some method of consulting the platform handler on what to do, because ^/sys/dev/sdhci/sdhci.c doesn't necessarily know what the platform handler might have been doing.

Second, "platform-driven transfer [...] keeping the bits enabled in SDHCI_INT_ENABLE for internal handling" sounds like using SDHCI_INT_ENABLE as a mere scratchpad register in the platform-specific code. If that's what you mean, it generally would be better implemented using e. g. a variable or a member in the front-end-specific softc.

It's really wanting to disable the interrupts while still retaining the ability to read the interrupt status after it's flushed the current FIFO worth of data via DMA, which is effectively doing PIO via chip external to the ARM core we're running on.

[kevans]

Sorry, it occurs to me that I probably misinterpreted about dumping. Assuming previous transfers are aborted/cleaned up, we should probably make sure the platform driver doesn't handle that.