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