Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

drm/msm/dp: Return IRQ_NONE for unhandled interrupts

If our interrupt handler gets called and we don't really handle the
interrupt then we should return IRQ_NONE. The current interrupt
handler didn't do this, so let's fix it.

NOTE: for some of the cases it's clear that we should return IRQ_NONE
and some cases it's clear that we should return IRQ_HANDLED. However,
there are a few that fall somewhere in between. Specifically, the
documentation for when to return IRQ_NONE vs. IRQ_HANDLED is probably
best spelled out in the commit message of commit d9e4ad5badf4 ("Document
that IRQ_NONE should be returned when IRQ not actually handled"). That
commit makes it clear that we should return IRQ_HANDLED if we've done
something to make the interrupt stop happening.

The case where it's unclear is, for instance, in dp_aux_isr() after
we've read the interrupt using dp_catalog_aux_get_irq() and confirmed
that "isr" is non-zero. The function dp_catalog_aux_get_irq() not only
reads the interrupts but it also "ack"s all the interrupts that are
returned. For an "unknown" interrupt this has a very good chance of
actually stopping the interrupt from happening. That would mean we've
identified that it's our device and done something to stop them from
happening and should return IRQ_HANDLED. Specifically, it should be
noted that most interrupts that need "ack"ing are ones that are
one-time events and doing an "ack" is enough to clear them. However,
since these interrupts are unknown then, by definition, it's unknown
if "ack"ing them is truly enough to clear them. It's possible that we
also need to remove the original source of the interrupt. In this
case, IRQ_NONE would be a better choice.

Given that returning an occasional IRQ_NONE isn't the absolute end of
the world, however, let's choose that course of action. The IRQ
framework will forgive a few IRQ_NONE returns now and again (and it
won't even log them, which is why we have to log them ourselves). This
means that if we _do_ end hitting an interrupt where "ack"ing isn't
enough the kernel will eventually detect the problem and shut our
device down.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Patchwork: https://patchwork.freedesktop.org/patch/520660/
Link: https://lore.kernel.org/r/20230126170745.v2.2.I2d7aec2fadb9c237cd0090a47d6a8ba2054bf0f8@changeid
[DB: reformatted commit message to make checkpatch happy]
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

authored by

Douglas Anderson and committed by
Dmitry Baryshkov
bfc12020 b20566cd

+22 -12
+7 -5
drivers/gpu/drm/msm/dp/dp_aux.c
··· 368 368 return ret; 369 369 } 370 370 371 - void dp_aux_isr(struct drm_dp_aux *dp_aux) 371 + irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux) 372 372 { 373 373 u32 isr; 374 374 struct dp_aux_private *aux; 375 375 376 376 if (!dp_aux) { 377 377 DRM_ERROR("invalid input\n"); 378 - return; 378 + return IRQ_NONE; 379 379 } 380 380 381 381 aux = container_of(dp_aux, struct dp_aux_private, dp_aux); ··· 384 384 385 385 /* no interrupts pending, return immediately */ 386 386 if (!isr) 387 - return; 387 + return IRQ_NONE; 388 388 389 389 if (!aux->cmd_busy) { 390 390 DRM_ERROR("Unexpected DP AUX IRQ %#010x when not busy\n", isr); 391 - return; 391 + return IRQ_NONE; 392 392 } 393 393 394 394 /* ··· 420 420 aux->aux_error_num = DP_AUX_ERR_NONE; 421 421 } else { 422 422 DRM_WARN("Unexpected interrupt: %#010x\n", isr); 423 - return; 423 + return IRQ_NONE; 424 424 } 425 425 426 426 complete(&aux->comp); 427 + 428 + return IRQ_HANDLED; 427 429 } 428 430 429 431 void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
+1 -1
drivers/gpu/drm/msm/dp/dp_aux.h
··· 11 11 12 12 int dp_aux_register(struct drm_dp_aux *dp_aux); 13 13 void dp_aux_unregister(struct drm_dp_aux *dp_aux); 14 - void dp_aux_isr(struct drm_dp_aux *dp_aux); 14 + irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux); 15 15 void dp_aux_init(struct drm_dp_aux *dp_aux); 16 16 void dp_aux_deinit(struct drm_dp_aux *dp_aux); 17 17 void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
+8 -2
drivers/gpu/drm/msm/dp/dp_ctrl.c
··· 2042 2042 return ret; 2043 2043 } 2044 2044 2045 - void dp_ctrl_isr(struct dp_ctrl *dp_ctrl) 2045 + irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl) 2046 2046 { 2047 2047 struct dp_ctrl_private *ctrl; 2048 2048 u32 isr; 2049 + irqreturn_t ret = IRQ_NONE; 2049 2050 2050 2051 if (!dp_ctrl) 2051 - return; 2052 + return IRQ_NONE; 2052 2053 2053 2054 ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); 2054 2055 ··· 2071 2070 2072 2071 isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog); 2073 2072 2073 + 2074 2074 if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) { 2075 2075 drm_dbg_dp(ctrl->drm_dev, "dp_video_ready\n"); 2076 2076 complete(&ctrl->video_comp); 2077 + ret = IRQ_HANDLED; 2077 2078 } 2078 2079 2079 2080 if (isr & DP_CTRL_INTR_IDLE_PATTERN_SENT) { 2080 2081 drm_dbg_dp(ctrl->drm_dev, "idle_patterns_sent\n"); 2081 2082 complete(&ctrl->idle_comp); 2083 + ret = IRQ_HANDLED; 2082 2084 } 2085 + 2086 + return ret; 2083 2087 } 2084 2088 2085 2089 struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
+1 -1
drivers/gpu/drm/msm/dp/dp_ctrl.h
··· 25 25 int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl); 26 26 int dp_ctrl_off(struct dp_ctrl *dp_ctrl); 27 27 void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl); 28 - void dp_ctrl_isr(struct dp_ctrl *dp_ctrl); 28 + irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl); 29 29 void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl); 30 30 struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link, 31 31 struct dp_panel *panel, struct drm_dp_aux *aux,
+5 -3
drivers/gpu/drm/msm/dp/dp_display.c
··· 1215 1215 static irqreturn_t dp_display_irq_handler(int irq, void *dev_id) 1216 1216 { 1217 1217 struct dp_display_private *dp = dev_id; 1218 - irqreturn_t ret = IRQ_HANDLED; 1218 + irqreturn_t ret = IRQ_NONE; 1219 1219 u32 hpd_isr_status; 1220 1220 1221 1221 if (!dp) { ··· 1243 1243 1244 1244 if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK) 1245 1245 dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0); 1246 + 1247 + ret = IRQ_HANDLED; 1246 1248 } 1247 1249 1248 1250 /* DP controller isr */ 1249 - dp_ctrl_isr(dp->ctrl); 1251 + ret |= dp_ctrl_isr(dp->ctrl); 1250 1252 1251 1253 /* DP aux isr */ 1252 - dp_aux_isr(dp->aux); 1254 + ret |= dp_aux_isr(dp->aux); 1253 1255 1254 1256 return ret; 1255 1257 }