On 9/6/24 17:51, Niklas Cassel wrote:
>> +static int ata_dev_init_cdl_resources(struct ata_device *dev)
>> +{
>> + struct ata_cdl *cdl = dev->cdl;
>> + unsigned int err_mask;
>> +
>> + if (!cdl) {
>> + cdl = kzalloc(sizeof(*cdl), GFP_KERNEL);
>> + if (!cdl)
>> + return -ENOMEM;
>> + dev->cdl = cdl;
>> + }
>> +
>> + err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, cdl->desc_log_buf,
>> + ATA_LOG_CDL_SIZE / ATA_SECT_SIZE);
>> + if (err_mask) {
>> + ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
>> + return -EIO;
>
> Usually when a function return error, you expect it to have freed the
> resources that it might have allocated, so perhaps free and set
> dev->cdl to NULL here.
Indeed. I added a call to ata_dev_cleanup_cdl_resources().
>> @@ -2564,37 +2592,20 @@ static void ata_dev_config_cdl(struct ata_device *dev)
>> }
>> }
>>
>> - /*
>> - * Allocate a buffer to handle reading the sense data for successful
>> - * NCQ Commands log page for commands using a CDL with one of the limit
>> - * policy set to 0xD (successful completion with sense data available
>> - * bit set).
>> - */
>> - if (!ap->ncq_sense_buf) {
>> - ap->ncq_sense_buf = kmalloc(ATA_LOG_SENSE_NCQ_SIZE, GFP_KERNEL);
>> - if (!ap->ncq_sense_buf)
>> - goto not_supported;
>> - }
>> -
>> - /*
>> - * Command duration limits is supported: cache the CDL log page 18h
>> - * (command duration descriptors).
>> - */
>> - err_mask = ata_read_log_page(dev, ATA_LOG_CDL, 0, dev->sector_buf, 1);
>> - if (err_mask) {
>> - ata_dev_warn(dev, "Read Command Duration Limits log failed\n");
>> + /* CDL is supported: allocate and initialize needed resources. */
>> + ret = ata_dev_init_cdl_resources(dev);
>> + if (ret) {
>> + ata_dev_warn(dev, "Initialize CDL resources failed\n");
>> goto not_supported;
>> }
>>
>> - memcpy(dev->cdl, dev->sector_buf, ATA_LOG_CDL_SIZE);
>> dev->flags |= ATA_DFLAG_CDL;
>>
>> return;
>>
>> not_supported:
>> dev->flags &= ~(ATA_DFLAG_CDL | ATA_DFLAG_CDL_ENABLED);
>> - kfree(ap->ncq_sense_buf);
>> - ap->ncq_sense_buf = NULL;
>> + ata_dev_cleanup_cdl_resources(dev);
>
> Considering that you now do ata_dev_init_cdl_resources() at the end,
> if you implement my suggestion above, we can remove the call to
> ata_dev_cleanup_cdl_resources() here, since we will only jump here
> if the ata_dev_init_cdl_resources() call failed.
Nope. I think we need to keep that call because this function is going to be
called again for device revalidation, with the CDL resource already allocated.
So if we hit an error before reaching ata_dev_init_cdl_resources(), we should
free the resources. So I prefer to keep it.
> Either way:
> Reviewed-by: Niklas Cassel <cassel@xxxxxxxxxx>
Thanks.
--
Damien Le Moal
Western Digital Research
