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

[Andrew Boyer]

> On Dec 9, 2020, at 8:47 AM, Ferruh Yigit <ferruh.yigit at 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 at 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.

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

-Andrew