linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH v1 0/3] block: use generic power management
@ 2020-07-17  8:09 Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 1/3] mtip32xx: " Vaibhav Gupta
                   ` (2 more replies)
  0 siblings, 3 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-17  8:09 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta


Linux Kernel Mentee: Remove Legacy Power Management.

The purpose of this patch series is to upgrade power management in block
drivers. This has been done by upgrading .suspend() and .resume() callbacks.

The upgrade makes sure that the involvement of PCI Core does not change the
order of operations executed in a driver. Thus, does not change its behavior.

In general, drivers with legacy PM, .suspend() and .resume() make use of PCI
helper functions like pci_enable/disable_device_mem(), pci_set_power_state(),
pci_save/restore_state(), pci_enable/disable_device(), etc. to complete
their job.

The conversion requires the removal of those function calls, change the
callbacks' definition accordingly and make use of dev_pm_ops structure.

All patches are compile-tested only.

Test tools:
    - Compiler: gcc (GCC) 10.1.0
    - allmodconfig build: make -j$(nproc) W=1 all

Vaibhav Gupta (3):
  mtip32xx: use generic power management
  rsxx: use generic power management
  skd: use generic power management

 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 drivers/block/rsxx/core.c         |  9 ++++--
 drivers/block/skd_main.c          | 36 ++++++---------------
 3 files changed, 29 insertions(+), 70 deletions(-)

-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v1 1/3] mtip32xx: use generic power management
  2020-07-17  8:09 [Linux-kernel-mentees] [PATCH v1 0/3] block: use generic power management Vaibhav Gupta
@ 2020-07-17  8:09 ` Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 2/3] rsxx: " Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-17  8:09 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver makes use of PCI helper functions like
pci_save/restore_state(), pci_disable_device(), pcim_enable_device(),
pci_set_power_state() and pci_set_master() to do required operations. In
generic mode, they are no longer needed.

Change function parameter in both .suspend() and .resume() to
"struct device*" type. Use dev_get_drvdata() to get drv data.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index f6bafa9a68b9..7d1280952b35 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -4246,14 +4246,13 @@ static void mtip_pci_remove(struct pci_dev *pdev)
  *	0  Success
  *	<0 Error
  */
-static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
+static int __maybe_unused mtip_pci_suspend(struct device *dev)
 {
 	int rv = 0;
-	struct driver_data *dd = pci_get_drvdata(pdev);
+	struct driver_data *dd = dev_get_drvdata(dev);
 
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
@@ -4261,21 +4260,8 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 
 	/* Disable ports & interrupts then send standby immediate */
 	rv = mtip_block_suspend(dd);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to suspend controller\n");
-		return rv;
-	}
-
-	/*
-	 * Save the pci config space to pdev structure &
-	 * disable the device
-	 */
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-
-	/* Move to Low power state*/
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (rv < 0)
+		dev_err(dev, "Failed to suspend controller\n");
 
 	return rv;
 }
@@ -4287,42 +4273,25 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
  *      0  Success
  *      <0 Error
  */
-static int mtip_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused mtip_pci_resume(struct device *dev)
 {
 	int rv = 0;
 	struct driver_data *dd;
 
-	dd = pci_get_drvdata(pdev);
+	dd = dev_get_drvdata(dev);
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
-	/* Move the device to active State */
-	pci_set_power_state(pdev, PCI_D0);
-
-	/* Restore PCI configuration space */
-	pci_restore_state(pdev);
-
-	/* Enable the PCI device*/
-	rv = pcim_enable_device(pdev);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to enable card during resume\n");
-		goto err;
-	}
-	pci_set_master(pdev);
-
 	/*
 	 * Calls hbaReset, initPort, & startPort function
 	 * then enables interrupts
 	 */
 	rv = mtip_block_resume(dd);
 	if (rv < 0)
-		dev_err(&pdev->dev, "Unable to resume\n");
+		dev_err(dev, "Unable to resume\n");
 
-err:
 	clear_bit(MTIP_DDF_RESUME_BIT, &dd->dd_flag);
 
 	return rv;
@@ -4353,14 +4322,15 @@ static const struct pci_device_id mtip_pci_tbl[] = {
 	{ 0 }
 };
 
+static SIMPLE_DEV_PM_OPS(mtip_pci_pm_ops, mtip_pci_suspend, mtip_pci_resume);
+
 /* Structure that describes the PCI driver functions. */
 static struct pci_driver mtip_pci_driver = {
 	.name			= MTIP_DRV_NAME,
 	.id_table		= mtip_pci_tbl,
 	.probe			= mtip_pci_probe,
 	.remove			= mtip_pci_remove,
-	.suspend		= mtip_pci_suspend,
-	.resume			= mtip_pci_resume,
+	.driver.pm		= &mtip_pci_pm_ops,
 	.shutdown		= mtip_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v1 2/3] rsxx: use generic power management
  2020-07-17  8:09 [Linux-kernel-mentees] [PATCH v1 0/3] block: use generic power management Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 1/3] mtip32xx: " Vaibhav Gupta
@ 2020-07-17  8:09 ` Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-17  8:09 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver has not defined .resume(), hence define it as NULL.

Change function parameter in both .suspend() to "struct device*" type and
mark the parameter as "__attribute__((unused)) " as the function body is
empty.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/rsxx/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 10f6368117d8..866153fd380a 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -1030,12 +1030,15 @@ static void rsxx_pci_remove(struct pci_dev *dev)
 	kfree(card);
 }
 
-static int rsxx_pci_suspend(struct pci_dev *dev, pm_message_t state)
+static int __maybe_unused rsxx_pci_suspend(
+	__attribute__((unused)) struct device *dev)
 {
 	/* We don't support suspend at this time. */
 	return -ENOSYS;
 }
 
+#define rsxx_pci_resume NULL
+
 static void rsxx_pci_shutdown(struct pci_dev *dev)
 {
 	struct rsxx_cardinfo *card = pci_get_drvdata(dev);
@@ -1071,12 +1074,14 @@ static const struct pci_device_id rsxx_pci_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, rsxx_pci_ids);
 
+static SIMPLE_DEV_PM_OPS(rsxx_pci_pm_ops, rsxx_pci_suspend, rsxx_pci_resume);
+
 static struct pci_driver rsxx_pci_driver = {
 	.name		= DRIVER_NAME,
 	.id_table	= rsxx_pci_ids,
 	.probe		= rsxx_pci_probe,
 	.remove		= rsxx_pci_remove,
-	.suspend	= rsxx_pci_suspend,
+	.driver.pm	= &rsxx_pci_pm_ops,
 	.shutdown	= rsxx_pci_shutdown,
 	.err_handler    = &rsxx_err_handler,
 };
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v1 3/3] skd: use generic power management
  2020-07-17  8:09 [Linux-kernel-mentees] [PATCH v1 0/3] block: use generic power management Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 1/3] mtip32xx: " Vaibhav Gupta
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 2/3] rsxx: " Vaibhav Gupta
@ 2020-07-17  8:09 ` Vaibhav Gupta
  2020-07-20 12:52   ` Damien Le Moal
  2 siblings, 1 reply; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-17  8:09 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver makes use of PCI helper functions like
pci_save/restore_state(), pci_enable/disable_device(),
pci_request/release_regions(), pci_set_power_state() and
pci_set_master() to do required operations. In generic mode, they are no
longer needed.

Change function parameter in both .suspend() and .resume() to
"struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/skd_main.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 51569c199a6c..d8d1042e7338 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3315,12 +3315,12 @@ static void skd_pci_remove(struct pci_dev *pdev)
 	return;
 }
 
-static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused skd_pci_suspend(struct device *dev)
 {
 	int i;
-	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct skd_device *skdev = pci_get_drvdata(pdev);
 
-	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
 		dev_err(&pdev->dev, "no device data for PCI\n");
 		return -EIO;
@@ -3337,35 +3337,23 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 	if (skdev->pcie_error_reporting_is_enabled)
 		pci_disable_pcie_error_reporting(pdev);
 
-	pci_release_regions(pdev);
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	return 0;
 }
 
-static int skd_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused skd_pci_resume(struct device *dev)
 {
 	int i;
 	int rc = 0;
-	struct skd_device *skdev;
 
-	skdev = pci_get_drvdata(pdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct skd_device *skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
 		dev_err(&pdev->dev, "no device data for PCI\n");
 		return -1;
 	}
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_enable_wake(pdev, PCI_D0, 0);
-	pci_restore_state(pdev);
+	device_wakeup_disable(dev);
 
-	rc = pci_enable_device(pdev);
-	if (rc)
-		return rc;
-	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
-		goto err_out;
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
@@ -3374,7 +3362,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		goto err_out_regions;
 	}
 
-	pci_set_master(pdev);
 	rc = pci_enable_pcie_error_reporting(pdev);
 	if (rc) {
 		dev_err(&pdev->dev,
@@ -3427,10 +3414,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		pci_disable_pcie_error_reporting(pdev);
 
 err_out_regions:
-	pci_release_regions(pdev);
-
-err_out:
-	pci_disable_device(pdev);
 	return rc;
 }
 
@@ -3450,13 +3433,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
 	skd_stop_device(skdev);
 }
 
+static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
+
 static struct pci_driver skd_driver = {
 	.name		= DRV_NAME,
 	.id_table	= skd_pci_tbl,
 	.probe		= skd_pci_probe,
 	.remove		= skd_pci_remove,
-	.suspend	= skd_pci_suspend,
-	.resume		= skd_pci_resume,
+	.driver.pm	= &skd_pci_pm_ops,
 	.shutdown	= skd_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v1 3/3] skd: use generic power management
  2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 3/3] skd: " Vaibhav Gupta
@ 2020-07-20 12:52   ` Damien Le Moal
  2020-07-20 13:16     ` Vaibhav Gupta
  2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
  0 siblings, 2 replies; 28+ messages in thread
From: Damien Le Moal @ 2020-07-20 12:52 UTC (permalink / raw)
  To: Vaibhav Gupta, Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas,
	Vaibhav Gupta, Jens Axboe, Joshua Morris, Philip Kelleher
  Cc: linux-block, linux-kernel-mentees, linux-kernel

On 2020/07/17 17:10, Vaibhav Gupta wrote:
> Drivers using legacy PM have to manage PCI states and device's PM states
> themselves. They also need to take care of configuration registers.
> 
> With improved and powerful support of generic PM, PCI Core takes care of
> above mentioned, device-independent, jobs.
> 
> This driver makes use of PCI helper functions like
> pci_save/restore_state(), pci_enable/disable_device(),
> pci_request/release_regions(), pci_set_power_state() and
> pci_set_master() to do required operations. In generic mode, they are no
> longer needed.
> 
> Change function parameter in both .suspend() and .resume() to
> "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> 
> Compile-tested only.

I do not think this belongs into the commit message. This was mentioned in the
cover letter.

> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/block/skd_main.c | 36 ++++++++++--------------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index 51569c199a6c..d8d1042e7338 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3315,12 +3315,12 @@ static void skd_pci_remove(struct pci_dev *pdev)
>  	return;
>  }
>  
> -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused skd_pci_suspend(struct device *dev)
>  {
>  	int i;
> -	struct skd_device *skdev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct skd_device *skdev = pci_get_drvdata(pdev);

You can just add the pdev declaration here. The other 2 changes are not needed.

>  
> -	skdev = pci_get_drvdata(pdev);

You can keep this as is.

>  	if (!skdev) {
>  		dev_err(&pdev->dev, "no device data for PCI\n");
>  		return -EIO;
> @@ -3337,35 +3337,23 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>  	if (skdev->pcie_error_reporting_is_enabled)
>  		pci_disable_pcie_error_reporting(pdev);
>  
> -	pci_release_regions(pdev);
> -	pci_save_state(pdev);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
>  	return 0;
>  }
>  
> -static int skd_pci_resume(struct pci_dev *pdev)
> +static int __maybe_unused skd_pci_resume(struct device *dev)
>  {
>  	int i;
>  	int rc = 0;
> -	struct skd_device *skdev;
>  
> -	skdev = pci_get_drvdata(pdev);

Same comment as above. Keep these as is and add only pdev declaration.

> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct skd_device *skdev = pci_get_drvdata(pdev);
>  	if (!skdev) {
>  		dev_err(&pdev->dev, "no device data for PCI\n");
>  		return -1;
>  	}
>  
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_enable_wake(pdev, PCI_D0, 0);
> -	pci_restore_state(pdev);
> +	device_wakeup_disable(dev);
>  
> -	rc = pci_enable_device(pdev);
> -	if (rc)
> -		return rc;
> -	rc = pci_request_regions(pdev, DRV_NAME);
> -	if (rc)
> -		goto err_out;
>  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>  	if (rc)
>  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> @@ -3374,7 +3362,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		goto err_out_regions;
>  	}
>  
> -	pci_set_master(pdev);
>  	rc = pci_enable_pcie_error_reporting(pdev);
>  	if (rc) {
>  		dev_err(&pdev->dev,
> @@ -3427,10 +3414,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		pci_disable_pcie_error_reporting(pdev);
>  
>  err_out_regions:
> -	pci_release_regions(pdev);
> -
> -err_out:
> -	pci_disable_device(pdev);
>  	return rc;
>  }
>  
> @@ -3450,13 +3433,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
>  	skd_stop_device(skdev);
>  }
>  
> +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> +
>  static struct pci_driver skd_driver = {
>  	.name		= DRV_NAME,
>  	.id_table	= skd_pci_tbl,
>  	.probe		= skd_pci_probe,
>  	.remove		= skd_pci_remove,
> -	.suspend	= skd_pci_suspend,
> -	.resume		= skd_pci_resume,
> +	.driver.pm	= &skd_pci_pm_ops,
>  	.shutdown	= skd_pci_shutdown,
>  };
>  
> 


-- 
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v1 3/3] skd: use generic power management
  2020-07-20 12:52   ` Damien Le Moal
