Re: [PATCH net-next 02/18] ionic: Add hardware init and device commands

[Shannon Nelson <snelson@pensando.io>]

On 6/24/19 1:53 PM, Jakub Kicinski wrote:
> 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@pensando.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 :)

I'll see what I can do to add some detail in the ionic.rst, and maybe 
add a couple hints in driver comments.

>
>>   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.

Yes, this obviously needs to be removed.  I won't go into the history of 
why this is here, suffice to say I didn't get everything cleaned out.

>
>> +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

This made debugging easier for someone

>
>> +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 :)

They don't seem to be used in the drivers from a company I used to work 
for, but that doesn't necessarily mean I'm not a fan of them. My only 
problem with them is that this particular file is an API description 
used by other OSs as well, so I'll have to see how easily we can adapt 
them into the other platforms.  I'd rather not have to duplicate all the 
macro magic for other OSs, or have one version of this file for Linux 
and a different version for other OSs.  I suspect this may be the same 
concern with that other company.

Yes, I fully understand this is not a great argument for upstream code, 
but it is a practical matter I'm juggling.

That said, I will keep an eye out for where these can be used in the 
rest of the driver.

>
>> +	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?

It is useful in debugging the services inside 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!!

Oh crap.  Yes, that goes away.

Thanks for the detailed review time on a rather large file.

sln

>
>> +		__le32  mtu;
>> +		u8      mac[6];
>> +		__le64  features;
>> +		u8      rsvd2[11];
>> +	};
>> +	u8     color;
>> +};