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

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

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

<...>

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