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