[PATCH V2] hpsa: refine the pci enable/disable handling

[PATCH V2] hpsa: refine the pci enable/disable handling

[Tomas Henzl]

When a second(kdump) kernel starts and the hard reset method is used
the driver calls pci_disable_device without previously enabling it,
so the kernel shows a warning -
[   16.876248] WARNING: at drivers/pci/pci.c:1431 pci_disable_device+0x84/0x90()
[   16.882686] Device hpsa
disabling already-disabled device
...
This patch fixes it, in addition to this I tried to balance also some other pairs
of enable/disable device in the driver.
Unfortunately I wasn't able to verify the functionality for the case of a sw reset,
because of a lack of proper hw.

Changes in v2:
- fixed a checkpatch issue
- removed a no more valid comment
- added pci_enable/disable pair when kdump kernel starts
- fixed a typo in subject line

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/hpsa.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 6b35d0d..cf793c3 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5971,10 +5971,6 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)
 
 	/* Save the PCI command register */
 	pci_read_config_word(pdev, 4, &command_register);
-	/* Turn the board off.  This is so that later pci_restore_state()
-	 * won't turn the board on before the rest of config space is ready.
-	 */
-	pci_disable_device(pdev);
 	pci_save_state(pdev);
 
 	/* find the first memory BAR, so we can find the cfg table */
@@ -6022,11 +6018,6 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)
 		goto unmap_cfgtable;
 
 	pci_restore_state(pdev);
-	rc = pci_enable_device(pdev);
-	if (rc) {
-		dev_warn(&pdev->dev, "failed to enable device.\n");
-		goto unmap_cfgtable;
-	}
 	pci_write_config_word(pdev, 4, command_register);
 
 	/* Some devices (notably the HP Smart Array 5i Controller)
@@ -6541,6 +6532,23 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
 	if (!reset_devices)
 		return 0;
 
+	/* kdump kernel is loading, we don't know in which state is
+	 * the pci interface. The dev->enable_cnt is equal zero
+	 * so we call enable+disable, wait a while and switch it on.
+	 */
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_warn(&pdev->dev, "Failed to enable PCI device\n");
+		return -ENODEV;
+	}
+	pci_disable_device(pdev);
+	msleep(260);			/* a randomly chosen number */
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_warn(&pdev->dev, "failed to enable device.\n");
+		return -ENODEV;
+	}
+
 	/* Reset the controller with a PCI power-cycle or via doorbell */
 	rc = hpsa_kdump_hard_reset_controller(pdev);
 
@@ -6549,10 +6557,11 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
 	 * "performant mode".  Or, it might be 640x, which can't reset
 	 * due to concerns about shared bbwc between 6402/6404 pair.
 	 */
-	if (rc == -ENOTSUPP)
-		return rc; /* just try to do the kdump anyhow. */
-	if (rc)
-		return -ENODEV;
+	if (rc) {
+		if (rc != -ENOTSUPP) /* just try to do the kdump anyhow. */
+			rc = -ENODEV;
+		goto out_disable;
+	}
 
 	/* Now try to get the controller to respond to a no-op */
 	dev_warn(&pdev->dev, "Waiting for controller to respond to no-op\n");
@@ -6563,7 +6572,11 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
 			dev_warn(&pdev->dev, "no-op failed%s\n",
 					(i < 11 ? "; re-trying" : ""));
 	}
-	return 0;
+
+out_disable:
+
+	pci_disable_device(pdev);
+	return rc;
 }
 
 static int hpsa_allocate_cmd_pool(struct ctlr_info *h)
@@ -6743,6 +6756,7 @@ static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
 		iounmap(h->transtable);
 	if (h->cfgtable)
 		iounmap(h->cfgtable);
+	pci_disable_device(h->pdev);
 	pci_release_regions(h->pdev);
 	kfree(h);
 }
-- 
1.8.3.1

Re: [PATCH V2] hpsa: refine the pci enable/disable handling

[scameron]

