[v3,9/9] net/ionic: minor logging fixups

Message ID 20201204201646.51746-10-aboyer@pensando.io (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/ionic: minor updates and documentation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Andrew Boyer Dec. 4, 2020, 8:16 p.m. UTC
  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(-)
  

Comments

Ferruh Yigit Dec. 9, 2020, 1:47 p.m. UTC | #1
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?
  
Andrew Boyer Dec. 9, 2020, 2:45 p.m. UTC | #2
> On Dec 9, 2020, at 8:47 AM, Ferruh Yigit <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>
>> ---
>>  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
  
Ferruh Yigit Dec. 9, 2020, 3:42 p.m. UTC | #3
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.
  
Andrew Boyer Dec. 9, 2020, 7:26 p.m. UTC | #4
> On Dec 9, 2020, at 10:42 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> 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.

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
  
Ferruh Yigit Dec. 10, 2020, 9:58 a.m. UTC | #5
On 12/9/2020 7:26 PM, Andrew Boyer wrote:
> 
> 
>> On Dec 9, 2020, at 10:42 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> 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.
> 
> 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
>
  

Patch

diff --git a/drivers/net/ionic/ionic.h b/drivers/net/ionic/ionic.h
index a93110326..7ad0ab69e 100644
--- a/drivers/net/ionic/ionic.h
+++ b/drivers/net/ionic/ionic.h
@@ -48,6 +48,7 @@  struct ionic_hw {
 struct ionic_adapter {
 	struct ionic_hw hw;
 	struct ionic_dev idev;
+	const char *name;
 	struct ionic_dev_bar bars[IONIC_BARS_MAX];
 	struct ionic_identity	ident;
 	struct ionic_lif *lifs[IONIC_LIFS_MAX];
diff --git a/drivers/net/ionic/ionic_dev.c b/drivers/net/ionic/ionic_dev.c
index fc68f5c74..f32966521 100644
--- a/drivers/net/ionic/ionic_dev.c
+++ b/drivers/net/ionic/ionic_dev.c
@@ -102,6 +102,9 @@  ionic_dev_cmd_go(struct ionic_dev *idev, union ionic_dev_cmd *cmd)
 	uint32_t cmd_size = sizeof(cmd->words) /
 		sizeof(cmd->words[0]);
 
+	IONIC_PRINT(DEBUG, "Sending %s (%d) via dev_cmd",
+		    ionic_opcode_to_str(cmd->cmd.opcode), cmd->cmd.opcode);
+
 	for (i = 0; i < cmd_size; i++)
 		iowrite32(cmd->words[i], &idev->dev_cmd->cmd.words[i]);
 
@@ -348,6 +351,8 @@  ionic_dev_cmd_adminq_init(struct ionic_dev *idev,
 		.q_init.cq_ring_base = cq->base_pa,
 	};
 
+	IONIC_PRINT(DEBUG, "adminq.q_init.ver %u", cmd.q_init.ver);
+
 	ionic_dev_cmd_go(idev, &cmd);
 }
 
diff --git a/drivers/net/ionic/ionic_dev.h b/drivers/net/ionic/ionic_dev.h
index 7150f7f2c..026c4a9f3 100644
--- a/drivers/net/ionic/ionic_dev.h
+++ b/drivers/net/ionic/ionic_dev.h
@@ -205,6 +205,8 @@  struct ionic_qcq;
 void ionic_intr_init(struct ionic_dev *idev, struct ionic_intr_info *intr,
 	unsigned long index);
 
+const char *ionic_opcode_to_str(enum ionic_cmd_opcode opcode);
+
 int ionic_dev_setup(struct ionic_adapter *adapter);
 
 void ionic_dev_cmd_go(struct ionic_dev *idev, union ionic_dev_cmd *cmd);
diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
index a1c35ace3..d700aa745 100644
--- a/drivers/net/ionic/ionic_ethdev.c
+++ b/drivers/net/ionic/ionic_ethdev.c
@@ -571,7 +571,7 @@  ionic_dev_rss_reta_update(struct rte_eth_dev *eth_dev,
 
 	if (reta_size != ident->lif.eth.rss_ind_tbl_sz) {
 		IONIC_PRINT(ERR, "The size of hash lookup table configured "
-			"(%d) doesn't match the number hardware can supported "
+			"(%d) does not match the number hardware can support "
 			"(%d)",
 			reta_size, ident->lif.eth.rss_ind_tbl_sz);
 		return -EINVAL;
@@ -605,7 +605,7 @@  ionic_dev_rss_reta_query(struct rte_eth_dev *eth_dev,
 
 	if (reta_size != ident->lif.eth.rss_ind_tbl_sz) {
 		IONIC_PRINT(ERR, "The size of hash lookup table configured "
-			"(%d) doesn't match the number hardware can supported "
+			"(%d) does not match the number hardware can support "
 			"(%d)",
 			reta_size, ident->lif.eth.rss_ind_tbl_sz);
 		return -EINVAL;
diff --git a/drivers/net/ionic/ionic_lif.c b/drivers/net/ionic/ionic_lif.c
index 722a89565..112bd54fa 100644
--- a/drivers/net/ionic/ionic_lif.c
+++ b/drivers/net/ionic/ionic_lif.c
@@ -84,7 +84,7 @@  ionic_lif_reset(struct ionic_lif *lif)
 	ionic_dev_cmd_lif_reset(idev, lif->index);
 	err = ionic_dev_cmd_wait_check(idev, IONIC_DEVCMD_TIMEOUT);
 	if (err)
-		IONIC_PRINT(WARNING, "Failed to reset lif");
+		IONIC_PRINT(WARNING, "Failed to reset %s", lif->name);
 }
 
 static void
@@ -554,7 +554,7 @@  ionic_intr_alloc(struct ionic_lif *lif, struct ionic_intr_info *intr)
 	/*
 	 * Note: interrupt handler is called for index = 0 only
 	 * (we use interrupts for the notifyq only anyway,
-	 * which hash index = 0)
+	 * which has index = 0)
 	 */
 
 	for (index = 0; index < adapter->nintrs; index++)
@@ -687,8 +687,8 @@  ionic_qcq_alloc(struct ionic_lif *lif, uint8_t type,
 		ionic_q_sg_map(&new->q, sg_base, sg_base_pa);
 	}
 
-	IONIC_PRINT(DEBUG, "Q-Base-PA = %ju CQ-Base-PA = %ju "
-		"SG-base-PA = %ju",
+	IONIC_PRINT(DEBUG, "Q-Base-PA = %#jx CQ-Base-PA = %#jx "
+		"SG-base-PA = %#jx",
 		q_base_pa, cq_base_pa, sg_base_pa);
 
 	ionic_q_map(&new->q, q_base, q_base_pa);
@@ -827,7 +827,13 @@  ionic_lif_alloc(struct ionic_lif *lif)
 	int dbpage_num;
 	int err;
 
-	snprintf(lif->name, sizeof(lif->name), "lif%u", lif->index);
+	/*
+	 * lif->name was zeroed on allocation.
+	 * Copy (sizeof() - 1) bytes to ensure that it is NULL terminated.
+	 */
+	memcpy(lif->name, lif->eth_dev->data->name, sizeof(lif->name) - 1);
+
+	IONIC_PRINT(DEBUG, "LIF: %s", lif->name);
 
 	IONIC_PRINT(DEBUG, "Allocating Lif Info");
 
@@ -868,8 +874,6 @@  ionic_lif_alloc(struct ionic_lif *lif)
 
 	IONIC_PRINT(DEBUG, "Allocating Admin Queue");
 
-	IONIC_PRINT(DEBUG, "Allocating Admin Queue");
-
 	err = ionic_admin_qcq_alloc(lif);
 	if (err) {
 		IONIC_PRINT(ERR, "Cannot allocate admin queue");
@@ -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);
 	IONIC_PRINT(DEBUG, "notifyq_init.ring_size %d",
 		ctx.cmd.q_init.ring_size);
+	IONIC_PRINT(DEBUG, "notifyq_init.ver %u", ctx.cmd.q_init.ver);
 
 	err = ionic_adminq_post_wait(lif, &ctx);
 	if (err)
@@ -1327,11 +1330,11 @@  ionic_lif_txq_init(struct ionic_qcq *qcq)
 	};
 	int err;
 
-	IONIC_PRINT(DEBUG, "txq_init.index %d", ctx.cmd.q_init.index);
-	IONIC_PRINT(DEBUG, "txq_init.ring_base 0x%" PRIx64 "",
-		ctx.cmd.q_init.ring_base);
-	IONIC_PRINT(DEBUG, "txq_init.ring_size %d",
-		ctx.cmd.q_init.ring_size);
+
+	IONIC_PRINT(DEBUG, "txq_init.index %d", q->index);
+	IONIC_PRINT(DEBUG, "txq_init.ring_base %#jx", q->base_pa);
+	IONIC_PRINT(DEBUG, "txq_init.ring_size %d", ctx.cmd.q_init.ring_size);
+	IONIC_PRINT(DEBUG, "txq_init.ver %u", ctx.cmd.q_init.ver);
 
 	err = ionic_adminq_post_wait(qcq->lif, &ctx);
 	if (err)
@@ -1373,11 +1376,10 @@  ionic_lif_rxq_init(struct ionic_qcq *qcq)
 	};
 	int err;
 
-	IONIC_PRINT(DEBUG, "rxq_init.index %d", ctx.cmd.q_init.index);
-	IONIC_PRINT(DEBUG, "rxq_init.ring_base 0x%" PRIx64 "",
-		ctx.cmd.q_init.ring_base);
-	IONIC_PRINT(DEBUG, "rxq_init.ring_size %d",
-		ctx.cmd.q_init.ring_size);
+	IONIC_PRINT(DEBUG, "rxq_init.index %d", q->index);
+	IONIC_PRINT(DEBUG, "rxq_init.ring_base %#jx", q->base_pa);
+	IONIC_PRINT(DEBUG, "rxq_init.ring_size %d", ctx.cmd.q_init.ring_size);
+	IONIC_PRINT(DEBUG, "rxq_init.ver %u", ctx.cmd.q_init.ver);
 
 	err = ionic_adminq_post_wait(qcq->lif, &ctx);
 	if (err)
@@ -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));
 
 	ionic_adminq_post_wait(lif, &ctx);
 }
@@ -1643,23 +1646,23 @@  ionic_lif_identify(struct ionic_adapter *adapter)
 	for (i = 0; i < nwords; i++)
 		ident->lif.words[i] = ioread32(&idev->dev_cmd->data[i]);
 
-	IONIC_PRINT(INFO, "capabilities 0x%" PRIx64 " ",
+	IONIC_PRINT(INFO, "capabilities %#jx",
 		ident->lif.capabilities);
 
-	IONIC_PRINT(INFO, "eth.max_ucast_filters 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.max_ucast_filters %#x",
 		ident->lif.eth.max_ucast_filters);
-	IONIC_PRINT(INFO, "eth.max_mcast_filters 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.max_mcast_filters %#x",
 		ident->lif.eth.max_mcast_filters);
 
-	IONIC_PRINT(INFO, "eth.features 0x%" PRIx64 " ",
+	IONIC_PRINT(INFO, "eth.features %#jx",
 		ident->lif.eth.config.features);
-	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_ADMINQ] 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_ADMINQ] %#x",
 		ident->lif.eth.config.queue_count[IONIC_QTYPE_ADMINQ]);
