Re: [PATCH 1/9] scsi: Use Scsi_Host as argument for eh_host_reset_handler

[Bart Van Assche <bvanassche@xxxxxxx>]

On 10/16/23 05:15, Hannes Reinecke wrote:
> diff --git a/Documentation/scsi/scsi_mid_low_api.rst b/Documentation/scsi/scsi_mid_low_api.rst
> index 022198c51350..85b1384ba4fd 100644
> --- a/Documentation/scsi/scsi_mid_low_api.rst
> +++ b/Documentation/scsi/scsi_mid_low_api.rst
> @@ -777,7 +777,7 @@ Details::
>   
>       /**
>       *      eh_host_reset_handler - reset host (host bus adapter)
> -    *      @scp: SCSI host that contains this device should be reset
> +    *      @shp: SCSI host that contains this device should be reset

Please change this into "@shp: SCSI host that should be reset".

>   	/*  If we can't locate the host to reset, then we failed. */
> -	if ((hd = shost_priv(SCpnt->device->host)) == NULL){
> -		printk(KERN_ERR MYNAM ": host reset: "
> -		    "Can't locate host! (sc=%p)\n", SCpnt);
> +	if ((hd = shost_priv(sh)) == NULL){
> +		printk(KERN_ERR MYNAM ": host reset: Can't locate host!\n");
>   		return FAILED;
>   	}

Please move the assignment out of the if-statement since the 
if-statement has to be modified anyway.

> -	printk(MYIOC_s_INFO_FMT "host reset: %s (sc=%p)\n",
> -	    ioc->name, ((retval == 0) ? "SUCCESS" : "FAILED" ), SCpnt);
> +	printk(MYIOC_s_INFO_FMT "host reset: %s\n",
> +	    ioc->name, ((retval == 0) ? "SUCCESS" : "FAILED" ));

Please consider removing the superfluous parentheses from the modified code.

>   	hostdata->eh_complete = NULL;
> -	/* Revalidate the transport parameters of the failing device */
> -	if(hostdata->fast)
> -		spi_schedule_dv_device(SCp->device);
> -
> -	spin_unlock_irq(SCp->device->host->host_lock);
> +	/* Revalidate the transport parameters for attached devices */
> +	if(hostdata->fast) {
> +		struct scsi_device *sdev;
> +		__shost_for_each_device(sdev, host)
> +			spi_schedule_dv_device(sdev);
> +	}
> +	spin_unlock_irq(host->host_lock);
>   	return SUCCESS;

Since the above changes behavior of the SPI transport, shouldn't this be 
mentioned in the patch description?

> diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
> index 72ceaf650b0d..9be45b7a2571 100644
> --- a/drivers/scsi/BusLogic.c
> +++ b/drivers/scsi/BusLogic.c
> @@ -2852,21 +2852,14 @@ static bool blogic_write_outbox(struct blogic_adapter *adapter,
>   
>   /* Error Handling (EH) support */
>   
> -static int blogic_hostreset(struct scsi_cmnd *SCpnt)
> +static int blogic_hostreset(struct Scsi_Host *shost)
>   {
> -	struct blogic_adapter *adapter =
> -		(struct blogic_adapter *) SCpnt->device->host->hostdata;
> -
> -	unsigned int id = SCpnt->device->id;
> -	struct blogic_tgt_stats *stats = &adapter->tgt_stats[id];
> +	struct blogic_adapter *adapter = shost_priv(shost);
>   	int rc;
>   
> -	spin_lock_irq(SCpnt->device->host->host_lock);
> -
> -	blogic_inc_count(&stats->adapter_reset_req);
> -
> +	spin_lock_irq(shost->host_lock);
>   	rc = blogic_resetadapter(adapter, false);
> -	spin_unlock_irq(SCpnt->device->host->host_lock);
> +	spin_unlock_irq(shost->host_lock);
>   	return rc;
>   }

Why has the blogic_inc_count() call been left out?

Thanks,

Bart.