Re: [PATCH v4 2/4] irqchip/gic: common: add support to device tree based quirks

[Marc Zyngier]

On 10/12/2018 10:23, Srinivas Kandagatla wrote:
> This patch adds support to device tree based quirks based on
> device tree compatible string.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
>  drivers/irqchip/irq-gic-common.c | 12 ++++++++++++
>  drivers/irqchip/irq-gic-common.h |  3 +++
>  drivers/irqchip/irq-gic-v3.c     |  5 +++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index 01e673c680cd..3c93c6f4d1f1 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -36,6 +36,18 @@ void gic_set_kvm_info(const struct gic_kvm_info *info)
>  	gic_kvm_info = info;
>  }
>  
> +void gic_enable_of_quirks(const struct device_node *np,
> +			  const struct gic_quirk *quirks, void *data)
> +{
> +	for (; quirks->desc; quirks++) {

So you expect quirks->desc to be NULL at some point, just like for any
other quirk table we have.

> +		if (!of_device_is_compatible(np, quirks->compatible))
> +			continue;
> +		if (quirks->init(data))
> +			pr_info("GIC: enabling workaround for %s\n",
> +				quirks->desc);
> +	}
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  		void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 3919cd7c5285..97e58fb6c232 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -23,6 +23,7 @@
>  
>  struct gic_quirk {
>  	const char *desc;
> +	const char *compatible;
>  	bool (*init)(void *data);
>  	u32 iidr;
>  	u32 mask;
> @@ -35,6 +36,8 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
>  void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  		void *data);
> +void gic_enable_of_quirks(const struct device_node *np,
> +			  const struct gic_quirk *quirks, void *data);
>  
>  void gic_set_kvm_info(const struct gic_kvm_info *info);
>  
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 29e9d47be97d..c95796fa4de6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1271,6 +1271,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  	gic_set_kvm_info(&gic_v3_kvm_info);
>  }
>  
> +static const struct gic_quirk gic_quirks[] = {
> +};

... and yet here you provide an empty table. That's not going to work
very well. You definitely need to have an empty entry at the end of the
array, always.

I guess you want to test your code on a non affected platform, and I'm
pretty sure you'll see it exploding.

> +
>  static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>  	void __iomem *dist_base;
> @@ -1318,6 +1321,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
>  		redist_stride = 0;
>  
> +	gic_enable_of_quirks(node, gic_quirks, &gic_data);
> +
>  	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
>  			     redist_stride, &node->fwnode);
>  	if (err)
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...