diff mbox series

drm/probe_helper: fix the warning reported when calling drm_kms_helper_poll_disable during suspend

Message ID 20230328023129.3596968-1-zhouzongmin@kylinos.cn (mailing list archive)
State Handled Elsewhere
Headers show
Series drm/probe_helper: fix the warning reported when calling drm_kms_helper_poll_disable during suspend | expand

Commit Message

Zongmin Zhou March 28, 2023, 2:31 a.m. UTC
When drivers call drm_kms_helper_poll_disable from
their device suspend implementation without enabled output polling before,
following warning will be reported,due to work->func not be initialized:

[   55.141361] WARNING: CPU: 3 PID: 372 at kernel/workqueue.c:3066 __flush_work+0x22f/0x240
[   55.141382] Modules linked in: nls_iso8859_1 snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3 aesni_intel drm_kms_helper joydev input_leds syscopyarea crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp parport drm ramoops reed_solomon pstore_blk pstore_zone efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover virtio_blk xhci_pci_renesas failover
[   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted 6.2.0-rc6+ #16
[   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
[   55.141435] Workqueue: events_unbound async_run_entry_fn
[   55.141441] RIP: 0010:__flush_work+0x22f/0x240
[   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66 90 90 90 90 90 90
[   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
[   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff9b72bcbe
[   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ff3ea01e4265e330
[   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09: 8080808080808080
[   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12: ff3ea01e4265e330
[   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15: 0000000000000001
[   55.141455] FS:  0000000000000000(0000) GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
[   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4: 0000000000771ee0
[   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   55.141466] PKRU: 55555554
[   55.141467] Call Trace:
[   55.141469]  <TASK>
[   55.141472]  ? pcie_wait_cmd+0xdf/0x220
[   55.141478]  ? mptcp_seq_show+0xe0/0x180
[   55.141484]  __cancel_work_timer+0x124/0x1b0
[   55.141487]  cancel_delayed_work_sync+0x17/0x20
[   55.141490]  drm_kms_helper_poll_disable+0x26/0x40 [drm_kms_helper]
[   55.141516]  drm_mode_config_helper_suspend+0x25/0x90 [drm_kms_helper]
[   55.141531]  ? __pm_runtime_resume+0x64/0x90
[   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
[   55.141540]  pci_pm_suspend+0x8b/0x1b0
[   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
[   55.141547]  dpm_run_callback+0x4c/0x160
[   55.141550]  __device_suspend+0x14c/0x4c0
[   55.141553]  async_suspend+0x24/0xa0
[   55.141555]  async_run_entry_fn+0x34/0x120
[   55.141557]  process_one_work+0x21a/0x3f0
[   55.141560]  worker_thread+0x4e/0x3c0
[   55.141563]  ? __pfx_worker_thread+0x10/0x10
[   55.141565]  kthread+0xf2/0x120
[   55.141568]  ? __pfx_kthread+0x10/0x10
[   55.141570]  ret_from_fork+0x29/0x50
[   55.141575]  </TASK>
[   55.141575] ---[ end trace 0000000000000000 ]---

Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running vs poll_enabled")
Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
---
 drivers/gpu/drm/drm_probe_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Janne Grunau April 20, 2023, 8:01 p.m. UTC | #1
On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> When drivers call drm_kms_helper_poll_disable from
> their device suspend implementation without enabled output polling before,
> following warning will be reported,due to work->func not be initialized:

we see the same warning with the wpork in progress kms driver for apple 
silicon SoCs. The connectors do not need to polled so the driver never 
calls drm_kms_helper_poll_init().
 
> [   55.141361] WARNING: CPU: 3 PID: 372 at kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> [   55.141382] Modules linked in: nls_iso8859_1 snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3 aesni_intel drm_kms_helper joydev input_leds syscopyarea crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp parport drm ramoops reed_solomon pstore_blk pstore_zone efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover virtio_blk xhci_pci_renesas failover
> [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted 6.2.0-rc6+ #16
> [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> [   55.141435] Workqueue: events_unbound async_run_entry_fn
> [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66 90 90 90 90 90 90
> [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff9b72bcbe
> [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ff3ea01e4265e330
> [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09: 8080808080808080
> [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12: ff3ea01e4265e330
> [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15: 0000000000000001
> [   55.141455] FS:  0000000000000000(0000) GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4: 0000000000771ee0
> [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   55.141466] PKRU: 55555554
> [   55.141467] Call Trace:
> [   55.141469]  <TASK>
> [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> [   55.141484]  __cancel_work_timer+0x124/0x1b0
> [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40 [drm_kms_helper]
> [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90 [drm_kms_helper]
> [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> [   55.141547]  dpm_run_callback+0x4c/0x160
> [   55.141550]  __device_suspend+0x14c/0x4c0
> [   55.141553]  async_suspend+0x24/0xa0
> [   55.141555]  async_run_entry_fn+0x34/0x120
> [   55.141557]  process_one_work+0x21a/0x3f0
> [   55.141560]  worker_thread+0x4e/0x3c0
> [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> [   55.141565]  kthread+0xf2/0x120
> [   55.141568]  ? __pfx_kthread+0x10/0x10
> [   55.141570]  ret_from_fork+0x29/0x50
> [   55.141575]  </TASK>
> [   55.141575] ---[ end trace 0000000000000000 ]---
> 
> Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running vs poll_enabled")
> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 8127be134c39..ac72b18e2257 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct drm_device *dev)
>  	if (dev->mode_config.poll_running)
>  		drm_kms_helper_disable_hpd(dev);
>  
> -	cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
> +	if (dev->mode_config.poll_enabled)
> +		cancel_delayed_work_sync(&dev->mode_config.output_poll_work);

Checking for dev->mode_config.poll_enabled at the start of the function 
and return early if it is not true looks more in style with the rest of 
drm_probe_helper.c.

No difference functionally of course. Tested with the apple kms driver.

Reviewed-by: Janne Grunau <j@jannau.net>

ciao
Janne
Dmitry Baryshkov April 20, 2023, 8:07 p.m. UTC | #2
On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net> wrote:
>
> On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > When drivers call drm_kms_helper_poll_disable from
> > their device suspend implementation without enabled output polling before,
> > following warning will be reported,due to work->func not be initialized:
>
> we see the same warning with the wpork in progress kms driver for apple
> silicon SoCs. The connectors do not need to polled so the driver never
> calls drm_kms_helper_poll_init().
>
> > [   55.141361] WARNING: CPU: 3 PID: 372 at kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > [   55.141382] Modules linked in: nls_iso8859_1 snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3 aesni_intel drm_kms_helper joydev input_leds syscopyarea crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp parport drm ramoops reed_solomon pstore_blk pstore_zone efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover virtio_blk xhci_pci_renesas failover
> > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted 6.2.0-rc6+ #16
> > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66 90 90 90 90 90 90
> > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff9b72bcbe
> > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ff3ea01e4265e330
> > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09: 8080808080808080
> > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12: ff3ea01e4265e330
> > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15: 0000000000000001
> > [   55.141455] FS:  0000000000000000(0000) GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4: 0000000000771ee0
> > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [   55.141466] PKRU: 55555554
> > [   55.141467] Call Trace:
> > [   55.141469]  <TASK>
> > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40 [drm_kms_helper]
> > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90 [drm_kms_helper]
> > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > [   55.141547]  dpm_run_callback+0x4c/0x160
> > [   55.141550]  __device_suspend+0x14c/0x4c0
> > [   55.141553]  async_suspend+0x24/0xa0
> > [   55.141555]  async_run_entry_fn+0x34/0x120
> > [   55.141557]  process_one_work+0x21a/0x3f0
> > [   55.141560]  worker_thread+0x4e/0x3c0
> > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > [   55.141565]  kthread+0xf2/0x120
> > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > [   55.141570]  ret_from_fork+0x29/0x50
> > [   55.141575]  </TASK>
> > [   55.141575] ---[ end trace 0000000000000000 ]---
> >
> > Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running vs poll_enabled")
> > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index 8127be134c39..ac72b18e2257 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct drm_device *dev)
> >       if (dev->mode_config.poll_running)
> >               drm_kms_helper_disable_hpd(dev);
> >
> > -     cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
> > +     if (dev->mode_config.poll_enabled)
> > +             cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
>
> Checking for dev->mode_config.poll_enabled at the start of the function
> and return early if it is not true looks more in style with the rest of
> drm_probe_helper.c.

I think it is an error to call drm_kms_helper_poll_disable() if
polling was not initialized. So, in my opinion the fix should go to
the drm_mode_config_helper_suspend() / _resume() instead. Please add a
guard there using dev->mode_config.poll_enabled.


> No difference functionally of course. Tested with the apple kms driver.
>
> Reviewed-by: Janne Grunau <j@jannau.net>
>
> ciao
> Janne
Zongmin Zhou April 21, 2023, 3 a.m. UTC | #3
On Thu, 2023-04-20 at 23:07 +0300, Dmitry Baryshkov wrote:
> On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net> wrote:
> > 
> > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > > When drivers call drm_kms_helper_poll_disable from
> > > their device suspend implementation without enabled output
> > > polling before,
> > > following warning will be reported,due to work->func not be
> > > initialized:
> > 
> > we see the same warning with the wpork in progress kms driver for
> > apple
> > silicon SoCs. The connectors do not need to polled so the driver
> > never
> > calls drm_kms_helper_poll_init().
> > 
> > > [   55.141361] WARNING: CPU: 3 PID: 372 at
> > > kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > > [   55.141382] Modules linked in: nls_iso8859_1
> > > snd_hda_codec_generic ledtrig_audio snd_hda_intel
> > > snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core
> > > snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi
> > > snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper
> > > drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul snd_timer
> > > ghash_clmulni_intel binfmt_misc sha512_ssse3 aesni_intel
> > > drm_kms_helper joydev input_leds syscopyarea crypto_simd snd
> > > cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore
> > > qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp parport drm
> > > ramoops reed_solomon pstore_blk pstore_zone efi_pstore virtio_rng
> > > ip_tables x_tables autofs4 hid_generic usbhid hid ahci virtio_net
> > > i2c_i801 crc32_pclmul psmouse virtio_scsi libahci i2c_smbus
> > > lpc_ich xhci_pci net_failover virtio_blk xhci_pci_renesas
> > > failover
> > > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted
> > > 6.2.0-rc6+ #16
> > > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
> > > 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
> > > 04/01/2014
> > > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c
> > > 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b
> > > e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66
> > > 90 90 90 90 90 90
> > > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> > > ffffffff9b72bcbe
> > > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI:
> > > ff3ea01e4265e330
> > > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09:
> > > 8080808080808080
> > > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12:
> > > ff3ea01e4265e330
> > > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15:
> > > 0000000000000001
> > > [   55.141455] FS:  0000000000000000(0000)
> > > GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4:
> > > 0000000000771ee0
> > > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > > 0000000000000000
> > > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > > 0000000000000400
> > > [   55.141466] PKRU: 55555554
> > > [   55.141467] Call Trace:
> > > [   55.141469]  <TASK>
> > > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
> > > [drm_kms_helper]
> > > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
> > > [drm_kms_helper]
> > > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > > [   55.141547]  dpm_run_callback+0x4c/0x160
> > > [   55.141550]  __device_suspend+0x14c/0x4c0
> > > [   55.141553]  async_suspend+0x24/0xa0
> > > [   55.141555]  async_run_entry_fn+0x34/0x120
> > > [   55.141557]  process_one_work+0x21a/0x3f0
> > > [   55.141560]  worker_thread+0x4e/0x3c0
> > > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > > [   55.141565]  kthread+0xf2/0x120
> > > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > > [   55.141570]  ret_from_fork+0x29/0x50
> > > [   55.141575]  </TASK>
> > > [   55.141575] ---[ end trace 0000000000000000 ]---
> > > 
> > > Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running vs
> > > poll_enabled")
> > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > > ---
> > >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > b/drivers/gpu/drm/drm_probe_helper.c
> > > index 8127be134c39..ac72b18e2257 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
> > > drm_device *dev)
> > >       if (dev->mode_config.poll_running)
> > >               drm_kms_helper_disable_hpd(dev);
> > > 
> > > -     cancel_delayed_work_sync(&dev-
> > > >mode_config.output_poll_work);
> > > +     if (dev->mode_config.poll_enabled)
> > > +             cancel_delayed_work_sync(&dev-
> > > >mode_config.output_poll_work);
> > 
> > Checking for dev->mode_config.poll_enabled at the start of the
> > function
> > and return early if it is not true looks more in style with the
> > rest of
> > drm_probe_helper.c.
> 
> I think it is an error to call drm_kms_helper_poll_disable() if
> polling was not initialized. So, in my opinion the fix should go to
> the drm_mode_config_helper_suspend() / _resume() instead. Please add
> a
> guard there using dev->mode_config.poll_enabled.
> 
In my test case,It's ok to check for dev->mode_config.poll_enabled in
drm_mode_config_helper_suspend() / _resume() before calling
drm_kms_helper_poll_disable().But I find that some device driver call
drm_kms_helper_poll_disable()/_enable directly without call
_suspend()/_resume() function.

So I think the safest way is to add a guard in
drm_kms_helper_poll_disable() using dev->mode_config.poll_enabled.

I'm looking forward to your comments and Suggestions.

Thanks.
> 
> > No difference functionally of course. Tested with the apple kms
> > driver.
> > 
> > Reviewed-by: Janne Grunau <j@jannau.net>
> > 
> > ciao
> > Janne
> 
> 
>
Janne Grunau April 23, 2023, 8:51 p.m. UTC | #4
On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
> On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net> wrote:
> >
> > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > > When drivers call drm_kms_helper_poll_disable from
> > > their device suspend implementation without enabled output polling before,
> > > following warning will be reported,due to work->func not be initialized:
> >
> > we see the same warning with the wpork in progress kms driver for apple
> > silicon SoCs. The connectors do not need to polled so the driver never
> > calls drm_kms_helper_poll_init().
> >
> > > [   55.141361] WARNING: CPU: 3 PID: 372 at kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > > [   55.141382] Modules linked in: nls_iso8859_1 snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3 aesni_intel drm_kms_helper joydev input_leds syscopyarea crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp parport drm ramoops reed_solomon pstore_blk pstore_zone efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover virtio_blk xhci_pci_renesas failover
> > > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted 6.2.0-rc6+ #16
> > > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> > > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54 d8 00 66 90 90 90 90 90 90
> > > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff9b72bcbe
> > > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ff3ea01e4265e330
> > > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09: 8080808080808080
> > > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12: ff3ea01e4265e330
> > > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15: 0000000000000001
> > > [   55.141455] FS:  0000000000000000(0000) GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4: 0000000000771ee0
> > > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > [   55.141466] PKRU: 55555554
> > > [   55.141467] Call Trace:
> > > [   55.141469]  <TASK>
> > > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40 [drm_kms_helper]
> > > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90 [drm_kms_helper]
> > > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > > [   55.141547]  dpm_run_callback+0x4c/0x160
> > > [   55.141550]  __device_suspend+0x14c/0x4c0
> > > [   55.141553]  async_suspend+0x24/0xa0
> > > [   55.141555]  async_run_entry_fn+0x34/0x120
> > > [   55.141557]  process_one_work+0x21a/0x3f0
> > > [   55.141560]  worker_thread+0x4e/0x3c0
> > > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > > [   55.141565]  kthread+0xf2/0x120
> > > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > > [   55.141570]  ret_from_fork+0x29/0x50
> > > [   55.141575]  </TASK>
> > > [   55.141575] ---[ end trace 0000000000000000 ]---
> > >
> > > Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running vs poll_enabled")
> > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > > ---
> > >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 8127be134c39..ac72b18e2257 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct drm_device *dev)
> > >       if (dev->mode_config.poll_running)
> > >               drm_kms_helper_disable_hpd(dev);
> > >
> > > -     cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
> > > +     if (dev->mode_config.poll_enabled)
> > > +             cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
> >
> > Checking for dev->mode_config.poll_enabled at the start of the function
> > and return early if it is not true looks more in style with the rest of
> > drm_probe_helper.c.
> 
> I think it is an error to call drm_kms_helper_poll_disable() if
> polling was not initialized. So, in my opinion the fix should go to
> the drm_mode_config_helper_suspend() / _resume() instead. Please add a
> guard there using dev->mode_config.poll_enabled.

While I tend to agree to the sentiment I do not think this is the 
correct fix in this situation. drm_kms_helper_poll_disable had the
check since at least 2014. a4e771729a51 is a regression. If we want to  
change the behavior it should be done explicitly and after verifying all 
drm_kms_helper_poll_disable() calls.

#regzbot ^introduced a4e771729a51

ciao
Janne
Zongmin Zhou April 26, 2023, 9:08 a.m. UTC | #5
On Sun, 2023-04-23 at 22:51 +0200, Janne Grunau wrote:
> On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
> > On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net> wrote:
> > > 
> > > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > > > When drivers call drm_kms_helper_poll_disable from
> > > > their device suspend implementation without enabled output
> > > > polling before,
> > > > following warning will be reported,due to work->func not be
> > > > initialized:
> > > 
> > > we see the same warning with the wpork in progress kms driver for
> > > apple
> > > silicon SoCs. The connectors do not need to polled so the driver
> > > never
> > > calls drm_kms_helper_poll_init().
> > > 
> > > > [   55.141361] WARNING: CPU: 3 PID: 372 at
> > > > kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > > > [   55.141382] Modules linked in: nls_iso8859_1
> > > > snd_hda_codec_generic ledtrig_audio snd_hda_intel
> > > > snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core
> > > > snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi
> > > > snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper
> > > > drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul
> > > > snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3
> > > > aesni_intel drm_kms_helper joydev input_leds syscopyarea
> > > > crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw
> > > > soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp
> > > > parport drm ramoops reed_solomon pstore_blk pstore_zone
> > > > efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic
> > > > usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse
> > > > virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover
> > > > virtio_blk xhci_pci_renesas failover
> > > > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted
> > > > 6.2.0-rc6+ #16
> > > > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
> > > > 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
> > > > 04/01/2014
> > > > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > > > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > > > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff
> > > > 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff
> > > > 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54
> > > > d8 00 66 90 90 90 90 90 90
> > > > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > > > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> > > > ffffffff9b72bcbe
> > > > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI:
> > > > ff3ea01e4265e330
> > > > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09:
> > > > 8080808080808080
> > > > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12:
> > > > ff3ea01e4265e330
> > > > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15:
> > > > 0000000000000001
> > > > [   55.141455] FS:  0000000000000000(0000)
> > > > GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > > > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0:
> > > > 0000000080050033
> > > > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4:
> > > > 0000000000771ee0
> > > > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > > > 0000000000000000
> > > > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > > > 0000000000000400
> > > > [   55.141466] PKRU: 55555554
> > > > [   55.141467] Call Trace:
> > > > [   55.141469]  <TASK>
> > > > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > > > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > > > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > > > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > > > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
> > > > [drm_kms_helper]
> > > > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
> > > > [drm_kms_helper]
> > > > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > > > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > > > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > > > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > > > [   55.141547]  dpm_run_callback+0x4c/0x160
> > > > [   55.141550]  __device_suspend+0x14c/0x4c0
> > > > [   55.141553]  async_suspend+0x24/0xa0
> > > > [   55.141555]  async_run_entry_fn+0x34/0x120
> > > > [   55.141557]  process_one_work+0x21a/0x3f0
> > > > [   55.141560]  worker_thread+0x4e/0x3c0
> > > > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > > > [   55.141565]  kthread+0xf2/0x120
> > > > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > > > [   55.141570]  ret_from_fork+0x29/0x50
> > > > [   55.141575]  </TASK>
> > > > [   55.141575] ---[ end trace 0000000000000000 ]---
> > > > 
> > > > Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running
> > > > vs poll_enabled")
> > > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > > > ---
> > > >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > index 8127be134c39..ac72b18e2257 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
> > > > drm_device *dev)
> > > >       if (dev->mode_config.poll_running)
> > > >               drm_kms_helper_disable_hpd(dev);
> > > > 
> > > > -     cancel_delayed_work_sync(&dev-
> > > > >mode_config.output_poll_work);
> > > > +     if (dev->mode_config.poll_enabled)
> > > > +             cancel_delayed_work_sync(&dev-
> > > > >mode_config.output_poll_work);
> > > 
> > > Checking for dev->mode_config.poll_enabled at the start of the
> > > function
> > > and return early if it is not true looks more in style with the
> > > rest of
> > > drm_probe_helper.c.
> > 
> > I think it is an error to call drm_kms_helper_poll_disable() if
> > polling was not initialized. So, in my opinion the fix should go to
> > the drm_mode_config_helper_suspend() / _resume() instead. Please
> > add a
> > guard there using dev->mode_config.poll_enabled.
> 
> While I tend to agree to the sentiment I do not think this is the 
> correct fix in this situation. drm_kms_helper_poll_disable had the
> check since at least 2014. a4e771729a51 is a regression. If we want
> to  
> change the behavior it should be done explicitly and after verifying
> all 
> drm_kms_helper_poll_disable() calls.
> 
> #regzbot ^introduced a4e771729a51
> 
> ciao
> Janne

Dear Janne:

I agree with you like I mentioned on last letter.
Thanks for your time.


Dear Dmitry:

Is there anything else I can do?
Looking forward to your reply.

Thanks
Dmitry Baryshkov April 26, 2023, 1:10 p.m. UTC | #6
On Wed, 26 Apr 2023 at 12:09, zongmin zhou <zhouzongmin@kylinos.cn> wrote:
>
> On Sun, 2023-04-23 at 22:51 +0200, Janne Grunau wrote:
> > On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
> > > On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net> wrote:
> > > >
> > > > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > > > > When drivers call drm_kms_helper_poll_disable from
> > > > > their device suspend implementation without enabled output
> > > > > polling before,
> > > > > following warning will be reported,due to work->func not be
> > > > > initialized:
> > > >
> > > > we see the same warning with the wpork in progress kms driver for
> > > > apple
> > > > silicon SoCs. The connectors do not need to polled so the driver
> > > > never
> > > > calls drm_kms_helper_poll_init().
> > > >
> > > > > [   55.141361] WARNING: CPU: 3 PID: 372 at
> > > > > kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > > > > [   55.141382] Modules linked in: nls_iso8859_1
> > > > > snd_hda_codec_generic ledtrig_audio snd_hda_intel
> > > > > snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core
> > > > > snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi
> > > > > snd_seq intel_rapl_msr intel_rapl_common bochs drm_vram_helper
> > > > > drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul
> > > > > snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3
> > > > > aesni_intel drm_kms_helper joydev input_leds syscopyarea
> > > > > crypto_simd snd cryptd sysfillrect sysimgblt mac_hid serio_raw
> > > > > soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp
> > > > > parport drm ramoops reed_solomon pstore_blk pstore_zone
> > > > > efi_pstore virtio_rng ip_tables x_tables autofs4 hid_generic
> > > > > usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse
> > > > > virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover
> > > > > virtio_blk xhci_pci_renesas failover
> > > > > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not tainted
> > > > > 6.2.0-rc6+ #16
> > > > > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
> > > > > 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
> > > > > 04/01/2014
> > > > > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > > > > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > > > > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff ff
> > > > > 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff ff
> > > > > 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2 54
> > > > > d8 00 66 90 90 90 90 90 90
> > > > > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > > > > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> > > > > ffffffff9b72bcbe
> > > > > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001 RDI:
> > > > > ff3ea01e4265e330
> > > > > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000 R09:
> > > > > 8080808080808080
> > > > > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f R12:
> > > > > ff3ea01e4265e330
> > > > > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80 R15:
> > > > > 0000000000000001
> > > > > [   55.141455] FS:  0000000000000000(0000)
> > > > > GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > > > > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0:
> > > > > 0000000080050033
> > > > > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005 CR4:
> > > > > 0000000000771ee0
> > > > > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > > > > 0000000000000000
> > > > > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > > > > 0000000000000400
> > > > > [   55.141466] PKRU: 55555554
> > > > > [   55.141467] Call Trace:
> > > > > [   55.141469]  <TASK>
> > > > > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > > > > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > > > > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > > > > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > > > > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
> > > > > [drm_kms_helper]
> > > > > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
> > > > > [drm_kms_helper]
> > > > > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > > > > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > > > > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > > > > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > > > > [   55.141547]  dpm_run_callback+0x4c/0x160
> > > > > [   55.141550]  __device_suspend+0x14c/0x4c0
> > > > > [   55.141553]  async_suspend+0x24/0xa0
> > > > > [   55.141555]  async_run_entry_fn+0x34/0x120
> > > > > [   55.141557]  process_one_work+0x21a/0x3f0
> > > > > [   55.141560]  worker_thread+0x4e/0x3c0
> > > > > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > > > > [   55.141565]  kthread+0xf2/0x120
> > > > > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > > > > [   55.141570]  ret_from_fork+0x29/0x50
> > > > > [   55.141575]  </TASK>
> > > > > [   55.141575] ---[ end trace 0000000000000000 ]---
> > > > >
> > > > > Fixes: a4e771729a51 ("drm/probe_helper: sort out poll_running
> > > > > vs poll_enabled")
> > > > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > > index 8127be134c39..ac72b18e2257 100644
> > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
> > > > > drm_device *dev)
> > > > >       if (dev->mode_config.poll_running)
> > > > >               drm_kms_helper_disable_hpd(dev);
> > > > >
> > > > > -     cancel_delayed_work_sync(&dev-
> > > > > >mode_config.output_poll_work);
> > > > > +     if (dev->mode_config.poll_enabled)
> > > > > +             cancel_delayed_work_sync(&dev-
> > > > > >mode_config.output_poll_work);
> > > >
> > > > Checking for dev->mode_config.poll_enabled at the start of the
> > > > function
> > > > and return early if it is not true looks more in style with the
> > > > rest of
> > > > drm_probe_helper.c.
> > >
> > > I think it is an error to call drm_kms_helper_poll_disable() if
> > > polling was not initialized. So, in my opinion the fix should go to
> > > the drm_mode_config_helper_suspend() / _resume() instead. Please
> > > add a
> > > guard there using dev->mode_config.poll_enabled.
> >
> > While I tend to agree to the sentiment I do not think this is the
> > correct fix in this situation. drm_kms_helper_poll_disable had the
> > check since at least 2014. a4e771729a51 is a regression. If we want
> > to
> > change the behavior it should be done explicitly and after verifying
> > all
> > drm_kms_helper_poll_disable() calls.
> >
> > #regzbot ^introduced a4e771729a51
> >
> > ciao
> > Janne
>
> Dear Janne:
>
> I agree with you like I mentioned on last letter.
> Thanks for your time.
>
>
> Dear Dmitry:
>
> Is there anything else I can do?
> Looking forward to your reply.

If it is a common consensus, I'm fine with your approach.
Zongmin Zhou April 28, 2023, 1:17 a.m. UTC | #7
On Wed, 2023-04-26 at 16:10 +0300, Dmitry Baryshkov wrote:
> On Wed, 26 Apr 2023 at 12:09, zongmin zhou <zhouzongmin@kylinos.cn>
> wrote:
> > 
> > On Sun, 2023-04-23 at 22:51 +0200, Janne Grunau wrote:
> > > On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
> > > > On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net>
> > > > wrote:
> > > > > 
> > > > > On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
> > > > > > When drivers call drm_kms_helper_poll_disable from
> > > > > > their device suspend implementation without enabled output
> > > > > > polling before,
> > > > > > following warning will be reported,due to work->func not be
> > > > > > initialized:
> > > > > 
> > > > > we see the same warning with the wpork in progress kms driver
> > > > > for
> > > > > apple
> > > > > silicon SoCs. The connectors do not need to polled so the
> > > > > driver
> > > > > never
> > > > > calls drm_kms_helper_poll_init().
> > > > > 
> > > > > > [   55.141361] WARNING: CPU: 3 PID: 372 at
> > > > > > kernel/workqueue.c:3066 __flush_work+0x22f/0x240
> > > > > > [   55.141382] Modules linked in: nls_iso8859_1
> > > > > > snd_hda_codec_generic ledtrig_audio snd_hda_intel
> > > > > > snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec
> > > > > > snd_hda_core
> > > > > > snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event
> > > > > > snd_rawmidi
> > > > > > snd_seq intel_rapl_msr intel_rapl_common bochs
> > > > > > drm_vram_helper
> > > > > > drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul
> > > > > > snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3
> > > > > > aesni_intel drm_kms_helper joydev input_leds syscopyarea
> > > > > > crypto_simd snd cryptd sysfillrect sysimgblt mac_hid
> > > > > > serio_raw
> > > > > > soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp
> > > > > > parport drm ramoops reed_solomon pstore_blk pstore_zone
> > > > > > efi_pstore virtio_rng ip_tables x_tables autofs4
> > > > > > hid_generic
> > > > > > usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse
> > > > > > virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover
> > > > > > virtio_blk xhci_pci_renesas failover
> > > > > > [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not
> > > > > > tainted
> > > > > > 6.2.0-rc6+ #16
> > > > > > [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
> > > > > > 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
> > > > > > 04/01/2014
> > > > > > [   55.141435] Workqueue: events_unbound async_run_entry_fn
> > > > > > [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
> > > > > > [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff
> > > > > > ff
> > > > > > 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff
> > > > > > ff
> > > > > > 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2
> > > > > > 54
> > > > > > d8 00 66 90 90 90 90 90 90
> > > > > > [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
> > > > > > [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000
> > > > > > RCX:
> > > > > > ffffffff9b72bcbe
> > > > > > [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001
> > > > > > RDI:
> > > > > > ff3ea01e4265e330
> > > > > > [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000
> > > > > > R09:
> > > > > > 8080808080808080
> > > > > > [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f
> > > > > > R12:
> > > > > > ff3ea01e4265e330
> > > > > > [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80
> > > > > > R15:
> > > > > > 0000000000000001
> > > > > > [   55.141455] FS:  0000000000000000(0000)
> > > > > > GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
> > > > > > [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0:
> > > > > > 0000000080050033
> > > > > > [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005
> > > > > > CR4:
> > > > > > 0000000000771ee0
> > > > > > [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000
> > > > > > DR2:
> > > > > > 0000000000000000
> > > > > > [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0
> > > > > > DR7:
> > > > > > 0000000000000400
> > > > > > [   55.141466] PKRU: 55555554
> > > > > > [   55.141467] Call Trace:
> > > > > > [   55.141469]  <TASK>
> > > > > > [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
> > > > > > [   55.141478]  ? mptcp_seq_show+0xe0/0x180
> > > > > > [   55.141484]  __cancel_work_timer+0x124/0x1b0
> > > > > > [   55.141487]  cancel_delayed_work_sync+0x17/0x20
> > > > > > [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
> > > > > > [drm_kms_helper]
> > > > > > [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
> > > > > > [drm_kms_helper]
> > > > > > [   55.141531]  ? __pm_runtime_resume+0x64/0x90
> > > > > > [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
> > > > > > [   55.141540]  pci_pm_suspend+0x8b/0x1b0
> > > > > > [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
> > > > > > [   55.141547]  dpm_run_callback+0x4c/0x160
> > > > > > [   55.141550]  __device_suspend+0x14c/0x4c0
> > > > > > [   55.141553]  async_suspend+0x24/0xa0
> > > > > > [   55.141555]  async_run_entry_fn+0x34/0x120
> > > > > > [   55.141557]  process_one_work+0x21a/0x3f0
> > > > > > [   55.141560]  worker_thread+0x4e/0x3c0
> > > > > > [   55.141563]  ? __pfx_worker_thread+0x10/0x10
> > > > > > [   55.141565]  kthread+0xf2/0x120
> > > > > > [   55.141568]  ? __pfx_kthread+0x10/0x10
> > > > > > [   55.141570]  ret_from_fork+0x29/0x50
> > > > > > [   55.141575]  </TASK>
> > > > > > [   55.141575] ---[ end trace 0000000000000000 ]---
> > > > > > 
> > > > > > Fixes: a4e771729a51 ("drm/probe_helper: sort out
> > > > > > poll_running
> > > > > > vs poll_enabled")
> > > > > > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > index 8127be134c39..ac72b18e2257 100644
> > > > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > > > @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
> > > > > > drm_device *dev)
> > > > > >       if (dev->mode_config.poll_running)
> > > > > >               drm_kms_helper_disable_hpd(dev);
> > > > > > 
> > > > > > -     cancel_delayed_work_sync(&dev-
> > > > > > > mode_config.output_poll_work);
> > > > > > +     if (dev->mode_config.poll_enabled)
> > > > > > +             cancel_delayed_work_sync(&dev-
> > > > > > > mode_config.output_poll_work);
> > > > > 
> > > > > Checking for dev->mode_config.poll_enabled at the start of
> > > > > the
> > > > > function
> > > > > and return early if it is not true looks more in style with
> > > > > the
> > > > > rest of
> > > > > drm_probe_helper.c.
> > > > 
> > > > I think it is an error to call drm_kms_helper_poll_disable() if
> > > > polling was not initialized. So, in my opinion the fix should
> > > > go to
> > > > the drm_mode_config_helper_suspend() / _resume() instead.
> > > > Please
> > > > add a
> > > > guard there using dev->mode_config.poll_enabled.
> > > 
> > > While I tend to agree to the sentiment I do not think this is the
> > > correct fix in this situation. drm_kms_helper_poll_disable had
> > > the
> > > check since at least 2014. a4e771729a51 is a regression. If we
> > > want
> > > to
> > > change the behavior it should be done explicitly and after
> > > verifying
> > > all
> > > drm_kms_helper_poll_disable() calls.
> > > 
> > > #regzbot ^introduced a4e771729a51
> > > 
> > > ciao
> > > Janne
> > 
> > Dear Janne:
> > 
> > I agree with you like I mentioned on last letter.
> > Thanks for your time.
> > 
> > 
> > Dear Dmitry:
> > 
> > Is there anything else I can do?
> > Looking forward to your reply.
> 
> If it is a common consensus, I'm fine with your approach.
> 
Dear Dmitry:

Ok.Thanks for your reply.

Best regards!
Thorsten Leemhuis May 17, 2023, 3:15 p.m. UTC | #8
Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.

Dmitry, was any progress made to address this regression? Doesn't look
like it, but I strongly suspect I'm missing something, as I'm not really
sure if I properly understood this thread. It sounded a bit like
a4e771729a51 should be reverted for now until all
drm_kms_helper_poll_disable() calls have been verified. Is that right?
Or did somebody already verify and fix all of them with bugs?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

On 28.04.23 03:17, zongmin zhou wrote:
> On Wed, 2023-04-26 at 16:10 +0300, Dmitry Baryshkov wrote:
>> On Wed, 26 Apr 2023 at 12:09, zongmin zhou <zhouzongmin@kylinos.cn>
>> wrote:
>>> On Sun, 2023-04-23 at 22:51 +0200, Janne Grunau wrote:
>>>> On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
>>>>> On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net>
>>>>> wrote:
>>>>>>
>>>>>> On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
>>>>>>> When drivers call drm_kms_helper_poll_disable from
>>>>>>> their device suspend implementation without enabled output
>>>>>>> polling before,
>>>>>>> following warning will be reported,due to work->func not be
>>>>>>> initialized:
>>>>>>
>>>>>> we see the same warning with the wpork in progress kms driver
>>>>>> for
>>>>>> apple
>>>>>> silicon SoCs. The connectors do not need to polled so the
>>>>>> driver
>>>>>> never
>>>>>> calls drm_kms_helper_poll_init().
>>>>>>
>>>>>>> [   55.141361] WARNING: CPU: 3 PID: 372 at
>>>>>>> kernel/workqueue.c:3066 __flush_work+0x22f/0x240
>>>>>>> [   55.141382] Modules linked in: nls_iso8859_1
>>>>>>> snd_hda_codec_generic ledtrig_audio snd_hda_intel
>>>>>>> snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec
>>>>>>> snd_hda_core
>>>>>>> snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event
>>>>>>> snd_rawmidi
>>>>>>> snd_seq intel_rapl_msr intel_rapl_common bochs
>>>>>>> drm_vram_helper
>>>>>>> drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul
>>>>>>> snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3
>>>>>>> aesni_intel drm_kms_helper joydev input_leds syscopyarea
>>>>>>> crypto_simd snd cryptd sysfillrect sysimgblt mac_hid
>>>>>>> serio_raw
>>>>>>> soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp
>>>>>>> parport drm ramoops reed_solomon pstore_blk pstore_zone
>>>>>>> efi_pstore virtio_rng ip_tables x_tables autofs4
>>>>>>> hid_generic
>>>>>>> usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse
>>>>>>> virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover
>>>>>>> virtio_blk xhci_pci_renesas failover
>>>>>>> [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not
>>>>>>> tainted
>>>>>>> 6.2.0-rc6+ #16
>>>>>>> [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
>>>>>>> 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
>>>>>>> 04/01/2014
>>>>>>> [   55.141435] Workqueue: events_unbound async_run_entry_fn
>>>>>>> [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
>>>>>>> [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff
>>>>>>> ff
>>>>>>> 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff
>>>>>>> ff
>>>>>>> 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2
>>>>>>> 54
>>>>>>> d8 00 66 90 90 90 90 90 90
>>>>>>> [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
>>>>>>> [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000
>>>>>>> RCX:
>>>>>>> ffffffff9b72bcbe
>>>>>>> [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001
>>>>>>> RDI:
>>>>>>> ff3ea01e4265e330
>>>>>>> [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000
>>>>>>> R09:
>>>>>>> 8080808080808080
>>>>>>> [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f
>>>>>>> R12:
>>>>>>> ff3ea01e4265e330
>>>>>>> [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80
>>>>>>> R15:
>>>>>>> 0000000000000001
>>>>>>> [   55.141455] FS:  0000000000000000(0000)
>>>>>>> GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
>>>>>>> [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0:
>>>>>>> 0000000080050033
>>>>>>> [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005
>>>>>>> CR4:
>>>>>>> 0000000000771ee0
>>>>>>> [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000
>>>>>>> DR2:
>>>>>>> 0000000000000000
>>>>>>> [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0
>>>>>>> DR7:
>>>>>>> 0000000000000400
>>>>>>> [   55.141466] PKRU: 55555554
>>>>>>> [   55.141467] Call Trace:
>>>>>>> [   55.141469]  <TASK>
>>>>>>> [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
>>>>>>> [   55.141478]  ? mptcp_seq_show+0xe0/0x180
>>>>>>> [   55.141484]  __cancel_work_timer+0x124/0x1b0
>>>>>>> [   55.141487]  cancel_delayed_work_sync+0x17/0x20
>>>>>>> [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
>>>>>>> [drm_kms_helper]
>>>>>>> [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
>>>>>>> [drm_kms_helper]
>>>>>>> [   55.141531]  ? __pm_runtime_resume+0x64/0x90
>>>>>>> [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
>>>>>>> [   55.141540]  pci_pm_suspend+0x8b/0x1b0
>>>>>>> [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
>>>>>>> [   55.141547]  dpm_run_callback+0x4c/0x160
>>>>>>> [   55.141550]  __device_suspend+0x14c/0x4c0
>>>>>>> [   55.141553]  async_suspend+0x24/0xa0
>>>>>>> [   55.141555]  async_run_entry_fn+0x34/0x120
>>>>>>> [   55.141557]  process_one_work+0x21a/0x3f0
>>>>>>> [   55.141560]  worker_thread+0x4e/0x3c0
>>>>>>> [   55.141563]  ? __pfx_worker_thread+0x10/0x10
>>>>>>> [   55.141565]  kthread+0xf2/0x120
>>>>>>> [   55.141568]  ? __pfx_kthread+0x10/0x10
>>>>>>> [   55.141570]  ret_from_fork+0x29/0x50
>>>>>>> [   55.141575]  </TASK>
>>>>>>> [   55.141575] ---[ end trace 0000000000000000 ]---
>>>>>>>
>>>>>>> Fixes: a4e771729a51 ("drm/probe_helper: sort out
>>>>>>> poll_running
>>>>>>> vs poll_enabled")
>>>>>>> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
>>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>>>>> index 8127be134c39..ac72b18e2257 100644
>>>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>>>> @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
>>>>>>> drm_device *dev)
>>>>>>>       if (dev->mode_config.poll_running)
>>>>>>>               drm_kms_helper_disable_hpd(dev);
>>>>>>>
>>>>>>> -     cancel_delayed_work_sync(&dev-
>>>>>>>> mode_config.output_poll_work);
>>>>>>> +     if (dev->mode_config.poll_enabled)
>>>>>>> +             cancel_delayed_work_sync(&dev-
>>>>>>>> mode_config.output_poll_work);
>>>>>>
>>>>>> Checking for dev->mode_config.poll_enabled at the start of
>>>>>> the
>>>>>> function
>>>>>> and return early if it is not true looks more in style with
>>>>>> the
>>>>>> rest of
>>>>>> drm_probe_helper.c.
>>>>>
>>>>> I think it is an error to call drm_kms_helper_poll_disable() if
>>>>> polling was not initialized. So, in my opinion the fix should
>>>>> go to
>>>>> the drm_mode_config_helper_suspend() / _resume() instead.
>>>>> Please
>>>>> add a
>>>>> guard there using dev->mode_config.poll_enabled.
>>>>
>>>> While I tend to agree to the sentiment I do not think this is the
>>>> correct fix in this situation. drm_kms_helper_poll_disable had
>>>> the
>>>> check since at least 2014. a4e771729a51 is a regression. If we
>>>> want
>>>> to
>>>> change the behavior it should be done explicitly and after
>>>> verifying
>>>> all
>>>> drm_kms_helper_poll_disable() calls.
>>>>
>>>> #regzbot ^introduced a4e771729a51
>>>>
>>>> ciao
>>>> Janne
>>>
>>> Dear Janne:
>>>
>>> I agree with you like I mentioned on last letter.
>>> Thanks for your time.
>>>
>>>
>>> Dear Dmitry:
>>>
>>> Is there anything else I can do?
>>> Looking forward to your reply.
>>
>> If it is a common consensus, I'm fine with your approach.
>>
> Dear Dmitry:
> 
> Ok.Thanks for your reply.
> 
> Best regards!
> 
>
Thorsten Leemhuis June 5, 2023, 11:35 a.m. UTC | #9
On 17.05.23 17:15, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
> for once, to make this easily accessible to everyone.
> 
> Dmitry, was any progress made to address this regression? Doesn't look
> like it, but I strongly suspect I'm missing something,

FWIW, I'm dropping this from the regression tracking. It might be still
around, but it's a warning and nothing that really breaks things afaics.

Please holler if I got this wrong and think it's something that needs to
be fixed to ensure the "no regressions" rule is adhered.

#regzbot inconclusive: a warn() without serious impact, see discussion
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

> as I'm not really
> sure if I properly understood this thread. It sounded a bit like
> a4e771729a51 should be reverted for now until all
> drm_kms_helper_poll_disable() calls have been verified. Is that right?
> Or did somebody already verify and fix all of them with bugs?
>
> On 28.04.23 03:17, zongmin zhou wrote:
>> On Wed, 2023-04-26 at 16:10 +0300, Dmitry Baryshkov wrote:
>>> On Wed, 26 Apr 2023 at 12:09, zongmin zhou <zhouzongmin@kylinos.cn>
>>> wrote:
>>>> On Sun, 2023-04-23 at 22:51 +0200, Janne Grunau wrote:
>>>>> On 2023-04-20 23:07:01 +0300, Dmitry Baryshkov wrote:
>>>>>> On Thu, 20 Apr 2023 at 23:01, Janne Grunau <j@jannau.net>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 2023-03-28 10:31:29 +0800, Zongmin Zhou wrote:
>>>>>>>> When drivers call drm_kms_helper_poll_disable from
>>>>>>>> their device suspend implementation without enabled output
>>>>>>>> polling before,
>>>>>>>> following warning will be reported,due to work->func not be
>>>>>>>> initialized:
>>>>>>>
>>>>>>> we see the same warning with the wpork in progress kms driver
>>>>>>> for
>>>>>>> apple
>>>>>>> silicon SoCs. The connectors do not need to polled so the
>>>>>>> driver
>>>>>>> never
>>>>>>> calls drm_kms_helper_poll_init().
>>>>>>>
>>>>>>>> [   55.141361] WARNING: CPU: 3 PID: 372 at
>>>>>>>> kernel/workqueue.c:3066 __flush_work+0x22f/0x240
>>>>>>>> [   55.141382] Modules linked in: nls_iso8859_1
>>>>>>>> snd_hda_codec_generic ledtrig_audio snd_hda_intel
>>>>>>>> snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec
>>>>>>>> snd_hda_core
>>>>>>>> snd_hwdep snd_pcm snd_seq_midi snd_seq_midi_event
>>>>>>>> snd_rawmidi
>>>>>>>> snd_seq intel_rapl_msr intel_rapl_common bochs
>>>>>>>> drm_vram_helper
>>>>>>>> drm_ttm_helper snd_seq_device nfit ttm crct10dif_pclmul
>>>>>>>> snd_timer ghash_clmulni_intel binfmt_misc sha512_ssse3
>>>>>>>> aesni_intel drm_kms_helper joydev input_leds syscopyarea
>>>>>>>> crypto_simd snd cryptd sysfillrect sysimgblt mac_hid
>>>>>>>> serio_raw
>>>>>>>> soundcore qemu_fw_cfg sch_fq_codel msr parport_pc ppdev lp
>>>>>>>> parport drm ramoops reed_solomon pstore_blk pstore_zone
>>>>>>>> efi_pstore virtio_rng ip_tables x_tables autofs4
>>>>>>>> hid_generic
>>>>>>>> usbhid hid ahci virtio_net i2c_i801 crc32_pclmul psmouse
>>>>>>>> virtio_scsi libahci i2c_smbus lpc_ich xhci_pci net_failover
>>>>>>>> virtio_blk xhci_pci_renesas failover
>>>>>>>> [   55.141430] CPU: 3 PID: 372 Comm: kworker/u16:9 Not
>>>>>>>> tainted
>>>>>>>> 6.2.0-rc6+ #16
>>>>>>>> [   55.141433] Hardware name: QEMU Standard PC (Q35 + ICH9,
>>>>>>>> 2009), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org
>>>>>>>> 04/01/2014
>>>>>>>> [   55.141435] Workqueue: events_unbound async_run_entry_fn
>>>>>>>> [   55.141441] RIP: 0010:__flush_work+0x22f/0x240
>>>>>>>> [   55.141444] Code: 8b 43 28 48 8b 53 30 89 c1 e9 f9 fe ff
>>>>>>>> ff
>>>>>>>> 4c 89 f7 e8 b5 95 d9 00 e8 00 53 08 00 45 31 ff e9 11 ff ff
>>>>>>>> ff
>>>>>>>> 0f 0b e9 0a ff ff ff <0f> 0b 45 31 ff e9 00 ff ff ff e8 e2
>>>>>>>> 54
>>>>>>>> d8 00 66 90 90 90 90 90 90
>>>>>>>> [   55.141446] RSP: 0018:ff59221940833c18 EFLAGS: 00010246
>>>>>>>> [   55.141449] RAX: 0000000000000000 RBX: 0000000000000000
>>>>>>>> RCX:
>>>>>>>> ffffffff9b72bcbe
>>>>>>>> [   55.141450] RDX: 0000000000000001 RSI: 0000000000000001
>>>>>>>> RDI:
>>>>>>>> ff3ea01e4265e330
>>>>>>>> [   55.141451] RBP: ff59221940833c90 R08: 0000000000000000
>>>>>>>> R09:
>>>>>>>> 8080808080808080
>>>>>>>> [   55.141453] R10: ff3ea01e42b3caf4 R11: 000000000000000f
>>>>>>>> R12:
>>>>>>>> ff3ea01e4265e330
>>>>>>>> [   55.141454] R13: 0000000000000001 R14: ff3ea01e505e5e80
>>>>>>>> R15:
>>>>>>>> 0000000000000001
>>>>>>>> [   55.141455] FS:  0000000000000000(0000)
>>>>>>>> GS:ff3ea01fb7cc0000(0000) knlGS:0000000000000000
>>>>>>>> [   55.141456] CS:  0010 DS: 0000 ES: 0000 CR0:
>>>>>>>> 0000000080050033
>>>>>>>> [   55.141458] CR2: 0000563543ad1546 CR3: 000000010ee82005
>>>>>>>> CR4:
>>>>>>>> 0000000000771ee0
>>>>>>>> [   55.141464] DR0: 0000000000000000 DR1: 0000000000000000
>>>>>>>> DR2:
>>>>>>>> 0000000000000000
>>>>>>>> [   55.141465] DR3: 0000000000000000 DR6: 00000000fffe0ff0
>>>>>>>> DR7:
>>>>>>>> 0000000000000400
>>>>>>>> [   55.141466] PKRU: 55555554
>>>>>>>> [   55.141467] Call Trace:
>>>>>>>> [   55.141469]  <TASK>
>>>>>>>> [   55.141472]  ? pcie_wait_cmd+0xdf/0x220
>>>>>>>> [   55.141478]  ? mptcp_seq_show+0xe0/0x180
>>>>>>>> [   55.141484]  __cancel_work_timer+0x124/0x1b0
>>>>>>>> [   55.141487]  cancel_delayed_work_sync+0x17/0x20
>>>>>>>> [   55.141490]  drm_kms_helper_poll_disable+0x26/0x40
>>>>>>>> [drm_kms_helper]
>>>>>>>> [   55.141516]  drm_mode_config_helper_suspend+0x25/0x90
>>>>>>>> [drm_kms_helper]
>>>>>>>> [   55.141531]  ? __pm_runtime_resume+0x64/0x90
>>>>>>>> [   55.141536]  bochs_pm_suspend+0x16/0x20 [bochs]
>>>>>>>> [   55.141540]  pci_pm_suspend+0x8b/0x1b0
>>>>>>>> [   55.141545]  ? __pfx_pci_pm_suspend+0x10/0x10
>>>>>>>> [   55.141547]  dpm_run_callback+0x4c/0x160
>>>>>>>> [   55.141550]  __device_suspend+0x14c/0x4c0
>>>>>>>> [   55.141553]  async_suspend+0x24/0xa0
>>>>>>>> [   55.141555]  async_run_entry_fn+0x34/0x120
>>>>>>>> [   55.141557]  process_one_work+0x21a/0x3f0
>>>>>>>> [   55.141560]  worker_thread+0x4e/0x3c0
>>>>>>>> [   55.141563]  ? __pfx_worker_thread+0x10/0x10
>>>>>>>> [   55.141565]  kthread+0xf2/0x120
>>>>>>>> [   55.141568]  ? __pfx_kthread+0x10/0x10
>>>>>>>> [   55.141570]  ret_from_fork+0x29/0x50
>>>>>>>> [   55.141575]  </TASK>
>>>>>>>> [   55.141575] ---[ end trace 0000000000000000 ]---
>>>>>>>>
>>>>>>>> Fixes: a4e771729a51 ("drm/probe_helper: sort out
>>>>>>>> poll_running
>>>>>>>> vs poll_enabled")
>>>>>>>> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/drm_probe_helper.c | 3 ++-
>>>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c
>>>>>>>> b/drivers/gpu/drm/drm_probe_helper.c
>>>>>>>> index 8127be134c39..ac72b18e2257 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>>>>> @@ -855,7 +855,8 @@ void drm_kms_helper_poll_disable(struct
>>>>>>>> drm_device *dev)
>>>>>>>>       if (dev->mode_config.poll_running)
>>>>>>>>               drm_kms_helper_disable_hpd(dev);
>>>>>>>>
>>>>>>>> -     cancel_delayed_work_sync(&dev-
>>>>>>>>> mode_config.output_poll_work);
>>>>>>>> +     if (dev->mode_config.poll_enabled)
>>>>>>>> +             cancel_delayed_work_sync(&dev-
>>>>>>>>> mode_config.output_poll_work);
>>>>>>>
>>>>>>> Checking for dev->mode_config.poll_enabled at the start of
>>>>>>> the
>>>>>>> function
>>>>>>> and return early if it is not true looks more in style with
>>>>>>> the
>>>>>>> rest of
>>>>>>> drm_probe_helper.c.
>>>>>>
>>>>>> I think it is an error to call drm_kms_helper_poll_disable() if
>>>>>> polling was not initialized. So, in my opinion the fix should
>>>>>> go to
>>>>>> the drm_mode_config_helper_suspend() / _resume() instead.
>>>>>> Please
>>>>>> add a
>>>>>> guard there using dev->mode_config.poll_enabled.
>>>>>
>>>>> While I tend to agree to the sentiment I do not think this is the
>>>>> correct fix in this situation. drm_kms_helper_poll_disable had
>>>>> the
>>>>> check since at least 2014. a4e771729a51 is a regression. If we
>>>>> want
>>>>> to
>>>>> change the behavior it should be done explicitly and after
>>>>> verifying
>>>>> all
>>>>> drm_kms_helper_poll_disable() calls.
>>>>>
>>>>> #regzbot ^introduced a4e771729a51
>>>>>
>>>>> ciao
>>>>> Janne
>>>>
>>>> Dear Janne:
>>>>
>>>> I agree with you like I mentioned on last letter.
>>>> Thanks for your time.
>>>>
>>>>
>>>> Dear Dmitry:
>>>>
>>>> Is there anything else I can do?
>>>> Looking forward to your reply.
>>>
>>> If it is a common consensus, I'm fine with your approach.
>>>
>> Dear Dmitry:
>>
>> Ok.Thanks for your reply.
>>
>> Best regards!
>>
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 8127be134c39..ac72b18e2257 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -855,7 +855,8 @@  void drm_kms_helper_poll_disable(struct drm_device *dev)
 	if (dev->mode_config.poll_running)
 		drm_kms_helper_disable_hpd(dev);
 
-	cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
+	if (dev->mode_config.poll_enabled)
+		cancel_delayed_work_sync(&dev->mode_config.output_poll_work);
 
 	dev->mode_config.poll_running = false;
 }