[PATCH 0/3] libata-eh cleanups

[PATCH 0/3] libata-eh cleanups

[Damien Le Moal]

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

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              |  3 --
 4 files changed, 31 insertions(+), 59 deletions(-)

-- 
2.50.0

[PATCH 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 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() with the hardreset operation change of
ata_sff_error_handler(). With this, ata_sff_error_handler() is
simplified and only need 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   |  3 ---
 3 files changed, 14 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..4a7f308fa4e5 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);
-- 
2.50.0

[PATCH 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 2/3] ata: libata-eh: Remove ata_do_eh()

[kernel test robot]

Hi Damien,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.16-rc5 next-20250704]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/ata-libata-eh-Make-ata_eh_followup_srst_needed-return-a-bool/20250707-190306
base:   linus/master
patch link:    https://lore.kernel.org/r/20250707105931.548315-3-dlemoal%40kernel.org
patch subject: [PATCH 2/3] ata: libata-eh: Remove ata_do_eh()
config: csky-randconfig-001-20250708 (https://download.01.org/0day-ci/archive/20250708/202507081215.hXgFd0Tv-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250708/202507081215.hXgFd0Tv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507081215.hXgFd0Tv-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/ata/libata-eh.c: In function 'ata_std_error_handler':
>> drivers/ata/libata-eh.c:4056:27: error: 'sata_sff_hardreset' undeclared (first use in this function); did you mean 'sata_std_hardreset'?
    4056 |              hardreset == sata_sff_hardreset) && !sata_scr_valid(&ap->link))
         |                           ^~~~~~~~~~~~~~~~~~
         |                           sata_std_hardreset
   drivers/ata/libata-eh.c:4056:27: note: each undeclared identifier is reported only once for each function it appears in


vim +4056 drivers/ata/libata-eh.c

  4038	
  4039	/**
  4040	 *	ata_std_error_handler - standard error handler
  4041	 *	@ap: host port to handle error for
  4042	 *
  4043	 *	Perform standard error handling sequence.
  4044	 *
  4045	 *	LOCKING:
  4046	 *	Kernel thread context (may sleep).
  4047	 */
  4048	void ata_std_error_handler(struct ata_port *ap)
  4049	{
  4050		struct ata_port_operations *ops = ap->ops;
  4051		ata_reset_fn_t hardreset = ops->hardreset;
  4052		int rc;
  4053	
  4054		/* Ignore built-in hardresets if SCR access is not available */
  4055		if ((hardreset == sata_std_hardreset ||
> 4056		     hardreset == sata_sff_hardreset) && !sata_scr_valid(&ap->link))
  4057			hardreset = NULL;
  4058	
  4059		ata_eh_autopsy(ap);
  4060		ata_eh_report(ap);
  4061	
  4062		rc = ata_eh_recover(ap, ops->prereset, ops->softreset,
  4063				    hardreset, ops->postreset, NULL);
  4064		if (rc) {
  4065			struct ata_device *dev;
  4066	
  4067			ata_for_each_dev(dev, &ap->link, ALL)
  4068				ata_dev_disable(dev);
  4069		}
  4070	
  4071		ata_eh_finish(ap);
  4072	}
  4073	EXPORT_SYMBOL_GPL(ata_std_error_handler);
  4074	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki