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

[Niklas Cassel <cassel@kernel.org>]

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
>