Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1615478 > unrolled thread

[PATCH] EDAC, altera: Fix peripheral warnings for Cyclone5

Back to article view | Back to linux.kernel

Contents

  [PATCH] EDAC, altera: Fix peripheral warnings for Cyclone5 thor.thayer@linux.intel.com - 2017-04-03 21:00 +0200
    Re: [PATCH] EDAC, altera: Fix peripheral warnings for Cyclone5 Borislav Petkov <bp@alien8.de> - 2017-04-04 12:00 +0200
      Re: [PATCH] EDAC, altera: Fix peripheral warnings for Cyclone5 Thor Thayer <thor.thayer@linux.intel.com> - 2017-04-04 16:30 +0200

#1615478 — [PATCH] EDAC, altera: Fix peripheral warnings for Cyclone5

From: Thor Thayer <thor.thayer@linux.intel.com>

The peripherals EDACs only exist on the Arria10 SoCFPGA. The Cyclone5
initialization has EDAC warnings when the peripherals aren't found
in the device tree. Fix by checking for Arria10 in the init functions.

Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
 drivers/edac/altera_edac.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 6421cc3..98e83f5 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1024,13 +1024,30 @@ static int __maybe_unused altr_init_memory_port(void __iomem *ioaddr, int port)
 	return ret;
 }
 
+static int invalid_model(void)
+{
+	struct device_node *np = of_find_node_by_path("/");
+	const char *model = of_get_property(np, "model", NULL);
+
+	of_node_put(np);
+	if (!model || strncmp(model, "Altera SOCFPGA Arria 10", 23) != 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 static int validate_parent_available(struct device_node *np);
 static const struct of_device_id altr_edac_a10_device_of_match[];
 static int __init __maybe_unused altr_init_a10_ecc_device_type(char *compat)
 {
 	int irq;
-	struct device_node *child, *np = of_find_compatible_node(NULL, NULL,
-					"altr,socfpga-a10-ecc-manager");
+	struct device_node *child, *np;
+
+	if (invalid_model())
+		return -ENODEV;
+
+	np = of_find_compatible_node(NULL, NULL,
+				     "altr,socfpga-a10-ecc-manager");
 	if (!np) {
 		edac_printk(KERN_ERR, EDAC_DEVICE, "ECC Manager not found\n");
 		return -ENODEV;
@@ -1546,8 +1563,12 @@ static irqreturn_t altr_edac_a10_ecc_irq_portb(int irq, void *dev_id)
 static int __init socfpga_init_sdmmc_ecc(void)
 {
 	int rc = -ENODEV;
-	struct device_node *child = of_find_compatible_node(NULL, NULL,
-						"altr,socfpga-sdmmc-ecc");
+	struct device_node *child;
+
+	if (invalid_model())
+		return -ENODEV;
+
+	child = of_find_compatible_node(NULL, NULL, "altr,socfpga-sdmmc-ecc");
 	if (!child) {
 		edac_printk(KERN_WARNING, EDAC_DEVICE, "SDMMC node not found\n");
 		return -ENODEV;
-- 
1.9.1

[toc] | [next] | [standalone]

#1615852

On Mon, Apr 03, 2017 at 02:01:06PM -0500, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> The peripherals EDACs only exist on the Arria10 SoCFPGA. The Cyclone5
> initialization has EDAC warnings when the peripherals aren't found
> in the device tree. Fix by checking for Arria10 in the init functions.
> 
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
>  drivers/edac/altera_edac.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 6421cc3..98e83f5 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1024,13 +1024,30 @@ static int __maybe_unused altr_init_memory_port(void __iomem *ioaddr, int port)
>  	return ret;
>  }
>  
> +static int invalid_model(void)
> +{
> +	struct device_node *np = of_find_node_by_path("/");

That needs to have its return value checked, of course.

> +	const char *model = of_get_property(np, "model", NULL);
> +
> +	of_node_put(np);
> +	if (!model || strncmp(model, "Altera SOCFPGA Arria 10", 23) != 0)

No need for the "!= 0"

> +		return -ENODEV;
> +
> +	return 0;
> +}

That function name "invalid_model" sounds like a boolean: is the model
invalid. So you can simply return bools and not -ENODEV as the context
you're using it is boolean.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1616069

Hi Boris,

On 04/04/2017 04:59 AM, Borislav Petkov wrote:
> On Mon, Apr 03, 2017 at 02:01:06PM -0500, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> The peripherals EDACs only exist on the Arria10 SoCFPGA. The Cyclone5
>> initialization has EDAC warnings when the peripherals aren't found
>> in the device tree. Fix by checking for Arria10 in the init functions.
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>>  drivers/edac/altera_edac.c | 29 +++++++++++++++++++++++++----
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
>> index 6421cc3..98e83f5 100644
>> --- a/drivers/edac/altera_edac.c
>> +++ b/drivers/edac/altera_edac.c
>> @@ -1024,13 +1024,30 @@ static int __maybe_unused altr_init_memory_port(void __iomem *ioaddr, int port)
>>  	return ret;
>>  }
>>
>> +static int invalid_model(void)
>> +{
>> +	struct device_node *np = of_find_node_by_path("/");
>
> That needs to have its return value checked, of course.
>
Actually, some of the of_ functions have validity checking in them.  In 
this case, of_get_property() calls of_find_property() which returns NULL 
if np is 0 which is propagated back up to model so this code should be safe.

In the case of the of_node_put() below, the node is also checked for 
non-NULL so I should be OK there.

>> +	const char *model = of_get_property(np, "model", NULL);
>> +
>> +	of_node_put(np);
>> +	if (!model || strncmp(model, "Altera SOCFPGA Arria 10", 23) != 0)
>
> No need for the "!= 0"
>
OK. Thanks.

>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>
> That function name "invalid_model" sounds like a boolean: is the model
> invalid. So you can simply return bools and not -ENODEV as the context
> you're using it is boolean.
>
Good point, thanks. I'll fix and re-submit.

Thanks for reviewing!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web