@ 2020-07-20 13:16     ` Vaibhav Gupta
  2020-07-21  2:49       ` Damien Le Moal
  2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
  1 sibling, 1 reply; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-20 13:16 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Jens Axboe, linux-kernel, linux-block, Bjorn Helgaas,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On Mon, Jul 20, 2020 at 12:52:14PM +0000, Damien Le Moal wrote:
> On 2020/07/17 17:10, Vaibhav Gupta wrote:
> > Drivers using legacy PM have to manage PCI states and device's PM states
> > themselves. They also need to take care of configuration registers.
> > 
> > With improved and powerful support of generic PM, PCI Core takes care of
> > above mentioned, device-independent, jobs.
> > 
> > This driver makes use of PCI helper functions like
> > pci_save/restore_state(), pci_enable/disable_device(),
> > pci_request/release_regions(), pci_set_power_state() and
> > pci_set_master() to do required operations. In generic mode, they are no
> > longer needed.
> > 
> > Change function parameter in both .suspend() and .resume() to
> > "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> > 
> > Compile-tested only.
> 
> I do not think this belongs into the commit message. This was mentioned in the
> cover letter.
>
The messages in cover letter and commit message are bit similar. But the commit
message has patch specific changes mentioned in it.
> > 
> > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > ---
> >  drivers/block/skd_main.c | 36 ++++++++++--------------------------
> >  1 file changed, 10 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > index 51569c199a6c..d8d1042e7338 100644
> > --- a/drivers/block/skd_main.c
> > +++ b/drivers/block/skd_main.c
> > @@ -3315,12 +3315,12 @@ static void skd_pci_remove(struct pci_dev *pdev)
> >  	return;
> >  }
> >  
> > -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __maybe_unused skd_pci_suspend(struct device *dev)
> >  {
> >  	int i;
> > -	struct skd_device *skdev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct skd_device *skdev = pci_get_drvdata(pdev);
> 
> You can just add the pdev declaration here. The other 2 changes are not needed.
> 
Okay.
> >  
> > -	skdev = pci_get_drvdata(pdev);
> 
> You can keep this as is.
> 
> >  	if (!skdev) {
> >  		dev_err(&pdev->dev, "no device data for PCI\n");
> >  		return -EIO;
> > @@ -3337,35 +3337,23 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> >  	if (skdev->pcie_error_reporting_is_enabled)
> >  		pci_disable_pcie_error_reporting(pdev);
> >  
> > -	pci_release_regions(pdev);
> > -	pci_save_state(pdev);
> > -	pci_disable_device(pdev);
> > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >  	return 0;
> >  }
> >  
> > -static int skd_pci_resume(struct pci_dev *pdev)
> > +static int __maybe_unused skd_pci_resume(struct device *dev)
> >  {
> >  	int i;
> >  	int rc = 0;
> > -	struct skd_device *skdev;
> >  
> > -	skdev = pci_get_drvdata(pdev);
> 
> Same comment as above. Keep these as is and add only pdev declaration.
> 
Okay.
Sending v2 with suggested changes.

--Vaibhav Gupta
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct skd_device *skdev = pci_get_drvdata(pdev);
> >  	if (!skdev) {
> >  		dev_err(&pdev->dev, "no device data for PCI\n");
> >  		return -1;
> >  	}
> >  
> > -	pci_set_power_state(pdev, PCI_D0);
> > -	pci_enable_wake(pdev, PCI_D0, 0);
> > -	pci_restore_state(pdev);
> > +	device_wakeup_disable(dev);
> >  
> > -	rc = pci_enable_device(pdev);
> > -	if (rc)
> > -		return rc;
> > -	rc = pci_request_regions(pdev, DRV_NAME);
> > -	if (rc)
> > -		goto err_out;
> >  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >  	if (rc)
> >  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > @@ -3374,7 +3362,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> >  		goto err_out_regions;
> >  	}
> >  
> > -	pci_set_master(pdev);
> >  	rc = pci_enable_pcie_error_reporting(pdev);
> >  	if (rc) {
> >  		dev_err(&pdev->dev,
> > @@ -3427,10 +3414,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> >  		pci_disable_pcie_error_reporting(pdev);
> >  
> >  err_out_regions:
> > -	pci_release_regions(pdev);
> > -
> > -err_out:
> > -	pci_disable_device(pdev);
> >  	return rc;
> >  }
> >  
> > @@ -3450,13 +3433,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
> >  	skd_stop_device(skdev);
> >  }
> >  
> > +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> > +
> >  static struct pci_driver skd_driver = {
> >  	.name		= DRV_NAME,
> >  	.id_table	= skd_pci_tbl,
> >  	.probe		= skd_pci_probe,
> >  	.remove		= skd_pci_remove,
> > -	.suspend	= skd_pci_suspend,
> > -	.resume		= skd_pci_resume,
> > +	.driver.pm	= &skd_pci_pm_ops,
> >  	.shutdown	= skd_pci_shutdown,
> >  };
> >  
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v2 0/3] block: use generic power management
  2020-07-20 12:52   ` Damien Le Moal
  2020-07-20 13:16     ` Vaibhav Gupta
@ 2020-07-20 13:29     ` Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 1/3] mtip32xx: " Vaibhav Gupta
                         ` (2 more replies)
  1 sibling, 3 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-20 13:29 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Linux Kernel Mentee: Remove Legacy Power Management.

The purpose of this patch series is to upgrade power management in block
drivers. This has been done by upgrading .suspend() and .resume() callbacks.

The upgrade makes sure that the involvement of PCI Core does not change the
order of operations executed in a driver. Thus, does not change its behavior.

In general, drivers with legacy PM, .suspend() and .resume() make use of PCI
helper functions like pci_enable/disable_device_mem(), pci_set_power_state(),
pci_save/restore_state(), pci_enable/disable_device(), etc. to complete
their job.

The conversion requires the removal of those function calls, change the
callbacks' definition accordingly and make use of dev_pm_ops structure.

All patches are compile-tested only.

Test tools:
    - Compiler: gcc (GCC) 10.1.0
    - allmodconfig build: make -j$(nproc) W=1 all

Vaibhav Gupta (3):
  mtip32xx: use generic power management
  rsxx: use generic power management
  skd: use generic power management

 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 drivers/block/rsxx/core.c         |  9 ++++--
 drivers/block/skd_main.c          | 30 +++++------------
 3 files changed, 27 insertions(+), 66 deletions(-)

-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v2 1/3] mtip32xx: use generic power management
  2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
@ 2020-07-20 13:30       ` Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 2/3] rsxx: " Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-20 13:30 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver makes use of PCI helper functions like
pci_save/restore_state(), pci_disable_device(), pcim_enable_device(),
pci_set_power_state() and pci_set_master() to do required operations. In
generic mode, they are no longer needed.

Change function parameter in both .suspend() and .resume() to
"struct device*" type. Use dev_get_drvdata() to get drv data.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index f6bafa9a68b9..7d1280952b35 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -4246,14 +4246,13 @@ static void mtip_pci_remove(struct pci_dev *pdev)
  *	0  Success
  *	<0 Error
  */
-static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
+static int __maybe_unused mtip_pci_suspend(struct device *dev)
 {
 	int rv = 0;
-	struct driver_data *dd = pci_get_drvdata(pdev);
+	struct driver_data *dd = dev_get_drvdata(dev);
 
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
@@ -4261,21 +4260,8 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 
 	/* Disable ports & interrupts then send standby immediate */
 	rv = mtip_block_suspend(dd);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to suspend controller\n");
-		return rv;
-	}
-
-	/*
-	 * Save the pci config space to pdev structure &
-	 * disable the device
-	 */
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-
-	/* Move to Low power state*/
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (rv < 0)
+		dev_err(dev, "Failed to suspend controller\n");
 
 	return rv;
 }
@@ -4287,42 +4273,25 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
  *      0  Success
  *      <0 Error
  */
-static int mtip_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused mtip_pci_resume(struct device *dev)
 {
 	int rv = 0;
 	struct driver_data *dd;
 
-	dd = pci_get_drvdata(pdev);
+	dd = dev_get_drvdata(dev);
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
-	/* Move the device to active State */
-	pci_set_power_state(pdev, PCI_D0);
-
-	/* Restore PCI configuration space */
-	pci_restore_state(pdev);
-
-	/* Enable the PCI device*/
-	rv = pcim_enable_device(pdev);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to enable card during resume\n");
-		goto err;
-	}
-	pci_set_master(pdev);
-
 	/*
 	 * Calls hbaReset, initPort, & startPort function
 	 * then enables interrupts
 	 */
 	rv = mtip_block_resume(dd);
 	if (rv < 0)
-		dev_err(&pdev->dev, "Unable to resume\n");
+		dev_err(dev, "Unable to resume\n");
 
-err:
 	clear_bit(MTIP_DDF_RESUME_BIT, &dd->dd_flag);
 
 	return rv;
@@ -4353,14 +4322,15 @@ static const struct pci_device_id mtip_pci_tbl[] = {
 	{ 0 }
 };
 
+static SIMPLE_DEV_PM_OPS(mtip_pci_pm_ops, mtip_pci_suspend, mtip_pci_resume);
+
 /* Structure that describes the PCI driver functions. */
 static struct pci_driver mtip_pci_driver = {
 	.name			= MTIP_DRV_NAME,
 	.id_table		= mtip_pci_tbl,
 	.probe			= mtip_pci_probe,
 	.remove			= mtip_pci_remove,
-	.suspend		= mtip_pci_suspend,
-	.resume			= mtip_pci_resume,
+	.driver.pm		= &mtip_pci_pm_ops,
 	.shutdown		= mtip_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v2 2/3] rsxx: use generic power management
  2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 1/3] mtip32xx: " Vaibhav Gupta
@ 2020-07-20 13:30       ` Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-20 13:30 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver has not defined .resume(), hence define it as NULL.

Change function parameter in both .suspend() to "struct device*" type and
mark the parameter as "__attribute__((unused)) " as the function body is
empty.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/rsxx/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 10f6368117d8..866153fd380a 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -1030,12 +1030,15 @@ static void rsxx_pci_remove(struct pci_dev *dev)
 	kfree(card);
 }
 
-static int rsxx_pci_suspend(struct pci_dev *dev, pm_message_t state)
+static int __maybe_unused rsxx_pci_suspend(
+	__attribute__((unused)) struct device *dev)
 {
 	/* We don't support suspend at this time. */
 	return -ENOSYS;
 }
 
+#define rsxx_pci_resume NULL
+
 static void rsxx_pci_shutdown(struct pci_dev *dev)
 {
 	struct rsxx_cardinfo *card = pci_get_drvdata(dev);
@@ -1071,12 +1074,14 @@ static const struct pci_device_id rsxx_pci_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, rsxx_pci_ids);
 
+static SIMPLE_DEV_PM_OPS(rsxx_pci_pm_ops, rsxx_pci_suspend, rsxx_pci_resume);
+
 static struct pci_driver rsxx_pci_driver = {
 	.name		= DRIVER_NAME,
 	.id_table	= rsxx_pci_ids,
 	.probe		= rsxx_pci_probe,
 	.remove		= rsxx_pci_remove,
-	.suspend	= rsxx_pci_suspend,
+	.driver.pm	= &rsxx_pci_pm_ops,
 	.shutdown	= rsxx_pci_shutdown,
 	.err_handler    = &rsxx_err_handler,
 };
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 1/3] mtip32xx: " Vaibhav Gupta
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 2/3] rsxx: " Vaibhav Gupta
@ 2020-07-20 13:30       ` Vaibhav Gupta
  2020-07-21  2:57         ` Damien Le Moal
  2 siblings, 1 reply; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-20 13:30 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver makes use of PCI helper functions like
