[PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Jeremy Kerr]

Use a temporary variable of the correct type instead.

Also, this allows us to check the return value in ls_uarts_init, to
avoid dereferencing a null pointer if required properties aren't
present.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 arch/powerpc/platforms/embedded6xx/ls_uart.c |   14 +++++++++-----
 arch/powerpc/sysdev/fsl_soc.c                |   18 ++++++++++--------
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/embedded6xx/ls_uart.c b/arch/powerpc/platforms/embedded6xx/ls_uart.c
index d0bee9f..9d4b40c 100644
--- a/arch/powerpc/platforms/embedded6xx/ls_uart.c
+++ b/arch/powerpc/platforms/embedded6xx/ls_uart.c
@@ -103,20 +103,24 @@ static void __init ls_uart_init(void)
 static int __init ls_uarts_init(void)
 {
 	struct device_node *avr;
-	phys_addr_t phys_addr;
+	const u32 *avr_clock_prop, *phys_addr;
 	int len;
 
 	avr = of_find_node_by_path("/soc10x/serial@80004500");
 	if (!avr)
 		return -EINVAL;
 
-	avr_clock = *(u32*)of_get_property(avr, "clock-frequency", &len);
-	phys_addr = ((u32*)of_get_property(avr, "reg", &len))[0];
+	phys_addr = of_get_property(avr, "reg", &len);
+	avr_clock_prop = of_get_property(avr, "clock-frequency", &len);
 
-	if (!avr_clock || !phys_addr)
+	if (!avr_clock_prop || !phys_addr)
 		return -EINVAL;
 
-	avr_addr = ioremap(phys_addr, 32);
+	avr_clock = *avr_clock_prop;
+	if (!avr_clock || !phys_addr[0])
+		return -EINVAL;
+
+	avr_addr = ioremap(phys_addr[0], 32);
 	if (!avr_addr)
 		return -EFAULT;
 
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..564d5e5 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -673,6 +673,7 @@ static int __init fs_enet_of_init(void)
 		struct fs_platform_info fs_enet_data;
 		const unsigned int *id, *phy_addr, *phy_irq;
 		const void *mac_addr;
+		const u32 *clk;
 		const phandle *ph;
 		const char *model;
 
@@ -740,10 +741,10 @@ static int __init fs_enet_of_init(void)
                         goto unreg;
                 }
 
-		fs_enet_data.clk_rx = *((u32 *)of_get_property(np,
-						"rx-clock", NULL));
-		fs_enet_data.clk_tx = *((u32 *)of_get_property(np,
-						"tx-clock", NULL));
+		clk = of_get_property(np, "rx-clock", NULL);
+		fs_enet_data.clk_rx = *clk;
+		clk = of_get_property(np, "tx-clock", NULL);
+		fs_enet_data.clk_tx = *clk;
 
 		if (strstr(model, "FCC")) {
 			int fcc_index = *id - 1;
@@ -844,6 +845,7 @@ static int __init cpm_uart_of_init(void)
 		struct resource r[3];
 		struct fs_uart_platform_info cpm_uart_data;
 		const int *id;
+		const u32 *clk;
 		const char *model;
 
 		memset(r, 0, sizeof(r));
@@ -882,10 +884,10 @@ static int __init cpm_uart_of_init(void)
 		cpm_uart_data.tx_buf_size = 32;
 		cpm_uart_data.rx_num_fifo = 4;
 		cpm_uart_data.rx_buf_size = 32;
-		cpm_uart_data.clk_rx = *((u32 *)of_get_property(np,
-						"rx-clock", NULL));
-		cpm_uart_data.clk_tx = *((u32 *)of_get_property(np,
-						"tx-clock", NULL));
+		clk = of_get_property(np, "rx-clock", NULL);
+		cpm_uart_data.clk_rx = *clk;
+		clk = of_get_property(np, "tx-clock", NULL);
+		cpm_uart_data.clk_tx = *clk;
 
 		ret =
 		    platform_device_add_data(cpm_uart_dev, &cpm_uart_data,

[PATCH 1/2] cell: don't cast the result of of_get_property()

[Jeremy Kerr]

The cast to u32 * isn't required, of_get_property returns a void *.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
 arch/powerpc/platforms/cell/spu_manage.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/cell/spu_manage.c b/arch/powerpc/platforms/cell/spu_manage.c
index 0e14f53..1b01070 100644
--- a/arch/powerpc/platforms/cell/spu_manage.c
+++ b/arch/powerpc/platforms/cell/spu_manage.c
@@ -377,10 +377,10 @@ static int qs20_reg_memory[QS20_SPES_PER_BE] = { 1, 1, 0, 0, 0, 0, 0, 0 };
 static struct spu *spu_lookup_reg(int node, u32 reg)
 {
 	struct spu *spu;
-	u32 *spu_reg;
+	const u32 *spu_reg;
 
 	list_for_each_entry(spu, &cbe_spu_info[node].spus, cbe_list) {
-		spu_reg = (u32*)of_get_property(spu_devnode(spu), "reg", NULL);
+		spu_reg = of_get_property(spu_devnode(spu), "reg", NULL);
 		if (*spu_reg == reg)
 			return spu;
 	}

Re: [PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Kumar Gala]

On Sep 14, 2007, at 12:46 AM, Jeremy Kerr wrote:

> Use a temporary variable of the correct type instead.
>
> Also, this allows us to check the return value in ls_uarts_init, to
> avoid dereferencing a null pointer if required properties aren't
> present.
>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

Can I get you to respin this against for-2.6.24.  For some reason  
this doesn't apply cleanly for me.

- k

Re: [PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Fri, 14 Sep 2007 15:46:40 +1000 Jeremy Kerr <jk@ozlabs.org> wrote:
>
> -	avr_clock = *(u32*)of_get_property(avr, "clock-frequency", &len);
> -	phys_addr = ((u32*)of_get_property(avr, "reg", &len))[0];
> +	phys_addr = of_get_property(avr, "reg", &len);
> +	avr_clock_prop = of_get_property(avr, "clock-frequency", &len);

If you don't use the value of the len variable, then you can pass NULL to
of_get_property.

> +	if (!avr_clock || !phys_addr[0])

Not *phys_addr?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Jeremy Kerr]

Stephen,

> If you don't use the value of the len variable, then you can pass
> NULL to of_get_property.

Sure, but that's probably a separate patch, no ?

> Not *phys_addr?

That's what the existing code does. I'm guessing that it treats 
phys_addr as an array with one entry, not a single u32.

Cheers,


Jeremy

Re: [PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Wed, 26 Sep 2007 10:52:23 +1000 Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Stephen,
> 
> > If you don't use the value of the len variable, then you can pass
> > NULL to of_get_property.
> 
> Sure, but that's probably a separate patch, no ?

OK.

> > Not *phys_addr?
> 
> That's what the existing code does. I'm guessing that it treats 
> phys_addr as an array with one entry, not a single u32.

OK.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] fsl/embedded6xx: don't cast the result of of_get_property

[Segher Boessenkool]

>> -	avr_clock = *(u32*)of_get_property(avr, "clock-frequency", &len);
>> -	phys_addr = ((u32*)of_get_property(avr, "reg", &len))[0];
>> +	phys_addr = of_get_property(avr, "reg", &len);
>> +	avr_clock_prop = of_get_property(avr, "clock-frequency", &len);
>
> If you don't use the value of the len variable, then you can pass NULL 
> to
> of_get_property.

OTOH, it is almost always an error to not use the length (missing
error check).


Segher