Re: [PATCH 3/4] coresight: allow to build as modules

[Kim Phillips <kim.phillips@arm.com>]

On Tue, 8 May 2018 12:31:08 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> Hi,

Hi,

> On 05/08/2018 12:06 PM, Kim Phillips wrote:
> 
> >  if CORESIGHT
> >  config CORESIGHT_LINKS_AND_SINKS
> > -	bool "CoreSight Link and Sink drivers"
> > +	tristate "CoreSight Link and Sink drivers"
> > +	depends on CORESIGHT
> 
> The "if CORESIGHT" line serves as a "depends on CORESIGHT" for the entire "if"
> block, so please don't repeat the "depends on" here.

Thanks, removed.

> >  config CORESIGHT_CPU_DEBUG
> >  	tristate "CoreSight CPU Debug driver"
> > -	depends on ARM || ARM64
> > -	depends on DEBUG_FS
> > +	depends on CORESIGHT && DEBUG_FS
> 
> "depends on CORESIGHT" is not needed if this is still inside the
> if CORESIGHT/endif block.  (I think it is but I can't tell from just looking
> at the patch itself.)

Thanks, it is, removed.

> 
> >  	help
> >  	  This driver provides support for coresight debugging module. This
> >  	  is primarily used to dump sample-based profiling registers when
> > @@ -103,4 +127,7 @@ config CORESIGHT_CPU_DEBUG
> >  	  properly, please refer Documentation/trace/coresight-cpu-debug.txt
> >  	  for detailed description and the example for usage.
> >  
> > +	  To compile this code as a module, choose M here: the
> > +	  module will be called coresight-cpu-debug.
> > +
> >  endif
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
> > index 3ffc9feb2d64..8c49c7b82d84 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
> > @@ -54,7 +54,7 @@ struct etm_filters {
> >  };
> >  
> >  
> > -#ifdef CONFIG_CORESIGHT
> > +#if IS_ENABLED(CONFIG_CORESIGHT)
> 
> Have you found (observed) that this change (above) is necessary (and others
> like it below)?  I thought that they would be equivalent.
> 
> From include/linux/kconfig.h:
> /*
>  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
>  * 0 otherwise.
>  */
> #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
> 
> 
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index f1d0e21d8cab..335bca44b42d 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> 
> > @@ -143,7 +149,7 @@ struct list_head *coresight_build_path(struct coresight_device *csdev,
> >  				       struct coresight_device *sink);
> >  void coresight_release_path(struct list_head *path);
> >  
> > -#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
> > +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
> 
> ditto.
> 
> >  extern int etm_readl_cp14(u32 off, unsigned int *val);
> >  extern int etm_writel_cp14(u32 off, u32 val);
> >  #else
> 
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index d950dad5056a..5863eb1a7335 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -243,7 +243,7 @@ struct coresight_ops {
> >  	const struct coresight_ops_source *source_ops;
> >  };
> >  
> > -#ifdef CONFIG_CORESIGHT
> > +#if IS_ENABLED(CONFIG_CORESIGHT)
> 
> ditto.

If I revert all three IS_ENABLED back to plain #ifdefs, and rebuild with
CONFIG_CORESIGHT*=m, I get:

  CC [M]  drivers/hwtracing/coresight/coresight.o
  CC [M]  drivers/hwtracing/coresight/of_coresight.o
../drivers/hwtracing/coresight/coresight.c:586:5: error: redefinition of ‘coresight_enable’
 int coresight_enable(struct coresight_device *csdev)
     ^~~~~~~~~~~~~~~~
In file included from ../drivers/hwtracing/coresight/coresight.c:23:0:
../include/linux/coresight.h:259:1: note: previous definition of ‘coresight_enable’ was here
 coresight_enable(struct coresight_device *csdev) { return -ENOSYS; }
 ^~~~~~~~~~~~~~~~
../drivers/hwtracing/coresight/coresight.c:670:6: error: redefinition of ‘coresight_disable’
 void coresight_disable(struct coresight_device *csdev)
      ^~~~~~~~~~~~~~~~~
In file included from ../drivers/hwtracing/coresight/coresight.c:23:0:
../include/linux/coresight.h:260:20: note: previous definition of ‘coresight_disable’ was here
 static inline void coresight_disable(struct coresight_device *csdev) {}
                    ^~~~~~~~~~~~~~~~~
../drivers/hwtracing/coresight/coresight.c:945:5: error: redefinition of ‘coresight_timeout’
 int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
     ^~~~~~~~~~~~~~~~~
In file included from ../drivers/hwtracing/coresight/coresight.c:23:0:
../include/linux/coresight.h:261:19: note: previous definition of ‘coresight_timeout’ was here
 static inline int coresight_timeout(void __iomem *addr, u32 offset,
                   ^~~~~~~~~~~~~~~~~
../drivers/hwtracing/coresight/coresight.c:991:26: error: redefinition of ‘coresight_register’
 struct coresight_device *coresight_register(struct coresight_desc *desc)
                          ^~~~~~~~~~~~~~~~~~
In file included from ../drivers/hwtracing/coresight/coresight.c:23:0:
../include/linux/coresight.h:256:1: note: previous definition of ‘coresight_register’ was here
 coresight_register(struct coresight_desc *desc) { return NULL; }
 ^~~~~~~~~~~~~~~~~~
../drivers/hwtracing/coresight/coresight.c:1081:6: error: redefinition of ‘coresight_unregister’
 void coresight_unregister(struct coresight_device *csdev)
      ^~~~~~~~~~~~~~~~~~~~
In file included from ../drivers/hwtracing/coresight/coresight.c:23:0:
../include/linux/coresight.h:257:20: note: previous definition of ‘coresight_unregister’ was here
 static inline void coresight_unregister(struct coresight_device *csdev) {}
                    ^~~~~~~~~~~~~~~~~~~~
../scripts/Makefile.build:312: recipe for target 'drivers/hwtracing/coresight/coresight.o' failed
make[3]: *** [drivers/hwtracing/coresight/coresight.o] Error 1

Building CORESIGHT=y builds ok, so, building it as a module causes the
latter stubs to be compiled:

#ifdef CONFIG_CORESIGHT
extern struct coresight_device *
coresight_register(struct coresight_desc *desc);
extern void coresight_unregister(struct coresight_device *csdev);
extern int coresight_enable(struct coresight_device *csdev);
extern void coresight_disable(struct coresight_device *csdev);
extern int coresight_timeout(void __iomem *addr, u32 offset,
                             int position, int value);
#else
static inline struct coresight_device *
coresight_register(struct coresight_desc *desc) { return NULL; }
static inline void coresight_unregister(struct coresight_device *csdev) {}
static inline int
coresight_enable(struct coresight_device *csdev) { return -ENOSYS; }
static inline void coresight_disable(struct coresight_device *csdev) {}
static inline int coresight_timeout(void __iomem *addr, u32 offset,
                                     int position, int value) { return 1; }
#endif

Adding kconfig.h to coresight.h's #include list doesn't help.  So we
need the IS_ENABLED for its __or(..., IS_MODULE()) case.

That being said, I don't know of any outside kernel-build dependencies
coresight.h might have.

Kim