| Message ID | 20230405-pl180-busydetect-fix-v1-1-28ac19a74e5e@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix busydetect on Ux500 PL180/MMCI | expand |
On Wed, 5 Apr 2023 at 08:50, Linus Walleij <linus.walleij@linaro.org> wrote: > > The code unconditionally calls host->ops->busy_complete() > if we get a busy response and the variant supports busy > detection (variant->busy_detect = true). > > However there are several STM32 variants that define > variant->busy_detect to true but do not define any > busy_complete() callback. This looks like a recepie for > a NULL pointer exception. This isn't correct, as things would have exploded by now. :-) > > Check that the callback is valid before calling it. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/host/mmci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index b9e5dfe74e5c..bc150c0d5eed 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -1381,7 +1381,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, > return; > > /* Handle busy detection on DAT0 if the variant supports it. */ > - if (busy_resp && host->variant->busy_detect) > + if (busy_resp && host->variant->busy_detect && host->ops->busy_complete) > if (!host->ops->busy_complete(host, status, err_msk)) > return; All variants that have the .busy_detect flags set, need to assign the ->busy_complete() callback too. To me it seems a bit silly, to check for a mandatory callback, although if you prefer it, then I suggest we do it during ->probe() instead. Kind regards Uffe
On Thu, Apr 6, 2023 at 11:31 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > All variants that have the .busy_detect flags set, need to assign the > ->busy_complete() callback too. > > To me it seems a bit silly, to check for a mandatory callback, > although if you prefer it, then I suggest we do it during ->probe() > instead. Nah I drop it. It's just a bit redundant, what you say is that instead of if (host->variant->busy_detect) { ... } it would suffice to everywhere just check if we have the callback: if (host->ops->busy_complete) {...} and we can drop .busy_detect altogether. But I can deal with that another time. Yours, Linus Walleij
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index b9e5dfe74e5c..bc150c0d5eed 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -1381,7 +1381,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, return; /* Handle busy detection on DAT0 if the variant supports it. */ - if (busy_resp && host->variant->busy_detect) + if (busy_resp && host->variant->busy_detect && host->ops->busy_complete) if (!host->ops->busy_complete(host, status, err_msk)) return;
The code unconditionally calls host->ops->busy_complete() if we get a busy response and the variant supports busy detection (variant->busy_detect = true). However there are several STM32 variants that define variant->busy_detect to true but do not define any busy_complete() callback. This looks like a recepie for a NULL pointer exception. Check that the callback is valid before calling it. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/host/mmci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)