[PATCH 0/7] Code cleanup and CDL memory usage reduction

[PATCH 0/7] Code cleanup and CDL memory usage reduction

[Damien Le Moal]

This patch series starts by moving code that is SATA specific from
libata-core.c to libata-sata.c, without any functional change. The
benefit is a smaller ata code for hosts that do not support SATA. This
is done in patch 1 to 4.

The second part of the series (patch 5 to 7) cleanup the CDL support
code by moving device resources from struct ata_port to struct ata_dev
and reduce the size of struct ata_dev by allocating buffers needed for
CDL only for drives that actually support this feature.

Damien Le Moal (7):
  ata: libata: Fix ata_tdev_free() kdoc comment
  ata: libata: Improve __ata_qc_complete()
  ata: libata: Move sata_down_spd_limit() to libata-sata.c
  ata: libata: Move sata_std_hardreset() definition to libata-sata.c
  ata: libata: Rename ata_eh_read_sense_success_ncq_log()
  ata: libata: Move ncq_sense_buf to struct ata_device
  ata: libata: Improve CDL resource management

 drivers/ata/libata-core.c      | 189 +++++++--------------------------
 drivers/ata/libata-eh.c        |   6 +-
 drivers/ata/libata-sata.c      | 125 +++++++++++++++++++++-
 drivers/ata/libata-scsi.c      |   2 +-
 drivers/ata/libata-transport.c |  11 +-
 drivers/ata/libata.h           |  23 +++-
 include/linux/libata.h         |  34 ++++--
 7 files changed, 217 insertions(+), 173 deletions(-)

-- 
2.46.0

[PATCH 1/7] ata: libata: Fix ata_tdev_free() kdoc comment

[Damien Le Moal]

Fix the kdoc comment of ata_tdev_free() to correctly describe that this
function operates on a ATA device and not on a link or PHY.

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

diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 48800cd0e75d..474816a9efa1 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -660,14 +660,14 @@ static int ata_tdev_match(struct attribute_container *cont,
 }
 
 /**
- * ata_tdev_free  --  free a ATA LINK
- * @dev:	ATA PHY to free
+ * ata_tdev_free  --  free a ATA device
+ * @dev:	ATA device to free
  *
- * Frees the specified ATA PHY.
+ * Free the specified ATA device.
  *
  * Note:
- *   This function must only be called on a PHY that has not
- *   successfully been added using ata_tdev_add().
+ *   This function must only be called on a device that has not successfully
+ *   been added using ata_tdev_add().
  */
 static void ata_tdev_free(struct ata_device *dev)
 {
-- 
2.46.0

Re: [PATCH 1/7] ata: libata: Fix ata_tdev_free() kdoc comment

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:00PM +0900, Damien Le Moal wrote:
> Fix the kdoc comment of ata_tdev_free() to correctly describe that this
> function operates on a ATA device and not on a link or PHY.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-transport.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 48800cd0e75d..474816a9efa1 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -660,14 +660,14 @@ static int ata_tdev_match(struct attribute_container *cont,
>  }
>  
>  /**
> - * ata_tdev_free  --  free a ATA LINK
> - * @dev:	ATA PHY to free
> + * ata_tdev_free  --  free a ATA device
> + * @dev:	ATA device to free
>   *
> - * Frees the specified ATA PHY.
> + * Free the specified ATA device.

Perhaps: "Frees the sysfs entry for the specified ATA device."

Perhaps do similar changes to the other ata_tdev_ functions.

E.g. for ata_tdev_delete() - "Remove the sysfs entry for the specified ATA device".


>   *
>   * Note:
> - *   This function must only be called on a PHY that has not
> - *   successfully been added using ata_tdev_add().
> + *   This function must only be called on a device that has not successfully
> + *   been added using ata_tdev_add().
>   */
>  static void ata_tdev_free(struct ata_device *dev)
>  {
> -- 
> 2.46.0
> 

[PATCH 2/7] ata: libata: Improve __ata_qc_complete()

[Damien Le Moal]

The function __ata_qc_complete() is always called with a qc that already
has been dereferenced and so is guaranteed to be non-NULL (as otherwise
the kernel would have crashed). So remove the warning for a NULL qc as
it is useless.

Furthermore, the qc passed to __ata_qc_complete() must always be marked
as active with the ATA_QCFLAG_ACTIVE flag. If that is not the case, in
addition to the existing warning, return early so that we do not attempt
to complete an invalid qc.

Finally, fix the comment related to clearing the qc active flag as that
operation applies to all devices, not just ATAPI ones.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e4023fc288ac..5acc37397f4b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4829,8 +4829,9 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
 	struct ata_port *ap;
 	struct ata_link *link;
 
-	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
-	WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE));
+	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE)))
+		return;
+
 	ap = qc->ap;
 	link = qc->dev->link;
 
@@ -4852,9 +4853,10 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
 		     ap->excl_link == link))
 		ap->excl_link = NULL;
 
-	/* atapi: mark qc as inactive to prevent the interrupt handler
-	 * from completing the command twice later, before the error handler
-	 * is called. (when rc != 0 and atapi request sense is needed)
+	/*
+	 * Mark qc as inactive to prevent the port interrupt handler from
+	 * completing the command twice later, before the error handler is
+	 * called.
 	 */
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 	ap->qc_active &= ~(1ULL << qc->tag);
-- 
2.46.0

Re: [PATCH 2/7] ata: libata: Improve __ata_qc_complete()

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:01PM +0900, Damien Le Moal wrote:
> The function __ata_qc_complete() is always called with a qc that already
> has been dereferenced and so is guaranteed to be non-NULL (as otherwise
> the kernel would have crashed). So remove the warning for a NULL qc as
> it is useless.
> 
> Furthermore, the qc passed to __ata_qc_complete() must always be marked
> as active with the ATA_QCFLAG_ACTIVE flag. If that is not the case, in
> addition to the existing warning, return early so that we do not attempt
> to complete an invalid qc.
> 
> Finally, fix the comment related to clearing the qc active flag as that
> operation applies to all devices, not just ATAPI ones.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e4023fc288ac..5acc37397f4b 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4829,8 +4829,9 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
>  	struct ata_port *ap;
>  	struct ata_link *link;
>  
> -	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
> -	WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE));
> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE)))
> +		return;
> +
>  	ap = qc->ap;
>  	link = qc->dev->link;
>  
> @@ -4852,9 +4853,10 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
>  		     ap->excl_link == link))
>  		ap->excl_link = NULL;
>  
> -	/* atapi: mark qc as inactive to prevent the interrupt handler
> -	 * from completing the command twice later, before the error handler
> -	 * is called. (when rc != 0 and atapi request sense is needed)
> +	/*
> +	 * Mark qc as inactive to prevent the port interrupt handler from
> +	 * completing the command twice later, before the error handler is
> +	 * called.
>  	 */
>  	qc->flags &= ~ATA_QCFLAG_ACTIVE;
>  	ap->qc_active &= ~(1ULL << qc->tag);
> -- 
> 2.46.0
> 

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

[PATCH 3/7] ata: libata: Move sata_down_spd_limit() to libata-sata.c

