linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] celleb: bug fix caused by not casting pointer types
@ 2007-03-02  7:59 Ishizaki Kou
  2007-03-02  9:11 ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 2+ messages in thread
From: Ishizaki Kou @ 2007-03-02  7:59 UTC (permalink / raw)
  To: paulus; +Cc: linuxppc-dev

This patch fixes a bug caused by changes of pointer type in
commit f1fda89522c5aaa1bd4ef69605e85e6ee9c85faf.

hose->cfg_addr type is "volatile unsigned int __iomem *", so
"hose->cfg_addr + X" will not make an intended address.

This patch also adds comments for usage of cfg_addr and cfg_data in
pci_controller structure. We use them in irregular way, and the
original code is short of explanations about them.

Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
---

Index: linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c
diff -u linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.1.1.2 linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.20
--- linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.1.1.2	Wed Feb 14 10:52:11 2007
+++ linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c	Wed Feb 21 14:06:25 2007
@@ -43,11 +43,34 @@
 
 #define iob()  __asm__ __volatile__("eieio; sync":::"memory")
 
+static inline volatile void __iomem *celleb_epci_get_epci_base(
+					struct pci_controller *hose)
+{
+	/*
+	 * Note:
+	 * Celleb epci uses cfg_addr as a base address for
+	 * epci control registers.
+	 */
+
+	return hose->cfg_addr;
+}
+
+static inline volatile void __iomem *celleb_epci_get_epci_cfg(
+					struct pci_controller *hose)
+{
+	/*
+	 * Note:
+	 * Celleb epci uses cfg_data as a base address for
+	 * configuration area for epci devices.
+	 */
+
+	return hose->cfg_data;
+}
 
 #if 0 /* test code for epci dummy read */
 static void celleb_epci_dummy_read(struct pci_dev *dev)
 {
-	void __iomem *epci_base;
+	volatile void __iomem *epci_base;
 	struct device_node *node;
 	struct pci_controller *hose;
 	u32 val;
@@ -58,7 +81,7 @@
 	if (!hose)
 		return;
 
-	epci_base = hose->cfg_addr;
+	epci_base = celleb_epci_get_epci_base(hose);
 
 	val = in_be32(epci_base + SCC_EPCI_WATRP);
 	iosync();
@@ -70,19 +93,20 @@
 static inline void clear_and_disable_master_abort_interrupt(
 					struct pci_controller *hose)
 {
-	void __iomem *addr;
-	addr = hose->cfg_addr + PCI_COMMAND;
-	out_be32(addr, in_be32(addr) | (PCI_STATUS_REC_MASTER_ABORT << 16));
+	volatile void __iomem *epci_base, *reg;
+	epci_base = celleb_epci_get_epci_base(hose);
+	reg = epci_base + PCI_COMMAND;
+	out_be32(reg, in_be32(reg) | (PCI_STATUS_REC_MASTER_ABORT << 16));
 }
 
 static int celleb_epci_check_abort(struct pci_controller *hose,
-				   void __iomem *addr)
+				   volatile void __iomem *addr)
 {
-	void __iomem *reg, *epci_base;
+	volatile void __iomem *reg, *epci_base;
 	u32 val;
 
 	iob();
-	epci_base = hose->cfg_addr;
+	epci_base = celleb_epci_get_epci_base(hose);
 
 	reg = epci_base + PCI_COMMAND;
 	val = in_be32(reg);
@@ -108,20 +132,21 @@
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static void __iomem *celleb_epci_make_config_addr(struct pci_controller *hose,
+static volatile void __iomem *celleb_epci_make_config_addr(
+					struct pci_controller *hose,
 					unsigned int devfn, int where)
 {
-	void __iomem *addr;
+	volatile void __iomem *addr;
 	struct pci_bus *bus = hose->bus;
 
 	if (bus->self)
-		addr = hose->cfg_data +
+		addr = celleb_epci_get_epci_cfg(hose) +
 		       (((bus->number & 0xff) << 16)
 		        | ((devfn & 0xff) << 8)
 		        | (where & 0xff)
 		        | 0x01000000);
 	else
-		addr = hose->cfg_data +
+		addr = celleb_epci_get_epci_cfg(hose) +
 		       (((devfn & 0xff) << 8) | (where & 0xff));
 
 	pr_debug("EPCI: config_addr = 0x%p\n", addr);
@@ -132,7 +157,7 @@
 static int celleb_epci_read_config(struct pci_bus *bus,
 			unsigned int devfn, int where, int size, u32 * val)
 {
-	void __iomem *addr;
+	volatile void __iomem *epci_base, *addr;
 	struct device_node *node;
 	struct pci_controller *hose;
 
@@ -142,13 +167,14 @@
 	node = (struct device_node *)bus->sysdata;
 	hose = pci_find_hose_for_OF_device(node);
 
-	if (!hose->cfg_data)
+	if (!celleb_epci_get_epci_cfg(hose))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (bus->number == hose->first_busno && devfn == 0) {
 		/* EPCI controller self */
 
-		addr = hose->cfg_addr + where;
+		epci_base = celleb_epci_get_epci_base(hose);
+		addr = epci_base + where;
 
 		switch (size) {
 		case 1:
@@ -185,7 +211,7 @@
 	}
 
 	pr_debug("EPCI: "
-		 "addr=0x%lx, devfn=0x%x, where=0x%x, size=0x%x, val=0x%x\n",
+		 "addr=0x%p, devfn=0x%x, where=0x%x, size=0x%x, val=0x%x\n",
 		 addr, devfn, where, size, *val);
 
 	return celleb_epci_check_abort(hose, NULL);
@@ -194,7 +220,7 @@
 static int celleb_epci_write_config(struct pci_bus *bus,
 			unsigned int devfn, int where, int size, u32 val)
 {
-	void __iomem *addr;
+	volatile void __iomem *epci_base, *addr;
 	struct device_node *node;
 	struct pci_controller *hose;
 
@@ -204,13 +230,15 @@
 	node = (struct device_node *)bus->sysdata;
 	hose = pci_find_hose_for_OF_device(node);
 
-	if (!hose->cfg_data)
+
+	if (!celleb_epci_get_epci_cfg(hose))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (bus->number == hose->first_busno && devfn == 0) {
 		/* EPCI controller self */
 
-		addr = hose->cfg_addr + where;
+		epci_base = celleb_epci_get_epci_base(hose);
+		addr = epci_base + where;
 
 		switch (size) {
 		case 1:
@@ -258,10 +286,10 @@
 static int __devinit celleb_epci_init(struct pci_controller *hose)
 {
 	u32 val;
-	void __iomem *reg, *epci_base;
+	volatile void __iomem *reg, *epci_base;
 	int hwres = 0;
 
-	epci_base = hose->cfg_addr;
+	epci_base = celleb_epci_get_epci_base(hose);
 
 	/* PCI core reset(Internal bus and PCI clock) */
 	reg = epci_base + SCC_EPCI_CKCTRL;
@@ -382,6 +410,18 @@
 
 	pr_debug("PCI: celleb_setup_epci()\n");
 
+	/*
+	 * Note:
+	 * Celleb epci uses cfg_addr and cfg_data member of  
+	 * pci_controller structure in irregular way.
+	 *
+	 * cfg_addr is used to map for control registers of 
+	 * celleb epci.
+	 *
+	 * cfg_data is used for configuration area of devices
+	 * on Celleb epci buses.
+	 */
+
 	if (of_address_to_resource(node, 0, &r))
 		goto error;
 	hose->cfg_addr = ioremap(r.start, (r.end - r.start + 1));

^ permalink raw reply	[flat|nested] 2+ messages in thread
* Re: [PATCH] celleb: bug fix caused by not casting pointer types
  2007-03-02  7:59 [PATCH] celleb: bug fix caused by not casting pointer types Ishizaki Kou
@ 2007-03-02  9:11 ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 2+ messages in thread
From: Benjamin Herrenschmidt @ 2007-03-02  9:11 UTC (permalink / raw)
  To: Ishizaki Kou; +Cc: linuxppc-dev, paulus

On Fri, 2007-03-02 at 16:59 +0900, Ishizaki Kou wrote:
> This patch fixes a bug caused by changes of pointer type in
> commit f1fda89522c5aaa1bd4ef69605e85e6ee9c85faf.
> 
> hose->cfg_addr type is "volatile unsigned int __iomem *", so
> "hose->cfg_addr + X" will not make an intended address.
> 
> This patch also adds comments for usage of cfg_addr and cfg_data in
> pci_controller structure. We use them in irregular way, and the
> original code is short of explanations about them.
> 
> Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

NOTE: It would be preferrable to actually change the host->cfg_addr
field to be void __iomem * instead of what it is now and make sure
we fix every user. Your patch is fine and can go in as a bug fix, I'll
try to remember to do the cleanup for 2.6.22 if you don't beat me at it
though.

Ben.


> ---
> 
> Index: linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c
> diff -u linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.1.1.2 linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.20
> --- linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c:1.1.1.2	Wed Feb 14 10:52:11 2007
> +++ linux-powerpc-git/arch/powerpc/platforms/celleb/scc_epci.c	Wed Feb 21 14:06:25 2007
> @@ -43,11 +43,34 @@
>  
>  #define iob()  __asm__ __volatile__("eieio; sync":::"memory")
>  
> +static inline volatile void __iomem *celleb_epci_get_epci_base(
> +					struct pci_controller *hose)
> +{
> +	/*
> +	 * Note:
> +	 * Celleb epci uses cfg_addr as a base address for
> +	 * epci control registers.
> +	 */
> +
> +	return hose->cfg_addr;
> +}
> +
> +static inline volatile void __iomem *celleb_epci_get_epci_cfg(
> +					struct pci_controller *hose)
> +{
> +	/*
> +	 * Note:
> +	 * Celleb epci uses cfg_data as a base address for
> +	 * configuration area for epci devices.
> +	 */
> +
> +	return hose->cfg_data;
> +}
>  
>  #if 0 /* test code for epci dummy read */
>  static void celleb_epci_dummy_read(struct pci_dev *dev)
>  {
> -	void __iomem *epci_base;
> +	volatile void __iomem *epci_base;
>  	struct device_node *node;
>  	struct pci_controller *hose;
>  	u32 val;
> @@ -58,7 +81,7 @@
>  	if (!hose)
>  		return;
>  
> -	epci_base = hose->cfg_addr;
> +	epci_base = celleb_epci_get_epci_base(hose);
>  
>  	val = in_be32(epci_base + SCC_EPCI_WATRP);
>  	iosync();
> @@ -70,19 +93,20 @@
>  static inline void clear_and_disable_master_abort_interrupt(
>  					struct pci_controller *hose)
>  {
> -	void __iomem *addr;
> -	addr = hose->cfg_addr + PCI_COMMAND;
> -	out_be32(addr, in_be32(addr) | (PCI_STATUS_REC_MASTER_ABORT << 16));
> +	volatile void __iomem *epci_base, *reg;
> +	epci_base = celleb_epci_get_epci_base(hose);
> +	reg = epci_base + PCI_COMMAND;
> +	out_be32(reg, in_be32(reg) | (PCI_STATUS_REC_MASTER_ABORT << 16));
>  }
>  
>  static int celleb_epci_check_abort(struct pci_controller *hose,
> -				   void __iomem *addr)
> +				   volatile void __iomem *addr)
>  {
> -	void __iomem *reg, *epci_base;
> +	volatile void __iomem *reg, *epci_base;
>  	u32 val;
>  
>  	iob();
> -	epci_base = hose->cfg_addr;
> +	epci_base = celleb_epci_get_epci_base(hose);
>  
>  	reg = epci_base + PCI_COMMAND;
>  	val = in_be32(reg);
> @@ -108,20 +132,21 @@
>  	return PCIBIOS_SUCCESSFUL;
>  }
>  
> -static void __iomem *celleb_epci_make_config_addr(struct pci_controller *hose,
> +static volatile void __iomem *celleb_epci_make_config_addr(
> +					struct pci_controller *hose,
>  					unsigned int devfn, int where)
>  {
> -	void __iomem *addr;
> +	volatile void __iomem *addr;
>  	struct pci_bus *bus = hose->bus;
>  
>  	if (bus->self)
> -		addr = hose->cfg_data +
> +		addr = celleb_epci_get_epci_cfg(hose) +
>  		       (((bus->number & 0xff) << 16)
>  		        | ((devfn & 0xff) << 8)
>  		        | (where & 0xff)
>  		        | 0x01000000);
>  	else
> -		addr = hose->cfg_data +
> +		addr = celleb_epci_get_epci_cfg(hose) +
>  		       (((devfn & 0xff) << 8) | (where & 0xff));
>  
>  	pr_debug("EPCI: config_addr = 0x%p\n", addr);
> @@ -132,7 +157,7 @@
>  static int celleb_epci_read_config(struct pci_bus *bus,
>  			unsigned int devfn, int where, int size, u32 * val)
>  {
> -	void __iomem *addr;
> +	volatile void __iomem *epci_base, *addr;
>  	struct device_node *node;
>  	struct pci_controller *hose;
>  
> @@ -142,13 +167,14 @@
>  	node = (struct device_node *)bus->sysdata;
>  	hose = pci_find_hose_for_OF_device(node);
>  
> -	if (!hose->cfg_data)
> +	if (!celleb_epci_get_epci_cfg(hose))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	if (bus->number == hose->first_busno && devfn == 0) {
>  		/* EPCI controller self */
>  
> -		addr = hose->cfg_addr + where;
> +		epci_base = celleb_epci_get_epci_base(hose);
> +		addr = epci_base + where;
>  
>  		switch (size) {
>  		case 1:
> @@ -185,7 +211,7 @@
>  	}
>  
>  	pr_debug("EPCI: "
> -		 "addr=0x%lx, devfn=0x%x, where=0x%x, size=0x%x, val=0x%x\n",
> +		 "addr=0x%p, devfn=0x%x, where=0x%x, size=0x%x, val=0x%x\n",
>  		 addr, devfn, where, size, *val);
>  
>  	return celleb_epci_check_abort(hose, NULL);
> @@ -194,7 +220,7 @@
>  static int celleb_epci_write_config(struct pci_bus *bus,
>  			unsigned int devfn, int where, int size, u32 val)
>  {
> -	void __iomem *addr;
> +	volatile void __iomem *epci_base, *addr;
>  	struct device_node *node;
>  	struct pci_controller *hose;
>  
> @@ -204,13 +230,15 @@
>  	node = (struct device_node *)bus->sysdata;
>  	hose = pci_find_hose_for_OF_device(node);
>  
> -	if (!hose->cfg_data)
> +
> +	if (!celleb_epci_get_epci_cfg(hose))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	if (bus->number == hose->first_busno && devfn == 0) {
>  		/* EPCI controller self */
>  
> -		addr = hose->cfg_addr + where;
> +		epci_base = celleb_epci_get_epci_base(hose);
> +		addr = epci_base + where;
>  
>  		switch (size) {
>  		case 1:
> @@ -258,10 +286,10 @@
>  static int __devinit celleb_epci_init(struct pci_controller *hose)
>  {
>  	u32 val;
> -	void __iomem *reg, *epci_base;
> +	volatile void __iomem *reg, *epci_base;
>  	int hwres = 0;
>  
> -	epci_base = hose->cfg_addr;
> +	epci_base = celleb_epci_get_epci_base(hose);
>  
>  	/* PCI core reset(Internal bus and PCI clock) */
>  	reg = epci_base + SCC_EPCI_CKCTRL;
> @@ -382,6 +410,18 @@
>  
>  	pr_debug("PCI: celleb_setup_epci()\n");
>  
> +	/*
> +	 * Note:
> +	 * Celleb epci uses cfg_addr and cfg_data member of  
> +	 * pci_controller structure in irregular way.
> +	 *
> +	 * cfg_addr is used to map for control registers of 
> +	 * celleb epci.
> +	 *
> +	 * cfg_data is used for configuration area of devices
> +	 * on Celleb epci buses.
> +	 */
> +
>  	if (of_address_to_resource(node, 0, &r))
>  		goto error;
>  	hose->cfg_addr = ioremap(r.start, (r.end - r.start + 1));
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2007-03-02  9:11 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-02  7:59 [PATCH] celleb: bug fix caused by not casting pointer types Ishizaki Kou
2007-03-02  9:11 ` Benjamin Herrenschmidt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).