Re: [PATCH 3/7] megaraid_sas: Do not fire DCMDs during PCI shutdown/detach

[Hannes Reinecke <hare@suse.de>]

On 10/17/2016 12:24 PM, Sumit Saxena wrote:
> This patch addresses the issue of driver firing DCMDs in PCI
> shutdown/detach path irrespective of firmware state.
> Driver will check for whether firmware is operational state or not
> before firing DCMDs. If firmware is in unrecoverbale
> state or does not become operational within specfied time, driver will
> skip firing DCMDs.
> 
> Signed-off-by: Sumit Saxena <sumit.saxena@broadcom.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_base.c   | 46 +++++++++++++++++++++++++++++
>  drivers/scsi/megaraid/megaraid_sas_fusion.c |  9 ++++--
>  2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 9ff57de..092387f 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -6248,6 +6248,32 @@ fail_reenable_msix:
>  #define megasas_resume	NULL
>  #endif
>  
> +static inline int
> +megasas_wait_for_adapter_operational(struct megasas_instance *instance)
> +{
> +	int wait_time = MEGASAS_RESET_WAIT_TIME * 2;
> +	int i;
> +
> +	for (i = 0; i < wait_time; i++) {
> +		if (atomic_read(&instance->adprecovery)
> +			== MEGASAS_HBA_OPERATIONAL)
> +			break;
> +
> +		if (!(i % MEGASAS_RESET_NOTICE_INTERVAL))
> +			dev_notice(&instance->pdev->dev, "waiting for controller reset to finish\n");
> +
> +		msleep(1000);
> +	}
> +
> +	if (atomic_read(&instance->adprecovery) != MEGASAS_HBA_OPERATIONAL) {
> +		dev_info(&instance->pdev->dev, "%s timed out while waiting for HBA to recover.\n",
> +			__func__);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * megasas_detach_one -	PCI hot"un"plug entry point
>   * @pdev:		PCI device structure
> @@ -6272,9 +6298,18 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  	if (instance->fw_crash_state != UNAVAILABLE)
>  		megasas_free_host_crash_buffer(instance);
>  	scsi_remove_host(instance->host);
> +
> +	msleep(1000);
> +	if ((atomic_read(&instance->adprecovery) == MEGASAS_HW_CRITICAL_ERROR)
> +		|| ((atomic_read(&instance->adprecovery)
> +			!= MEGASAS_HBA_OPERATIONAL) &&
> +		megasas_wait_for_adapter_operational(instance)))
> +		goto skip_firing_dcmds;
> +
>  	megasas_flush_cache(instance);
>  	megasas_shutdown_controller(instance, MR_DCMD_CTRL_SHUTDOWN);
>  
> +skip_firing_dcmds:
>  	/* cancel the delayed work if this work still in queue*/
>  	if (instance->ev != NULL) {
>  		struct megasas_aen_event *ev = instance->ev;
Why not move the 'msleep' and 'atomic_read' into the call to
megasas_wait_for_adapter_operational?
Plus I'm not sure if the unconditional 'msleep' is a good idea here;
maybe one should read 'adprecovery' first and _then_ call msleep if
required?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)