Re: [PATCH v5 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Joe Perches]

On Tue, 2018-05-08 at 15:07 -0500, Gary R Hook wrote:
> On 05/08/2018 01:48 PM, Joe Perches wrote:
> > On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:
> > > On 5/7/2018 6:47 PM, kbuild test robot wrote:
> > > > 
> > > > All error/warnings (new ones prefixed by >>):
> > > > 
> > > >      In file included from include/linux/intel-iommu.h:32:0,
> > > >                       from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >                       from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > >      include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
> > > > > > include/linux/iommu.h:706:8: error: parameter name omitted
> > > > 
> > > >       struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >              ^~~~~~
> > > >      In file included from include/linux/intel-iommu.h:32:0,
> > > >                       from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >                       from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > > > > include/linux/iommu.h:706:8: warning: control reaches end of non-void function [-Wreturn-type]
> > > > 
> > > >       struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >              ^~~~~~
> > > > 
> > > > vim +706 include/linux/iommu.h
> > > > 
> > > >      700	
> > > >      701	#ifdef CONFIG_IOMMU_DEBUGFS
> > > >      702	void iommu_debugfs_setup(void);
> > > >      703	struct dentry *iommu_debugfs_new_driver_dir(char *);
> > > >      704	#else
> > > >      705	static inline void iommu_debugfs_setup(void) {}
> > > >    > 706	struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >      707	#endif
> > > >      708	
> > > 
> > > I have no problems with adding parameter names. But
> > > scripts/checkpatch.pl doesn't seem to check for this, nor require it.
> > > Should checkpatch be updated?
> > 
> > I'm pretty sure that's not feasible.
> 
> Ugh. This is a definition, not a declaration. My bad. Which is likely 
> why I decided to apologize up front.
> 
> > And when the compiler tells you you've stuffed up some
> > syntactical bit, why should checkpatch duplicate the
> > output error message too?
> 
> Well, that's the point: neither the 4.8 nor 5.4 compiler complained 
> about this.

Perhaps because CONFIG_IOMMU_DEBUGFS was set in the .config
for all the compilation previously performed?

>  Not as an error, despite the fact that (now that I read what 
> is actually here, as opposed to what I think is there) this is wrong. 
> Had an error message been emitted, and the make stopped, I would have 
> figure this out before embarrassing myself in front of the entire interwebs.

There's no reason for that figuring out to be necessary.
Linux compilation complexity is pretty high and almost
no one understands it completely.

cheers, Joe