diff mbox

[V10,7/8] irqchip/gicv3-its: Factor out PCI-MSI part that might be reused for ACPI

Message ID 1473152938-17388-8-git-send-email-tn@semihalf.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Nowicki Sept. 6, 2016, 9:08 a.m. UTC 
Firmware agnostic code lands in common functions which do necessary
domain initialization based on unique domain handler. DT specific
code goes to DT specific init call.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 15 deletions(-)

Comments

Thomas Gleixner Sept. 6, 2016, 9:50 a.m. UTC | #1 
On Tue, 6 Sep 2016, Tomasz Nowicki wrote:
> -static int __init its_pci_msi_init(void)
> +static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
> +				       const char *name)
>  {
> -	struct device_node *np;
>  	struct irq_domain *parent;
>  
> +	parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
> +	if (!parent || !msi_get_domain_info(parent)) {
> +		pr_err("%s: Unable to locate ITS domain\n", name);
> +		return -ENXIO;
> +	}
> +
> +	if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
> +				       parent)) {
> +		pr_err("%s: Unable to create PCI domain\n", name);
> +		return -ENOMEM;

So you have error codes here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int __init its_pci_of_msi_init(void)
> +{
> +	struct device_node *np;
> +
>  	for (np = of_find_matching_node(NULL, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
>  		if (!of_property_read_bool(np, "msi-controller"))
>  			continue;
>  
> -		parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
> -		if (!parent || !msi_get_domain_info(parent)) {
> -			pr_err("%s: unable to locate ITS domain\n",
> -			       np->full_name);
> +		if (its_pci_msi_init_one(of_node_to_fwnode(np), np->full_name))
>  			continue;
> -		}
> -
> -		if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
> -					       &its_pci_msi_domain_info,
> -					       parent)) {
> -			pr_err("%s: unable to create PCI domain\n",
> -			       np->full_name);
> -			continue;
> -		}
>  
>  		pr_info("PCI/MSI: %s domain created\n", np->full_name);
>  	}
>  
>  	return 0;

And no matter what you return success

>  }
> +
> +static int __init its_pci_msi_init(void)
> +{
> +	its_pci_of_msi_init();

Which you subsequently ignore here. What's the point?

Thanks,

	tglx
Tomasz Nowicki Sept. 6, 2016, 10:22 a.m. UTC | #2 
Hi Thomas,

On 06.09.2016 11:50, Thomas Gleixner wrote:
> On Tue, 6 Sep 2016, Tomasz Nowicki wrote:
>> -static int __init its_pci_msi_init(void)
>> +static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
>> +				       const char *name)
>>  {
>> -	struct device_node *np;
>>  	struct irq_domain *parent;
>>
>> +	parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
>> +	if (!parent || !msi_get_domain_info(parent)) {
>> +		pr_err("%s: Unable to locate ITS domain\n", name);
>> +		return -ENXIO;
>> +	}
>> +
>> +	if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
>> +				       parent)) {
>> +		pr_err("%s: Unable to create PCI domain\n", name);
>> +		return -ENOMEM;
>
> So you have error codes here.
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init its_pci_of_msi_init(void)
>> +{
>> +	struct device_node *np;
>> +
>>  	for (np = of_find_matching_node(NULL, its_device_id); np;
>>  	     np = of_find_matching_node(np, its_device_id)) {
>>  		if (!of_property_read_bool(np, "msi-controller"))
>>  			continue;
>>
>> -		parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
>> -		if (!parent || !msi_get_domain_info(parent)) {
>> -			pr_err("%s: unable to locate ITS domain\n",
>> -			       np->full_name);
>> +		if (its_pci_msi_init_one(of_node_to_fwnode(np), np->full_name))
>>  			continue;
>> -		}
>> -
>> -		if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
>> -					       &its_pci_msi_domain_info,
>> -					       parent)) {
>> -			pr_err("%s: unable to create PCI domain\n",
>> -			       np->full_name);
>> -			continue;
>> -		}
>>
>>  		pr_info("PCI/MSI: %s domain created\n", np->full_name);
>>  	}
>>
>>  	return 0;
>
> And no matter what you return success
>
>>  }
>> +
>> +static int __init its_pci_msi_init(void)
>> +{
>> +	its_pci_of_msi_init();
>
> Which you subsequently ignore here. What's the point?
>

Well, this patch is meant to refactor the code. It does not change the 
error handing nor the functionality. However, you have got a point but 
IMO it should be fixed in separate patch if possible.

Thanks,
Tomasz
Rafael J. Wysocki Sept. 6, 2016, 11:29 a.m. UTC | #3 
On Tue, Sep 6, 2016 at 12:22 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> Hi Thomas,
>
>
> On 06.09.2016 11:50, Thomas Gleixner wrote:
>>
>> On Tue, 6 Sep 2016, Tomasz Nowicki wrote:
>>>
>>> -static int __init its_pci_msi_init(void)
>>> +static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
>>> +                                      const char *name)
>>>  {
>>> -       struct device_node *np;
>>>         struct irq_domain *parent;
>>>
>>> +       parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
>>> +       if (!parent || !msi_get_domain_info(parent)) {
>>> +               pr_err("%s: Unable to locate ITS domain\n", name);
>>> +               return -ENXIO;
>>> +       }
>>> +
>>> +       if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
>>> +                                      parent)) {
>>> +               pr_err("%s: Unable to create PCI domain\n", name);
>>> +               return -ENOMEM;
>>
>>
>> So you have error codes here.
>>
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int __init its_pci_of_msi_init(void)
>>> +{
>>> +       struct device_node *np;
>>> +
>>>         for (np = of_find_matching_node(NULL, its_device_id); np;
>>>              np = of_find_matching_node(np, its_device_id)) {
>>>                 if (!of_property_read_bool(np, "msi-controller"))
>>>                         continue;
>>>
>>> -               parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
>>> -               if (!parent || !msi_get_domain_info(parent)) {
>>> -                       pr_err("%s: unable to locate ITS domain\n",
>>> -                              np->full_name);
>>> +               if (its_pci_msi_init_one(of_node_to_fwnode(np),
>>> np->full_name))
>>>                         continue;
>>> -               }
>>> -
>>> -               if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
>>> -                                              &its_pci_msi_domain_info,
>>> -                                              parent)) {
>>> -                       pr_err("%s: unable to create PCI domain\n",
>>> -                              np->full_name);
>>> -                       continue;
>>> -               }
>>>
>>>                 pr_info("PCI/MSI: %s domain created\n", np->full_name);
>>>         }
>>>
>>>         return 0;
>>
>>
>> And no matter what you return success
>>
>>>  }
>>> +
>>> +static int __init its_pci_msi_init(void)
>>> +{
>>> +       its_pci_of_msi_init();
>>
>>
>> Which you subsequently ignore here. What's the point?
>>
>
> Well, this patch is meant to refactor the code. It does not change the error
> handing nor the functionality. However, you have got a point but IMO it
> should be fixed in separate patch if possible.

But you are refactoring the code, so it would make sense to clean it
up while at it.  Perhaps add one more patch to the series?

Thanks,
Rafael
Tomasz Nowicki Sept. 7, 2016, 7:49 a.m. UTC | #4 
On 06.09.2016 13:29, Rafael J. Wysocki wrote:
> On Tue, Sep 6, 2016 at 12:22 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>> Hi Thomas,
>>
>>
>> On 06.09.2016 11:50, Thomas Gleixner wrote:
>>>
>>> On Tue, 6 Sep 2016, Tomasz Nowicki wrote:
>>>>
>>>> -static int __init its_pci_msi_init(void)
>>>> +static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
>>>> +                                      const char *name)
>>>>  {
>>>> -       struct device_node *np;
>>>>         struct irq_domain *parent;
>>>>
>>>> +       parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
>>>> +       if (!parent || !msi_get_domain_info(parent)) {
>>>> +               pr_err("%s: Unable to locate ITS domain\n", name);
>>>> +               return -ENXIO;
>>>> +       }
>>>> +
>>>> +       if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
>>>> +                                      parent)) {
>>>> +               pr_err("%s: Unable to create PCI domain\n", name);
>>>> +               return -ENOMEM;
>>>
>>>
>>> So you have error codes here.
>>>
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int __init its_pci_of_msi_init(void)
>>>> +{
>>>> +       struct device_node *np;
>>>> +
>>>>         for (np = of_find_matching_node(NULL, its_device_id); np;
>>>>              np = of_find_matching_node(np, its_device_id)) {
>>>>                 if (!of_property_read_bool(np, "msi-controller"))
>>>>                         continue;
>>>>
>>>> -               parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
>>>> -               if (!parent || !msi_get_domain_info(parent)) {
>>>> -                       pr_err("%s: unable to locate ITS domain\n",
>>>> -                              np->full_name);
>>>> +               if (its_pci_msi_init_one(of_node_to_fwnode(np),
>>>> np->full_name))
>>>>                         continue;
>>>> -               }
>>>> -
>>>> -               if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
>>>> -                                              &its_pci_msi_domain_info,
>>>> -                                              parent)) {
>>>> -                       pr_err("%s: unable to create PCI domain\n",
>>>> -                              np->full_name);
>>>> -                       continue;
>>>> -               }
>>>>
>>>>                 pr_info("PCI/MSI: %s domain created\n", np->full_name);
>>>>         }
>>>>
>>>>         return 0;
>>>
>>>
>>> And no matter what you return success
>>>
>>>>  }
>>>> +
>>>> +static int __init its_pci_msi_init(void)
>>>> +{
>>>> +       its_pci_of_msi_init();
>>>
>>>
>>> Which you subsequently ignore here. What's the point?
>>>
>>
>> Well, this patch is meant to refactor the code. It does not change the error
>> handing nor the functionality. However, you have got a point but IMO it
>> should be fixed in separate patch if possible.
>
> But you are refactoring the code, so it would make sense to clean it
> up while at it.  Perhaps add one more patch to the series?
>

I have a look at the code again and come to the conclusion the code is 
fine now. We iterate over available its nodes and try to probe driver. 
There might be an error for one of it (print error message) but we keep 
trying for the rest. One of it might be probed successfully. So there is 
no reason to escape in half way. Since its_pci_of_msi_init() always 
returns 0 I can remove return statement so we would have:
void its_pci_of_msi_init(void) instead.

Thanks,
Tomasz
diff mbox

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index aee60ed..d2c2496 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -106,34 +106,48 @@  static struct of_device_id its_device_id[] = {
 	{},
 };
 
-static int __init its_pci_msi_init(void)
+static int __init its_pci_msi_init_one(struct fwnode_handle *handle,
+				       const char *name)
 {
-	struct device_node *np;
 	struct irq_domain *parent;
 
+	parent = irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
+	if (!parent || !msi_get_domain_info(parent)) {
+		pr_err("%s: Unable to locate ITS domain\n", name);
+		return -ENXIO;
+	}
+
+	if (!pci_msi_create_irq_domain(handle, &its_pci_msi_domain_info,
+				       parent)) {
+		pr_err("%s: Unable to create PCI domain\n", name);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int __init its_pci_of_msi_init(void)
+{
+	struct device_node *np;
+
 	for (np = of_find_matching_node(NULL, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
 		if (!of_property_read_bool(np, "msi-controller"))
 			continue;
 
-		parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
-		if (!parent || !msi_get_domain_info(parent)) {
-			pr_err("%s: unable to locate ITS domain\n",
-			       np->full_name);
+		if (its_pci_msi_init_one(of_node_to_fwnode(np), np->full_name))
 			continue;
-		}
-
-		if (!pci_msi_create_irq_domain(of_node_to_fwnode(np),
-					       &its_pci_msi_domain_info,
-					       parent)) {
-			pr_err("%s: unable to create PCI domain\n",
-			       np->full_name);
-			continue;
-		}
 
 		pr_info("PCI/MSI: %s domain created\n", np->full_name);
 	}
 
 	return 0;
 }
+
+static int __init its_pci_msi_init(void)
+{
+	its_pci_of_msi_init();
+
+	return 0;
+}
 early_initcall(its_pci_msi_init);