-	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_NOTIFYQ] 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_NOTIFYQ] %#x",
 		ident->lif.eth.config.queue_count[IONIC_QTYPE_NOTIFYQ]);
-	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_RXQ] 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_RXQ] %#x",
 		ident->lif.eth.config.queue_count[IONIC_QTYPE_RXQ]);
-	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_TXQ] 0x%" PRIx32 " ",
+	IONIC_PRINT(INFO, "eth.queue_count[IONIC_QTYPE_TXQ] %#x",
 		ident->lif.eth.config.queue_count[IONIC_QTYPE_TXQ]);
 
 	return 0;
@@ -1680,7 +1683,8 @@  ionic_lifs_size(struct ionic_adapter *adapter)
 	nintrs = nlifs * 1 /* notifyq */;
 
 	if (nintrs > dev_nintrs) {
-		IONIC_PRINT(ERR, "At most %d intr queues supported, minimum required is %u",
+		IONIC_PRINT(ERR,
+			"At most %d intr supported, minimum req'd is %u",
 			dev_nintrs, nintrs);
 		return -ENOSPC;
 	}
diff --git a/drivers/net/ionic/ionic_mac_api.c b/drivers/net/ionic/ionic_mac_api.c
index c0ea042bc..7817b33e4 100644
--- a/drivers/net/ionic/ionic_mac_api.c
+++ b/drivers/net/ionic/ionic_mac_api.c
@@ -37,7 +37,7 @@  ionic_set_mac_type(struct ionic_hw *hw)
 	IONIC_PRINT_CALL();
 
 	if (hw->vendor_id != IONIC_PENSANDO_VENDOR_ID) {
-		IONIC_PRINT(ERR, "Unsupported vendor id: %" PRIx32 "",
+		IONIC_PRINT(ERR, "Unsupported vendor id: %#x",
 			hw->vendor_id);
 		return -EINVAL;
 	}
@@ -50,7 +50,7 @@  ionic_set_mac_type(struct ionic_hw *hw)
 		break;
 	default:
 		err = -EINVAL;
-		IONIC_PRINT(ERR, "Unsupported device id: %" PRIx32 "",
+		IONIC_PRINT(ERR, "Unsupported device id: %#x",
 			hw->device_id);
 		break;
 	}
diff --git a/drivers/net/ionic/ionic_main.c b/drivers/net/ionic/ionic_main.c
index 92cf0f398..ce5d11311 100644
--- a/drivers/net/ionic/ionic_main.c
+++ b/drivers/net/ionic/ionic_main.c
@@ -61,7 +61,7 @@  ionic_error_to_str(enum ionic_status_code code)
 	}
 }
 
