Re: [PATCH v5 0/5] Functional dependencies between devices

[Rafael J. Wysocki]

Hi,

On Thu, Oct 20, 2016 at 12:21 PM, Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
> Hi Rafael,
>
>
> On 2016-10-19 13:57, Rafael J. Wysocki wrote:
>>
>> On Tue, Oct 18, 2016 at 12:46 PM, Marek Szyprowski
>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>
>>> On 2016-10-10 14:36, Rafael J. Wysocki wrote:
>>>>
>>>> [...]
>>>>
>>>> One more update after some conversations during LinuxCon Europe.
>>>>
>>>> The main point was to make it possible for device_link_add() to figure
>>>> out
>>>> the
>>>> initial state of the link instead of expecting the caller to provide it
>>>> which
>>>> might not be reliable enough in general.
>>>>
>>>> In this version device_link_add() takes three arguments, the supplier
>>>> and
>>>> consumer pointers and flags and it sets the correct initial state of the
>>>> link
>>>> automatically (unless invoked with the "stateless" flag, of course).
>>>> The
>>>> cost
>>>> is one additional field in struct device (I moved all of the
>>>> links-related
>>>> fields in struct device to a separate sub-structure while at it) to
>>>> track
>>>> the "driver presence status" of the device (to be used by
>>>> device_link_add()).
>>>>
>>>> In addition to that, the links list walks in the core.c and dd.c code
>>>> are
>>>> under the device links mutex now, so the iternal link spinlock is not
>>>> needed
>>>> any more and I have renamed symbols to distinguish between flags, link
>>>> states
>>>> and device "driver presence statuses".
>>>>
>>>> More information is there in the changelogs.
>>>
>>> Thanks for the update. This version is indeed easier to use and still
>>> works
>>> fine
>>> with my Exynos IOMMU runtime pm rework. You can keep my:
>>>
>>> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>
>>> I will send updated version of my patchset soon.
>>
>> Thanks for the testing, much appreciated!
>>
>> The series is in a new branch called "device-links-test" in my tree now:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
>> device-links-test
>
>
> While working on integrating IOMMU deferred probing patches I found a bug,
> which has been introduced in v4 of device dependency patchset (v3 worked
> fine in this area, v5 also contains this bug). The following fixup is
> needed to properly create links with DL_FLAG_PM_RUNTIME flag set during
> consumer device probing:
>
> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Date: Thu, 20 Oct 2016 12:12:14 +0200
> Subject: [PATCH] driver core: fix runtime pm state for
>  DEVICE_LINK_CONSUMER_PROBE links
>
> If link is added during consumer probe with DL_FLAG_PM_RUNTIME flag set,
> the code will do additional pm_runtime_put() on the supplier after
> finishing consumer probing. This will break runtime pm operation for
> the supplier device. To solve this issue, enforce additional call to
> pm_runtime_get_sync() on the supplier device while creating such link.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
>  drivers/base/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 48bc5a362f7d..d4cc285a1df7 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -217,6 +217,9 @@ struct device_link *device_link_add(struct device
> *consumer,
>          }
>      }
>
> +    if (flags & DL_FLAG_PM_RUNTIME && status == DL_STATE_CONSUMER_PROBE)
> +        pm_runtime_get_sync(supplier);
> +

Good catch!

I'd prefer to do this slightly differently, though, so I'll send an
update of the runtime PM patch with this folded in shortly.

Thanks,
Rafael