Re: [patch added to the 3.12 stable tree] hpsa: refine the pci enable/disable handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/06/2015 05:34 PM, Jiri Slaby wrote:
> From: Tomas Henzl <thenzl@xxxxxxxxxx>
>
> This patch has been added to the 3.12 stable tree. If you have any
> objections, please let us know.

Well yes I have objections, you have probably missed my mail sent just a while ago
let me just copy from that mail -

Please do not apply this patch isolated from his friend, the
859c75aba20264d87dd026bab0d0ca3bff385955 hpsa: add missing pci_set_master in kdump path
needs to be applied together with the 132aa220b45d60e9b20def1e9d8be9422eed9616 .

In addition to that, after the original issue goes away you may notice sometimes
an unhandled irq 16 message, to fix this a patch is posted 
here http://www.spinics.net/lists/linux-scsi/msg80316.html
This patch still awaits a maintainers review though.

Probably the best idea is to wait until the issue is solved completely.

Tomas 

>
> ===============
>
> commit 132aa220b45d60e9b20def1e9d8be9422eed9616 upstream.
>
> 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.
>
> Signed-off-by: Tomas Henzl <thenzl@xxxxxxxxxx>
> Reviewed-by: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
> ---
>  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 d535e7504ea0..3639f05fafd0 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3920,10 +3920,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 */
> @@ -3971,11 +3967,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)
> @@ -4470,6 +4461,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);
>  
> @@ -4478,10 +4486,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");
> @@ -4492,7 +4501,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)
> @@ -4635,6 +4648,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);
>  }

--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]