Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Christoffer Dall]

On Tue, Mar 15, 2016 at 12:26:29PM +, Julien Grall wrote:
> Hi Christoffer,
> 
> On 09/03/16 05:39, Christoffer Dall wrote:
> >On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:
> >>Even though all the variables aren't marked with __initdata, they are
> >>only used during initialization. So the structure is marked with
> >>__initdata.
> >
> >Not sure I understand this commit message.
> >
> >As I see it, this commit includes two changes:
> >
> >1. Mark the variables only used during init with __initdata
> >
> >2. Move the variables into a structure
> >
> >If I get that right, can you argue for both changes?
> 
> What about:
> 
> "The ACPI code requires to use global variables in order to collect
> information from the tables.
> 
> To make clear those variables are ACPI specific, gather all of them
> in a single structure.
> 
> Furthermore, even if some of the variables are not marked with
> __initdata, they are all only used during the initialization.
> Therefore, the new variable, which hold the structure, can be marked
> __initdata."
> 
Looks ok (the part about some of the variables not marked with
__initdata is confusing but I think I understand what you mean).

Thanks,
-Christoffer

Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Christoffer Dall]

On Tue, Mar 15, 2016 at 12:26:29PM +, Julien Grall wrote:
> Hi Christoffer,
> 
> On 09/03/16 05:39, Christoffer Dall wrote:
> >On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:
> >>Even though all the variables aren't marked with __initdata, they are
> >>only used during initialization. So the structure is marked with
> >>__initdata.
> >
> >Not sure I understand this commit message.
> >
> >As I see it, this commit includes two changes:
> >
> >1. Mark the variables only used during init with __initdata
> >
> >2. Move the variables into a structure
> >
> >If I get that right, can you argue for both changes?
> 
> What about:
> 
> "The ACPI code requires to use global variables in order to collect
> information from the tables.
> 
> To make clear those variables are ACPI specific, gather all of them
> in a single structure.
> 
> Furthermore, even if some of the variables are not marked with
> __initdata, they are all only used during the initialization.
> Therefore, the new variable, which hold the structure, can be marked
> __initdata."
> 
Looks ok (the part about some of the variables not marked with
__initdata is confusing but I think I understand what you mean).

Thanks,
-Christoffer

Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Julien Grall]

Hi Christoffer,

On 09/03/16 05:39, Christoffer Dall wrote:

On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:

Even though all the variables aren't marked with __initdata, they are
only used during initialization. So the structure is marked with
__initdata.


Not sure I understand this commit message.

As I see it, this commit includes two changes:

1. Mark the variables only used during init with __initdata

2. Move the variables into a structure

If I get that right, can you argue for both changes?


What about:

"The ACPI code requires to use global variables in order to collect 
information from the tables.


To make clear those variables are ACPI specific, gather all of them in a 
single structure.


Furthermore, even if some of the variables are not marked with 
__initdata, they are all only used during the initialization. Therefore, 
the new variable, which hold the structure, can be marked __initdata."


Cheers,

--
Julien Grall

Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Julien Grall]

Hi Christoffer,

On 09/03/16 05:39, Christoffer Dall wrote:

On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:

Even though all the variables aren't marked with __initdata, they are
only used during initialization. So the structure is marked with
__initdata.


Not sure I understand this commit message.

As I see it, this commit includes two changes:

1. Mark the variables only used during init with __initdata

2. Move the variables into a structure

If I get that right, can you argue for both changes?


What about:

"The ACPI code requires to use global variables in order to collect 
information from the tables.


To make clear those variables are ACPI specific, gather all of them in a 
single structure.


Furthermore, even if some of the variables are not marked with 
__initdata, they are all only used during the initialization. Therefore, 
the new variable, which hold the structure, can be marked __initdata."


Cheers,

--
Julien Grall

Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Christoffer Dall]

On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:
> Even though all the variables aren't marked with __initdata, they are
> only used during initialization. So the structure is marked with
> __initdata.

Not sure I understand this commit message.

As I see it, this commit includes two changes:

1. Mark the variables only used during init with __initdata

2. Move the variables into a structure

If I get that right, can you argue for both changes?

Thanks,
-Christoffer

