[PATCH 0/3] Improve log directory handling and some cleanups

[PATCH 0/3] Improve log directory handling and some cleanups

[Damien Le Moal]

The first patch improves handling of a device general purpose log
directory by avoiding repeated accesses to it using a cache.

The following 2 patches are simple cleanups that do not introduce
functional changes.

Damien Le Moal (3):
  ata: libata-core: Cache the general purpose log directory
  ata: libata-core: Make ata_dev_cleanup_cdl_resources() static
  ata: libata-eh: Rename and make ata_set_mode() static

 drivers/ata/libata-core.c   | 45 ++++++++++++++++++++++++++++++++-----
 drivers/ata/libata-eh.c     | 11 ++++-----
 drivers/ata/libata.h        |  2 --
 drivers/ata/pata_optidma.c  |  4 +++-
 drivers/ata/pata_pcmcia.c   |  4 ++--
 drivers/ata/pata_pdc2027x.c |  2 +-
 drivers/ata/sata_sil.c      |  2 +-
 include/linux/libata.h      |  5 ++++-
 8 files changed, 56 insertions(+), 19 deletions(-)

-- 
2.50.0

[PATCH 1/3] ata: libata-core: Cache the general purpose log directory

[Damien Le Moal]

The function ata_log_supported() tests if a log page is supported by a
device using the General Purpose Log Directory log page, which lists the
size of all surported log pages. However, this log page is read from the
device using ata_read_log_page() every time ata_log_supported() is
called. That is not necessary.

Avoid reading the General Purpose Log Directory log page by caching its
content in the gp_log_dir buffer defined as part of struct ata_device.
The functions ata_read_log_directory() and ata_clear_log_directory() are
introduced to manage this buffer. ata_clear_log_directory() zero-fill
the gp_log_dir buffer every time ata_dev_configure() is called, that is,
when the device is first scanned and when it is being revalidated.
The function ata_log_supported() is modified to call
ata_read_log_directory() instead of ata_read_log_page().

The function ata_read_log_directory() calls ata_read_log_page() to read
the General Purpose Log Directory log page from the device only if the
first 16-bits word of the log is not equal to 0x0001, that is, it is not
equal to the ACS mandated value for the log version.

With this, the log page is read from the device only once for every
ata_dev_configure() call. For instance, with pr_debug enabled, a call
to ata_dev_configure() before this patch generates the following log
page accesses:

ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x13, page 0x0
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x12, page 0x0
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x30, page 0x8
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x30, page 0x3
ata3.00: read log page - log 0x30, page 0x4
ata3.00: read log page - log 0x18, page 0x0

That is, the general purpose log directory page is read 7 times.
With this patch applied, the number of accesses to this log page is
reduced to one:

ata3.00: read log page - log 0x0, page 0x0
ata3.00: read log page - log 0x13, page 0x0
ata3.00: read log page - log 0x12, page 0x0
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x30, page 0x8
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x30, page 0x0
ata3.00: read log page - log 0x30, page 0x3
ata3.00: read log page - log 0x30, page 0x4
ata3.00: read log page - log 0x18, page 0x0

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 37 +++++++++++++++++++++++++++++++++++--
 include/linux/libata.h    |  3 +++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7f6cebe61b33..806ff5cee2b4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2154,14 +2154,44 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 	return err_mask;
 }
 
+static inline void ata_clear_log_directory(struct ata_device *dev)
+{
+	memset(dev->gp_log_dir, 0, ATA_SECT_SIZE);
+}
+
+static int ata_read_log_directory(struct ata_device *dev)
+{
+	u16 version;
+
+	/* If the log page is already cached, do nothing. */
+	version = get_unaligned_le16(&dev->gp_log_dir[0]);
+	if (version == 0x0001)
+		return 0;
+
+	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->gp_log_dir, 1)) {
+		ata_clear_log_directory(dev);
+		return -EIO;
+	}
+
+	version = get_unaligned_le16(&dev->gp_log_dir[0]);
+	if (version != 0x0001) {
+		ata_dev_err(dev, "Invalid log directory version 0x%04x\n",
+			    version);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ata_log_supported(struct ata_device *dev, u8 log)
 {
 	if (dev->quirks & ATA_QUIRK_NO_LOG_DIR)
 		return 0;
 
-	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->sector_buf, 1))
+	if (ata_read_log_directory(dev))
 		return 0;
