Re: [PATCH 1/6] ata: remove reference to non-existing error_handler()

[Damien Le Moal <dlemoal@kernel.org>]

On 5/11/23 07:52, Hannes Reinecke wrote:
> With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all
> libata drivers now have the error_handler() callback provided,
> so we can stop checking for non-existing error_handler callback.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/libata-core.c | 158 ++++++++++++++------------------------
>  drivers/ata/libata-eh.c   | 148 +++++++++++++++--------------------
>  drivers/ata/libata-sata.c |   5 +-
>  drivers/ata/libata-scsi.c |  21 ++---
>  drivers/ata/libata-sff.c  |  32 +++-----
>  5 files changed, 139 insertions(+), 225 deletions(-)
> 

[...]

> -		case ATA_CMD_SLEEP:
> -			dev->flags |= ATA_DFLAG_SLEEPING;
> +	trace_ata_qc_complete_done(qc);
> +	/* Some commands need post-processing after successful
> +	 * completion.
> +	 */

While at it, can you fix this comment style ?
It may actually fit on one line...

[...]

> +	/* Exception might have happened after ->error_handler
> +	 * recovered the port but before this point.  Repeat
> +	 * EH in such case.
> +	 */

Same here. Multi-line comment format...

[...]

> -		/* end eh (clear host_eh_scheduled) while holding
> -		 * ap->lock such that if exception occurs after this
> -		 * point but before EH completion, SCSI midlayer will
> -		 * re-initiate EH.
> -		 */
> -		ap->ops->end_eh(ap);
> +	/* end eh (clear host_eh_scheduled) while holding
> +	 * ap->lock such that if exception occurs after this
> +	 * point but before EH completion, SCSI midlayer will
> +	 * re-initiate EH.
> +	 */

And here.

[...]

> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index f3e7396e3191..bd2a754b645c 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1138,11 +1138,8 @@ EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
>  int ata_sas_port_start(struct ata_port *ap)
>  {
>  	/*
> -	 * the port is marked as frozen at allocation time, but if we don't
> -	 * have new eh, we won't thaw it
> +	 * the port is marked as frozen at allocation time
>  	 */

This one can be a single line comment, no ?

[...]

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 9d28badfe41d..aa286d48e847 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -883,31 +883,23 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
>  {
>  	struct ata_port *ap = qc->ap;
>  
> -	if (ap->ops->error_handler) {
> -		if (in_wq) {
> -			/* EH might have kicked in while host lock is
> -			 * released.
> -			 */
> -			qc = ata_qc_from_tag(ap, qc->tag);
> -			if (qc) {
> -				if (likely(!(qc->err_mask & AC_ERR_HSM))) {
> -					ata_sff_irq_on(ap);
> -					ata_qc_complete(qc);
> -				} else
> -					ata_port_freeze(ap);
> -			}
> -		} else {
> -			if (likely(!(qc->err_mask & AC_ERR_HSM)))
> +	if (in_wq) {
> +		/* EH might have kicked in while host lock is
> +		 * released.
> +		 */

Comment format again.

Also, please add linux-scsi and John to the distribution list when resending.

-- 
Damien Le Moal
Western Digital Research