| Message ID | 20201107082151.58239-1-vaibhavgupta40@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] i2c: nvidia-gpu: drop empty stub for runtime pm | expand |
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
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>
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
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
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!
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) {
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(-)