| Message ID | 20181009222527.74260-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: qcom: q6v5: shore up resource probe handling | expand |
Hi, On Tue, Oct 9, 2018 at 3:33 PM Brian Norris <briannorris@chromium.org> wrote: > + if (q6v5->wdog_irq < 0) { > + if (q6v5->wdog_irq != -EPROBE_DEFER) > + dev_err(&pdev->dev, > + "failed to retrieve wdog IRQ: %d\n", > + q6v5->wdog_irq); > + return q6v5->wdog_irq; > + } optional: Since there's the same pattern 5 times here, I wonder if we should abstract it out to a helper function that would print the error? ...in the ideal case it would be somewhere that all Linux drivers could use since this is a super common pattern, but that might be a bit too much yak shaving... > @@ -237,8 +260,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > disable_irq(q6v5->handover_irq); > > q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack"); > - if (q6v5->stop_irq == -EPROBE_DEFER) > - return -EPROBE_DEFER; > + if (q6v5->stop_irq < 0) { > + if (q6v5->stop_irq != -EPROBE_DEFER) > + dev_err(&pdev->dev, > + "failed to retrieve stop IRQ: %d\n", > + q6v5->stop_irq); > + return q6v5->stop_irq; Nitty nit that it's the "stop-ack" IRQ, not the "stop" IRQ. The error message below this one calls it stop-ack too so it would be ideal to keep it consistent between the two error messages. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Tue, Oct 9, 2018 at 4:34 PM Doug Anderson <dianders@chromium.org> wrote: > On Tue, Oct 9, 2018 at 3:33 PM Brian Norris <briannorris@chromium.org> wrote: > > + if (q6v5->wdog_irq < 0) { > > + if (q6v5->wdog_irq != -EPROBE_DEFER) > > + dev_err(&pdev->dev, > > + "failed to retrieve wdog IRQ: %d\n", > > + q6v5->wdog_irq); > > + return q6v5->wdog_irq; > > + } > > optional: Since there's the same pattern 5 times here, I wonder if we > should abstract it out to a helper function that would print the > error? I'll leave that up to Bjorn. I don't personally care to do this, but I also acknowledge that there are a bunch of drivers that do this wrong. > ...in the ideal case it would be somewhere that all Linux drivers > could use since this is a super common pattern, but that might be a > bit too much yak shaving... Yeah, I'll pass on shaving that yak. > > @@ -237,8 +260,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > > disable_irq(q6v5->handover_irq); > > > > q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack"); > > - if (q6v5->stop_irq == -EPROBE_DEFER) > > - return -EPROBE_DEFER; > > + if (q6v5->stop_irq < 0) { > > + if (q6v5->stop_irq != -EPROBE_DEFER) > > + dev_err(&pdev->dev, > > + "failed to retrieve stop IRQ: %d\n", > > + q6v5->stop_irq); > > + return q6v5->stop_irq; > > Nitty nit that it's the "stop-ack" IRQ, not the "stop" IRQ. The error > message below this one calls it stop-ack too so it would be ideal to > keep it consistent between the two error messages. Ack. I thought I double checked on comparing the error messages. Bjorn already has this on his radar, so I'll let him decide if he wants other changes, or if he wants to make his own transformations before applying it. > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks, Brian
On Tue 09 Oct 16:34 PDT 2018, Doug Anderson wrote: > Hi, > > On Tue, Oct 9, 2018 at 3:33 PM Brian Norris <briannorris@chromium.org> wrote: > > + if (q6v5->wdog_irq < 0) { > > + if (q6v5->wdog_irq != -EPROBE_DEFER) > > + dev_err(&pdev->dev, > > + "failed to retrieve wdog IRQ: %d\n", > > + q6v5->wdog_irq); > > + return q6v5->wdog_irq; > > + } > > optional: Since there's the same pattern 5 times here, I wonder if we > should abstract it out to a helper function that would print the > error? > Before breaking this code out to a common function shared between the various q6v5 drivers this was a separate helper function, but that wasn't pretty either :/ > ...in the ideal case it would be somewhere that all Linux drivers > could use since this is a super common pattern, but that might be a > bit too much yak shaving... > Yeah I agree, it would be a nice helper but there's a bunch of variants involved. I've added it on the "someday/maybe" list. > > > @@ -237,8 +260,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > > disable_irq(q6v5->handover_irq); > > > > q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack"); > > - if (q6v5->stop_irq == -EPROBE_DEFER) > > - return -EPROBE_DEFER; > > + if (q6v5->stop_irq < 0) { > > + if (q6v5->stop_irq != -EPROBE_DEFER) > > + dev_err(&pdev->dev, > > + "failed to retrieve stop IRQ: %d\n", > > + q6v5->stop_irq); > > + return q6v5->stop_irq; > > Nitty nit that it's the "stop-ack" IRQ, not the "stop" IRQ. The error > message below this one calls it stop-ack too so it would be ideal to > keep it consistent between the two error messages. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> I applied the patch with the nit fixed and your r-b. Thanks, Bjorn
diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index edeb2e43209e..7d6a98072b21 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -187,6 +187,14 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, init_completion(&q6v5->stop_done); q6v5->wdog_irq = platform_get_irq_byname(pdev, "wdog"); + if (q6v5->wdog_irq < 0) { + if (q6v5->wdog_irq != -EPROBE_DEFER) + dev_err(&pdev->dev, + "failed to retrieve wdog IRQ: %d\n", + q6v5->wdog_irq); + return q6v5->wdog_irq; + } + ret = devm_request_threaded_irq(&pdev->dev, q6v5->wdog_irq, NULL, q6v5_wdog_interrupt, IRQF_TRIGGER_RISING | IRQF_ONESHOT, @@ -197,8 +205,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, } q6v5->fatal_irq = platform_get_irq_byname(pdev, "fatal"); - if (q6v5->fatal_irq == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (q6v5->fatal_irq < 0) { + if (q6v5->fatal_irq != -EPROBE_DEFER) + dev_err(&pdev->dev, + "failed to retrieve fatal IRQ: %d\n", + q6v5->fatal_irq); + return q6v5->fatal_irq; + } ret = devm_request_threaded_irq(&pdev->dev, q6v5->fatal_irq, NULL, q6v5_fatal_interrupt, @@ -210,8 +223,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, } q6v5->ready_irq = platform_get_irq_byname(pdev, "ready"); - if (q6v5->ready_irq == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (q6v5->ready_irq < 0) { + if (q6v5->ready_irq != -EPROBE_DEFER) + dev_err(&pdev->dev, + "failed to retrieve ready IRQ: %d\n", + q6v5->ready_irq); + return q6v5->ready_irq; + } ret = devm_request_threaded_irq(&pdev->dev, q6v5->ready_irq, NULL, q6v5_ready_interrupt, @@ -223,8 +241,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, } q6v5->handover_irq = platform_get_irq_byname(pdev, "handover"); - if (q6v5->handover_irq == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (q6v5->handover_irq < 0) { + if (q6v5->handover_irq != -EPROBE_DEFER) + dev_err(&pdev->dev, + "failed to retrieve handover IRQ: %d\n", + q6v5->handover_irq); + return q6v5->handover_irq; + } ret = devm_request_threaded_irq(&pdev->dev, q6v5->handover_irq, NULL, q6v5_handover_interrupt, @@ -237,8 +260,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, disable_irq(q6v5->handover_irq); q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack"); - if (q6v5->stop_irq == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (q6v5->stop_irq < 0) { + if (q6v5->stop_irq != -EPROBE_DEFER) + dev_err(&pdev->dev, + "failed to retrieve stop IRQ: %d\n", + q6v5->stop_irq); + return q6v5->stop_irq; + } ret = devm_request_threaded_irq(&pdev->dev, q6v5->stop_irq, NULL, q6v5_stop_interrupt,
Commit d5269c4553a6 ("remoteproc: qcom: q6v5: Propagate EPROBE_DEFER") fixed up our probe code to handle -EPROBE_DEFER, but it ignored one of our interrupts, and it also didn't really handle all the other error codes you might get (e.g., with a bad DT definition). Handle those all explicitly. Fixes: d5269c4553a6 ("remoteproc: qcom: q6v5: Propagate EPROBE_DEFER") Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/remoteproc/qcom_q6v5.c | 44 +++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 8 deletions(-)