-	return get_unaligned_le16(&dev->sector_buf[log * 2]);
+
+	return get_unaligned_le16(&dev->gp_log_dir[log * 2]);
 }
 
 static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
@@ -2890,6 +2920,9 @@ int ata_dev_configure(struct ata_device *dev)
 		return 0;
 	}
 
+	/* Clear the general purpose log directory cache. */
+	ata_clear_log_directory(dev);
+
 	/* Set quirks */
 	dev->quirks |= ata_dev_quirks(dev);
 	ata_force_quirks(dev);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7462218312ad..78a4addc6659 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -761,6 +761,9 @@ struct ata_device {
 		u32		gscr[SATA_PMP_GSCR_DWORDS]; /* PMP GSCR block */
 	} ____cacheline_aligned;
 
+	/* General Purpose Log Directory log page */
+	u8			gp_log_dir[ATA_SECT_SIZE] ____cacheline_aligned;
+
 	/* DEVSLP Timing Variables from Identify Device Data Log */
 	u8			devslp_timing[ATA_LOG_DEVSLP_SIZE];
 
-- 
2.50.0

Re: [PATCH 1/3] ata: libata-core: Cache the general purpose log directory

[Niklas Cassel]

On Thu, Jul 03, 2025 at 04:15:39PM +0900, Damien Le Moal wrote:
> The function ata_log_supported() tests if a log page is supported by a
> device using the General Purpose Log Directory log page, which lists the
> size of all surported log pages. However, this log page is read from the
> device using ata_read_log_page() every time ata_log_supported() is
> called. That is not necessary.
> 
> Avoid reading the General Purpose Log Directory log page by caching its
> content in the gp_log_dir buffer defined as part of struct ata_device.
> The functions ata_read_log_directory() and ata_clear_log_directory() are
> introduced to manage this buffer. ata_clear_log_directory() zero-fill
> the gp_log_dir buffer every time ata_dev_configure() is called, that is,
> when the device is first scanned and when it is being revalidated.
> The function ata_log_supported() is modified to call
> ata_read_log_directory() instead of ata_read_log_page().
> 
> The function ata_read_log_directory() calls ata_read_log_page() to read
> the General Purpose Log Directory log page from the device only if the
> first 16-bits word of the log is not equal to 0x0001, that is, it is not
> equal to the ACS mandated value for the log version.
> 
> With this, the log page is read from the device only once for every
> ata_dev_configure() call. For instance, with pr_debug enabled, a call
> to ata_dev_configure() before this patch generates the following log
> page accesses:
> 
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x13, page 0x0
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x12, page 0x0
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x30, page 0x8
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x30, page 0x3
> ata3.00: read log page - log 0x30, page 0x4
> ata3.00: read log page - log 0x18, page 0x0
> 
> That is, the general purpose log directory page is read 7 times.
> With this patch applied, the number of accesses to this log page is
> reduced to one:
> 
> ata3.00: read log page - log 0x0, page 0x0
> ata3.00: read log page - log 0x13, page 0x0
> ata3.00: read log page - log 0x12, page 0x0
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x30, page 0x8
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x30, page 0x0
> ata3.00: read log page - log 0x30, page 0x3
> ata3.00: read log page - log 0x30, page 0x4
> ata3.00: read log page - log 0x18, page 0x0
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 37 +++++++++++++++++++++++++++++++++++--
>  include/linux/libata.h    |  3 +++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 7f6cebe61b33..806ff5cee2b4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2154,14 +2154,44 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	return err_mask;
>  }
>  
> +static inline void ata_clear_log_directory(struct ata_device *dev)
> +{
> +	memset(dev->gp_log_dir, 0, ATA_SECT_SIZE);
> +}
> +
> +static int ata_read_log_directory(struct ata_device *dev)
> +{
> +	u16 version;
> +
> +	/* If the log page is already cached, do nothing. */
> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
> +	if (version == 0x0001)
> +		return 0;
> +
> +	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->gp_log_dir, 1)) {
> +		ata_clear_log_directory(dev);

Why do we need to clear the log here?

If we had something cached, we would have returned in the if-statement above
already.

And if the read failed, wouldn't the buffer be unmodified?

Since we are only reading a single sector, which is the smallest unit we
can read, I would expect the buffer to be unmodified on failure.

Is that an incorrect assumption?


> +		return -EIO;
> +	}
> +
> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
> +	if (version != 0x0001) {
> +		ata_dev_err(dev, "Invalid log directory version 0x%04x\n",
> +			    version);
> +		return -EINVAL;

Don't you want to call ata_clear_log_directory() here?


> +	}
> +
> +	return 0;
> +}
> +
>  static int ata_log_supported(struct ata_device *dev, u8 log)
>  {
>  	if (dev->quirks & ATA_QUIRK_NO_LOG_DIR)
>  		return 0;
>  
> -	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->sector_buf, 1))
> +	if (ata_read_log_directory(dev))
>  		return 0;
> -	return get_unaligned_le16(&dev->sector_buf[log * 2]);
> +
> +	return get_unaligned_le16(&dev->gp_log_dir[log * 2]);
>  }
>  
>  static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2890,6 +2920,9 @@ int ata_dev_configure(struct ata_device *dev)
>  		return 0;
>  	}
>  
> +	/* Clear the general purpose log directory cache. */
> +	ata_clear_log_directory(dev);
> +
>  	/* Set quirks */
>  	dev->quirks |= ata_dev_quirks(dev);
>  	ata_force_quirks(dev);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 7462218312ad..78a4addc6659 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -761,6 +761,9 @@ struct ata_device {
>  		u32		gscr[SATA_PMP_GSCR_DWORDS]; /* PMP GSCR block */
>  	} ____cacheline_aligned;
>  
> +	/* General Purpose Log Directory log page */
> +	u8			gp_log_dir[ATA_SECT_SIZE] ____cacheline_aligned;

