| Message ID | 20180919191518.18764-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Resume BYT/CHT LPSS I2C controller before the iGPU | expand |
On 19/09/18 22:15, Hans de Goede wrote: > Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card > dependency on I2C") tries to add device links from consumer to > supplier *and* the other way around. That is not the intention. The intention is to add a link irrespective of which device (consumer or supplier) is created last. > > When the consumer gets registered the supplier is not yet registered, so > acpi_lpss_find_device() cannot find it and we never end up adding the > supplier link. Although not intended this is a good thing, because the > device links are dependencies and if we add a link in both directions the > power-management core will not know which one to suspend/resume first. Did that actually happen? It seems to me that could only happen if the _DEP methods also pointed to each other. > > I've verified through debug printk-s that indeed only the consumer > to supplier dependency gets added even before this commit removes > the code to add the link the other way. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpi_lpss.c | 25 ------------------------- > 1 file changed, 25 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index 83875305b1d4..40a8cab81cbd 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -486,13 +486,6 @@ static bool acpi_lpss_is_supplier(struct acpi_device *adev, > link->supplier_hid, link->supplier_uid); > } > > -static bool acpi_lpss_is_consumer(struct acpi_device *adev, > - const struct lpss_device_links *link) > -{ > - return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev), > - link->consumer_hid, link->consumer_uid); > -} > - > struct hid_uid { > const char *hid; > const char *uid; > @@ -559,21 +552,6 @@ static void acpi_lpss_link_consumer(struct device *dev1, > put_device(dev2); > } > > -static void acpi_lpss_link_supplier(struct device *dev1, > - const struct lpss_device_links *link) > -{ > - struct device *dev2; > - > - dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid); > - if (!dev2) > - return; > - > - if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2))) > - device_link_add(dev1, dev2, link->flags); > - > - put_device(dev2); > -} > - > static void acpi_lpss_create_device_links(struct acpi_device *adev, > struct platform_device *pdev) > { > @@ -584,9 +562,6 @@ static void acpi_lpss_create_device_links(struct acpi_device *adev, > > if (acpi_lpss_is_supplier(adev, link)) > acpi_lpss_link_consumer(&pdev->dev, link); > - > - if (acpi_lpss_is_consumer(adev, link)) > - acpi_lpss_link_supplier(&pdev->dev, link); > } > } > >
Hi, On 09/21/2018 03:01 PM, Adrian Hunter wrote: > On 19/09/18 22:15, Hans de Goede wrote: >> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card >> dependency on I2C") tries to add device links from consumer to >> supplier *and* the other way around. > > That is not the intention. The intention is to add a link irrespective of > which device (consumer or supplier) is created last. Ah I see an upon rereading the code you are right. >> When the consumer gets registered the supplier is not yet registered, so >> acpi_lpss_find_device() cannot find it and we never end up adding the >> supplier link. Although not intended this is a good thing, because the >> device links are dependencies and if we add a link in both directions the >> power-management core will not know which one to suspend/resume first. > > Did that actually happen? It seems to me that could only happen if the _DEP > methods also pointed to each other. You are right again. So I guess that this patch can be dropped. Note that for the new GPU -> PMIC I2C controller consumer -> supplier link this patchset adds we rely on probe ordering. Relying on probe ordering works for the SDCARD -> PMIC I2C too, so we could keep this patch as it does simplify the code. If we do that the commit message needs to be corrected of course. Regards, Hans >> I've verified through debug printk-s that indeed only the consumer >> to supplier dependency gets added even before this commit removes >> the code to add the link the other way. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/acpi_lpss.c | 25 ------------------------- >> 1 file changed, 25 deletions(-) >> >> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c >> index 83875305b1d4..40a8cab81cbd 100644 >> --- a/drivers/acpi/acpi_lpss.c >> +++ b/drivers/acpi/acpi_lpss.c >> @@ -486,13 +486,6 @@ static bool acpi_lpss_is_supplier(struct acpi_device *adev, >> link->supplier_hid, link->supplier_uid); >> } >> >> -static bool acpi_lpss_is_consumer(struct acpi_device *adev, >> - const struct lpss_device_links *link) >> -{ >> - return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev), >> - link->consumer_hid, link->consumer_uid); >> -} >> - >> struct hid_uid { >> const char *hid; >> const char *uid; >> @@ -559,21 +552,6 @@ static void acpi_lpss_link_consumer(struct device *dev1, >> put_device(dev2); >> } >> >> -static void acpi_lpss_link_supplier(struct device *dev1, >> - const struct lpss_device_links *link) >> -{ >> - struct device *dev2; >> - >> - dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid); >> - if (!dev2) >> - return; >> - >> - if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2))) >> - device_link_add(dev1, dev2, link->flags); >> - >> - put_device(dev2); >> -} >> - >> static void acpi_lpss_create_device_links(struct acpi_device *adev, >> struct platform_device *pdev) >> { >> @@ -584,9 +562,6 @@ static void acpi_lpss_create_device_links(struct acpi_device *adev, >> >> if (acpi_lpss_is_supplier(adev, link)) >> acpi_lpss_link_consumer(&pdev->dev, link); >> - >> - if (acpi_lpss_is_consumer(adev, link)) >> - acpi_lpss_link_supplier(&pdev->dev, link); >> } >> } >> >> >
On 21/09/18 17:03, Hans de Goede wrote: > Hi, > > On 09/21/2018 03:01 PM, Adrian Hunter wrote: >> On 19/09/18 22:15, Hans de Goede wrote: >>> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card >>> dependency on I2C") tries to add device links from consumer to >>> supplier *and* the other way around. >> >> That is not the intention. The intention is to add a link irrespective of >> which device (consumer or supplier) is created last. > > Ah I see an upon rereading the code you are right. > >>> When the consumer gets registered the supplier is not yet registered, so >>> acpi_lpss_find_device() cannot find it and we never end up adding the >>> supplier link. Although not intended this is a good thing, because the >>> device links are dependencies and if we add a link in both directions the >>> power-management core will not know which one to suspend/resume first. >> >> Did that actually happen? It seems to me that could only happen if the _DEP >> methods also pointed to each other. > > You are right again. > > So I guess that this patch can be dropped. > > Note that for the new GPU -> PMIC I2C controller consumer -> supplier link this > patchset adds we rely on probe ordering. Relying on probe ordering works > for the SDCARD -> PMIC I2C too, so we could keep this patch as it does > simplify the code. If we do that the commit message needs to be corrected > of course. IIRC the ACPI platform devices end up being created in the order the device nodes appear in the DSDT, so no guarantee of consumer before supplier. > > Regards, > > Hans > > >>> I've verified through debug printk-s that indeed only the consumer >>> to supplier dependency gets added even before this commit removes >>> the code to add the link the other way. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/acpi/acpi_lpss.c | 25 ------------------------- >>> 1 file changed, 25 deletions(-) >>> >>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c >>> index 83875305b1d4..40a8cab81cbd 100644 >>> --- a/drivers/acpi/acpi_lpss.c >>> +++ b/drivers/acpi/acpi_lpss.c >>> @@ -486,13 +486,6 @@ static bool acpi_lpss_is_supplier(struct acpi_device >>> *adev, >>> link->supplier_hid, link->supplier_uid); >>> } >>> -static bool acpi_lpss_is_consumer(struct acpi_device *adev, >>> - const struct lpss_device_links *link) >>> -{ >>> - return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev), >>> - link->consumer_hid, link->consumer_uid); >>> -} >>> - >>> struct hid_uid { >>> const char *hid; >>> const char *uid; >>> @@ -559,21 +552,6 @@ static void acpi_lpss_link_consumer(struct device >>> *dev1, >>> put_device(dev2); >>> } >>> -static void acpi_lpss_link_supplier(struct device *dev1, >>> - const struct lpss_device_links *link) >>> -{ >>> - struct device *dev2; >>> - >>> - dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid); >>> - if (!dev2) >>> - return; >>> - >>> - if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2))) >>> - device_link_add(dev1, dev2, link->flags); >>> - >>> - put_device(dev2); >>> -} >>> - >>> static void acpi_lpss_create_device_links(struct acpi_device *adev, >>> struct platform_device *pdev) >>> { >>> @@ -584,9 +562,6 @@ static void acpi_lpss_create_device_links(struct >>> acpi_device *adev, >>> if (acpi_lpss_is_supplier(adev, link)) >>> acpi_lpss_link_consumer(&pdev->dev, link); >>> - >>> - if (acpi_lpss_is_consumer(adev, link)) >>> - acpi_lpss_link_supplier(&pdev->dev, link); >>> } >>> } >>> >> >
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 83875305b1d4..40a8cab81cbd 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -486,13 +486,6 @@ static bool acpi_lpss_is_supplier(struct acpi_device *adev, link->supplier_hid, link->supplier_uid); } -static bool acpi_lpss_is_consumer(struct acpi_device *adev, - const struct lpss_device_links *link) -{ - return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev), - link->consumer_hid, link->consumer_uid); -} - struct hid_uid { const char *hid; const char *uid; @@ -559,21 +552,6 @@ static void acpi_lpss_link_consumer(struct device *dev1, put_device(dev2); } -static void acpi_lpss_link_supplier(struct device *dev1, - const struct lpss_device_links *link) -{ - struct device *dev2; - - dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid); - if (!dev2) - return; - - if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2))) - device_link_add(dev1, dev2, link->flags); - - put_device(dev2); -} - static void acpi_lpss_create_device_links(struct acpi_device *adev, struct platform_device *pdev) { @@ -584,9 +562,6 @@ static void acpi_lpss_create_device_links(struct acpi_device *adev, if (acpi_lpss_is_supplier(adev, link)) acpi_lpss_link_consumer(&pdev->dev, link); - - if (acpi_lpss_is_consumer(adev, link)) - acpi_lpss_link_supplier(&pdev->dev, link); } }
Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card dependency on I2C") tries to add device links from consumer to supplier *and* the other way around. When the consumer gets registered the supplier is not yet registered, so acpi_lpss_find_device() cannot find it and we never end up adding the supplier link. Although not intended this is a good thing, because the device links are dependencies and if we add a link in both directions the power-management core will not know which one to suspend/resume first. I've verified through debug printk-s that indeed only the consumer to supplier dependency gets added even before this commit removes the code to add the link the other way. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpi_lpss.c | 25 ------------------------- 1 file changed, 25 deletions(-)