linux-samsung-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
       [not found]   ` <CGME20210111111245eucas1p15acde7ecc2ca7f7782beb8ed74c72022@eucas1p1.samsung.com>
@ 2021-01-11 11:12     ` Marek Szyprowski
       [not found]       ` <CGME20210111141814eucas1p1f388df07b789693a999042b27f0d8c2a@eucas1p1.samsung.com>
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2021-01-11 11:12 UTC (permalink / raw)
  To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki
  Cc: kernel-team, linux-kernel, Jisheng Zhang, Kevin Hilman,
	John Stultz, Nicolas Saenz Julienne, Marc Zyngier,
	'Linux Samsung SOC',
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz

Hi Saravana,

On 18.12.2020 04:17, Saravana Kannan wrote:
> Cyclic dependencies in some firmware was one of the last remaining
> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> dependencies don't block probing, set fw_devlink=on by default.
>
> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> only for systems with device tree firmware):
> * Significantly cuts down deferred probes.
> * Device probe is effectively attempted in graph order.
> * Makes it much easier to load drivers as modules without having to
>    worry about functional dependencies between modules (depmod is still
>    needed for symbol dependencies).
>
> If this patch prevents some devices from probing, it's very likely due
> to the system having one or more device drivers that "probe"/set up a
> device (DT node with compatible property) without creating a struct
> device for it.  If we hit such cases, the device drivers need to be
> fixed so that they populate struct devices and probe them like normal
> device drivers so that the driver core is aware of the devices and their
> status. See [1] for an example of such a case.
>
> [1] - https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> Signed-off-by: Saravana Kannan <saravanak@google.com>

This patch landed recently in linux next-20210111 as commit e590474768f1 
("driver core: Set fw_devlink=on by default"). Sadly it breaks Exynos 
IOMMU operation, what causes lots of devices being deferred and not 
probed at all. I've briefly checked and noticed that 
exynos_sysmmu_probe() is never called after this patch. This is really 
strange for me, as the SYSMMU controllers on Exynos platform are regular 
platform devices registered by the OF code. The driver code is here: 
drivers/iommu/exynos-iommu.c, example dts: 
arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").

> ---
>   drivers/base/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4cc030361165..803bfa6eb823 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1457,7 +1457,7 @@ static void device_links_purge(struct device *dev)
>   #define FW_DEVLINK_FLAGS_RPM		(FW_DEVLINK_FLAGS_ON | \
>   					 DL_FLAG_PM_RUNTIME)
>   
> -static u32 fw_devlink_flags = FW_DEVLINK_FLAGS_PERMISSIVE;
> +static u32 fw_devlink_flags = FW_DEVLINK_FLAGS_ON;
>   static int __init fw_devlink_setup(char *arg)
>   {
>   	if (!arg)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
       [not found]       ` <CGME20210111141814eucas1p1f388df07b789693a999042b27f0d8c2a@eucas1p1.samsung.com>
@ 2021-01-11 14:18         ` Marek Szyprowski
  2021-01-11 21:47           ` Saravana Kannan
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2021-01-11 14:18 UTC (permalink / raw)
  To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki
  Cc: kernel-team, linux-kernel, Jisheng Zhang, Kevin Hilman,
	John Stultz, Nicolas Saenz Julienne, Marc Zyngier,
	'Linux Samsung SOC',
	Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz

On 11.01.2021 12:12, Marek Szyprowski wrote:
> On 18.12.2020 04:17, Saravana Kannan wrote:
>> Cyclic dependencies in some firmware was one of the last remaining
>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
>> dependencies don't block probing, set fw_devlink=on by default.
>>
>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
>> only for systems with device tree firmware):
>> * Significantly cuts down deferred probes.
>> * Device probe is effectively attempted in graph order.
>> * Makes it much easier to load drivers as modules without having to
>>    worry about functional dependencies between modules (depmod is still
>>    needed for symbol dependencies).
>>
>> If this patch prevents some devices from probing, it's very likely due
>> to the system having one or more device drivers that "probe"/set up a
>> device (DT node with compatible property) without creating a struct
>> device for it.  If we hit such cases, the device drivers need to be
>> fixed so that they populate struct devices and probe them like normal
>> device drivers so that the driver core is aware of the devices and their
>> status. See [1] for an example of such a case.
>>
>> [1] - 
>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> This patch landed recently in linux next-20210111 as commit 
> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it 
> breaks Exynos IOMMU operation, what causes lots of devices being 
> deferred and not probed at all. I've briefly checked and noticed that 
> exynos_sysmmu_probe() is never called after this patch. This is really 
> strange for me, as the SYSMMU controllers on Exynos platform are 
> regular platform devices registered by the OF code. The driver code is 
> here: drivers/iommu/exynos-iommu.c, example dts: 
> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").

