IPU3 cameras not working with latest kernel code ?

IPU3 cameras not working with latest kernel code ?

[Hans de Goede]

Hi All,

While trying to test Kate's tps68470 patches for the privacy LED
on the back of the Surface Go 1/2 I hit this oops when trying
to run qcam:

[  162.502551] BUG: unable to handle page fault for address: 000000010000008f
[  162.502561] #PF: supervisor read access in kernel mode
[  162.502566] #PF: error_code(0x0000) - not-present page
[  162.502570] PGD 0 P4D 0 
[  162.502577] Oops: 0000 [#1] PREEMPT SMP PTI
[  162.502583] CPU: 3 PID: 2030 Comm: qcam Tainted: G         C         6.2.0+ #132
[  162.502589] Hardware name: Microsoft Corporation Surface Go/Surface Go, BIOS 1.0.30 12/22/2020
[  162.502593] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
[  162.502631] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
[  162.502637] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
[  162.502642] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
[  162.502646] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
[  162.502650] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
[  162.502654] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
[  162.502657] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
[  162.502661] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
[  162.502666] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  162.502670] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0
[  162.502675] Call Trace:
[  162.502678]  <TASK>
[  162.502685]  __media_pipeline_start+0x505/0x6f0 [mc]
[  162.502710]  media_pipeline_start+0x32/0x50 [mc]
[  162.502728]  imgu_vb2_start_streaming+0x90/0x270 [ipu3_imgu]
[  162.502750]  vb2_start_streaming+0x6d/0x120 [videobuf2_common]
[  162.502767]  vb2_core_streamon+0xa4/0x100 [videobuf2_common]
[  162.502782]  __video_do_ioctl+0x18b/0x3c0 [videodev]
[  162.502822]  video_usercopy+0x2b9/0x8f0 [videodev]
[  162.502852]  ? __pfx___video_do_ioctl+0x10/0x10 [videodev]
[  162.502895]  ? lock_release+0x135/0x2d0
[  162.502911]  v4l2_ioctl+0x46/0x50 [videodev]
[  162.502937]  __x64_sys_ioctl+0x8d/0xd0
[  162.502946]  do_syscall_64+0x58/0x80
[  162.502956]  ? do_syscall_64+0x67/0x80
[  162.502962]  ? lockdep_hardirqs_on+0x7d/0x100
[  162.502969]  ? do_syscall_64+0x67/0x80
[  162.502975]  ? do_syscall_64+0x67/0x80
[  162.502982]  ? asm_exc_page_fault+0x22/0x30
[  162.502991]  ? lockdep_hardirqs_on+0x7d/0x100
[  162.502997]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  162.503004] RIP: 0033:0x7f8194b23baf
[  162.503010] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  162.503015] RSP: 002b:00007f8181d9c460 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  162.503021] RAX: ffffffffffffffda RBX: 00007f8178011120 RCX: 00007f8194b23baf
[  162.503025] RDX: 00007f8178011350 RSI: 0000000040045612 RDI: 0000000000000010
[  162.503028] RBP: 00007f8178010bd0 R08: 00007f817800db10 R09: 00007f8178016670
[  162.503032] R10: 0000000000000000 R11: 0000000000000246 R12: 00007f81780127f0
[  162.503035] R13: 00007f8178010ab0 R14: 00007f8178010bd0 R15: 00007f8178012950
[  162.503050]  </TASK>
[  162.503053] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_sof_pci_intel_skl snd_sof_intel_hda_common bnep soundwire_intel soundwire_generic_allocation soundwire_cadence snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof dw9719 snd_sof_utils soundwire_bus mei_pxp mei_hdcp snd_soc_avs snd_soc_hda_codec gpio_tps68470 intel_rapl_msr snd_soc_skl snd_soc_hdac_hda snd_hda_ext_core snd_soc_sst_ipc snd_hda_codec_hdmi snd_soc_sst_dsp snd_soc_acpi_intel_match snd_hda_codec_realtek snd_soc_acpi snd_hda_codec_generic ledtrig_audio qrtr snd_soc_core snd_compress ac97_bus intel_tcc_cooling snd_pcm_dmaengine x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel snd_intel_dspcfg kvm_intel ath10k_pci snd_intel_sdw_acpi ath10k_core kvm snd_hda_codec irqbypass rapl intel_cstate intel_uncore mac80211 snd_hda_core btusb snd_hwdep snd_seq btrtl snd_seq_device btbcm intel_wmi_thunderbolt libarc4 pcspkr snd_pcm btintel ath btmtk bluetooth cfg80211 snd_timer snd hid_sensor_als
[  162.503166]  hid_sensor_rotation hid_sensor_gyro_3d hid_sensor_accel_3d ecdh_generic soundcore joydev hid_sensor_trigger hid_sensor_iio_common industrialio_triggered_buffer kfifo_buf rfkill industrialio intel_xhci_usb_role_switch ipu3_imgu(C) ipu3_cio2 processor_thermal_device_pci_legacy processor_thermal_device processor_thermal_rfim processor_thermal_mbox videobuf2_dma_sg mei_me videobuf2_memops mei processor_thermal_rapl intel_rapl_common videobuf2_v4l2 videobuf2_common intel_soc_dts_iosf idma64 intel_pch_thermal intel_skl_int3472_tps68470 leds_tps68470 tps68470_regulator clk_tps68470 ov5693 soc_button_array vfat ov7251 fat ov8865 v4l2_fwnode v4l2_async intel_skl_int3472_discrete videodev int3403_thermal mc int340x_thermal_zone intel_hid int3400_thermal sparse_keymap acpi_thermal_rel acpi_pad zram hid_sensor_hub intel_ishtp_hid i915 mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel drm_buddy drm_display_helper sdhci_pci cqhci rtsx_pci_sdmmc sdhci cec intel_ish_ipc ucsi_acpi
[  162.503269]  ghash_clmulni_intel hid_multitouch sha512_ssse3 typec_ucsi serio_raw mmc_core intel_ishtp rtsx_pci ttm typec i2c_hid_acpi i2c_hid video wmi pinctrl_sunrisepoint ip6_tables ip_tables i2c_dev fuse
[  162.503304] CR2: 000000010000008f
[  162.503309] ---[ end trace 0000000000000000 ]---
[  162.503313] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
[  162.503347] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
[  162.503351] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
[  162.503356] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
[  162.503360] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
[  162.503363] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
[  162.503366] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
[  162.503370] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
[  162.503373] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
[  162.503378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  162.503381] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0

