Re: [PATCH v4 net-next 08/19] ionic: Add notifyq support

[Shannon Nelson]

On 7/24/19 4:21 PM, Saeed Mahameed wrote:
On Mon, 2019-07-22 at 14:40 -0700, Shannon Nelson wrote:
The AdminQ is fine for sending messages and requests to the NIC,
but we also need to have events published from the NIC to the
driver.  The NotifyQ handles this for us, using the same interrupt
as AdminQ.
Signed-off-by: Shannon Nelson<snel...@pensando.io>
---
  .../ethernet/pensando/ionic/ionic_debugfs.c   |  16 ++
  .../net/ethernet/pensando/ionic/ionic_lif.c   | 181
+++++++++++++++++-
  .../net/ethernet/pensando/ionic/ionic_lif.h   |   4 +
  3 files changed, 200 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
index 9af15c69b2a6..1d05b23de303 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
@@ -126,6 +126,7 @@ int ionic_debugfs_add_qcq(struct lif *lif, struct
qcq *qcq)
        struct debugfs_blob_wrapper *desc_blob;
        struct device *dev = lif->ionic->dev;
        struct intr *intr = &qcq->intr;
+       struct dentry *stats_dentry;
        struct queue *q = &qcq->q;
        struct cq *cq = &qcq->cq;
  
@@ -219,6 +220,21 @@ int ionic_debugfs_add_qcq(struct lif *lif,
struct qcq *qcq)
                                        intr_ctrl_regset);
        }
  
+	if (qcq->flags & QCQ_F_NOTIFYQ) {
+               stats_dentry = debugfs_create_dir("notifyblock",
qcq_dentry);
+               if (IS_ERR_OR_NULL(stats_dentry))
+                       return PTR_ERR(stats_dentry);
+
+               debugfs_create_u64("eid", 0400, stats_dentry,
+                                  (u64 *)&lif->info->status.eid);
+               debugfs_create_u16("link_status", 0400, stats_dentry,
+                                  (u16 *)&lif->info-
status.link_status);
+               debugfs_create_u32("link_speed", 0400, stats_dentry,
+                                  (u32 *)&lif->info-
status.link_speed);
+               debugfs_create_u16("link_down_count", 0400,
stats_dentry,
+                                  (u16 *)&lif->info-
status.link_down_count);
+       }
+
you never write to these lif->info->status.xyz ..

This is data coming out of DMA memory from the nic, the driver only 
reads it.

and link state and speed are/should be available  in "ethtool <ifname>"
so this looks redundant to me. you can also use ethtool -S to report
linkdown count.

The notifyblock is a chunk of data that usually stays together, but 
isn't really something for ethtool -S, I'd prefer to leave this here.

[...]
+       case EVENT_OPCODE_LOG:
+               netdev_info(netdev, "Notifyq EVENT_OPCODE_LOG
eid=%lld\n", eid);
+               print_hex_dump(KERN_INFO, "notifyq ",
DUMP_PREFIX_OFFSET, 16, 1,
+                              comp->log.data, sizeof(comp->log.data),
true);
So your device can generate log buffer dump into the kernel log ..
I am not sure how acceptable this is, maybe trace buffer is more
appropriate for this.

It turns out that this early design feature has gone unused, so I'll 
drop it out of here for now.

Thanks,
sln