| 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
| ||
|
Message-ID: <20190624135304.48755745@cakuba.netronome.com>
Date: Mon, 24 Jun 2019 13:53:04 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Shannon Nelson <snelson@...sando.io>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 02/18] ionic: Add hardware init and device
commands
On Thu, 20 Jun 2019 13:24:08 -0700, Shannon Nelson wrote:
> The ionic device has a small set of PCI registers, including a
> device control and data space, and a large set of message
> commands.
>
> Signed-off-by: Shannon Nelson <snelson@...sando.io>
> struct ionic {
> struct pci_dev *pdev;
> struct device *dev;
> + struct ionic_dev idev;
> + struct mutex dev_cmd_lock; /* lock for dev_cmd operations */
> + struct dentry *dentry;
> + struct ionic_dev_bar bars[IONIC_BARS_MAX];
> + unsigned int num_bars;
> + struct identity ident;
> + bool is_mgmt_nic;
What's a management NIC?
> + ionic->is_mgmt_nic =
> + ent->device == PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT;
You spent time in the docs describing how to use lspci, yet this magic
NIC is not mentioned :)
> static struct pci_driver ionic_driver = {
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> new file mode 100644
> index 000000000000..e5e45e6bec9d
> --- /dev/null
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
> +
> +#include <linux/netdevice.h>
> +
> +#include "ionic.h"
> +#include "ionic_bus.h"
> +#include "ionic_debugfs.h"
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int blob_open(struct inode *inode, struct file *filp)
> +{
> + filp->private_data = inode->i_private;
> + return 0;
> +}
> +
> +static ssize_t blob_read(struct file *filp, char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + struct debugfs_blob_wrapper *blob = filp->private_data;
> +
> + if (*ppos >= blob->size)
> + return 0;
> + if (*ppos + count > blob->size)
> + count = blob->size - *ppos;
> +
> + if (copy_to_user(buffer, blob->data + *ppos, count))
> + return -EFAULT;
> +
> + *ppos += count;
> +
> + return count;
> +}
> +
> +static ssize_t blob_write(struct file *filp, const char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + struct debugfs_blob_wrapper *blob = filp->private_data;
> +
> + if (*ppos >= blob->size)
> + return 0;
> + if (*ppos + count > blob->size)
> + count = blob->size - *ppos;
> +
> + if (copy_from_user(blob->data + *ppos, buffer, count))
> + return -EFAULT;
> +
> + *ppos += count;
> +
> + return count;
> +}
Why would you ever have to write to a debugfs blob? Red flag.
> +static const struct file_operations blob_fops = {
> + .owner = THIS_MODULE,
> + .open = blob_open,
> + .read = blob_read,
> + .write = blob_write,
> +};
> +
> +struct dentry *debugfs_create_blob(const char *name, umode_t mode,
> + struct dentry *parent,
> + struct debugfs_blob_wrapper *blob)
> +{
> + return debugfs_create_file(name, mode | 0200, parent, blob,
> + &blob_fops);
> +}
> +
> +static struct dentry *ionic_dir;
> +
> +#define single(name) \
> +static int name##_open(struct inode *inode, struct file *f) \
> +{ \
> + return single_open(f, name##_show, inode->i_private); \
> +} \
> + \
> +static const struct file_operations name##_fops = { \
> + .owner = THIS_MODULE, \
> + .open = name##_open, \
> + .read = seq_read, \
> + .llseek = seq_lseek, \
> + .release = single_release, \
> +}
DEFINE_SHOW_ATTRIBUTE() and friends.
> +static int bars_show(struct seq_file *seq, void *v)
> +{
> + struct ionic *ionic = seq->private;
> + struct ionic_dev_bar *bars = ionic->bars;
> + unsigned int i;
> +
> + for (i = 0; i < IONIC_BARS_MAX; i++)
> + if (bars[i].vaddr)
> + seq_printf(seq, "BAR%d: len 0x%lx vaddr %pK bus_addr %pad\n",
> + i, bars[i].len, bars[i].vaddr,
> + &bars[i].bus_addr);
Why? What's the value of this print beyond what's already visible from
PCI subsystem? :S
> +static inline u64 encode_txq_desc_cmd(u8 opcode, u8 flags,
> + u8 nsge, u64 addr)
> +{
> + u64 cmd;
> +
> + cmd = (opcode & IONIC_TXQ_DESC_OPCODE_MASK) << IONIC_TXQ_DESC_OPCODE_SHIFT;
IIRC you're not a fan of the FIELD_* macros, but let me suggest them
again :)
> + cmd |= (flags & IONIC_TXQ_DESC_FLAGS_MASK) << IONIC_TXQ_DESC_FLAGS_SHIFT;
> + cmd |= (nsge & IONIC_TXQ_DESC_NSGE_MASK) << IONIC_TXQ_DESC_NSGE_SHIFT;
> + cmd |= (addr & IONIC_TXQ_DESC_ADDR_MASK) << IONIC_TXQ_DESC_ADDR_SHIFT;
> +
> + return cmd;
> +};
> +
> +static inline void decode_txq_desc_cmd(u64 cmd, u8 *opcode, u8 *flags,
> + u8 *nsge, u64 *addr)
> +{
> + *opcode = (cmd >> IONIC_TXQ_DESC_OPCODE_SHIFT) & IONIC_TXQ_DESC_OPCODE_MASK;
> + *flags = (cmd >> IONIC_TXQ_DESC_FLAGS_SHIFT) & IONIC_TXQ_DESC_FLAGS_MASK;
> + *nsge = (cmd >> IONIC_TXQ_DESC_NSGE_SHIFT) & IONIC_TXQ_DESC_NSGE_MASK;
> + *addr = (cmd >> IONIC_TXQ_DESC_ADDR_SHIFT) & IONIC_TXQ_DESC_ADDR_MASK;
> +};
> +
> +#define IONIC_TX_MAX_SG_ELEMS 8
> +#define IONIC_RX_MAX_SG_ELEMS 8
> +/**
> + * struct dev_setattr_cmd - Set Device attributes on the NIC
> + * @opcode: Opcode
> + * @attr: Attribute type (enum dev_attr)
> + * @state: Device state (enum dev_state)
> + * @name: The bus info, e.g. PCI slot-device-function, 0 terminated
Interesting, why would this be of interest to the device?
> + * @features: Device features
> + */
> +struct dev_setattr_cmd {
> + u8 opcode;
> + u8 attr;
> + __le16 rsvd;
> + union {
> + u8 state;
> + char name[IONIC_IFNAMSIZ];
> + __le64 features;
> + u8 rsvd2[60];
> + };
> +};
> +/**
> + * struct lif_getattr_comp - LIF get attr command completion
> + * @status: The status of the command (enum status_code)
> + * @comp_index: The index in the descriptor ring for which this
> + * is the completion.
> + * @state: lif state (enum lif_state)
> + * @name: The netdev name string, 0 terminated
> + * @mtu: Mtu
> + * @mac: Station mac
> + * @features: Features (enum eth_hw_features)
> + * @color: Color bit
> + */
> +struct lif_getattr_comp {
> + u8 status;
> + u8 rsvd;
> + __le16 comp_index;
> + union {
> + u8 state;
> + //char name[IONIC_IFNAMSIZ];
Hi!!
> + __le32 mtu;
> + u8 mac[6];
> + __le64 features;
> + u8 rsvd2[11];
> + };
> + u8 color;
> +};
Powered by blists - more mailing lists