Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490021 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies) "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-23 14:50 +0200
    Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use  device dependencies) Lukas Wunner <lukas@wunner.de> - 2016-09-23 15:50 +0200
      Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies) "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-24 03:20 +0200
        Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device  dependencies) Marek Szyprowski <m.szyprowski@samsung.com> - 2016-09-26 10:20 +0200
          Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies) "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-26 14:30 +0200

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

On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> Hi All,
> 
> 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.
> 
> Rafael:
> What is your opinion?

Well, this is a problem. :-)

> 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.

> My code used to do that, because IOMMUs are configured from 
> of_platform_device_create_pdata()
> of_dma_configure() of_iommu_configure(), which happens before device_add().
> 
> To solve the reported corruption of devices_kset list, following change is
> needed:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index aa8196508db9..4542ba9f60d4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct 
> device *deva, struct device *devb)
>    */
>   void devices_kset_move_last(struct device *dev)
>   {
> +       struct device *d;
> +
>          if (!devices_kset)
>                  return;
>          pr_debug("devices_kset: Moving %s to end of list\n", 
> dev_name(dev));
>          spin_lock(&devices_kset->list_lock);
> -       list_move_tail(&dev->kobj.entry, &devices_kset->list);
> +       list_for_each_entry(d, &devices_kset->list, kobj.entry) {
> +               if (d == dev) {
> +                       list_move_tail(&dev->kobj.entry, 
> &devices_kset->list);
> +                       break;
> +               }
> +       }
>          spin_unlock(&devices_kset->list_lock);
>   }
> 
> 
> If you think that links can be created only to a device, which has been 
> fully
> added to the system, I will register a bus notifier and create a link on 
> consumers
> device probe then.

That would be a workaround for a coverage gap, so not particularly attractive.

Thanks,
Rafael

[toc] | [next] | [standalone]

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

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?


> > 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().

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...

Best regards,

Lukas

[toc] | [prev] | [next] | [standalone]

#1490522

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?
> 
> 
> > > 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.

> 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.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

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

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();
+       }

         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. 
If you
think that supporting such case makes no sense, then I will use the 
workaround
with bus notifier I mentioned earlier.

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

[toc] | [prev] | [next] | [standalone]

#1491251

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web