> 
> Signed-off-by: Julien Grall 
> 
> ---
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Marc Zyngier 
> 
> Changes in v3:
> - Patch added
> ---
>  drivers/irqchip/irq-gic-v3.c | 60 
> 
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5b7d3c2..50e87e6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -968,19 +968,22 @@ out_unmap_dist:
>  IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>  
>  #ifdef CONFIG_ACPI
> -static void __iomem *dist_base;
> -static struct redist_region *redist_regs __initdata;
> -static u32 nr_redist_regions __initdata;
> -static bool single_redist;
> +static struct
> +{
> + void __iomem *dist_base;
> + struct redist_region *redist_regs;
> + u32 nr_redist_regions;
> + bool single_redist;
> +} acpi_data __initdata;
>  
>  static void __init
>  gic_acpi_register_redist(phys_addr_t phys_base, void __iomem *redist_base)
>  {
>   static int count = 0;
>  
> - redist_regs[count].phys_base = phys_base;
> - redist_regs[count].redist_base = redist_base;
> - redist_regs[count].single_redist = single_redist;
> + acpi_data.redist_regs[count].phys_base = phys_base;
> + acpi_data.redist_regs[count].redist_base = redist_base;
> + acpi_data.redist_regs[count].single_redist = acpi_data.single_redist;
>   count++;
>  }
>  
> @@ -1008,7 +1011,7 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header 
> *header,
>  {
>   struct acpi_madt_generic_interrupt *gicc =
>   (struct acpi_madt_generic_interrupt *)header;
> - u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> + u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & 
> GIC_PIDR2_ARCH_MASK;
>   u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
>   void __iomem *redist_base;
>  
> @@ -1025,7 +1028,7 @@ static int __init gic_acpi_collect_gicr_base(void)
>   acpi_tbl_entry_handler redist_parser;
>   enum acpi_madt_type type;
>  
> - if (single_redist) {
> + if (acpi_data.single_redist) {
>   type = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
>   redist_parser = gic_acpi_parse_madt_gicc;
>   } else {
> @@ -1076,14 +1079,14 @@ static int __init gic_acpi_count_gicr_regions(void)
>   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> gic_acpi_match_gicr, 0);
>   if (count > 0) {
> - single_redist = false;
> + acpi_data.single_redist = false;
>   return count;
>   }
>  
>   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> gic_acpi_match_gicc, 0);
>   if (count > 0)
> - single_redist = true;
> + acpi_data.single_redist = true;
>  
>   return count;
>  }
> @@ -1103,7 +1106,7 @@ static bool __init acpi_validate_gic_table(struct 
> acpi_subtable_header *header,
>   if (count <= 0)
>   return false;
>  
> - nr_redist_regions = count;
> + acpi_data.nr_redist_regions = count;
>   return true;
>  }
>  
> @@ -1114,25 +1117,28 @@ gic_acpi_init(struct acpi_subtable_header *header, 
> const unsigned long end)
>  {
>   struct acpi_madt_generic_distributor *dist;
>   struct fwnode_handle *domain_handle;
> + size_t size;
>   int i, err;
>  
>   /* Get distributor base address */
>   dist = (struct acpi_madt_generic_distributor *)header;
> - dist_base = ioremap(dist->base_address, ACPI_GICV3_DIST_MEM_SIZE);
> - if (!dist_base) {
> + acpi_data.dist_base = ioremap(dist->base_address,
> +   ACPI_GICV3_DIST_MEM_SIZE);
> + if (!acpi_data.dist_base) {
>   pr_err("Unable to map GICD registers\n");
>   return -ENOMEM;
>   }
>  
> - err = gic_validate_dist_version(dist_base);
> + err = gic_validate_dist_version(acpi_data.dist_base);
>   if (err) {
> - pr_err("No distributor detected at @%p, giving up", dist_base);
> + pr_err("No distributor detected at @%p, giving up",
> +acpi_data.dist_base);
>   goto out_dist_unmap;
>   }
>  
> - redist_regs = kzalloc(sizeof(*redist_regs) * 

Re: [PATCH v3 5/9] irqchip/gic-v3: Gather all ACPI specific data in a single structure

[Christoffer Dall]

On Tue, Mar 08, 2016 at 11:29:29AM +, Julien Grall wrote:
> Even though all the variables aren't marked with __initdata, they are
> only used during initialization. So the structure is marked with
> __initdata.

Not sure I understand this commit message.

As I see it, this commit includes two changes:

1. Mark the variables only used during init with __initdata

2. Move the variables into a structure

If I get that right, can you argue for both changes?

Thanks,
-Christoffer

> 
> Signed-off-by: Julien Grall 
> 
> ---
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Marc Zyngier 
> 
> Changes in v3:
> - Patch added
> ---
>  drivers/irqchip/irq-gic-v3.c | 60 
> 
>  1 file changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5b7d3c2..50e87e6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -968,19 +968,22 @@ out_unmap_dist:
>  IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>  
>  #ifdef CONFIG_ACPI
> -static void __iomem *dist_base;
> -static struct redist_region *redist_regs __initdata;
> -static u32 nr_redist_regions __initdata;
> -static bool single_redist;
> +static struct
> +{
> + void __iomem *dist_base;
> + struct redist_region *redist_regs;
> + u32 nr_redist_regions;
> + bool single_redist;
> +} acpi_data __initdata;
>  
>  static void __init
>  gic_acpi_register_redist(phys_addr_t phys_base, void __iomem *redist_base)
>  {
>   static int count = 0;
>  
> - redist_regs[count].phys_base = phys_base;
> - redist_regs[count].redist_base = redist_base;
> - redist_regs[count].single_redist = single_redist;
> + acpi_data.redist_regs[count].phys_base = phys_base;
> + acpi_data.redist_regs[count].redist_base = redist_base;
> + acpi_data.redist_regs[count].single_redist = acpi_data.single_redist;
>   count++;
>  }
>  
> @@ -1008,7 +1011,7 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header 
> *header,
>  {
>   struct acpi_madt_generic_interrupt *gicc =
>   (struct acpi_madt_generic_interrupt *)header;
> - u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> + u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & 
> GIC_PIDR2_ARCH_MASK;
>   u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
>   void __iomem *redist_base;
>  
> @@ -1025,7 +1028,7 @@ static int __init gic_acpi_collect_gicr_base(void)
>   acpi_tbl_entry_handler redist_parser;
>   enum acpi_madt_type type;
>  
> - if (single_redist) {
> + if (acpi_data.single_redist) {
>   type = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
>   redist_parser = gic_acpi_parse_madt_gicc;
>   } else {
> @@ -1076,14 +1079,14 @@ static int __init gic_acpi_count_gicr_regions(void)
>   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> gic_acpi_match_gicr, 0);
>   if (count > 0) {
> - single_redist = false;
> + acpi_data.single_redist = false;
>   return count;
>   }
>  
>   count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> gic_acpi_match_gicc, 0);
>   if (count > 0)
> - single_redist = true;
> + acpi_data.single_redist = true;
>  
>   return count;
>  }
> @@ -1103,7 +1106,7 @@ static bool __init acpi_validate_gic_table(struct 
> acpi_subtable_header *header,
>   if (count <= 0)
>   return false;
>  
> - nr_redist_regions = count;
> + acpi_data.nr_redist_regions = count;
>   return true;
>  }
>  
> @@ -1114,25 +1117,28 @@ gic_acpi_init(struct acpi_subtable_header *header, 
> const unsigned long end)
>  {
>   struct acpi_madt_generic_distributor *dist;
>   struct fwnode_handle *domain_handle;
> + size_t size;
>   int i, err;
>  
>   /* Get distributor base address */
>   dist = (struct acpi_madt_generic_distributor *)header;
> - dist_base = ioremap(dist->base_address, ACPI_GICV3_DIST_MEM_SIZE);
> - if (!dist_base) {
> + acpi_data.dist_base = ioremap(dist->base_address,
> +   ACPI_GICV3_DIST_MEM_SIZE);
> + if (!acpi_data.dist_base) {
>   pr_err("Unable to map GICD registers\n");
>   return -ENOMEM;
>   }
>  
> - err = gic_validate_dist_version(dist_base);
> + err = gic_validate_dist_version(acpi_data.dist_base);
>   if (err) {
> - pr_err("No distributor detected at @%p, giving up", dist_base);
> + pr_err("No distributor detected at @%p, giving up",
> +acpi_data.dist_base);
>   goto out_dist_unmap;
>   }
>  
> - redist_regs = kzalloc(sizeof(*redist_regs) * nr_redist_regions,
> -   GFP_KERNEL);
> - if