[PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Vaibhav Gupta]

After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
functions") we no more need empty stubs for runtime-pm to work.

The driver has no device specific task(s) for .suspend() . The stub was
placed just for runtime-pm, which can be dropped now.

Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index f9a69b109e5c..6b20601ffb13 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
 	pci_free_irq_vectors(pdev);
 }
 
-/*
- * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
- * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
- * Documentation/power/pci.rst also insists for driver to provide this.
- */
-static __maybe_unused int gpu_i2c_suspend(struct device *dev)
-{
-	return 0;
-}
+#define gpu_i2c_suspend NULL
 
 static __maybe_unused int gpu_i2c_resume(struct device *dev)
 {
-- 
2.28.0

Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Vaibhav Gupta]

On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
> After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
> functions") we no more need empty stubs for runtime-pm to work.
> 
> The driver has no device specific task(s) for .suspend() . The stub was
> placed just for runtime-pm, which can be dropped now.
> 
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> index f9a69b109e5c..6b20601ffb13 100644
> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
>  	pci_free_irq_vectors(pdev);
>  }
>  
> -/*
> - * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
> - * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
> - * Documentation/power/pci.rst also insists for driver to provide this.
> - */
> -static __maybe_unused int gpu_i2c_suspend(struct device *dev)
> -{
> -	return 0;
> -}
> +#define gpu_i2c_suspend NULL
>  
>  static __maybe_unused int gpu_i2c_resume(struct device *dev)
>  {
> -- 
> 2.28.0
> 
The patch is only compile-tested.

--Vaibhav

Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Jarkko Nikula]

On 11/7/20 11:04 AM, Vaibhav Gupta wrote:
> On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
>> After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
>> functions") we no more need empty stubs for runtime-pm to work.
>>
>> The driver has no device specific task(s) for .suspend() . The stub was
>> placed just for runtime-pm, which can be dropped now.
>>
>> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
>> ---
>>   drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +---------
>>   1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
>> index f9a69b109e5c..6b20601ffb13 100644
>> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
>> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
>> @@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
>>   	pci_free_irq_vectors(pdev);
>>   }
>>   
>> -/*
>> - * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
>> - * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
>> - * Documentation/power/pci.rst also insists for driver to provide this.
>> - */
>> -static __maybe_unused int gpu_i2c_suspend(struct device *dev)
>> -{
>> -	return 0;
>> -}
>> +#define gpu_i2c_suspend NULL
>>   
Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the 
suspend callback?

>>   static __maybe_unused int gpu_i2c_resume(struct device *dev)
>>   {
>> -- 
>> 2.28.0
>>
> The patch is only compile-tested.
> 
It should work also system suspend point of view. This patch affects 
also it since callbacks are set with the UNIVERSAL_DEV_PM_OPS(). Means 
the same callback is used for runtime PM and system suspend.

I quickly debugged this with an another driver and PCI stack does put 
the device into D3 state in system suspend from pci_prepare_to_sleep() 
if the suspend callback is not set and device was on prior it.

Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Vaibhav Gupta]

On Tue, Nov 10, 2020 at 02:33:43PM +0200, Jarkko Nikula wrote:
> On 11/7/20 11:04 AM, Vaibhav Gupta wrote:
> > On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
> > > After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
> > > functions") we no more need empty stubs for runtime-pm to work.
> > > 
> > > The driver has no device specific task(s) for .suspend() . The stub was
> > > placed just for runtime-pm, which can be dropped now.
> > > 
> > > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > ---
> > >   drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +---------
> > >   1 file changed, 1 insertion(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > index f9a69b109e5c..6b20601ffb13 100644
> > > --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > @@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
> > >   	pci_free_irq_vectors(pdev);
> > >   }
> > > -/*
> > > - * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
> > > - * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
> > > - * Documentation/power/pci.rst also insists for driver to provide this.
> > > - */
> > > -static __maybe_unused int gpu_i2c_suspend(struct device *dev)
> > > -{
> > > -	return 0;
> > > -}
> > > +#define gpu_i2c_suspend NULL
> Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the suspend
> callback?
> 
Yes. I have noticed that the approach for this is random. Many drivers pass
NULL directly to the dev_pm_ops type variable, and rest of them use a macro.

I used it for symmetry. I mean there is 'gpu_i2c_resume', so although a macro,
I have put a 'gpu_i2c_suspend'.

Although it won't make any significant change, but if required, I can send
another patch where NULL is passed into UNIVERSAL_DEV_PM_OPS() instead.
> > >   static __maybe_unused int gpu_i2c_resume(struct device *dev)
> > >   {
> > > -- 
> > > 2.28.0
> > > 
> > The patch is only compile-tested.
> > 
> It should work also system suspend point of view. This patch affects also it
> since callbacks are set with the UNIVERSAL_DEV_PM_OPS(). Means the same
> callback is used for runtime PM and system suspend.
> 
> I quickly debugged this with an another driver and PCI stack does put the
> device into D3 state in system suspend from pci_prepare_to_sleep() if the
> suspend callback is not set and device was on prior it.
> 
> Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>


Thanks for debugging. Theoretically too, according to generic framework, drivers
are supposed to do only device specific jobs. If there is none, or callback is
set as NULL, remaining work to put the device in lower power state is PCI
stack's task.

--Vaibhav

Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Jarkko Nikula]

On 11/11/20 12:54 PM, Vaibhav Gupta wrote:
> On Tue, Nov 10, 2020 at 02:33:43PM +0200, Jarkko Nikula wrote:
>>>> +#define gpu_i2c_suspend NULL
>> Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the suspend
>> callback?
>>
> Yes. I have noticed that the approach for this is random. Many drivers pass
> NULL directly to the dev_pm_ops type variable, and rest of them use a macro.
> 
> I used it for symmetry. I mean there is 'gpu_i2c_resume', so although a macro,
> I have put a 'gpu_i2c_suspend'.
> 
> Although it won't make any significant change, but if required, I can send
> another patch where NULL is passed into UNIVERSAL_DEV_PM_OPS() instead.

No need to resend from my side, it was just a remark and I gave already 
the reviewed-by tag.

Jarkko

Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
> After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
> functions") we no more need empty stubs for runtime-pm to work.
> 
> The driver has no device specific task(s) for .suspend() . The stub was
> placed just for runtime-pm, which can be dropped now.
> 
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]