Okay, I found the source of this problem. It is caused by Exynos power 
domain driver, which is not platform driver yet. I will post a patch, 
which converts it to the platform driver.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-11 14:18         ` Marek Szyprowski
@ 2021-01-11 21:47           ` Saravana Kannan
  2021-01-12  7:11             ` Marek Szyprowski
  0 siblings, 1 reply; 10+ messages in thread
From: Saravana Kannan @ 2021-01-11 21:47 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 11.01.2021 12:12, Marek Szyprowski wrote:
> > On 18.12.2020 04:17, Saravana Kannan wrote:
> >> Cyclic dependencies in some firmware was one of the last remaining
> >> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> >> dependencies don't block probing, set fw_devlink=on by default.
> >>
> >> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> >> only for systems with device tree firmware):
> >> * Significantly cuts down deferred probes.
> >> * Device probe is effectively attempted in graph order.
> >> * Makes it much easier to load drivers as modules without having to
> >>    worry about functional dependencies between modules (depmod is still
> >>    needed for symbol dependencies).
> >>
> >> If this patch prevents some devices from probing, it's very likely due
> >> to the system having one or more device drivers that "probe"/set up a
> >> device (DT node with compatible property) without creating a struct
> >> device for it.  If we hit such cases, the device drivers need to be
> >> fixed so that they populate struct devices and probe them like normal
> >> device drivers so that the driver core is aware of the devices and their
> >> status. See [1] for an example of such a case.
> >>
> >> [1] -
> >> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > This patch landed recently in linux next-20210111 as commit
> > e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
> > breaks Exynos IOMMU operation, what causes lots of devices being
> > deferred and not probed at all. I've briefly checked and noticed that
> > exynos_sysmmu_probe() is never called after this patch. This is really
> > strange for me, as the SYSMMU controllers on Exynos platform are
> > regular platform devices registered by the OF code. The driver code is
> > here: drivers/iommu/exynos-iommu.c, example dts:
> > arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
>
> Okay, I found the source of this problem. It is caused by Exynos power
> domain driver, which is not platform driver yet. I will post a patch,
> which converts it to the platform driver.

Thanks Marek! Hopefully the debug logs I added were sufficient to
figure out the reason.

-Saravana

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-11 21:47           ` Saravana Kannan
@ 2021-01-12  7:11             ` Marek Szyprowski
  2021-01-12 20:51               ` Saravana Kannan
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2021-01-12  7:11 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

On 11.01.2021 22:47, Saravana Kannan wrote:
> On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 11.01.2021 12:12, Marek Szyprowski wrote:
>>> On 18.12.2020 04:17, Saravana Kannan wrote:
>>>> Cyclic dependencies in some firmware was one of the last remaining
>>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
>>>> dependencies don't block probing, set fw_devlink=on by default.
>>>>
>>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
>>>> only for systems with device tree firmware):
>>>> * Significantly cuts down deferred probes.
>>>> * Device probe is effectively attempted in graph order.
>>>> * Makes it much easier to load drivers as modules without having to
>>>>     worry about functional dependencies between modules (depmod is still
>>>>     needed for symbol dependencies).
>>>>
>>>> If this patch prevents some devices from probing, it's very likely due
>>>> to the system having one or more device drivers that "probe"/set up a
>>>> device (DT node with compatible property) without creating a struct
>>>> device for it.  If we hit such cases, the device drivers need to be
>>>> fixed so that they populate struct devices and probe them like normal
>>>> device drivers so that the driver core is aware of the devices and their
>>>> status. See [1] for an example of such a case.
>>>>
>>>> [1] -
>>>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>> This patch landed recently in linux next-20210111 as commit
>>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
>>> breaks Exynos IOMMU operation, what causes lots of devices being
>>> deferred and not probed at all. I've briefly checked and noticed that
>>> exynos_sysmmu_probe() is never called after this patch. This is really
>>> strange for me, as the SYSMMU controllers on Exynos platform are
>>> regular platform devices registered by the OF code. The driver code is
>>> here: drivers/iommu/exynos-iommu.c, example dts:
>>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
>> Okay, I found the source of this problem. It is caused by Exynos power
>> domain driver, which is not platform driver yet. I will post a patch,
>> which converts it to the platform driver.
> Thanks Marek! Hopefully the debug logs I added were sufficient to
> figure out the reason.

Frankly, it took me a while to figure out that device core waits for the 
power domain devices. Maybe it would be possible to add some more debug 
messages or hints? Like the reason of the deferred probe in 
/sys/kernel/debug/devices_deferred ?

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-12  7:11             ` Marek Szyprowski
@ 2021-01-12 20:51               ` Saravana Kannan
  2021-01-13  7:04                 ` Marek Szyprowski
  2021-01-18 17:43                 ` Geert Uytterhoeven
  0 siblings, 2 replies; 10+ messages in thread
From: Saravana Kannan @ 2021-01-12 20:51 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 11.01.2021 22:47, Saravana Kannan wrote:
> > On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 11.01.2021 12:12, Marek Szyprowski wrote:
> >>> On 18.12.2020 04:17, Saravana Kannan wrote:
> >>>> Cyclic dependencies in some firmware was one of the last remaining
> >>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> >>>> dependencies don't block probing, set fw_devlink=on by default.
> >>>>
> >>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> >>>> only for systems with device tree firmware):
> >>>> * Significantly cuts down deferred probes.
> >>>> * Device probe is effectively attempted in graph order.
> >>>> * Makes it much easier to load drivers as modules without having to
> >>>>     worry about functional dependencies between modules (depmod is still
> >>>>     needed for symbol dependencies).
> >>>>
> >>>> If this patch prevents some devices from probing, it's very likely due
> >>>> to the system having one or more device drivers that "probe"/set up a
> >>>> device (DT node with compatible property) without creating a struct
> >>>> device for it.  If we hit such cases, the device drivers need to be
> >>>> fixed so that they populate struct devices and probe them like normal
> >>>> device drivers so that the driver core is aware of the devices and their
> >>>> status. See [1] for an example of such a case.
> >>>>
> >>>> [1] -
> >>>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>> This patch landed recently in linux next-20210111 as commit
> >>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
> >>> breaks Exynos IOMMU operation, what causes lots of devices being
> >>> deferred and not probed at all. I've briefly checked and noticed that
> >>> exynos_sysmmu_probe() is never called after this patch. This is really
> >>> strange for me, as the SYSMMU controllers on Exynos platform are
> >>> regular platform devices registered by the OF code. The driver code is
> >>> here: drivers/iommu/exynos-iommu.c, example dts:
> >>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
> >> Okay, I found the source of this problem. It is caused by Exynos power
> >> domain driver, which is not platform driver yet. I will post a patch,
> >> which converts it to the platform driver.
> > Thanks Marek! Hopefully the debug logs I added were sufficient to
> > figure out the reason.
>
> Frankly, it took me a while to figure out that device core waits for the
> power domain devices. Maybe it would be possible to add some more debug
> messages or hints? Like the reason of the deferred probe in
> /sys/kernel/debug/devices_deferred ?

