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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bea92090-fb89-23bf-161e-c9814b3e09d6@pensando.io>
Date:   Tue, 30 Jul 2019 11:35:01 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v4 net-next 08/19] ionic: Add notifyq support

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