[Damien Le Moal]

Move the definition of the function sata_down_spd_limit() to
libata-sata.c where it belongs, together with sata_set_spd().
The helper function ata_sstatus_online() is also changed to be an
inline function defined in drivers/ata/libata.h.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 85 ---------------------------------------
 drivers/ata/libata-sata.c | 80 ++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h      | 17 +++++++-
 3 files changed, 96 insertions(+), 86 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5acc37397f4b..b957eb900a00 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -167,11 +167,6 @@ static inline bool ata_dev_print_info(const struct ata_device *dev)
 	return ehc->i.flags & ATA_EHI_PRINTINFO;
 }
 
-static bool ata_sstatus_online(u32 sstatus)
-{
-	return (sstatus & 0xf) == 0x3;
-}
-
 /**
  *	ata_link_next - link iteration helper
  *	@link: the previous link, NULL to start
@@ -3200,86 +3195,6 @@ struct ata_device *ata_dev_pair(struct ata_device *adev)
 }
 EXPORT_SYMBOL_GPL(ata_dev_pair);
 
-/**
- *	sata_down_spd_limit - adjust SATA spd limit downward
- *	@link: Link to adjust SATA spd limit for
- *	@spd_limit: Additional limit
- *
- *	Adjust SATA spd limit of @link downward.  Note that this
- *	function only adjusts the limit.  The change must be applied
- *	using sata_set_spd().
- *
- *	If @spd_limit is non-zero, the speed is limited to equal to or
- *	lower than @spd_limit if such speed is supported.  If
- *	@spd_limit is slower than any supported speed, only the lowest
- *	supported speed is allowed.
- *
- *	LOCKING:
- *	Inherited from caller.
- *
- *	RETURNS:
- *	0 on success, negative errno on failure
- */
-int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
-{
-	u32 sstatus, spd, mask;
-	int rc, bit;
-
-	if (!sata_scr_valid(link))
-		return -EOPNOTSUPP;
-
-	/* If SCR can be read, use it to determine the current SPD.
-	 * If not, use cached value in link->sata_spd.
-	 */
-	rc = sata_scr_read(link, SCR_STATUS, &sstatus);
-	if (rc == 0 && ata_sstatus_online(sstatus))
-		spd = (sstatus >> 4) & 0xf;
-	else
-		spd = link->sata_spd;
-
-	mask = link->sata_spd_limit;
-	if (mask <= 1)
-		return -EINVAL;
-
-	/* unconditionally mask off the highest bit */
-	bit = fls(mask) - 1;
-	mask &= ~(1 << bit);
-
-	/*
-	 * Mask off all speeds higher than or equal to the current one.  At
-	 * this point, if current SPD is not available and we previously
-	 * recorded the link speed from SStatus, the driver has already
-	 * masked off the highest bit so mask should already be 1 or 0.
-	 * Otherwise, we should not force 1.5Gbps on a link where we have
-	 * not previously recorded speed from SStatus.  Just return in this
-	 * case.
-	 */
-	if (spd > 1)
-		mask &= (1 << (spd - 1)) - 1;
-	else if (link->sata_spd)
-		return -EINVAL;
-
-	/* were we already at the bottom? */
-	if (!mask)
-		return -EINVAL;
-
-	if (spd_limit) {
-		if (mask & ((1 << spd_limit) - 1))
-			mask &= (1 << spd_limit) - 1;
-		else {
-			bit = ffs(mask) - 1;
-			mask = 1 << bit;
-		}
-	}
-
-	link->sata_spd_limit = mask;
-
-	ata_link_warn(link, "limiting SATA link speed to %s\n",
-		      sata_spd_string(fls(mask)));
-
-	return 0;
-}
-
 #ifdef CONFIG_ATA_ACPI
 /**
  *	ata_timing_cycle2mode - find xfer mode for the specified cycle duration
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index ecd37649d4d4..549377b82670 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -517,6 +517,86 @@ int sata_set_spd(struct ata_link *link)
 }
 EXPORT_SYMBOL_GPL(sata_set_spd);
 
+/**
+ *	sata_down_spd_limit - adjust SATA spd limit downward
+ *	@link: Link to adjust SATA spd limit for
+ *	@spd_limit: Additional limit
+ *
+ *	Adjust SATA spd limit of @link downward.  Note that this
+ *	function only adjusts the limit.  The change must be applied
+ *	using sata_set_spd().
+ *
+ *	If @spd_limit is non-zero, the speed is limited to equal to or
+ *	lower than @spd_limit if such speed is supported.  If
+ *	@spd_limit is slower than any supported speed, only the lowest
+ *	supported speed is allowed.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure
+ */
+int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
+{
+	u32 sstatus, spd, mask;
+	int rc, bit;
+
+	if (!sata_scr_valid(link))
+		return -EOPNOTSUPP;
+
+	/* If SCR can be read, use it to determine the current SPD.
+	 * If not, use cached value in link->sata_spd.
+	 */
+	rc = sata_scr_read(link, SCR_STATUS, &sstatus);
+	if (rc == 0 && ata_sstatus_online(sstatus))
+		spd = (sstatus >> 4) & 0xf;
+	else
+		spd = link->sata_spd;
+
+	mask = link->sata_spd_limit;
+	if (mask <= 1)
+		return -EINVAL;
+
+	/* unconditionally mask off the highest bit */
+	bit = fls(mask) - 1;
+	mask &= ~(1 << bit);
+
+	/*
+	 * Mask off all speeds higher than or equal to the current one.  At
+	 * this point, if current SPD is not available and we previously
+	 * recorded the link speed from SStatus, the driver has already
+	 * masked off the highest bit so mask should already be 1 or 0.
+	 * Otherwise, we should not force 1.5Gbps on a link where we have
+	 * not previously recorded speed from SStatus.  Just return in this
+	 * case.
+	 */
+	if (spd > 1)
+		mask &= (1 << (spd - 1)) - 1;
+	else if (link->sata_spd)
+		return -EINVAL;
+
+	/* were we already at the bottom? */
+	if (!mask)
+		return -EINVAL;
+
+	if (spd_limit) {
+		if (mask & ((1 << spd_limit) - 1))
+			mask &= (1 << spd_limit) - 1;
+		else {
+			bit = ffs(mask) - 1;
+			mask = 1 << bit;
+		}
+	}
+
+	link->sata_spd_limit = mask;
+
+	ata_link_warn(link, "limiting SATA link speed to %s\n",
+		      sata_spd_string(fls(mask)));
+
+	return 0;
+}
+
 /**
  *	sata_link_hardreset - reset link via SATA phy reset
  *	@link: link to reset
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 6abf265f626e..eda2d5dfd234 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -38,6 +38,12 @@ extern int libata_noacpi;
 extern int libata_allow_tpm;
 extern const struct device_type ata_port_type;
 extern struct ata_link *ata_dev_phys_link(struct ata_device *dev);
+
+static inline bool ata_sstatus_online(u32 sstatus)
+{
+	return (sstatus & 0xf) == 0x3;
+}
+
 #ifdef CONFIG_ATA_FORCE
 extern void ata_force_cbl(struct ata_port *ap);
 #else
@@ -65,7 +71,6 @@ extern bool ata_dev_power_init_tf(struct ata_device *dev,
 				  struct ata_taskfile *tf, bool set_active);
 extern void ata_dev_power_set_standby(struct ata_device *dev);
 extern void ata_dev_power_set_active(struct ata_device *dev);
-extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
 extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
 extern unsigned int ata_dev_set_feature(struct ata_device *dev,
 					u8 subcmd, u8 action);
@@ -87,6 +92,16 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
+/* libata-sata.c */
+#ifdef CONFIG_SATA_HOST
+int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
+#else
+static inline int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern unsigned int ata_acpi_gtf_filter;
-- 
2.46.0

