Bug 1382530 - CONFIG_DEBUG_TEST_DRIVER_REMOVE breaks all the things
Summary: CONFIG_DEBUG_TEST_DRIVER_REMOVE breaks all the things
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: kernel
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Kernel Maintainer List
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1382318 (view as bug list)
Depends On:
Blocks: TRACKER-bugs-affecting-libguestfs
TreeView+ depends on / blocked
 
Reported: 2016-10-07 00:18 UTC by Laura Abbott
Modified: 2018-04-06 18:17 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-04-06 18:17:40 UTC
Type: Bug


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Linux Kernel 177021 None None None 2016-10-07 18:58:24 UTC

Description Laura Abbott 2016-10-07 00:18:24 UTC
This option tests driver removal which is not working at all right now. This is designed to be a bug to dupe other bugs caused by turning on this option.

Comment 1 Laura Abbott 2016-10-07 00:19:30 UTC
*** Bug 1382318 has been marked as a duplicate of this bug. ***

Comment 2 Laura Abbott 2016-10-07 00:21:37 UTC
The option is going to be turned off in the Oct 07 build

Comment 3 Laszlo Ersek 2016-10-07 15:25:20 UTC
This config option, added in

commit bea5b158ff0da9c7246ff391f754f5f38e34577a
Author: Rob Herring <robh@kernel.org>
Date:   Thu Aug 11 10:20:58 2016 -0500

    driver core: add test of driver remove calls during probe
    
    In recent discussions on ksummit-discuss[1], it was suggested to do a
    sequence of probe, remove, probe for testing driver remove paths. This
    adds a kconfig option for said test.
    
    [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003459.html
    
    Suggested-by: Arnd Bergmann <arnd@arndb.de>
    Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Rob Herring <robh@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

seems to be a valid and useful development / cleanup tool; it's just that it probably shouldn't be enabled in production kernels as long as it exposes critical bugs in commonly used drivers. (Clearly the problems exist in those drivers, not in CONFIG_DEBUG_TEST_DRIVER_REMOVE.)

Thank you for pinpointing this, Laura! I'll be sure to retest bug 1366842 with an aarch64 Rawhide nightly ISO after this BZ is addressed.

Comment 4 Laszlo Ersek 2016-10-07 17:51:42 UTC
Actually, Ard Biesheuvel pointed out to me that upstream commit bea5b158ff0d
may not be correct:

On 10/07/16 17:19, Ard Biesheuvel wrote:
> they simply probe a driver twice even if it does not have a .remove()
> method, so the feature itself may need some work as well.

On 10/07/16 17:51, Ard Biesheuvel wrote:

> I [...] noticed that the pci-host-generic driver does not have a remove
> function, which the test code does not seem to deal with

(quoted with permission)

Looking at the patch:

> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec99c7d..fdf44cac08e6 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -212,6 +212,16 @@ config DEBUG_DEVRES
>
>  	  If you are unsure about this, Say N here.
>
> +config DEBUG_TEST_DRIVER_REMOVE
> +	bool "Test driver remove calls during probe"
> +	depends on DEBUG_KERNEL
> +	help
> +	  Say Y here if you want the Driver core to test driver remove functions
> +	  by calling probe, remove, probe. This tests the remove path without
> +	  having to unbind the driver or unload the driver module.
> +
> +	  If you are unsure about this, say N here.
> +
>  config SYS_HYPERVISOR
>  	bool
>  	default n
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 16688f50729c..4910e6db2a34 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -329,6 +329,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = -EPROBE_DEFER;
>  	int local_trigger_count = atomic_read(&deferred_trigger_count);
> +	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE);
>
>  	if (defer_all_probes) {
>  		/*
> @@ -346,6 +347,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  		 drv->bus->name, __func__, drv->name, dev_name(dev));
>  	WARN_ON(!list_empty(&dev->devres_head));
>
> +re_probe:
>  	dev->driver = drv;
>
>  	/* If using pinctrl, bind pins now before probing */
> @@ -383,6 +385,25 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  			goto probe_failed;
>  	}
>
> +	if (test_remove) {
> +		test_remove = false;
> +
> +		if (dev->bus && dev->bus->remove)
> +			dev->bus->remove(dev);
> +		else if (drv->remove)
> +			drv->remove(dev);
> +
> +		devres_release_all(dev);
> +		driver_sysfs_remove(dev);
> +		dev->driver = NULL;
> +		dev_set_drvdata(dev, NULL);
> +		if (dev->pm_domain && dev->pm_domain->dismiss)
> +			dev->pm_domain->dismiss(dev);
> +		pm_runtime_reinit(dev);
> +
> +		goto re_probe;
> +	}
> +
>  	pinctrl_init_done(dev);
>
>  	if (dev->pm_domain && dev->pm_domain->sync)

The code checks for the remove() member functions, but if none of those are
available, it only skips calling them -- it does not skip releasing other
resources, and also doesn't skip calling probe() on the device that is still
bound.

Comment 5 Laszlo Ersek 2016-10-12 15:07:17 UTC
(In reply to Laura Abbott from comment #2)
> The option is going to be turned off in the Oct 07 build

dist-git commit cb6ef876a0ac2bdd3c2c3ca929a318919ff04c7d ("Disable CONFIG_DEBUG_TEST_DRIVER_REMOVE")

Comment 6 Laura Abbott 2018-04-06 18:17:40 UTC
My finest hour.


Note You need to log in before you can comment on or make changes to this bug.