Note this is with a 6.2 kernel with:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

merged into it, which brings in most of media_stage.git/master, so chances are
this is caused by some of recent v4l2-core changes heading towards 6.3 .

I have checked if later media_stage.git/master changes may fix this,
but the only possibly relevant from after I rebased my media-atomisp
on top of media_stage.git/master is:

https://git.linuxtv.org/media_stage.git/commit/?id=a967a3a788028f541e4db54beabcebc3648997db

which does not directly seem likes a likely candidate for this.

Before I spend time on debugging this I was wondering if anyone has any
idea of what might be going on here ?

Also note that the invalid pointer causing this does not look like
a pointer at all, it looks more like 2 32 bit int fields to me?  :

"BUG: unable to handle page fault for address: 000000010000008f"

Regards,

Hans

Re: IPU3 cameras not working with latest kernel code ?

[Sakari Ailus]

Hi Hans,

On Thu, Feb 23, 2023 at 11:04:12PM +0100, Hans de Goede wrote:
> Hi All,
> 
> While trying to test Kate's tps68470 patches for the privacy LED
> on the back of the Surface Go 1/2 I hit this oops when trying
> to run qcam:

Thanks for reporting this.

There have been recent changes in how link validation works, also cc Tomi.

> 
> [  162.502551] BUG: unable to handle page fault for address: 000000010000008f
> [  162.502561] #PF: supervisor read access in kernel mode
> [  162.502566] #PF: error_code(0x0000) - not-present page
> [  162.502570] PGD 0 P4D 0 
> [  162.502577] Oops: 0000 [#1] PREEMPT SMP PTI
> [  162.502583] CPU: 3 PID: 2030 Comm: qcam Tainted: G         C         6.2.0+ #132
> [  162.502589] Hardware name: Microsoft Corporation Surface Go/Surface Go, BIOS 1.0.30 12/22/2020
> [  162.502593] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
> [  162.502631] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
> [  162.502637] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
> [  162.502642] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
> [  162.502646] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
> [  162.502650] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
> [  162.502654] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
> [  162.502657] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
> [  162.502661] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
> [  162.502666] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  162.502670] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0
> [  162.502675] Call Trace:
> [  162.502678]  <TASK>
> [  162.502685]  __media_pipeline_start+0x505/0x6f0 [mc]
> [  162.502710]  media_pipeline_start+0x32/0x50 [mc]
> [  162.502728]  imgu_vb2_start_streaming+0x90/0x270 [ipu3_imgu]
> [  162.502750]  vb2_start_streaming+0x6d/0x120 [videobuf2_common]
> [  162.502767]  vb2_core_streamon+0xa4/0x100 [videobuf2_common]
> [  162.502782]  __video_do_ioctl+0x18b/0x3c0 [videodev]
> [  162.502822]  video_usercopy+0x2b9/0x8f0 [videodev]
> [  162.502852]  ? __pfx___video_do_ioctl+0x10/0x10 [videodev]
> [  162.502895]  ? lock_release+0x135/0x2d0
> [  162.502911]  v4l2_ioctl+0x46/0x50 [videodev]
> [  162.502937]  __x64_sys_ioctl+0x8d/0xd0
> [  162.502946]  do_syscall_64+0x58/0x80
> [  162.502956]  ? do_syscall_64+0x67/0x80
> [  162.502962]  ? lockdep_hardirqs_on+0x7d/0x100
> [  162.502969]  ? do_syscall_64+0x67/0x80
> [  162.502975]  ? do_syscall_64+0x67/0x80
> [  162.502982]  ? asm_exc_page_fault+0x22/0x30
> [  162.502991]  ? lockdep_hardirqs_on+0x7d/0x100
> [  162.502997]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  162.503004] RIP: 0033:0x7f8194b23baf
> [  162.503010] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
> [  162.503015] RSP: 002b:00007f8181d9c460 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [  162.503021] RAX: ffffffffffffffda RBX: 00007f8178011120 RCX: 00007f8194b23baf
> [  162.503025] RDX: 00007f8178011350 RSI: 0000000040045612 RDI: 0000000000000010
> [  162.503028] RBP: 00007f8178010bd0 R08: 00007f817800db10 R09: 00007f8178016670
> [  162.503032] R10: 0000000000000000 R11: 0000000000000246 R12: 00007f81780127f0
> [  162.503035] R13: 00007f8178010ab0 R14: 00007f8178010bd0 R15: 00007f8178012950
> [  162.503050]  </TASK>
> [  162.503053] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_sof_pci_intel_skl snd_sof_intel_hda_common bnep soundwire_intel soundwire_generic_allocation soundwire_cadence snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof dw9719 snd_sof_utils soundwire_bus mei_pxp mei_hdcp snd_soc_avs snd_soc_hda_codec gpio_tps68470 intel_rapl_msr snd_soc_skl snd_soc_hdac_hda snd_hda_ext_core snd_soc_sst_ipc snd_hda_codec_hdmi snd_soc_sst_dsp snd_soc_acpi_intel_match snd_hda_codec_realtek snd_soc_acpi snd_hda_codec_generic ledtrig_audio qrtr snd_soc_core snd_compress ac97_bus intel_tcc_cooling snd_pcm_dmaengine x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel snd_intel_dspcfg kvm_intel ath10k_pci snd_intel_sdw_acpi ath10k_core kvm snd_hda_codec irqbypass rapl intel_cstate intel_uncore mac80211 snd_hda_core btusb snd_hwdep snd_seq btrtl snd_seq_device btbcm intel_wmi_thunderbolt libarc4 pcspkr snd_pcm btintel ath btmtk bluetooth cfg80211 snd_timer snd hid_sensor_als
> [  162.503166]  hid_sensor_rotation hid_sensor_gyro_3d hid_sensor_accel_3d ecdh_generic soundcore joydev hid_sensor_trigger hid_sensor_iio_common industrialio_triggered_buffer kfifo_buf rfkill industrialio intel_xhci_usb_role_switch ipu3_imgu(C) ipu3_cio2 processor_thermal_device_pci_legacy processor_thermal_device processor_thermal_rfim processor_thermal_mbox videobuf2_dma_sg mei_me videobuf2_memops mei processor_thermal_rapl intel_rapl_common videobuf2_v4l2 videobuf2_common intel_soc_dts_iosf idma64 intel_pch_thermal intel_skl_int3472_tps68470 leds_tps68470 tps68470_regulator clk_tps68470 ov5693 soc_button_array vfat ov7251 fat ov8865 v4l2_fwnode v4l2_async intel_skl_int3472_discrete videodev int3403_thermal mc int340x_thermal_zone intel_hid int3400_thermal sparse_keymap acpi_thermal_rel acpi_pad zram hid_sensor_hub intel_ishtp_hid i915 mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel drm_buddy drm_display_helper sdhci_pci cqhci rtsx_pci_sdmmc sdhci cec intel_ish_ipc ucsi_acpi
> [  162.503269]  ghash_clmulni_intel hid_multitouch sha512_ssse3 typec_ucsi serio_raw mmc_core intel_ishtp rtsx_pci ttm typec i2c_hid_acpi i2c_hid video wmi pinctrl_sunrisepoint ip6_tables ip_tables i2c_dev fuse
> [  162.503304] CR2: 000000010000008f
> [  162.503309] ---[ end trace 0000000000000000 ]---
> [  162.503313] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
> [  162.503347] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
> [  162.503351] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
> [  162.503356] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
> [  162.503360] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
> [  162.503363] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
> [  162.503366] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
> [  162.503370] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
> [  162.503373] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
> [  162.503378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  162.503381] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0
> 
> Note this is with a 6.2 kernel with:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp
> 
> merged into it, which brings in most of media_stage.git/master, so chances are
> this is caused by some of recent v4l2-core changes heading towards 6.3 .
> 
> I have checked if later media_stage.git/master changes may fix this,
> but the only possibly relevant from after I rebased my media-atomisp
> on top of media_stage.git/master is:
> 
> https://git.linuxtv.org/media_stage.git/commit/?id=a967a3a788028f541e4db54beabcebc3648997db
> 
> which does not directly seem likes a likely candidate for this.
> 
> Before I spend time on debugging this I was wondering if anyone has any
> idea of what might be going on here ?
> 
> Also note that the invalid pointer causing this does not look like
> a pointer at all, it looks more like 2 32 bit int fields to me?  :
> 
> "BUG: unable to handle page fault for address: 000000010000008f"

If you have a little bit more time, could you enable dynamic debug in
v4l2-subdev to see if it prints anything interesting?

-- 
Kind regards,

Sakari Ailus

Re: IPU3 cameras not working with latest kernel code ?

[Sakari Ailus]

Hi Hans,

On Fri, Feb 24, 2023 at 10:14:00AM +0200, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Feb 23, 2023 at 11:04:12PM +0100, Hans de Goede wrote:
> > Hi All,
> > 
> > While trying to test Kate's tps68470 patches for the privacy LED
> > on the back of the Surface Go 1/2 I hit this oops when trying
> > to run qcam:
> 
> Thanks for reporting this.
> 
> There have been recent changes in how link validation works, also cc Tomi.

It seems link validation is broken in the ImgU driver --- it tries to use
the default link validator that works between two sub-devices but not
between a video node and a sub-device. I guess this somehow got ignored
before Tomi's patches.

It looks like it's calling v4l2_subdev_get_unlocked_active_state() with
NULL argument. No idea why the 32nd bit is set though.

I'll see how to address this.

-- 
Kind regards,

Sakari Ailus

[PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Link validation currently accesses invalid pointers if the link passed to it
is not between two sub-devices. This is of course a driver bug.

Ignore the error but print a debug message, as this is how it used to work
previously.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Hans,

Could you test this?

The bug is of course in the ImgU driver and this reverts to the old
pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
needs to be fixed and I think we could make this return an error at the same
time. Right now I can't be sure the ImgU driver is the only one suffering
from this, but if so, it's likely to be broken anyway.

- Sakari

 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index dff1d9be7841..a6c80096586e 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	struct v4l2_subdev_state *source_state, *sink_state;
 	int ret;
 
+	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->sink->entity->name);
+		return 0;
+	}
+	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->source->entity->name);
+		return 0;
+	}
+
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
 	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
 
