ata: libata: Improve CDL resource management

The ncq_sense_buf buffer field of struct ata_port is allocated and used
only for devices that support the Command Duration Limits (CDL) feature.
However, the cdl buffer of struct ata_device, which is used to cache the
command duration limits log page for devices supporting CDL is always
allocated as part of struct ata_device, which is wasteful of memory for
devices that do not support this feature.

Clean this up by defining both buffers as part of the new ata_cdl
structure and allocating this structure only for devices that support
the CDL feature. This new structure is attached to struct ata_device
using the cdl pointer.

The functions ata_dev_init_cdl_resources() and
ata_dev_cleanup_cdl_resources() are defined to manage this new structure
allocation, initialization and freeing when a port is removed or a
device disabled. ata_dev_init_cdl_resources() is called from
ata_dev_config_cdl() only for devices that support CDL.
ata_dev_cleanup_cdl_resources() is called from ata_dev_free_resources()
to free the ata_cdl structure when a device is being disabled by EH.

Note that the name of the former cdl log buffer of struct ata_device is
changed to desc_log_buf to make it clearer that it is a buffer for the
limit descriptors log page.

This change reduces the size of struct ata_device, thus reducing memory
usage for ATA devices that do not support the CDL feature.

Signed-off-by: Damien Le Moal <[email protected]>
Reviewed-by: Niklas Cassel <[email protected]>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bfd452b0d46d02a911ec28374a065a1196894b83..082179c38e1b74b3e750260d2f4596ff52e529b7 100644 (file)
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2464,12 +2464,41 @@ 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)
+{
+       kfree(dev->cdl);
+       dev->cdl = NULL;
+}
+
+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(*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");
+               ata_dev_cleanup_cdl_resources(dev);
+               return -EIO;
+       }
+
+       return 0;
+}
+
 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;
@@ -2564,37 +2593,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 (!ap->ncq_sense_buf) {
-               ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
-               if (!ap->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, dev->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, dev->sector_buf, ATA_LOG_CDL_SIZE);
        dev->flags |= ATA_DFLAG_CDL;
 
        return;
 
 not_supported:
        dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
-       kfree(ap->ncq_sense_buf);
-       ap->ncq_sense_buf = NULL;
+       ata_dev_cleanup_cdl_resources(dev);
 }
 
 static int ata_dev_config_lba(struct ata_device *dev)
@@ -5451,7 +5463,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);
 }
@@ -5989,6 +6000,8 @@ void ata_dev_free_resources(struct ata_device *dev)
 {
        if (zpodd_dev_enabled(dev))
                zpodd_exit(dev);
+
+       ata_dev_cleanup_cdl_resources(dev);
 }
 
 /**

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 498430db86f73a04c72b6857ec2ad5d3fb0a02f8..c8b119a06bb23dc1d7f4f9bd92d3b5481530dc42 100644 (file)
--- 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->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 4fb45565c142d4fbb125b56a6fbe224817500672..9f75d26808bfd2dc2e333023d23f99e3ce83de5b 100644 (file)
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2248,7 +2248,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.h b/drivers/ata/libata.h
index 927d77bde7ef67fad5a7c5625b815141898d96e2..0337be4faec7c4c0e480fff45aaee62d1a503e4d 100644 (file)
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -90,6 +90,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 aac38dcd22307765615e8ea28b8d1b320086f017..9b4a6ff03235bcf1bb26d188799a6fde10a119a1 100644 (file)
--- 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 */
@@ -762,8 +777,8 @@ struct ata_device {
        /* Concurrent positioning ranges */
        struct ata_cpr_log      *cpr_log;
 
-       /* Command Duration Limits log support */
-       u8                      cdl[ATA_LOG_CDL_SIZE];
+       /* Command Duration Limits support */
+       struct ata_cdl          *cdl;
 
        /* error history */
        int                     spdn_cnt;
@@ -917,8 +932,6 @@ struct ata_port {
 #ifdef CONFIG_ATA_ACPI
        struct ata_acpi_gtm     __acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
-       /* owned by EH */
-       u8                      *ncq_sense_buf;
 };
 
 /* The following initializer overrides a method to NULL whether one of