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

[Shannon Nelson <snelson@pensando.io>]

On 6/20/19 2:54 PM, Andrew Lunn wrote:
> On Thu, Jun 20, 2019 at 01:24:08PM -0700, Shannon Nelson wrote:
>> +	err = ionic_debugfs_add_dev(ionic);
>> +	if (err) {
>> +		dev_err(dev, "Cannot add device debugfs: %d , aborting\n", err);
>> +		goto err_out_clear_drvdata;
>> +	}
> Hi Shannon
>
> debugfs should not fail, since it is optional. debugfs failing should
> also not be considered fatal. And lastly, debugfs is not very liked by
> network people, so you should avoid it as much as possible. Use
> ethtool, lspci, devlink, etc.
Yes, we can drop this error check.

>
>> +
>> +#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;
>> +}
> Write is pretty much a no-no. We don't want proprietary user space
> tools manipulating the hardware/driver state.
Yep, this needs to come 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 const struct debugfs_reg32 dev_cmd_regs[] = {
>> +	{ .name = "db", .offset = 0, },
>> +	{ .name = "done", .offset = 4, },
>> +	{ .name = "cmd.word[0]", .offset = 8, },
>> +	{ .name = "cmd.word[1]", .offset = 12, },
>> +	{ .name = "cmd.word[2]", .offset = 16, },
>> +	{ .name = "cmd.word[3]", .offset = 20, },
>> +	{ .name = "cmd.word[4]", .offset = 24, },
>> +	{ .name = "cmd.word[5]", .offset = 28, },
>> +	{ .name = "cmd.word[6]", .offset = 32, },
>> +	{ .name = "cmd.word[7]", .offset = 36, },
>> +	{ .name = "cmd.word[8]", .offset = 40, },
>> +	{ .name = "cmd.word[9]", .offset = 44, },
>> +	{ .name = "cmd.word[10]", .offset = 48, },
>> +	{ .name = "cmd.word[11]", .offset = 52, },
>> +	{ .name = "cmd.word[12]", .offset = 56, },
>> +	{ .name = "cmd.word[13]", .offset = 60, },
>> +	{ .name = "cmd.word[14]", .offset = 64, },
>> +	{ .name = "cmd.word[15]", .offset = 68, },
>> +	{ .name = "comp.word[0]", .offset = 72, },
>> +	{ .name = "comp.word[1]", .offset = 76, },
>> +	{ .name = "comp.word[2]", .offset = 80, },
>> +	{ .name = "comp.word[3]", .offset = 84, },
>> +};
>> +
>> +int ionic_debugfs_add_dev_cmd(struct ionic *ionic)
>> +{
>> +	struct debugfs_regset32 *dev_cmd_regset;
>> +	struct device *dev = ionic->dev;
>> +	struct dentry *dentry;
>> +
>> +	dev_cmd_regset = devm_kzalloc(dev, sizeof(*dev_cmd_regset),
>> +				      GFP_KERNEL);
>> +	if (!dev_cmd_regset)
>> +		return -ENOMEM;
>> +	dev_cmd_regset->regs = dev_cmd_regs;
>> +	dev_cmd_regset->nregs = ARRAY_SIZE(dev_cmd_regs);
>> +	dev_cmd_regset->base = ionic->idev.dev_cmd_regs;
>> +
>> +	dentry = debugfs_create_regset32("dev_cmd", 0400,
>> +					 ionic->dentry, dev_cmd_regset);
>> +	if (IS_ERR_OR_NULL(dentry))
>> +		return PTR_ERR(dentry);
> ethtool -d seems like a more acceptable method for exporting
> registers.
Yes, that is one of the near future additions planned.

>
>> +static int identity_show(struct seq_file *seq, void *v)
>> +{
>> +	struct ionic *ionic = seq->private;
>> +	struct identity *ident = &ionic->ident;
>> +	struct ionic_dev *idev = &ionic->idev;
>> +
>> +	seq_printf(seq, "asic_type:        0x%x\n", idev->dev_info.asic_type);
>> +	seq_printf(seq, "asic_rev:         0x%x\n", idev->dev_info.asic_rev);
>> +	seq_printf(seq, "serial_num:       %s\n", idev->dev_info.serial_num);
>> +	seq_printf(seq, "fw_version:       %s\n", idev->dev_info.fw_version);
>> +	seq_printf(seq, "fw_status:        0x%x\n",
>> +		   ioread8(&idev->dev_info_regs->fw_status));
>> +	seq_printf(seq, "fw_heartbeat:     0x%x\n",
>> +		   ioread32(&idev->dev_info_regs->fw_heartbeat));
> devlink just gained a much more flexible version of ethtool -i. Please
> remove all this and use that.
Yes, we intend to add a devlink interface, it just isn't in this first 
patchset, which is already plenty big.
>
>> +int ionic_dev_setup(struct ionic *ionic)
>> +{
>> +	struct ionic_dev_bar *bar = ionic->bars;
>> +	unsigned int num_bars = ionic->num_bars;
>> +	struct ionic_dev *idev = &ionic->idev;
>> +	struct device *dev = ionic->dev;
>> +	u32 sig;
>> +
>> +	/* BAR0: dev_cmd and interrupts */
>> +	if (num_bars < 1) {
>> +		dev_info(dev, "No bars found, aborting\n");
> dev_err(), since this is fatal. The same for most of these dev_info()
> calls.
Will do
>
>> +enum os_type {
>> +	IONIC_OS_TYPE_LINUX   = 1,
>> +	IONIC_OS_TYPE_WIN     = 2,
>> +	IONIC_OS_TYPE_DPDK    = 3,
>> +	IONIC_OS_TYPE_FREEBSD = 4,
>> +	IONIC_OS_TYPE_IPXE    = 5,
>> +	IONIC_OS_TYPE_ESXI    = 6,
>> +};
>> +
>> +/**
>> + * union drv_identity - driver identity information
>> + * @os_type:          OS type (see enum os_type)
>> + * @os_dist:          OS distribution, numeric format
>> + * @os_dist_str:      OS distribution, string format
>> + * @kernel_ver:       Kernel version, numeric format
>> + * @kernel_ver_str:   Kernel version, string format
>> + * @driver_ver_str:   Driver version, string format
>> + */
>> +union drv_identity {
>> +	struct {
>> +		__le32 os_type;
>> +		__le32 os_dist;
>> +		char   os_dist_str[128];
>> +		__le32 kernel_ver;
>> +		char   kernel_ver_str[32];
>> +		char   driver_ver_str[32];
>> +	};
>> +	__le32 words[512];
>> +};
>> +int ionic_identify(struct ionic *ionic)
>> +{
>> +	struct identity *ident = &ionic->ident;
>> +	struct ionic_dev *idev = &ionic->idev;
>> +	size_t sz;
>> +	int err;
>> +
>> +	memset(ident, 0, sizeof(*ident));
>> +
>> +	ident->drv.os_type = cpu_to_le32(IONIC_OS_TYPE_LINUX);
>> +	ident->drv.os_dist = 0;
>> +	strncpy(ident->drv.os_dist_str, utsname()->release,
>> +		sizeof(ident->drv.os_dist_str) - 1);
>> +	ident->drv.kernel_ver = cpu_to_le32(LINUX_VERSION_CODE);
>> +	strncpy(ident->drv.kernel_ver_str, utsname()->version,
>> +		sizeof(ident->drv.kernel_ver_str) - 1);
>> +	strncpy(ident->drv.driver_ver_str, DRV_VERSION,
>> +		sizeof(ident->drv.driver_ver_str) - 1);
> Creepy.
It does seem odd at first to be telling your NIC about the OS, but this 
is becomes part of a larger whole and is used in our device support and 
management.

Thanks,
sln

>
> 	Andrew