-- 
2.30.2

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 02/03/2023 22:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?
> 
> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.

Maybe it should be at least a warn? How do we catch other broken drivers 
otherwise?

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi Tomi,

On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> On 02/03/2023 22:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> > 
> > The bug is of course in the ImgU driver and this reverts to the old
> > pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> > needs to be fixed and I think we could make this return an error at the same
> > time. Right now I can't be sure the ImgU driver is the only one suffering
> > from this, but if so, it's likely to be broken anyway.
> 
> Maybe it should be at least a warn? How do we catch other broken drivers
> otherwise?

The purpose of this patch is just to restore the old behaviour, and merge
it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
error but I'd like that change to be present in the media tree for some
time first.

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 03/03/2023 13:30, Sakari Ailus wrote:
> Hi Tomi,
> 
> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>> Link validation currently accesses invalid pointers if the link passed to it
>>> is not between two sub-devices. This is of course a driver bug.
>>>
>>> Ignore the error but print a debug message, as this is how it used to work
>>> previously.
>>>
>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> Hi Hans,
>>>
>>> Could you test this?
>>>
>>> The bug is of course in the ImgU driver and this reverts to the old
>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>> needs to be fixed and I think we could make this return an error at the same
>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>> from this, but if so, it's likely to be broken anyway.
>>
>> Maybe it should be at least a warn? How do we catch other broken drivers
>> otherwise?
> 
> The purpose of this patch is just to restore the old behaviour, and merge
> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> error but I'd like that change to be present in the media tree for some
> time first.