pci_save/restore_state(), pci_enable/disable_device(),
pci_request/release_regions(), pci_set_power_state() and
pci_set_master() to do required operations. In generic mode, they are no
longer needed.

Change function parameter in both .suspend() and .resume() to
"struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/block/skd_main.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 51569c199a6c..7f2d42900b38 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
 	return;
 }
 
-static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused skd_pci_suspend(struct device *dev)
 {
 	int i;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 	if (skdev->pcie_error_reporting_is_enabled)
 		pci_disable_pcie_error_reporting(pdev);
 
-	pci_release_regions(pdev);
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	return 0;
 }
 
-static int skd_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused skd_pci_resume(struct device *dev)
 {
 	int i;
 	int rc = 0;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		return -1;
 	}
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_enable_wake(pdev, PCI_D0, 0);
-	pci_restore_state(pdev);
+	device_wakeup_disable(dev);
 
-	rc = pci_enable_device(pdev);
-	if (rc)
-		return rc;
-	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
-		goto err_out;
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
@@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		goto err_out_regions;
 	}
 
-	pci_set_master(pdev);
 	rc = pci_enable_pcie_error_reporting(pdev);
 	if (rc) {
 		dev_err(&pdev->dev,
@@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		pci_disable_pcie_error_reporting(pdev);
 
 err_out_regions:
-	pci_release_regions(pdev);
-
-err_out:
-	pci_disable_device(pdev);
 	return rc;
 }
 
@@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
 	skd_stop_device(skdev);
 }
 
+static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
+
 static struct pci_driver skd_driver = {
 	.name		= DRV_NAME,
 	.id_table	= skd_pci_tbl,
 	.probe		= skd_pci_probe,
 	.remove		= skd_pci_remove,
-	.suspend	= skd_pci_suspend,
-	.resume		= skd_pci_resume,
+	.driver.pm	= &skd_pci_pm_ops,
 	.shutdown	= skd_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v1 3/3] skd: use generic power management
  2020-07-20 13:16     ` Vaibhav Gupta
@ 2020-07-21  2:49       ` Damien Le Moal
  2020-07-21  7:04         ` Vaibhav Gupta
  0 siblings, 1 reply; 28+ messages in thread
From: Damien Le Moal @ 2020-07-21  2:49 UTC (permalink / raw)
  To: Vaibhav Gupta
  Cc: Jens Axboe, linux-kernel, linux-block, Bjorn Helgaas,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On 2020/07/20 22:18, Vaibhav Gupta wrote:
> On Mon, Jul 20, 2020 at 12:52:14PM +0000, Damien Le Moal wrote:
>> On 2020/07/17 17:10, Vaibhav Gupta wrote:
>>> Drivers using legacy PM have to manage PCI states and device's PM states
>>> themselves. They also need to take care of configuration registers.
>>>
>>> With improved and powerful support of generic PM, PCI Core takes care of
>>> above mentioned, device-independent, jobs.
>>>
>>> This driver makes use of PCI helper functions like
>>> pci_save/restore_state(), pci_enable/disable_device(),
>>> pci_request/release_regions(), pci_set_power_state() and
>>> pci_set_master() to do required operations. In generic mode, they are no
>>> longer needed.
>>>
>>> Change function parameter in both .suspend() and .resume() to
>>> "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
>>>
>>> Compile-tested only.
>>
>> I do not think this belongs into the commit message. This was mentioned in the
>> cover letter.
>>
> The messages in cover letter and commit message are bit similar. But the commit
> message has patch specific changes mentioned in it.

My point was about the "compile-tested only" mention in the commit message. That
should be mentioned in the cover letter only. The goal of the patch review
process is also to get these patches tested by others if you do not have access
to the hardware. That is fine, and I can test for the skd driver. But a patch
mentioning that it is untested cannot be applied, for obvious reasons :)

So I only requested that you remove the "compiled tested only" note. The commit
message could be simpler too, see followup comment.

>>>
>>> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
>>> ---
>>>  drivers/block/skd_main.c | 36 ++++++++++--------------------------
>>>  1 file changed, 10 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
>>> index 51569c199a6c..d8d1042e7338 100644
>>> --- a/drivers/block/skd_main.c
>>> +++ b/drivers/block/skd_main.c
>>> @@ -3315,12 +3315,12 @@ static void skd_pci_remove(struct pci_dev *pdev)
>>>  	return;
>>>  }
>>>  
>>> -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>>> +static int __maybe_unused skd_pci_suspend(struct device *dev)
>>>  {
>>>  	int i;
>>> -	struct skd_device *skdev;
>>> +	struct pci_dev *pdev = to_pci_dev(dev);
>>> +	struct skd_device *skdev = pci_get_drvdata(pdev);
>>
>> You can just add the pdev declaration here. The other 2 changes are not needed.
>>
> Okay.
>>>  
>>> -	skdev = pci_get_drvdata(pdev);
>>
>> You can keep this as is.
>>
>>>  	if (!skdev) {
>>>  		dev_err(&pdev->dev, "no device data for PCI\n");
>>>  		return -EIO;
>>> @@ -3337,35 +3337,23 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>>>  	if (skdev->pcie_error_reporting_is_enabled)
>>>  		pci_disable_pcie_error_reporting(pdev);
>>>  
>>> -	pci_release_regions(pdev);
>>> -	pci_save_state(pdev);
>>> -	pci_disable_device(pdev);
>>> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
>>>  	return 0;
>>>  }
>>>  
>>> -static int skd_pci_resume(struct pci_dev *pdev)
>>> +static int __maybe_unused skd_pci_resume(struct device *dev)
>>>  {
>>>  	int i;
>>>  	int rc = 0;
>>> -	struct skd_device *skdev;
>>>  
>>> -	skdev = pci_get_drvdata(pdev);
>>
>> Same comment as above. Keep these as is and add only pdev declaration.
>>
> Okay.
> Sending v2 with suggested changes.
> 
> --Vaibhav Gupta
>>> +	struct pci_dev *pdev = to_pci_dev(dev);
>>> +	struct skd_device *skdev = pci_get_drvdata(pdev);
>>>  	if (!skdev) {
>>>  		dev_err(&pdev->dev, "no device data for PCI\n");
>>>  		return -1;
>>>  	}
>>>  
>>> -	pci_set_power_state(pdev, PCI_D0);
>>> -	pci_enable_wake(pdev, PCI_D0, 0);
>>> -	pci_restore_state(pdev);
>>> +	device_wakeup_disable(dev);
>>>  
>>> -	rc = pci_enable_device(pdev);
>>> -	if (rc)
>>> -		return rc;
>>> -	rc = pci_request_regions(pdev, DRV_NAME);
>>> -	if (rc)
>>> -		goto err_out;
>>>  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>>  	if (rc)
>>>  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>>> @@ -3374,7 +3362,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>>>  		goto err_out_regions;
>>>  	}
>>>  
>>> -	pci_set_master(pdev);
>>>  	rc = pci_enable_pcie_error_reporting(pdev);
>>>  	if (rc) {
>>>  		dev_err(&pdev->dev,
>>> @@ -3427,10 +3414,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>>>  		pci_disable_pcie_error_reporting(pdev);
>>>  
>>>  err_out_regions:
>>> -	pci_release_regions(pdev);
>>> -
>>> -err_out:
>>> -	pci_disable_device(pdev);
>>>  	return rc;
>>>  }
>>>  
>>> @@ -3450,13 +3433,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
>>>  	skd_stop_device(skdev);
>>>  }
>>>  
>>> +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
>>> +
>>>  static struct pci_driver skd_driver = {
>>>  	.name		= DRV_NAME,
>>>  	.id_table	= skd_pci_tbl,
>>>  	.probe		= skd_pci_probe,
>>>  	.remove		= skd_pci_remove,
>>> -	.suspend	= skd_pci_suspend,
>>> -	.resume		= skd_pci_resume,
>>> +	.driver.pm	= &skd_pci_pm_ops,
>>>  	.shutdown	= skd_pci_shutdown,
>>>  };
>>>  
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
> 


-- 
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 3/3] skd: " Vaibhav Gupta
@ 2020-07-21  2:57         ` Damien Le Moal
  2020-07-21  7:09           ` Vaibhav Gupta
  0 siblings, 1 reply; 28+ messages in thread
From: Damien Le Moal @ 2020-07-21  2:57 UTC (permalink / raw)
  To: Vaibhav Gupta, Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas,
	Vaibhav Gupta, Jens Axboe, Joshua Morris, Philip Kelleher
  Cc: linux-block, linux-kernel-mentees, linux-kernel

On 2020/07/20 22:32, Vaibhav Gupta wrote:
> Drivers using legacy PM have to manage PCI states and device's PM states
> themselves. They also need to take care of configuration registers.
> 
> With improved and powerful support of generic PM, PCI Core takes care of
> above mentioned, device-independent, jobs.
> 
> This driver makes use of PCI helper functions like
> pci_save/restore_state(), pci_enable/disable_device(),
> pci_request/release_regions(), pci_set_power_state() and
> pci_set_master() to do required operations. In generic mode, they are no
> longer needed.
> 
> Change function parameter in both .suspend() and .resume() to
> "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.

This commit message is rather vague, and the last sentence actually does not
describe correctly the change. What about something very simple, yet clear, like
this:

skd: use generic power management

Switch from the legacy .suspend()/.resume() power management interface to the
generic power management interface using the single .driver.pm() method. This
avoids the need for the driver to directly call most of the PCI helper functions
and device power state control functions as the generic power management
interface takes care of the necessary operations.

> 
> Compile-tested only.
> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/block/skd_main.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index 51569c199a6c..7f2d42900b38 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
>  	return;
>  }
>  
> -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused skd_pci_suspend(struct device *dev)
>  {
>  	int i;
>  	struct skd_device *skdev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	skdev = pci_get_drvdata(pdev);
>  	if (!skdev) {
> @@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>  	if (skdev->pcie_error_reporting_is_enabled)
>  		pci_disable_pcie_error_reporting(pdev);
>  
> -	pci_release_regions(pdev);
> -	pci_save_state(pdev);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
>  	return 0;
>  }
>  
> -static int skd_pci_resume(struct pci_dev *pdev)
> +static int __maybe_unused skd_pci_resume(struct device *dev)
>  {
>  	int i;
>  	int rc = 0;
>  	struct skd_device *skdev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	skdev = pci_get_drvdata(pdev);
>  	if (!skdev) {
> @@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		return -1;
>  	}
>  
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_enable_wake(pdev, PCI_D0, 0);
> -	pci_restore_state(pdev);
> +	device_wakeup_disable(dev);
>  
> -	rc = pci_enable_device(pdev);
> -	if (rc)
> -		return rc;
> -	rc = pci_request_regions(pdev, DRV_NAME);
> -	if (rc)
> -		goto err_out;
>  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>  	if (rc)
>  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> @@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		goto err_out_regions;
>  	}
>  
> -	pci_set_master(pdev);
>  	rc = pci_enable_pcie_error_reporting(pdev);
>  	if (rc) {
>  		dev_err(&pdev->dev,
> @@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		pci_disable_pcie_error_reporting(pdev);
>  
>  err_out_regions:
> -	pci_release_regions(pdev);
> -
> -err_out:
> -	pci_disable_device(pdev);
>  	return rc;
>  }
>  
> @@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
>  	skd_stop_device(skdev);
>  }
>  
> +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> +
>  static struct pci_driver skd_driver = {
>  	.name		= DRV_NAME,
>  	.id_table	= skd_pci_tbl,
>  	.probe		= skd_pci_probe,
>  	.remove		= skd_pci_remove,
> -	.suspend	= skd_pci_suspend,
> -	.resume		= skd_pci_resume,
> +	.driver.pm	= &skd_pci_pm_ops,
>  	.shutdown	= skd_pci_shutdown,
>  };
>  
> 

Apart from the commit message, this looks OK to me.
I will give this a spin today on the hardware to check.


-- 
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v1 3/3] skd: use generic power management
  2020-07-21  2:49       ` Damien Le Moal
@ 2020-07-21  7:04         ` Vaibhav Gupta
  0 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-21  7:04 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Jens Axboe, linux-kernel, linux-block, Bjorn Helgaas,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On Tue, Jul 21, 2020 at 02:49:06AM +0000, Damien Le Moal wrote:
> On 2020/07/20 22:18, Vaibhav Gupta wrote:
> > On Mon, Jul 20, 2020 at 12:52:14PM +0000, Damien Le Moal wrote:
> >> On 2020/07/17 17:10, Vaibhav Gupta wrote:
> >>> Change function parameter in both .suspend() and .resume() to
> >>> "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> >>>
> >>> Compile-tested only.
> >>
> >> I do not think this belongs into the commit message. This was mentioned in the
> >> cover letter.
> >>
> > The messages in cover letter and commit message are bit similar. But the commit
> > message has patch specific changes mentioned in it.
> 
> My point was about the "compile-tested only" mention in the commit message. That
> should be mentioned in the cover letter only. The goal of the patch review
> process is also to get these patches tested by others if you do not have access
> to the hardware. That is fine, and I can test for the skd driver. But a patch
> mentioning that it is untested cannot be applied, for obvious reasons :)
> 
> So I only requested that you remove the "compiled tested only" note. The commit
> message could be simpler too, see followup comment.
>
Oh okay. Thanks for clarification and pointing this out :) . From now on, I
will put "Compile-tested only." message only in cover letters.

Thanks.
--Vaibhav Gupta
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-21  2:57         ` Damien Le Moal
@ 2020-07-21  7:09           ` Vaibhav Gupta
  2020-07-22  6:28             ` Damien Le Moal
  2020-07-22 17:52             ` [Linux-kernel-mentees] [PATCH v2 " Bjorn Helgaas
  0 siblings, 2 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-21  7:09 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Jens Axboe, linux-kernel, linux-block, Bjorn Helgaas,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On Tue, Jul 21, 2020 at 02:57:28AM +0000, Damien Le Moal wrote:
> On 2020/07/20 22:32, Vaibhav Gupta wrote:
> > Drivers using legacy PM have to manage PCI states and device's PM states
> > themselves. They also need to take care of configuration registers.
> > 
> > With improved and powerful support of generic PM, PCI Core takes care of
> > above mentioned, device-independent, jobs.
> > 
> > This driver makes use of PCI helper functions like
> > pci_save/restore_state(), pci_enable/disable_device(),
> > pci_request/release_regions(), pci_set_power_state() and
> > pci_set_master() to do required operations. In generic mode, they are no
> > longer needed.
> > 
> > Change function parameter in both .suspend() and .resume() to
> > "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> 
> This commit message is rather vague, and the last sentence actually does not
> describe correctly the change. What about something very simple, yet clear, like
> this:
> 
> skd: use generic power management
> 
> Switch from the legacy .suspend()/.resume() power management interface to the
> generic power management interface using the single .driver.pm() method. This
> avoids the need for the driver to directly call most of the PCI helper functions
> and device power state control functions as the generic power management
> interface takes care of the necessary operations.
>
Okay. I will improve on it. Just inform me after testing that if any other
changes are required. I guess [PATCH 1/3] and [PATCH 2/3] are okay, so I will
only send v3 of [PATCH 3/3] after suggested changes.
> > 
> > Compile-tested only.
> > 
> > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > ---
> >  drivers/block/skd_main.c | 30 ++++++++----------------------
> >  1 file changed, 8 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > index 51569c199a6c..7f2d42900b38 100644
> > --- a/drivers/block/skd_main.c
> > +++ b/drivers/block/skd_main.c
> > @@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
> >  	return;
> >  }
> >  
> > -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __maybe_unused skd_pci_suspend(struct device *dev)
> >  {
> >  	int i;
> >  	struct skd_device *skdev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> >  
> >  	skdev = pci_get_drvdata(pdev);
> >  	if (!skdev) {
> > @@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> >  	if (skdev->pcie_error_reporting_is_enabled)
> >  		pci_disable_pcie_error_reporting(pdev);
> >  
> > -	pci_release_regions(pdev);
> > -	pci_save_state(pdev);
> > -	pci_disable_device(pdev);
> > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >  	return 0;
> >  }
> >  
> > -static int skd_pci_resume(struct pci_dev *pdev)
> > +static int __maybe_unused skd_pci_resume(struct device *dev)
> >  {
> >  	int i;
> >  	int rc = 0;
> >  	struct skd_device *skdev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> >  
> >  	skdev = pci_get_drvdata(pdev);
> >  	if (!skdev) {
> > @@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
> >  		return -1;
> >  	}
> >  
> > -	pci_set_power_state(pdev, PCI_D0);
> > -	pci_enable_wake(pdev, PCI_D0, 0);
> > -	pci_restore_state(pdev);
> > +	device_wakeup_disable(dev);
> >  
> > -	rc = pci_enable_device(pdev);
> > -	if (rc)
> > -		return rc;
> > -	rc = pci_request_regions(pdev, DRV_NAME);
> > -	if (rc)
> > -		goto err_out;
> >  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >  	if (rc)
> >  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > @@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> >  		goto err_out_regions;
> >  	}
> >  
> > -	pci_set_master(pdev);
> >  	rc = pci_enable_pcie_error_reporting(pdev);
> >  	if (rc) {
> >  		dev_err(&pdev->dev,
> > @@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> >  		pci_disable_pcie_error_reporting(pdev);
> >  
> >  err_out_regions:
> > -	pci_release_regions(pdev);
> > -
> > -err_out:
> > -	pci_disable_device(pdev);
> >  	return rc;
> >  }
> >  
> > @@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
> >  	skd_stop_device(skdev);
> >  }
> >  
> > +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> > +
> >  static struct pci_driver skd_driver = {
> >  	.name		= DRV_NAME,
> >  	.id_table	= skd_pci_tbl,
> >  	.probe		= skd_pci_probe,
> >  	.remove		= skd_pci_remove,
> > -	.suspend	= skd_pci_suspend,
> > -	.resume		= skd_pci_resume,
> > +	.driver.pm	= &skd_pci_pm_ops,
> >  	.shutdown	= skd_pci_shutdown,
> >  };
> >  
> > 
> 
> Apart from the commit message, this looks OK to me.
> I will give this a spin today on the hardware to check.
> 
Thanks!
--Vaibhav Gupta
> 
> -- 
> Damien Le Moal
> Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-21  7:09           ` Vaibhav Gupta
@ 2020-07-22  6:28             ` Damien Le Moal
  2020-07-22  7:21               ` Vaibhav Gupta
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
  2020-07-22 17:52             ` [Linux-kernel-mentees] [PATCH v2 " Bjorn Helgaas
  1 sibling, 2 replies; 28+ messages in thread
From: Damien Le Moal @ 2020-07-22  6:28 UTC (permalink / raw)
  To: vaibhavgupta40
  Cc: axboe, linux-kernel, linux-block, bhelgaas, helgaas, pjk1939,
	linux-kernel-mentees, josh.h.morris

On Tue, 2020-07-21 at 12:39 +0530, Vaibhav Gupta wrote:
> On Tue, Jul 21, 2020 at 02:57:28AM +0000, Damien Le Moal wrote:
> > On 2020/07/20 22:32, Vaibhav Gupta wrote:
> > > Drivers using legacy PM have to manage PCI states and device's PM states
> > > themselves. They also need to take care of configuration registers.
> > > 
> > > With improved and powerful support of generic PM, PCI Core takes care of
> > > above mentioned, device-independent, jobs.
> > > 
> > > This driver makes use of PCI helper functions like
> > > pci_save/restore_state(), pci_enable/disable_device(),
> > > pci_request/release_regions(), pci_set_power_state() and
> > > pci_set_master() to do required operations. In generic mode, they are no
> > > longer needed.
> > > 
> > > Change function parameter in both .suspend() and .resume() to
> > > "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> > 
> > This commit message is rather vague, and the last sentence actually does not
> > describe correctly the change. What about something very simple, yet clear, like
> > this:
> > 
> > skd: use generic power management
> > 
> > Switch from the legacy .suspend()/.resume() power management interface to the
> > generic power management interface using the single .driver.pm() method. This
> > avoids the need for the driver to directly call most of the PCI helper functions
> > and device power state control functions as the generic power management
> > interface takes care of the necessary operations.
> > 
> Okay. I will improve on it. Just inform me after testing that if any other
> changes are required. I guess [PATCH 1/3] and [PATCH 2/3] are okay, so I will
> only send v3 of [PATCH 3/3] after suggested changes.
> > > Compile-tested only.
> > > 
> > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > ---
> > >  drivers/block/skd_main.c | 30 ++++++++----------------------
> > >  1 file changed, 8 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > > index 51569c199a6c..7f2d42900b38 100644
> > > --- a/drivers/block/skd_main.c
> > > +++ b/drivers/block/skd_main.c
> > > @@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
> > >  	return;
> > >  }
> > >  
> > > -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > > +static int __maybe_unused skd_pci_suspend(struct device *dev)
> > >  {
> > >  	int i;
> > >  	struct skd_device *skdev;
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > >  
> > >  	skdev = pci_get_drvdata(pdev);
> > >  	if (!skdev) {
> > > @@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > >  	if (skdev->pcie_error_reporting_is_enabled)
> > >  		pci_disable_pcie_error_reporting(pdev);
> > >  
> > > -	pci_release_regions(pdev);
> > > -	pci_save_state(pdev);
> > > -	pci_disable_device(pdev);
> > > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > >  	return 0;
> > >  }
> > >  
> > > -static int skd_pci_resume(struct pci_dev *pdev)
> > > +static int __maybe_unused skd_pci_resume(struct device *dev)
> > >  {
> > >  	int i;
> > >  	int rc = 0;
> > >  	struct skd_device *skdev;
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > >  
> > >  	skdev = pci_get_drvdata(pdev);
> > >  	if (!skdev) {
> > > @@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > >  		return -1;
> > >  	}
> > >  
> > > -	pci_set_power_state(pdev, PCI_D0);
> > > -	pci_enable_wake(pdev, PCI_D0, 0);
> > > -	pci_restore_state(pdev);
> > > +	device_wakeup_disable(dev);
> > >  
> > > -	rc = pci_enable_device(pdev);
> > > -	if (rc)
> > > -		return rc;
> > > -	rc = pci_request_regions(pdev, DRV_NAME);
> > > -	if (rc)
> > > -		goto err_out;
> > >  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > >  	if (rc)
> > >  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > @@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > >  		goto err_out_regions;
> > >  	}
> > >  
> > > -	pci_set_master(pdev);
> > >  	rc = pci_enable_pcie_error_reporting(pdev);
> > >  	if (rc) {
> > >  		dev_err(&pdev->dev,
> > > @@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > >  		pci_disable_pcie_error_reporting(pdev);
> > >  
> > >  err_out_regions:
> > > -	pci_release_regions(pdev);
> > > -
> > > -err_out:
> > > -	pci_disable_device(pdev);
> > >  	return rc;
> > >  }
> > >  
> > > @@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
> > >  	skd_stop_device(skdev);
> > >  }
> > >  
> > > +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> > > +
> > >  static struct pci_driver skd_driver = {
> > >  	.name		= DRV_NAME,
> > >  	.id_table	= skd_pci_tbl,
> > >  	.probe		= skd_pci_probe,
> > >  	.remove		= skd_pci_remove,
> > > -	.suspend	= skd_pci_suspend,
> > > -	.resume		= skd_pci_resume,
> > > +	.driver.pm	= &skd_pci_pm_ops,
> > >  	.shutdown	= skd_pci_shutdown,
> > >  };
> > >  
> > > 
> > 
> > Apart from the commit message, this looks OK to me.
> > I will give this a spin today on the hardware to check.
> > 
> Thanks!

Tested this and it seems OK.

Of note, after having removed the device (echo 1 >
/sys/block/skd0/device/remove) and rescaning its PCI slot, I got a
lockdep splat:

[ 4449.098757] ======================================================
[ 4449.104945] WARNING: possible circular locking dependency detected
[ 4449.111136] 5.8.0-rc6+ #846 Not tainted
[ 4449.114980] ------------------------------------------------------
[ 4449.121168] tcsh/1550 is trying to acquire lock:
[ 4449.125798] ffffffffa85c3748 (pci_rescan_remove_lock){+.+.}-{3:3},
at: dev_rescan_store+0x38/0x60
[ 4449.134686] 
[ 4449.134686] but task is already holding lock:
[ 4449.140522] ffff8f6cd352ed28 (kn->active#294){++++}-{0:0}, at:
kernfs_fop_write+0xad/0x1c0
[ 4449.148795] 
[ 4449.148795] which lock already depends on the new lock.
[ 4449.148795] 
[ 4449.156979] 
[ 4449.156979] the existing dependency chain (in reverse order) is:
[ 4449.164464] 
[ 4449.164464] -> #1 (kn->active#294){++++}-{0:0}:
[ 4449.170486]        __kernfs_remove+0x276/0x2f0
[ 4449.174939]        kernfs_remove_by_name_ns+0x41/0x80
[ 4449.180001]        remove_files+0x2b/0x60
[ 4449.184018]        sysfs_remove_group+0x38/0x80
[ 4449.188560]        sysfs_remove_groups+0x29/0x40
[ 4449.193190]        device_remove_attrs+0x4b/0x70
[ 4449.197818]        device_del+0x167/0x3f0
[ 4449.201843]        pci_remove_bus_device+0x70/0x110
[ 4449.206728]        pci_stop_and_remove_bus_device_locked+0x1e/0x30
[ 4449.212915]        remove_store+0x55/0x60
[ 4449.216939]        kernfs_fop_write+0xd9/0x1c0
[ 4449.221398]        vfs_write+0xc7/0x1f0
[ 4449.225239]        ksys_write+0x58/0xd0
[ 4449.229089]        do_syscall_64+0x4f/0x2c0
[ 4449.233284]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 4449.238863] 
[ 4449.238863] -> #0 (pci_rescan_remove_lock){+.+.}-{3:3}:
[ 4449.245575]        __lock_acquire+0x1194/0x1fd0
[ 4449.250113]        lock_acquire+0x97/0x390
[ 4449.254221]        __mutex_lock+0x58/0x820
[ 4449.258330]        dev_rescan_store+0x38/0x60
[ 4449.262697]        kernfs_fop_write+0xd9/0x1c0
[ 4449.267154]        vfs_write+0xc7/0x1f0
[ 4449.270999]        ksys_write+0x58/0xd0
[ 4449.274846]        do_syscall_64+0x4f/0x2c0
[ 4449.279041]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 4449.284620] 
[ 4449.284620] other info that might help us debug this:
[ 4449.284620] 
[ 4449.292628]  Possible unsafe locking scenario:
[ 4449.292628] 
[ 4449.298555]        CPU0                    CPU1
[ 4449.303090]        ----                    ----
[ 4449.307630]   lock(kn->active#294);
[
4449.311131]                                lock(pci_rescan_remove_lock
);
[ 4449.317838]                                lock(kn->active#294);
[ 4449.323855]   lock(pci_rescan_remove_lock);
[ 4449.328048] 
[ 4449.328048]  *** DEADLOCK ***
[ 4449.328048] 
[ 4449.333977] 3 locks held by tcsh/1550:
[ 4449.337731]  #0: ffff8f6f18b7d448 (sb_writers#4){.+.+}-{0:0}, at:
vfs_write+0x174/0x1f0
[ 4449.345747]  #1: ffff8f6e74d09a88 (&of->mutex){+.+.}-{3:3}, at:
kernfs_fop_write+0xa5/0x1c0
[ 4449.354111]  #2: ffff8f6cd352ed28 (kn->active#294){++++}-{0:0}, at:
kernfs_fop_write+0xad/0x1c0
[ 4449.362821] 
[ 4449.362821] stack backtrace:
[ 4449.367192] CPU: 15 PID: 1550 Comm: tcsh Not tainted 5.8.0-rc6+ #846
[ 4449.373548] Hardware name: Supermicro Super Server/X11DPL-i, BIOS
3.1 05/21/2019
[ 4449.380953] Call Trace:
[ 4449.383419]  dump_stack+0x78/0xa0
[ 4449.386744]  check_noncircular+0x12d/0x150
[ 4449.390853]  __lock_acquire+0x1194/0x1fd0
[ 4449.394875]  lock_acquire+0x97/0x390
[ 4449.398462]  ? dev_rescan_store+0x38/0x60
[ 4449.402486]  __mutex_lock+0x58/0x820
[ 4449.406067]  ? dev_rescan_store+0x38/0x60
[ 4449.410090]  ? dev_rescan_store+0x38/0x60
[ 4449.414113]  ? lock_acquire+0x97/0x390
[ 4449.417876]  ? kernfs_fop_write+0xad/0x1c0
[ 4449.421983]  dev_rescan_store+0x38/0x60
[ 4449.425831]  kernfs_fop_write+0xd9/0x1c0
[ 4449.429768]  vfs_write+0xc7/0x1f0
[ 4449.433092]  ksys_write+0x58/0xd0
[ 4449.436421]  do_syscall_64+0x4f/0x2c0
[ 4449.440094]  ? prepare_exit_to_usermode+0xa/0x40
[ 4449.444725]  ? rcu_read_lock_sched_held+0x3f/0x50
[ 4449.449436]  ? asm_exc_page_fault+0x8/0x30
[ 4449.453546]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 4449.458608] RIP: 0033:0x7fcce58f45e7
[ 4449.462191] Code: Bad RIP value.
[ 4449.465426] RSP: 002b:00007fff2592c898 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 4449.473000] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
00007fcce58f45e7
[ 4449.480141] RDX: 0000000000000002 RSI: 0000556d30b96100 RDI:
0000000000000001
[ 4449.487283] RBP: 0000556d30b96100 R08: 0000000000000000 R09:
00007fff2592c7f0
[ 4449.494425] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000000001
[ 4449.501567] R13: 0000556d31fc4fb8 R14: 0000556d31fc37c0 R15:
0000556d31fc4fb4
[ 4449.520721] pci 0000:d8:00.0: [1b39:0001] type 00 class 0x018000
[ 4449.526802] pci 0000:d8:00.0: reg 0x10: [mem 0xfbe00000-0xfbe0ffff]
[ 4449.533121] pci 0000:d8:00.0: reg 0x14: [mem 0xfbe10000-0xfbe10fff]
[ 4449.539474] pci 0000:d8:00.0: reg 0x30: [mem 0xfbd00000-0xfbdfffff
pref]
[ 4449.546327] pci 0000:d8:00.0: supports D1 D2
[ 4449.550936] pci 0000:d8:00.0: BAR 6: assigned [mem 0xfbd00000-
0xfbdfffff pref]
[ 4449.559804] pci 0000:d8:00.0: BAR 0: assigned [mem 0xfbe00000-
0xfbe0ffff]
[ 4449.568849] pci 0000:d8:00.0: BAR 1: assigned [mem 0xfbe10000-
0xfbe10fff]
[ 4449.577178] skd 0000:d8:00.0: PCIe (5.0GT/s 4X) 64bit
[ 4449.582272] skd 0000:d8:00.0: bad enable of PCIe error reporting
rc=-5
[ 4449.589764] skd 0000:d8:00.0: s1120 state INIT(1)=>SOFT_RESET(8)
[ 4449.595788] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
[ 4449.829900] skd 0000:d8:00.0: s1120 state SOFT_RESET(8)=>INIT(1)
[ 4449.835929] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
[ 4450.076148] skd 0000:d8:00.0: Time sync driver=0x5f17d872
device=0x162bdfe9
[ 4450.083139] skd 0000:d8:00.0: s1120 state INIT(1)=>ONLINE(3)
[ 4450.088822] skd 0000:d8:00.0: Queue depth limit=64 dev=255 lowat=43
[ 4450.095101] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
[ 4450.101539] skd 0000:d8:00.0: Driver state STARTING(3)=>ONLINE(4)
[ 4450.107653] skd 0000:d8:00.0: STEC s1120 ONLINE
[ 4450.120422]  skd1: p1

But that looks completely unrelated to your patch. Will have a look at
this.

-- 
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-22  6:28             ` Damien Le Moal
@ 2020-07-22  7:21               ` Vaibhav Gupta
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
  1 sibling, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22  7:21 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: axboe, linux-kernel, linux-block, bhelgaas, helgaas, pjk1939,
	linux-kernel-mentees, josh.h.morris

On Wed, Jul 22, 2020 at 06:28:58AM +0000, Damien Le Moal wrote:
> On Tue, 2020-07-21 at 12:39 +0530, Vaibhav Gupta wrote:
> > On Tue, Jul 21, 2020 at 02:57:28AM +0000, Damien Le Moal wrote:
> > > On 2020/07/20 22:32, Vaibhav Gupta wrote:
> > > > Drivers using legacy PM have to manage PCI states and device's PM states
> > > > themselves. They also need to take care of configuration registers.
> > > > 
> > > > With improved and powerful support of generic PM, PCI Core takes care of
> > > > above mentioned, device-independent, jobs.
> > > > 
> > > > This driver makes use of PCI helper functions like
> > > > pci_save/restore_state(), pci_enable/disable_device(),
> > > > pci_request/release_regions(), pci_set_power_state() and
> > > > pci_set_master() to do required operations. In generic mode, they are no
> > > > longer needed.
> > > > 
> > > > Change function parameter in both .suspend() and .resume() to
> > > > "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variable.
> > > 
> > > This commit message is rather vague, and the last sentence actually does not
> > > describe correctly the change. What about something very simple, yet clear, like
> > > this:
> > > 
> > > skd: use generic power management
> > > 
> > > Switch from the legacy .suspend()/.resume() power management interface to the
> > > generic power management interface using the single .driver.pm() method. This
> > > avoids the need for the driver to directly call most of the PCI helper functions
> > > and device power state control functions as the generic power management
> > > interface takes care of the necessary operations.
> > > 
> > Okay. I will improve on it. Just inform me after testing that if any other
> > changes are required. I guess [PATCH 1/3] and [PATCH 2/3] are okay, so I will
> > only send v3 of [PATCH 3/3] after suggested changes.
> > > > Compile-tested only.
> > > > 
> > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > > ---
> > > >  drivers/block/skd_main.c | 30 ++++++++----------------------
> > > >  1 file changed, 8 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> > > > index 51569c199a6c..7f2d42900b38 100644
> > > > --- a/drivers/block/skd_main.c
> > > > +++ b/drivers/block/skd_main.c
> > > > @@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
> > > >  	return;
> > > >  }
> > > >  
> > > > -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > > > +static int __maybe_unused skd_pci_suspend(struct device *dev)
> > > >  {
> > > >  	int i;
> > > >  	struct skd_device *skdev;
> > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > >  
> > > >  	skdev = pci_get_drvdata(pdev);
> > > >  	if (!skdev) {
> > > > @@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> > > >  	if (skdev->pcie_error_reporting_is_enabled)
> > > >  		pci_disable_pcie_error_reporting(pdev);
> > > >  
> > > > -	pci_release_regions(pdev);
> > > > -	pci_save_state(pdev);
> > > > -	pci_disable_device(pdev);
> > > > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int skd_pci_resume(struct pci_dev *pdev)
> > > > +static int __maybe_unused skd_pci_resume(struct device *dev)
> > > >  {
> > > >  	int i;
> > > >  	int rc = 0;
> > > >  	struct skd_device *skdev;
> > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > >  
> > > >  	skdev = pci_get_drvdata(pdev);
> > > >  	if (!skdev) {
> > > > @@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > > >  		return -1;
> > > >  	}
> > > >  
> > > > -	pci_set_power_state(pdev, PCI_D0);
> > > > -	pci_enable_wake(pdev, PCI_D0, 0);
> > > > -	pci_restore_state(pdev);
> > > > +	device_wakeup_disable(dev);
> > > >  
> > > > -	rc = pci_enable_device(pdev);
> > > > -	if (rc)
> > > > -		return rc;
> > > > -	rc = pci_request_regions(pdev, DRV_NAME);
> > > > -	if (rc)
> > > > -		goto err_out;
> > > >  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > > >  	if (rc)
> > > >  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > > @@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > > >  		goto err_out_regions;
> > > >  	}
> > > >  
> > > > -	pci_set_master(pdev);
> > > >  	rc = pci_enable_pcie_error_reporting(pdev);
> > > >  	if (rc) {
> > > >  		dev_err(&pdev->dev,
> > > > @@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
> > > >  		pci_disable_pcie_error_reporting(pdev);
> > > >  
> > > >  err_out_regions:
> > > > -	pci_release_regions(pdev);
> > > > -
> > > > -err_out:
> > > > -	pci_disable_device(pdev);
> > > >  	return rc;
> > > >  }
> > > >  
> > > > @@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
> > > >  	skd_stop_device(skdev);
> > > >  }
> > > >  
> > > > +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> > > > +
> > > >  static struct pci_driver skd_driver = {
> > > >  	.name		= DRV_NAME,
> > > >  	.id_table	= skd_pci_tbl,
> > > >  	.probe		= skd_pci_probe,
> > > >  	.remove		= skd_pci_remove,
> > > > -	.suspend	= skd_pci_suspend,
> > > > -	.resume		= skd_pci_resume,
> > > > +	.driver.pm	= &skd_pci_pm_ops,
> > > >  	.shutdown	= skd_pci_shutdown,
> > > >  };
> > > >  
> > > > 
> > > 
> > > Apart from the commit message, this looks OK to me.
> > > I will give this a spin today on the hardware to check.
> > > 
> > Thanks!
> 
> Tested this and it seems OK.
> 
> Of note, after having removed the device (echo 1 >
> /sys/block/skd0/device/remove) and rescaning its PCI slot, I got a
> lockdep splat:
> 
> [ 4449.098757] ======================================================
> [ 4449.104945] WARNING: possible circular locking dependency detected
> [ 4449.111136] 5.8.0-rc6+ #846 Not tainted
> [ 4449.114980] ------------------------------------------------------
> [ 4449.121168] tcsh/1550 is trying to acquire lock:
> [ 4449.125798] ffffffffa85c3748 (pci_rescan_remove_lock){+.+.}-{3:3},
> at: dev_rescan_store+0x38/0x60
> [ 4449.134686] 
> [ 4449.134686] but task is already holding lock:
> [ 4449.140522] ffff8f6cd352ed28 (kn->active#294){++++}-{0:0}, at:
> kernfs_fop_write+0xad/0x1c0
> [ 4449.148795] 
> [ 4449.148795] which lock already depends on the new lock.
> [ 4449.148795] 
> [ 4449.156979] 
> [ 4449.156979] the existing dependency chain (in reverse order) is:
> [ 4449.164464] 
> [ 4449.164464] -> #1 (kn->active#294){++++}-{0:0}:
> [ 4449.170486]        __kernfs_remove+0x276/0x2f0
> [ 4449.174939]        kernfs_remove_by_name_ns+0x41/0x80
> [ 4449.180001]        remove_files+0x2b/0x60
> [ 4449.184018]        sysfs_remove_group+0x38/0x80
> [ 4449.188560]        sysfs_remove_groups+0x29/0x40
> [ 4449.193190]        device_remove_attrs+0x4b/0x70
> [ 4449.197818]        device_del+0x167/0x3f0
> [ 4449.201843]        pci_remove_bus_device+0x70/0x110
> [ 4449.206728]        pci_stop_and_remove_bus_device_locked+0x1e/0x30
> [ 4449.212915]        remove_store+0x55/0x60
> [ 4449.216939]        kernfs_fop_write+0xd9/0x1c0
> [ 4449.221398]        vfs_write+0xc7/0x1f0
> [ 4449.225239]        ksys_write+0x58/0xd0
> [ 4449.229089]        do_syscall_64+0x4f/0x2c0
> [ 4449.233284]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 4449.238863] 
> [ 4449.238863] -> #0 (pci_rescan_remove_lock){+.+.}-{3:3}:
> [ 4449.245575]        __lock_acquire+0x1194/0x1fd0
> [ 4449.250113]        lock_acquire+0x97/0x390
> [ 4449.254221]        __mutex_lock+0x58/0x820
> [ 4449.258330]        dev_rescan_store+0x38/0x60
> [ 4449.262697]        kernfs_fop_write+0xd9/0x1c0
> [ 4449.267154]        vfs_write+0xc7/0x1f0
> [ 4449.270999]        ksys_write+0x58/0xd0
> [ 4449.274846]        do_syscall_64+0x4f/0x2c0
> [ 4449.279041]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 4449.284620] 
> [ 4449.284620] other info that might help us debug this:
> [ 4449.284620] 
> [ 4449.292628]  Possible unsafe locking scenario:
> [ 4449.292628] 
> [ 4449.298555]        CPU0                    CPU1
> [ 4449.303090]        ----                    ----
> [ 4449.307630]   lock(kn->active#294);
> [
> 4449.311131]                                lock(pci_rescan_remove_lock
> );
> [ 4449.317838]                                lock(kn->active#294);
> [ 4449.323855]   lock(pci_rescan_remove_lock);
> [ 4449.328048] 
> [ 4449.328048]  *** DEADLOCK ***
> [ 4449.328048] 
> [ 4449.333977] 3 locks held by tcsh/1550:
> [ 4449.337731]  #0: ffff8f6f18b7d448 (sb_writers#4){.+.+}-{0:0}, at:
> vfs_write+0x174/0x1f0
> [ 4449.345747]  #1: ffff8f6e74d09a88 (&of->mutex){+.+.}-{3:3}, at:
> kernfs_fop_write+0xa5/0x1c0
> [ 4449.354111]  #2: ffff8f6cd352ed28 (kn->active#294){++++}-{0:0}, at:
> kernfs_fop_write+0xad/0x1c0
> [ 4449.362821] 
> [ 4449.362821] stack backtrace:
> [ 4449.367192] CPU: 15 PID: 1550 Comm: tcsh Not tainted 5.8.0-rc6+ #846
> [ 4449.373548] Hardware name: Supermicro Super Server/X11DPL-i, BIOS
> 3.1 05/21/2019
> [ 4449.380953] Call Trace:
> [ 4449.383419]  dump_stack+0x78/0xa0
> [ 4449.386744]  check_noncircular+0x12d/0x150
> [ 4449.390853]  __lock_acquire+0x1194/0x1fd0
> [ 4449.394875]  lock_acquire+0x97/0x390
> [ 4449.398462]  ? dev_rescan_store+0x38/0x60
> [ 4449.402486]  __mutex_lock+0x58/0x820
> [ 4449.406067]  ? dev_rescan_store+0x38/0x60
> [ 4449.410090]  ? dev_rescan_store+0x38/0x60
> [ 4449.414113]  ? lock_acquire+0x97/0x390
> [ 4449.417876]  ? kernfs_fop_write+0xad/0x1c0
> [ 4449.421983]  dev_rescan_store+0x38/0x60
> [ 4449.425831]  kernfs_fop_write+0xd9/0x1c0
> [ 4449.429768]  vfs_write+0xc7/0x1f0
> [ 4449.433092]  ksys_write+0x58/0xd0
> [ 4449.436421]  do_syscall_64+0x4f/0x2c0
> [ 4449.440094]  ? prepare_exit_to_usermode+0xa/0x40
> [ 4449.444725]  ? rcu_read_lock_sched_held+0x3f/0x50
> [ 4449.449436]  ? asm_exc_page_fault+0x8/0x30
> [ 4449.453546]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 4449.458608] RIP: 0033:0x7fcce58f45e7
> [ 4449.462191] Code: Bad RIP value.
> [ 4449.465426] RSP: 002b:00007fff2592c898 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000001
> [ 4449.473000] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
> 00007fcce58f45e7
> [ 4449.480141] RDX: 0000000000000002 RSI: 0000556d30b96100 RDI:
> 0000000000000001
> [ 4449.487283] RBP: 0000556d30b96100 R08: 0000000000000000 R09:
> 00007fff2592c7f0
> [ 4449.494425] R10: 0000000000000000 R11: 0000000000000246 R12:
> 0000000000000001
> [ 4449.501567] R13: 0000556d31fc4fb8 R14: 0000556d31fc37c0 R15:
> 0000556d31fc4fb4
> [ 4449.520721] pci 0000:d8:00.0: [1b39:0001] type 00 class 0x018000
> [ 4449.526802] pci 0000:d8:00.0: reg 0x10: [mem 0xfbe00000-0xfbe0ffff]
> [ 4449.533121] pci 0000:d8:00.0: reg 0x14: [mem 0xfbe10000-0xfbe10fff]
> [ 4449.539474] pci 0000:d8:00.0: reg 0x30: [mem 0xfbd00000-0xfbdfffff
> pref]
> [ 4449.546327] pci 0000:d8:00.0: supports D1 D2
> [ 4449.550936] pci 0000:d8:00.0: BAR 6: assigned [mem 0xfbd00000-
> 0xfbdfffff pref]
> [ 4449.559804] pci 0000:d8:00.0: BAR 0: assigned [mem 0xfbe00000-
> 0xfbe0ffff]
> [ 4449.568849] pci 0000:d8:00.0: BAR 1: assigned [mem 0xfbe10000-
> 0xfbe10fff]
> [ 4449.577178] skd 0000:d8:00.0: PCIe (5.0GT/s 4X) 64bit
> [ 4449.582272] skd 0000:d8:00.0: bad enable of PCIe error reporting
> rc=-5
> [ 4449.589764] skd 0000:d8:00.0: s1120 state INIT(1)=>SOFT_RESET(8)
> [ 4449.595788] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
> [ 4449.829900] skd 0000:d8:00.0: s1120 state SOFT_RESET(8)=>INIT(1)
> [ 4449.835929] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
> [ 4450.076148] skd 0000:d8:00.0: Time sync driver=0x5f17d872
> device=0x162bdfe9
> [ 4450.083139] skd 0000:d8:00.0: s1120 state INIT(1)=>ONLINE(3)
> [ 4450.088822] skd 0000:d8:00.0: Queue depth limit=64 dev=255 lowat=43
> [ 4450.095101] skd 0000:d8:00.0: Driver state STARTING(3)=>STARTING(3)
> [ 4450.101539] skd 0000:d8:00.0: Driver state STARTING(3)=>ONLINE(4)
> [ 4450.107653] skd 0000:d8:00.0: STEC s1120 ONLINE
> [ 4450.120422]  skd1: p1
> 
> But that looks completely unrelated to your patch. Will have a look at
> this.
> 
Thanks. I will send the patch-series as v3 with updated commit message as
suggested.

--Vaibhav Gupta
> -- 
> Damien Le Moal
> Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v3 0/3] block: use generic power management
  2020-07-22  6:28             ` Damien Le Moal
  2020-07-22  7:21               ` Vaibhav Gupta
@ 2020-07-22  8:33               ` Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 1/3] mtip32xx: " Vaibhav Gupta
                                   ` (4 more replies)
  1 sibling, 5 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22  8:33 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

Linux Kernel Mentee: Remove Legacy Power Management.

The purpose of this patch series is to upgrade power management in block
drivers. This has been done by upgrading .suspend() and .resume() callbacks.

The upgrade makes sure that the involvement of PCI Core does not change the
order of operations executed in a driver. Thus, does not change its behavior.

In general, drivers with legacy PM, .suspend() and .resume() make use of PCI
helper functions like pci_enable/disable_device_mem(), pci_set_power_state(),
pci_save/restore_state(), pci_enable/disable_device(), etc. to complete
their job.

The conversion requires the removal of those function calls, change the
callbacks' definition accordingly and make use of dev_pm_ops structure.

All patches are compile-tested only.

Test tools:
    - Compiler: gcc (GCC) 10.1.0
    - allmodconfig build: make -j$(nproc) W=1 all

v3: v2 needed some changes in commit messages.

Vaibhav Gupta (3):
  mtip32xx: use generic power management
  rsxx: use generic power management
  skd: use generic power management

 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 drivers/block/rsxx/core.c         |  9 ++++--
 drivers/block/skd_main.c          | 30 +++++------------
 3 files changed, 27 insertions(+), 66 deletions(-)

-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v3 1/3] mtip32xx: use generic power management
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
@ 2020-07-22  8:33                 ` Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 2/3] rsxx: " Vaibhav Gupta
                                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22  8:33 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

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 <vaibhavgupta40@gmail.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index f6bafa9a68b9..7d1280952b35 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -4246,14 +4246,13 @@ static void mtip_pci_remove(struct pci_dev *pdev)
  *	0  Success
  *	<0 Error
  */
