Re: [dpdk-dev] [PATCH v3 9/9] net/ionic: minor logging fixups

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/9/2020 2:45 PM, Andrew Boyer wrote:
> 
> 
>> On Dec 9, 2020, at 8:47 AM, Ferruh Yigit <ferruh.yigit@intel.com 
>> <mailto:ferruh.yigit@intel.com>> wrote:
>>
>> On 12/4/2020 8:16 PM, Andrew Boyer wrote:
>>> Expose ionic_opcode_to_str() so it can be used for dev cmds, too.
>>> Store the device name in struct adapter.
>>> Switch to memcpy() to work around gcc false positives.
>>> Signed-off-by: Andrew Boyer <aboyer@pensando.io <mailto:aboyer@pensando.io>>
>>> ---
>>>  drivers/net/ionic/ionic.h         |  1 +
>>>  drivers/net/ionic/ionic_dev.c     |  5 +++
>>>  drivers/net/ionic/ionic_dev.h     |  2 +
>>>  drivers/net/ionic/ionic_ethdev.c  |  4 +-
>>>  drivers/net/ionic/ionic_lif.c     | 68 ++++++++++++++++---------------
>>>  drivers/net/ionic/ionic_mac_api.c |  4 +-
>>>  drivers/net/ionic/ionic_main.c    | 32 ++++++++-------
>>>  drivers/net/ionic/ionic_rxtx.c    | 41 ++++++++-----------
>>>  8 files changed, 84 insertions(+), 73 deletions(-)
>>
>> <...>
>>
>>> @@ -1217,12 +1221,11 @@ ionic_lif_notifyq_init(struct ionic_lif *lif)
>>> }
>>> };
>>>  -IONIC_PRINT(DEBUG, "notifyq_init.index %d",
>>> -ctx.cmd.q_init.index);
>>> -IONIC_PRINT(DEBUG, "notifyq_init.ring_base 0x%" PRIx64 "",
>>> -ctx.cmd.q_init.ring_base);
>>> +IONIC_PRINT(DEBUG, "notifyq_init.index %d", q->index);
>>> +IONIC_PRINT(DEBUG, "notifyq_init.ring_base %#jx", q->base_pa);
>>
>> There are lots of similar PRIx64 -> %j change in this patch,
>> '%j' specifier is for 'intmax_t' and which seems 64bit storage, so this should 
>> work with 64 bit variable 'q->base_pa',
>> but the variable is explicitly uint64_t why replacing 'PRIx64' usage which is 
>> correct and more common usage in the DPDK? Why ionic is want to do this in its 
>> own way, I am not clear of the motivation of these changes really, can you 
>> please clarify?
> 
> As best I know, I am following the (two different) contribute guidelines pages, 
> both of which direct submitters to run checkpatch. One of things checkpatch 
> flags is lines over 80 columns. Many of these lines were over 80 columns or 
> oddly broken to meet the 80 column limit.
> 
> %j is used in many other places in this PMD - as originally written by Alfredo, 
> one of your core contributors. If we are allowed to use %j, I want to, since I 
> much prefer it to the hideous PRIx64.
> 

%j is accepted, that is not an issue. But you are making an active effort to 
convert PRIx64 -> %j, which is very unnecessary in my opinion.

80 column limit is not for log strings, but even if you are fixing them that is 
different thing from the PRIx64 -> %j conversion, you can keep PRIx64 and stay 
in 80 columns, and indeed lots of the cases the column limit seems not an issue 
at all.

Andrew, this is a driver currently marked as 'UNMAINTAINED', I kindly suggest 
focusing your 70+ functional changes instead of this PRIx64 -> %j syntax 
changes, but it is all up to you of course.

>> <...>
>>
>>> @@ -1448,8 +1450,9 @@ ionic_lif_set_name(struct ionic_lif *lif)
>>> },
>>> };
>>>  -snprintf(ctx.cmd.lif_setattr.name, sizeof(ctx.cmd.lif_setattr.name),
>>> -"%d", lif->port_id);
>>> +/* FW is responsible for NULL terminating this field */
>>> +memcpy(ctx.cmd.lif_setattr.name, lif->name,
>>> +sizeof(ctx.cmd.lif_setattr.name));
>>
>> Even though FW may be guaranting the string will be null terminated, won't it 
>> be nice to provide input as null terminated if this is the expectation?
> 
> No, that is not the expectation. We prefer it to be this way.
> 

It is know that FW will add NULL terminate the string but you "prefer" to 
provide 'name' without NULL termination. Why?
"we prefer it to be this way" is not a good justification, please either change 
or explain in a logical way.