| Message ID | 20180716224103.7912-4-0v3rdr0n3@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Adding Hans de Goede... On Mon, Jul 16, 2018 at 10:41 PM, Sam Morris <0v3rdr0n3@gmail.com> wrote: > From: Samuel Morris <samorris@lexmark.com> > > This prevents the first resume of the ahci_platform device, while still > allowing parent devices to be resumed. > > Signed-off-by: Samuel Morris <samorris@lexmark.com> > --- > 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); > - 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); > > @@ -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 >
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@gmail.com> wrote: >> From: Samuel Morris <samorris@lexmark.com> >> >> This prevents the first resume of the ahci_platform device, while still >> allowing parent devices to be resumed. >> >> Signed-off-by: Samuel Morris <samorris@lexmark.com> >> --- >> 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. 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 >>
On Thu, Jul 19, 2018 at 12:51 PM, Hans de Goede <hdegoede@redhat.com> 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@gmail.com> wrote: >>> >>> From: Samuel Morris <samorris@lexmark.com> >>> >>> This prevents the first resume of the ahci_platform device, while still >>> allowing parent devices to be resumed. >>> >>> Signed-off-by: Samuel Morris <samorris@lexmark.com> >>> --- >>> 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 >>> >
+Tero/Tony, On 19/07/18 20:43, Samuel Morris wrote: > On Thu, Jul 19, 2018 at 12:51 PM, Hans de Goede <hdegoede@redhat.com> 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@gmail.com> wrote: >>>> >>>> From: Samuel Morris <samorris@lexmark.com> >>>> >>>> This prevents the first resume of the ahci_platform device, while still >>>> allowing parent devices to be resumed. >>>> >>>> Signed-off-by: Samuel Morris <samorris@lexmark.com> >>>> --- >>>> 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. Is this something that can be fixed in OMAP PM/bus code? It seems like OMAP is behaving oddly here and requires the driver to do some weird things. If we ensure that the device is enabled by bus driver during probe then it should make things easier for the device driver. > >> >> 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 >>>> >>
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); - 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); @@ -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;