[PATCH v2 0/3] libata-eh cleanups

[PATCH v2 0/3] libata-eh cleanups

[Damien Le Moal]

3 patches to cleanup libata-eh code and its documentation.
No functional changes are introduced.

Changes from v1:
 - Correct compilation error when CONFIG_ATA_SFF is not enabled by
   defining a stub for sata_sff_hardreset().

Damien Le Moal (3):
  ata: libata-eh: Make ata_eh_followup_srst_needed() return a bool
  ata: libata-eh: Remove ata_do_eh()
  Documentation: driver-api: Update libata error handler information

 Documentation/driver-api/libata.rst | 21 ++++++-----
 drivers/ata/libata-eh.c             | 56 ++++++++++-------------------
 drivers/ata/libata-sff.c            | 10 +-----
 include/linux/libata.h              | 11 ++++--
 4 files changed, 39 insertions(+), 59 deletions(-)

-- 
2.50.0

[PATCH v2 1/3] ata: libata-eh: Make ata_eh_followup_srst_needed() return a bool

[Damien Le Moal]

ata_eh_followup_srst_needed() returns an integer used as a boolean. So
change this function to return that type.

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 42aafb1ddb5a..436536112043 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2781,15 +2781,15 @@ static int ata_do_reset(struct ata_link *link, ata_reset_fn_t reset,
 	return reset(link, classes, deadline);
 }
 
-static int ata_eh_followup_srst_needed(struct ata_link *link, int rc)
+static bool ata_eh_followup_srst_needed(struct ata_link *link, int rc)
 {
 	if ((link->flags & ATA_LFLAG_NO_SRST) || ata_link_offline(link))
-		return 0;
+		return false;
 	if (rc == -EAGAIN)
-		return 1;
+		return true;
 	if (sata_pmp_supported(link->ap) && ata_is_host_link(link))
-		return 1;
-	return 0;
+		return true;
+	return false;
 }
 
 int ata_eh_reset(struct ata_link *link, int classify,
-- 
2.50.0

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

[Damien Le Moal]

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
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() 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
hardreset port operations is modified to be the one that was used in
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;
+
 	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 */
+
+static inline int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
+				     unsigned long deadline)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_ATA_SFF */
 
 #endif /* __LINUX_LIBATA_H__ */
-- 
2.50.0

[PATCH v2 3/3] Documentation: driver-api: Update libata error handler information

[Damien Le Moal]

Update ``->error_handler()`` section of the libata documentation file
Documentation/driver-api/libata.rst to remove the reference to the
function ata_do_eh() as that function was removed. The reference to the
function ata_bmdma_drive_eh() is also removed as that function does not
exist at all. And while at it, cleanup the description of the various
port reset operations using a bullet list.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 Documentation/driver-api/libata.rst | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst
index 5da27a749246..e160ef2e0791 100644
--- a/Documentation/driver-api/libata.rst
+++ b/Documentation/driver-api/libata.rst
@@ -283,18 +283,21 @@ interrupts, start DMA engine, etc.
 
 ``->error_handler()`` is a driver's hook into probe, hotplug, and recovery
 and other exceptional conditions. The primary responsibility of an
-implementation is to call :c:func:`ata_do_eh` or :c:func:`ata_bmdma_drive_eh`
-with a set of EH hooks as arguments:
+implementation is to call :c:func:`ata_std_error_handler`.
 
-'prereset' hook (may be NULL) is called during an EH reset, before any
-other actions are taken.
+:c:func:`ata_std_error_handler` will call the various port reset operations as
+needed.
 
-'postreset' hook (may be NULL) is called after the EH reset is
-performed. Based on existing conditions, severity of the problem, and
-hardware capabilities,
+* The 'prereset' operation (may be NULL) is called during an EH reset, before
+  any other action is taken.
 
-Either 'softreset' (may be NULL) or 'hardreset' (may be NULL) will be
-called to perform the low-level EH reset.
+* The 'postreset' hook (may be NULL) is called after the EH reset is performed.
+  Based on existing conditions, severity of the problem, and hardware
+  capabilities,
+
+* Either the 'softreset' operation (may be NULL) or the 'hardreset' operation
+  (may be NULL) will be called to perform the low-level EH reset. If both
+  operations are defined, 'hardreset' is preferred and used.
 
 ::
 
-- 
2.50.0

Re: [PATCH v2 1/3] ata: libata-eh: Make ata_eh_followup_srst_needed() return a bool

[Hannes Reinecke]

