diff mbox series

[1/2] drm/msm: don't free the IRQ if it was not requested

Message ID 20220507010021.1667700-1-dmitry.baryshkov@linaro.org
State Accepted
Commit 577e2a9dfc8fba7938aaf75db63fae7e328cc3cb
Headers show
Series [1/2] drm/msm: don't free the IRQ if it was not requested | expand

Commit Message

Dmitry Baryshkov May 7, 2022, 1 a.m. UTC
As msm_drm_uninit() is called from the msm_drm_init() error path,
additional care should be necessary as not to call the free_irq() for
the IRQ that was not requested before (because an error occured earlier
than the request_irq() call).

This fixed the issue reported with the following backtrace:

[    8.571329] Trying to free already-free IRQ 187
[    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
[    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
[    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
[    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
[    8.641496] Workqueue: events_unbound deferred_probe_work_func
[    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.654681] pc : free_irq+0x1e0/0x35c
[    8.658454] lr : free_irq+0x1e0/0x35c
[    8.662228] sp : ffff800008ab3950
[    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
[    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
[    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
[    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
[    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
[    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
[    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
[    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
[    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
[    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
[    8.739217] Call trace:
[    8.741755]  free_irq+0x1e0/0x35c
[    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
[    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
[    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
[    8.760657]  __component_add+0xa0/0x170
[    8.764626]  component_add+0x14/0x20
[    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
[    8.773242]  platform_probe+0x68/0xe0
[    8.777043]  really_probe.part.0+0x9c/0x28c
[    8.781368]  __driver_probe_device+0x98/0x144
[    8.785871]  driver_probe_device+0x40/0x140
[    8.790191]  __device_attach_driver+0xb4/0x120
[    8.794788]  bus_for_each_drv+0x78/0xd0
[    8.798751]  __device_attach+0xdc/0x184
[    8.802713]  device_initial_probe+0x14/0x20
[    8.807031]  bus_probe_device+0x9c/0xa4
[    8.810991]  deferred_probe_work_func+0x88/0xc0
[    8.815667]  process_one_work+0x1d0/0x320
[    8.819809]  worker_thread+0x14c/0x444
[    8.823688]  kthread+0x10c/0x110
[    8.827036]  ret_from_fork+0x10/0x20

Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 7 ++++++-
 drivers/gpu/drm/msm/msm_kms.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Abhinav Kumar May 7, 2022, 6:08 p.m. UTC | #1
On 5/6/2022 6:00 PM, Dmitry Baryshkov wrote:
> As msm_drm_uninit() is called from the msm_drm_init() error path,
> additional care should be necessary as not to call the free_irq() for
> the IRQ that was not requested before (because an error occured earlier
> than the request_irq() call).
> 
> This fixed the issue reported with the following backtrace:
> 
> [    8.571329] Trying to free already-free IRQ 187
> [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.654681] pc : free_irq+0x1e0/0x35c
> [    8.658454] lr : free_irq+0x1e0/0x35c
> [    8.662228] sp : ffff800008ab3950
> [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> [    8.739217] Call trace:
> [    8.741755]  free_irq+0x1e0/0x35c
> [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> [    8.760657]  __component_add+0xa0/0x170
> [    8.764626]  component_add+0x14/0x20
> [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> [    8.773242]  platform_probe+0x68/0xe0
> [    8.777043]  really_probe.part.0+0x9c/0x28c
> [    8.781368]  __driver_probe_device+0x98/0x144
> [    8.785871]  driver_probe_device+0x40/0x140
> [    8.790191]  __device_attach_driver+0xb4/0x120
> [    8.794788]  bus_for_each_drv+0x78/0xd0
> [    8.798751]  __device_attach+0xdc/0x184
> [    8.802713]  device_initial_probe+0x14/0x20
> [    8.807031]  bus_probe_device+0x9c/0xa4
> [    8.810991]  deferred_probe_work_func+0x88/0xc0
> [    8.815667]  process_one_work+0x1d0/0x320
> [    8.819809]  worker_thread+0x14c/0x444
> [    8.823688]  kthread+0x10c/0x110
> [    8.827036]  ret_from_fork+0x10/0x20
> 
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I was looking at how other drm drivers handle this and they also have a 
similar logic. So this is a good addition and thanks to the -EDEFER path 
getting exposed we finally uncovered this.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> ---
>   drivers/gpu/drm/msm/msm_drv.c | 7 ++++++-
>   drivers/gpu/drm/msm/msm_kms.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4a3dda23e3e0..44485363f37a 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -113,6 +113,8 @@ static int msm_irq_postinstall(struct drm_device *dev)
>   
>   static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>   {
> +	struct msm_drm_private *priv = dev->dev_private;
> +	struct msm_kms *kms = priv->kms;
>   	int ret;
>   
>   	if (irq == IRQ_NOTCONNECTED)
> @@ -124,6 +126,8 @@ static int msm_irq_install(struct drm_device *dev, unsigned int irq)
>   	if (ret)
>   		return ret;
>   
> +	kms->irq_requested = true;
> +
>   	ret = msm_irq_postinstall(dev);
>   	if (ret) {
>   		free_irq(irq, dev);
> @@ -139,7 +143,8 @@ static void msm_irq_uninstall(struct drm_device *dev)
>   	struct msm_kms *kms = priv->kms;
>   
>   	kms->funcs->irq_uninstall(kms);
> -	free_irq(kms->irq, dev);
> +	if (kms->irq_requested)
> +		free_irq(kms->irq, dev);
>   }
>   
>   struct msm_vblank_work {
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index ab25fff271f9..f8ed7588928c 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -148,6 +148,7 @@ struct msm_kms {
>   
>   	/* irq number to be passed on to msm_irq_install */
>   	int irq;
> +	bool irq_requested;
>   
>   	/* mapper-id used to request GEM buffer mapped for scanout: */
>   	struct msm_gem_address_space *aspace;
Stephen Boyd May 12, 2022, 12:54 a.m. UTC | #2
Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> As msm_drm_uninit() is called from the msm_drm_init() error path,
> additional care should be necessary as not to call the free_irq() for
> the IRQ that was not requested before (because an error occured earlier
> than the request_irq() call).
>
> This fixed the issue reported with the following backtrace:
>
> [    8.571329] Trying to free already-free IRQ 187
> [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.654681] pc : free_irq+0x1e0/0x35c
> [    8.658454] lr : free_irq+0x1e0/0x35c
> [    8.662228] sp : ffff800008ab3950
> [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> [    8.739217] Call trace:
> [    8.741755]  free_irq+0x1e0/0x35c
> [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> [    8.760657]  __component_add+0xa0/0x170
> [    8.764626]  component_add+0x14/0x20
> [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> [    8.773242]  platform_probe+0x68/0xe0
> [    8.777043]  really_probe.part.0+0x9c/0x28c
> [    8.781368]  __driver_probe_device+0x98/0x144
> [    8.785871]  driver_probe_device+0x40/0x140
> [    8.790191]  __device_attach_driver+0xb4/0x120
> [    8.794788]  bus_for_each_drv+0x78/0xd0
> [    8.798751]  __device_attach+0xdc/0x184
> [    8.802713]  device_initial_probe+0x14/0x20
> [    8.807031]  bus_probe_device+0x9c/0xa4
> [    8.810991]  deferred_probe_work_func+0x88/0xc0
> [    8.815667]  process_one_work+0x1d0/0x320
> [    8.819809]  worker_thread+0x14c/0x444
> [    8.823688]  kthread+0x10c/0x110
> [    8.827036]  ret_from_fork+0x10/0x20
>
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")

Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
clearing hw interrupts if hw_intr is null during drm uninit")? I mean
that with this patch applied kms->irq_requested makes the check in
dpu_core_irq_uninstall() irrelevant because it isn't called anymore?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Dmitry Baryshkov May 12, 2022, 1:01 a.m. UTC | #3
On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> > As msm_drm_uninit() is called from the msm_drm_init() error path,
> > additional care should be necessary as not to call the free_irq() for
> > the IRQ that was not requested before (because an error occured earlier
> > than the request_irq() call).
> >
> > This fixed the issue reported with the following backtrace:
> >
> > [    8.571329] Trying to free already-free IRQ 187
> > [    8.571339] WARNING: CPU: 0 PID: 76 at kernel/irq/manage.c:1895 free_irq+0x1e0/0x35c
> > [    8.588746] Modules linked in: pmic_glink pdr_interface fastrpc qrtr_smd snd_soc_hdmi_codec msm fsa4480 gpu_sched drm_dp_aux_bus qrtr i2c_qcom_geni crct10dif_ce qcom_stats qcom_q6v5_pas drm_display_helper gpi qcom_pil_info drm_kms_helper qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qcom_rng mdt_loader qmi_helpers phy_qcom_qmp ufs_qcom typec qnoc_sm8350 socinfo rmtfs_mem fuse drm ipv6
> > [    8.624154] CPU: 0 PID: 76 Comm: kworker/u16:2 Not tainted 5.18.0-rc5-next-20220506-00033-g6cee8cab6089-dirty #419
> > [    8.624161] Hardware name: Qualcomm Technologies, Inc. SM8350 HDK (DT)
> > [    8.641496] Workqueue: events_unbound deferred_probe_work_func
> > [    8.647510] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    8.654681] pc : free_irq+0x1e0/0x35c
> > [    8.658454] lr : free_irq+0x1e0/0x35c
> > [    8.662228] sp : ffff800008ab3950
> > [    8.665642] x29: ffff800008ab3950 x28: 0000000000000000 x27: ffff16350f56a700
> > [    8.672994] x26: ffff1635025df080 x25: ffff16350251badc x24: ffff16350251bb90
> > [    8.680343] x23: 0000000000000000 x22: 00000000000000bb x21: ffff16350e8f9800
> > [    8.687690] x20: ffff16350251ba00 x19: ffff16350cbd5880 x18: ffffffffffffffff
> > [    8.695039] x17: 0000000000000000 x16: ffffa2dd12179434 x15: ffffa2dd1431d02d
> > [    8.702391] x14: 0000000000000000 x13: ffffa2dd1431d028 x12: 662d79646165726c
> > [    8.709740] x11: ffffa2dd13fd2438 x10: 000000000000000a x9 : 00000000000000bb
> > [    8.717111] x8 : ffffa2dd13fd23f0 x7 : ffff800008ab3750 x6 : 00000000fffff202
> > [    8.724487] x5 : ffff16377e870a18 x4 : 00000000fffff202 x3 : ffff735a6ae1b000
> > [    8.731851] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff1635015f8000
> > [    8.739217] Call trace:
> > [    8.741755]  free_irq+0x1e0/0x35c
> > [    8.745198]  msm_drm_uninit.isra.0+0x14c/0x294 [msm]
> > [    8.750548]  msm_drm_bind+0x28c/0x5d0 [msm]
> > [    8.755081]  try_to_bring_up_aggregate_device+0x164/0x1d0
> > [    8.760657]  __component_add+0xa0/0x170
> > [    8.764626]  component_add+0x14/0x20
> > [    8.768337]  dp_display_probe+0x2a4/0x464 [msm]
> > [    8.773242]  platform_probe+0x68/0xe0
> > [    8.777043]  really_probe.part.0+0x9c/0x28c
> > [    8.781368]  __driver_probe_device+0x98/0x144
> > [    8.785871]  driver_probe_device+0x40/0x140
> > [    8.790191]  __device_attach_driver+0xb4/0x120
> > [    8.794788]  bus_for_each_drv+0x78/0xd0
> > [    8.798751]  __device_attach+0xdc/0x184
> > [    8.802713]  device_initial_probe+0x14/0x20
> > [    8.807031]  bus_probe_device+0x9c/0xa4
> > [    8.810991]  deferred_probe_work_func+0x88/0xc0
> > [    8.815667]  process_one_work+0x1d0/0x320
> > [    8.819809]  worker_thread+0x14c/0x444
> > [    8.823688]  kthread+0x10c/0x110
> > [    8.827036]  ret_from_fork+0x10/0x20
> >
> > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Fixes: f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")
>
> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> that with this patch applied kms->irq_requested makes the check in
> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?

Yes.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Stephen Boyd May 12, 2022, 1:29 a.m. UTC | #4
Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> >
> > Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> > clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> > that with this patch applied kms->irq_requested makes the check in
> > dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>
> Yes.
>

I didn't see it deleted in the second patch so is a revert going to be
sent?
Dmitry Baryshkov May 12, 2022, 1:30 a.m. UTC | #5
On 12/05/2022 04:29, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
>> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>>>
>>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
>>>
>>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
>>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
>>> that with this patch applied kms->irq_requested makes the check in
>>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>>
>> Yes.
>>
> 
> I didn't see it deleted in the second patch so is a revert going to be
> sent?

No need to. They are separate checks. The older one is superseded (as it 
will be never hit),  but it is still valid and useful (to protect from 
the crash if this code changes again).
Stephen Boyd May 12, 2022, 1:41 a.m. UTC | #6
Quoting Dmitry Baryshkov (2022-05-11 18:30:55)
> On 12/05/2022 04:29, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
> >> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
> >>>
> >>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
> >>>
> >>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
> >>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
> >>> that with this patch applied kms->irq_requested makes the check in
> >>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
> >>
> >> Yes.
> >>
> >
> > I didn't see it deleted in the second patch so is a revert going to be
> > sent?
>
> No need to. They are separate checks. The older one is superseded (as it
> will be never hit),  but it is still valid and useful (to protect from
> the crash if this code changes again).
>

Ew, gross. The extra conditionals everywhere really makes it hard to
follow along.
Dmitry Baryshkov May 13, 2022, 11:55 a.m. UTC | #7
On 12/05/2022 04:41, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-05-11 18:30:55)
>> On 12/05/2022 04:29, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-05-11 18:01:31)
>>>> On Thu, 12 May 2022 at 03:54, Stephen Boyd <swboyd@chromium.org> wrote:
>>>>>
>>>>> Quoting Dmitry Baryshkov (2022-05-06 18:00:20)
>>>>>
>>>>> Does this supersede commit 01013ba9bbdd ("drm/msm/disp/dpu1: avoid
>>>>> clearing hw interrupts if hw_intr is null during drm uninit")? I mean
>>>>> that with this patch applied kms->irq_requested makes the check in
>>>>> dpu_core_irq_uninstall() irrelevant because it isn't called anymore?
>>>>
>>>> Yes.
>>>>
>>>
>>> I didn't see it deleted in the second patch so is a revert going to be
>>> sent?
>>
>> No need to. They are separate checks. The older one is superseded (as it
>> will be never hit),  but it is still valid and useful (to protect from
>> the crash if this code changes again).
>>
> 
> Ew, gross. The extra conditionals everywhere really makes it hard to
> follow along.

With the second patch being dropped I'd prefer to leave both conditions 
in place.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4a3dda23e3e0..44485363f37a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -113,6 +113,8 @@  static int msm_irq_postinstall(struct drm_device *dev)
 
 static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 {
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
 	int ret;
 
 	if (irq == IRQ_NOTCONNECTED)
@@ -124,6 +126,8 @@  static int msm_irq_install(struct drm_device *dev, unsigned int irq)
 	if (ret)
 		return ret;
 
+	kms->irq_requested = true;
+
 	ret = msm_irq_postinstall(dev);
 	if (ret) {
 		free_irq(irq, dev);
@@ -139,7 +143,8 @@  static void msm_irq_uninstall(struct drm_device *dev)
 	struct msm_kms *kms = priv->kms;
 
 	kms->funcs->irq_uninstall(kms);
-	free_irq(kms->irq, dev);
+	if (kms->irq_requested)
+		free_irq(kms->irq, dev);
 }
 
 struct msm_vblank_work {
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index ab25fff271f9..f8ed7588928c 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -148,6 +148,7 @@  struct msm_kms {
 
 	/* irq number to be passed on to msm_irq_install */
 	int irq;
+	bool irq_requested;
 
 	/* mapper-id used to request GEM buffer mapped for scanout: */
 	struct msm_gem_address_space *aspace;