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