On Tue, 13 Jun 2023 at 22:32, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Tue, Jun 13, 2023 at 2:08 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> > Typically, the cmd->busy_timeout contains the current value of the
> > timeout that should be used for the commands that have the flags
> > MMC_RSP_BUSY set for it.
> >
> > The stm variant already uses cmd->busy_timeout, but also assigns a
> > default timeout, just to make sure if the core has failed to set
> > cmd->busy_timeout (it shouldn't but who knows).
>
> I generalized the STM32 solution with the core-specified timeout
> and default and used that.
>
> If we know the core will always provide correct timeouts we should
> probably delete hacks like this though (but that would be a separate
> patch).
>
> > > +static void busy_timeout_work(struct work_struct *work)
> > > +{
> > > + struct mmci_host *host =
> > > + container_of(work, struct mmci_host, busy_timeout_work.work);
> > > + u32 status;
> > > +
> > > + dev_dbg(mmc_dev(host->mmc), "timeout waiting for busy IRQ\n");
> > > + status = readl(host->base + MMCISTATUS);
> > > + mmci_cmd_irq(host, host->cmd, status);
> >
> > There's no locking here. I assume that's because we call
> > cancel_delayed_work_sync() from an atomic context and we don't want
> > that to hang.
> >
> > However, can't the call to mmci_cmd_irq() race with a proper irq being
> > managed in parallel?
>
> It will not be a problem AFAIC: the MMCI is using level IRQs, meaning it
> will handle all flags that are set for command or data IRQs before exiting
> the IRQ handler, the order of the IRQ handling if several fire at the same
> time is actually dependent on the order the IRQ flags are checked in the
> code.
>
> When the interrupt handler exits, all IRQs should be handled and the
> respective IRQ lines and thus the IRQ from the MMCI should be
> de-asserted.
Right, I think I follow.
>
> In this case, our problem is that an interrupt is not fired at all, but if
> the actual IRQ (that should have been fired) or a totally different IRQ
> (such as an error) is fired at the same time doesn't matter: the pending
> IRQs will be handled, and if the timer then fires an additional IRQ
> the IRQ handler will check if there are any IRQs to handle and then
> conclude there isn't and then just return.
To clarify, I am not worried about the IRQ handling as such.
However, we use the spin_lock to protect some members in the struct
mmci_host. In this case, the mmci_cmd_irq() is using "host->cmd" to
understand whether there is an active command to manage. When the
command has been completed, we set host->cmd to NULL.
[...]
Kind regards
Uffe