-static const char *
+const char *
 ionic_opcode_to_str(enum ionic_cmd_opcode opcode)
 {
 	switch (opcode) {
@@ -107,6 +107,8 @@  ionic_opcode_to_str(enum ionic_cmd_opcode opcode)
 		return "IONIC_CMD_Q_INIT";
 	case IONIC_CMD_Q_CONTROL:
 		return "IONIC_CMD_Q_CONTROL";
+	case IONIC_CMD_Q_IDENTIFY:
+		return "IONIC_CMD_Q_IDENTIFY";
 	case IONIC_CMD_RDMA_RESET_LIF:
 		return "IONIC_CMD_RDMA_RESET_LIF";
 	case IONIC_CMD_RDMA_CREATE_EQ:
@@ -126,8 +128,9 @@  ionic_adminq_check_err(struct ionic_admin_ctx *ctx, bool timeout)
 	const char *name;
 	const char *status;
 
+	name = ionic_opcode_to_str(ctx->cmd.cmd.opcode);
+
 	if (ctx->comp.comp.status || timeout) {
-		name = ionic_opcode_to_str(ctx->cmd.cmd.opcode);
 		status = ionic_error_to_str(ctx->comp.comp.status);
 		IONIC_PRINT(ERR, "%s (%d) failed: %s (%d)",
 			name,
@@ -137,6 +140,8 @@  ionic_adminq_check_err(struct ionic_admin_ctx *ctx, bool timeout)
 		return -EIO;
 	}
 
+	IONIC_PRINT(DEBUG, "%s (%d) succeeded", name, ctx->cmd.cmd.opcode);
+
 	return 0;
 }
 
@@ -174,14 +179,13 @@  ionic_adminq_post_wait(struct ionic_lif *lif, struct ionic_admin_ctx *ctx)
 	bool done;
 	int err;
 
-	IONIC_PRINT(DEBUG, "Sending %s to the admin queue",
-		ionic_opcode_to_str(ctx->cmd.cmd.opcode));
+	IONIC_PRINT(DEBUG, "Sending %s (%d) via the admin queue",
+		ionic_opcode_to_str(ctx->cmd.cmd.opcode), ctx->cmd.cmd.opcode);
 
 	err = ionic_adminq_post(lif, ctx);
 	if (err) {
-		IONIC_PRINT(ERR, "Failure posting to the admin queue %d (%d)",
+		IONIC_PRINT(ERR, "Failure posting %d to the admin queue (%d)",
 			ctx->cmd.cmd.opcode, err);
-
 		return err;
 	}
 
@@ -244,6 +248,7 @@  ionic_dev_cmd_wait_check(struct ionic_dev *idev, unsigned long max_wait)
 	if (!err)
 		err = ionic_dev_cmd_check_error(idev);
 
+	IONIC_PRINT(DEBUG, "dev_cmd returned %d", err);
 	return err;
 }
 
@@ -335,12 +340,12 @@  ionic_port_identify(struct ionic_adapter *adapter)
 				ioread32(&idev->dev_cmd->data[i]);
 	}
 
-	IONIC_PRINT(INFO, "speed %d ", ident->port.config.speed);
-	IONIC_PRINT(INFO, "mtu %d ", ident->port.config.mtu);
-	IONIC_PRINT(INFO, "state %d ", ident->port.config.state);
-	IONIC_PRINT(INFO, "an_enable %d ", ident->port.config.an_enable);
-	IONIC_PRINT(INFO, "fec_type %d ", ident->port.config.fec_type);
-	IONIC_PRINT(INFO, "pause_type %d ", ident->port.config.pause_type);
+	IONIC_PRINT(INFO, "speed %d", ident->port.config.speed);
+	IONIC_PRINT(INFO, "mtu %d", ident->port.config.mtu);
+	IONIC_PRINT(INFO, "state %d", ident->port.config.state);
+	IONIC_PRINT(INFO, "an_enable %d", ident->port.config.an_enable);
+	IONIC_PRINT(INFO, "fec_type %d", ident->port.config.fec_type);
+	IONIC_PRINT(INFO, "pause_type %d", ident->port.config.pause_type);
 	IONIC_PRINT(INFO, "loopback_mode %d",
 		ident->port.config.loopback_mode);
 
@@ -381,8 +386,7 @@  ionic_port_init(struct ionic_adapter *adapter)
 	idev->port_info_sz = RTE_ALIGN(sizeof(*idev->port_info), PAGE_SIZE);
 
 	snprintf(z_name, sizeof(z_name), "%s_port_%s_info",
-		IONIC_DRV_NAME,
-		adapter->pci_dev->device.name);
+		IONIC_DRV_NAME, adapter->name);
 
 	idev->port_info_z = ionic_memzone_reserve(z_name, idev->port_info_sz,
 		SOCKET_ID_ANY);
