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@xxxxxxx>
>>>> ---
>>>> 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