-static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
+static int __maybe_unused mtip_pci_suspend(struct device *dev)
 {
 	int rv = 0;
-	struct driver_data *dd = pci_get_drvdata(pdev);
+	struct driver_data *dd = dev_get_drvdata(dev);
 
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
@@ -4261,21 +4260,8 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 
 	/* Disable ports & interrupts then send standby immediate */
 	rv = mtip_block_suspend(dd);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to suspend controller\n");
-		return rv;
-	}
-
-	/*
-	 * Save the pci config space to pdev structure &
-	 * disable the device
-	 */
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-
-	/* Move to Low power state*/
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (rv < 0)
+		dev_err(dev, "Failed to suspend controller\n");
 
 	return rv;
 }
@@ -4287,42 +4273,25 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
  *      0  Success
  *      <0 Error
  */
-static int mtip_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused mtip_pci_resume(struct device *dev)
 {
 	int rv = 0;
 	struct driver_data *dd;
 
-	dd = pci_get_drvdata(pdev);
+	dd = dev_get_drvdata(dev);
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
-	/* Move the device to active State */
-	pci_set_power_state(pdev, PCI_D0);
-
-	/* Restore PCI configuration space */
-	pci_restore_state(pdev);
-
-	/* Enable the PCI device*/
-	rv = pcim_enable_device(pdev);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to enable card during resume\n");
-		goto err;
-	}
-	pci_set_master(pdev);
-
 	/*
 	 * Calls hbaReset, initPort, & startPort function
 	 * then enables interrupts
 	 */
 	rv = mtip_block_resume(dd);
 	if (rv < 0)