diff --git a/drivers/net/ionic/ionic_rxtx.c b/drivers/net/ionic/ionic_rxtx.c
index b953aff49..b689c8381 100644
--- a/drivers/net/ionic/ionic_rxtx.c
+++ b/drivers/net/ionic/ionic_rxtx.c
@@ -133,7 +133,7 @@  ionic_dev_tx_queue_stop(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id)
 {
 	struct ionic_qcq *txq;
 
-	IONIC_PRINT_CALL();
+	IONIC_PRINT(DEBUG, "Stopping TX queue %u", tx_queue_id);
 
 	txq = eth_dev->data->tx_queues[tx_queue_id];
 
@@ -156,7 +156,7 @@  ionic_dev_tx_queue_stop(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id)
 
 int __rte_cold
 ionic_dev_tx_queue_setup(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id,
-		uint16_t nb_desc, uint32_t socket_id __rte_unused,
+		uint16_t nb_desc, uint32_t socket_id,
 		const struct rte_eth_txconf *tx_conf)
 {
 	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
@@ -164,11 +164,6 @@  ionic_dev_tx_queue_setup(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id,
 	uint64_t offloads;
 	int err;
 
-	IONIC_PRINT_CALL();
-
-	IONIC_PRINT(DEBUG, "Configuring TX queue %u with %u buffers",
-		tx_queue_id, nb_desc);
-
 	if (tx_queue_id >= lif->ntxqcqs) {
 		IONIC_PRINT(DEBUG, "Queue index %u not available "
 			"(max %u queues)",
@@ -177,6 +172,9 @@  ionic_dev_tx_queue_setup(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id,
 	}
 
 	offloads = tx_conf->offloads | eth_dev->data->dev_conf.txmode.offloads;
+	IONIC_PRINT(DEBUG,
+		"Configuring skt %u TX queue %u with %u buffers, offloads %jx",
+		socket_id, tx_queue_id, nb_desc, offloads);
 
 	/* Validate number of receive descriptors */
 	if (!rte_is_power_of_2(nb_desc) || nb_desc < IONIC_MIN_RING_DESC)
@@ -215,10 +213,11 @@  ionic_dev_tx_queue_start(struct rte_eth_dev *eth_dev, uint16_t tx_queue_id)
 	struct ionic_qcq *txq;
 	int err;
 
-	IONIC_PRINT_CALL();
-
 	txq = eth_dev->data->tx_queues[tx_queue_id];
 
+	IONIC_PRINT(DEBUG, "Starting TX queue %u, %u descs",
+		tx_queue_id, txq->q.num_descs);
+
 	err = ionic_lif_txq_init(txq);
 	if (err)
 		return err;
@@ -642,7 +641,7 @@  int __rte_cold
 ionic_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
 		uint16_t rx_queue_id,
 		uint16_t nb_desc,
-		uint32_t socket_id __rte_unused,
+		uint32_t socket_id,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mp)
 {
@@ -651,11 +650,6 @@  ionic_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
 	uint64_t offloads;
 	int err;
 
-	IONIC_PRINT_CALL();
-
-	IONIC_PRINT(DEBUG, "Configuring RX queue %u with %u buffers",
-		rx_queue_id, nb_desc);
-
 	if (rx_queue_id >= lif->nrxqcqs) {
 		IONIC_PRINT(ERR,
 			"Queue index %u not available (max %u queues)",
@@ -664,13 +658,16 @@  ionic_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
 	}
 
 	offloads = rx_conf->offloads | eth_dev->data->dev_conf.rxmode.offloads;
+	IONIC_PRINT(DEBUG,
+		"Configuring skt %u RX queue %u with %u buffers, offloads %jx",
+		socket_id, rx_queue_id, nb_desc, offloads);
 
 	/* Validate number of receive descriptors */
 	if (!rte_is_power_of_2(nb_desc) ||
 			nb_desc < IONIC_MIN_RING_DESC ||
 			nb_desc > IONIC_MAX_RING_DESC) {
 		IONIC_PRINT(ERR,
-			"Bad number of descriptors (%u) for queue %u (min: %u)",
+			"Bad descriptor count (%u) for queue %u (min: %u)",
 			nb_desc, rx_queue_id, IONIC_MIN_RING_DESC);
 		return -EINVAL; /* or use IONIC_DEFAULT_RING_DESC */
 	}
@@ -687,7 +684,7 @@  ionic_dev_rx_queue_setup(struct rte_eth_dev *eth_dev,
 
 	err = ionic_rx_qcq_alloc(lif, rx_queue_id, nb_desc, &rxq);
 	if (err) {
-		IONIC_PRINT(ERR, "Queue allocation failure");
+		IONIC_PRINT(ERR, "Queue %d allocation failure", rx_queue_id);
 		return -EINVAL;
 	}
 
@@ -959,13 +956,11 @@  ionic_dev_rx_queue_start(struct rte_eth_dev *eth_dev, uint16_t rx_queue_id)
 	struct ionic_qcq *rxq;
 	int err;
 
-	IONIC_PRINT_CALL();
-
-	IONIC_PRINT(DEBUG, "Allocating RX queue buffers (size: %u)",
-		frame_size);
-
 	rxq = eth_dev->data->rx_queues[rx_queue_id];
 
+	IONIC_PRINT(DEBUG, "Starting RX queue %u, %u descs (size: %u)",
+		rx_queue_id, rxq->q.num_descs, frame_size);
+
 	err = ionic_lif_rxq_init(rxq);
 	if (err)
 		return err;
@@ -1045,7 +1040,7 @@  ionic_dev_rx_queue_stop(struct rte_eth_dev *eth_dev, uint16_t rx_queue_id)
 {
 	struct ionic_qcq *rxq;
 
-	IONIC_PRINT_CALL();
+	IONIC_PRINT(DEBUG, "Stopping RX queue %u", rx_queue_id);
 
 	rxq = eth_dev->data->rx_queues[rx_queue_id];