Why align this to a cacheline?

It shouldn't be needed for get_unaligned_le16() to work correctly.

Is it just to increase the chance of this being in the cache?

If so, do we really access this that often that it needs to be
cacheline aligned? (We mostly access it during probe time, no?)


Kind regards,
Niklas

Re: [PATCH 1/3] ata: libata-core: Cache the general purpose log directory

[Damien Le Moal]

On 7/3/25 6:49 PM, Niklas Cassel wrote:
>> +static int ata_read_log_directory(struct ata_device *dev)
>> +{
>> +	u16 version;
>> +
>> +	/* If the log page is already cached, do nothing. */
>> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
>> +	if (version == 0x0001)
>> +		return 0;
>> +
>> +	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->gp_log_dir, 1)) {
>> +		ata_clear_log_directory(dev);
> 
> Why do we need to clear the log here?
> 
> If we had something cached, we would have returned in the if-statement above
> already.
> 
> And if the read failed, wouldn't the buffer be unmodified?
> 
> Since we are only reading a single sector, which is the smallest unit we
> can read, I would expect the buffer to be unmodified on failure.
> 
> Is that an incorrect assumption?

I do not think it is needed either. It is a little bit of paranoia, e.g. in
case we lost the link in the middle of transfers and have a half-full buffer.

> 
> 
>> +		return -EIO;
>> +	}
>> +
>> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
>> +	if (version != 0x0001) {
>> +		ata_dev_err(dev, "Invalid log directory version 0x%04x\n",
>> +			    version);
>> +		return -EINVAL;
> 
> Don't you want to call ata_clear_log_directory() here?

Yep. Forgot to add it. And I think we should also set ATA_QUIRK_NO_LOG_DIR if
we ever get this error because it will be pointless to retry.

>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 7462218312ad..78a4addc6659 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -761,6 +761,9 @@ struct ata_device {
>>  		u32		gscr[SATA_PMP_GSCR_DWORDS]; /* PMP GSCR block */
>>  	} ____cacheline_aligned;
>>  
>> +	/* General Purpose Log Directory log page */
>> +	u8			gp_log_dir[ATA_SECT_SIZE] ____cacheline_aligned;
> 
> Why align this to a cacheline?
> 
> It shouldn't be needed for get_unaligned_le16() to work correctly.
> 
> Is it just to increase the chance of this being in the cache?
> 
> If so, do we really access this that often that it needs to be
> cacheline aligned? (We mostly access it during probe time, no?)

It is used as the buffer for the read log command, so similar to the
sector_buf, I made it cacheline aligned. Better for DMA I guess.


-- 
Damien Le Moal
Western Digital Research

[PATCH 2/3] ata: libata-core: Make ata_dev_cleanup_cdl_resources() static

[Damien Le Moal]

The function ata_dev_cleanup_cdl_resources() is called only from
ata_dev_init_cdl_resources() and ata_dev_free_resources() which are
both defined in drivers/ata/libata-core.c. So there is no need for
ata_dev_cleanup_cdl_resources() to be visible from outside of this
file. Change this function definition to be a static function.

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 2 +-
 drivers/ata/libata.h      | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 806ff5cee2b4..fb06aef8e0f6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2514,7 +2514,7 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 		dev->flags |= ATA_DFLAG_TRUSTED;
 }
 
