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

[Shannon Nelson <snelson@pensando.io>]

On 8/26/19 7:26 PM, Andrew Lunn wrote:
> 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.

With this check I end up either with a valid dentry value or NULL in 
ionic->dentry.  Without this check I possibly get an error value in 
ionic->dentry, which can get used later as the parent dentry to try to 
make a new debugfs node.  Some quick tracing looks like this error value 
will get dereferenced in a call to inode_lock(), which would likely 
cause us some heartburn.

I'd prefer to keep this check and leave ionic->dentry as NULL.

I've removed several of the other error checks and messages that were in 
this code, but left a few of the IS_ERR_OR_NULL checks to be sure we 
don't try dereferencing similar bogus values.

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

If CONFIG_DEBUG_FS is not enabled, I would prefer this driver's debugfs 
code to also be left out rather than be compiled in and left useless.

>
>> +/**
>> + * 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 suppose the phrase "you can trust us" wouldn't help much here, would 
it... :-)

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

That makes perfect sense.

I can add a full description here of how the information will be used, 
which should help most folks, but I'm sure there will still be some that 
don't want this info released.

What I'd like to propose here is that I do the hardcoded strings myself 
for now, and I work up a way for the users to enable the feature as 
desired, with a reasonable comment here in the code and in the 
Documentation/.../ionic.rst file.  This might end up as an ethtool 
priv-flag that defaults to off and can set a NIC value that is 
remembered for later.

Does that sound reasonable?

Cheers,
sln