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

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Aug 27, 2019 at 10:39:20AM -0700, Shannon Nelson wrote:
> 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.

Hi Shannon

What you should find is that every debugfs function will have
something like:

	if (IS_ERR(dentry))
	   return dentry;
or
	if (IS_ERR(parent))
	   return parent;

If you know of a API which is missing such protection, i'm sure GregKH
would like to know. Especially since he just ripped all such
protection in driver out. Meaning he just broken some drivers if such
IS_ERR() calls are missing in the debugfs core.

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

Yes, that sounds reasonable.

Thanks
	Andrew