-void ata_dev_cleanup_cdl_resources(struct ata_device *dev)
+static void ata_dev_cleanup_cdl_resources(struct ata_device *dev)
 {
 	kfree(dev->cdl);
 	dev->cdl = NULL;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 8e68f4556962..767a61f8ff89 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -97,7 +97,6 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern const char *sata_spd_string(unsigned int spd);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
-void ata_dev_cleanup_cdl_resources(struct ata_device *dev);
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
-- 
2.50.0

Re: [PATCH 2/3] ata: libata-core: Make ata_dev_cleanup_cdl_resources() static

[Niklas Cassel]

On Thu, Jul 03, 2025 at 04:15:40PM +0900, Damien Le Moal wrote:
> The function ata_dev_cleanup_cdl_resources() is called only from
> ata_dev_init_cdl_resources() and ata_dev_free_resources() which are
> both defined in drivers/ata/libata-core.c. So there is no need for
> ata_dev_cleanup_cdl_resources() to be visible from outside of this
> file. Change this function definition to be a static function.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

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

[PATCH 3/3] ata: libata-eh: Rename and make ata_set_mode() static

[Damien Le Moal]

The function ata_set_mode() is defined and used only in libata-eh.c.
Make this function static to libata-eh.c and rename it
ata_eh_set_mode() to make it clear where its definition is. This
renaming allows renaming ata_do_set_mode() to the simpler name
ata_set_mode().

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c   |  6 +++---
 drivers/ata/libata-eh.c     | 11 ++++++-----
 drivers/ata/libata.h        |  1 -
 drivers/ata/pata_optidma.c  |  4 +++-
 drivers/ata/pata_pcmcia.c   |  4 ++--
 drivers/ata/pata_pdc2027x.c |  2 +-
 drivers/ata/sata_sil.c      |  2 +-
 include/linux/libata.h      |  2 +-
 8 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fb06aef8e0f6..3bdd65bbff49 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3508,7 +3508,7 @@ static int ata_dev_set_mode(struct ata_device *dev)
 }
 
 /**
- *	ata_do_set_mode - Program timings and issue SET FEATURES - XFER
+ *	ata_set_mode - Program timings and issue SET FEATURES - XFER
  *	@link: link on which timings will be programmed
  *	@r_failed_dev: out parameter for failed device
  *
@@ -3524,7 +3524,7 @@ static int ata_dev_set_mode(struct ata_device *dev)
  *	0 on success, negative errno otherwise
  */
 
-int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
+int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_device *dev;
@@ -3605,7 +3605,7 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 		*r_failed_dev = dev;
 	return rc;
 }
-EXPORT_SYMBOL_GPL(ata_do_set_mode);
+EXPORT_SYMBOL_GPL(ata_set_mode);
 
 /**
  *	ata_wait_ready - wait for link to become ready
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 0f9c30f9bc1e..e5fa61fb8a59 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3408,12 +3408,12 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 }
 
 /**
- *	ata_set_mode - Program timings and issue SET FEATURES - XFER
+ *	ata_eh_set_mode - Program timings and issue SET FEATURES - XFER
  *	@link: link on which timings will be programmed
  *	@r_failed_dev: out parameter for failed device
  *
  *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
- *	ata_set_mode() fails, pointer to the failing device is
+ *	ata_eh_set_mode() fails, pointer to the failing device is
  *	returned in @r_failed_dev.
  *
  *	LOCKING:
@@ -3422,7 +3422,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
  *	RETURNS:
  *	0 on success, negative errno otherwise
  */