Re: [PATCH 3/7] ata: libata: Move sata_down_spd_limit() to libata-sata.c

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:02PM +0900, Damien Le Moal wrote:
> Move the definition of the function sata_down_spd_limit() to
> libata-sata.c where it belongs, together with sata_set_spd().
> The helper function ata_sstatus_online() is also changed to be an
> inline function defined in drivers/ata/libata.h.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 85 ---------------------------------------
>  drivers/ata/libata-sata.c | 80 ++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata.h      | 17 +++++++-
>  3 files changed, 96 insertions(+), 86 deletions(-)
> 

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

[PATCH 4/7] ata: libata: Move sata_std_hardreset() definition to libata-sata.c

[Damien Le Moal]

Unlike ata_std_prereset() and ata_std_postreset(), the function
sata_std_hardreset() applies only to SATA devices, as its name implies.
So move its definition to libata-sata.c.

Together with this, also move the definition of sata_port_ops to
libata-sata.c, where it belongs.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b957eb900a00..b5a051bbb01f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -72,14 +72,6 @@ const struct ata_port_operations ata_base_port_ops = {
 	.end_eh			= ata_std_end_eh,
 };
 
-const struct ata_port_operations sata_port_ops = {
-	.inherits		= &ata_base_port_ops,
-
-	.qc_defer		= ata_std_qc_defer,
-	.hardreset		= sata_std_hardreset,
-};
-EXPORT_SYMBOL_GPL(sata_port_ops);
-
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
@@ -3676,33 +3668,6 @@ int ata_std_prereset(struct ata_link *link, unsigned long deadline)
 }
 EXPORT_SYMBOL_GPL(ata_std_prereset);
 
-/**
- *	sata_std_hardreset - COMRESET w/o waiting or classification
- *	@link: link to reset
- *	@class: resulting class of attached device
- *	@deadline: deadline jiffies for the operation
- *
- *	Standard SATA COMRESET w/o waiting or classification.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- *
- *	RETURNS:
- *	0 if link offline, -EAGAIN if link online, -errno on errors.
- */
-int sata_std_hardreset(struct ata_link *link, unsigned int *class,
-		       unsigned long deadline)
-{
-	const unsigned int *timing = sata_ehc_deb_timing(&link->eh_context);
-	bool online;
-	int rc;
-
-	/* do hardreset */
-	rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
-	return online ? -EAGAIN : rc;
-}
-EXPORT_SYMBOL_GPL(sata_std_hardreset);
-
 /**
  *	ata_std_postreset - standard postreset callback
  *	@link: the target ata_link
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 549377b82670..286b5699dafd 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -706,6 +706,34 @@ int sata_link_hardreset(struct ata_link *link, const unsigned int *timing,
 }
 EXPORT_SYMBOL_GPL(sata_link_hardreset);
 
+/**
+ *	sata_std_hardreset - COMRESET w/o waiting or classification
+ *	@link: link to reset
+ *	@class: resulting class of attached device
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Standard SATA COMRESET w/o waiting or classification.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 if link offline, -EAGAIN if link online, -errno on errors.
+ */
+int sata_std_hardreset(struct ata_link *link, unsigned int *class,
+		       unsigned long deadline)
+{
+	const unsigned int *timing = sata_ehc_deb_timing(&link->eh_context);
+	bool online;
+	int rc;
+
+	rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
+	if (online)
+		return -EAGAIN;
+	return rc;
+}
+EXPORT_SYMBOL_GPL(sata_std_hardreset);
+
 /**
  *	ata_qc_complete_multiple - Complete multiple qcs successfully
  *	@ap: port in question
@@ -1656,3 +1684,11 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 	ehc->i.err_mask &= ~AC_ERR_DEV;
 }
 EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
+
+const struct ata_port_operations sata_port_ops = {
+	.inherits		= &ata_base_port_ops,
+
+	.qc_defer		= ata_std_qc_defer,
+	.hardreset		= sata_std_hardreset,
+};
+EXPORT_SYMBOL_GPL(sata_port_ops);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0279c0a6302f..46e35acc611c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1104,8 +1104,6 @@ static inline bool ata_port_is_frozen(const struct ata_port *ap)
 extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
 extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
 				int (*check_ready)(struct ata_link *link));
-extern int sata_std_hardreset(struct ata_link *link, unsigned int *class,
-			      unsigned long deadline);
 extern void ata_std_postreset(struct ata_link *link, unsigned int *classes);
 
 extern struct ata_host *ata_host_alloc(struct device *dev, int n_ports);
@@ -1229,6 +1227,8 @@ extern int sata_scr_read(struct ata_link *link, int reg, u32 *val);
 extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
 extern int sata_scr_write_flush(struct ata_link *link, int reg, u32 val);
 extern int sata_set_spd(struct ata_link *link);
+int sata_std_hardreset(struct ata_link *link, unsigned int *class,
+		       unsigned long deadline);
 extern int sata_link_hardreset(struct ata_link *link,
 			const unsigned int *timing, unsigned long deadline,
 			bool *online, int (*check_ready)(struct ata_link *));
@@ -1256,6 +1256,11 @@ static inline int sata_scr_write_flush(struct ata_link *link, int reg, u32 val)
 	return -EOPNOTSUPP;
 }
 static inline int sata_set_spd(struct ata_link *link) { return -EOPNOTSUPP; }
+static inline int sata_std_hardreset(struct ata_link *link, unsigned int *class,
+				     unsigned long deadline)
+{
+	return -EOPNOTSUPP;
+}
 static inline int sata_link_hardreset(struct ata_link *link,
 				      const unsigned int *timing,
 				      unsigned long deadline,
-- 
2.46.0

Re: [PATCH 4/7] ata: libata: Move sata_std_hardreset() definition to libata-sata.c

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:03PM +0900, Damien Le Moal wrote:
> Unlike ata_std_prereset() and ata_std_postreset(), the function
> sata_std_hardreset() applies only to SATA devices, as its name implies.
> So move its definition to libata-sata.c.
> 
> Together with this, also move the definition of sata_port_ops to
> libata-sata.c, where it belongs.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

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

[PATCH 5/7] ata: libata: Rename ata_eh_read_sense_success_ncq_log()

[Damien Le Moal]

The function ata_eh_read_sense_success_ncq_log() does more that just
reading the sense data for successful NCQ commands log page as it also
sets the sense data for all commands listed in the log page.

Rename this function to ata_eh_get_ncq_success_sense() to better
describe what the function does. Furthermore, since this function is
only called from ata_eh_get_success_sense() in libata-eh.c, there is no
need to export it and its declaration can be moved to
drivers/ata/libata.h.

To be consistent with this change, the function
ata_eh_read_sense_success_non_ncq() is also renamed to
ata_eh_get_non_ncq_success_sense().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c   | 6 +++---
 drivers/ata/libata-sata.c | 7 +++----
 drivers/ata/libata.h      | 5 +++++
 include/linux/libata.h    | 5 -----
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 214b935c2ced..107aad2a1af5 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1924,7 +1924,7 @@ static inline bool ata_eh_quiet(struct ata_queued_cmd *qc)
 	return qc->flags & ATA_QCFLAG_QUIET;
 }
 
-static int ata_eh_read_sense_success_non_ncq(struct ata_link *link)
+static int ata_eh_get_non_ncq_success_sense(struct ata_link *link)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_queued_cmd *qc;
@@ -1976,9 +1976,9 @@ static void ata_eh_get_success_sense(struct ata_link *link)
 	 * request sense ext command to retrieve the sense data.
 	 */
 	if (link->sactive)
