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

[Vaibhav Gupta <vaibhavgupta40@gmail.com>]

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