Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Rafael J. Wysocki]

On Monday, September 26, 2016 10:15:24 AM Marek Szyprowski wrote:
> Hi Rafael,
> 
> 
> On 2016-09-24 03:25, Rafael J. Wysocki wrote:
> > On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote:
> >> On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> >>> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> >>>> On 2016-09-19 23:45, Tobias Jakobi wrote:
> >>>>> I did some tests with the new version today. Sadly the reboot/shutdown
> >>>>> issues are still present.
> >>>> Thanks for the report. I've managed to reproduce this issue and it is again
> >>>> caused by modifying device on devices_kset list before it will be finally
> >>>> added by device_add(). I thought that the new patchset allows creating
> >>>> links to a device, which has not been yet added to system device list.
> >> Hm, Marek, why isn't it possible to set up the links from the consumer's
> >> ->probe hook in this case?
> 
> Because consumers are unaware of the IOMMU presence, so they are also 
> unaware
> of the links that have to be created.
> 
> >>>> Should it be allowed to create a link to device, which
> >>>> has not yet been added to system device list by device_add()?
> >>> While it would be easy to require both the consumer and producer devices to
> >>> be registered for creating a link between them, that would just make it
> >>> harder to use links in the first place.
> >>>
> >>> So ideally, it should be possible to create links between devices before
> >>> registering them, but since I didn't take that into account in the current
> >>> patch series, some quite substantial changes are needed to cover that.
> >>>
> >>> Additional link states come to mind, but then the "stateless" links are
> >>> affected by this problem too.
> >> device_link_add() could be changed to call device_reorder_to_tail()
> >> only if device_is_registered(consumer) returns true.
> >>
> >> That's an inline function defined in <linux/device.h> which returns
> >> dev->kobj.state_in_sysfs, a flag which is set in kobject_add().
> > I know what that function is, but using it alone is not sufficient,
> > because dev->kobj.state_in_sysfs is set before the device is added to
> > dpm_list.
> 
> I found that checking for dev->p was enough to check if device has been
> added to system or not, but this seems to be some kind of ugly workaround:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4542ba9f60d4..780495918b53 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -180,9 +180,11 @@ struct device_link *device_link_add(struct device 
> *consumer,
>           * It is necessary to hold dpm_list locked throughout all that 
> or else
>           * we may end up suspending with a wrong ordering of it.
>           */
> -       device_pm_lock();
> -       device_reorder_to_tail(consumer, NULL);
> -       device_pm_unlock();
> +       if (consumer->p) {
> +               device_pm_lock();
> +               device_reorder_to_tail(consumer, NULL);
> +               device_pm_unlock();
> +       }

This still is somewhat racy, because the device may not be in dpm_list yet
even if consumer->p is set.

There needs to be something checked and set under device_pm_lock() to avoid
that race.

Let me add it to the patchset and we'll see.

> 
>          list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
>          list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);
> 
> 
> >
> >> Then device_add() would have to check if any links are already
> >> set up and reorder the consumer behind the suppliers.
> >>
> >> Doesn't seem to be *that* complex, but probably I'm missing something,
> >> this is just off the cuff...
> > There are some cases to consider and some races to avoid AFAICS.
> >
> > It all gets a lot simpler if device_link_add() is allowed to return NULL when
> > the supplier device passed to it has not been registered yet.  That looks like
> > a reasonable thing to do to me, but I wonder if someone has a use case in which
> > it would be a substantial limitation.
> 
> Hmmm, you are talking here about the supplier, but my case is that 
> supplier is
> already registered and probed, but the consumer is about to be created. 

Right.

> If you
> think that supporting such case makes no sense,

I think that it does make sense.

I only was wondering if that was going to be sufficient. :-)

Thanks,
Rafael