-		ret = ata_eh_read_sense_success_ncq_log(link);
+		ret = ata_eh_get_ncq_success_sense(link);
 	else
-		ret = ata_eh_read_sense_success_non_ncq(link);
+		ret = ata_eh_get_non_ncq_success_sense(link);
 	if (ret)
 		goto out;
 
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 286b5699dafd..020893da900d 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1487,8 +1487,8 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
 }
 
 /**
- *	ata_eh_read_sense_success_ncq_log - Read the sense data for successful
- *					    NCQ commands log
+ *	ata_eh_get_ncq_success_sense - Read and process the sense data for
+ *				       successful NCQ commands log page
  *	@link: ATA link to get sense data for
  *
  *	Read the sense data for successful NCQ commands log page to obtain
@@ -1501,7 +1501,7 @@ static int ata_eh_read_log_10h(struct ata_device *dev,
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
+int ata_eh_get_ncq_success_sense(struct ata_link *link)
 {
 	struct ata_device *dev = link->device;
 	struct ata_port *ap = dev->link->ap;
@@ -1573,7 +1573,6 @@ int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(ata_eh_read_sense_success_ncq_log);
 
 /**
  *	ata_eh_analyze_ncq_error - analyze NCQ error
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index eda2d5dfd234..5ca17784a350 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -95,11 +95,16 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 /* libata-sata.c */
 #ifdef CONFIG_SATA_HOST
 int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
+int ata_eh_get_ncq_success_sense(struct ata_link *link);
 #else
 static inline int sata_down_spd_limit(struct ata_link *link, u32 spd_limit)
 {
 	return -EOPNOTSUPP;
 }
+static inline int ata_eh_get_ncq_success_sense(struct ata_link *link)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 /* libata-acpi.c */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 46e35acc611c..e07a9b5d45df 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1234,7 +1234,6 @@ extern int sata_link_hardreset(struct ata_link *link,
 			bool *online, int (*check_ready)(struct ata_link *));
 extern int sata_link_resume(struct ata_link *link, const unsigned int *params,
 			    unsigned long deadline);
-extern int ata_eh_read_sense_success_ncq_log(struct ata_link *link);
 extern void ata_eh_analyze_ncq_error(struct ata_link *link);
 #else
 static inline const unsigned int *
