Re: [PATCH v2 2/3] ata: libata-eh: Remove ata_do_eh()

[Niklas Cassel <cassel@kernel.org>]

On Tue, Jul 08, 2025 at 04:36:47PM +0900, Damien Le Moal wrote:
> The only reason for ata_do_eh() to exist is that the two caller sites,
> ata_std_error_handler() and ata_sff_error_handler() may pass to it a

s/may pass to it a/may pass it a/


> NULL hardreset operation so that the built-in (generic) hardreset
> operation for a driver is ignored if the adapter SCR access is not
> available.
> 
> However, ata_std_error_handler() and ata_sff_error_handler()
> modifications of the hardreset port operation can easily be combined as
> they are mutually exclusive. That is, a driver using

sata_std_hardreset() can be on previous line without exceeding 75 columns.


> sata_std_hardreset() as its hardreset operation cannot use
> sata_sff_hardreset() and vice-versa.
> 
> With this observation, ata_do_eh() can be removed and its code moved to
> ata_std_error_handler(). The condition used to ignore the builtin

s/builtin/built-in/


> hardreset port operations is modified to be the one that was used in

s/operations/operations/

(you are ignoring the built-in hardreset operation, which is either
sata_std_hardreset() or sata_sff_hardreset(), but still the function
pointer will point to a single operation, so I think singular is more
correct here.)


