Hello Damien, On Tue, Aug 27, 2024 at 04:50:14PM +0900, Damien Le Moal wrote: > On 8/26/24 23:48, Niklas Cassel wrote: > > 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. > > No, we have cdl descriptor log page buffer (ap->cdl) which takes > ATA_LOG_CDL_SIZE (512 B). Furthermore, thinking of this some more, having that > buffer attached to the port is totally wrong if we are dealing with a pmp > attached device: we can have multiple devices supporting CDL that will all end > up overwriting that buffer. So this is actually a bug: either we move the cdl > log buffer to ata_device, or we must not probe for CDL in the case of a PMP > attached device. The latter is fine I think as CDL is really an enterprise > feature that is very unlikely to be used with consumer PMP. But the former is > more logical. I think you are mixing things up, we don't have any ap->cdl. We have: 1) ap->ncq_sense_buf (u8 *ncq_sense_buf) 2) dev->cdl (u8 cdl[ATA_LOG_CDL_SIZE]) 3) ap->sector_buf (u8 sector_buf[ATA_SECT_SIZE]) I do not think there is any bug in having ap->ncq_sense_buf in struct ata_port, like the code currently does, because like you say, we currently only fetch the Successful NCQ Commands log from EH context, so this should be safe even when using PMPs. For dev->cdl, it is currently in struct ata_device, which is correct, because as you say, caching the CDL descriptor log page in the ata_port would not be correct, since it could then get overwritten by another device if using a PMP. (Changing this to be dynamically allocated would be a nice optimization.) For ap->sector_buf, I suggested to perhaps move this to ata_device, such that the ata_read_log() functions (which already take a ata_device as first argument) do no longer need to take a "buffer" argument, the buffer could be derived from the ata_device if we move the buffer there. If we move ap->sector_buf to struct ata_device, then I think that it makes sense to also move ap->ncq_sense_buf to struct ata_device. Kind regards, Niklas