@@ -1277,10 +1276,6 @@ static inline int sata_link_resume(struct ata_link *link,
 {
 	return -EOPNOTSUPP;
 }
-static inline int ata_eh_read_sense_success_ncq_log(struct ata_link *link)
-{
-	return -EOPNOTSUPP;
-}
 static inline void ata_eh_analyze_ncq_error(struct ata_link *link) { }
 #endif
 extern int sata_link_debounce(struct ata_link *link,
-- 
2.46.0

Re: [PATCH 5/7] ata: libata: Rename ata_eh_read_sense_success_ncq_log()

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:04PM +0900, Damien Le Moal wrote:
> The function ata_eh_read_sense_success_ncq_log() does more that just
> reading the sense data for successful NCQ commands log page as it also
> sets the sense data for all commands listed in the log page.
> 
> Rename this function to ata_eh_get_ncq_success_sense() to better
> describe what the function does. Furthermore, since this function is
> only called from ata_eh_get_success_sense() in libata-eh.c, there is no
> need to export it and its declaration can be moved to
> drivers/ata/libata.h.
> 
> To be consistent with this change, the function
> ata_eh_read_sense_success_non_ncq() is also renamed to
> ata_eh_get_non_ncq_success_sense().
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---

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

[PATCH 6/7] ata: libata: Move ncq_sense_buf to struct ata_device

[Damien Le Moal]

The ncq_sense_buf buffer field of struct ata_port is allocated and used
only for devices that support command duration limits. So move this
field out of struct ata_port and into struct ata_device together with
the CDL log buffer.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b5a051bbb01f..6a1d300dd1f5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2581,9 +2581,9 @@ static void ata_dev_config_cdl(struct ata_device *dev)
 	 * policy set to 0xD (successful completion with sense data available
 	 * bit set).
 	 */
-	if (!ap->ncq_sense_buf) {
-		ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
-		if (!ap->ncq_sense_buf)
+	if (!dev->ncq_sense_buf) {
+		dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
+		if (!dev->ncq_sense_buf)
 			goto not_supported;
 	}
 
@@ -2604,8 +2604,8 @@ static void ata_dev_config_cdl(struct ata_device *dev)
 
 not_supported:
 	dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
-	kfree(ap->ncq_sense_buf);
-	ap->ncq_sense_buf = NULL;
+	kfree(dev->ncq_sense_buf);
+	dev->ncq_sense_buf = NULL;
 }
 
 static int ata_dev_config_lba(struct ata_device *dev)
@@ -5462,7 +5462,6 @@ void ata_port_free(struct ata_port *ap)
 
 	kfree(ap->pmp_link);
 	kfree(ap->slave_link);
-	kfree(ap->ncq_sense_buf);
 	ida_free(&ata_ida, ap->print_id);
 	kfree(ap);
 }
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 020893da900d..50ea254a213d 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 {
 	struct ata_device *dev = link->device;
 	struct ata_port *ap = dev->link->ap;
-	u8 *buf = ap->ncq_sense_buf;
+	u8 *buf = dev->ncq_sense_buf;
 	struct ata_queued_cmd *qc;
 	unsigned int err_mask, tag;
 	u8 *sense, sk = 0, asc = 0, ascq = 0;
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 474816a9efa1..14f50c91ceb9 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -671,6 +671,9 @@ static int ata_tdev_match(struct attribute_container *cont,
  */
 static void ata_tdev_free(struct ata_device *dev)
 {
+	kfree(dev->ncq_sense_buf);
+	dev->ncq_sense_buf = NULL;
+
 	transport_destroy_device(&dev->tdev);
 	put_device(&dev->tdev);
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e07a9b5d45df..3fb6980c8aa1 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -762,7 +762,8 @@ struct ata_device {
 	/* Concurrent positioning ranges */
 	struct ata_cpr_log	*cpr_log;
 
-	/* Command Duration Limits log support */
+	/* Command Duration Limits support */
+	u8			*ncq_sense_buf;
 	u8			cdl[ATA_LOG_CDL_SIZE];
 
 	/* error history */
@@ -915,7 +916,6 @@ struct ata_port {
 	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
 	/* owned by EH */
-	u8			*ncq_sense_buf;
 	u8			sector_buf[ATA_SECT_SIZE] ____cacheline_aligned;
 };
 
-- 
2.46.0

Re: [PATCH 6/7] ata: libata: Move ncq_sense_buf to struct ata_device

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote:
> The ncq_sense_buf buffer field of struct ata_port is allocated and used
> only for devices that support command duration limits. So move this
> field out of struct ata_port and into struct ata_device together with
> the CDL log buffer.

The ncq_sense_buf buf is currently only allocated if the device supports CDL,
so the currently extra space that we take up in struct ata_port, for non-CDL
devices is just an empty pointer.

Additionally, sector_buf, which we use for reading all the log pages belongs
to struct ata_port, not struct ata_device.

If you also still keep sector_buf in struct ata_port, then I think that this
change makes the code more inconsistent. I would suggest to either move both
or move none. But even then I don't really see the value of moving this from
struct ata_port to ata_device.


> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c      | 11 +++++------
>  drivers/ata/libata-sata.c      |  2 +-
>  drivers/ata/libata-transport.c |  3 +++
>  include/linux/libata.h         |  4 ++--
>  4 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b5a051bbb01f..6a1d300dd1f5 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2581,9 +2581,9 @@ static void ata_dev_config_cdl(struct ata_device *dev)
>  	 * policy set to 0xD (successful completion with sense data available
>  	 * bit set).
>  	 */
> -	if (!ap->ncq_sense_buf) {
> -		ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
> -		if (!ap->ncq_sense_buf)
> +	if (!dev->ncq_sense_buf) {
> +		dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
> +		if (!dev->ncq_sense_buf)
>  			goto not_supported;
>  	}
>  
> @@ -2604,8 +2604,8 @@ static void ata_dev_config_cdl(struct ata_device *dev)
>  
>  not_supported:
>  	dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
> -	kfree(ap->ncq_sense_buf);
> -	ap->ncq_sense_buf = NULL;
> +	kfree(dev->ncq_sense_buf);
> +	dev->ncq_sense_buf = NULL;
>  }
>  
>  static int ata_dev_config_lba(struct ata_device *dev)
> @@ -5462,7 +5462,6 @@ void ata_port_free(struct ata_port *ap)
>  
>  	kfree(ap->pmp_link);
>  	kfree(ap->slave_link);
> -	kfree(ap->ncq_sense_buf);
>  	ida_free(&ata_ida, ap->print_id);
>  	kfree(ap);
>  }
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 020893da900d..50ea254a213d 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
>  {
>  	struct ata_device *dev = link->device;
>  	struct ata_port *ap = dev->link->ap;
> -	u8 *buf = ap->ncq_sense_buf;
> +	u8 *buf = dev->ncq_sense_buf;
>  	struct ata_queued_cmd *qc;
>  	unsigned int err_mask, tag;
>  	u8 *sense, sk = 0, asc = 0, ascq = 0;
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 474816a9efa1..14f50c91ceb9 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -671,6 +671,9 @@ static int ata_tdev_match(struct attribute_container *cont,
>   */
>  static void ata_tdev_free(struct ata_device *dev)
>  {
> +	kfree(dev->ncq_sense_buf);
> +	dev->ncq_sense_buf = NULL;
> +

I don't like that you are freeing ncq_sense_buf in ata_tdev_free(),
a function that is use to free the sysfs entry for a struct ata_device.

ata_tdev_add() and ata_tdev_delete() is about adding and removing the sysfs
entry for a certain struct ata_device. It is not where the ata_device is
allocated and freed, so it seems out of place to do free struct ata_device
struct members here.


ata_device is allocated when calling ata_port_alloc().
(struct ata_port has a "struct ata_link link;", and struct ata_link has a
"struct ata_device device[ATA_MAX_DEVICES]", so struct ata_device is allocated
by ata_port_alloc(), then initialized by ata_link_init(), which calls
ata_dev_init().)

If you want to free the ncq_sense_buf, then I think you should do so where we
free the ata_device, which is currently when we free the struct ata_port.
So I still think the best place to currently free this is in ata_port_free().


Considering both review comments, doesn't it make more sense to just keep
ncq_sense_buf in struct ata_port?


>  	transport_destroy_device(&dev->tdev);
>  	put_device(&dev->tdev);
>  }
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index e07a9b5d45df..3fb6980c8aa1 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -762,7 +762,8 @@ struct ata_device {
>  	/* Concurrent positioning ranges */
>  	struct ata_cpr_log	*cpr_log;
>  
> -	/* Command Duration Limits log support */
> +	/* Command Duration Limits support */
> +	u8			*ncq_sense_buf;
>  	u8			cdl[ATA_LOG_CDL_SIZE];
>  
>  	/* error history */
> @@ -915,7 +916,6 @@ struct ata_port {
>  	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
>  #endif
>  	/* owned by EH */
> -	u8			*ncq_sense_buf;
>  	u8			sector_buf[ATA_SECT_SIZE] ____cacheline_aligned;
>  };
>  
> -- 
> 2.46.0
> 

Re: [PATCH 6/7] ata: libata: Move ncq_sense_buf to struct ata_device

[Damien Le Moal]

On 8/26/24 23:48, Niklas Cassel wrote:
> On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote:
>> The ncq_sense_buf buffer field of struct ata_port is allocated and used
>> only for devices that support command duration limits. So move this
>> field out of struct ata_port and into struct ata_device together with
>> the CDL log buffer.
> 
> The ncq_sense_buf buf is currently only allocated if the device supports CDL,
> so the currently extra space that we take up in struct ata_port, for non-CDL
> devices is just an empty pointer.

No, we have cdl descriptor log page buffer (ap->cdl) which takes
ATA_LOG_CDL_SIZE (512 B). Furthermore, thinking of this some more, having that
buffer attached to the port is totally wrong if we are dealing with a pmp
attached device: we can have multiple devices supporting CDL that will all end
up overwriting that buffer. So this is actually a bug: either we move the cdl
log buffer to ata_device, or we must not probe for CDL in the case of a PMP
attached device. The latter is fine I think as CDL is really an enterprise
feature that is very unlikely to be used with consumer PMP. But the former is
more logical.

> Additionally, sector_buf, which we use for reading all the log pages belongs
> to struct ata_port, not struct ata_device.

Yes, but that buffer is only used in EH context when all devices attached to a
port (1 for most cases but more than 1 for PMP) are idle. So this is fine. This
buffer is not used at run-time.

> If you also still keep sector_buf in struct ata_port, then I think that this
> change makes the code more inconsistent. I would suggest to either move both
> or move none. But even then I don't really see the value of moving this from
> struct ata_port to ata_device.

The justification of the move is above. My commit message did not reflect this
though, so I will improve that. Also, it may make sense to squash this path with
patch 7... Will see how that looks.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 6/7] ata: libata: Move ncq_sense_buf to struct ata_device

[Niklas Cassel]

Hello Damien,

On Tue, Aug 27, 2024 at 04:50:14PM +0900, Damien Le Moal wrote:
> On 8/26/24 23:48, Niklas Cassel wrote:
> > On Mon, Aug 26, 2024 at 04:31:05PM +0900, Damien Le Moal wrote:
> >> The ncq_sense_buf buffer field of struct ata_port is allocated and used
> >> only for devices that support command duration limits. So move this
> >> field out of struct ata_port and into struct ata_device together with
> >> the CDL log buffer.
> > 
> > The ncq_sense_buf buf is currently only allocated if the device supports CDL,
> > so the currently extra space that we take up in struct ata_port, for non-CDL
> > devices is just an empty pointer.
> 
> No, we have cdl descriptor log page buffer (ap->cdl) which takes
> ATA_LOG_CDL_SIZE (512 B). Furthermore, thinking of this some more, having that
> buffer attached to the port is totally wrong if we are dealing with a pmp
> attached device: we can have multiple devices supporting CDL that will all end
> up overwriting that buffer. So this is actually a bug: either we move the cdl
> log buffer to ata_device, or we must not probe for CDL in the case of a PMP
> attached device. The latter is fine I think as CDL is really an enterprise
> feature that is very unlikely to be used with consumer PMP. But the former is
> more logical.

I think you are mixing things up, we don't have any ap->cdl.

We have:
1) ap->ncq_sense_buf	(u8 *ncq_sense_buf)
2) dev->cdl		(u8 cdl[ATA_LOG_CDL_SIZE])
3) ap->sector_buf	(u8 sector_buf[ATA_SECT_SIZE])

I do not think there is any bug in having ap->ncq_sense_buf in struct ata_port,
like the code currently does, because like you say, we currently only fetch the
Successful NCQ Commands log from EH context, so this should be safe even when
using PMPs.

For dev->cdl, it is currently in struct ata_device, which is correct, because
as you say, caching the CDL descriptor log page in the ata_port would not be
correct, since it could then get overwritten by another device if using a PMP.
(Changing this to be dynamically allocated would be a nice optimization.)

For ap->sector_buf, I suggested to perhaps move this to ata_device, such that
the ata_read_log() functions (which already take a ata_device as first
argument) do no longer need to take a "buffer" argument, the buffer could be
derived from the ata_device if we move the buffer there.

If we move ap->sector_buf to struct ata_device, then I think that it makes
sense to also move ap->ncq_sense_buf to struct ata_device.


Kind regards,
Niklas

[PATCH 7/7] ata: libata: Improve CDL resource management

[Damien Le Moal]

The command duration limits (CDL) log buffer of struct ata_device is
needed only if a device actually supports CDL. The same applies to the
ncq_sense_log buffer.

Group these 2 buffers into a new structure ata_cdl defining both buffers
as embedded buffers (no allocation needed) and allocate this structure
from ata_dev_config_cdl() only for devices that support CDL.

The functions ata_dev_init_cdl_resources() and
ata_dev_cleanup_cdl_resources() are defined to manage this new structure
allocation, initialization and cleanup when a device is removed.
ata_dev_cleanup_cdl_resources() is called from ata_tdev_free().

Note that the cdl log buffer name is changed to desc_log_buf to make it
clearer what it is.

This change reduces the size of struct ata_device and reduces memory
usage for ATA devices that do not support CDL.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c      | 56 +++++++++++++++++++++-------------
 drivers/ata/libata-sata.c      |  2 +-
 drivers/ata/libata-scsi.c      |  2 +-
 drivers/ata/libata-transport.c |  4 +--
 drivers/ata/libata.h           |  1 +
 include/linux/libata.h         | 18 +++++++++--
 6 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6a1d300dd1f5..bcee96e29b34 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2475,12 +2475,41 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 		dev->flags |= ATA_DFLAG_TRUSTED;
 }
 
+static int ata_dev_init_cdl_resources(struct ata_device *dev)
+{
+	struct ata_cdl *cdl = dev->cdl;
+	unsigned int err_mask;
+
+	if (!cdl) {
+		cdl = kzalloc(sizeof(struct ata_cdl), GFP_KERNEL);
+		if (!cdl)
+			return -ENOMEM;
+		dev->cdl = cdl;
+	}
+
+	err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, cdl->desc_log_buf,
+				     ATA_LOG_CDL_SIZE / ATA_SECT_SIZE);
+	if (err_mask) {
+		ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+void ata_dev_cleanup_cdl_resources(struct ata_device *dev)
+{
+	kfree(dev->cdl);
+	dev->cdl = NULL;
+}
+
 static void ata_dev_config_cdl(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->link->ap;
 	unsigned int err_mask;
 	bool cdl_enabled;
 	u64 val;
+	int ret;
 
 	if (ata_id_major_version(dev->id) < 11)
 		goto not_supported;
@@ -2575,37 +2604,20 @@ static void ata_dev_config_cdl(struct ata_device *dev)
 		}
 	}
 
-	/*
-	 * Allocate a buffer to handle reading the sense data for successful
-	 * NCQ Commands log page for commands using a CDL with one of the limit
-	 * policy set to 0xD (successful completion with sense data available
-	 * bit set).
-	 */
-	if (!dev->ncq_sense_buf) {
-		dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
-		if (!dev->ncq_sense_buf)
-			goto not_supported;
-	}
-
-	/*
-	 * Command duration limits is supported: cache the CDL log page 18h
-	 * (command duration descriptors).
-	 */
-	err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, ap->sector_buf, 1);
-	if (err_mask) {
-		ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
+	/* CDL is supported: allocate and initialize needed resources. */
+	ret = ata_dev_init_cdl_resources(dev);
+	if (ret) {
+		ata_dev_warn(dev, "Initialize CDL resources failed\n");
 		goto not_supported;
 	}
 
-	memcpy(dev->cdl, ap->sector_buf, ATA_LOG_CDL_SIZE);
 	dev->flags |= ATA_DFLAG_CDL;
 
 	return;
 
 not_supported:
 	dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
-	kfree(dev->ncq_sense_buf);
-	dev->ncq_sense_buf = NULL;
+	ata_dev_cleanup_cdl_resources(dev);
 }
 
 static int ata_dev_config_lba(struct ata_device *dev)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 50ea254a213d..e05fb09af061 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
 {
 	struct ata_device *dev = link->device;
 	struct ata_port *ap = dev->link->ap;
-	u8 *buf = dev->ncq_sense_buf;
+	u8 *buf = dev->cdl->ncq_sense_log_buf;
 	struct ata_queued_cmd *qc;
 	unsigned int err_mask, tag;
 	u8 *sense, sk = 0, asc = 0, ascq = 0;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a3ffce4b218d..7fed924d6561 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2259,7 +2259,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)
 static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf,
 					     u8 spg)
 {
-	u8 *b, *cdl = dev->cdl, *desc;
+	u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;
 	u32 policy;
 	int i;
 
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 14f50c91ceb9..add230c0d51e 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -671,9 +671,7 @@ static int ata_tdev_match(struct attribute_container *cont,
  */
 static void ata_tdev_free(struct ata_device *dev)
 {
-	kfree(dev->ncq_sense_buf);
-	dev->ncq_sense_buf = NULL;
-
+	ata_dev_cleanup_cdl_resources(dev);
 	transport_destroy_device(&dev->tdev);
 	put_device(&dev->tdev);
 }
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5ca17784a350..df11f923e1a2 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -89,6 +89,7 @@ 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)
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3fb6980c8aa1..37a5509adc77 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -700,6 +700,21 @@ struct ata_cpr_log {
 	struct ata_cpr		cpr[] __counted_by(nr_cpr);
 };
 
+struct ata_cdl {
+	/*
+	 * Buffer to cache the CDL log page 18h (command duration descriptors)
+	 * for SCSI-ATA translation.
+	 */
+	u8			desc_log_buf[ATA_LOG_CDL_SIZE];
+
+	/*
+	 * Buffer to handle reading the sense data for successful NCQ Commands
+	 * log page for commands using a CDL with one of the limits policy set
+	 * to 0xD (successful completion with sense data available bit set).
+	 */
+	u8			ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE];
+};
+
 struct ata_device {
 	struct ata_link		*link;
 	unsigned int		devno;		/* 0 or 1 */
@@ -763,8 +778,7 @@ struct ata_device {
 	struct ata_cpr_log	*cpr_log;
 
 	/* Command Duration Limits support */
-	u8			*ncq_sense_buf;
-	u8			cdl[ATA_LOG_CDL_SIZE];
+	struct ata_cdl		*cdl;
 
 	/* error history */
 	int			spdn_cnt;
-- 
2.46.0

Re: [PATCH 7/7] ata: libata: Improve CDL resource management

[Niklas Cassel]

On Mon, Aug 26, 2024 at 04:31:06PM +0900, Damien Le Moal wrote:
> The command duration limits (CDL) log buffer of struct ata_device is
> needed only if a device actually supports CDL. The same applies to the
> ncq_sense_log buffer.
> 
> Group these 2 buffers into a new structure ata_cdl defining both buffers
> as embedded buffers (no allocation needed) and allocate this structure
> from ata_dev_config_cdl() only for devices that support CDL.
> 
> The functions ata_dev_init_cdl_resources() and
> ata_dev_cleanup_cdl_resources() are defined to manage this new structure
> allocation, initialization and cleanup when a device is removed.
> ata_dev_cleanup_cdl_resources() is called from ata_tdev_free().
> 
> Note that the cdl log buffer name is changed to desc_log_buf to make it
> clearer what it is.
> 
> This change reduces the size of struct ata_device and reduces memory
> usage for ATA devices that do not support CDL.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Like previous comment, as long as sector_buf belongs to struct ata_port,
it seems a bit weird to keep this in struct ata_device.

Perhaps we can move sector_buf to struct ata_device?
(and modify the ata_read_log() functions to not take a buffer,
as the buffer would not be in the struct ata_device, which we already
supply to the ata_read_log() functions as the first argument.)


If we do that, then I think it is okay to keep a struct ata_cdl
in struct ata_device. Although I still don't like cleaning this
up in ata_tdev_() functions.

Perhaps we could call ata_device_cleanup()/ata_device_free_resource()
from ata_port_free() ?

ata_tport_release() will call ata_host_put().
ata_host_put() calls kref_put(&host->kref, ata_host_release);
ata_host_release() calls ata_port_free().

But ata_tport_add() is simply adding the sysfs entry IMO.

The sysfs entry takes a ata_host reference.
Once the refcount is zero, it will be freed.

Which is why I think that it makes more sense to clean this up in in
ata_port_free() (at least with the current design where the fixed size
ata_device array is part of struct ata_port (via struct ata_link)).


Perhaps something like:

void ata_port_free(struct ata_port *ap)
{
	....

	 ata_for_each_link (link, ap, ...) {
	 	ata_for_each_dev (dev, link, ...) {
			ata_device_cleanup(dev);
		}
	}
}


Kind regards,
Niklas


> ---
>  drivers/ata/libata-core.c      | 56 +++++++++++++++++++++-------------
>  drivers/ata/libata-sata.c      |  2 +-
>  drivers/ata/libata-scsi.c      |  2 +-
>  drivers/ata/libata-transport.c |  4 +--
>  drivers/ata/libata.h           |  1 +
>  include/linux/libata.h         | 18 +++++++++--
>  6 files changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 6a1d300dd1f5..bcee96e29b34 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2475,12 +2475,41 @@ static void ata_dev_config_trusted(struct ata_device *dev)
>  		dev->flags |= ATA_DFLAG_TRUSTED;
>  }
>  
> +static int ata_dev_init_cdl_resources(struct ata_device *dev)
> +{
> +	struct ata_cdl *cdl = dev->cdl;
> +	unsigned int err_mask;
> +
> +	if (!cdl) {
> +		cdl = kzalloc(sizeof(struct ata_cdl), GFP_KERNEL);
> +		if (!cdl)
> +			return -ENOMEM;
> +		dev->cdl = cdl;
> +	}
> +
> +	err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, cdl->desc_log_buf,
> +				     ATA_LOG_CDL_SIZE / ATA_SECT_SIZE);
> +	if (err_mask) {
> +		ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +void ata_dev_cleanup_cdl_resources(struct ata_device *dev)
> +{
> +	kfree(dev->cdl);
> +	dev->cdl = NULL;
> +}
> +
>  static void ata_dev_config_cdl(struct ata_device *dev)
>  {
>  	struct ata_port *ap = dev->link->ap;
>  	unsigned int err_mask;
>  	bool cdl_enabled;
>  	u64 val;
> +	int ret;
>  
>  	if (ata_id_major_version(dev->id) < 11)
>  		goto not_supported;
> @@ -2575,37 +2604,20 @@ static void ata_dev_config_cdl(struct ata_device *dev)
>  		}
>  	}
>  
> -	/*
> -	 * Allocate a buffer to handle reading the sense data for successful
> -	 * NCQ Commands log page for commands using a CDL with one of the limit
> -	 * policy set to 0xD (successful completion with sense data available
> -	 * bit set).
> -	 */
> -	if (!dev->ncq_sense_buf) {
> -		dev->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
> -		if (!dev->ncq_sense_buf)
> -			goto not_supported;
> -	}
> -
> -	/*
> -	 * Command duration limits is supported: cache the CDL log page 18h
> -	 * (command duration descriptors).
> -	 */
> -	err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, ap->sector_buf, 1);
> -	if (err_mask) {
> -		ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
> +	/* CDL is supported: allocate and initialize needed resources. */
> +	ret = ata_dev_init_cdl_resources(dev);
> +	if (ret) {
> +		ata_dev_warn(dev, "Initialize CDL resources failed\n");
>  		goto not_supported;
>  	}
>  
> -	memcpy(dev->cdl, ap->sector_buf, ATA_LOG_CDL_SIZE);
>  	dev->flags |= ATA_DFLAG_CDL;
>  
>  	return;
>  
>  not_supported:
>  	dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
> -	kfree(dev->ncq_sense_buf);
> -	dev->ncq_sense_buf = NULL;
> +	ata_dev_cleanup_cdl_resources(dev);
>  }
>  
>  static int ata_dev_config_lba(struct ata_device *dev)
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 50ea254a213d..e05fb09af061 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link)
>  {
>  	struct ata_device *dev = link->device;
>  	struct ata_port *ap = dev->link->ap;
> -	u8 *buf = dev->ncq_sense_buf;
> +	u8 *buf = dev->cdl->ncq_sense_log_buf;
>  	struct ata_queued_cmd *qc;
>  	unsigned int err_mask, tag;
>  	u8 *sense, sk = 0, asc = 0, ascq = 0;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a3ffce4b218d..7fed924d6561 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2259,7 +2259,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)
>  static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf,
>  					     u8 spg)
>  {
> -	u8 *b, *cdl = dev->cdl, *desc;
> +	u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;
>  	u32 policy;
>  	int i;
>  
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 14f50c91ceb9..add230c0d51e 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -671,9 +671,7 @@ static int ata_tdev_match(struct attribute_container *cont,
>   */
>  static void ata_tdev_free(struct ata_device *dev)
>  {
> -	kfree(dev->ncq_sense_buf);
> -	dev->ncq_sense_buf = NULL;
> -
> +	ata_dev_cleanup_cdl_resources(dev);
>  	transport_destroy_device(&dev->tdev);
>  	put_device(&dev->tdev);
>  }
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 5ca17784a350..df11f923e1a2 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -89,6 +89,7 @@ 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)
>  
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 3fb6980c8aa1..37a5509adc77 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -700,6 +700,21 @@ struct ata_cpr_log {
>  	struct ata_cpr		cpr[] __counted_by(nr_cpr);
>  };
>  
> +struct ata_cdl {
> +	/*
> +	 * Buffer to cache the CDL log page 18h (command duration descriptors)
> +	 * for SCSI-ATA translation.
> +	 */
> +	u8			desc_log_buf[ATA_LOG_CDL_SIZE];
> +
> +	/*
> +	 * Buffer to handle reading the sense data for successful NCQ Commands
> +	 * log page for commands using a CDL with one of the limits policy set
> +	 * to 0xD (successful completion with sense data available bit set).
> +	 */
> +	u8			ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE];
> +};
> +
>  struct ata_device {
>  	struct ata_link		*link;
>  	unsigned int		devno;		/* 0 or 1 */
> @@ -763,8 +778,7 @@ struct ata_device {
>  	struct ata_cpr_log	*cpr_log;
>  
>  	/* Command Duration Limits support */
> -	u8			*ncq_sense_buf;
> -	u8			cdl[ATA_LOG_CDL_SIZE];
> +	struct ata_cdl		*cdl;
>  
>  	/* error history */
>  	int			spdn_cnt;
> -- 
> 2.46.0
> 

Re: [PATCH 7/7] ata: libata: Improve CDL resource management

[Damien Le Moal]

On 8/27/24 00:37, Niklas Cassel wrote:
> On Mon, Aug 26, 2024 at 04:31:06PM +0900, Damien Le Moal wrote:
>> The command duration limits (CDL) log buffer of struct ata_device is
>> needed only if a device actually supports CDL. The same applies to the
>> ncq_sense_log buffer.
>>
>> Group these 2 buffers into a new structure ata_cdl defining both buffers
>> as embedded buffers (no allocation needed) and allocate this structure
>> from ata_dev_config_cdl() only for devices that support CDL.
>>
>> The functions ata_dev_init_cdl_resources() and
>> ata_dev_cleanup_cdl_resources() are defined to manage this new structure
>> allocation, initialization and cleanup when a device is removed.
>> ata_dev_cleanup_cdl_resources() is called from ata_tdev_free().
>>
>> Note that the cdl log buffer name is changed to desc_log_buf to make it
>> clearer what it is.
>>
>> This change reduces the size of struct ata_device and reduces memory
>> usage for ATA devices that do not support CDL.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Like previous comment, as long as sector_buf belongs to struct ata_port,
> it seems a bit weird to keep this in struct ata_device.
> 
> Perhaps we can move sector_buf to struct ata_device?
> (and modify the ata_read_log() functions to not take a buffer,
> as the buffer would not be in the struct ata_device, which we already
> supply to the ata_read_log() functions as the first argument.)
> 
> 
> If we do that, then I think it is okay to keep a struct ata_cdl
> in struct ata_device. Although I still don't like cleaning this
> up in ata_tdev_() functions.

OK. Let me rework this.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/7] Code cleanup and CDL memory usage reduction

[Niklas Cassel]

Hello Damien,

On Mon, Aug 26, 2024 at 04:30:59PM +0900, Damien Le Moal wrote:
> This patch series starts by moving code that is SATA specific from
> libata-core.c to libata-sata.c, without any functional change. The
> benefit is a smaller ata code for hosts that do not support SATA. This
> is done in patch 1 to 4.
> 
> The second part of the series (patch 5 to 7) cleanup the CDL support
> code by moving device resources from struct ata_port to struct ata_dev
> and reduce the size of struct ata_dev by allocating buffers needed for
> CDL only for drives that actually support this feature.

I think that you should reword "... by allocating buffers needed for
CDL only for drives that actually support this feature".

When reading this sentence, I read it as a claim that the current code always
allocates buffers needed for CDL, even for drives that do not support CDL.
However, that is not true.

ata_dev_config_cdl() currently allocates ap->ncq_sense_buf, after, and only
after, checking the Command Duration Limit Supported bits. If these bits are
not set, we "goto not_supported;" before ever allocating ap->ncq_sense_buf.
So we are currently not allocating the ncq_sense_buf buffer for drives that
do not support CDL.


Kind regards,
Niklas

> 
> Damien Le Moal (7):
>   ata: libata: Fix ata_tdev_free() kdoc comment
>   ata: libata: Improve __ata_qc_complete()
>   ata: libata: Move sata_down_spd_limit() to libata-sata.c
>   ata: libata: Move sata_std_hardreset() definition to libata-sata.c
>   ata: libata: Rename ata_eh_read_sense_success_ncq_log()
>   ata: libata: Move ncq_sense_buf to struct ata_device
>   ata: libata: Improve CDL resource management
> 
>  drivers/ata/libata-core.c      | 189 +++++++--------------------------
>  drivers/ata/libata-eh.c        |   6 +-
>  drivers/ata/libata-sata.c      | 125 +++++++++++++++++++++-
>  drivers/ata/libata-scsi.c      |   2 +-
>  drivers/ata/libata-transport.c |  11 +-
>  drivers/ata/libata.h           |  23 +++-
>  include/linux/libata.h         |  34 ++++--
>  7 files changed, 217 insertions(+), 173 deletions(-)
> 
> -- 
> 2.46.0
>