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
