Drivers using legacy power management .suspen()/.resume() callbacks
have to manage PCI states and device's PM states themselves. They also
need to take care of standard configuration registers.
Switch to generic power management framework using a single
"struct dev_pm_ops" variable to take the unnecessary load from the driver.
This also avoids the need for the driver to directly call most of the PCI
helper functions and device power state control functions as through
the generic framework, PCI Core takes care of the necessary operations,
and drivers are required to do only device-specific jobs.
Signed-off-by: Vaibhav Gupta <[email protected]>
---
drivers/crypto/ccp/ccp-dev.c | 8 +++-----
drivers/crypto/ccp/sp-dev.c | 6 ++----
drivers/crypto/ccp/sp-dev.h | 6 +++---
drivers/crypto/ccp/sp-pci.c | 17 ++++++-----------
drivers/crypto/ccp/sp-platform.c | 2 +-
5 files changed, 15 insertions(+), 24 deletions(-)
diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
index 19ac509ed76e..8ae26d3cffff 100644
--- a/drivers/crypto/ccp/ccp-dev.c
+++ b/drivers/crypto/ccp/ccp-dev.c
@@ -531,8 +531,7 @@ int ccp_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
return len;
}
-#ifdef CONFIG_PM
-bool ccp_queues_suspended(struct ccp_device *ccp)
+bool __maybe_unused ccp_queues_suspended(struct ccp_device *ccp)
{
unsigned int suspended = 0;
unsigned long flags;
@@ -549,7 +548,7 @@ bool ccp_queues_suspended(struct ccp_device *ccp)
return ccp->cmd_q_count == suspended;
}
-int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
+int __maybe_unused ccp_dev_suspend(struct sp_device *sp)
{
struct ccp_device *ccp = sp->ccp_data;
unsigned long flags;
@@ -577,7 +576,7 @@ int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
return 0;
}
-int ccp_dev_resume(struct sp_device *sp)
+int __maybe_unused ccp_dev_resume(struct sp_device *sp)
{
struct ccp_device *ccp = sp->ccp_data;
unsigned long flags;
@@ -601,7 +600,6 @@ int ccp_dev_resume(struct sp_device *sp)
return 0;
}
-#endif
int ccp_dev_init(struct sp_device *sp)
{
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index ce42675d3274..6284a15e5047 100644
--- a/drivers/crypto/ccp/sp-dev.c
+++ b/drivers/crypto/ccp/sp-dev.c
@@ -211,13 +211,12 @@ void sp_destroy(struct sp_device *sp)
sp_del_device(sp);
}
-#ifdef CONFIG_PM
-int sp_suspend(struct sp_device *sp, pm_message_t state)
+int sp_suspend(struct sp_device *sp)
{
int ret;
if (sp->dev_vdata->ccp_vdata) {
- ret = ccp_dev_suspend(sp, state);
+ ret = ccp_dev_suspend(sp);
if (ret)
return ret;
}
@@ -237,7 +236,6 @@ int sp_resume(struct sp_device *sp)
return 0;
}
-#endif
struct sp_device *sp_get_psp_master_device(void)
{
diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
index f913f1494af9..0218d0670eee 100644
--- a/drivers/crypto/ccp/sp-dev.h
+++ b/drivers/crypto/ccp/sp-dev.h
@@ -119,7 +119,7 @@ int sp_init(struct sp_device *sp);
void sp_destroy(struct sp_device *sp);
struct sp_device *sp_get_master(void);
-int sp_suspend(struct sp_device *sp, pm_message_t state);
+int sp_suspend(struct sp_device *sp);
int sp_resume(struct sp_device *sp);
int sp_request_ccp_irq(struct sp_device *sp, irq_handler_t handler,
const char *name, void *data);
@@ -134,7 +134,7 @@ struct sp_device *sp_get_psp_master_device(void);
int ccp_dev_init(struct sp_device *sp);
void ccp_dev_destroy(struct sp_device *sp);
-int ccp_dev_suspend(struct sp_device *sp, pm_message_t state);
+int ccp_dev_suspend(struct sp_device *sp);
int ccp_dev_resume(struct sp_device *sp);
#else /* !CONFIG_CRYPTO_DEV_SP_CCP */
@@ -145,7 +145,7 @@ static inline int ccp_dev_init(struct sp_device *sp)
}
static inline void ccp_dev_destroy(struct sp_device *sp) { }
-static inline int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
+static inline int ccp_dev_suspend(struct sp_device *sp)
{
return 0;
}
diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
index cb6cb47053f4..f471dbaef1fb 100644
--- a/drivers/crypto/ccp/sp-pci.c
+++ b/drivers/crypto/ccp/sp-pci.c
@@ -252,23 +252,19 @@ static void sp_pci_remove(struct pci_dev *pdev)
sp_free_irqs(sp);
}
-#ifdef CONFIG_PM
-static int sp_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused sp_pci_suspend(struct device *dev)
{
- struct device *dev = &pdev->dev;
struct sp_device *sp = dev_get_drvdata(dev);
- return sp_suspend(sp, state);
+ return sp_suspend(sp);
}
-static int sp_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused sp_pci_resume(struct device *dev)
{
- struct device *dev = &pdev->dev;
struct sp_device *sp = dev_get_drvdata(dev);
return sp_resume(sp);
}
-#endif
#ifdef CONFIG_CRYPTO_DEV_SP_PSP
static const struct sev_vdata sevv1 = {
@@ -365,15 +361,14 @@ static const struct pci_device_id sp_pci_table[] = {
};
MODULE_DEVICE_TABLE(pci, sp_pci_table);
+static SIMPLE_DEV_PM_OPS(sp_pci_pm_ops, sp_pci_suspend, sp_pci_resume);
+
static struct pci_driver sp_pci_driver = {
.name = "ccp",
.id_table = sp_pci_table,
.probe = sp_pci_probe,
.remove = sp_pci_remove,
-#ifdef CONFIG_PM
- .suspend = sp_pci_suspend,
- .resume = sp_pci_resume,
-#endif
+ .driver.pm = &sp_pci_pm_ops,
};
int sp_pci_init(void)
diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
index 831aac1393a2..9dba52fbee99 100644
--- a/drivers/crypto/ccp/sp-platform.c
+++ b/drivers/crypto/ccp/sp-platform.c
@@ -207,7 +207,7 @@ static int sp_platform_suspend(struct platform_device *pdev,
struct device *dev = &pdev->dev;
struct sp_device *sp = dev_get_drvdata(dev);
- return sp_suspend(sp, state);
+ return sp_suspend(sp);
}
static int sp_platform_resume(struct platform_device *pdev)
--
2.27.0
On 7/21/20 7:31 AM, Vaibhav Gupta wrote:
> Drivers using legacy power management .suspen()/.resume() callbacks
> have to manage PCI states and device's PM states themselves. They also
> need to take care of standard configuration registers.
>
> Switch to generic power management framework using a single
> "struct dev_pm_ops" variable to take the unnecessary load from the driver.
> This also avoids the need for the driver to directly call most of the PCI
> helper functions and device power state control functions as through
> the generic framework, PCI Core takes care of the necessary operations,
> and drivers are required to do only device-specific jobs.
>
> Signed-off-by: Vaibhav Gupta <[email protected]>
> ---
> drivers/crypto/ccp/ccp-dev.c | 8 +++-----
> drivers/crypto/ccp/sp-dev.c | 6 ++----
> drivers/crypto/ccp/sp-dev.h | 6 +++---
> drivers/crypto/ccp/sp-pci.c | 17 ++++++-----------
> drivers/crypto/ccp/sp-platform.c | 2 +-
> 5 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
> index 19ac509ed76e..8ae26d3cffff 100644
> --- a/drivers/crypto/ccp/ccp-dev.c
> +++ b/drivers/crypto/ccp/ccp-dev.c
> @@ -531,8 +531,7 @@ int ccp_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> return len;
> }
>
> -#ifdef CONFIG_PM
> -bool ccp_queues_suspended(struct ccp_device *ccp)
> +bool __maybe_unused ccp_queues_suspended(struct ccp_device *ccp)
These aren't static functions, so is the __maybe_unused necessary?
(Same comment on the other non-static functions changed below)
> {
> unsigned int suspended = 0;
> unsigned long flags;
> @@ -549,7 +548,7 @@ bool ccp_queues_suspended(struct ccp_device *ccp)
> return ccp->cmd_q_count == suspended;
> }
>
> -int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> +int __maybe_unused ccp_dev_suspend(struct sp_device *sp)
> {
> struct ccp_device *ccp = sp->ccp_data;
> unsigned long flags;
> @@ -577,7 +576,7 @@ int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> return 0;
> }
>
> -int ccp_dev_resume(struct sp_device *sp)
> +int __maybe_unused ccp_dev_resume(struct sp_device *sp)
> {
> struct ccp_device *ccp = sp->ccp_data;
> unsigned long flags;
> @@ -601,7 +600,6 @@ int ccp_dev_resume(struct sp_device *sp)
>
> return 0;
> }
> -#endif
>
> int ccp_dev_init(struct sp_device *sp)
> {
> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
> index ce42675d3274..6284a15e5047 100644
> --- a/drivers/crypto/ccp/sp-dev.c
> +++ b/drivers/crypto/ccp/sp-dev.c
> @@ -211,13 +211,12 @@ void sp_destroy(struct sp_device *sp)
> sp_del_device(sp);
> }
>
> -#ifdef CONFIG_PM
> -int sp_suspend(struct sp_device *sp, pm_message_t state)
> +int sp_suspend(struct sp_device *sp)
> {
> int ret;
>
> if (sp->dev_vdata->ccp_vdata) {
> - ret = ccp_dev_suspend(sp, state);
> + ret = ccp_dev_suspend(sp);
> if (ret)
> return ret;
> }
> @@ -237,7 +236,6 @@ int sp_resume(struct sp_device *sp)
>
> return 0;
> }
> -#endif
>
> struct sp_device *sp_get_psp_master_device(void)
> {
> diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
> index f913f1494af9..0218d0670eee 100644
> --- a/drivers/crypto/ccp/sp-dev.h
> +++ b/drivers/crypto/ccp/sp-dev.h
> @@ -119,7 +119,7 @@ int sp_init(struct sp_device *sp);
> void sp_destroy(struct sp_device *sp);
> struct sp_device *sp_get_master(void);
>
> -int sp_suspend(struct sp_device *sp, pm_message_t state);
> +int sp_suspend(struct sp_device *sp);
> int sp_resume(struct sp_device *sp);
> int sp_request_ccp_irq(struct sp_device *sp, irq_handler_t handler,
> const char *name, void *data);
> @@ -134,7 +134,7 @@ struct sp_device *sp_get_psp_master_device(void);
> int ccp_dev_init(struct sp_device *sp);
> void ccp_dev_destroy(struct sp_device *sp);
>
> -int ccp_dev_suspend(struct sp_device *sp, pm_message_t state);
> +int ccp_dev_suspend(struct sp_device *sp);
> int ccp_dev_resume(struct sp_device *sp);
>
> #else /* !CONFIG_CRYPTO_DEV_SP_CCP */
> @@ -145,7 +145,7 @@ static inline int ccp_dev_init(struct sp_device *sp)
> }
> static inline void ccp_dev_destroy(struct sp_device *sp) { }
>
> -static inline int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> +static inline int ccp_dev_suspend(struct sp_device *sp)
> {
> return 0;
> }
> diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
> index cb6cb47053f4..f471dbaef1fb 100644
> --- a/drivers/crypto/ccp/sp-pci.c
> +++ b/drivers/crypto/ccp/sp-pci.c
> @@ -252,23 +252,19 @@ static void sp_pci_remove(struct pci_dev *pdev)
> sp_free_irqs(sp);
> }
>
> -#ifdef CONFIG_PM
> -static int sp_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused sp_pci_suspend(struct device *dev)
> {
> - struct device *dev = &pdev->dev;
> struct sp_device *sp = dev_get_drvdata(dev);
>
> - return sp_suspend(sp, state);
> + return sp_suspend(sp);
> }
>
> -static int sp_pci_resume(struct pci_dev *pdev)
> +static int __maybe_unused sp_pci_resume(struct device *dev)
> {
> - struct device *dev = &pdev->dev;
> struct sp_device *sp = dev_get_drvdata(dev);
>
> return sp_resume(sp);
> }
> -#endif
>
> #ifdef CONFIG_CRYPTO_DEV_SP_PSP
> static const struct sev_vdata sevv1 = {
> @@ -365,15 +361,14 @@ static const struct pci_device_id sp_pci_table[] = {
> };
> MODULE_DEVICE_TABLE(pci, sp_pci_table);
>
> +static SIMPLE_DEV_PM_OPS(sp_pci_pm_ops, sp_pci_suspend, sp_pci_resume);
> +
> static struct pci_driver sp_pci_driver = {
> .name = "ccp",
> .id_table = sp_pci_table,
> .probe = sp_pci_probe,
> .remove = sp_pci_remove,
> -#ifdef CONFIG_PM
> - .suspend = sp_pci_suspend,
> - .resume = sp_pci_resume,
> -#endif
> + .driver.pm = &sp_pci_pm_ops,
> };
>
> int sp_pci_init(void)
> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> index 831aac1393a2..9dba52fbee99 100644
> --- a/drivers/crypto/ccp/sp-platform.c
> +++ b/drivers/crypto/ccp/sp-platform.c
This file use the same "#ifdef CONFIG_PM" to define the suspend and resume
functions (see sp_platform_driver variable). Doesn't that need to be
updated similar to sp-pci.c? Not sure how this compile tested successfully
unless your kernel config didn't have CONFIG_PM defined?
Thanks,
Tom
> @@ -207,7 +207,7 @@ static int sp_platform_suspend(struct platform_device *pdev,
> struct device *dev = &pdev->dev;
> struct sp_device *sp = dev_get_drvdata(dev);
>
> - return sp_suspend(sp, state);
> + return sp_suspend(sp);
> }
>
> static int sp_platform_resume(struct platform_device *pdev)
>
On Tue, Jul 21, 2020 at 10:19:33AM -0500, Tom Lendacky wrote:
> On 7/21/20 7:31 AM, Vaibhav Gupta wrote:
> > Drivers using legacy power management .suspen()/.resume() callbacks
> > have to manage PCI states and device's PM states themselves. They also
> > need to take care of standard configuration registers.
> >
> > Switch to generic power management framework using a single
> > "struct dev_pm_ops" variable to take the unnecessary load from the driver.
> > This also avoids the need for the driver to directly call most of the PCI
> > helper functions and device power state control functions as through
> > the generic framework, PCI Core takes care of the necessary operations,
> > and drivers are required to do only device-specific jobs.
> >
> > Signed-off-by: Vaibhav Gupta <[email protected]>
> > ---
> > drivers/crypto/ccp/ccp-dev.c | 8 +++-----
> > drivers/crypto/ccp/sp-dev.c | 6 ++----
> > drivers/crypto/ccp/sp-dev.h | 6 +++---
> > drivers/crypto/ccp/sp-pci.c | 17 ++++++-----------
> > drivers/crypto/ccp/sp-platform.c | 2 +-
> > 5 files changed, 15 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c
> > index 19ac509ed76e..8ae26d3cffff 100644
> > --- a/drivers/crypto/ccp/ccp-dev.c
> > +++ b/drivers/crypto/ccp/ccp-dev.c
> > @@ -531,8 +531,7 @@ int ccp_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> > return len;
> > }
> >
> > -#ifdef CONFIG_PM
> > -bool ccp_queues_suspended(struct ccp_device *ccp)
> > +bool __maybe_unused ccp_queues_suspended(struct ccp_device *ccp)
>
> These aren't static functions, so is the __maybe_unused necessary?
>
> (Same comment on the other non-static functions changed below)
>
> > {
> > unsigned int suspended = 0;
> > unsigned long flags;
> > @@ -549,7 +548,7 @@ bool ccp_queues_suspended(struct ccp_device *ccp)
> > return ccp->cmd_q_count == suspended;
> > }
> >
> > -int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> > +int __maybe_unused ccp_dev_suspend(struct sp_device *sp)
> > {
> > struct ccp_device *ccp = sp->ccp_data;
> > unsigned long flags;
> > @@ -577,7 +576,7 @@ int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> > return 0;
> > }
> >
> > -int ccp_dev_resume(struct sp_device *sp)
> > +int __maybe_unused ccp_dev_resume(struct sp_device *sp)
> > {
> > struct ccp_device *ccp = sp->ccp_data;
> > unsigned long flags;
> > @@ -601,7 +600,6 @@ int ccp_dev_resume(struct sp_device *sp)
> >
> > return 0;
> > }
> > -#endif
> >
> > int ccp_dev_init(struct sp_device *sp)
> > {
> > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
> > index ce42675d3274..6284a15e5047 100644
> > --- a/drivers/crypto/ccp/sp-dev.c
> > +++ b/drivers/crypto/ccp/sp-dev.c
> > @@ -211,13 +211,12 @@ void sp_destroy(struct sp_device *sp)
> > sp_del_device(sp);
> > }
> >
> > -#ifdef CONFIG_PM
> > -int sp_suspend(struct sp_device *sp, pm_message_t state)
> > +int sp_suspend(struct sp_device *sp)
> > {
> > int ret;
> >
> > if (sp->dev_vdata->ccp_vdata) {
> > - ret = ccp_dev_suspend(sp, state);
> > + ret = ccp_dev_suspend(sp);
> > if (ret)
> > return ret;
> > }
> > @@ -237,7 +236,6 @@ int sp_resume(struct sp_device *sp)
> >
> > return 0;
> > }
> > -#endif
> >
> > struct sp_device *sp_get_psp_master_device(void)
> > {
> > diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h
> > index f913f1494af9..0218d0670eee 100644
> > --- a/drivers/crypto/ccp/sp-dev.h
> > +++ b/drivers/crypto/ccp/sp-dev.h
> > @@ -119,7 +119,7 @@ int sp_init(struct sp_device *sp);
> > void sp_destroy(struct sp_device *sp);
> > struct sp_device *sp_get_master(void);
> >
> > -int sp_suspend(struct sp_device *sp, pm_message_t state);
> > +int sp_suspend(struct sp_device *sp);
> > int sp_resume(struct sp_device *sp);
> > int sp_request_ccp_irq(struct sp_device *sp, irq_handler_t handler,
> > const char *name, void *data);
> > @@ -134,7 +134,7 @@ struct sp_device *sp_get_psp_master_device(void);
> > int ccp_dev_init(struct sp_device *sp);
> > void ccp_dev_destroy(struct sp_device *sp);
> >
> > -int ccp_dev_suspend(struct sp_device *sp, pm_message_t state);
> > +int ccp_dev_suspend(struct sp_device *sp);
> > int ccp_dev_resume(struct sp_device *sp);
> >
> > #else /* !CONFIG_CRYPTO_DEV_SP_CCP */
> > @@ -145,7 +145,7 @@ static inline int ccp_dev_init(struct sp_device *sp)
> > }
> > static inline void ccp_dev_destroy(struct sp_device *sp) { }
> >
> > -static inline int ccp_dev_suspend(struct sp_device *sp, pm_message_t state)
> > +static inline int ccp_dev_suspend(struct sp_device *sp)
> > {
> > return 0;
> > }
> > diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
> > index cb6cb47053f4..f471dbaef1fb 100644
> > --- a/drivers/crypto/ccp/sp-pci.c
> > +++ b/drivers/crypto/ccp/sp-pci.c
> > @@ -252,23 +252,19 @@ static void sp_pci_remove(struct pci_dev *pdev)
> > sp_free_irqs(sp);
> > }
> >
> > -#ifdef CONFIG_PM
> > -static int sp_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __maybe_unused sp_pci_suspend(struct device *dev)
> > {
> > - struct device *dev = &pdev->dev;
> > struct sp_device *sp = dev_get_drvdata(dev);
> >
> > - return sp_suspend(sp, state);
> > + return sp_suspend(sp);
> > }
> >
> > -static int sp_pci_resume(struct pci_dev *pdev)
> > +static int __maybe_unused sp_pci_resume(struct device *dev)
> > {
> > - struct device *dev = &pdev->dev;
> > struct sp_device *sp = dev_get_drvdata(dev);
> >
> > return sp_resume(sp);
> > }
> > -#endif
> >
> > #ifdef CONFIG_CRYPTO_DEV_SP_PSP
> > static const struct sev_vdata sevv1 = {
> > @@ -365,15 +361,14 @@ static const struct pci_device_id sp_pci_table[] = {
> > };
> > MODULE_DEVICE_TABLE(pci, sp_pci_table);
> >
> > +static SIMPLE_DEV_PM_OPS(sp_pci_pm_ops, sp_pci_suspend, sp_pci_resume);
> > +
> > static struct pci_driver sp_pci_driver = {
> > .name = "ccp",
> > .id_table = sp_pci_table,
> > .probe = sp_pci_probe,
> > .remove = sp_pci_remove,
> > -#ifdef CONFIG_PM
> > - .suspend = sp_pci_suspend,
> > - .resume = sp_pci_resume,
> > -#endif
> > + .driver.pm = &sp_pci_pm_ops,
> > };
> >
> > int sp_pci_init(void)
> > diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> > index 831aac1393a2..9dba52fbee99 100644
> > --- a/drivers/crypto/ccp/sp-platform.c
> > +++ b/drivers/crypto/ccp/sp-platform.c
>
> This file use the same "#ifdef CONFIG_PM" to define the suspend and resume
> functions (see sp_platform_driver variable). Doesn't that need to be
> updated similar to sp-pci.c? Not sure how this compile tested successfully
> unless your kernel config didn't have CONFIG_PM defined?
I am not sure but my .config file has "CONFIG_PM=y"
The motive is update PM of sp-pci. Months ago, we decided to remove
"#ifdef CONFIG_PM" container and mark the callbacks as __maybe_unused.
Hence, I did similar changes to all files on which sp-pci is dependent.
This caused change in function defintion and declaration of sp_suspend().
sp-pci is not depended upon sp-platform. sp-platform was modified only because
it also invoked sp_suspend() which got modified.
Thus, I didn't made any other changes to sp-platofrm.
Thanks
Vaibhav Gupta
>
> Thanks,
> Tom
>
> > @@ -207,7 +207,7 @@ static int sp_platform_suspend(struct platform_device *pdev,
> > struct device *dev = &pdev->dev;
> > struct sp_device *sp = dev_get_drvdata(dev);
> >
> > - return sp_suspend(sp, state);
> > + return sp_suspend(sp);
> > }
> >
> > static int sp_platform_resume(struct platform_device *pdev)
> >
On Tue, Jul 21, 2020 at 12:15:13PM -0500, Tom Lendacky wrote:
> On 7/21/20 11:30 AM, Vaibhav Gupta wrote:
> > On Tue, Jul 21, 2020 at 10:19:33AM -0500, Tom Lendacky wrote:
> >> On 7/21/20 7:31 AM, Vaibhav Gupta wrote:
> >>> +bool __maybe_unused ccp_queues_suspended(struct ccp_device *ccp)
> >>
> >> These aren't static functions, so is the __maybe_unused necessary?
> >>
> >> (Same comment on the other non-static functions changed below)
> >>
> >>> {
> >>> + .driver.pm = &sp_pci_pm_ops,
> >>> };
> >>>
> >>> int sp_pci_init(void)
> >>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
> >>> index 831aac1393a2..9dba52fbee99 100644
> >>> --- a/drivers/crypto/ccp/sp-platform.c
> >>> +++ b/drivers/crypto/ccp/sp-platform.c
> >>
> >> This file use the same "#ifdef CONFIG_PM" to define the suspend and resume
> >> functions (see sp_platform_driver variable). Doesn't that need to be
> >> updated similar to sp-pci.c? Not sure how this compile tested successfully
> >> unless your kernel config didn't have CONFIG_PM defined?
> > I am not sure but my .config file has "CONFIG_PM=y"
>
> Ok, my miss on that, you didn't update the sp_platform_suspend signature,
> so no issue there.
>
> >
> > The motive is update PM of sp-pci. Months ago, we decided to remove
> > "#ifdef CONFIG_PM" container and mark the callbacks as __maybe_unused.
>
> Is this being done only for struct pci_driver structures then? Or will
> there be a follow on effort for struct platform_driver structures?
>
For now, I am updating all the PCI drivers supporting legacy callbacks for
power management. It is part of my Linux Kernel Mentorsship Program project.
I will talk to Bjorn about this for sure.
> I can see the need for the __maybe_unused on the sp_pci_suspend() and
> sp_pci_resume() functions since those are static functions that may cause
> warnings depending on whether CONFIG_PM_SLEEP is defined or not.
>
> The ccp_dev_suspend() and ccp_dev_resume() functions, though, are external
> functions that I would think shouldn't require the __may_unused attribute
> because the compiler wouldn't know if they are invoked or not within that
> file (similar to how the sp_suspend() and sp_resume() weren't modified to
> include the __maybe_unused attribute).
Yes you are right. External functions should not require __maybe_unused. I got
confused by the kbuild error in one of my previous patches.
But those functions must have been static. I will have to dig out the mail to
check it.
> Can you try a compile test without
> changing those functions and without CONFIG_PM=y and see if you run into
> issues?
Sure, I will run this and check if it throws any warning/error.
Thanks a lot!
--Vaibhav Gupta
>
> Thanks,
> Tom
>
> > Hence, I did similar changes to all files on which sp-pci is dependent.
> >
> > This caused change in function defintion and declaration of sp_suspend().
> >
> > sp-pci is not depended upon sp-platform. sp-platform was modified only because
> > it also invoked sp_suspend() which got modified.
> >
> > Thus, I didn't made any other changes to sp-platofrm.
> >
> > Thanks
> > Vaibhav Gupta
> >>
> >> Thanks,
> >> Tom
> >>
> >>> @@ -207,7 +207,7 @@ static int sp_platform_suspend(struct platform_device *pdev,
> >>> struct device *dev = &pdev->dev;
> >>> struct sp_device *sp = dev_get_drvdata(dev);
> >>>
> >>> - return sp_suspend(sp, state);
> >>> + return sp_suspend(sp);
> >>> }
> >>>
> >>> static int sp_platform_resume(struct platform_device *pdev)
> >>>