On 7/8/25 09:36, Damien Le Moal wrote:
> ata_eh_followup_srst_needed() returns an integer used as a boolean. So
> change this function to return that type.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 42aafb1ddb5a..436536112043 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2781,15 +2781,15 @@ static int ata_do_reset(struct ata_link *link, ata_reset_fn_t reset,
>   	return reset(link, classes, deadline);
>   }
>   
> -static int ata_eh_followup_srst_needed(struct ata_link *link, int rc)
> +static bool ata_eh_followup_srst_needed(struct ata_link *link, int rc)
>   {
>   	if ((link->flags & ATA_LFLAG_NO_SRST) || ata_link_offline(link))
> -		return 0;
> +		return false;
>   	if (rc == -EAGAIN)
> -		return 1;
> +		return true;
>   	if (sata_pmp_supported(link->ap) && ata_is_host_link(link))
> -		return 1;
> -	return 0;
> +		return true;
> +	return false;
>   }
>   
>   int ata_eh_reset(struct ata_link *link, int classify,

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

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

[Hannes Reinecke]

On 7/8/25 09:36, 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
> 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() 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
> hardreset port operations is modified to be the one that was used in
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Re: [PATCH v2 3/3] Documentation: driver-api: Update libata error handler information

[Hannes Reinecke]

On 7/8/25 09:36, Damien Le Moal wrote:
> Update ``->error_handler()`` section of the libata documentation file
> Documentation/driver-api/libata.rst to remove the reference to the
> function ata_do_eh() as that function was removed. The reference to the
> function ata_bmdma_drive_eh() is also removed as that function does not
> exist at all. And while at it, cleanup the description of the various
> port reset operations using a bullet list.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   Documentation/driver-api/libata.rst | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

Re: [PATCH v2 3/3] Documentation: driver-api: Update libata error handler information

[Niklas Cassel]

On Tue, Jul 08, 2025 at 04:36:48PM +0900, Damien Le Moal wrote:
> Update ``->error_handler()`` section of the libata documentation file
> Documentation/driver-api/libata.rst to remove the reference to the
> function ata_do_eh() as that function was removed. The reference to the
> function ata_bmdma_drive_eh() is also removed as that function does not
> exist at all. And while at it, cleanup the description of the various
> port reset operations using a bullet list.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  Documentation/driver-api/libata.rst | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst
> index 5da27a749246..e160ef2e0791 100644
> --- a/Documentation/driver-api/libata.rst
> +++ b/Documentation/driver-api/libata.rst
> @@ -283,18 +283,21 @@ interrupts, start DMA engine, etc.
>  
>  ``->error_handler()`` is a driver's hook into probe, hotplug, and recovery
>  and other exceptional conditions. The primary responsibility of an
> -implementation is to call :c:func:`ata_do_eh` or :c:func:`ata_bmdma_drive_eh`
> -with a set of EH hooks as arguments:
> +implementation is to call :c:func:`ata_std_error_handler`.
>  
> -'prereset' hook (may be NULL) is called during an EH reset, before any
> -other actions are taken.
> +:c:func:`ata_std_error_handler` will call the various port reset operations as
> +needed.
>  
> -'postreset' hook (may be NULL) is called after the EH reset is
> -performed. Based on existing conditions, severity of the problem, and
> -hardware capabilities,
> +* The 'prereset' operation (may be NULL) is called during an EH reset, before
> +  any other action is taken.
>  
> -Either 'softreset' (may be NULL) or 'hardreset' (may be NULL) will be
> -called to perform the low-level EH reset.
> +* The 'postreset' hook (may be NULL) is called after the EH reset is performed.
> +  Based on existing conditions, severity of the problem, and hardware
> +  capabilities,
> +
> +* Either the 'softreset' operation (may be NULL) or the 'hardreset' operation
> +  (may be NULL) will be called to perform the low-level EH reset. If both
> +  operations are defined, 'hardreset' is preferred and used.

Since you are mentioning what happens when both are defined, should we mention
what happens when none are defined? (None of the callbacks will be called.)


Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v2 1/3] ata: libata-eh: Make ata_eh_followup_srst_needed() return a bool

[Niklas Cassel]

On Tue, Jul 08, 2025 at 04:36:46PM +0900, Damien Le Moal wrote:
> ata_eh_followup_srst_needed() returns an integer used as a boolean. So
> change this function to return that type.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

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

[Niklas Cassel]

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
> 

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

[Niklas Cassel]

On Tue, Jul 08, 2025 at 02:34:14PM +0200, Niklas Cassel wrote:
> s/operations/operations/

I of course meant:

s/operations/operation/


> 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;
> 	}

Oops.. these are swapped, but you get the point.


Kind regards,
Niklas

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

[Niklas Cassel]

On Tue, Jul 08, 2025 at 02:34:14PM +0200, Niklas Cassel wrote:
> On Tue, Jul 08, 2025 at 04:36:47PM +0900, Damien Le Moal wrote:
> 
> 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)

Even nicer would be create a new:

ata_reset_port_ops {
        ata_prereset_fn_t       prereset;
        ata_reset_fn_t          softreset;
        ata_reset_fn_t          hardreset;
        ata_postreset_fn_t      postreset;
}

And then modify:

struct ata_port_operations {
	...
	struct ata_reset_port_ops *reset_port_ops;
	struct ata_reset_port_ops *reset_port_pmp_ops;
	...
}


and then modify ata_eh_recover() to:
int ata_eh_recover(struct ata_port *ap, struct ata_reset_port_ops *ops,
                   struct ata_link **r_failed_link)


and then modify ata_eh_reset() to:

int ata_eh_reset(struct ata_link *link, int classify, struct ata_reset_port_ops *ops)


Kind regards,
Niklas