On 12/9/2020 7:26 PM, Andrew Boyer wrote:


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?

for the fixed size variables, like uint64_t or uint32_t, better to use PRInNN

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?


If there is no real reason to change, like unless it is wrong/broken, please don't change them. So I think all PRIx64 -> %j changes in this patch can be dropped.

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.


Discussing may take time, don't get down by it, it will be OK, it won't take years ;) And I am aware rebasing can be hassle, but it can't be justification of a change, this is side affect of accumulating too many patches in the backlog unfortunately.

<...>

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


Reply via email to