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

[Hannes Reinecke <hare@suse.de>]

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?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer