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 Shannondebugfs 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.
Yes, we intend to add a devlink interface, it just isn't in this first patchset, which is already plenty big.+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.
+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
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.+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.
Thanks, sln
Andrew