-		dev_err(&pdev->dev, "Unable to resume\n");
+		dev_err(dev, "Unable to resume\n");
 
-err:
 	clear_bit(MTIP_DDF_RESUME_BIT, &dd->dd_flag);
 
 	return rv;
@@ -4353,14 +4322,15 @@ static const struct pci_device_id mtip_pci_tbl[] = {
 	{ 0 }
 };
 
+static SIMPLE_DEV_PM_OPS(mtip_pci_pm_ops, mtip_pci_suspend, mtip_pci_resume);
+
 /* Structure that describes the PCI driver functions. */
 static struct pci_driver mtip_pci_driver = {
 	.name			= MTIP_DRV_NAME,
 	.id_table		= mtip_pci_tbl,
 	.probe			= mtip_pci_probe,
 	.remove			= mtip_pci_remove,
-	.suspend		= mtip_pci_suspend,
-	.resume			= mtip_pci_resume,
+	.driver.pm		= &mtip_pci_pm_ops,
 	.shutdown		= mtip_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v3 2/3] rsxx: use generic power management
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 1/3] mtip32xx: " Vaibhav Gupta
@ 2020-07-22  8:33                 ` Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 3/3] skd: " Vaibhav Gupta
                                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22  8:33 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

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 <vaibhavgupta40@gmail.com>
---
 drivers/block/rsxx/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 10f6368117d8..866153fd380a 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -1030,12 +1030,15 @@ static void rsxx_pci_remove(struct pci_dev *dev)
 	kfree(card);
 }
 
-static int rsxx_pci_suspend(struct pci_dev *dev, pm_message_t state)
+static int __maybe_unused rsxx_pci_suspend(
+	__attribute__((unused)) struct device *dev)
 {
 	/* We don't support suspend at this time. */
 	return -ENOSYS;
 }
 
+#define rsxx_pci_resume NULL
+
 static void rsxx_pci_shutdown(struct pci_dev *dev)
 {
 	struct rsxx_cardinfo *card = pci_get_drvdata(dev);
@@ -1071,12 +1074,14 @@ static const struct pci_device_id rsxx_pci_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, rsxx_pci_ids);
 
+static SIMPLE_DEV_PM_OPS(rsxx_pci_pm_ops, rsxx_pci_suspend, rsxx_pci_resume);
+
 static struct pci_driver rsxx_pci_driver = {
 	.name		= DRIVER_NAME,
 	.id_table	= rsxx_pci_ids,
 	.probe		= rsxx_pci_probe,
 	.remove		= rsxx_pci_remove,
-	.suspend	= rsxx_pci_suspend,
+	.driver.pm	= &rsxx_pci_pm_ops,
 	.shutdown	= rsxx_pci_shutdown,
 	.err_handler    = &rsxx_err_handler,
 };
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v3 3/3] skd: use generic power management
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 1/3] mtip32xx: " Vaibhav Gupta
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 2/3] rsxx: " Vaibhav Gupta
@ 2020-07-22  8:33                 ` Vaibhav Gupta
  2020-07-27  2:14                   ` Damien Le Moal
  2020-08-17  7:55                 ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
  2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
  4 siblings, 1 reply; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22  8:33 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel, Vaibhav Gupta

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 <vaibhavgupta40@gmail.com>
---
 drivers/block/skd_main.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index 51569c199a6c..7f2d42900b38 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
 	return;
 }
 
-static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused skd_pci_suspend(struct device *dev)
 {
 	int i;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 	if (skdev->pcie_error_reporting_is_enabled)
 		pci_disable_pcie_error_reporting(pdev);
 
-	pci_release_regions(pdev);
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	return 0;
 }
 
-static int skd_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused skd_pci_resume(struct device *dev)
 {
 	int i;
 	int rc = 0;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		return -1;
 	}
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_enable_wake(pdev, PCI_D0, 0);
-	pci_restore_state(pdev);
+	device_wakeup_disable(dev);
 
-	rc = pci_enable_device(pdev);
-	if (rc)
-		return rc;
-	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
-		goto err_out;
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
@@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		goto err_out_regions;
 	}
 
-	pci_set_master(pdev);
 	rc = pci_enable_pcie_error_reporting(pdev);
 	if (rc) {
 		dev_err(&pdev->dev,
@@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		pci_disable_pcie_error_reporting(pdev);
 
 err_out_regions:
-	pci_release_regions(pdev);
-
-err_out:
-	pci_disable_device(pdev);
 	return rc;
 }
 
@@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
 	skd_stop_device(skdev);
 }
 
+static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
+
 static struct pci_driver skd_driver = {
 	.name		= DRV_NAME,
 	.id_table	= skd_pci_tbl,
 	.probe		= skd_pci_probe,
 	.remove		= skd_pci_remove,
-	.suspend	= skd_pci_suspend,
-	.resume		= skd_pci_resume,
+	.driver.pm	= &skd_pci_pm_ops,
 	.shutdown	= skd_pci_shutdown,
 };
 
-- 
2.27.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-21  7:09           ` Vaibhav Gupta
  2020-07-22  6:28             ` Damien Le Moal
@ 2020-07-22 17:52             ` Bjorn Helgaas
  2020-07-22 18:07               ` Vaibhav Gupta
  1 sibling, 1 reply; 28+ messages in thread
From: Bjorn Helgaas @ 2020-07-22 17:52 UTC (permalink / raw)
  To: Vaibhav Gupta
  Cc: Jens Axboe, Damien Le Moal, linux-kernel, linux-block,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On Tue, Jul 21, 2020 at 12:39:39PM +0530, Vaibhav Gupta wrote:
> Okay. I will improve on it. Just inform me after testing that if any other
> changes are required. I guess [PATCH 1/3] and [PATCH 2/3] are okay, so I will
> only send v3 of [PATCH 3/3] after suggested changes.

FWIW, there's a recent conversation on users@linux.kernel.org about
updating individual patches in a series (sorry, can't find a link to
it).  But the gist of it was that posting only [v3 3/3] leads to
confusion because

  - we can't tell whether [v3 1/3] and [v3 2/3] got lost en-route, and

  - collecting things from v2 and v3 is more work for the maintainer.

Bottom line: repost the whole series, even if some patches haven't
changed.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 3/3] skd: use generic power management
  2020-07-22 17:52             ` [Linux-kernel-mentees] [PATCH v2 " Bjorn Helgaas
@ 2020-07-22 18:07               ` Vaibhav Gupta
  0 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-07-22 18:07 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jens Axboe, Damien Le Moal, linux-kernel, linux-block,
	Bjorn Helgaas, Philip Kelleher, linux-kernel-mentees,
	Joshua Morris

On Wed, Jul 22, 2020 at 12:52:47PM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 21, 2020 at 12:39:39PM +0530, Vaibhav Gupta wrote:
> > Okay. I will improve on it. Just inform me after testing that if any other
> > changes are required. I guess [PATCH 1/3] and [PATCH 2/3] are okay, so I will
> > only send v3 of [PATCH 3/3] after suggested changes.
> 
> FWIW, there's a recent conversation on users@linux.kernel.org about
> updating individual patches in a series (sorry, can't find a link to
> it).  But the gist of it was that posting only [v3 3/3] leads to
> confusion because
> 
>   - we can't tell whether [v3 1/3] and [v3 2/3] got lost en-route, and
> 
>   - collecting things from v2 and v3 is more work for the maintainer.
> 
> Bottom line: repost the whole series, even if some patches haven't
> changed.
Thanks a lot for updating me with the info :D .
I sent the whole series as v3 hours ago.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3 3/3] skd: use generic power management
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 3/3] skd: " Vaibhav Gupta
@ 2020-07-27  2:14                   ` Damien Le Moal
  0 siblings, 0 replies; 28+ messages in thread
From: Damien Le Moal @ 2020-07-27  2:14 UTC (permalink / raw)
  To: Vaibhav Gupta, Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas,
	Vaibhav Gupta, Jens Axboe, Joshua Morris, Philip Kelleher
  Cc: linux-block, linux-kernel-mentees, linux-kernel

On 2020/07/22 17:35, 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 <vaibhavgupta40@gmail.com>
> ---
>  drivers/block/skd_main.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
> index 51569c199a6c..7f2d42900b38 100644
> --- a/drivers/block/skd_main.c
> +++ b/drivers/block/skd_main.c
> @@ -3315,10 +3315,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
>  	return;
>  }
>  
> -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused skd_pci_suspend(struct device *dev)
>  {
>  	int i;
>  	struct skd_device *skdev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	skdev = pci_get_drvdata(pdev);
>  	if (!skdev) {
> @@ -3337,18 +3338,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>  	if (skdev->pcie_error_reporting_is_enabled)
>  		pci_disable_pcie_error_reporting(pdev);
>  
> -	pci_release_regions(pdev);
> -	pci_save_state(pdev);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
>  	return 0;
>  }
>  
> -static int skd_pci_resume(struct pci_dev *pdev)
> +static int __maybe_unused skd_pci_resume(struct device *dev)
>  {
>  	int i;
>  	int rc = 0;
>  	struct skd_device *skdev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	skdev = pci_get_drvdata(pdev);
>  	if (!skdev) {
> @@ -3356,16 +3354,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		return -1;
>  	}
>  
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_enable_wake(pdev, PCI_D0, 0);
> -	pci_restore_state(pdev);
> +	device_wakeup_disable(dev);
>  
> -	rc = pci_enable_device(pdev);
> -	if (rc)
> -		return rc;
> -	rc = pci_request_regions(pdev, DRV_NAME);
> -	if (rc)
> -		goto err_out;
>  	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>  	if (rc)
>  		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> @@ -3374,7 +3364,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		goto err_out_regions;
>  	}
>  
> -	pci_set_master(pdev);
>  	rc = pci_enable_pcie_error_reporting(pdev);
>  	if (rc) {
>  		dev_err(&pdev->dev,
> @@ -3427,10 +3416,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
>  		pci_disable_pcie_error_reporting(pdev);
>  
>  err_out_regions:
> -	pci_release_regions(pdev);
> -
> -err_out:
> -	pci_disable_device(pdev);
>  	return rc;
>  }
>  
> @@ -3450,13 +3435,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
>  	skd_stop_device(skdev);
>  }
>  
> +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
> +
>  static struct pci_driver skd_driver = {
>  	.name		= DRV_NAME,
>  	.id_table	= skd_pci_tbl,
>  	.probe		= skd_pci_probe,
>  	.remove		= skd_pci_remove,
> -	.suspend	= skd_pci_suspend,
> -	.resume		= skd_pci_resume,
> +	.driver.pm	= &skd_pci_pm_ops,
>  	.shutdown	= skd_pci_shutdown,
>  };
>  
> 

Tested-by: Damien Le Moal <damien.lemoal@wdc.com>
Acked-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3 0/3] block: use generic power management
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
                                   ` (2 preceding siblings ...)
  2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 3/3] skd: " Vaibhav Gupta
@ 2020-08-17  7:55                 ` Vaibhav Gupta
  2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
  4 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2020-08-17  7:55 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel

