[PATCH] drivers: CCI: fix used_mask init in validate_group()

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

Currently in validate_group(), there is a static initializer
for fake_pmu.used_mask which is based on CPU_BITS_NONE but
the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
is not correct and will cause a build failure if NR_CPUS
is set high enough to make CPU_BITS_NONE larger than used_mask.
This patch changes the used_mask initialization to be runtime
based on the actual size of the array.

Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/bus/arm-cci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..1d83072 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -679,13 +679,13 @@ static int
 validate_group(struct perf_event *event)
 {
 	struct perf_event *sibling, *leader = event->group_leader;
-	struct cci_pmu_hw_events fake_pmu = {
-		/*
-		 * Initialise the fake PMU. We only need to populate the
-		 * used_mask for the purposes of validation.
-		 */
-		.used_mask = CPU_BITS_NONE,
-	};
+	struct cci_pmu_hw_events fake_pmu;
+
+	/*
+	 * Initialise the fake PMU. We only need to populate the
+	 * used_mask for the purposes of validation.
+	 */
+	memset(fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
 	if (!validate_event(&fake_pmu, leader))
 		return -EINVAL;
-- 
1.9.3

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

Currently in validate_group(), there is a static initializer
for fake_pmu.used_mask which is based on CPU_BITS_NONE but
the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
is not correct and will cause a build failure if NR_CPUS
is set high enough to make CPU_BITS_NONE larger than used_mask.
This patch changes the used_mask initialization to be runtime
based on the actual size of the array.

Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/bus/arm-cci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..1d83072 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -679,13 +679,13 @@ static int
 validate_group(struct perf_event *event)
 {
 	struct perf_event *sibling, *leader = event->group_leader;
-	struct cci_pmu_hw_events fake_pmu = {
-		/*
-		 * Initialise the fake PMU. We only need to populate the
-		 * used_mask for the purposes of validation.
-		 */
-		.used_mask = CPU_BITS_NONE,
-	};
+	struct cci_pmu_hw_events fake_pmu;
+
+	/*
+	 * Initialise the fake PMU. We only need to populate the
+	 * used_mask for the purposes of validation.
+	 */
+	memset(fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
 	if (!validate_event(&fake_pmu, leader))
 		return -EINVAL;
-- 
1.9.3

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 08/04/15 19:21, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
> This patch changes the used_mask initialization to be runtime
> based on the actual size of the array.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>   drivers/bus/arm-cci.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..1d83072 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -679,13 +679,13 @@ static int
>   validate_group(struct perf_event *event)
>   {
>   	struct perf_event *sibling, *leader = event->group_leader;
> -	struct cci_pmu_hw_events fake_pmu = {
> -		/*
> -		 * Initialise the fake PMU. We only need to populate the
> -		 * used_mask for the purposes of validation.
> -		 */
> -		.used_mask = CPU_BITS_NONE,
> -	};
> +	struct cci_pmu_hw_events fake_pmu;
> +
> +	/*
> +	 * Initialise the fake PMU. We only need to populate the
> +	 * used_mask for the purposes of validation.
> +	 */
> +	memset(fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
The patch looks good to me.

Reviewed-by: Suzuki K. Poulose <suzuki.poulose@arm.com>

Suzuki

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 08/04/15 19:21, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
> This patch changes the used_mask initialization to be runtime
> based on the actual size of the array.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>   drivers/bus/arm-cci.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..1d83072 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -679,13 +679,13 @@ static int
>   validate_group(struct perf_event *event)
>   {
>   	struct perf_event *sibling, *leader = event->group_leader;
> -	struct cci_pmu_hw_events fake_pmu = {
> -		/*
> -		 * Initialise the fake PMU. We only need to populate the
> -		 * used_mask for the purposes of validation.
> -		 */
> -		.used_mask = CPU_BITS_NONE,
> -	};
> +	struct cci_pmu_hw_events fake_pmu;
> +
> +	/*
> +	 * Initialise the fake PMU. We only need to populate the
> +	 * used_mask for the purposes of validation.
> +	 */
> +	memset(fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
The patch looks good to me.

Reviewed-by: Suzuki K. Poulose <suzuki.poulose@arm.com>

Suzuki

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.

Whoops. My bad.

> This patch changes the used_mask initialization to be runtime
> based on the actual size of the array.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>  drivers/bus/arm-cci.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..1d83072 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -679,13 +679,13 @@ static int
>  validate_group(struct perf_event *event)
>  {
>  	struct perf_event *sibling, *leader = event->group_leader;
> -	struct cci_pmu_hw_events fake_pmu = {
> -		/*
> -		 * Initialise the fake PMU. We only need to populate the
> -		 * used_mask for the purposes of validation.
> -		 */
> -		.used_mask = CPU_BITS_NONE,

Can we not simply change this to:

		.used_mask = { 0 },

That should result in the entire array being zeroed.

Thanks,
Mark.

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.

Whoops. My bad.

> This patch changes the used_mask initialization to be runtime
> based on the actual size of the array.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>  drivers/bus/arm-cci.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..1d83072 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -679,13 +679,13 @@ static int
>  validate_group(struct perf_event *event)
>  {
>  	struct perf_event *sibling, *leader = event->group_leader;
> -	struct cci_pmu_hw_events fake_pmu = {
> -		/*
> -		 * Initialise the fake PMU. We only need to populate the
> -		 * used_mask for the purposes of validation.
> -		 */
> -		.used_mask = CPU_BITS_NONE,

Can we not simply change this to:

		.used_mask = { 0 },

That should result in the entire array being zeroed.

Thanks,
Mark.

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> 
> Whoops. My bad.
> 
> > This patch changes the used_mask initialization to be runtime
> > based on the actual size of the array.
> > 
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> >  drivers/bus/arm-cci.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > index 84fd660..1d83072 100644
> > --- a/drivers/bus/arm-cci.c
> > +++ b/drivers/bus/arm-cci.c
> > @@ -679,13 +679,13 @@ static int
> >  validate_group(struct perf_event *event)
> >  {
> >  	struct perf_event *sibling, *leader = event->group_leader;
> > -	struct cci_pmu_hw_events fake_pmu = {
> > -		/*
> > -		 * Initialise the fake PMU. We only need to populate the
> > -		 * used_mask for the purposes of validation.
> > -		 */
> > -		.used_mask = CPU_BITS_NONE,
> 
> Can we not simply change this to:
> 
> 		.used_mask = { 0 },
> 
> That should result in the entire array being zeroed.

It does, but it also causes the whole struct to be cleared.
With the memset, only used_mask gets cleared.

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> 
> Whoops. My bad.
> 
> > This patch changes the used_mask initialization to be runtime
> > based on the actual size of the array.
> > 
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> >  drivers/bus/arm-cci.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > index 84fd660..1d83072 100644
> > --- a/drivers/bus/arm-cci.c
> > +++ b/drivers/bus/arm-cci.c
> > @@ -679,13 +679,13 @@ static int
> >  validate_group(struct perf_event *event)
> >  {
> >  	struct perf_event *sibling, *leader = event->group_leader;
> > -	struct cci_pmu_hw_events fake_pmu = {
> > -		/*
> > -		 * Initialise the fake PMU. We only need to populate the
> > -		 * used_mask for the purposes of validation.
> > -		 */
> > -		.used_mask = CPU_BITS_NONE,
> 
> Can we not simply change this to:
> 
> 		.used_mask = { 0 },
> 
> That should result in the entire array being zeroed.

It does, but it also causes the whole struct to be cleared.
With the memset, only used_mask gets cleared.

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 09, 2015 at 03:11:43PM +0100, Mark Salter wrote:
> On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> > On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > > Currently in validate_group(), there is a static initializer
> > > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > > is not correct and will cause a build failure if NR_CPUS
> > > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > 
> > Whoops. My bad.
> > 
> > > This patch changes the used_mask initialization to be runtime
> > > based on the actual size of the array.
> > > 
> > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > ---
> > >  drivers/bus/arm-cci.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > > index 84fd660..1d83072 100644
> > > --- a/drivers/bus/arm-cci.c
> > > +++ b/drivers/bus/arm-cci.c
> > > @@ -679,13 +679,13 @@ static int
> > >  validate_group(struct perf_event *event)
> > >  {
> > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > -	struct cci_pmu_hw_events fake_pmu = {
> > > -		/*
> > > -		 * Initialise the fake PMU. We only need to populate the
> > > -		 * used_mask for the purposes of validation.
> > > -		 */
> > > -		.used_mask = CPU_BITS_NONE,
> > 
> > Can we not simply change this to:
> > 
> > 		.used_mask = { 0 },
> > 
> > That should result in the entire array being zeroed.
> 
> It does, but it also causes the whole struct to be cleared.

Sure, but it's also the minimal diff, and it's easier to read. This was
what the code was intended to be initially.

> With the memset, only used_mask gets cleared.

Is there an appreciable difference between the two performance-wise?

Mark.

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 09, 2015 at 03:11:43PM +0100, Mark Salter wrote:
> On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> > On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > > Currently in validate_group(), there is a static initializer
> > > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > > is not correct and will cause a build failure if NR_CPUS
> > > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > 
> > Whoops. My bad.
> > 
> > > This patch changes the used_mask initialization to be runtime
> > > based on the actual size of the array.
> > > 
> > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > ---
> > >  drivers/bus/arm-cci.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > > index 84fd660..1d83072 100644
> > > --- a/drivers/bus/arm-cci.c
> > > +++ b/drivers/bus/arm-cci.c
> > > @@ -679,13 +679,13 @@ static int
> > >  validate_group(struct perf_event *event)
> > >  {
> > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > -	struct cci_pmu_hw_events fake_pmu = {
> > > -		/*
> > > -		 * Initialise the fake PMU. We only need to populate the
> > > -		 * used_mask for the purposes of validation.
> > > -		 */
> > > -		.used_mask = CPU_BITS_NONE,
> > 
> > Can we not simply change this to:
> > 
> > 		.used_mask = { 0 },
> > 
> > That should result in the entire array being zeroed.
> 
> It does, but it also causes the whole struct to be cleared.

Sure, but it's also the minimal diff, and it's easier to read. This was
what the code was intended to be initially.

> With the memset, only used_mask gets cleared.

Is there an appreciable difference between the two performance-wise?

Mark.

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-09 at 15:20 +0100, Mark Rutland wrote:
> On Thu, Apr 09, 2015 at 03:11:43PM +0100, Mark Salter wrote:
> > On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> > > On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > > > Currently in validate_group(), there is a static initializer
> > > > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > > > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > > > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > > > is not correct and will cause a build failure if NR_CPUS
> > > > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > > 
> > > Whoops. My bad.
> > > 
> > > > This patch changes the used_mask initialization to be runtime
> > > > based on the actual size of the array.
> > > > 
> > > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > > ---
> > > >  drivers/bus/arm-cci.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > > > index 84fd660..1d83072 100644
> > > > --- a/drivers/bus/arm-cci.c
> > > > +++ b/drivers/bus/arm-cci.c
> > > > @@ -679,13 +679,13 @@ static int
> > > >  validate_group(struct perf_event *event)
> > > >  {
> > > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > > -	struct cci_pmu_hw_events fake_pmu = {
> > > > -		/*
> > > > -		 * Initialise the fake PMU. We only need to populate the
> > > > -		 * used_mask for the purposes of validation.
> > > > -		 */
> > > > -		.used_mask = CPU_BITS_NONE,
> > > 
> > > Can we not simply change this to:
> > > 
> > > 		.used_mask = { 0 },
> > > 
> > > That should result in the entire array being zeroed.
> > 
> > It does, but it also causes the whole struct to be cleared.
> 
> Sure, but it's also the minimal diff, and it's easier to read. This was
> what the code was intended to be initially.
> 
> > With the memset, only used_mask gets cleared.
> 
> Is there an appreciable difference between the two performance-wise?

I dunno. It is 3 strp insns vs 1 str.
If you want the static init, I'll send another patch.

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-09 at 15:20 +0100, Mark Rutland wrote:
> On Thu, Apr 09, 2015 at 03:11:43PM +0100, Mark Salter wrote:
> > On Thu, 2015-04-09 at 14:51 +0100, Mark Rutland wrote:
> > > On Wed, Apr 08, 2015 at 07:21:24PM +0100, Mark Salter wrote:
> > > > Currently in validate_group(), there is a static initializer
> > > > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > > > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > > > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > > > is not correct and will cause a build failure if NR_CPUS
> > > > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > > 
> > > Whoops. My bad.
> > > 
> > > > This patch changes the used_mask initialization to be runtime
> > > > based on the actual size of the array.
> > > > 
> > > > Signed-off-by: Mark Salter <msalter@redhat.com>
> > > > ---
> > > >  drivers/bus/arm-cci.c | 14 +++++++-------
> > > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > > > index 84fd660..1d83072 100644
> > > > --- a/drivers/bus/arm-cci.c
> > > > +++ b/drivers/bus/arm-cci.c
> > > > @@ -679,13 +679,13 @@ static int
> > > >  validate_group(struct perf_event *event)
> > > >  {
> > > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > > -	struct cci_pmu_hw_events fake_pmu = {
> > > > -		/*
> > > > -		 * Initialise the fake PMU. We only need to populate the
> > > > -		 * used_mask for the purposes of validation.
> > > > -		 */
> > > > -		.used_mask = CPU_BITS_NONE,
> > > 
> > > Can we not simply change this to:
> > > 
> > > 		.used_mask = { 0 },
> > > 
> > > That should result in the entire array being zeroed.
> > 
> > It does, but it also causes the whole struct to be cleared.
> 
> Sure, but it's also the minimal diff, and it's easier to read. This was
> what the code was intended to be initially.
> 
> > With the memset, only used_mask gets cleared.
> 
> Is there an appreciable difference between the two performance-wise?

I dunno. It is 3 strp insns vs 1 str.
If you want the static init, I'll send another patch.

[PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

> > > > >  validate_group(struct perf_event *event)
> > > > >  {
> > > > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > > > -	struct cci_pmu_hw_events fake_pmu = {
> > > > > -		/*
> > > > > -		 * Initialise the fake PMU. We only need to populate the
> > > > > -		 * used_mask for the purposes of validation.
> > > > > -		 */
> > > > > -		.used_mask = CPU_BITS_NONE,
> > > > 
> > > > Can we not simply change this to:
> > > > 
> > > > 		.used_mask = { 0 },
> > > > 
> > > > That should result in the entire array being zeroed.
> > > 
> > > It does, but it also causes the whole struct to be cleared.
> > 
> > Sure, but it's also the minimal diff, and it's easier to read. This was
> > what the code was intended to be initially.
> > 
> > > With the memset, only used_mask gets cleared.
> > 
> > Is there an appreciable difference between the two performance-wise?
> 
> I dunno. It is 3 strp insns vs 1 str.
> If you want the static init, I'll send another patch.

I'd prefer the designated initializer to the memset.

Thanks,
Mark.

Re: [PATCH] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

> > > > >  validate_group(struct perf_event *event)
> > > > >  {
> > > > >  	struct perf_event *sibling, *leader = event->group_leader;
> > > > > -	struct cci_pmu_hw_events fake_pmu = {
> > > > > -		/*
> > > > > -		 * Initialise the fake PMU. We only need to populate the
> > > > > -		 * used_mask for the purposes of validation.
> > > > > -		 */
> > > > > -		.used_mask = CPU_BITS_NONE,
> > > > 
> > > > Can we not simply change this to:
> > > > 
> > > > 		.used_mask = { 0 },
> > > > 
> > > > That should result in the entire array being zeroed.
> > > 
> > > It does, but it also causes the whole struct to be cleared.
> > 
> > Sure, but it's also the minimal diff, and it's easier to read. This was
> > what the code was intended to be initially.
> > 
> > > With the memset, only used_mask gets cleared.
> > 
> > Is there an appreciable difference between the two performance-wise?
> 
> I dunno. It is 3 strp insns vs 1 str.
> If you want the static init, I'll send another patch.

I'd prefer the designated initializer to the memset.

Thanks,
Mark.

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

Currently in validate_group(), there is a static initializer
for fake_pmu.used_mask which is based on CPU_BITS_NONE but
the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
is not correct and will cause a build failure if NR_CPUS
is set high enough to make CPU_BITS_NONE larger than used_mask.

Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/bus/arm-cci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..759e41c 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
 		 * Initialise the fake PMU. We only need to populate the
 		 * used_mask for the purposes of validation.
 		 */
-		.used_mask = CPU_BITS_NONE,
+		.used_mask = { 0 },
 	};
 
 	if (!validate_event(&fake_pmu, leader))
-- 
1.8.3.1

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

Currently in validate_group(), there is a static initializer
for fake_pmu.used_mask which is based on CPU_BITS_NONE but
the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
is not correct and will cause a build failure if NR_CPUS
is set high enough to make CPU_BITS_NONE larger than used_mask.

Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/bus/arm-cci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..759e41c 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
 		 * Initialise the fake PMU. We only need to populate the
 		 * used_mask for the purposes of validation.
 		 */
-		.used_mask = CPU_BITS_NONE,
+		.used_mask = { 0 },
 	};
 
 	if (!validate_event(&fake_pmu, leader))
-- 
1.8.3.1

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Arnd, Olof, are you happy to take this via arm-soc?

My (broken) patch went via Will's tree because of a perf dependency, but
other CCI patches have gone via you guys.

Thanks,
Mark.

> ---
>  drivers/bus/arm-cci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..759e41c 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>  		 * Initialise the fake PMU. We only need to populate the
>  		 * used_mask for the purposes of validation.
>  		 */
> -		.used_mask = CPU_BITS_NONE,
> +		.used_mask = { 0 },
>  	};
>  
>  	if (!validate_event(&fake_pmu, leader))
> -- 
> 1.8.3.1
> 

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
> 
> Signed-off-by: Mark Salter <msalter@redhat.com>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Arnd, Olof, are you happy to take this via arm-soc?

My (broken) patch went via Will's tree because of a perf dependency, but
other CCI patches have gone via you guys.

Thanks,
Mark.

> ---
>  drivers/bus/arm-cci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..759e41c 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>  		 * Initialise the fake PMU. We only need to populate the
>  		 * used_mask for the purposes of validation.
>  		 */
> -		.used_mask = CPU_BITS_NONE,
> +		.used_mask = { 0 },
>  	};
>  
>  	if (!validate_event(&fake_pmu, leader))
> -- 
> 1.8.3.1
> 

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Will Deacon]

On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > 
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Arnd, Olof, are you happy to take this via arm-soc?
> 
> My (broken) patch went via Will's tree because of a perf dependency, but
> other CCI patches have gone via you guys.

Yeah, arm-soc is the best place for this. You should resend to
arm at kernel.org as a new patch with the relevant acks.

Will

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Will Deacon]

On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> > 
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Arnd, Olof, are you happy to take this via arm-soc?
> 
> My (broken) patch went via Will's tree because of a perf dependency, but
> other CCI patches have gone via you guys.

Yeah, arm-soc is the best place for this. You should resend to
arm@kernel.org as a new patch with the relevant acks.

Will

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 13/04/15 13:41, Will Deacon wrote:
> On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
>> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
>>> Currently in validate_group(), there is a static initializer
>>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
>>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
>>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
>>> is not correct and will cause a build failure if NR_CPUS
>>> is set high enough to make CPU_BITS_NONE larger than used_mask.
>>>
>>> Signed-off-by: Mark Salter <msalter@redhat.com>
>>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>
>> Arnd, Olof, are you happy to take this via arm-soc?
>>
>> My (broken) patch went via Will's tree because of a perf dependency, but
>> other CCI patches have gone via you guys.
>
> Yeah, arm-soc is the best place for this. You should resend to
> arm at kernel.org as a new patch with the relevant acks.
Mark

Could you please send this to arm-soc as suggested by Will, with the 
relevant acks/reviews  ?

Thanks
Suzuki

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 13/04/15 13:41, Will Deacon wrote:
> On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
>> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
>>> Currently in validate_group(), there is a static initializer
>>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
>>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
>>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
>>> is not correct and will cause a build failure if NR_CPUS
>>> is set high enough to make CPU_BITS_NONE larger than used_mask.
>>>
>>> Signed-off-by: Mark Salter <msalter@redhat.com>
>>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>
>> Arnd, Olof, are you happy to take this via arm-soc?
>>
>> My (broken) patch went via Will's tree because of a perf dependency, but
>> other CCI patches have gone via you guys.
>
> Yeah, arm-soc is the best place for this. You should resend to
> arm@kernel.org as a new patch with the relevant acks.
Mark

Could you please send this to arm-soc as suggested by Will, with the 
relevant acks/reviews  ?

Thanks
Suzuki

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 11:55 +0100, Suzuki K. Poulose wrote:
> On 13/04/15 13:41, Will Deacon wrote:
> > On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
> >> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> >>> Currently in validate_group(), there is a static initializer
> >>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> >>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> >>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> >>> is not correct and will cause a build failure if NR_CPUS
> >>> is set high enough to make CPU_BITS_NONE larger than used_mask.
> >>>
> >>> Signed-off-by: Mark Salter <msalter@redhat.com>
> >>
> >> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> >>
> >> Arnd, Olof, are you happy to take this via arm-soc?
> >>
> >> My (broken) patch went via Will's tree because of a perf dependency, but
> >> other CCI patches have gone via you guys.
> >
> > Yeah, arm-soc is the best place for this. You should resend to
> > arm at kernel.org as a new patch with the relevant acks.
> Mark
> 
> Could you please send this to arm-soc as suggested by Will, with the 
> relevant acks/reviews  ?
> 

I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
address? I got the cc:

From: Mark Salter <msalter@redhat.com>
To: arm at kernel.org
Cc: Mark Salter <msalter@redhat.com>
Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
Date: Tue, 28 Apr 2015 13:09:32 -0400
Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 11:55 +0100, Suzuki K. Poulose wrote:
> On 13/04/15 13:41, Will Deacon wrote:
> > On Thu, Apr 09, 2015 at 04:36:29PM +0100, Mark Rutland wrote:
> >> On Thu, Apr 09, 2015 at 03:57:05PM +0100, Mark Salter wrote:
> >>> Currently in validate_group(), there is a static initializer
> >>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> >>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> >>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> >>> is not correct and will cause a build failure if NR_CPUS
> >>> is set high enough to make CPU_BITS_NONE larger than used_mask.
> >>>
> >>> Signed-off-by: Mark Salter <msalter@redhat.com>
> >>
> >> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> >>
> >> Arnd, Olof, are you happy to take this via arm-soc?
> >>
> >> My (broken) patch went via Will's tree because of a perf dependency, but
> >> other CCI patches have gone via you guys.
> >
> > Yeah, arm-soc is the best place for this. You should resend to
> > arm@kernel.org as a new patch with the relevant acks.
> Mark
> 
> Could you please send this to arm-soc as suggested by Will, with the 
> relevant acks/reviews  ?
> 

I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
address? I got the cc:

From: Mark Salter <msalter@redhat.com>
To: arm@kernel.org
Cc: Mark Salter <msalter@redhat.com>
Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
Date: Tue, 28 Apr 2015 13:09:32 -0400
Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

> > Could you please send this to arm-soc as suggested by Will, with the 
> > relevant acks/reviews  ?
> > 
> 
> I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
> address? I got the cc:
> 
> From: Mark Salter <msalter@redhat.com>
> To: arm at kernel.org
> Cc: Mark Salter <msalter@redhat.com>
> Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> Date: Tue, 28 Apr 2015 13:09:32 -0400
> Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>

That's the right address, but that only goes to the maintainers, and
doesn't get copied to any list. In future, please Cc linux-arm-kernel in
addition.

Thanks,
Mark.

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

> > Could you please send this to arm-soc as suggested by Will, with the 
> > relevant acks/reviews  ?
> > 
> 
> I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
> address? I got the cc:
> 
> From: Mark Salter <msalter@redhat.com>
> To: arm@kernel.org
> Cc: Mark Salter <msalter@redhat.com>
> Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> Date: Tue, 28 Apr 2015 13:09:32 -0400
> Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>

That's the right address, but that only goes to the maintainers, and
doesn't get copied to any list. In future, please Cc linux-arm-kernel in
addition.

Thanks,
Mark.

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > Could you please send this to arm-soc as suggested by Will, with the 
> > > relevant acks/reviews  ?
> > > 
> > 
> > I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
> > address? I got the cc:
> > 
> > From: Mark Salter <msalter@redhat.com>
> > To: arm at kernel.org
> > Cc: Mark Salter <msalter@redhat.com>
> > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> 
> That's the right address, but that only goes to the maintainers, and
> doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> addition.

That's where I sent it originally.

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > Could you please send this to arm-soc as suggested by Will, with the 
> > > relevant acks/reviews  ?
> > > 
> > 
> > I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
> > address? I got the cc:
> > 
> > From: Mark Salter <msalter@redhat.com>
> > To: arm@kernel.org
> > Cc: Mark Salter <msalter@redhat.com>
> > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> 
> That's the right address, but that only goes to the maintainers, and
> doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> addition.

That's where I sent it originally.

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > relevant acks/reviews  ?
> > > > 
> > > 
> > > I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
> > > address? I got the cc:
> > > 
> > > From: Mark Salter <msalter@redhat.com>
> > > To: arm at kernel.org
> > > Cc: Mark Salter <msalter@redhat.com>
> > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > 
> > That's the right address, but that only goes to the maintainers, and
> > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > addition.
> 
> That's where I sent it originally.

Sure, but it's good to Cc when sending to arm-soc so as to make it
visible that the patches have been sent. Doing so avoids the necessity
of queries like Suzuki's, and makes it possible for others to reply to
the version sent to arm at kernel.org in the case of conflicts or other
issues.

Thanks,
Mark.

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Rutland]

On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > relevant acks/reviews  ?
> > > > 
> > > 
> > > I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
> > > address? I got the cc:
> > > 
> > > From: Mark Salter <msalter@redhat.com>
> > > To: arm@kernel.org
> > > Cc: Mark Salter <msalter@redhat.com>
> > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > 
> > That's the right address, but that only goes to the maintainers, and
> > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > addition.
> 
> That's where I sent it originally.

Sure, but it's good to Cc when sending to arm-soc so as to make it
visible that the patches have been sent. Doing so avoids the necessity
of queries like Suzuki's, and makes it possible for others to reply to
the version sent to arm@kernel.org in the case of conflicts or other
issues.

Thanks,
Mark.

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 15:38 +0100, Mark Rutland wrote:
> On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> > On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > > relevant acks/reviews  ?
> > > > > 
> > > > 
> > > > I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
> > > > address? I got the cc:
> > > > 
> > > > From: Mark Salter <msalter@redhat.com>
> > > > To: arm at kernel.org
> > > > Cc: Mark Salter <msalter@redhat.com>
> > > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > > 
> > > That's the right address, but that only goes to the maintainers, and
> > > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > > addition.
> > 
> > That's where I sent it originally.
> 
> Sure, but it's good to Cc when sending to arm-soc so as to make it
> visible that the patches have been sent. Doing so avoids the necessity
> of queries like Suzuki's, and makes it possible for others to reply to
> the version sent to arm at kernel.org in the case of conflicts or other
> issues.

But why did it need to be sent to a private maintainer's list in the
first place? I think that the destination addresses of the original
posting was perfectly reasonable given output from get_maintainer.pl
and that sending me to a private list was an unnecessary hoop to
jump through.

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Mark Salter]

On Thu, 2015-04-30 at 15:38 +0100, Mark Rutland wrote:
> On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> > On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > > relevant acks/reviews  ?
> > > > > 
> > > > 
> > > > I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
> > > > address? I got the cc:
> > > > 
> > > > From: Mark Salter <msalter@redhat.com>
> > > > To: arm@kernel.org
> > > > Cc: Mark Salter <msalter@redhat.com>
> > > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > > 
> > > That's the right address, but that only goes to the maintainers, and
> > > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > > addition.
> > 
> > That's where I sent it originally.
> 
> Sure, but it's good to Cc when sending to arm-soc so as to make it
> visible that the patches have been sent. Doing so avoids the necessity
> of queries like Suzuki's, and makes it possible for others to reply to
> the version sent to arm@kernel.org in the case of conflicts or other
> issues.

But why did it need to be sent to a private maintainer's list in the
first place? I think that the destination addresses of the original
posting was perfectly reasonable given output from get_maintainer.pl
and that sending me to a private list was an unnecessary hoop to
jump through.

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Arnd Bergmann]

On Thursday 30 April 2015 10:46:13 Mark Salter wrote:
> On Thu, 2015-04-30 at 15:38 +0100, Mark Rutland wrote:
> > On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> > > On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > > > relevant acks/reviews  ?
> > > > > > 
> > > > > 
> > > > > I sent it on Tuesday. Did it not show up? Is arm at kernel.org the correct
> > > > > address? I got the cc:
> > > > > 
> > > > > From: Mark Salter <msalter@redhat.com>
> > > > > To: arm at kernel.org
> > > > > Cc: Mark Salter <msalter@redhat.com>
> > > > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > > > 
> > > > That's the right address, but that only goes to the maintainers, and
> > > > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > > > addition.
> > > 
> > > That's where I sent it originally.
> > 
> > Sure, but it's good to Cc when sending to arm-soc so as to make it
> > visible that the patches have been sent. Doing so avoids the necessity
> > of queries like Suzuki's, and makes it possible for others to reply to
> > the version sent to arm at kernel.org in the case of conflicts or other
> > issues.
> 
> But why did it need to be sent to a private maintainer's list in the
> first place? I think that the destination addresses of the original
> posting was perfectly reasonable given output from get_maintainer.pl
> and that sending me to a private list was an unnecessary hoop to
> jump through.

The purpose of the arm at kernel.org alias is for subarch maintainers to send
us stuff, it's not really meant for normal developers, unless specifically
advised by a maintainer. Each file we maintain through arm-soc normally
belongs to one subarch, so we tend to not pick up any patches on the mailing
list and instead wait for that subarch maintainer to pick them up and forward
the changes to us.

That model model breaks down to some degree for drivers/bus, in particular
for stuff that is not specific to just one SoC. I have the patch in my
todo list now, sorry about missing that earlier. We should probably come up
with a better way to handle patches like this one.

	Arnd

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Arnd Bergmann]

On Thursday 30 April 2015 10:46:13 Mark Salter wrote:
> On Thu, 2015-04-30 at 15:38 +0100, Mark Rutland wrote:
> > On Thu, Apr 30, 2015 at 03:03:07PM +0100, Mark Salter wrote:
> > > On Thu, 2015-04-30 at 14:33 +0100, Mark Rutland wrote:
> > > > > > Could you please send this to arm-soc as suggested by Will, with the 
> > > > > > relevant acks/reviews  ?
> > > > > > 
> > > > > 
> > > > > I sent it on Tuesday. Did it not show up? Is arm@kernel.org the correct
> > > > > address? I got the cc:
> > > > > 
> > > > > From: Mark Salter <msalter@redhat.com>
> > > > > To: arm@kernel.org
> > > > > Cc: Mark Salter <msalter@redhat.com>
> > > > > Subject: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()
> > > > > Date: Tue, 28 Apr 2015 13:09:32 -0400
> > > > > Message-Id: <1430240972-16386-1-git-send-email-msalter@redhat.com>
> > > > 
> > > > That's the right address, but that only goes to the maintainers, and
> > > > doesn't get copied to any list. In future, please Cc linux-arm-kernel in
> > > > addition.
> > > 
> > > That's where I sent it originally.
> > 
> > Sure, but it's good to Cc when sending to arm-soc so as to make it
> > visible that the patches have been sent. Doing so avoids the necessity
> > of queries like Suzuki's, and makes it possible for others to reply to
> > the version sent to arm@kernel.org in the case of conflicts or other
> > issues.
> 
> But why did it need to be sent to a private maintainer's list in the
> first place? I think that the destination addresses of the original
> posting was perfectly reasonable given output from get_maintainer.pl
> and that sending me to a private list was an unnecessary hoop to
> jump through.

The purpose of the arm@kernel.org alias is for subarch maintainers to send
us stuff, it's not really meant for normal developers, unless specifically
advised by a maintainer. Each file we maintain through arm-soc normally
belongs to one subarch, so we tend to not pick up any patches on the mailing
list and instead wait for that subarch maintainer to pick them up and forward
the changes to us.

That model model breaks down to some degree for drivers/bus, in particular
for stuff that is not specific to just one SoC. I have the patch in my
todo list now, sorry about missing that earlier. We should probably come up
with a better way to handle patches like this one.

	Arnd

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 09/04/15 15:57, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>   drivers/bus/arm-cci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..759e41c 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>   		 * Initialise the fake PMU. We only need to populate the
>   		 * used_mask for the purposes of validation.
>   		 */
> -		.used_mask = CPU_BITS_NONE,
> +		.used_mask = { 0 },
>   	};
>
>   	if (!validate_event(&fake_pmu, leader))
>
I have a series of patches to add CCI-500 PMU, targeting 4.2, which 
changes the used_mask to a pointer and changes it to depend on the 
number of counters available on the CCI PMU.  I will post it in very 
soon here, which could make this patch obsolete. Let me know if you 
don't mind getting this sorted out through my series. I am fine either way.

Regards
Suzuki

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 09/04/15 15:57, Mark Salter wrote:
> Currently in validate_group(), there is a static initializer
> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> is not correct and will cause a build failure if NR_CPUS
> is set high enough to make CPU_BITS_NONE larger than used_mask.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>   drivers/bus/arm-cci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 84fd660..759e41c 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>   		 * Initialise the fake PMU. We only need to populate the
>   		 * used_mask for the purposes of validation.
>   		 */
> -		.used_mask = CPU_BITS_NONE,
> +		.used_mask = { 0 },
>   	};
>
>   	if (!validate_event(&fake_pmu, leader))
>
I have a series of patches to add CCI-500 PMU, targeting 4.2, which 
changes the used_mask to a pointer and changes it to depend on the 
number of counters available on the CCI PMU.  I will post it in very 
soon here, which could make this patch obsolete. Let me know if you 
don't mind getting this sorted out through my series. I am fine either way.

Regards
Suzuki

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Will Deacon]

On Wed, Apr 15, 2015 at 11:44:06AM +0100, Suzuki K. Poulose wrote:
> On 09/04/15 15:57, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> >
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> >   drivers/bus/arm-cci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > index 84fd660..759e41c 100644
> > --- a/drivers/bus/arm-cci.c
> > +++ b/drivers/bus/arm-cci.c
> > @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
> >   		 * Initialise the fake PMU. We only need to populate the
> >   		 * used_mask for the purposes of validation.
> >   		 */
> > -		.used_mask = CPU_BITS_NONE,
> > +		.used_mask = { 0 },
> >   	};
> >
> >   	if (!validate_event(&fake_pmu, leader))
> >
> I have a series of patches to add CCI-500 PMU, targeting 4.2, which 
> changes the used_mask to a pointer and changes it to depend on the 
> number of counters available on the CCI PMU.  I will post it in very 
> soon here, which could make this patch obsolete. Let me know if you 
> don't mind getting this sorted out through my series. I am fine either way.

I'd rather have the fix in for 4.1 and then have your CCI-500 patches based
on that.

Will

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Will Deacon]

On Wed, Apr 15, 2015 at 11:44:06AM +0100, Suzuki K. Poulose wrote:
> On 09/04/15 15:57, Mark Salter wrote:
> > Currently in validate_group(), there is a static initializer
> > for fake_pmu.used_mask which is based on CPU_BITS_NONE but
> > the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
> > CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
> > is not correct and will cause a build failure if NR_CPUS
> > is set high enough to make CPU_BITS_NONE larger than used_mask.
> >
> > Signed-off-by: Mark Salter <msalter@redhat.com>
> > ---
> >   drivers/bus/arm-cci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > index 84fd660..759e41c 100644
> > --- a/drivers/bus/arm-cci.c
> > +++ b/drivers/bus/arm-cci.c
> > @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
> >   		 * Initialise the fake PMU. We only need to populate the
> >   		 * used_mask for the purposes of validation.
> >   		 */
> > -		.used_mask = CPU_BITS_NONE,
> > +		.used_mask = { 0 },
> >   	};
> >
> >   	if (!validate_event(&fake_pmu, leader))
> >
> I have a series of patches to add CCI-500 PMU, targeting 4.2, which 
> changes the used_mask to a pointer and changes it to depend on the 
> number of counters available on the CCI PMU.  I will post it in very 
> soon here, which could make this patch obsolete. Let me know if you 
> don't mind getting this sorted out through my series. I am fine either way.

I'd rather have the fix in for 4.1 and then have your CCI-500 patches based
on that.

Will

[PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 15/04/15 12:58, Will Deacon wrote:
> On Wed, Apr 15, 2015 at 11:44:06AM +0100, Suzuki K. Poulose wrote:
>> On 09/04/15 15:57, Mark Salter wrote:
>>> Currently in validate_group(), there is a static initializer
>>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
>>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
>>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
>>> is not correct and will cause a build failure if NR_CPUS
>>> is set high enough to make CPU_BITS_NONE larger than used_mask.
>>>
>>> Signed-off-by: Mark Salter <msalter@redhat.com>
>>> ---
>>>    drivers/bus/arm-cci.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>> index 84fd660..759e41c 100644
>>> --- a/drivers/bus/arm-cci.c
>>> +++ b/drivers/bus/arm-cci.c
>>> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>>>    		 * Initialise the fake PMU. We only need to populate the
>>>    		 * used_mask for the purposes of validation.
>>>    		 */
>>> -		.used_mask = CPU_BITS_NONE,
>>> +		.used_mask = { 0 },
>>>    	};
>>>
>>>    	if (!validate_event(&fake_pmu, leader))
>>>
>> I have a series of patches to add CCI-500 PMU, targeting 4.2, which
>> changes the used_mask to a pointer and changes it to depend on the
>> number of counters available on the CCI PMU.  I will post it in very
>> soon here, which could make this patch obsolete. Let me know if you
>> don't mind getting this sorted out through my series. I am fine either way.
>
> I'd rather have the fix in for 4.1 and then have your CCI-500 patches based
> on that.
>
> Will
>
OK, will rebase my patches on top of this one.

Thanks
Suzuki

Re: [PATCH V2] drivers: CCI: fix used_mask init in validate_group()

[Suzuki K. Poulose]

On 15/04/15 12:58, Will Deacon wrote:
> On Wed, Apr 15, 2015 at 11:44:06AM +0100, Suzuki K. Poulose wrote:
>> On 09/04/15 15:57, Mark Salter wrote:
>>> Currently in validate_group(), there is a static initializer
>>> for fake_pmu.used_mask which is based on CPU_BITS_NONE but
>>> the used_mask array size is based on CCI_PMU_MAX_HW_EVENTS.
>>> CCI_PMU_MAX_HW_EVENTS is not based on NR_CPUS, so CPU_BITS_NONE
>>> is not correct and will cause a build failure if NR_CPUS
>>> is set high enough to make CPU_BITS_NONE larger than used_mask.
>>>
>>> Signed-off-by: Mark Salter <msalter@redhat.com>
>>> ---
>>>    drivers/bus/arm-cci.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>> index 84fd660..759e41c 100644
>>> --- a/drivers/bus/arm-cci.c
>>> +++ b/drivers/bus/arm-cci.c
>>> @@ -684,7 +684,7 @@ validate_group(struct perf_event *event)
>>>    		 * Initialise the fake PMU. We only need to populate the
>>>    		 * used_mask for the purposes of validation.
>>>    		 */
>>> -		.used_mask = CPU_BITS_NONE,
>>> +		.used_mask = { 0 },
>>>    	};
>>>
>>>    	if (!validate_event(&fake_pmu, leader))
>>>
>> I have a series of patches to add CCI-500 PMU, targeting 4.2, which
>> changes the used_mask to a pointer and changes it to depend on the
>> number of counters available on the CCI PMU.  I will post it in very
>> soon here, which could make this patch obsolete. Let me know if you
>> don't mind getting this sorted out through my series. I am fine either way.
>
> I'd rather have the fix in for 4.1 and then have your CCI-500 patches based
> on that.
>
> Will
>
OK, will rebase my patches on top of this one.

Thanks
Suzuki