On Thu, Aug 14, 2014 at 04:12:39PM +0200, Tomas Henzl wrote:
> When a second(kdump) kernel starts and the hard reset method is used
> the driver calls pci_disable_device without previously enabling it,
> so the kernel shows a warning -
> [   16.876248] WARNING: at drivers/pci/pci.c:1431 pci_disable_device+0x84/0x90()
> [   16.882686] Device hpsa
> disabling already-disabled device
> ...
> This patch fixes it, in addition to this I tried to balance also some other pairs
> of enable/disable device in the driver.
> Unfortunately I wasn't able to verify the functionality for the case of a sw reset,
> because of a lack of proper hw.
> 
> Changes in v2:
> - fixed a checkpatch issue
> - removed a no more valid comment
> - added pci_enable/disable pair when kdump kernel starts
> - fixed a typo in subject line
> 
> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> ---
>  drivers/scsi/hpsa.c | 42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 6b35d0d..cf793c3 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -5971,10 +5971,6 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)
>  
>  	/* Save the PCI command register */
>  	pci_read_config_word(pdev, 4, &command_register);
> -	/* Turn the board off.  This is so that later pci_restore_state()
> -	 * won't turn the board on before the rest of config space is ready.
> -	 */
> -	pci_disable_device(pdev);
>  	pci_save_state(pdev);
>  
>  	/* find the first memory BAR, so we can find the cfg table */
> @@ -6022,11 +6018,6 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)
>  		goto unmap_cfgtable;
>  
>  	pci_restore_state(pdev);
> -	rc = pci_enable_device(pdev);
> -	if (rc) {
> -		dev_warn(&pdev->dev, "failed to enable device.\n");
> -		goto unmap_cfgtable;
> -	}
>  	pci_write_config_word(pdev, 4, command_register);
>  
>  	/* Some devices (notably the HP Smart Array 5i Controller)
> @@ -6541,6 +6532,23 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
>  	if (!reset_devices)
>  		return 0;
>  
> +	/* kdump kernel is loading, we don't know in which state is
> +	 * the pci interface. The dev->enable_cnt is equal zero
> +	 * so we call enable+disable, wait a while and switch it on.
> +	 */
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "Failed to enable PCI device\n");
> +		return -ENODEV;
> +	}
> +	pci_disable_device(pdev);
> +	msleep(260);			/* a randomly chosen number */
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "failed to enable device.\n");
> +		return -ENODEV;
> +	}
> +
>  	/* Reset the controller with a PCI power-cycle or via doorbell */
>  	rc = hpsa_kdump_hard_reset_controller(pdev);
>  
> @@ -6549,10 +6557,11 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
>  	 * "performant mode".  Or, it might be 640x, which can't reset
>  	 * due to concerns about shared bbwc between 6402/6404 pair.
>  	 */
> -	if (rc == -ENOTSUPP)
> -		return rc; /* just try to do the kdump anyhow. */
> -	if (rc)
> -		return -ENODEV;
> +	if (rc) {
> +		if (rc != -ENOTSUPP) /* just try to do the kdump anyhow. */
> +			rc = -ENODEV;
> +		goto out_disable;
> +	}
>  
>  	/* Now try to get the controller to respond to a no-op */
>  	dev_warn(&pdev->dev, "Waiting for controller to respond to no-op\n");
> @@ -6563,7 +6572,11 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
>  			dev_warn(&pdev->dev, "no-op failed%s\n",
>  					(i < 11 ? "; re-trying" : ""));
>  	}
> -	return 0;
> +
> +out_disable:
> +
> +	pci_disable_device(pdev);
> +	return rc;
>  }
>  
>  static int hpsa_allocate_cmd_pool(struct ctlr_info *h)
> @@ -6743,6 +6756,7 @@ static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
>  		iounmap(h->transtable);
>  	if (h->cfgtable)
>  		iounmap(h->cfgtable);
> +	pci_disable_device(h->pdev);
>  	pci_release_regions(h->pdev);
>  	kfree(h);
>  }
> -- 
> 1.8.3.1

Ack.

Reviewed-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>

-- steve

Re: [PATCH V2] hpsa: refine the pci enable/disable handling

[Christoph Hellwig]

Thanks,

applied to the drivers-for-3.18 branch.

RE: [PATCH V2] hpsa: refine the pci enable/disable handling

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Tomas Henzl
...

