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

["Rafael J. Wysocki" <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org>]

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