Re: [Linux-kernel-mentees] [PATCH v1] crypto: ccp: sp-pci: use generic power management

On Tue, Jul 21, 2020 at 12:15:13PM -0500, Tom Lendacky wrote:

On 7/21/20 11:30 AM, Vaibhav Gupta wrote:

quoted

On Tue, Jul 21, 2020 at 10:19:33AM -0500, Tom Lendacky wrote:

quoted

On 7/21/20 7:31 AM, Vaibhav Gupta wrote:

quoted

+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)

quoted

 {
+	.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.

quoted

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

quoted

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

quoted

Thanks,
Tom

quoted

@@ -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)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees