Re: [PATCH v2 4/9] libata: cleanup ata_dev_configure()

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/08/06 18:28, Hannes Reinecke wrote:
> On 8/6/21 11:12 AM, Damien Le Moal wrote:
>> On 2021/08/06 18:07, Hannes Reinecke wrote:
>>> On 8/6/21 9:42 AM, Damien Le Moal wrote:
>>>> Introduce the helper functions ata_dev_config_lba() and
>>>> ata_dev_config_chs() to configure the addressing capabilities of a
>>>> device. Each helper takes a string as argument for the addressing
>>>> information printed after these helpers execution completes.
>>>>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>>> ---
>>>>  drivers/ata/libata-core.c | 110 ++++++++++++++++++++------------------
>>>>  1 file changed, 59 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index b13194432e5a..2b6054cdd8fc 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
> [ .. ]
>>>>  				return rc;
>>>> -
>>>> -			/* print device info to dmesg */
>>>> -			if (ata_msg_drv(ap) && print_info) {
>>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> -					     revbuf, modelbuf, fwrevbuf,
>>>> -					     ata_mode_string(xfer_mask));
>>>> -				ata_dev_info(dev,
>>>> -					     "%llu sectors, multi %u: %s %s\n",
>>>> -					(unsigned long long)dev->n_sectors,
>>>> -					dev->multi_count, lba_desc, ncq_desc);
>>>> -			}
>>>>  		} else {
>>>> -			/* CHS */
>>>> -
>>>> -			/* Default translation */
>>>> -			dev->cylinders	= id[1];
>>>> -			dev->heads	= id[3];
>>>> -			dev->sectors	= id[6];
>>>> -
>>>> -			if (ata_id_current_chs_valid(id)) {
>>>> -				/* Current CHS translation is valid. */
>>>> -				dev->cylinders = id[54];
>>>> -				dev->heads     = id[55];
>>>> -				dev->sectors   = id[56];
>>>> -			}
>>>> +			ata_dev_config_chs(dev, lba_info, sizeof(lba_info));
>>>> +		}
>>>>  
>>>> -			/* print device info to dmesg */
>>>> -			if (ata_msg_drv(ap) && print_info) {
>>>> -				ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> -					     revbuf,	modelbuf, fwrevbuf,
>>>> -					     ata_mode_string(xfer_mask));
>>>> -				ata_dev_info(dev,
>>>> -					     "%llu sectors, multi %u, CHS %u/%u/%u\n",
>>>> -					     (unsigned long long)dev->n_sectors,
>>>> -					     dev->multi_count, dev->cylinders,
>>>> -					     dev->heads, dev->sectors);
>>>> -			}
>>>> +		/* print device info to dmesg */
>>>> +		if (ata_msg_drv(ap) && print_info) {
>>>> +			ata_dev_info(dev, "%s: %s, %s, max %s\n",
>>>> +				     revbuf, modelbuf, fwrevbuf,
>>>> +				     ata_mode_string(xfer_mask));
>>>> +			ata_dev_info(dev,
>>>> +				     "%llu sectors, multi %u, %s\n",
>>>> +				     (unsigned long long)dev->n_sectors,
>>>> +				     dev->multi_count, lba_info);
>>>>  		}
>>>>  
>>>>  		ata_dev_config_devslp(dev);
>>>>
>>> Hmm. Can't say I like it.
>>> Can't you move the second 'ata_dev_info()' call into the respective
>>> functions, and kill the temporary buffer?
>>
>> That would reverse the order of the messages... And I wanted to avoid having an
>> "if (ata_id_has_lba(id))" again just for the print. Moving the 2 ata_dev_info()
>> calls into the respective functions was the other solution I tried, but then the
>> functions require *a lot* more arguments (revbuf, modelbuf, fwrevbuf, ...) wich
>> was not super nice.
>>
>> This one is the least ugly I thought... Any other idea ?
>>
> Well, it should be possible to move the first 'ata_dev_info()' call
> _prior_ to the if(ata_id_has_lba(id)) condition, and then we can move
> the second call into the respective functions, no?

Indeed, it looks like that would work. Let me try that.

> 
> Cheers,
> 
> Hannes
> 


-- 
Damien Le Moal
Western Digital Research