* [PATCH] media: fix kernel hang in media_device_unregister() during device removal @ 2015-11-12 14:41 Shuah Khan 2015-11-12 16:15 ` Shuah Khan 2015-11-15 23:02 ` Sakari Ailus 0 siblings, 2 replies; 7+ messages in thread From: Shuah Khan @ 2015-11-12 14:41 UTC (permalink / raw) To: mchehab; +Cc: Shuah Khan, linux-media, linux-kernel Media core drivers (dvb, v4l2, bridge driver) unregister their entities calling media_device_unregister_entity() during device removal from their unregister paths. In addition media_device_unregister() tries to unregister entity calling media_device_unregister_entity() for each one of them. This adds lot of contention on mdev->lock in device removal sequence. Fix to not unregister entities from media_device_unregister(), and let drivers take care of it. Drivers need to unregister to cover the case of module removal. This patch fixes the problem by deleting the entity list walk to call media_device_unregister_entity() for each entity. With this fix there is no kernel hang after a sequence of device insertions followed by removal. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/media-device.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 1312e93..c7ab7c9 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); */ void media_device_unregister(struct media_device *mdev) { - struct media_entity *entity; - struct media_entity *next; struct media_link *link, *tmp_link; struct media_interface *intf, *tmp_intf; @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) kfree(intf); } - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) - media_device_unregister_entity(entity); - device_remove_file(&mdev->devnode.dev, &dev_attr_model); media_devnode_unregister(&mdev->devnode); -- 2.5.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-12 14:41 [PATCH] media: fix kernel hang in media_device_unregister() during device removal Shuah Khan @ 2015-11-12 16:15 ` Shuah Khan 2015-11-15 23:02 ` Sakari Ailus 1 sibling, 0 replies; 7+ messages in thread From: Shuah Khan @ 2015-11-12 16:15 UTC (permalink / raw) To: mchehab; +Cc: linux-media, linux-kernel, Shuah Khan On 11/12/2015 07:41 AM, Shuah Khan wrote: > Media core drivers (dvb, v4l2, bridge driver) unregister > their entities calling media_device_unregister_entity() > during device removal from their unregister paths. In > addition media_device_unregister() tries to unregister > entity calling media_device_unregister_entity() for each > one of them. This adds lot of contention on mdev->lock in > device removal sequence. Fix to not unregister entities > from media_device_unregister(), and let drivers take care > of it. Drivers need to unregister to cover the case of > module removal. This patch fixes the problem by deleting > the entity list walk to call media_device_unregister_entity() > for each entity. With this fix there is no kernel hang after > a sequence of device insertions followed by removal. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> This is MC Next Gen patch mc_next_gen.v8.4 branch Sorry I forgot to tag it -- Shuah > --- > drivers/media/media-device.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index 1312e93..c7ab7c9 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); > */ > void media_device_unregister(struct media_device *mdev) > { > - struct media_entity *entity; > - struct media_entity *next; > struct media_link *link, *tmp_link; > struct media_interface *intf, *tmp_intf; > > @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) > kfree(intf); > } > > - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > - media_device_unregister_entity(entity); > - > device_remove_file(&mdev->devnode.dev, &dev_attr_model); > media_devnode_unregister(&mdev->devnode); > > -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shuahkh@osg.samsung.com | (970) 217-8978 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-12 14:41 [PATCH] media: fix kernel hang in media_device_unregister() during device removal Shuah Khan 2015-11-12 16:15 ` Shuah Khan @ 2015-11-15 23:02 ` Sakari Ailus 2015-11-16 18:36 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 7+ messages in thread From: Sakari Ailus @ 2015-11-15 23:02 UTC (permalink / raw) To: Shuah Khan; +Cc: mchehab, linux-media, linux-kernel Hi Shuah, On Thu, Nov 12, 2015 at 07:41:47AM -0700, Shuah Khan wrote: > Media core drivers (dvb, v4l2, bridge driver) unregister > their entities calling media_device_unregister_entity() > during device removal from their unregister paths. In > addition media_device_unregister() tries to unregister > entity calling media_device_unregister_entity() for each > one of them. This adds lot of contention on mdev->lock in > device removal sequence. Fix to not unregister entities > from media_device_unregister(), and let drivers take care > of it. Drivers need to unregister to cover the case of > module removal. This patch fixes the problem by deleting > the entity list walk to call media_device_unregister_entity() > for each entity. With this fix there is no kernel hang after > a sequence of device insertions followed by removal. > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/media-device.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index 1312e93..c7ab7c9 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); > */ > void media_device_unregister(struct media_device *mdev) > { > - struct media_entity *entity; > - struct media_entity *next; > struct media_link *link, *tmp_link; > struct media_interface *intf, *tmp_intf; > > @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) > kfree(intf); > } > > - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > - media_device_unregister_entity(entity); > - > device_remove_file(&mdev->devnode.dev, &dev_attr_model); > media_devnode_unregister(&mdev->devnode); > media_device_unregister() is expected to clean up all the entities still registered with it (as it does to links and interfaces). Could you share details of the problems you saw in case you haven't found the actual underlying issue causing them? -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-15 23:02 ` Sakari Ailus @ 2015-11-16 18:36 ` Mauro Carvalho Chehab 2015-11-16 19:54 ` Shuah Khan 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2015-11-16 18:36 UTC (permalink / raw) To: Sakari Ailus; +Cc: Shuah Khan, linux-media, linux-kernel Em Mon, 16 Nov 2015 01:02:56 +0200 Sakari Ailus <sakari.ailus@iki.fi> escreveu: > Hi Shuah, > > On Thu, Nov 12, 2015 at 07:41:47AM -0700, Shuah Khan wrote: > > Media core drivers (dvb, v4l2, bridge driver) unregister > > their entities calling media_device_unregister_entity() > > during device removal from their unregister paths. In > > addition media_device_unregister() tries to unregister > > entity calling media_device_unregister_entity() for each > > one of them. This adds lot of contention on mdev->lock in > > device removal sequence. Fix to not unregister entities > > from media_device_unregister(), and let drivers take care > > of it. Drivers need to unregister to cover the case of > > module removal. This patch fixes the problem by deleting > > the entity list walk to call media_device_unregister_entity() > > for each entity. With this fix there is no kernel hang after > > a sequence of device insertions followed by removal. > > > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > > --- > > drivers/media/media-device.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > index 1312e93..c7ab7c9 100644 > > --- a/drivers/media/media-device.c > > +++ b/drivers/media/media-device.c > > @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); > > */ > > void media_device_unregister(struct media_device *mdev) > > { > > - struct media_entity *entity; > > - struct media_entity *next; > > struct media_link *link, *tmp_link; > > struct media_interface *intf, *tmp_intf; > > > > @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) > > kfree(intf); > > } > > > > - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > > - media_device_unregister_entity(entity); > > - > > device_remove_file(&mdev->devnode.dev, &dev_attr_model); > > media_devnode_unregister(&mdev->devnode); > > > > media_device_unregister() is expected to clean up all the entities still > registered with it (as it does to links and interfaces). Could you share > details of the problems you saw in case you haven't found the actual > underlying issue causing them? > I was not able to reproduce here with au0828. Tried to register/unregister 20 times with: $ i=1; while :; do echo $i; sudo modprobe au0828 && sleep 2 && sudo rmmod au0828; i=$((i+1)); done The results are at: http://pastebin.com/1B9c9nYm PS.: I had to add the 2 seconds sleep there, as otherwise unregister may fail, because udev is started every time the devnodes are created. Without that, sometimes it returns -EBUSY, because udev didn't close the devnode yet. Still no problem, as a later rmmod works: $ sudo modprobe au0828 && sudo rmmod au0828 $ sudo modprobe au0828 && sudo rmmod au0828 rmmod: ERROR: Module au0828 is in use $ sudo rmmod au0828 $ So, I don't think that the issues you're experiencing are related to the MC next generation. As a reference, those are the modules I'm using on my test machine (after removing the remaining media modules): $ lsmod Module Size Used by cpufreq_powersave 16384 0 cpufreq_conservative 16384 0 cpufreq_userspace 16384 0 cpufreq_stats 16384 0 parport_pc 28672 0 ppdev 20480 0 lp 20480 0 parport 40960 3 lp,ppdev,parport_pc snd_usb_audio 151552 0 snd_hda_codec_hdmi 53248 1 snd_usbmidi_lib 28672 1 snd_usb_audio snd_rawmidi 24576 1 snd_usbmidi_lib snd_seq_device 16384 1 snd_rawmidi btusb 40960 0 btrtl 16384 1 btusb btbcm 16384 1 btusb btintel 16384 1 btusb bluetooth 434176 5 btbcm,btrtl,btusb,btintel rfkill 20480 1 bluetooth i915 1110016 4 intel_rapl 20480 0 iosf_mbi 16384 1 intel_rapl x86_pkg_temp_thermal 16384 0 intel_powerclamp 16384 0 coretemp 16384 0 kvm_intel 163840 0 kvm 446464 1 kvm_intel irqbypass 16384 1 kvm crct10dif_pclmul 16384 0 snd_hda_codec_realtek 65536 1 crc32_pclmul 16384 0 crc32c_intel 24576 0 i2c_algo_bit 16384 1 i915 snd_hda_codec_generic 65536 1 snd_hda_codec_realtek drm_kms_helper 102400 1 i915 snd_hda_intel 32768 0 snd_hda_codec 102400 4 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_intel jitterentropy_rng 16384 0 drm 278528 5 i915,drm_kms_helper sha256_ssse3 32768 1 sha256_generic 24576 1 sha256_ssse3 snd_hwdep 16384 2 snd_usb_audio,snd_hda_codec hmac 16384 1 snd_hda_core 49152 5 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_codec,snd_hda_intel drbg 24576 1 iTCO_wdt 16384 0 snd_pcm 86016 5 snd_usb_audio,snd_hda_codec_hdmi,snd_hda_codec,snd_hda_intel,snd_hda_core iTCO_vendor_support 16384 1 iTCO_wdt evdev 24576 8 aesni_intel 167936 0 snd_timer 28672 1 snd_pcm aes_x86_64 20480 1 aesni_intel psmouse 110592 0 mei_me 28672 0 lrw 16384 1 aesni_intel snd 57344 12 snd_hda_codec_realtek,snd_usb_audio,snd_hwdep,snd_timer,snd_hda_codec_hdmi,snd_pcm,snd_rawmidi,snd_hda_codec_generic,snd_usbmidi_lib,snd_hda_codec,snd_hda_intel,snd_seq_device gf128mul 16384 1 lrw glue_helper 16384 1 aesni_intel mei 81920 1 mei_me ablk_helper 16384 1 aesni_intel cryptd 20480 2 aesni_intel,ablk_helper soundcore 16384 1 snd serio_raw 16384 0 shpchp 32768 0 pcspkr 16384 0 sg 32768 0 i2c_i801 20480 0 lpc_ich 24576 0 mfd_core 16384 1 lpc_ich battery 16384 0 i2c_designware_platform 16384 0 i2c_designware_core 20480 1 i2c_designware_platform dw_dmac 16384 0 tpm_tis 20480 0 video 32768 1 i915 tpm 36864 1 tpm_tis dw_dmac_core 24576 1 dw_dmac button 16384 1 i915 acpi_pad 24576 0 ext4 495616 6 crc16 16384 2 ext4,bluetooth mbcache 20480 1 ext4 jbd2 90112 1 ext4 dm_mod 98304 18 sd_mod 40960 3 ahci 36864 2 libahci 28672 1 ahci libata 192512 2 ahci,libahci ehci_pci 16384 0 scsi_mod 192512 3 sg,libata,sd_mod ehci_hcd 73728 1 ehci_pci e1000e 208896 0 xhci_pci 16384 0 ptp 20480 1 e1000e xhci_hcd 155648 1 xhci_pci pps_core 16384 1 ptp fan 16384 0 thermal 20480 0 sdhci_acpi 16384 0 sdhci 36864 1 sdhci_acpi mmc_core 106496 2 sdhci,sdhci_acpi i2c_hid 20480 0 hid 110592 1 i2c_hid A clear difference is that this machine uses the i915 graph driver, instead of the Radeon driver that you're using on your machine. So, I still think that the problem you've noticed is related to the radeon driver. Please test without it, booting the machine on console mode, in order to isolate eventual issues with the graphics stack. Regards, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-16 18:36 ` Mauro Carvalho Chehab @ 2015-11-16 19:54 ` Shuah Khan 2015-11-17 8:27 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 7+ messages in thread From: Shuah Khan @ 2015-11-16 19:54 UTC (permalink / raw) To: Mauro Carvalho Chehab, Sakari Ailus; +Cc: linux-media, Shuah Khan On 11/16/2015 11:36 AM, Mauro Carvalho Chehab wrote: > Em Mon, 16 Nov 2015 01:02:56 +0200 > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > >> Hi Shuah, >> >> On Thu, Nov 12, 2015 at 07:41:47AM -0700, Shuah Khan wrote: >>> Media core drivers (dvb, v4l2, bridge driver) unregister >>> their entities calling media_device_unregister_entity() >>> during device removal from their unregister paths. In >>> addition media_device_unregister() tries to unregister >>> entity calling media_device_unregister_entity() for each >>> one of them. This adds lot of contention on mdev->lock in >>> device removal sequence. Fix to not unregister entities >>> from media_device_unregister(), and let drivers take care >>> of it. Drivers need to unregister to cover the case of >>> module removal. This patch fixes the problem by deleting >>> the entity list walk to call media_device_unregister_entity() >>> for each entity. With this fix there is no kernel hang after >>> a sequence of device insertions followed by removal. >>> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> drivers/media/media-device.c | 5 ----- >>> 1 file changed, 5 deletions(-) >>> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c >>> index 1312e93..c7ab7c9 100644 >>> --- a/drivers/media/media-device.c >>> +++ b/drivers/media/media-device.c >>> @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); >>> */ >>> void media_device_unregister(struct media_device *mdev) >>> { >>> - struct media_entity *entity; >>> - struct media_entity *next; >>> struct media_link *link, *tmp_link; >>> struct media_interface *intf, *tmp_intf; >>> >>> @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) >>> kfree(intf); >>> } >>> >>> - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) >>> - media_device_unregister_entity(entity); >>> - >>> device_remove_file(&mdev->devnode.dev, &dev_attr_model); >>> media_devnode_unregister(&mdev->devnode); >>> >> >> media_device_unregister() is expected to clean up all the entities still >> registered with it (as it does to links and interfaces). Could you share >> details of the problems you saw in case you haven't found the actual >> underlying issue causing them? >> > > I was not able to reproduce here with au0828. Tried to register/unregister 20 > times with: > $ i=1; while :; do echo $i; sudo modprobe au0828 && sleep 2 && sudo rmmod au0828; i=$((i+1)); done > > The results are at: > http://pastebin.com/1B9c9nYm Mauro, Please remove the device physically to see the problem I am seeing. Also make sure you have the locking debug enabled. I just have to remove the device physically just once to see the hang. > > > PS.: I had to add the 2 seconds sleep there, as otherwise unregister may > fail, because udev is started every time the devnodes are created. Without > that, sometimes it returns -EBUSY, because udev didn't close the devnode > yet. Still no problem, as a later rmmod works: > > $ sudo modprobe au0828 && sudo rmmod au0828 > $ sudo modprobe au0828 && sudo rmmod au0828 > rmmod: ERROR: Module au0828 is in use > $ sudo rmmod au0828 > $ > > So, I don't think that the issues you're experiencing are related to the > MC next generation. On my system it is the case. I have run physical removal of the device on MC and didn't see the problem. > > As a reference, those are the modules I'm using on my test machine > (after removing the remaining media modules): > > $ lsmod > Module Size Used by > cpufreq_powersave 16384 0 > cpufreq_conservative 16384 0 > cpufreq_userspace 16384 0 > cpufreq_stats 16384 0 > parport_pc 28672 0 > ppdev 20480 0 > lp 20480 0 > parport 40960 3 lp,ppdev,parport_pc > snd_usb_audio 151552 0 > snd_hda_codec_hdmi 53248 1 > snd_usbmidi_lib 28672 1 snd_usb_audio > snd_rawmidi 24576 1 snd_usbmidi_lib > snd_seq_device 16384 1 snd_rawmidi > btusb 40960 0 > btrtl 16384 1 btusb > btbcm 16384 1 btusb > btintel 16384 1 btusb > bluetooth 434176 5 btbcm,btrtl,btusb,btintel > rfkill 20480 1 bluetooth > i915 1110016 4 > intel_rapl 20480 0 > iosf_mbi 16384 1 intel_rapl > x86_pkg_temp_thermal 16384 0 > intel_powerclamp 16384 0 > coretemp 16384 0 > kvm_intel 163840 0 > kvm 446464 1 kvm_intel > irqbypass 16384 1 kvm > crct10dif_pclmul 16384 0 > snd_hda_codec_realtek 65536 1 > crc32_pclmul 16384 0 > crc32c_intel 24576 0 > i2c_algo_bit 16384 1 i915 > snd_hda_codec_generic 65536 1 snd_hda_codec_realtek > drm_kms_helper 102400 1 i915 > snd_hda_intel 32768 0 > snd_hda_codec 102400 4 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_intel > jitterentropy_rng 16384 0 > drm 278528 5 i915,drm_kms_helper > sha256_ssse3 32768 1 > sha256_generic 24576 1 sha256_ssse3 > snd_hwdep 16384 2 snd_usb_audio,snd_hda_codec > hmac 16384 1 > snd_hda_core 49152 5 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_codec,snd_hda_intel > drbg 24576 1 > iTCO_wdt 16384 0 > snd_pcm 86016 5 snd_usb_audio,snd_hda_codec_hdmi,snd_hda_codec,snd_hda_intel,snd_hda_core > iTCO_vendor_support 16384 1 iTCO_wdt > evdev 24576 8 > aesni_intel 167936 0 > snd_timer 28672 1 snd_pcm > aes_x86_64 20480 1 aesni_intel > psmouse 110592 0 > mei_me 28672 0 > lrw 16384 1 aesni_intel > snd 57344 12 snd_hda_codec_realtek,snd_usb_audio,snd_hwdep,snd_timer,snd_hda_codec_hdmi,snd_pcm,snd_rawmidi,snd_hda_codec_generic,snd_usbmidi_lib,snd_hda_codec,snd_hda_intel,snd_seq_device > gf128mul 16384 1 lrw > glue_helper 16384 1 aesni_intel > mei 81920 1 mei_me > ablk_helper 16384 1 aesni_intel > cryptd 20480 2 aesni_intel,ablk_helper > soundcore 16384 1 snd > serio_raw 16384 0 > shpchp 32768 0 > pcspkr 16384 0 > sg 32768 0 > i2c_i801 20480 0 > lpc_ich 24576 0 > mfd_core 16384 1 lpc_ich > battery 16384 0 > i2c_designware_platform 16384 0 > i2c_designware_core 20480 1 i2c_designware_platform > dw_dmac 16384 0 > tpm_tis 20480 0 > video 32768 1 i915 > tpm 36864 1 tpm_tis > dw_dmac_core 24576 1 dw_dmac > button 16384 1 i915 > acpi_pad 24576 0 > ext4 495616 6 > crc16 16384 2 ext4,bluetooth > mbcache 20480 1 ext4 > jbd2 90112 1 ext4 > dm_mod 98304 18 > sd_mod 40960 3 > ahci 36864 2 > libahci 28672 1 ahci > libata 192512 2 ahci,libahci > ehci_pci 16384 0 > scsi_mod 192512 3 sg,libata,sd_mod > ehci_hcd 73728 1 ehci_pci > e1000e 208896 0 > xhci_pci 16384 0 > ptp 20480 1 e1000e > xhci_hcd 155648 1 xhci_pci > pps_core 16384 1 ptp > fan 16384 0 > thermal 20480 0 > sdhci_acpi 16384 0 > sdhci 36864 1 sdhci_acpi > mmc_core 106496 2 sdhci,sdhci_acpi > i2c_hid 20480 0 > hid 110592 1 i2c_hid > > A clear difference is that this machine uses the i915 graph driver, > instead of the Radeon driver that you're using on your machine. > > So, I still think that the problem you've noticed is related to the > radeon driver. Please test without it, booting the machine on > console mode, in order to isolate eventual issues with the graphics > stack. Please see above comment on physically removing the device. I am curious to see what happens on your system with lock debug enabled and then remove the device. thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shuahkh@osg.samsung.com | (970) 217-8978 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-16 19:54 ` Shuah Khan @ 2015-11-17 8:27 ` Mauro Carvalho Chehab 2015-11-17 14:12 ` Shuah Khan 0 siblings, 1 reply; 7+ messages in thread From: Mauro Carvalho Chehab @ 2015-11-17 8:27 UTC (permalink / raw) To: Shuah Khan; +Cc: Sakari Ailus, linux-media Em Mon, 16 Nov 2015 12:54:28 -0700 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > On 11/16/2015 11:36 AM, Mauro Carvalho Chehab wrote: > > Em Mon, 16 Nov 2015 01:02:56 +0200 > > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > >> Hi Shuah, > >> > >> On Thu, Nov 12, 2015 at 07:41:47AM -0700, Shuah Khan wrote: > >>> Media core drivers (dvb, v4l2, bridge driver) unregister > >>> their entities calling media_device_unregister_entity() > >>> during device removal from their unregister paths. In > >>> addition media_device_unregister() tries to unregister > >>> entity calling media_device_unregister_entity() for each > >>> one of them. This adds lot of contention on mdev->lock in > >>> device removal sequence. Fix to not unregister entities > >>> from media_device_unregister(), and let drivers take care > >>> of it. Drivers need to unregister to cover the case of > >>> module removal. This patch fixes the problem by deleting > >>> the entity list walk to call media_device_unregister_entity() > >>> for each entity. With this fix there is no kernel hang after > >>> a sequence of device insertions followed by removal. > >>> > >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > >>> --- > >>> drivers/media/media-device.c | 5 ----- > >>> 1 file changed, 5 deletions(-) > >>> > >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > >>> index 1312e93..c7ab7c9 100644 > >>> --- a/drivers/media/media-device.c > >>> +++ b/drivers/media/media-device.c > >>> @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); > >>> */ > >>> void media_device_unregister(struct media_device *mdev) > >>> { > >>> - struct media_entity *entity; > >>> - struct media_entity *next; > >>> struct media_link *link, *tmp_link; > >>> struct media_interface *intf, *tmp_intf; > >>> > >>> @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) > >>> kfree(intf); > >>> } > >>> > >>> - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) > >>> - media_device_unregister_entity(entity); > >>> - > >>> device_remove_file(&mdev->devnode.dev, &dev_attr_model); > >>> media_devnode_unregister(&mdev->devnode); > >>> > >> > >> media_device_unregister() is expected to clean up all the entities still > >> registered with it (as it does to links and interfaces). Could you share > >> details of the problems you saw in case you haven't found the actual > >> underlying issue causing them? > >> > > > > I was not able to reproduce here with au0828. Tried to register/unregister 20 > > times with: > > $ i=1; while :; do echo $i; sudo modprobe au0828 && sleep 2 && sudo rmmod au0828; i=$((i+1)); done > > > > The results are at: > > http://pastebin.com/1B9c9nYm > > Mauro, > > Please remove the device physically to see the problem I am seeing. > Also make sure you have the locking debug enabled. I just have to > remove the device physically just once to see the hang. I did that too: no problem. I can remove/reinsert the device without causing any hangs or any dmesg related to locking issues. > > > > > > > PS.: I had to add the 2 seconds sleep there, as otherwise unregister may > > fail, because udev is started every time the devnodes are created. Without > > that, sometimes it returns -EBUSY, because udev didn't close the devnode > > yet. Still no problem, as a later rmmod works: > > > > $ sudo modprobe au0828 && sudo rmmod au0828 > > $ sudo modprobe au0828 && sudo rmmod au0828 > > rmmod: ERROR: Module au0828 is in use > > $ sudo rmmod au0828 > > $ > > > > So, I don't think that the issues you're experiencing are related to the > > MC next generation. > > On my system it is the case. I have run physical removal of the device > on MC and didn't see the problem. Sorry. Do you mean that rmmod au0828 never works on your system? Then indeed there's something broken with udev and some other module, and the radeon patch you've applied didn't actually solve the bug. > > > > > As a reference, those are the modules I'm using on my test machine > > (after removing the remaining media modules): > > > > $ lsmod > > Module Size Used by > > cpufreq_powersave 16384 0 > > cpufreq_conservative 16384 0 > > cpufreq_userspace 16384 0 > > cpufreq_stats 16384 0 > > parport_pc 28672 0 > > ppdev 20480 0 > > lp 20480 0 > > parport 40960 3 lp,ppdev,parport_pc > > snd_usb_audio 151552 0 > > snd_hda_codec_hdmi 53248 1 > > snd_usbmidi_lib 28672 1 snd_usb_audio > > snd_rawmidi 24576 1 snd_usbmidi_lib > > snd_seq_device 16384 1 snd_rawmidi > > btusb 40960 0 > > btrtl 16384 1 btusb > > btbcm 16384 1 btusb > > btintel 16384 1 btusb > > bluetooth 434176 5 btbcm,btrtl,btusb,btintel > > rfkill 20480 1 bluetooth > > i915 1110016 4 > > intel_rapl 20480 0 > > iosf_mbi 16384 1 intel_rapl > > x86_pkg_temp_thermal 16384 0 > > intel_powerclamp 16384 0 > > coretemp 16384 0 > > kvm_intel 163840 0 > > kvm 446464 1 kvm_intel > > irqbypass 16384 1 kvm > > crct10dif_pclmul 16384 0 > > snd_hda_codec_realtek 65536 1 > > crc32_pclmul 16384 0 > > crc32c_intel 24576 0 > > i2c_algo_bit 16384 1 i915 > > snd_hda_codec_generic 65536 1 snd_hda_codec_realtek > > drm_kms_helper 102400 1 i915 > > snd_hda_intel 32768 0 > > snd_hda_codec 102400 4 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_intel > > jitterentropy_rng 16384 0 > > drm 278528 5 i915,drm_kms_helper > > sha256_ssse3 32768 1 > > sha256_generic 24576 1 sha256_ssse3 > > snd_hwdep 16384 2 snd_usb_audio,snd_hda_codec > > hmac 16384 1 > > snd_hda_core 49152 5 snd_hda_codec_realtek,snd_hda_codec_hdmi,snd_hda_codec_generic,snd_hda_codec,snd_hda_intel > > drbg 24576 1 > > iTCO_wdt 16384 0 > > snd_pcm 86016 5 snd_usb_audio,snd_hda_codec_hdmi,snd_hda_codec,snd_hda_intel,snd_hda_core > > iTCO_vendor_support 16384 1 iTCO_wdt > > evdev 24576 8 > > aesni_intel 167936 0 > > snd_timer 28672 1 snd_pcm > > aes_x86_64 20480 1 aesni_intel > > psmouse 110592 0 > > mei_me 28672 0 > > lrw 16384 1 aesni_intel > > snd 57344 12 snd_hda_codec_realtek,snd_usb_audio,snd_hwdep,snd_timer,snd_hda_codec_hdmi,snd_pcm,snd_rawmidi,snd_hda_codec_generic,snd_usbmidi_lib,snd_hda_codec,snd_hda_intel,snd_seq_device > > gf128mul 16384 1 lrw > > glue_helper 16384 1 aesni_intel > > mei 81920 1 mei_me > > ablk_helper 16384 1 aesni_intel > > cryptd 20480 2 aesni_intel,ablk_helper > > soundcore 16384 1 snd > > serio_raw 16384 0 > > shpchp 32768 0 > > pcspkr 16384 0 > > sg 32768 0 > > i2c_i801 20480 0 > > lpc_ich 24576 0 > > mfd_core 16384 1 lpc_ich > > battery 16384 0 > > i2c_designware_platform 16384 0 > > i2c_designware_core 20480 1 i2c_designware_platform > > dw_dmac 16384 0 > > tpm_tis 20480 0 > > video 32768 1 i915 > > tpm 36864 1 tpm_tis > > dw_dmac_core 24576 1 dw_dmac > > button 16384 1 i915 > > acpi_pad 24576 0 > > ext4 495616 6 > > crc16 16384 2 ext4,bluetooth > > mbcache 20480 1 ext4 > > jbd2 90112 1 ext4 > > dm_mod 98304 18 > > sd_mod 40960 3 > > ahci 36864 2 > > libahci 28672 1 ahci > > libata 192512 2 ahci,libahci > > ehci_pci 16384 0 > > scsi_mod 192512 3 sg,libata,sd_mod > > ehci_hcd 73728 1 ehci_pci > > e1000e 208896 0 > > xhci_pci 16384 0 > > ptp 20480 1 e1000e > > xhci_hcd 155648 1 xhci_pci > > pps_core 16384 1 ptp > > fan 16384 0 > > thermal 20480 0 > > sdhci_acpi 16384 0 > > sdhci 36864 1 sdhci_acpi > > mmc_core 106496 2 sdhci,sdhci_acpi > > i2c_hid 20480 0 > > hid 110592 1 i2c_hid > > > > A clear difference is that this machine uses the i915 graph driver, > > instead of the Radeon driver that you're using on your machine. > > > > So, I still think that the problem you've noticed is related to the > > radeon driver. Please test without it, booting the machine on > > console mode, in order to isolate eventual issues with the graphics > > stack. > > Please see above comment on physically removing the device. I am curious > to see what happens on your system with lock debug enabled and then > remove the device. It works. Regards, Mauro ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: fix kernel hang in media_device_unregister() during device removal 2015-11-17 8:27 ` Mauro Carvalho Chehab @ 2015-11-17 14:12 ` Shuah Khan 0 siblings, 0 replies; 7+ messages in thread From: Shuah Khan @ 2015-11-17 14:12 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media, Shuah Khan On 11/17/2015 01:27 AM, Mauro Carvalho Chehab wrote: > Em Mon, 16 Nov 2015 12:54:28 -0700 > Shuah Khan <shuahkh@osg.samsung.com> escreveu: > >> On 11/16/2015 11:36 AM, Mauro Carvalho Chehab wrote: >>> Em Mon, 16 Nov 2015 01:02:56 +0200 >>> Sakari Ailus <sakari.ailus@iki.fi> escreveu: >>> >>>> Hi Shuah, >>>> >>>> On Thu, Nov 12, 2015 at 07:41:47AM -0700, Shuah Khan wrote: >>>>> Media core drivers (dvb, v4l2, bridge driver) unregister >>>>> their entities calling media_device_unregister_entity() >>>>> during device removal from their unregister paths. In >>>>> addition media_device_unregister() tries to unregister >>>>> entity calling media_device_unregister_entity() for each >>>>> one of them. This adds lot of contention on mdev->lock in >>>>> device removal sequence. Fix to not unregister entities >>>>> from media_device_unregister(), and let drivers take care >>>>> of it. Drivers need to unregister to cover the case of >>>>> module removal. This patch fixes the problem by deleting >>>>> the entity list walk to call media_device_unregister_entity() >>>>> for each entity. With this fix there is no kernel hang after >>>>> a sequence of device insertions followed by removal. >>>>> >>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>>>> --- >>>>> drivers/media/media-device.c | 5 ----- >>>>> 1 file changed, 5 deletions(-) >>>>> >>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c >>>>> index 1312e93..c7ab7c9 100644 >>>>> --- a/drivers/media/media-device.c >>>>> +++ b/drivers/media/media-device.c >>>>> @@ -577,8 +577,6 @@ EXPORT_SYMBOL_GPL(__media_device_register); >>>>> */ >>>>> void media_device_unregister(struct media_device *mdev) >>>>> { >>>>> - struct media_entity *entity; >>>>> - struct media_entity *next; >>>>> struct media_link *link, *tmp_link; >>>>> struct media_interface *intf, *tmp_intf; >>>>> >>>>> @@ -596,9 +594,6 @@ void media_device_unregister(struct media_device *mdev) >>>>> kfree(intf); >>>>> } >>>>> >>>>> - list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) >>>>> - media_device_unregister_entity(entity); >>>>> - >>>>> device_remove_file(&mdev->devnode.dev, &dev_attr_model); >>>>> media_devnode_unregister(&mdev->devnode); >>>>> >>>> >>>> media_device_unregister() is expected to clean up all the entities still >>>> registered with it (as it does to links and interfaces). Could you share >>>> details of the problems you saw in case you haven't found the actual >>>> underlying issue causing them? >>>> >>> >>> I was not able to reproduce here with au0828. Tried to register/unregister 20 >>> times with: >>> $ i=1; while :; do echo $i; sudo modprobe au0828 && sleep 2 && sudo rmmod au0828; i=$((i+1)); done >>> >>> The results are at: >>> http://pastebin.com/1B9c9nYm >> >> Mauro, >> >> Please remove the device physically to see the problem I am seeing. >> Also make sure you have the locking debug enabled. I just have to >> remove the device physically just once to see the hang. > > I did that too: no problem. I can remove/reinsert the device without > causing any hangs or any dmesg related to locking issues. > >> >>> >>> >>> PS.: I had to add the 2 seconds sleep there, as otherwise unregister may >>> fail, because udev is started every time the devnodes are created. Without >>> that, sometimes it returns -EBUSY, because udev didn't close the devnode >>> yet. Still no problem, as a later rmmod works: >>> >>> $ sudo modprobe au0828 && sudo rmmod au0828 >>> $ sudo modprobe au0828 && sudo rmmod au0828 >>> rmmod: ERROR: Module au0828 is in use >>> $ sudo rmmod au0828 >>> $ >>> >>> So, I don't think that the issues you're experiencing are related to the >>> MC next generation. >> >> On my system it is the case. I have run physical removal of the device >> on MC and didn't see the problem. > > Sorry. Do you mean that rmmod au0828 never works on your system? No physical removal not rmmod that causes problems. > > Then indeed there's something broken with udev and some other module, > and the radeon patch you've applied didn't actually solve the bug. > Right - I will try without radeon to see if the problem persists. I think we are good based on your tests. This problem might indeed be an isolated one specific my setup. Thanks for testing on your system. -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shuahkh@osg.samsung.com | (970) 217-8978 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-17 14:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-12 14:41 [PATCH] media: fix kernel hang in media_device_unregister() during device removal Shuah Khan 2015-11-12 16:15 ` Shuah Khan 2015-11-15 23:02 ` Sakari Ailus 2015-11-16 18:36 ` Mauro Carvalho Chehab 2015-11-16 19:54 ` Shuah Khan 2015-11-17 8:27 ` Mauro Carvalho Chehab 2015-11-17 14:12 ` Shuah Khan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.