I meant that keep it returning 0 (no error), but instead of a debug 
print, use pr_warn. Or maybe pr_warn_once for now.

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi,

On 3/3/23 12:36, Tomi Valkeinen wrote:
> On 03/03/2023 13:30, Sakari Ailus wrote:
>> Hi Tomi,
>>
>> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>>> Link validation currently accesses invalid pointers if the link passed to it
>>>> is not between two sub-devices. This is of course a driver bug.
>>>>
>>>> Ignore the error but print a debug message, as this is how it used to work
>>>> previously.
>>>>
>>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>> Hi Hans,
>>>>
>>>> Could you test this?
>>>>
>>>> The bug is of course in the ImgU driver and this reverts to the old
>>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>>> needs to be fixed and I think we could make this return an error at the same
>>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>>> from this, but if so, it's likely to be broken anyway.
>>>
>>> Maybe it should be at least a warn? How do we catch other broken drivers
>>> otherwise?
>>
>> The purpose of this patch is just to restore the old behaviour, and merge
>> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
>> error but I'd like that change to be present in the media tree for some
>> time first.
> 
> I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.

Switching to pr_warn_once() sounds reasonable to me.

I'll try to give this a test-run sometime next week.

Regards,

Hans

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi,

On Fri, Mar 03, 2023 at 04:06:11PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/3/23 12:36, Tomi Valkeinen wrote:
> > On 03/03/2023 13:30, Sakari Ailus wrote:
> >> Hi Tomi,
> >>
> >> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> >>> On 02/03/2023 22:22, Sakari Ailus wrote:
> >>>> Link validation currently accesses invalid pointers if the link passed to it
> >>>> is not between two sub-devices. This is of course a driver bug.
> >>>>
> >>>> Ignore the error but print a debug message, as this is how it used to work
> >>>> previously.
> >>>>
> >>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> >>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> ---
> >>>> Hi Hans,
> >>>>
> >>>> Could you test this?
> >>>>
> >>>> The bug is of course in the ImgU driver and this reverts to the old
> >>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> >>>> needs to be fixed and I think we could make this return an error at the same
> >>>> time. Right now I can't be sure the ImgU driver is the only one suffering
> >>>> from this, but if so, it's likely to be broken anyway.
> >>>
> >>> Maybe it should be at least a warn? How do we catch other broken drivers
> >>> otherwise?
> >>
> >> The purpose of this patch is just to restore the old behaviour, and merge
> >> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> >> error but I'd like that change to be present in the media tree for some
> >> time first.
> > 
> > I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.
> 
> Switching to pr_warn_once() sounds reasonable to me.

I'll send v2 with that.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi Sakari,

On 3/2/23 21:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?

Yes, thank you for the fix.

I can confirm that this fixes the oops on my Surface Go 1 and
that it makes the IPU3 attached cameras work again:

Tested-by: Hans de Goede <hdegoede@redhat.com>

or better just replace the Reported-by with:

Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.
> 
> - Sakari
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index dff1d9be7841..a6c80096586e 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 01:09:11PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 3/2/23 21:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> 
> Yes, thank you for the fix.
> 
> I can confirm that this fixes the oops on my Surface Go 1 and
> that it makes the IPU3 attached cameras work again:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> or better just replace the Reported-by with:
> 
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

I'm putting this to my fixes PR I'm about to send soon.

-- 
Sakari Ailus