-int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
+static int ata_eh_set_mode(struct ata_link *link,
+			   struct ata_device **r_failed_dev)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_device *dev;
@@ -3443,7 +3444,7 @@ int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 	if (ap->ops->set_mode)
 		rc = ap->ops->set_mode(link, r_failed_dev);
 	else
-		rc = ata_do_set_mode(link, r_failed_dev);
+		rc = ata_set_mode(link, r_failed_dev);
 
 	/* if transfer mode has changed, set DUBIOUS_XFER on device */
 	ata_for_each_dev(dev, link, ENABLED) {
@@ -3926,7 +3927,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 
 		/* configure transfer mode if necessary */
 		if (ehc->i.flags & ATA_EHI_SETMODE) {
-			rc = ata_set_mode(link, &dev);
+			rc = ata_eh_set_mode(link, &dev);
 			if (rc)
 				goto rest_fail;
 			ehc->i.flags &= ~ATA_EHI_SETMODE;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 767a61f8ff89..135e1e5ee44e 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -177,7 +177,6 @@ extern void ata_eh_report(struct ata_port *ap);
 extern int ata_eh_reset(struct ata_link *link, int classify,
 			ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
 			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset);
-extern int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 			  ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			  ata_postreset_fn_t postreset,
diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
index dfc36b4ec9c6..cc876dc7a9d8 100644
--- a/drivers/ata/pata_optidma.c
+++ b/drivers/ata/pata_optidma.c
@@ -322,7 +322,9 @@ static int optidma_set_mode(struct ata_link *link, struct ata_device **r_failed)
 	u8 r;
 	int nybble = 4 * ap->port_no;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	int rc  = ata_do_set_mode(link, r_failed);
+	int rc;
+
+	rc = ata_set_mode(link, r_failed);
 	if (rc == 0) {
 		pci_read_config_byte(pdev, 0x43, &r);
 
diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index 5b602206c522..cf3810933a27 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -46,7 +46,7 @@ static int pcmcia_set_mode(struct ata_link *link, struct ata_device **r_failed_d
 	struct ata_device *slave = &link->device[1];
 
 	if (!ata_dev_enabled(master) || !ata_dev_enabled(slave))
-		return ata_do_set_mode(link, r_failed_dev);
+		return ata_set_mode(link, r_failed_dev);
 
 	if (memcmp(master->id + ATA_ID_FW_REV,  slave->id + ATA_ID_FW_REV,
 			   ATA_ID_FW_REV_LEN + ATA_ID_PROD_LEN) == 0) {
@@ -58,7 +58,7 @@ static int pcmcia_set_mode(struct ata_link *link, struct ata_device **r_failed_d
 			ata_dev_disable(slave);
 		}
 	}
-	return ata_do_set_mode(link, r_failed_dev);
+	return ata_set_mode(link, r_failed_dev);
 }
 
 /**
diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
index 6820c5597b14..a4ee3b92c9aa 100644
--- a/drivers/ata/pata_pdc2027x.c
+++ b/drivers/ata/pata_pdc2027x.c
@@ -387,7 +387,7 @@ static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed
 	struct ata_device *dev;
 	int rc;
 
-	rc = ata_do_set_mode(link, r_failed);
+	rc = ata_set_mode(link, r_failed);
 	if (rc < 0)
 		return rc;
 
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 3a99f66198a9..1b6dc950a42a 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -351,7 +351,7 @@ static int sil_set_mode(struct ata_link *link, struct ata_device **r_failed)
 	u32 tmp, dev_mode[2] = { };
 	int rc;
 
-	rc = ata_do_set_mode(link, r_failed);
+	rc = ata_set_mode(link, r_failed);
 	if (rc)
 		return rc;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 78a4addc6659..d092747be588 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1218,7 +1218,7 @@ extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
 extern int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
 			       bool enable);
 extern struct ata_device *ata_dev_pair(struct ata_device *adev);
-extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
+int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
 extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
 extern void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, struct list_head *eh_q);
 
-- 
2.50.0

Re: [PATCH 3/3] ata: libata-eh: Rename and make ata_set_mode() static

[Niklas Cassel]

On Thu, Jul 03, 2025 at 04:15:41PM +0900, Damien Le Moal wrote:
> The function ata_set_mode() is defined and used only in libata-eh.c.
> Make this function static to libata-eh.c and rename it
> ata_eh_set_mode() to make it clear where its definition is. This
> renaming allows renaming ata_do_set_mode() to the simpler name
> ata_set_mode().

"This renaming allows renaming" is slightly confusing read.

Perhaps:
... renaming ata_set_mode() to ata_eh_set_mode() allows us to
rename ata_do_set_mode() to ata_set_mode()."


However, I think this commit would be easier to review/less confusing if
it was two separate patches :)

Patch 1 that renames ata_set_mode() to ata_eh_set_mode() and makes it static.
Patch 2 that renames ata_do_set_mode() to ata_set_mode().


> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c   |  6 +++---
>  drivers/ata/libata-eh.c     | 11 ++++++-----
>  drivers/ata/libata.h        |  1 -
>  drivers/ata/pata_optidma.c  |  4 +++-
>  drivers/ata/pata_pcmcia.c   |  4 ++--
>  drivers/ata/pata_pdc2027x.c |  2 +-
>  drivers/ata/sata_sil.c      |  2 +-
>  include/linux/libata.h      |  2 +-
>  8 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index fb06aef8e0f6..3bdd65bbff49 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3508,7 +3508,7 @@ static int ata_dev_set_mode(struct ata_device *dev)
>  }
>  
>  /**
> - *	ata_do_set_mode - Program timings and issue SET FEATURES - XFER
> + *	ata_set_mode - Program timings and issue SET FEATURES - XFER
>   *	@link: link on which timings will be programmed
>   *	@r_failed_dev: out parameter for failed device
>   *
> @@ -3524,7 +3524,7 @@ static int ata_dev_set_mode(struct ata_device *dev)
>   *	0 on success, negative errno otherwise
>   */
>  
> -int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
> +int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
>  {
>  	struct ata_port *ap = link->ap;
>  	struct ata_device *dev;
> @@ -3605,7 +3605,7 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
>  		*r_failed_dev = dev;
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(ata_do_set_mode);
> +EXPORT_SYMBOL_GPL(ata_set_mode);
>  
>  /**
>   *	ata_wait_ready - wait for link to become ready
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 0f9c30f9bc1e..e5fa61fb8a59 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3408,12 +3408,12 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>  }
>  
>  /**
> - *	ata_set_mode - Program timings and issue SET FEATURES - XFER
> + *	ata_eh_set_mode - Program timings and issue SET FEATURES - XFER
>   *	@link: link on which timings will be programmed
>   *	@r_failed_dev: out parameter for failed device
>   *
>   *	Set ATA device disk transfer mode (PIO3, UDMA6, etc.).  If
> - *	ata_set_mode() fails, pointer to the failing device is
> + *	ata_eh_set_mode() fails, pointer to the failing device is
>   *	returned in @r_failed_dev.
>   *
>   *	LOCKING:
> @@ -3422,7 +3422,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>   *	RETURNS:
>   *	0 on success, negative errno otherwise
>   */
> -int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
> +static int ata_eh_set_mode(struct ata_link *link,
> +			   struct ata_device **r_failed_dev)
>  {
>  	struct ata_port *ap = link->ap;
>  	struct ata_device *dev;
> @@ -3443,7 +3444,7 @@ int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
>  	if (ap->ops->set_mode)
>  		rc = ap->ops->set_mode(link, r_failed_dev);
>  	else
> -		rc = ata_do_set_mode(link, r_failed_dev);
> +		rc = ata_set_mode(link, r_failed_dev);
>  
>  	/* if transfer mode has changed, set DUBIOUS_XFER on device */
>  	ata_for_each_dev(dev, link, ENABLED) {
> @@ -3926,7 +3927,7 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  
>  		/* configure transfer mode if necessary */
>  		if (ehc->i.flags & ATA_EHI_SETMODE) {
> -			rc = ata_set_mode(link, &dev);
> +			rc = ata_eh_set_mode(link, &dev);
>  			if (rc)
>  				goto rest_fail;
>  			ehc->i.flags &= ~ATA_EHI_SETMODE;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 767a61f8ff89..135e1e5ee44e 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -177,7 +177,6 @@ extern void ata_eh_report(struct ata_port *ap);
>  extern int ata_eh_reset(struct ata_link *link, int classify,
>  			ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
>  			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset);
> -extern int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
>  extern int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
>  			  ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
>  			  ata_postreset_fn_t postreset,
> diff --git a/drivers/ata/pata_optidma.c b/drivers/ata/pata_optidma.c
> index dfc36b4ec9c6..cc876dc7a9d8 100644
> --- a/drivers/ata/pata_optidma.c
> +++ b/drivers/ata/pata_optidma.c
> @@ -322,7 +322,9 @@ static int optidma_set_mode(struct ata_link *link, struct ata_device **r_failed)
>  	u8 r;
>  	int nybble = 4 * ap->port_no;
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> -	int rc  = ata_do_set_mode(link, r_failed);
> +	int rc;
> +
> +	rc = ata_set_mode(link, r_failed);
>  	if (rc == 0) {
>  		pci_read_config_byte(pdev, 0x43, &r);
>  
> diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
> index 5b602206c522..cf3810933a27 100644
> --- a/drivers/ata/pata_pcmcia.c
> +++ b/drivers/ata/pata_pcmcia.c
> @@ -46,7 +46,7 @@ static int pcmcia_set_mode(struct ata_link *link, struct ata_device **r_failed_d
>  	struct ata_device *slave = &link->device[1];
>  
>  	if (!ata_dev_enabled(master) || !ata_dev_enabled(slave))
> -		return ata_do_set_mode(link, r_failed_dev);
> +		return ata_set_mode(link, r_failed_dev);
>  
>  	if (memcmp(master->id + ATA_ID_FW_REV,  slave->id + ATA_ID_FW_REV,
>  			   ATA_ID_FW_REV_LEN + ATA_ID_PROD_LEN) == 0) {
> @@ -58,7 +58,7 @@ static int pcmcia_set_mode(struct ata_link *link, struct ata_device **r_failed_d
>  			ata_dev_disable(slave);
>  		}
>  	}
> -	return ata_do_set_mode(link, r_failed_dev);
> +	return ata_set_mode(link, r_failed_dev);
>  }
>  
>  /**
> diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
> index 6820c5597b14..a4ee3b92c9aa 100644
> --- a/drivers/ata/pata_pdc2027x.c
> +++ b/drivers/ata/pata_pdc2027x.c
> @@ -387,7 +387,7 @@ static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed
>  	struct ata_device *dev;
>  	int rc;
>  
> -	rc = ata_do_set_mode(link, r_failed);
> +	rc = ata_set_mode(link, r_failed);
>  	if (rc < 0)
>  		return rc;
>  
> diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> index 3a99f66198a9..1b6dc950a42a 100644
> --- a/drivers/ata/sata_sil.c
> +++ b/drivers/ata/sata_sil.c
> @@ -351,7 +351,7 @@ static int sil_set_mode(struct ata_link *link, struct ata_device **r_failed)
>  	u32 tmp, dev_mode[2] = { };
>  	int rc;
>  
> -	rc = ata_do_set_mode(link, r_failed);
> +	rc = ata_set_mode(link, r_failed);
>  	if (rc)
>  		return rc;
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 78a4addc6659..d092747be588 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1218,7 +1218,7 @@ extern int ata_ncq_prio_enabled(struct ata_port *ap, struct scsi_device *sdev,
>  extern int ata_ncq_prio_enable(struct ata_port *ap, struct scsi_device *sdev,
>  			       bool enable);
>  extern struct ata_device *ata_dev_pair(struct ata_device *adev);
> -extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
> +int ata_set_mode(struct ata_link *link, struct ata_device **r_failed_dev);
>  extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
>  extern void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, struct list_head *eh_q);
>  
> -- 
> 2.50.0
>