All of lore.kernel.org
 help / color / mirror / Atom feed
From: gregkh@linuxfoundation.org (Greg Kroah-Hartman)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 03/14] coresight: move shared barrier_pkt[] to coresight_priv.h
Date: Wed, 6 Jun 2018 10:27:57 +0200	[thread overview]
Message-ID: <20180606082757.GD19727@kroah.com> (raw)
In-Reply-To: <20180605210710.22227-4-kim.phillips@arm.com>

On Tue, Jun 05, 2018 at 04:06:59PM -0500, Kim Phillips wrote:
> barrier_pkt[] is used in the etb and tmc-etf coresight
> components.  Change barrier_pkt[] to a static definition,
> so as to allow them to be built as modules.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-priv.h | 8 +++++++-
>  drivers/hwtracing/coresight/coresight.c      | 7 -------
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 158c720119dd..e76f19ca9e04 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -57,7 +57,13 @@ static DEVICE_ATTR_RO(name)
>  #define coresight_simple_reg64(type, name, lo_off, hi_off)		\
>  	__coresight_simple_func(type, NULL, name, lo_off, hi_off)
>  
> -extern const u32 barrier_pkt[4];
> +/*
> + * When losing synchronisation a new barrier packet needs to be inserted at the
> + * beginning of the data collected in a buffer.  That way the decoder knows that
> + * it needs to look for another sync sequence.
> + */
> +static const u32 barrier_pkt[4] = { 0x7fffffff, 0x7fffffff,
> +				    0x7fffffff, 0x7fffffff };

Are you _sure_ this is doing what you think it is doing?

You now just created a bunch of different copies of this structure,
which might change the logic involved, right?

Putting a static variable in a .h file is generally considered a very
bad thing to do, I need a lot more "proof" this is ok before I can
accept this.  Worse case, just put the variable in the individual places
where it is needed.

thanks,

greg k-h

WARNING: multiple messages have this Message-ID (diff)
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Kim Phillips <kim.phillips@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Leo Yan <leo.yan@linaro.org>,
	Suzuki K Poulose <Suzuki.Poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	David Howells <dhowells@redhat.com>,
	Eric Auger <eric.auger@redhat.com>,
	Eric Biederman <ebiederm@xmission.com>,
	Gargi Sharma <gs051095@gmail.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	Kirill Tkhai <ktkhai@virtuozzo.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Pavel Tatashin <pasha.tatashin@oracle.com>,
	Rik van Riel <riel@redhat.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Russell King <linux@armlinux.org.uk>,
	Thierry Reding <treding@nvidia.com>, Todd Kjos <tkjos@google.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 03/14] coresight: move shared barrier_pkt[] to coresight_priv.h
Date: Wed, 6 Jun 2018 10:27:57 +0200	[thread overview]
Message-ID: <20180606082757.GD19727@kroah.com> (raw)
In-Reply-To: <20180605210710.22227-4-kim.phillips@arm.com>

On Tue, Jun 05, 2018 at 04:06:59PM -0500, Kim Phillips wrote:
> barrier_pkt[] is used in the etb and tmc-etf coresight
> components.  Change barrier_pkt[] to a static definition,
> so as to allow them to be built as modules.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Leo Yan <leo.yan@linaro.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-priv.h | 8 +++++++-
>  drivers/hwtracing/coresight/coresight.c      | 7 -------
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> index 158c720119dd..e76f19ca9e04 100644
> --- a/drivers/hwtracing/coresight/coresight-priv.h
> +++ b/drivers/hwtracing/coresight/coresight-priv.h
> @@ -57,7 +57,13 @@ static DEVICE_ATTR_RO(name)
>  #define coresight_simple_reg64(type, name, lo_off, hi_off)		\
>  	__coresight_simple_func(type, NULL, name, lo_off, hi_off)
>  
> -extern const u32 barrier_pkt[4];
> +/*
> + * When losing synchronisation a new barrier packet needs to be inserted at the
> + * beginning of the data collected in a buffer.  That way the decoder knows that
> + * it needs to look for another sync sequence.
> + */
> +static const u32 barrier_pkt[4] = { 0x7fffffff, 0x7fffffff,
> +				    0x7fffffff, 0x7fffffff };

Are you _sure_ this is doing what you think it is doing?

You now just created a bunch of different copies of this structure,
which might change the logic involved, right?

Putting a static variable in a .h file is generally considered a very
bad thing to do, I need a lot more "proof" this is ok before I can
accept this.  Worse case, just put the variable in the individual places
where it is needed.