There's already a /sys/devices/.../<device>/waiting_for_supplier file
that tells you if the device is waiting for a supplier device to be
added. That file goes away once the device probes. If the file has 1,
then it's waiting for the supplier device to be added (like your
case). If it's 0, then the device is just waiting on one of the
existing suppliers to probe. You can find the existing suppliers
through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
these dev_dbg() to dev_info() if you need more details about deferred
probing.

https://lore.kernel.org/lkml/20201218031703.3053753-2-saravanak@google.com/

Hopefully this meets what you are looking for?

-Saravana

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-12 20:51               ` Saravana Kannan
@ 2021-01-13  7:04                 ` Marek Szyprowski
  2021-01-13 19:23                   ` Saravana Kannan
  2021-01-18 17:43                 ` Geert Uytterhoeven
  1 sibling, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2021-01-13  7:04 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

Hi Saravana,

On 12.01.2021 21:51, Saravana Kannan wrote:
> On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 11.01.2021 22:47, Saravana Kannan wrote:
>>> On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 11.01.2021 12:12, Marek Szyprowski wrote:
>>>>> On 18.12.2020 04:17, Saravana Kannan wrote:
>>>>>> Cyclic dependencies in some firmware was one of the last remaining
>>>>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
>>>>>> dependencies don't block probing, set fw_devlink=on by default.
>>>>>>
>>>>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
>>>>>> only for systems with device tree firmware):
>>>>>> * Significantly cuts down deferred probes.
>>>>>> * Device probe is effectively attempted in graph order.
>>>>>> * Makes it much easier to load drivers as modules without having to
>>>>>>      worry about functional dependencies between modules (depmod is still
>>>>>>      needed for symbol dependencies).
>>>>>>
>>>>>> If this patch prevents some devices from probing, it's very likely due
>>>>>> to the system having one or more device drivers that "probe"/set up a
>>>>>> device (DT node with compatible property) without creating a struct
>>>>>> device for it.  If we hit such cases, the device drivers need to be
>>>>>> fixed so that they populate struct devices and probe them like normal
>>>>>> device drivers so that the driver core is aware of the devices and their
>>>>>> status. See [1] for an example of such a case.
>>>>>>
>>>>>> [1] -
>>>>>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>> This patch landed recently in linux next-20210111 as commit
>>>>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
>>>>> breaks Exynos IOMMU operation, what causes lots of devices being
>>>>> deferred and not probed at all. I've briefly checked and noticed that
>>>>> exynos_sysmmu_probe() is never called after this patch. This is really
>>>>> strange for me, as the SYSMMU controllers on Exynos platform are
>>>>> regular platform devices registered by the OF code. The driver code is
>>>>> here: drivers/iommu/exynos-iommu.c, example dts:
>>>>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
>>>> Okay, I found the source of this problem. It is caused by Exynos power
>>>> domain driver, which is not platform driver yet. I will post a patch,
>>>> which converts it to the platform driver.
>>> Thanks Marek! Hopefully the debug logs I added were sufficient to
>>> figure out the reason.
>> Frankly, it took me a while to figure out that device core waits for the
>> power domain devices. Maybe it would be possible to add some more debug
>> messages or hints? Like the reason of the deferred probe in
>> /sys/kernel/debug/devices_deferred ?
> There's already a /sys/devices/.../<device>/waiting_for_supplier file
> that tells you if the device is waiting for a supplier device to be
> added. That file goes away once the device probes. If the file has 1,
> then it's waiting for the supplier device to be added (like your
> case). If it's 0, then the device is just waiting on one of the
> existing suppliers to probe. You can find the existing suppliers
> through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
> these dev_dbg() to dev_info() if you need more details about deferred
> probing.

Frankly speaking I doubt that anyone will find those. Even experienced 
developer might need some time to figure it out.

