Re: [PATCH 2/2] ata: libata-core: Improve ata_dev_power_set_active()

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

On 10/14/23 00:14, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> writes:
>>  /**
>>   *	ata_dev_power_set_active -  Set a device power mode to active
>>   *	@dev: target device
>> @@ -2065,6 +2092,13 @@ void ata_dev_power_set_active(struct ata_device *dev)
>>  	if (!ata_dev_power_init_tf(dev, &tf, true))
>>  		return;
>>  
>> +	/*
>> +	 * Check the device power state & condition and force a spinup with
>> +	 * VERIFY command only if the drive is not already ACTIVE or IDLE.
>> +	 */
>> +	if (ata_dev_power_is_active(dev))
>> +		return;
>> +
>>  	ata_dev_notice(dev, "Entering active power mode\n");
>>  
>>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
> 
> This bit didn't apply cleanly to what I just pulled from Linus.  It

The patches are for 6.7 so they are based on for-6.7 branch (for-next is the same).

> seems there are soem differences in how the tf is set up.  Why not move
> this check to before the tf is set up?  There isn't much point in
> setting it up if it isn't going to be used.

ata_dev_power_init_tf() also checks for the device type and horkage flags and
return false if the operation is not necessary or desired. I left these checks
in that function instead of moving them to different places. That is easier to
maintain this way. This is not the hot path, so I really prefer prioritizing
simplicity here.

-- 
Damien Le Moal
Western Digital Research