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