[PATCH 0/5] qlge: Fixes for powerpc platform.

[PATCH 0/5] qlge: Fixes for powerpc platform.

[Ron Mercer]

The following 5 patches fix bugs found during bringup on PPC platform.

[PATCH 1/5] qlge: bugfux: Add missing pci_mapping_err checking.

[Ron Mercer]

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge_main.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 drivers/net/qlge/qlge_main.c

diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
old mode 100644
new mode 100755
index 0214708..56c7531
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -963,6 +963,11 @@ static void ql_update_sbq(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 						     sbq_desc->p.skb->data,
 						     rx_ring->sbq_buf_size /
 						     2, PCI_DMA_FROMDEVICE);
+				if (pci_dma_mapping_error(qdev->pdev, map)) {
+					QPRINTK(qdev, IFUP, ERR, "PCI mapping failed.\n");
+					rx_ring->sbq_clean_idx = clean_idx;
+					return;
+				}
 				pci_unmap_addr_set(sbq_desc, mapaddr, map);
 				pci_unmap_len_set(sbq_desc, maplen,
 						  rx_ring->sbq_buf_size / 2);
-- 
1.6.0

[PATCH 2/5] qlge: bugfix: Add missing pci_unmap_page call in receive path.

[Ron Mercer]

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge_main.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 56c7531..92dc3f7 100755
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -1308,6 +1308,11 @@ static struct sk_buff *ql_build_rx_skb(struct ql_adapter *qdev,
 					"No skb available, drop the packet.\n");
 				return NULL;
 			}
+			pci_unmap_page(qdev->pdev,
+				       pci_unmap_addr(lbq_desc,
+						      mapaddr),
+				       pci_unmap_len(lbq_desc, maplen),
+				       PCI_DMA_FROMDEVICE);
 			skb_reserve(skb, NET_IP_ALIGN);
 			QPRINTK(qdev, RX_STATUS, DEBUG,
 				"%d bytes of headers and data in large. Chain page to new skb and pull tail.\n", length);
-- 
1.6.0

[PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Ron Mercer]

Shadow registers are host memory locations that the chip echos queue indexes to.
The chip does this in little endian values, so they need to be swapped before referencing.

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 drivers/net/qlge/qlge.h

diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
old mode 100644
new mode 100755
index ba2e1c5..f1751a2
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -1477,9 +1477,9 @@ static inline void ql_write_db_reg(u32 val, void __iomem *addr)
  * update the relevant index register and then copy the value to the
  * shadow register in host memory.
  */
-static inline unsigned int ql_read_sh_reg(const volatile void  *addr)
+static inline u32 ql_read_sh_reg(const volatile void  *addr)
 {
-	return *(volatile unsigned int __force *)addr;
+	return le32_to_cpu(*(volatile unsigned int __force *)addr);
 }
 
 extern char qlge_driver_name[];
-- 
1.6.0

[PATCH 4/5] qlge: bugfix: Fix rx ring and large and small rx buffer length settings.

[Ron Mercer]

The length field for rx ring and buffer rings is 16-bits.  To indicate
a maximum length of 65536 the value should be set to zero.

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge_main.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 92dc3f7..b58f5df 100755
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -2491,8 +2491,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 
 	memset((void *)cqicb, 0, sizeof(struct cqicb));
 	cqicb->msix_vect = rx_ring->irq;
+	bq_len = (rx_ring->cq_len == 65536) ? 0 : (u16)rx_ring->cq_len;
 
-	cqicb->len = cpu_to_le16(rx_ring->cq_len | LEN_V | LEN_CPP_CONT);
+	cqicb->len = cpu_to_le16(bq_len | LEN_V | LEN_CPP_CONT);
 
 	cqicb->addr_lo = cpu_to_le32(rx_ring->cq_base_dma);
 	cqicb->addr_hi = cpu_to_le32((u64) rx_ring->cq_base_dma >> 32);
@@ -2514,8 +2515,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 		    cpu_to_le32(rx_ring->lbq_base_indirect_dma);
 		cqicb->lbq_addr_hi =
 		    cpu_to_le32((u64) rx_ring->lbq_base_indirect_dma >> 32);
-		cqicb->lbq_buf_size = cpu_to_le32(rx_ring->lbq_buf_size);
-		bq_len = (u16) rx_ring->lbq_len;
+		bq_len = (rx_ring->lbq_buf_size == 65536) ? 0 : (u16)rx_ring->lbq_buf_size;
+		cqicb->lbq_buf_size = cpu_to_le16(bq_len);
+		bq_len = (rx_ring->lbq_len == 65536) ? 0 : (u16)rx_ring->lbq_len;
 		cqicb->lbq_len = cpu_to_le16(bq_len);
 		rx_ring->lbq_prod_idx = rx_ring->lbq_len - 16;
 		rx_ring->lbq_curr_idx = 0;
