> On Dec 9, 2020, at 10:42 AM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
>
> On 12/9/2020 2:45 PM, Andrew Boyer wrote:
>>> On Dec 9, 2020, at 8:47 AM, Ferruh Yigit <ferruh.yi...@intel.com
>>> <mailto:ferruh.yi...@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 <abo...@pensando.io
>>>> <mailto:abo...@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.
Ferruh, I made these changes months ago. Changing them back now is going to
take at least a few hours - many other changes are layered on top.
> 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.
Apparently it is not up to me, though, is it? I would very much appreciate if
you would respond to my request for a meeting, at any time you find convenient.
When I add new log messages in the future (including adding to these lists of
FW values and response codes), should I use PRIx64 or %jx?
Should I expect your objection to a mix of PRIx64 and %jx in the same paragraph?
Am I allowed to change from PRIx64 to %jx if I am also modifying the text or
the value logged?
This is going to involve respinning all of those functional patches, and since
I am not a mind-reader it seems likely that this is going to take years.
>>> <...>
>>>
>>>> @@ -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.
I will set the last character to NULL if that is what you want. I do not see
how it serves any purpose.
-Andrew