> +	/* kdump kernel is loading, we don't know in which state is
> +	 * the pci interface. The dev->enable_cnt is equal zero
> +	 * so we call enable+disable, wait a while and switch it on.
> +	 */
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "Failed to enable PCI device\n");
> +		return -ENODEV;
> +	}
> +	pci_disable_device(pdev);
> +	msleep(260);			/* a randomly chosen number */
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "failed to enable device.\n");
> +		return -ENODEV;
> +	}
> +
>  	/* Reset the controller with a PCI power-cycle or via doorbell
> */

I tested this patch by adding:
	reset_devices
to the kernel command line, which sets a kernel global variable that
triggers the driver to take this path.

Controller initialization failed with:
[   21.822789] hpsa 0000:02:00.0: Waiting for controller to respond to no-op
[  121.822219] hpsa 0000:02:00.0: controller message 03:00 timed out
[  121.854814] hpsa 0000:02:00.0: no-op failed; re-trying
...

The reason is that pci_disable_device clears the Bus Master Enable bit,
and there is no call to pci_set_master after pci_enable.  The controller is
unable to return the response for the command sent by hpsa_noop down
below.  Adding pci_set_master(pdev) got it working.

A call to pci_request_regions is also missing (that's supposed to
be called before accessing the controller's memory space), but
that's not fatal here.

---
Rob Elliott    HP Server Storage

Re: [PATCH V2] hpsa: refine the pci enable/disable handling

[Tomas Henzl]

On 09/07/2014 01:38 AM, Elliott, Robert (Server Storage) wrote:
>
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Tomas Henzl
> ...
>
>> +	/* kdump kernel is loading, we don't know in which state is
>> +	 * the pci interface. The dev->enable_cnt is equal zero
>> +	 * so we call enable+disable, wait a while and switch it on.
>> +	 */
>> +	rc = pci_enable_device(pdev);
>> +	if (rc) {
>> +		dev_warn(&pdev->dev, "Failed to enable PCI device\n");
>> +		return -ENODEV;
>> +	}
>> +	pci_disable_device(pdev);
>> +	msleep(260);			/* a randomly chosen number */
>> +	rc = pci_enable_device(pdev);
>> +	if (rc) {
>> +		dev_warn(&pdev->dev, "failed to enable device.\n");
>> +		return -ENODEV;
>> +	}
>> +
>>  	/* Reset the controller with a PCI power-cycle or via doorbell
>> */
> I tested this patch by adding:
> 	reset_devices
> to the kernel command line, which sets a kernel global variable that
> triggers the driver to take this path.
>
> Controller initialization failed with:
> [   21.822789] hpsa 0000:02:00.0: Waiting for controller to respond to no-op
> [  121.822219] hpsa 0000:02:00.0: controller message 03:00 timed out
> [  121.854814] hpsa 0000:02:00.0: no-op failed; re-trying
> ...
>
> The reason is that pci_disable_device clears the Bus Master Enable bit,
> and there is no call to pci_set_master after pci_enable.  The controller is
> unable to return the response for the command sent by hpsa_noop down
> below.  Adding pci_set_master(pdev) got it working.

And I was sure I've tested it, but apparently I haven't after adding the last part.
Thanks for testing.

>
> A call to pci_request_regions is also missing (that's supposed to
> be called before accessing the controller's memory space), but
> that's not fatal here.

This patch doesn't change this, pci_request_regions wasn't also called without
it before. Likely there probably are some weak points during the pci initialisation
and combined with the "OS BUG" there is room for changes.
I wanted just to get rid of the "disabling already-disabled device" warning
and change it only as least as possible so it doesn't stop working (but have failed though).


The patch below fixes the problem for me.

Christoph, what is the best approach - should  I post this to the list as new patch
or would you prefer to drop the old version and repost a fixed patch?
Maybe HP could add the patch to their upcoming series in that case.

Tomas

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 7828834..cef5d49 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6544,7 +6544,7 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev)
 		dev_warn(&pdev->dev, "failed to enable device.\n");
 		return -ENODEV;
 	}
-
+	pci_set_master(pdev);
 	/* Reset the controller with a PCI power-cycle or via doorbell */
 	rc = hpsa_kdump_hard_reset_controller(pdev);
 

>
> ---
> Rob Elliott    HP Server Storage
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html