Re: [PATCH 1/3] ata: libata-core: Cache the general purpose log directory

[Damien Le Moal <dlemoal@kernel.org>]

On 7/3/25 6:49 PM, Niklas Cassel wrote:
>> +static int ata_read_log_directory(struct ata_device *dev)
>> +{
>> +	u16 version;
>> +
>> +	/* If the log page is already cached, do nothing. */
>> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
>> +	if (version == 0x0001)
>> +		return 0;
>> +
>> +	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, dev->gp_log_dir, 1)) {
>> +		ata_clear_log_directory(dev);
> 
> Why do we need to clear the log here?
> 
> If we had something cached, we would have returned in the if-statement above
> already.
> 
> And if the read failed, wouldn't the buffer be unmodified?
> 
> Since we are only reading a single sector, which is the smallest unit we
> can read, I would expect the buffer to be unmodified on failure.
> 
> Is that an incorrect assumption?

I do not think it is needed either. It is a little bit of paranoia, e.g. in
case we lost the link in the middle of transfers and have a half-full buffer.

> 
> 
>> +		return -EIO;
>> +	}
>> +
>> +	version = get_unaligned_le16(&dev->gp_log_dir[0]);
>> +	if (version != 0x0001) {
>> +		ata_dev_err(dev, "Invalid log directory version 0x%04x\n",
>> +			    version);
>> +		return -EINVAL;
> 
> Don't you want to call ata_clear_log_directory() here?

Yep. Forgot to add it. And I think we should also set ATA_QUIRK_NO_LOG_DIR if
we ever get this error because it will be pointless to retry.

>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 7462218312ad..78a4addc6659 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -761,6 +761,9 @@ struct ata_device {
>>  		u32		gscr[SATA_PMP_GSCR_DWORDS]; /* PMP GSCR block */
>>  	} ____cacheline_aligned;
>>  
>> +	/* General Purpose Log Directory log page */
>> +	u8			gp_log_dir[ATA_SECT_SIZE] ____cacheline_aligned;
> 
> Why align this to a cacheline?
> 
> It shouldn't be needed for get_unaligned_le16() to work correctly.
> 
> Is it just to increase the chance of this being in the cache?
> 
> If so, do we really access this that often that it needs to be
> cacheline aligned? (We mostly access it during probe time, no?)

It is used as the buffer for the read log command, so similar to the
sector_buf, I made it cacheline aligned. Better for DMA I guess.


-- 
Damien Le Moal
Western Digital Research