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

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Aug 26, 2019 at 02:33:23PM -0700, Shannon Nelson wrote:
> +void ionic_debugfs_add_dev(struct ionic *ionic)
> +{
> +	struct dentry *dentry;
> +
> +	dentry = debugfs_create_dir(ionic_bus_info(ionic), ionic_dir);
> +	if (IS_ERR_OR_NULL(dentry))
> +		return;
> +
> +	ionic->dentry = dentry;
> +}

Hi Shannon

There was recently a big patchset from GregKH which removed all error
checking from drivers calling debugfs calls. I'm pretty sure you don't
need this check here.

> +#ifdef CONFIG_DEBUG_FS
> +
> +void ionic_debugfs_create(void);
> +void ionic_debugfs_destroy(void);
> +void ionic_debugfs_add_dev(struct ionic *ionic);
> +void ionic_debugfs_del_dev(struct ionic *ionic);
> +void ionic_debugfs_add_ident(struct ionic *ionic);
> +#else
> +static inline void ionic_debugfs_create(void) { }
> +static inline void ionic_debugfs_destroy(void) { }
> +static inline void ionic_debugfs_add_dev(struct ionic *ionic) { }
> +static inline void ionic_debugfs_del_dev(struct ionic *ionic) { }
> +static inline void ionic_debugfs_add_ident(struct ionic *ionic) { }
> +#endif

Is this really needed? I would expect there to be stubs for all the
debugfs calls if it is disabled.

> +/**
> + * 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 ionic_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 ionic_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, IONIC_DRV_VERSION,
> +		sizeof(ident->drv.driver_ver_str) - 1);
> +
> +	mutex_lock(&ionic->dev_cmd_lock);
> +

I don't know about others, but from a privacy prospective, i'm not so
happy about this. This is a smart NIC. It could be reporting back to
Mothership pensando with this information?

I would be happier if there was a privacy statement, right here,
saying what this information is used for, and an agreement it is not
used for anything else. If that gets violated, you can then only blame
yourself when we ripe this out and hard code it to static values.

	 Andrew