> ata_sff_error_handler(). This requires defining a stub for the function
> sata_sff_hardreset() to avoid compilation errors when CONFIG_ATA_SFF is
> not enabled.
> 
> This change simplifies ata_sff_error_handler() as this function now only
> needs to call ata_std_error_handler().
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c  | 46 ++++++++++++----------------------------
>  drivers/ata/libata-sff.c | 10 +--------
>  include/linux/libata.h   | 11 +++++++---
>  3 files changed, 22 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 436536112043..68581adc6f87 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4067,59 +4067,39 @@ void ata_eh_finish(struct ata_port *ap)
>  }
>  
>  /**
> - *	ata_do_eh - do standard error handling
> + *	ata_std_error_handler - standard error handler
>   *	@ap: host port to handle error for
>   *
> - *	@prereset: prereset method (can be NULL)
> - *	@softreset: softreset method (can be NULL)
> - *	@hardreset: hardreset method (can be NULL)
> - *	@postreset: postreset method (can be NULL)
> - *
>   *	Perform standard error handling sequence.
>   *
>   *	LOCKING:
>   *	Kernel thread context (may sleep).
>   */
> -void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
> -	       ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
> -	       ata_postreset_fn_t postreset)
> +void ata_std_error_handler(struct ata_port *ap)
>  {
> -	struct ata_device *dev;
> +	struct ata_port_operations *ops = ap->ops;
> +	ata_reset_fn_t hardreset = ops->hardreset;
>  	int rc;
>  
> +	/* Ignore built-in hardresets if SCR access is not available */
> +	if ((hardreset == sata_std_hardreset ||
> +	     hardreset == sata_sff_hardreset) && !sata_scr_valid(&ap->link))
> +		hardreset = NULL;

I think it would be nicer to just do:

if ((hardreset == sata_std_hardreset ||
     hardreset == sata_sff_hardreset) && !sata_scr_valid(&ap->link))
     link->flags |= ATA_LFLAG_NO_HRST;

since ata_eh_reset() will already do
hardreset = NULL; if that flag is set.

This way, we can also simplify the function signature of ata_eh_recover() to:

int ata_eh_recover(struct ata_port *ap, bool use_pmp_ops,
                   struct ata_link **r_failed_link)

and ata_eh_reset() to:

int ata_eh_reset(struct ata_link *link, int classify, bool use_pmp_ops)


And then in:
int ata_eh_reset(struct ata_link *link, int classify, bool use_pmp_ops)
{
	...
	if (use_pmp_ops) {
		prereset = ops->prereset;
		softreset = ops->softreset;
		hardreset = ops->hardreset;
		postreset = ops->postreset;
	} else {
		prereset = ops->pmp_prereset;
		softreset = ops->pmp_softreset;
		hardreset = ops->pmp_hardreset;
		postreset = ops->pmp_postreset;
	}

	if (link->flags & ATA_LFLAG_NO_HRST)
		hardreset = NULL;

	...
}


(The if (link->flags & ATA_LFLAG_NO_HRST) statement is already there in
ata_eh_reset().)


> +
>  	ata_eh_autopsy(ap);
>  	ata_eh_report(ap);
>  
> -	rc = ata_eh_recover(ap, prereset, softreset, hardreset, postreset,
> -			    NULL);
> +	rc = ata_eh_recover(ap, ops->prereset, ops->softreset,
> +			    hardreset, ops->postreset, NULL);
>  	if (rc) {
> +		struct ata_device *dev;
> +
>  		ata_for_each_dev(dev, &ap->link, ALL)
>  			ata_dev_disable(dev);
>  	}
>  
>  	ata_eh_finish(ap);
>  }
> -
> -/**
> - *	ata_std_error_handler - standard error handler
> - *	@ap: host port to handle error for
> - *
> - *	Standard error handler
> - *
> - *	LOCKING:
> - *	Kernel thread context (may sleep).
> - */
> -void ata_std_error_handler(struct ata_port *ap)
> -{
> -	struct ata_port_operations *ops = ap->ops;
> -	ata_reset_fn_t hardreset = ops->hardreset;
> -
> -	/* ignore built-in hardreset if SCR access is not available */
> -	if (hardreset == sata_std_hardreset && !sata_scr_valid(&ap->link))
> -		hardreset = NULL;
> -
> -	ata_do_eh(ap, ops->prereset, ops->softreset, hardreset, ops->postreset);
> -}
>  EXPORT_SYMBOL_GPL(ata_std_error_handler);
>  
>  #ifdef CONFIG_PM
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 5a46c066abc3..e61f00779e40 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2054,8 +2054,6 @@ EXPORT_SYMBOL_GPL(ata_sff_drain_fifo);
>   */
>  void ata_sff_error_handler(struct ata_port *ap)
>  {
> -	ata_reset_fn_t softreset = ap->ops->softreset;
> -	ata_reset_fn_t hardreset = ap->ops->hardreset;
>  	struct ata_queued_cmd *qc;
>  	unsigned long flags;
>  
> @@ -2077,13 +2075,7 @@ void ata_sff_error_handler(struct ata_port *ap)
>  
>  	spin_unlock_irqrestore(ap->lock, flags);
>  
> -	/* ignore built-in hardresets if SCR access is not available */
> -	if ((hardreset == sata_std_hardreset ||
> -	     hardreset == sata_sff_hardreset) && !sata_scr_valid(&ap->link))
> -		hardreset = NULL;
> -
> -	ata_do_eh(ap, ap->ops->prereset, softreset, hardreset,
> -		  ap->ops->postreset);
> +	ata_std_error_handler(ap);
>  }
>  EXPORT_SYMBOL_GPL(ata_sff_error_handler);
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index d092747be588..0bfdec20496f 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1412,9 +1412,6 @@ extern void ata_eh_thaw_port(struct ata_port *ap);
>  extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
>  extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
>  
> -extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
> -		      ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
> -		      ata_postreset_fn_t postreset);
>  extern void ata_std_error_handler(struct ata_port *ap);
>  extern void ata_std_sched_eh(struct ata_port *ap);
>  extern void ata_std_end_eh(struct ata_port *ap);
> @@ -2152,6 +2149,14 @@ static inline u8 ata_wait_idle(struct ata_port *ap)
>  
>  	return status;
>  }
> +#else /* CONFIG_ATA_SFF */
> +

Looking at this file, we usually don't have newlines after #else,
so I would drop this newline.


> +static inline int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
> +				     unsigned long deadline)
> +{
> +	return -EOPNOTSUPP;
> +}
> +

Looking at this file, we usually don't have newlines before #endif,
so I would drop this newline.


>  #endif /* CONFIG_ATA_SFF */
>  
>  #endif /* __LINUX_LIBATA_H__ */
> -- 
> 2.50.0
>