thanks,

greg k-h

  reply	other threads:[~2018-06-06  8:27 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 21:06 [PATCH v4 00/14] coresight: allow to build components as modules Kim Phillips
2018-06-05 21:06 ` Kim Phillips
2018-06-05 21:06 ` [PATCH v4 01/14] coresight: cpu_debug: minor module fixups Kim Phillips
2018-06-05 21:06   ` Kim Phillips
2018-06-06  8:25   ` Greg Kroah-Hartman
2018-06-06  8:25     ` Greg Kroah-Hartman
2018-06-05 21:06 ` [PATCH v4 02/14] coresight: use IS_ENABLED for CONFIGs that may be modules Kim Phillips
2018-06-05 21:06   ` Kim Phillips
2018-06-05 21:06 ` [PATCH v4 03/14] coresight: move shared barrier_pkt[] to coresight_priv.h Kim Phillips
2018-06-05 21:06   ` Kim Phillips
2018-06-06  8:27   ` Greg Kroah-Hartman [this message]
2018-06-06  8:27     ` Greg Kroah-Hartman
2018-06-05 21:07 ` [PATCH v4 04/14] coresight: export coresight_timeout and etm_perf_symlink Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 05/14] coresight: get/put module in coresight_build/release_path Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:14   ` Suzuki K Poulose
2018-06-05 21:14     ` Suzuki K Poulose
2018-06-06  8:24   ` Greg Kroah-Hartman
2018-06-06  8:24     ` Greg Kroah-Hartman
2018-06-06  9:46     ` Suzuki K Poulose
2018-06-06  9:46       ` Suzuki K Poulose
2018-06-06 20:55       ` Kim Phillips
2018-06-06 20:55         ` Kim Phillips
2018-06-07  8:34         ` Greg Kroah-Hartman
2018-06-07  8:34           ` Greg Kroah-Hartman
2018-06-07  9:04           ` Suzuki K Poulose
2018-06-07  9:04             ` Suzuki K Poulose
2018-06-07  9:13             ` Greg Kroah-Hartman
2018-06-07  9:13               ` Greg Kroah-Hartman
2018-06-07  9:32               ` Suzuki K Poulose
2018-06-07  9:32                 ` Suzuki K Poulose
2018-06-07  9:34                 ` Suzuki K Poulose
2018-06-07  9:34                   ` Suzuki K Poulose
2018-06-07  9:53                 ` Greg Kroah-Hartman
2018-06-07  9:53                   ` Greg Kroah-Hartman
2018-06-07 10:07                   ` Suzuki K Poulose
2018-06-07 10:07                     ` Suzuki K Poulose
2018-06-07 17:13                     ` Kim Phillips
2018-06-07 17:13                       ` Kim Phillips
2018-06-07 21:10                       ` Suzuki K Poulose
2018-06-07 21:10                         ` Suzuki K Poulose
2018-06-07 21:40                         ` Mathieu Poirier
2018-06-07 21:40                           ` Mathieu Poirier
2018-06-07 21:47                         ` Kim Phillips
2018-06-07 21:47                           ` Kim Phillips
2018-06-07 21:59                           ` Mathieu Poirier
2018-06-07 21:59                             ` Mathieu Poirier
2018-06-08  9:22                           ` Suzuki K Poulose
2018-06-08  9:22                             ` Suzuki K Poulose
2018-06-07  9:43         ` Suzuki K Poulose
2018-06-07  9:43           ` Suzuki K Poulose
2018-06-05 21:07 ` [PATCH v4 06/14] coresight: allow stm to be built as a module Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 07/14] coresight: allow dynamic-replicator " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 08/14] coresight: allow etm3x " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 09/14] coresight: allow etm4x " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 10/14] coresight: allow etb " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 11/14] coresight: allow tpiu " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 12/14] coresight: allow tmc " Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 13/14] coresight: allow funnel and replicator drivers to be built as modules Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-05 21:07 ` [PATCH v4 14/14] coresight: allow the coresight core driver to be built as a module Kim Phillips
2018-06-05 21:07   ` Kim Phillips
2018-06-06  8:33   ` Greg Kroah-Hartman
2018-06-06  8:33     ` Greg Kroah-Hartman
2018-06-06  9:57   ` Suzuki K Poulose
2018-06-06  9:57     ` Suzuki K Poulose
2018-06-07 21:46   ` Mathieu Poirier
2018-06-07 21:46     ` Mathieu Poirier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180606082757.GD19727@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.