| Message ID | 20240906015847.229539-10-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Code cleanup and memory usage reduction | expand |
On Fri, Sep 06, 2024 at 10:58:47AM +0900, Damien Le Moal wrote: > 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 <dlemoal@kernel.org> > --- > drivers/ata/libata-core.c | 60 +++++++++++++++++++++++---------------- > drivers/ata/libata-sata.c | 2 +- > drivers/ata/libata-scsi.c | 2 +- > drivers/ata/libata.h | 1 + > include/linux/libata.h | 21 +++++++++++--- > 5 files changed, 56 insertions(+), 30 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index bfd452b0d46d..bd2f8e442b14 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -2464,12 +2464,40 @@ 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(*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; Usually when a function return error, you expect it to have freed the resources that it might have allocated, so perhaps free and set dev->cdl to NULL here. > + } > + > + 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; > @@ -2564,37 +2592,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); Considering that you now do ata_dev_init_cdl_resources() at the end, if you implement my suggestion above, we can remove the call to ata_dev_cleanup_cdl_resources() here, since we will only jump here if the ata_dev_init_cdl_resources() call failed. Either way: Reviewed-by: Niklas Cassel <cassel@kernel.org> > } > > static int ata_dev_config_lba(struct ata_device *dev)
On 9/6/24 17:51, Niklas Cassel wrote: >> +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"); >> + return -EIO; > > Usually when a function return error, you expect it to have freed the > resources that it might have allocated, so perhaps free and set > dev->cdl to NULL here. Indeed. I added a call to ata_dev_cleanup_cdl_resources(). >> @@ -2564,37 +2592,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); > > Considering that you now do ata_dev_init_cdl_resources() at the end, > if you implement my suggestion above, we can remove the call to > ata_dev_cleanup_cdl_resources() here, since we will only jump here > if the ata_dev_init_cdl_resources() call failed. Nope. I think we need to keep that call because this function is going to be called again for device revalidation, with the CDL resource already allocated. So if we hit an error before reaching ata_dev_init_cdl_resources(), we should free the resources. So I prefer to keep it. > Either way: > Reviewed-by: Niklas Cassel <cassel@kernel.org> Thanks.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index bfd452b0d46d..bd2f8e442b14 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2464,12 +2464,40 @@ 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(*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; @@ -2564,37 +2592,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 +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); } @@ -5989,6 +5999,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 498430db86f7..c8b119a06bb2 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->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 a42726910fb8..d0cf30c23c71 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.h b/drivers/ata/libata.h index 927d77bde7ef..0337be4faec7 100644 --- 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 aac38dcd2230..9b4a6ff03235 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 */ @@ -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
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 <dlemoal@kernel.org> --- drivers/ata/libata-core.c | 60 +++++++++++++++++++++++---------------- drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 +++++++++++--- 5 files changed, 56 insertions(+), 30 deletions(-)