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. > Additionally, sector_buf, which we use for reading all the log pages belongs > to struct ata_port, not struct ata_device. Yes, but that buffer is only used in EH context when all devices attached to a port (1 for most cases but more than 1 for PMP) are idle. So this is fine. This buffer is not used at run-time. > 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. The justification of the move is above. My commit message did not reflect this though, so I will improve that. Also, it may make sense to squash this path with patch 7... Will see how that looks. -- Damien Le Moal Western Digital Research