I expect that such information will be at least in the mentioned 
/sys/kernel/debug/devices_deferred file. We already have infrastructure 
for putting the deferred probe reason there, see dev_err_probe() 
function. Even such a simple change makes the debugging this issue much 
easier:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index cd8e518fadd6..ceb5aed5a84c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -937,12 +937,13 @@ int device_links_check_suppliers(struct device *dev)
         mutex_lock(&fwnode_link_lock);
         if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
             !fw_devlink_is_permissive()) {
-               dev_dbg(dev, "probe deferral - wait for supplier %pfwP\n",
+               ret = dev_err_probe(dev, -EPROBE_DEFER,
+                       "probe deferral - wait for supplier %pfwP\n",
list_first_entry(&dev->fwnode->suppliers,
                         struct fwnode_link,
                         c_hook)->supplier);
                 mutex_unlock(&fwnode_link_lock);
-               return -EPROBE_DEFER;
+               return ret;
         }
         mutex_unlock(&fwnode_link_lock);

@@ -955,9 +956,9 @@ int device_links_check_suppliers(struct device *dev)
                 if (link->status != DL_STATE_AVAILABLE &&
                     !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
                         device_links_missing_supplier(dev);
-                       dev_dbg(dev, "probe deferral - supplier %s not 
ready\n",
+                       ret = dev_err_probe(dev, -EPROBE_DEFER,
+                               "probe deferral - supplier %s not ready\n",
                                 dev_name(link->supplier));
-                       ret = -EPROBE_DEFER;
                         break;
                 }
                 WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);


After such change:

# cat /sys/kernet/debug/devices_deferred
sound
13620000.sysmmu platform: probe deferral - supplier 
10023c40.power-domain not ready
13630000.sysmmu platform: probe deferral - supplier 
10023c40.power-domain not ready
12e20000.sysmmu platform: probe deferral - supplier 
10023c20.power-domain not ready
11a20000.sysmmu platform: probe deferral - supplier 
10023c00.power-domain not ready
11a30000.sysmmu platform: probe deferral - supplier 
10023c00.power-domain not ready
11a40000.sysmmu platform: probe deferral - supplier 
10023c00.power-domain not ready
11a50000.sysmmu platform: probe deferral - supplier 
10023c00.power-domain not ready
11a60000.sysmmu platform: probe deferral - supplier 
10023c00.power-domain not ready
11e20000.sysmmu platform: probe deferral - supplier 
10023c80.power-domain not ready
12d00000.hdmi   platform: probe deferral - supplier 
10023c20.power-domain not ready
10048000.clock-controller       platform: probe deferral - supplier 
10023ca0.power-domain not ready
12260000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
12270000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
122a0000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
122b0000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
123b0000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
123c0000.sysmmu platform: probe deferral - supplier 
10048000.clock-controller not ready
12c10000.mixer  platform: probe deferral - supplier 
10023c20.power-domain not ready
13000000.gpu    platform: probe deferral - supplier 
10023c60.power-domain not ready

Probably the message can be adjusted a bit, this would significantly 
help me finding that is the source of the problem.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-13  7:04                 ` Marek Szyprowski
@ 2021-01-13 19:23                   ` Saravana Kannan
  2021-01-14  7:36                     ` Marek Szyprowski
  0 siblings, 1 reply; 10+ messages in thread
From: Saravana Kannan @ 2021-01-13 19:23 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

On Tue, Jan 12, 2021 at 11:04 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 12.01.2021 21:51, Saravana Kannan wrote:
> > On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 11.01.2021 22:47, Saravana Kannan wrote:
> >>> On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 11.01.2021 12:12, Marek Szyprowski wrote:
> >>>>> On 18.12.2020 04:17, Saravana Kannan wrote:
> >>>>>> Cyclic dependencies in some firmware was one of the last remaining
> >>>>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> >>>>>> dependencies don't block probing, set fw_devlink=on by default.
> >>>>>>
> >>>>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> >>>>>> only for systems with device tree firmware):
> >>>>>> * Significantly cuts down deferred probes.
> >>>>>> * Device probe is effectively attempted in graph order.
> >>>>>> * Makes it much easier to load drivers as modules without having to
> >>>>>>      worry about functional dependencies between modules (depmod is still
> >>>>>>      needed for symbol dependencies).
> >>>>>>
> >>>>>> If this patch prevents some devices from probing, it's very likely due
> >>>>>> to the system having one or more device drivers that "probe"/set up a
> >>>>>> device (DT node with compatible property) without creating a struct
> >>>>>> device for it.  If we hit such cases, the device drivers need to be
> >>>>>> fixed so that they populate struct devices and probe them like normal
> >>>>>> device drivers so that the driver core is aware of the devices and their
> >>>>>> status. See [1] for an example of such a case.
> >>>>>>
> >>>>>> [1] -
> >>>>>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> >>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>>> This patch landed recently in linux next-20210111 as commit
> >>>>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
> >>>>> breaks Exynos IOMMU operation, what causes lots of devices being
> >>>>> deferred and not probed at all. I've briefly checked and noticed that
> >>>>> exynos_sysmmu_probe() is never called after this patch. This is really
> >>>>> strange for me, as the SYSMMU controllers on Exynos platform are
> >>>>> regular platform devices registered by the OF code. The driver code is
> >>>>> here: drivers/iommu/exynos-iommu.c, example dts:
> >>>>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
> >>>> Okay, I found the source of this problem. It is caused by Exynos power
> >>>> domain driver, which is not platform driver yet. I will post a patch,
> >>>> which converts it to the platform driver.
> >>> Thanks Marek! Hopefully the debug logs I added were sufficient to
> >>> figure out the reason.
> >> Frankly, it took me a while to figure out that device core waits for the
> >> power domain devices. Maybe it would be possible to add some more debug
> >> messages or hints? Like the reason of the deferred probe in
> >> /sys/kernel/debug/devices_deferred ?
> > There's already a /sys/devices/.../<device>/waiting_for_supplier file
> > that tells you if the device is waiting for a supplier device to be
> > added. That file goes away once the device probes. If the file has 1,
> > then it's waiting for the supplier device to be added (like your
> > case). If it's 0, then the device is just waiting on one of the
> > existing suppliers to probe. You can find the existing suppliers
> > through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
> > these dev_dbg() to dev_info() if you need more details about deferred
> > probing.
>
> Frankly speaking I doubt that anyone will find those. Even experienced
> developer might need some time to figure it out.
>
> I expect that such information will be at least in the mentioned
> /sys/kernel/debug/devices_deferred file. We already have infrastructure
> for putting the deferred probe reason there, see dev_err_probe()
> function. Even such a simple change makes the debugging this issue much
> easier:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index cd8e518fadd6..ceb5aed5a84c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -937,12 +937,13 @@ int device_links_check_suppliers(struct device *dev)
>          mutex_lock(&fwnode_link_lock);
>          if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
>              !fw_devlink_is_permissive()) {
> -               dev_dbg(dev, "probe deferral - wait for supplier %pfwP\n",
> +               ret = dev_err_probe(dev, -EPROBE_DEFER,
> +                       "probe deferral - wait for supplier %pfwP\n",
> list_first_entry(&dev->fwnode->suppliers,
>                          struct fwnode_link,
>                          c_hook)->supplier);
>                  mutex_unlock(&fwnode_link_lock);
> -               return -EPROBE_DEFER;
> +               return ret;
>          }
>          mutex_unlock(&fwnode_link_lock);
>
> @@ -955,9 +956,9 @@ int device_links_check_suppliers(struct device *dev)
>                  if (link->status != DL_STATE_AVAILABLE &&
>                      !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
>                          device_links_missing_supplier(dev);
> -                       dev_dbg(dev, "probe deferral - supplier %s not
> ready\n",
> +                       ret = dev_err_probe(dev, -EPROBE_DEFER,
> +                               "probe deferral - supplier %s not ready\n",
>                                  dev_name(link->supplier));
> -                       ret = -EPROBE_DEFER;
>                          break;
>                  }
>                  WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
>
>
> After such change:
>
> # cat /sys/kernet/debug/devices_deferred

Sweet! I wasn't aware of this file at all.

However, on a side note, one of my TODO items is to not add devices to
the deferred probe list if they'll never probe yet (due to suppliers
not having probed). On a board I tested on, it cut down really_probe()
calls by 75%! So the probe attempt itself effectively happens in graph
order (which I think is pretty cool). So that's going to conflict with
this file. I'll have to see what to do about that.

Thanks for this pointer. Let me sit on this for 2 weeks and see how I
can incorporate your suggestion while allowing for the above. And then
I'll send out a patch. Does that work?

-Saravana

> sound
> 13620000.sysmmu platform: probe deferral - supplier
> 10023c40.power-domain not ready
> 13630000.sysmmu platform: probe deferral - supplier
> 10023c40.power-domain not ready
> 12e20000.sysmmu platform: probe deferral - supplier
> 10023c20.power-domain not ready
> 11a20000.sysmmu platform: probe deferral - supplier
> 10023c00.power-domain not ready
> 11a30000.sysmmu platform: probe deferral - supplier
> 10023c00.power-domain not ready
> 11a40000.sysmmu platform: probe deferral - supplier
> 10023c00.power-domain not ready
> 11a50000.sysmmu platform: probe deferral - supplier
> 10023c00.power-domain not ready
> 11a60000.sysmmu platform: probe deferral - supplier
> 10023c00.power-domain not ready
> 11e20000.sysmmu platform: probe deferral - supplier
> 10023c80.power-domain not ready
> 12d00000.hdmi   platform: probe deferral - supplier
> 10023c20.power-domain not ready
> 10048000.clock-controller       platform: probe deferral - supplier
> 10023ca0.power-domain not ready
> 12260000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 12270000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 122a0000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 122b0000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 123b0000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 123c0000.sysmmu platform: probe deferral - supplier
> 10048000.clock-controller not ready
> 12c10000.mixer  platform: probe deferral - supplier
> 10023c20.power-domain not ready
> 13000000.gpu    platform: probe deferral - supplier
> 10023c60.power-domain not ready
>
> Probably the message can be adjusted a bit, this would significantly
> help me finding that is the source of the problem.
>
> Best regards
>
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-13 19:23                   ` Saravana Kannan
@ 2021-01-14  7:36                     ` Marek Szyprowski
  2021-01-14 18:08                       ` Saravana Kannan
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2021-01-14  7:36 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

Hi Saravana,

On 13.01.2021 20:23, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 11:04 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 12.01.2021 21:51, Saravana Kannan wrote:
>>> On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 11.01.2021 22:47, Saravana Kannan wrote:
>>>>> On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> On 11.01.2021 12:12, Marek Szyprowski wrote:
>>>>>>> On 18.12.2020 04:17, Saravana Kannan wrote:
>>>>>>>> Cyclic dependencies in some firmware was one of the last remaining
>>>>>>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
>>>>>>>> dependencies don't block probing, set fw_devlink=on by default.
>>>>>>>>
>>>>>>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
>>>>>>>> only for systems with device tree firmware):
>>>>>>>> * Significantly cuts down deferred probes.
>>>>>>>> * Device probe is effectively attempted in graph order.
>>>>>>>> * Makes it much easier to load drivers as modules without having to
>>>>>>>>       worry about functional dependencies between modules (depmod is still
>>>>>>>>       needed for symbol dependencies).
>>>>>>>>
>>>>>>>> If this patch prevents some devices from probing, it's very likely due
>>>>>>>> to the system having one or more device drivers that "probe"/set up a
>>>>>>>> device (DT node with compatible property) without creating a struct
>>>>>>>> device for it.  If we hit such cases, the device drivers need to be
>>>>>>>> fixed so that they populate struct devices and probe them like normal
>>>>>>>> device drivers so that the driver core is aware of the devices and their
>>>>>>>> status. See [1] for an example of such a case.
>>>>>>>>
>>>>>>>> [1] -
>>>>>>>> https://protect2.fireeye.com/v1/url?k=68f5d8ba-376ee1f5-68f453f5-0cc47a30d446-324e64700545ab93&q=1&e=fb455b9e-c8c7-40d0-8e3c-d9d3713d519b&u=https%3A%2F%2Flore.kernel.org%2Flkml%2FCAGETcx9PiX%3D%3DmLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw%40mail.gmail.com%2F
>>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>>>> This patch landed recently in linux next-20210111 as commit
>>>>>>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
>>>>>>> breaks Exynos IOMMU operation, what causes lots of devices being
>>>>>>> deferred and not probed at all. I've briefly checked and noticed that
>>>>>>> exynos_sysmmu_probe() is never called after this patch. This is really
>>>>>>> strange for me, as the SYSMMU controllers on Exynos platform are
>>>>>>> regular platform devices registered by the OF code. The driver code is
>>>>>>> here: drivers/iommu/exynos-iommu.c, example dts:
>>>>>>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
>>>>>> Okay, I found the source of this problem. It is caused by Exynos power
>>>>>> domain driver, which is not platform driver yet. I will post a patch,
>>>>>> which converts it to the platform driver.
>>>>> Thanks Marek! Hopefully the debug logs I added were sufficient to
>>>>> figure out the reason.
>>>> Frankly, it took me a while to figure out that device core waits for the
>>>> power domain devices. Maybe it would be possible to add some more debug
>>>> messages or hints? Like the reason of the deferred probe in
>>>> /sys/kernel/debug/devices_deferred ?
>>> There's already a /sys/devices/.../<device>/waiting_for_supplier file
>>> that tells you if the device is waiting for a supplier device to be
>>> added. That file goes away once the device probes. If the file has 1,
>>> then it's waiting for the supplier device to be added (like your
>>> case). If it's 0, then the device is just waiting on one of the
>>> existing suppliers to probe. You can find the existing suppliers
>>> through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
>>> these dev_dbg() to dev_info() if you need more details about deferred
>>> probing.
>> Frankly speaking I doubt that anyone will find those. Even experienced
>> developer might need some time to figure it out.
>>
>> I expect that such information will be at least in the mentioned
>> /sys/kernel/debug/devices_deferred file. We already have infrastructure
>> for putting the deferred probe reason there, see dev_err_probe()
>> function. Even such a simple change makes the debugging this issue much
>> easier:
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index cd8e518fadd6..ceb5aed5a84c 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -937,12 +937,13 @@ int device_links_check_suppliers(struct device *dev)
>>           mutex_lock(&fwnode_link_lock);
>>           if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
>>               !fw_devlink_is_permissive()) {
>> -               dev_dbg(dev, "probe deferral - wait for supplier %pfwP\n",
>> +               ret = dev_err_probe(dev, -EPROBE_DEFER,
>> +                       "probe deferral - wait for supplier %pfwP\n",
>> list_first_entry(&dev->fwnode->suppliers,
>>                           struct fwnode_link,
>>                           c_hook)->supplier);
>>                   mutex_unlock(&fwnode_link_lock);
>> -               return -EPROBE_DEFER;
>> +               return ret;
>>           }
>>           mutex_unlock(&fwnode_link_lock);
>>
>> @@ -955,9 +956,9 @@ int device_links_check_suppliers(struct device *dev)
>>                   if (link->status != DL_STATE_AVAILABLE &&
>>                       !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
>>                           device_links_missing_supplier(dev);
>> -                       dev_dbg(dev, "probe deferral - supplier %s not
>> ready\n",
>> +                       ret = dev_err_probe(dev, -EPROBE_DEFER,
>> +                               "probe deferral - supplier %s not ready\n",
>>                                   dev_name(link->supplier));
>> -                       ret = -EPROBE_DEFER;
>>                           break;
>>                   }
>>                   WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
>>
>>
>> After such change:
>>
>> # cat /sys/kernet/debug/devices_deferred
> Sweet! I wasn't aware of this file at all.
>
> However, on a side note, one of my TODO items is to not add devices to
> the deferred probe list if they'll never probe yet (due to suppliers
> not having probed). On a board I tested on, it cut down really_probe()
> calls by 75%! So the probe attempt itself effectively happens in graph
> order (which I think is pretty cool). So that's going to conflict with
> this file. I'll have to see what to do about that.
>
> Thanks for this pointer. Let me sit on this for 2 weeks and see how I
> can incorporate your suggestion while allowing for the above. And then
> I'll send out a patch. Does that work?

Fine for me.

Even if you want to change the core not to probe devices that miss their 
suppliers (what's good imho), the 'devices_deferred' file might still 
contain all of them. For user it is just a list of devices that are not 
yet available in the system with the optional reasons for that.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-14  7:36                     ` Marek Szyprowski
@ 2021-01-14 18:08                       ` Saravana Kannan
  0 siblings, 0 replies; 10+ messages in thread
From: Saravana Kannan @ 2021-01-14 18:08 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Android Kernel Team, LKML,
	Jisheng Zhang, Kevin Hilman, John Stultz, Nicolas Saenz Julienne,
	Marc Zyngier, Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz

On Wed, Jan 13, 2021 at 11:36 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 13.01.2021 20:23, Saravana Kannan wrote:
> > On Tue, Jan 12, 2021 at 11:04 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 12.01.2021 21:51, Saravana Kannan wrote:
> >>> On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 11.01.2021 22:47, Saravana Kannan wrote:
> >>>>> On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>> On 11.01.2021 12:12, Marek Szyprowski wrote:
> >>>>>>> On 18.12.2020 04:17, Saravana Kannan wrote:
> >>>>>>>> Cyclic dependencies in some firmware was one of the last remaining
> >>>>>>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> >>>>>>>> dependencies don't block probing, set fw_devlink=on by default.
> >>>>>>>>
> >>>>>>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> >>>>>>>> only for systems with device tree firmware):
> >>>>>>>> * Significantly cuts down deferred probes.
> >>>>>>>> * Device probe is effectively attempted in graph order.
> >>>>>>>> * Makes it much easier to load drivers as modules without having to
> >>>>>>>>       worry about functional dependencies between modules (depmod is still
> >>>>>>>>       needed for symbol dependencies).
> >>>>>>>>
> >>>>>>>> If this patch prevents some devices from probing, it's very likely due
> >>>>>>>> to the system having one or more device drivers that "probe"/set up a
> >>>>>>>> device (DT node with compatible property) without creating a struct
> >>>>>>>> device for it.  If we hit such cases, the device drivers need to be
> >>>>>>>> fixed so that they populate struct devices and probe them like normal
> >>>>>>>> device drivers so that the driver core is aware of the devices and their
> >>>>>>>> status. See [1] for an example of such a case.
> >>>>>>>>
> >>>>>>>> [1] -
> >>>>>>>> https://protect2.fireeye.com/v1/url?k=68f5d8ba-376ee1f5-68f453f5-0cc47a30d446-324e64700545ab93&q=1&e=fb455b9e-c8c7-40d0-8e3c-d9d3713d519b&u=https%3A%2F%2Flore.kernel.org%2Flkml%2FCAGETcx9PiX%3D%3DmLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw%40mail.gmail.com%2F
> >>>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>>>>> This patch landed recently in linux next-20210111 as commit
> >>>>>>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
> >>>>>>> breaks Exynos IOMMU operation, what causes lots of devices being
> >>>>>>> deferred and not probed at all. I've briefly checked and noticed that
> >>>>>>> exynos_sysmmu_probe() is never called after this patch. This is really
> >>>>>>> strange for me, as the SYSMMU controllers on Exynos platform are
> >>>>>>> regular platform devices registered by the OF code. The driver code is
> >>>>>>> here: drivers/iommu/exynos-iommu.c, example dts:
> >>>>>>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
> >>>>>> Okay, I found the source of this problem. It is caused by Exynos power
> >>>>>> domain driver, which is not platform driver yet. I will post a patch,
> >>>>>> which converts it to the platform driver.
> >>>>> Thanks Marek! Hopefully the debug logs I added were sufficient to
> >>>>> figure out the reason.
> >>>> Frankly, it took me a while to figure out that device core waits for the
> >>>> power domain devices. Maybe it would be possible to add some more debug
> >>>> messages or hints? Like the reason of the deferred probe in
> >>>> /sys/kernel/debug/devices_deferred ?
> >>> There's already a /sys/devices/.../<device>/waiting_for_supplier file
> >>> that tells you if the device is waiting for a supplier device to be
> >>> added. That file goes away once the device probes. If the file has 1,
> >>> then it's waiting for the supplier device to be added (like your
> >>> case). If it's 0, then the device is just waiting on one of the
> >>> existing suppliers to probe. You can find the existing suppliers
> >>> through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
> >>> these dev_dbg() to dev_info() if you need more details about deferred
> >>> probing.
> >> Frankly speaking I doubt that anyone will find those. Even experienced
> >> developer might need some time to figure it out.
> >>
> >> I expect that such information will be at least in the mentioned
> >> /sys/kernel/debug/devices_deferred file. We already have infrastructure
> >> for putting the deferred probe reason there, see dev_err_probe()
> >> function. Even such a simple change makes the debugging this issue much
> >> easier:
> >>
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index cd8e518fadd6..ceb5aed5a84c 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -937,12 +937,13 @@ int device_links_check_suppliers(struct device *dev)
> >>           mutex_lock(&fwnode_link_lock);
> >>           if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> >>               !fw_devlink_is_permissive()) {
> >> -               dev_dbg(dev, "probe deferral - wait for supplier %pfwP\n",
> >> +               ret = dev_err_probe(dev, -EPROBE_DEFER,
> >> +                       "probe deferral - wait for supplier %pfwP\n",
> >> list_first_entry(&dev->fwnode->suppliers,
> >>                           struct fwnode_link,
> >>                           c_hook)->supplier);
> >>                   mutex_unlock(&fwnode_link_lock);
> >> -               return -EPROBE_DEFER;
> >> +               return ret;
> >>           }
> >>           mutex_unlock(&fwnode_link_lock);
> >>
> >> @@ -955,9 +956,9 @@ int device_links_check_suppliers(struct device *dev)
> >>                   if (link->status != DL_STATE_AVAILABLE &&
> >>                       !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
> >>                           device_links_missing_supplier(dev);
> >> -                       dev_dbg(dev, "probe deferral - supplier %s not
> >> ready\n",
> >> +                       ret = dev_err_probe(dev, -EPROBE_DEFER,
> >> +                               "probe deferral - supplier %s not ready\n",
> >>                                   dev_name(link->supplier));
> >> -                       ret = -EPROBE_DEFER;
> >>                           break;
> >>                   }
> >>                   WRITE_ONCE(link->status, DL_STATE_CONSUMER_PROBE);
> >>
> >>
> >> After such change:
> >>
> >> # cat /sys/kernet/debug/devices_deferred
> > Sweet! I wasn't aware of this file at all.
> >
> > However, on a side note, one of my TODO items is to not add devices to
> > the deferred probe list if they'll never probe yet (due to suppliers
> > not having probed). On a board I tested on, it cut down really_probe()
> > calls by 75%! So the probe attempt itself effectively happens in graph
> > order (which I think is pretty cool). So that's going to conflict with
> > this file. I'll have to see what to do about that.
> >
> > Thanks for this pointer. Let me sit on this for 2 weeks and see how I
> > can incorporate your suggestion while allowing for the above. And then
> > I'll send out a patch. Does that work?
>
> Fine for me.
>
> Even if you want to change the core not to probe devices that miss their
> suppliers (what's good imho), the 'devices_deferred' file might still
> contain all of them. For user it is just a list of devices that are not
> yet available in the system with the optional reasons for that.

Right, I understood that :) My point was that I'm assuming the debugfs
file loops through the deferred devices list. But with my
optimization, it won't find all the devices. So, we might need YET
another list. :-(

-Saravana

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 5/5] driver core: Set fw_devlink=on by default
  2021-01-12 20:51               ` Saravana Kannan
  2021-01-13  7:04                 ` Marek Szyprowski
@ 2021-01-18 17:43                 ` Geert Uytterhoeven
  1 sibling, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2021-01-18 17:43 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Marek Szyprowski, Greg Kroah-Hartman, Rafael J. Wysocki,
	Android Kernel Team, LKML, Jisheng Zhang, Kevin Hilman,
	John Stultz, Nicolas Saenz Julienne, Marc Zyngier,
	Linux Samsung SOC, Krzysztof Kozlowski,
	Bartlomiej Zolnierkiewicz, Yoshihiro Shimoda, Linux-Renesas

Hi Saravana,

On Wed, Jan 13, 2021 at 3:34 AM Saravana Kannan <saravanak@google.com> wrote:
> On Mon, Jan 11, 2021 at 11:11 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > On 11.01.2021 22:47, Saravana Kannan wrote:
> > > On Mon, Jan 11, 2021 at 6:18 AM Marek Szyprowski
> > > <m.szyprowski@samsung.com> wrote:
> > >> On 11.01.2021 12:12, Marek Szyprowski wrote:
> > >>> On 18.12.2020 04:17, Saravana Kannan wrote:
> > >>>> Cyclic dependencies in some firmware was one of the last remaining
> > >>>> reasons fw_devlink=on couldn't be set by default. Now that cyclic
> > >>>> dependencies don't block probing, set fw_devlink=on by default.
> > >>>>
> > >>>> Setting fw_devlink=on by default brings a bunch of benefits (currently,
> > >>>> only for systems with device tree firmware):
> > >>>> * Significantly cuts down deferred probes.
> > >>>> * Device probe is effectively attempted in graph order.
> > >>>> * Makes it much easier to load drivers as modules without having to
> > >>>>     worry about functional dependencies between modules (depmod is still
> > >>>>     needed for symbol dependencies).
> > >>>>
> > >>>> If this patch prevents some devices from probing, it's very likely due
> > >>>> to the system having one or more device drivers that "probe"/set up a
> > >>>> device (DT node with compatible property) without creating a struct
> > >>>> device for it.  If we hit such cases, the device drivers need to be
> > >>>> fixed so that they populate struct devices and probe them like normal
> > >>>> device drivers so that the driver core is aware of the devices and their
> > >>>> status. See [1] for an example of such a case.
> > >>>>
> > >>>> [1] -
> > >>>> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/
> > >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >>> This patch landed recently in linux next-20210111 as commit
> > >>> e590474768f1 ("driver core: Set fw_devlink=on by default"). Sadly it
> > >>> breaks Exynos IOMMU operation, what causes lots of devices being
> > >>> deferred and not probed at all. I've briefly checked and noticed that
> > >>> exynos_sysmmu_probe() is never called after this patch. This is really
> > >>> strange for me, as the SYSMMU controllers on Exynos platform are
> > >>> regular platform devices registered by the OF code. The driver code is
> > >>> here: drivers/iommu/exynos-iommu.c, example dts:
> > >>> arch/arm/boot/dts/exynos3250.dtsi (compatible = "samsung,exynos-sysmmu").
> > >> Okay, I found the source of this problem. It is caused by Exynos power
> > >> domain driver, which is not platform driver yet. I will post a patch,
> > >> which converts it to the platform driver.
> > > Thanks Marek! Hopefully the debug logs I added were sufficient to
> > > figure out the reason.
> >
> > Frankly, it took me a while to figure out that device core waits for the
> > power domain devices. Maybe it would be possible to add some more debug
> > messages or hints? Like the reason of the deferred probe in
> > /sys/kernel/debug/devices_deferred ?
>
> There's already a /sys/devices/.../<device>/waiting_for_supplier file
> that tells you if the device is waiting for a supplier device to be
> added. That file goes away once the device probes. If the file has 1,
> then it's waiting for the supplier device to be added (like your
> case). If it's 0, then the device is just waiting on one of the
> existing suppliers to probe. You can find the existing suppliers
> through /sys/devices/.../<device>/supplier:*/supplier. Also, flip
> these dev_dbg() to dev_info() if you need more details about deferred
> probing.

How are we supposed to check the contents of that file, if the system
doesn't even boot into userspace with a ramdisk? All hardware drivers
fail to probe. The only thing that works is "earlycon keep_bootcon",
and kernel output just stops after a while.

Thanks for your suggestions!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-01-18 17:45 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20201218031703.3053753-1-saravanak@google.com>
     [not found] ` <20201218031703.3053753-6-saravanak@google.com>
     [not found]   ` <CGME20210111111245eucas1p15acde7ecc2ca7f7782beb8ed74c72022@eucas1p1.samsung.com>
2021-01-11 11:12     ` [PATCH v1 5/5] driver core: Set fw_devlink=on by default Marek Szyprowski
     [not found]       ` <CGME20210111141814eucas1p1f388df07b789693a999042b27f0d8c2a@eucas1p1.samsung.com>
2021-01-11 14:18         ` Marek Szyprowski
2021-01-11 21:47           ` Saravana Kannan
2021-01-12  7:11             ` Marek Szyprowski
2021-01-12 20:51               ` Saravana Kannan
2021-01-13  7:04                 ` Marek Szyprowski
2021-01-13 19:23                   ` Saravana Kannan
2021-01-14  7:36                     ` Marek Szyprowski
2021-01-14 18:08                       ` Saravana Kannan
2021-01-18 17:43                 ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).