On Wed, Jul 22, 2020 at 02:03:32PM +0530, Vaibhav Gupta wrote:
> Linux Kernel Mentee: Remove Legacy Power Management.
> 
> The purpose of this patch series is to upgrade power management in block
> drivers. This has been done by upgrading .suspend() and .resume() callbacks.
> 
> The upgrade makes sure that the involvement of PCI Core does not change the
> order of operations executed in a driver. Thus, does not change its behavior.
> 
> In general, drivers with legacy PM, .suspend() and .resume() make use of PCI
> helper functions like pci_enable/disable_device_mem(), pci_set_power_state(),
> pci_save/restore_state(), pci_enable/disable_device(), etc. to complete
> their job.
> 
> The conversion requires the removal of those function calls, change the
> callbacks' definition accordingly and make use of dev_pm_ops structure.
> 
> All patches are compile-tested only.
> 
> Test tools:
>     - Compiler: gcc (GCC) 10.1.0
>     - allmodconfig build: make -j$(nproc) W=1 all
> 
> v3: v2 needed some changes in commit messages.
> 
> Vaibhav Gupta (3):
>   mtip32xx: use generic power management
>   rsxx: use generic power management
>   skd: use generic power management
> 
>  drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
>  drivers/block/rsxx/core.c         |  9 ++++--
>  drivers/block/skd_main.c          | 30 +++++------------
>  3 files changed, 27 insertions(+), 66 deletions(-)
> 
> -- 
> 2.27.0
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v4 0/3] block: use generic power management
  2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
                                   ` (3 preceding siblings ...)
  2020-08-17  7:55                 ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
@ 2021-01-14 11:54                 ` Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 1/3] mtip32xx: " Vaibhav Gupta
                                     ` (2 more replies)
  4 siblings, 3 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2021-01-14 11:54 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel

Linux Kernel Mentee: Remove Legacy Power Management.

The purpose of this patch series is to upgrade power management in block
drivers. This has been done by upgrading .suspend() and .resume() callbacks.

The upgrade makes sure that the involvement of PCI Core does not change the
order of operations executed in a driver. Thus, does not change its behavior.

In general, drivers with legacy PM, .suspend() and .resume() make use of PCI
helper functions like pci_enable/disable_device_mem(), pci_set_power_state(),
pci_save/restore_state(), pci_enable/disable_device(), etc. to complete
their job.

The conversion requires the removal of those function calls, change the
callbacks' definition accordingly and make use of dev_pm_ops structure.

All patches are compile-tested only.

Test tools:
    - Compiler: gcc (GCC) 10.2.0
    - allmodconfig build: make -j$(nproc) W=1 all

v4: patch-set rebased.

Vaibhav Gupta (3):
  mtip32xx: use generic power management
  rsxx: use generic power management
  skd: use generic power management

 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 drivers/block/rsxx/core.c         |  9 ++++--
 drivers/block/skd_main.c          | 30 +++++------------
 3 files changed, 27 insertions(+), 66 deletions(-)

-- 
2.30.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v4 1/3] mtip32xx: use generic power management
  2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
@ 2021-01-14 11:54                   ` Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 2/3] rsxx: " Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2021-01-14 11:54 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel

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 <vaibhavgupta40@gmail.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 54 +++++++------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 53ac59d19ae5..de1ac3366b97 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -4232,14 +4232,13 @@ static void mtip_pci_remove(struct pci_dev *pdev)
  *	0  Success
  *	<0 Error
  */
