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@xxxxxxxxxx>
> ---
> 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
>