@@ -2530,8 +2532,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring)
 		cqicb->sbq_addr_hi =
 		    cpu_to_le32((u64) rx_ring->sbq_base_indirect_dma >> 32);
 		cqicb->sbq_buf_size =
-		    cpu_to_le16(((rx_ring->sbq_buf_size / 2) + 8) & 0xfffffff8);
-		bq_len = (u16) rx_ring->sbq_len;
+		    cpu_to_le16((u16)(rx_ring->sbq_buf_size/2));
+		bq_len = (rx_ring->sbq_len == 65536) ? 0 : (u16)rx_ring->sbq_len;
 		cqicb->sbq_len = cpu_to_le16(bq_len);
 		rx_ring->sbq_prod_idx = rx_ring->sbq_len - 16;
 		rx_ring->sbq_curr_idx = 0;
-- 
1.6.0

[PATCH 5/5] qlge: bugfix: Fix register access error checking.

[Ron Mercer]

Some indexed register do not have error bits.  In this case a
value of zero should be used for error checking.

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge_main.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index b58f5df..db226c5 100755
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -257,7 +257,7 @@ int ql_get_mac_addr_reg(struct ql_adapter *qdev, u32 type, u16 index,
 		{
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, (offset++) | /* offset */
@@ -265,13 +265,13 @@ int ql_get_mac_addr_reg(struct ql_adapter *qdev, u32 type, u16 index,
 				   MAC_ADDR_ADR | MAC_ADDR_RS | type); /* type */
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MR, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MR, 0);
 			if (status)
 				goto exit;
 			*value++ = ql_read32(qdev, MAC_ADDR_DATA);
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, (offset++) | /* offset */
@@ -279,14 +279,14 @@ int ql_get_mac_addr_reg(struct ql_adapter *qdev, u32 type, u16 index,
 				   MAC_ADDR_ADR | MAC_ADDR_RS | type); /* type */
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MR, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MR, 0);
 			if (status)
 				goto exit;
 			*value++ = ql_read32(qdev, MAC_ADDR_DATA);
 			if (type == MAC_ADDR_TYPE_CAM_MAC) {
 				status =
 				    ql_wait_reg_rdy(qdev,
-					MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+					MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 				if (status)
 					goto exit;
 				ql_write32(qdev, MAC_ADDR_IDX, (offset++) | /* offset */
@@ -294,7 +294,7 @@ int ql_get_mac_addr_reg(struct ql_adapter *qdev, u32 type, u16 index,
 					   MAC_ADDR_ADR | MAC_ADDR_RS | type); /* type */
 				status =
 				    ql_wait_reg_rdy(qdev, MAC_ADDR_IDX,
-						    MAC_ADDR_MR, MAC_ADDR_E);
+						    MAC_ADDR_MR, 0);
 				if (status)
 					goto exit;
 				*value++ = ql_read32(qdev, MAC_ADDR_DATA);
@@ -344,7 +344,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev, u8 *addr, u32 type,
 
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, (offset++) | /* offset */
@@ -353,7 +353,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev, u8 *addr, u32 type,
 			ql_write32(qdev, MAC_ADDR_DATA, lower);
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, (offset++) | /* offset */
@@ -362,7 +362,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev, u8 *addr, u32 type,
 			ql_write32(qdev, MAC_ADDR_DATA, upper);
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, (offset) |	/* offset */
@@ -400,7 +400,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev, u8 *addr, u32 type,
 
 			status =
 			    ql_wait_reg_rdy(qdev,
-				MAC_ADDR_IDX, MAC_ADDR_MW, MAC_ADDR_E);
+				MAC_ADDR_IDX, MAC_ADDR_MW, 0);
 			if (status)
 				goto exit;
 			ql_write32(qdev, MAC_ADDR_IDX, offset |	/* offset */
@@ -431,13 +431,13 @@ int ql_get_routing_reg(struct ql_adapter *qdev, u32 index, u32 *value)
 	if (status)
 		goto exit;
 
-	status = ql_wait_reg_rdy(qdev, RT_IDX, RT_IDX_MW, RT_IDX_E);
+	status = ql_wait_reg_rdy(qdev, RT_IDX, RT_IDX_MW, 0);
 	if (status)
 		goto exit;
 
 	ql_write32(qdev, RT_IDX,
 		   RT_IDX_TYPE_NICQ | RT_IDX_RS | (index << RT_IDX_IDX_SHIFT));
-	status = ql_wait_reg_rdy(qdev, RT_IDX, RT_IDX_MR, RT_IDX_E);
+	status = ql_wait_reg_rdy(qdev, RT_IDX, RT_IDX_MR, 0);
 	if (status)
 		goto exit;
 	*value = ql_read32(qdev, RT_DATA);
-- 
1.6.0

Re: [PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Ben Hutchings]

On Wed, 2008-12-24 at 10:21 -0800, Ron Mercer wrote:
> Shadow registers are host memory locations that the chip echos queue indexes to.
> The chip does this in little endian values, so they need to be swapped before referencing.
> 
> Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
> ---
>  drivers/net/qlge/qlge.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 drivers/net/qlge/qlge.h
> 
> diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
> old mode 100644
> new mode 100755
> index ba2e1c5..f1751a2
> --- a/drivers/net/qlge/qlge.h
> +++ b/drivers/net/qlge/qlge.h
> @@ -1477,9 +1477,9 @@ static inline void ql_write_db_reg(u32 val, void __iomem *addr)
>   * update the relevant index register and then copy the value to the
>   * shadow register in host memory.
>   */
> -static inline unsigned int ql_read_sh_reg(const volatile void  *addr)
> +static inline u32 ql_read_sh_reg(const volatile void  *addr)
>  {
> -	return *(volatile unsigned int __force *)addr;
> +	return le32_to_cpu(*(volatile unsigned int __force *)addr);

I think that should be:
	return le32_to_cpu(*(const volatile __le32 *)addr);

Ben.

>  }
>  
>  extern char qlge_driver_name[];

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Ron Mercer]

On Wed, Dec 24, 2008 at 10:35:31AM -0800, Ben Hutchings wrote:
> > +static inline u32 ql_read_sh_reg(const volatile void  *addr)
> >  {
> > -	return *(volatile unsigned int __force *)addr;
> > +	return le32_to_cpu(*(volatile unsigned int __force *)addr);
> 
> I think that should be:
> 	return le32_to_cpu(*(const volatile __le32 *)addr);
> 
> Ben.

Yes, you're right.  I'll fix that and resend the patches after waiting
for more feedback.

Thanks!

Re: [PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Christoph Hellwig]

On Wed, Dec 24, 2008 at 06:35:31PM +0000, Ben Hutchings wrote:
> > -static inline unsigned int ql_read_sh_reg(const volatile void  *addr)
> > +static inline u32 ql_read_sh_reg(const volatile void  *addr)
> >  {
> > -	return *(volatile unsigned int __force *)addr;
> > +	return le32_to_cpu(*(volatile unsigned int __force *)addr);
> 
> I think that should be:
> 	???return le32_to_cpu(*(const volatile __le32 *)addr);

No.  It needs more love than that.

ql_read_sh_reg is only used to access .prod_idx_sh_reg in the rx_ring
structure, which is an offset from the .rx_ring_shadow_reg_area member.
So instead of all this werid casting .prod_idx_sh_reg should simply
be a __le32 pointer, whithout volatile and the other mess, and then
you can use le32_to_cpu directly and kill the helper.

And please verify the whole driver for endianess issues using sparse
while you're at it...

Re: [PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Ron Mercer]

On Wed, Dec 24, 2008 at 01:15:21PM -0800, Christoph Hellwig wrote:
> > I think that should be:
> > 	???return le32_to_cpu(*(const volatile __le32 *)addr);
> 
> No.  It needs more love than that.
> 
> ql_read_sh_reg is only used to access .prod_idx_sh_reg in the rx_ring
> structure, which is an offset from the .rx_ring_shadow_reg_area member.
> So instead of all this werid casting .prod_idx_sh_reg should simply
> be a __le32 pointer, whithout volatile and the other mess, and then
> you can use le32_to_cpu directly and kill the helper.

Yes, .prod_idx_sh_reg should be an __le32 *, but the volatile modifier 
was used since the memory location is modified by the chip and read by 
the driver.  Isn't that a case where it should be used?

> 
> And please verify the whole driver for endianess issues using sparse
> while you're at it...

Will do...

Re: [PATCH 3/5] qlge: bugfix: Fix endian issue regarding shadow registers.

[Harvey Harrison]

On Wed, 2008-12-24 at 16:15 -0500, Christoph Hellwig wrote:
> On Wed, Dec 24, 2008 at 06:35:31PM +0000, Ben Hutchings wrote:
> > > -static inline unsigned int ql_read_sh_reg(const volatile void  *addr)
> > > +static inline u32 ql_read_sh_reg(const volatile void  *addr)
> > >  {
> > > -	return *(volatile unsigned int __force *)addr;
> > > +	return le32_to_cpu(*(volatile unsigned int __force *)addr);
> > 
> > I think that should be:
> > 	???return le32_to_cpu(*(const volatile __le32 *)addr);
> 
> No.  It needs more love than that.
> 
> ql_read_sh_reg is only used to access .prod_idx_sh_reg in the rx_ring
> structure, which is an offset from the .rx_ring_shadow_reg_area member.
> So instead of all this werid casting .prod_idx_sh_reg should simply
> be a __le32 pointer, whithout volatile and the other mess, and then
> you can use le32_to_cpu directly and kill the helper.
> 
> And please verify the whole driver for endianess issues using sparse
> while you're at it...

I've already deleted the patches from my inbox, but if you send them
again off-list I'll have a look as well.

Harvey