-static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
+static int __maybe_unused mtip_pci_suspend(struct device *dev)
 {
 	int rv = 0;
-	struct driver_data *dd = pci_get_drvdata(pdev);
+	struct driver_data *dd = dev_get_drvdata(dev);
 
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
@@ -4247,21 +4246,8 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
 
 	/* Disable ports & interrupts then send standby immediate */
 	rv = mtip_block_suspend(dd);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to suspend controller\n");
-		return rv;
-	}
-
-	/*
-	 * Save the pci config space to pdev structure &
-	 * disable the device
-	 */
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-
-	/* Move to Low power state*/
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (rv < 0)
+		dev_err(dev, "Failed to suspend controller\n");
 
 	return rv;
 }
@@ -4273,42 +4259,25 @@ static int mtip_pci_suspend(struct pci_dev *pdev, pm_message_t mesg)
  *      0  Success
  *      <0 Error
  */
-static int mtip_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused mtip_pci_resume(struct device *dev)
 {
 	int rv = 0;
 	struct driver_data *dd;
 
-	dd = pci_get_drvdata(pdev);
+	dd = dev_get_drvdata(dev);
 	if (!dd) {
-		dev_err(&pdev->dev,
-			"Driver private datastructure is NULL\n");
+		dev_err(dev, "Driver private datastructure is NULL\n");
 		return -EFAULT;
 	}
 
-	/* Move the device to active State */
-	pci_set_power_state(pdev, PCI_D0);
-
-	/* Restore PCI configuration space */
-	pci_restore_state(pdev);
-
-	/* Enable the PCI device*/
-	rv = pcim_enable_device(pdev);
-	if (rv < 0) {
-		dev_err(&pdev->dev,
-			"Failed to enable card during resume\n");
-		goto err;
-	}
-	pci_set_master(pdev);
-
 	/*
 	 * Calls hbaReset, initPort, & startPort function
 	 * then enables interrupts
 	 */
 	rv = mtip_block_resume(dd);
 	if (rv < 0)
-		dev_err(&pdev->dev, "Unable to resume\n");
+		dev_err(dev, "Unable to resume\n");
 
-err:
 	clear_bit(MTIP_DDF_RESUME_BIT, &dd->dd_flag);
 
 	return rv;
@@ -4339,14 +4308,15 @@ static const struct pci_device_id mtip_pci_tbl[] = {
 	{ 0 }
 };
 
+static SIMPLE_DEV_PM_OPS(mtip_pci_pm_ops, mtip_pci_suspend, mtip_pci_resume);
+
 /* Structure that describes the PCI driver functions. */
 static struct pci_driver mtip_pci_driver = {
 	.name			= MTIP_DRV_NAME,
 	.id_table		= mtip_pci_tbl,
 	.probe			= mtip_pci_probe,
 	.remove			= mtip_pci_remove,
-	.suspend		= mtip_pci_suspend,
-	.resume			= mtip_pci_resume,
+	.driver.pm		= &mtip_pci_pm_ops,
 	.shutdown		= mtip_pci_shutdown,
 };
 
-- 
2.30.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v4 2/3] rsxx: use generic power management
  2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 1/3] mtip32xx: " Vaibhav Gupta
@ 2021-01-14 11:54                   ` Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 3/3] skd: " Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2021-01-14 11:54 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: linux-block, linux-kernel-mentees, linux-kernel

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 <vaibhavgupta40@gmail.com>
---
 drivers/block/rsxx/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 63f549889f87..937ad8a28e85 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -1032,12 +1032,15 @@ static void rsxx_pci_remove(struct pci_dev *dev)
 	kfree(card);
 }
 
-static int rsxx_pci_suspend(struct pci_dev *dev, pm_message_t state)
+static int __maybe_unused rsxx_pci_suspend(
+	__attribute__((unused)) struct device *dev)
 {
 	/* We don't support suspend at this time. */
 	return -ENOSYS;
 }
 
+#define rsxx_pci_resume NULL
+
 static void rsxx_pci_shutdown(struct pci_dev *dev)
 {
 	struct rsxx_cardinfo *card = pci_get_drvdata(dev);
@@ -1073,12 +1076,14 @@ static const struct pci_device_id rsxx_pci_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, rsxx_pci_ids);
 
+static SIMPLE_DEV_PM_OPS(rsxx_pci_pm_ops, rsxx_pci_suspend, rsxx_pci_resume);
+
 static struct pci_driver rsxx_pci_driver = {
 	.name		= DRIVER_NAME,
 	.id_table	= rsxx_pci_ids,
 	.probe		= rsxx_pci_probe,
 	.remove		= rsxx_pci_remove,
-	.suspend	= rsxx_pci_suspend,
+	.driver.pm	= &rsxx_pci_pm_ops,
 	.shutdown	= rsxx_pci_shutdown,
 	.err_handler    = &rsxx_err_handler,
 };
-- 
2.30.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Linux-kernel-mentees] [PATCH v4 3/3] skd: use generic power management
  2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 1/3] mtip32xx: " Vaibhav Gupta
  2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 2/3] rsxx: " Vaibhav Gupta
@ 2021-01-14 11:54                   ` Vaibhav Gupta
  2 siblings, 0 replies; 28+ messages in thread
From: Vaibhav Gupta @ 2021-01-14 11:54 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas, Bjorn Helgaas, Vaibhav Gupta,
	Jens Axboe, Joshua Morris, Philip Kelleher, Damien Le Moal
  Cc: Damien Le Moal, linux-kernel, linux-block, linux-kernel-mentees

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 <vaibhavgupta40@gmail.com>
Tested-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/block/skd_main.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c
index a962b4551bed..8194b58525e2 100644
--- a/drivers/block/skd_main.c
+++ b/drivers/block/skd_main.c
@@ -3317,10 +3317,11 @@ static void skd_pci_remove(struct pci_dev *pdev)
 	return;
 }
 
-static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused skd_pci_suspend(struct device *dev)
 {
 	int i;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3339,18 +3340,15 @@ static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 	if (skdev->pcie_error_reporting_is_enabled)
 		pci_disable_pcie_error_reporting(pdev);
 
-	pci_release_regions(pdev);
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	return 0;
 }
 
-static int skd_pci_resume(struct pci_dev *pdev)
+static int __maybe_unused skd_pci_resume(struct device *dev)
 {
 	int i;
 	int rc = 0;
 	struct skd_device *skdev;
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	skdev = pci_get_drvdata(pdev);
 	if (!skdev) {
@@ -3358,16 +3356,8 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		return -1;
 	}
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_enable_wake(pdev, PCI_D0, 0);
-	pci_restore_state(pdev);
+	device_wakeup_disable(dev);
 
-	rc = pci_enable_device(pdev);
-	if (rc)
-		return rc;
-	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
-		goto err_out;
 	rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
@@ -3376,7 +3366,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		goto err_out_regions;
 	}
 
-	pci_set_master(pdev);
 	rc = pci_enable_pcie_error_reporting(pdev);
 	if (rc) {
 		dev_err(&pdev->dev,
@@ -3429,10 +3418,6 @@ static int skd_pci_resume(struct pci_dev *pdev)
 		pci_disable_pcie_error_reporting(pdev);
 
 err_out_regions:
-	pci_release_regions(pdev);
-
-err_out:
-	pci_disable_device(pdev);
 	return rc;
 }
 
@@ -3452,13 +3437,14 @@ static void skd_pci_shutdown(struct pci_dev *pdev)
 	skd_stop_device(skdev);
 }
 
+static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resume);
+
 static struct pci_driver skd_driver = {
 	.name		= DRV_NAME,
 	.id_table	= skd_pci_tbl,
 	.probe		= skd_pci_probe,
 	.remove		= skd_pci_remove,
-	.suspend	= skd_pci_suspend,
-	.resume		= skd_pci_resume,
+	.driver.pm	= &skd_pci_pm_ops,
 	.shutdown	= skd_pci_shutdown,
 };
 
-- 
2.30.0

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2021-01-14 11:57 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17  8:09 [Linux-kernel-mentees] [PATCH v1 0/3] block: use generic power management Vaibhav Gupta
2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 1/3] mtip32xx: " Vaibhav Gupta
2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 2/3] rsxx: " Vaibhav Gupta
2020-07-17  8:09 ` [Linux-kernel-mentees] [PATCH v1 3/3] skd: " Vaibhav Gupta
2020-07-20 12:52   ` Damien Le Moal
2020-07-20 13:16     ` Vaibhav Gupta
2020-07-21  2:49       ` Damien Le Moal
2020-07-21  7:04         ` Vaibhav Gupta
2020-07-20 13:29     ` [Linux-kernel-mentees] [PATCH v2 0/3] block: " Vaibhav Gupta
2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 1/3] mtip32xx: " Vaibhav Gupta
2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 2/3] rsxx: " Vaibhav Gupta
2020-07-20 13:30       ` [Linux-kernel-mentees] [PATCH v2 3/3] skd: " Vaibhav Gupta
2020-07-21  2:57         ` Damien Le Moal
2020-07-21  7:09           ` Vaibhav Gupta
2020-07-22  6:28             ` Damien Le Moal
2020-07-22  7:21               ` Vaibhav Gupta
2020-07-22  8:33               ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 1/3] mtip32xx: " Vaibhav Gupta
2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 2/3] rsxx: " Vaibhav Gupta
2020-07-22  8:33                 ` [Linux-kernel-mentees] [PATCH v3 3/3] skd: " Vaibhav Gupta
2020-07-27  2:14                   ` Damien Le Moal
2020-08-17  7:55                 ` [Linux-kernel-mentees] [PATCH v3 0/3] block: " Vaibhav Gupta
2021-01-14 11:54                 ` [Linux-kernel-mentees] [PATCH v4 " Vaibhav Gupta
2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 1/3] mtip32xx: " Vaibhav Gupta
2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 2/3] rsxx: " Vaibhav Gupta
2021-01-14 11:54                   ` [Linux-kernel-mentees] [PATCH v4 3/3] skd: " Vaibhav Gupta
2020-07-22 17:52             ` [Linux-kernel-mentees] [PATCH v2 " Bjorn Helgaas
2020-07-22 18:07               ` Vaibhav Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).