On Thu, Jul 19, 2018 at 12:51 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 18-07-18 19:54, Samuel Morris wrote:
>>
>> Adding Hans de Goede...
>
>
> Thanks, patch 2/3 looks good to me.
>
> I've some remarks inline in this one, note I'm not
> an expert on runtime pm support, it might be good if
> someone more knowledgeable on this subject replies.
>
>
>
>> On Mon, Jul 16, 2018 at 10:41 PM, Sam Morris <0v3rdr0n3@xxxxxxxxx> wrote:
>>>
>>> From: Samuel Morris <samorris@xxxxxxxxxxx>
>>>
>>> This prevents the first resume of the ahci_platform device, while still
>>> allowing parent devices to be resumed.
>>>
>>> Signed-off-by: Samuel Morris <samorris@xxxxxxxxxxx>
>>> ---
>>> drivers/ata/ahci.h | 1 +
>>> drivers/ata/libahci_platform.c | 19 +++++++++++++------
>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index 1609ebab4e23..8ec39f69fae4 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -349,6 +349,7 @@ struct ahci_host_priv {
>>> u32 em_buf_sz; /* EM buffer size in
>>> byte */
>>> u32 em_msg_type; /* EM message type */
>>> bool got_runtime_pm; /* Did we do
>>> pm_runtime_get? */
>>> + bool initialized;
>>> struct clk *clks[AHCI_MAX_CLKS]; /* Optional */
>>> struct regulator **target_pwrs; /* Optional */
>>> /*
>>> diff --git a/drivers/ata/libahci_platform.c
>>> b/drivers/ata/libahci_platform.c
>>> index feee2e11fb33..baad85adf90c 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -476,11 +476,6 @@ struct ahci_host_priv
>>> *ahci_platform_get_resources(struct platform_device *pdev)
>>> goto err_out;
>>> }
>>>
>>> - pm_runtime_set_active(dev);
>
>
> The set_active before enable you are removing here should already avoid
> resume being called on init and thus enable_resources being called
> twice (which I assume is the problem, resume should never be called
> twice itself).
>
>>> - pm_runtime_enable(dev);
>>> - pm_runtime_forbid(dev);
>>> - hpriv->got_runtime_pm = true;
>>> -
>>> devres_remove_group(dev, NULL);
>>> return hpriv;
>>>
>>> @@ -594,7 +589,16 @@ int ahci_platform_init_host(struct platform_device
>>> *pdev,
>>> ahci_init_controller(host);
>>> ahci_print_info(host, "platform");
>>>
>>> - return ahci_host_activate(host, sht);
>>> + rc = ahci_host_activate(host, sht);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + pm_runtime_enable(dev);
>>> + pm_runtime_forbid(dev);
>>> + hpriv->got_runtime_pm = true;
>>> + hpriv->initialized = true;
>>> +
>>> + return 0;
>>> }
>>> EXPORT_SYMBOL_GPL(ahci_platform_init_host);
>>>
>
> Hmm, moving thus from get_resources to platform_init_host while keeping
> the counter parts in put_resources makes get_resources and
> put_resources asymmetrical. As such I'm not a fan of this change.
I don't much like the ordering either, but if runtime_resume is
called, then it needs the platform data, which is not set until
ahci_platform_get_resources returns, naturally. This wouldn't be a
problem if we didn't need to resume the parent devices for these OMAP
devices, but again, I'm just guessing that's what caused the
beagleboard issue, and making sure we do exactly the same operations
on probe as before is my way of sidestepping that problem for now. A
better solution if I'm right perhaps would be to move or copy whatever
resume operations are necessary for those parent devices to their
probe, rather than relying on runtime_pm to initialize the device.
Then, to make sure the parent module loads before ahci_platform, I
typically use modprobe.d, or just build both dependent drivers into
the kernel. Or, at runtime, if you can keep track of that dependency,
you could just defer until it's ready, like the regulator subsystem.
As is though, it might be best to move the runtime_put out of
put_resources and into a new remove callback.
>
> Regards,
>
> Hans
>
>
>
>
>>> @@ -760,6 +764,9 @@ static int _ahci_platform_resume(struct device *dev)
>>> struct ahci_host_priv *hpriv = host->private_data;
>>> int rc;
>>>
>>> + if (!hpriv->initialized)
>>> + return 0;
>>> +
>>> rc = ahci_platform_enable_resources(hpriv);
>>> if (rc)
>>> return rc;
>>> --
>>> 2.17.0
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
