Keystone 2 boards boot failure

Keystone 2 boards boot failure

[Franklin S Cooper Jr.]

Hi All,

Latest mainline is currently failing to boot for Keystone 2
Hawking but I'm assuming its true for other Keystone 2
boards. Bisect shows that this issue popped up after the
patch "netcp: try to reduce type confusion in descriptors"
(commit 89907779) was introduced. There was another patch
"netcp: fix regression in receive processing" that seems to
fix some bugs that the prior patch introduced however it
still did not resolve the boot failure and was documented as
not being tested.

Should we revert these commits or does anyone have any
suggestions on how to fix these failures? I would be more
than happy to test any fix.

Bootlogs:
Bootlog 4.5.0-rc2: http://pastebin.com/bsH4u656 (Failed)
Bootlog 4.5.0-rc1: http://pastebin.com/MwY0eS7x (Failed)
Bootlog 4.4: http://pastebin.com/yfBND9Vi (Passed)

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Tuesday 02 February 2016 10:50:41 Franklin S Cooper Jr. wrote:
> Latest mainline is currently failing to boot for Keystone 2
> Hawking but I'm assuming its true for other Keystone 2
> boards. Bisect shows that this issue popped up after the
> patch "netcp: try to reduce type confusion in descriptors"
> (commit 89907779) was introduced. There was another patch
> "netcp: fix regression in receive processing" that seems to
> fix some bugs that the prior patch introduced however it
> still did not resolve the boot failure and was documented as
> not being tested.
> 
> Should we revert these commits or does anyone have any
> suggestions on how to fix these failures? I would be more
> than happy to test any fix.
> 

Have you tried to see if a revert fixes the problem on a
current kernel?

	Arnd

Re: Keystone 2 boards boot failure

[Franklin S Cooper Jr.]

On 02/02/2016 02:41 PM, Arnd Bergmann wrote:
> On Tuesday 02 February 2016 10:50:41 Franklin S Cooper Jr. wrote:
>> Latest mainline is currently failing to boot for Keystone 2
>> Hawking but I'm assuming its true for other Keystone 2
>> boards. Bisect shows that this issue popped up after the
>> patch "netcp: try to reduce type confusion in descriptors"
>> (commit 89907779) was introduced. There was another patch
>> "netcp: fix regression in receive processing" that seems to
>> fix some bugs that the prior patch introduced however it
>> still did not resolve the boot failure and was documented as
>> not being tested.
>>
>> Should we revert these commits or does anyone have any
>> suggestions on how to fix these failures? I would be more
>> than happy to test any fix.
>>
> Have you tried to see if a revert fixes the problem on a
> current kernel?

Yes. Here is a boot log on the latest master with the below
three patches reverted.
http://pastebin.com/W7RWSHpE (Working)

I reverted these three patches. The two latest patches seem
to be trying to correct/expand upon the last patch on this list.

commit 958d104e3d40eef5148c402887138f6594ff7e1e
netcp: fix regression in receive processing

commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
netcp: add more __le32 annotations

commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
netcp: try to reduce type confusion in descriptors



>
> 	Arnd

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
> 
> Yes. Here is a boot log on the latest master with the below
> three patches reverted.
> http://pastebin.com/W7RWSHpE (Working)
> 
> I reverted these three patches. The two latest patches seem
> to be trying to correct/expand upon the last patch on this list.
> 
> commit 958d104e3d40eef5148c402887138f6594ff7e1e
> netcp: fix regression in receive processing
> 
> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
> netcp: add more __le32 annotations
> 
> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
> netcp: try to reduce type confusion in descriptors
> 

The middle patch should have no effect on generated code, so I'm ignoring
that for now.

The next thing to rule out is an endianess bug. I assume you
are running this on with a little-endian kernel, correct? If
you are running big-endian, the base assumption that the driver needs
to swap the data was flawed and that portion needs to be done.

If you are running little-endian 32-bit, please try the partial
revert below, which just undoes the attempt to make it work with
64-bit kernels.

	Arnd
	
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index c61d66d38634..7e291c04a81a 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -117,20 +117,10 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
 	*ndesc = le32_to_cpu(desc->next_desc);
 }
 
-static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
+static void get_pad_info(u32 *pad0, u32 *pad1, struct knav_dma_desc *desc)
 {
 	*pad0 = le32_to_cpu(desc->pad[0]);
 	*pad1 = le32_to_cpu(desc->pad[1]);
-	*pad2 = le32_to_cpu(desc->pad[2]);
-}
-
-static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
-{
-	u64 pad64;
-
-	pad64 = le32_to_cpu(desc->pad[0]) +
-		((u64)le32_to_cpu(desc->pad[1]) << 32);
-	*padptr = (void *)(uintptr_t)pad64;
 }
 
 static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
@@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
 	desc->packet_info = cpu_to_le32(pkt_info);
 }
 
-static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
+static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
 {
 	desc->pad[0] = cpu_to_le32(pad0);
 	desc->pad[1] = cpu_to_le32(pad1);
-	desc->pad[2] = cpu_to_le32(pad1);
 }
 
 static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
@@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 	dma_addr_t dma_desc, dma_buf;
 	unsigned int buf_len, dma_sz = sizeof(*ndesc);
 	void *buf_ptr;
-	u32 pad[2];
 	u32 tmp;
 
 	get_words(&dma_desc, 1, &desc->next_desc);
@@ -593,15 +581,13 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 			break;
 		}
 		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
-		get_pad_ptr(&buf_ptr, ndesc);
+		get_pad_info((u32 *)&buf_ptr, &tmp, ndesc);
 		dma_unmap_page(netcp->dev, dma_buf, PAGE_SIZE, DMA_FROM_DEVICE);
 		__free_page(buf_ptr);
 		knav_pool_desc_put(netcp->rx_pool, desc);
 	}
 
-	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
-	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
-
+	get_pad_info((u32 *)&buf_ptr, &buf_len, desc);
 	if (buf_ptr)
 		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
 	knav_pool_desc_put(netcp->rx_pool, desc);
@@ -639,8 +625,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	dma_addr_t dma_desc, dma_buff;
 	struct netcp_packet p_info;
 	struct sk_buff *skb;
-	u32 pad[2];
 	void *org_buf_ptr;
+	u32 tmp;
 
 	dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz);
 	if (!dma_desc)
@@ -653,8 +639,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	}
 
 	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
-	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
-	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
+	get_pad_info((u32 *)&org_buf_ptr, &org_buf_len, desc);
 
 	if (unlikely(!org_buf_ptr)) {
 		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
@@ -679,7 +664,6 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	/* Fill in the page fragment list */
 	while (dma_desc) {
 		struct page *page;
-		void *ptr;
 
 		ndesc = knav_pool_desc_unmap(netcp->rx_pool, dma_desc, dma_sz);
 		if (unlikely(!ndesc)) {
@@ -688,8 +672,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 		}
 
 		get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc);
-		get_pad_ptr(&ptr, ndesc);
-		page = ptr;
+		get_pad_info((u32 *)&page, &tmp, ndesc);
 
 		if (likely(dma_buff && buf_len && page)) {
 			dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE,
@@ -767,6 +750,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
 	unsigned int buf_len, dma_sz;
 	dma_addr_t dma;
 	void *buf_ptr;
+	u32 tmp;
 
 	/* Allocate descriptor */
 	while ((dma = knav_queue_pop(netcp->rx_fdq[fdq], &dma_sz))) {
@@ -777,7 +761,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 
 		get_org_pkt_info(&dma, &buf_len, desc);
-		get_pad_ptr(&buf_ptr, desc);
+		get_pad_info((u32 *)&buf_ptr, &tmp, desc);
 
 		if (unlikely(!dma)) {
 			dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n");
@@ -829,7 +813,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 	struct page *page;
 	dma_addr_t dma;
 	void *bufptr;
-	u32 pad[3];
+	u32 pad[2];
 
 	/* Allocate descriptor */
 	hwdesc = knav_pool_desc_get(netcp->rx_pool);
@@ -846,7 +830,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
 		bufptr = netdev_alloc_frag(primary_buf_len);
-		pad[2] = primary_buf_len;
+		pad[1] = primary_buf_len;
 
 		if (unlikely(!bufptr)) {
 			dev_warn_ratelimited(netcp->ndev_dev,
@@ -858,8 +842,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		if (unlikely(dma_mapping_error(netcp->dev, dma)))
 			goto fail;
 
-		pad[0] = lower_32_bits((uintptr_t)bufptr);
-		pad[1] = upper_32_bits((uintptr_t)bufptr);
+		pad[0] = (u32)bufptr;
 
 	} else {
 		/* Allocate a secondary receive queue entry */
@@ -870,9 +853,8 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 		buf_len = PAGE_SIZE;
 		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
-		pad[0] = lower_32_bits(dma);
-		pad[1] = upper_32_bits(dma);
-		pad[2] = 0;
+		pad[0] = (u32)page;
+		pad[1] = 0;
 	}
 
 	desc_info =  KNAV_DMA_DESC_PS_INFO_IN_DESC;
@@ -882,7 +864,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
 		    KNAV_DMA_DESC_RETQ_SHIFT;
 	set_org_pkt_info(dma, buf_len, hwdesc);
-	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
+	set_pad_info(pad[0], pad[1], hwdesc);
 	set_desc_info(desc_info, pkt_info, hwdesc);
 
 	/* Push to FDQs */
@@ -971,11 +953,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 					  unsigned int budget)
 {
 	struct knav_dma_desc *desc;
-	void *ptr;
 	struct sk_buff *skb;
 	unsigned int dma_sz;
 	dma_addr_t dma;
 	int pkts = 0;
+	u32 tmp;
 
 	while (budget--) {
 		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
@@ -988,8 +970,7 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 			continue;
 		}
 
-		get_pad_ptr(&ptr, desc);
-		skb = ptr;
+		get_pad_info((u32 *)&skb, &tmp, desc);
 		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
 		if (!skb) {
 			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
@@ -1078,7 +1059,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
 		u32 page_offset = frag->page_offset;
 		u32 buf_len = skb_frag_size(frag);
 		dma_addr_t desc_dma;
-		u32 desc_dma_32;
 		u32 pkt_info;
 
 		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
@@ -1100,8 +1080,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
 			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
 				KNAV_DMA_DESC_RETQ_SHIFT;
 		set_pkt_info(dma_addr, buf_len, 0, ndesc);
-		desc_dma_32 = (u32)desc_dma;
-		set_words(&desc_dma_32, 1, &pdesc->next_desc);
+		set_words(&desc_dma, 1, &pdesc->next_desc);
 		pkt_len += buf_len;
 		if (pdesc != desc)
 			knav_pool_desc_map(netcp->tx_pool, pdesc,
@@ -1194,10 +1173,7 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
 	}
 
 	set_words(&tmp, 1, &desc->packet_info);
-	tmp = lower_32_bits((uintptr_t)&skb);
-	set_words(&tmp, 1, &desc->pad[0]);
-	tmp = upper_32_bits((uintptr_t)&skb);
-	set_words(&tmp, 1, &desc->pad[1]);
+	set_words((u32 *)&skb, 1, &desc->pad[0]);
 
 	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
 		tmp = tx_pipe->switch_to_port;

Re: Keystone 2 boards boot failure

[Franklin Cooper]

On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>
>> Yes. Here is a boot log on the latest master with the below
>> three patches reverted.
>> http://pastebin.com/W7RWSHpE (Working)
>>
>> I reverted these three patches. The two latest patches seem
>> to be trying to correct/expand upon the last patch on this list.
>>
>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>> netcp: fix regression in receive processing
>>
>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>> netcp: add more __le32 annotations
>>
>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>> netcp: try to reduce type confusion in descriptors
>>
> 
> The middle patch should have no effect on generated code, so I'm ignoring
> that for now.
> 
> The next thing to rule out is an endianess bug. I assume you
> are running this on with a little-endian kernel, correct? If
> you are running big-endian, the base assumption that the driver needs
> to swap the data was flawed and that portion needs to be done.
> 
> If you are running little-endian 32-bit, please try the partial
> revert below, which just undoes the attempt to make it work with
> 64-bit kernels.

Keystone 2 devices are little-endian 32-bit devices.

This partial revert fixes the boot problem for me.
> 
> 	Arnd
> 	
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..7e291c04a81a 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -117,20 +117,10 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>  	*ndesc = le32_to_cpu(desc->next_desc);
>  }
>  
> -static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
> +static void get_pad_info(u32 *pad0, u32 *pad1, struct knav_dma_desc *desc)
>  {
>  	*pad0 = le32_to_cpu(desc->pad[0]);
>  	*pad1 = le32_to_cpu(desc->pad[1]);
> -	*pad2 = le32_to_cpu(desc->pad[2]);
> -}
> -
> -static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
> -{
> -	u64 pad64;
> -
> -	pad64 = le32_to_cpu(desc->pad[0]) +
> -		((u64)le32_to_cpu(desc->pad[1]) << 32);
> -	*padptr = (void *)(uintptr_t)pad64;
>  }
>  
>  static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>  	desc->packet_info = cpu_to_le32(pkt_info);
>  }
>  
> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>  {
>  	desc->pad[0] = cpu_to_le32(pad0);
>  	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
>  }
>  
>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  	dma_addr_t dma_desc, dma_buf;
>  	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>  	void *buf_ptr;
> -	u32 pad[2];
>  	u32 tmp;
>  
>  	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -593,15 +581,13 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  			break;
>  		}
>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
> -		get_pad_ptr(&buf_ptr, ndesc);
> +		get_pad_info((u32 *)&buf_ptr, &tmp, ndesc);
>  		dma_unmap_page(netcp->dev, dma_buf, PAGE_SIZE, DMA_FROM_DEVICE);
>  		__free_page(buf_ptr);
>  		knav_pool_desc_put(netcp->rx_pool, desc);
>  	}
>  
> -	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
> -	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> -
> +	get_pad_info((u32 *)&buf_ptr, &buf_len, desc);
>  	if (buf_ptr)
>  		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
>  	knav_pool_desc_put(netcp->rx_pool, desc);
> @@ -639,8 +625,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	dma_addr_t dma_desc, dma_buff;
>  	struct netcp_packet p_info;
>  	struct sk_buff *skb;
> -	u32 pad[2];
>  	void *org_buf_ptr;
> +	u32 tmp;
>  
>  	dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz);
>  	if (!dma_desc)
> @@ -653,8 +639,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	}
>  
>  	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
> -	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
> -	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info((u32 *)&org_buf_ptr, &org_buf_len, desc);
>  
>  	if (unlikely(!org_buf_ptr)) {
>  		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
> @@ -679,7 +664,6 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	/* Fill in the page fragment list */
>  	while (dma_desc) {
>  		struct page *page;
> -		void *ptr;
>  
>  		ndesc = knav_pool_desc_unmap(netcp->rx_pool, dma_desc, dma_sz);
>  		if (unlikely(!ndesc)) {
> @@ -688,8 +672,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  		}
>  
>  		get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc);
> -		get_pad_ptr(&ptr, ndesc);
> -		page = ptr;
> +		get_pad_info((u32 *)&page, &tmp, ndesc);
>  
>  		if (likely(dma_buff && buf_len && page)) {
>  			dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE,
> @@ -767,6 +750,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
>  	unsigned int buf_len, dma_sz;
>  	dma_addr_t dma;
>  	void *buf_ptr;
> +	u32 tmp;
>  
>  	/* Allocate descriptor */
>  	while ((dma = knav_queue_pop(netcp->rx_fdq[fdq], &dma_sz))) {
> @@ -777,7 +761,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
>  		}
>  
>  		get_org_pkt_info(&dma, &buf_len, desc);
> -		get_pad_ptr(&buf_ptr, desc);
> +		get_pad_info((u32 *)&buf_ptr, &tmp, desc);
>  
>  		if (unlikely(!dma)) {
>  			dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n");
> @@ -829,7 +813,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  	struct page *page;
>  	dma_addr_t dma;
>  	void *bufptr;
> -	u32 pad[3];
> +	u32 pad[2];
>  
>  	/* Allocate descriptor */
>  	hwdesc = knav_pool_desc_get(netcp->rx_pool);
> @@ -846,7 +830,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>  
>  		bufptr = netdev_alloc_frag(primary_buf_len);
> -		pad[2] = primary_buf_len;
> +		pad[1] = primary_buf_len;
>  
>  		if (unlikely(!bufptr)) {
>  			dev_warn_ratelimited(netcp->ndev_dev,
> @@ -858,8 +842,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		if (unlikely(dma_mapping_error(netcp->dev, dma)))
>  			goto fail;
>  
> -		pad[0] = lower_32_bits((uintptr_t)bufptr);
> -		pad[1] = upper_32_bits((uintptr_t)bufptr);
> +		pad[0] = (u32)bufptr;
>  
>  	} else {
>  		/* Allocate a secondary receive queue entry */
> @@ -870,9 +853,8 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		}
>  		buf_len = PAGE_SIZE;
>  		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
> -		pad[0] = lower_32_bits(dma);
> -		pad[1] = upper_32_bits(dma);
> -		pad[2] = 0;
> +		pad[0] = (u32)page;
> +		pad[1] = 0;
>  	}
>  
>  	desc_info =  KNAV_DMA_DESC_PS_INFO_IN_DESC;
> @@ -882,7 +864,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>  		    KNAV_DMA_DESC_RETQ_SHIFT;
>  	set_org_pkt_info(dma, buf_len, hwdesc);
> -	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
> +	set_pad_info(pad[0], pad[1], hwdesc);
>  	set_desc_info(desc_info, pkt_info, hwdesc);
>  
>  	/* Push to FDQs */
> @@ -971,11 +953,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>  					  unsigned int budget)
>  {
>  	struct knav_dma_desc *desc;
> -	void *ptr;
>  	struct sk_buff *skb;
>  	unsigned int dma_sz;
>  	dma_addr_t dma;
>  	int pkts = 0;
> +	u32 tmp;
>  
>  	while (budget--) {
>  		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
> @@ -988,8 +970,7 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>  			continue;
>  		}
>  
> -		get_pad_ptr(&ptr, desc);
> -		skb = ptr;
> +		get_pad_info((u32 *)&skb, &tmp, desc);
>  		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
>  		if (!skb) {
>  			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
> @@ -1078,7 +1059,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  		u32 page_offset = frag->page_offset;
>  		u32 buf_len = skb_frag_size(frag);
>  		dma_addr_t desc_dma;
> -		u32 desc_dma_32;
>  		u32 pkt_info;
>  
>  		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
> @@ -1100,8 +1080,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>  				KNAV_DMA_DESC_RETQ_SHIFT;
>  		set_pkt_info(dma_addr, buf_len, 0, ndesc);
> -		desc_dma_32 = (u32)desc_dma;
> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>  		pkt_len += buf_len;
>  		if (pdesc != desc)
>  			knav_pool_desc_map(netcp->tx_pool, pdesc,
> @@ -1194,10 +1173,7 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>  	}
>  
>  	set_words(&tmp, 1, &desc->packet_info);
> -	tmp = lower_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[0]);
> -	tmp = upper_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[1]);
> +	set_words((u32 *)&skb, 1, &desc->pad[0]);
>  
>  	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>  		tmp = tx_pipe->switch_to_port;
> 

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
> > On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
> >>
> >> Yes. Here is a boot log on the latest master with the below
> >> three patches reverted.
> >> http://pastebin.com/W7RWSHpE (Working)
> >>
> >> I reverted these three patches. The two latest patches seem
> >> to be trying to correct/expand upon the last patch on this list.
> >>
> >> commit 958d104e3d40eef5148c402887138f6594ff7e1e
> >> netcp: fix regression in receive processing
> >>
> >> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
> >> netcp: add more __le32 annotations
> >>
> >> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
> >> netcp: try to reduce type confusion in descriptors
> >>
> > 
> > The middle patch should have no effect on generated code, so I'm ignoring
> > that for now.
> > 
> > The next thing to rule out is an endianess bug. I assume you
> > are running this on with a little-endian kernel, correct? If
> > you are running big-endian, the base assumption that the driver needs
> > to swap the data was flawed and that portion needs to be done.
> > 
> > If you are running little-endian 32-bit, please try the partial
> > revert below, which just undoes the attempt to make it work with
> > 64-bit kernels.
> 
> Keystone 2 devices are little-endian 32-bit devices.

I meant the kernel you are running on it, not the hardware.
You should always be able to run both a big-endian kernel and
a littl-endian kernel on any ARMv7 machine, and a couple of
platforms use 64-bit physical addresses even on 32-bit machines
(with the normal 32-bit instruction set).

I wasn't completely sure if there are already keystone-derived
products with 64-bit CPU cores, but I guess the driver would
fail really badly on those (with or without the patch).

> This partial revert fixes the boot problem for me.

Ok.


I tried to create a smaller version and stumbled over
a typo, maybe that's the whole problem. Can you try this one:

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index c61d66d38634..8490804416dd 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *des
 {
 	desc->pad[0] = cpu_to_le32(pad0);
 	desc->pad[1] = cpu_to_le32(pad1);
-	desc->pad[2] = cpu_to_le32(pad1);
+	desc->pad[2] = cpu_to_le32(pad2);
 }
 
 static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,


	Arnd

Re: Keystone 2 boards boot failure

[Franklin S Cooper Jr.]

On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>> Yes. Here is a boot log on the latest master with the below
>>>> three patches reverted.
>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>
>>>> I reverted these three patches. The two latest patches seem
>>>> to be trying to correct/expand upon the last patch on this list.
>>>>
>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>> netcp: fix regression in receive processing
>>>>
>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>> netcp: add more __le32 annotations
>>>>
>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>> netcp: try to reduce type confusion in descriptors
>>>>
>>> The middle patch should have no effect on generated code, so I'm ignoring
>>> that for now.
>>>
>>> The next thing to rule out is an endianess bug. I assume you
>>> are running this on with a little-endian kernel, correct? If
>>> you are running big-endian, the base assumption that the driver needs
>>> to swap the data was flawed and that portion needs to be done.
>>>
>>> If you are running little-endian 32-bit, please try the partial
>>> revert below, which just undoes the attempt to make it work with
>>> 64-bit kernels.
>> Keystone 2 devices are little-endian 32-bit devices.
> I meant the kernel you are running on it, not the hardware.
> You should always be able to run both a big-endian kernel and
> a littl-endian kernel on any ARMv7 machine, and a couple of
> platforms use 64-bit physical addresses even on 32-bit machines
> (with the normal 32-bit instruction set).

I'm not sure if Keystone 2 devices support this or if we
have support for this. I'll have to double check.
>
> I wasn't completely sure if there are already keystone-derived
> products with 64-bit CPU cores, but I guess the driver would
> fail really badly on those (with or without the patch).
>
>> This partial revert fixes the boot problem for me.
> Ok.
>
>
> I tried to create a smaller version and stumbled over
> a typo, maybe that's the whole problem. Can you try this one:
>
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..8490804416dd 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *des
>  {
>  	desc->pad[0] = cpu_to_le32(pad0);
>  	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
> +	desc->pad[2] = cpu_to_le32(pad2);
>  }
>  
>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>
>
> 	Arnd

So only making this change on the latest master with no
other changes I see the boot problem again.

Re: Keystone 2 boards boot failure

[Franklin S Cooper Jr.]

On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>
> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>> Yes. Here is a boot log on the latest master with the below
>>>>> three patches reverted.
>>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>>
>>>>> I reverted these three patches. The two latest patches seem
>>>>> to be trying to correct/expand upon the last patch on this list.
>>>>>
>>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>>> netcp: fix regression in receive processing
>>>>>
>>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>>> netcp: add more __le32 annotations
>>>>>
>>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>>> netcp: try to reduce type confusion in descriptors
>>>>>
>>>> The middle patch should have no effect on generated code, so I'm ignoring
>>>> that for now.
>>>>
>>>> The next thing to rule out is an endianess bug. I assume you
>>>> are running this on with a little-endian kernel, correct? If
>>>> you are running big-endian, the base assumption that the driver needs
>>>> to swap the data was flawed and that portion needs to be done.
>>>>
>>>> If you are running little-endian 32-bit, please try the partial
>>>> revert below, which just undoes the attempt to make it work with
>>>> 64-bit kernels.
>>> Keystone 2 devices are little-endian 32-bit devices.
>> I meant the kernel you are running on it, not the hardware.
>> You should always be able to run both a big-endian kernel and
>> a littl-endian kernel on any ARMv7 machine, and a couple of
>> platforms use 64-bit physical addresses even on 32-bit machines
>> (with the normal 32-bit instruction set).
> I'm not sure if Keystone 2 devices support this or if we
> have support for this. I'll have to double check.
>> I wasn't completely sure if there are already keystone-derived
>> products with 64-bit CPU cores, but I guess the driver would
>> fail really badly on those (with or without the patch).
>>
>>> This partial revert fixes the boot problem for me.
>> Ok.
>>
>>
>> I tried to create a smaller version and stumbled over
>> a typo, maybe that's the whole problem. Can you try this one:
>>
>> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
>> index c61d66d38634..8490804416dd 100644
>> --- a/drivers/net/ethernet/ti/netcp_core.c
>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *des
>>  {
>>  	desc->pad[0] = cpu_to_le32(pad0);
>>  	desc->pad[1] = cpu_to_le32(pad1);
>> -	desc->pad[2] = cpu_to_le32(pad1);
>> +	desc->pad[2] = cpu_to_le32(pad2);
>>  }
>>  
>>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>
>>
>> 	Arnd
> So only making this change on the latest master with no
> other changes I see the boot problem again.

+Grygorii

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

Hi Arnd,

On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>
>> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>>> Yes. Here is a boot log on the latest master with the below
>>>>>> three patches reverted.
>>>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>>>
>>>>>> I reverted these three patches. The two latest patches seem
>>>>>> to be trying to correct/expand upon the last patch on this list.
>>>>>>
>>>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>>>> netcp: fix regression in receive processing
>>>>>>
>>>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>>>> netcp: add more __le32 annotations
>>>>>>
>>>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>>>> netcp: try to reduce type confusion in descriptors
>>>>>>
>>>>> The middle patch should have no effect on generated code, so I'm ignoring
>>>>> that for now.
>>>>>
>>>>> The next thing to rule out is an endianess bug. I assume you
>>>>> are running this on with a little-endian kernel, correct? If
>>>>> you are running big-endian, the base assumption that the driver needs
>>>>> to swap the data was flawed and that portion needs to be done.
>>>>>
>>>>> If you are running little-endian 32-bit, please try the partial
>>>>> revert below, which just undoes the attempt to make it work with
>>>>> 64-bit kernels.
>>>> Keystone 2 devices are little-endian 32-bit devices.
>>> I meant the kernel you are running on it, not the hardware.
>>> You should always be able to run both a big-endian kernel and
>>> a littl-endian kernel on any ARMv7 machine, and a couple of
>>> platforms use 64-bit physical addresses even on 32-bit machines
>>> (with the normal 32-bit instruction set).
>> I'm not sure if Keystone 2 devices support this or if we
>> have support for this. I'll have to double check.
>>> I wasn't completely sure if there are already keystone-derived
>>> products with 64-bit CPU cores, but I guess the driver would
>>> fail really badly on those (with or without the patch).
>>>
>>>> This partial revert fixes the boot problem for me.
>>> Ok.
>>>
>>>
>>> I tried to create a smaller version and stumbled over
>>> a typo, maybe that's the whole problem. Can you try this one:
>>>
>>> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
>>> index c61d66d38634..8490804416dd 100644
>>> --- a/drivers/net/ethernet/ti/netcp_core.c
>>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>>> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *des
>>>   {
>>>   	desc->pad[0] = cpu_to_le32(pad0);
>>>   	desc->pad[1] = cpu_to_le32(pad1);
>>> -	desc->pad[2] = cpu_to_le32(pad1);
>>> +	desc->pad[2] = cpu_to_le32(pad2);
>>>   }
>>>   
>>>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>>
>>>
>> So only making this change on the latest master with no
>> other changes I see the boot problem again.

yep. I can confirm that.

Also, I'm today came up with the similar fix that you've proposed before in this thread.
So, Could we move forward this way?


>From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Wed, 3 Feb 2016 15:11:40 +0200
Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality

The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
introduces a regression in Kernel 4.5-rc1 as it breaks
get/set_pad_info() functionality.

The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
store DMA/MEM buffer pointer and buffer size respectively. And in both
cases the pointer type size is 32 bit regardless of LAPE enabled or
not.
			!LAPE	LPAE
sizeof(void*)		32bit	32bit
sizeof(dma_addr_t) 	32bit	32bit
sizeof(phys_addr_t) 	32bit	64bit

Unfortunately, above commit changed buffer's pointers save/restore
code (get/set_pad_info()) and added intermediate conversation to u64
which causes TI NETCP driver crash in RX/TX path due to "Unable to
handle kernel NULL pointer" exception.

Hence, fix it by partially reverting above commit and restoring
get/set_pad_info() functionality as it was before.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/netcp_core.c | 63 +++++++++++-------------------------
 1 file changed, 19 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index c61d66d..c8eafc6 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -117,20 +117,10 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
 	*ndesc = le32_to_cpu(desc->next_desc);
 }
 
-static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
+static void get_pad_info(u32 *pad0, u32 *pad1, struct knav_dma_desc *desc)
 {
 	*pad0 = le32_to_cpu(desc->pad[0]);
 	*pad1 = le32_to_cpu(desc->pad[1]);
-	*pad2 = le32_to_cpu(desc->pad[2]);
-}
-
-static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
-{
-	u64 pad64;
-
-	pad64 = le32_to_cpu(desc->pad[0]) +
-		((u64)le32_to_cpu(desc->pad[1]) << 32);
-	*padptr = (void *)(uintptr_t)pad64;
 }
 
 static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
@@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
 	desc->packet_info = cpu_to_le32(pkt_info);
 }
 
-static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
+static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
 {
 	desc->pad[0] = cpu_to_le32(pad0);
 	desc->pad[1] = cpu_to_le32(pad1);
-	desc->pad[2] = cpu_to_le32(pad1);
 }
 
 static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
@@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 	dma_addr_t dma_desc, dma_buf;
 	unsigned int buf_len, dma_sz = sizeof(*ndesc);
 	void *buf_ptr;
-	u32 pad[2];
 	u32 tmp;
 
 	get_words(&dma_desc, 1, &desc->next_desc);
@@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 			break;
 		}
 		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
-		get_pad_ptr(&buf_ptr, ndesc);
+		get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);
 		dma_unmap_page(netcp->dev, dma_buf, PAGE_SIZE, DMA_FROM_DEVICE);
 		__free_page(buf_ptr);
 		knav_pool_desc_put(netcp->rx_pool, desc);
 	}
-
-	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
-	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
+	get_pad_info((u32 *)&buf_ptr, &buf_len, desc);
 
 	if (buf_ptr)
 		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
@@ -639,8 +625,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	dma_addr_t dma_desc, dma_buff;
 	struct netcp_packet p_info;
 	struct sk_buff *skb;
-	u32 pad[2];
 	void *org_buf_ptr;
+	u32 tmp;
 
 	dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz);
 	if (!dma_desc)
@@ -653,8 +639,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	}
 
 	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
-	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
-	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
+	get_pad_info((u32 *)&org_buf_ptr, &org_buf_len, desc);
 
 	if (unlikely(!org_buf_ptr)) {
 		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
@@ -679,7 +664,6 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	/* Fill in the page fragment list */
 	while (dma_desc) {
 		struct page *page;
-		void *ptr;
 
 		ndesc = knav_pool_desc_unmap(netcp->rx_pool, dma_desc, dma_sz);
 		if (unlikely(!ndesc)) {
@@ -688,8 +672,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 		}
 
 		get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc);
-		get_pad_ptr(&ptr, ndesc);
-		page = ptr;
+		get_pad_info((u32 *)&page, &tmp, ndesc);
 
 		if (likely(dma_buff && buf_len && page)) {
 			dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE,
@@ -767,6 +750,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
 	unsigned int buf_len, dma_sz;
 	dma_addr_t dma;
 	void *buf_ptr;
+	u32 tmp;
 
 	/* Allocate descriptor */
 	while ((dma = knav_queue_pop(netcp->rx_fdq[fdq], &dma_sz))) {
@@ -777,7 +761,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 
 		get_org_pkt_info(&dma, &buf_len, desc);
-		get_pad_ptr(&buf_ptr, desc);
+		get_pad_info((u32 *)&buf_ptr, &tmp, desc);
 
 		if (unlikely(!dma)) {
 			dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n");
@@ -829,7 +813,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 	struct page *page;
 	dma_addr_t dma;
 	void *bufptr;
-	u32 pad[3];
+	u32 pad[2];
 
 	/* Allocate descriptor */
 	hwdesc = knav_pool_desc_get(netcp->rx_pool);
@@ -846,7 +830,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
 		bufptr = netdev_alloc_frag(primary_buf_len);
-		pad[2] = primary_buf_len;
+		pad[1] = primary_buf_len;
 
 		if (unlikely(!bufptr)) {
 			dev_warn_ratelimited(netcp->ndev_dev,
@@ -858,9 +842,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		if (unlikely(dma_mapping_error(netcp->dev, dma)))
 			goto fail;
 
-		pad[0] = lower_32_bits((uintptr_t)bufptr);
-		pad[1] = upper_32_bits((uintptr_t)bufptr);
-
+		pad[0] = (u32)bufptr;
 	} else {
 		/* Allocate a secondary receive queue entry */
 		page = alloc_page(GFP_ATOMIC | GFP_DMA | __GFP_COLD);
@@ -870,9 +852,8 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 		buf_len = PAGE_SIZE;
 		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
-		pad[0] = lower_32_bits(dma);
-		pad[1] = upper_32_bits(dma);
-		pad[2] = 0;
+		pad[0] = (u32)page;
+		pad[1] = 0;
 	}
 
 	desc_info =  KNAV_DMA_DESC_PS_INFO_IN_DESC;
@@ -882,7 +863,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
 		    KNAV_DMA_DESC_RETQ_SHIFT;
 	set_org_pkt_info(dma, buf_len, hwdesc);
-	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
+	set_pad_info(pad[0], pad[1], hwdesc);
 	set_desc_info(desc_info, pkt_info, hwdesc);
 
 	/* Push to FDQs */
@@ -971,11 +952,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 					  unsigned int budget)
 {
 	struct knav_dma_desc *desc;
-	void *ptr;
 	struct sk_buff *skb;
 	unsigned int dma_sz;
 	dma_addr_t dma;
 	int pkts = 0;
+	u32 tmp;
 
 	while (budget--) {
 		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
@@ -988,8 +969,7 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 			continue;
 		}
 
-		get_pad_ptr(&ptr, desc);
-		skb = ptr;
+		get_pad_info((u32 *)&skb, &tmp, desc);
 		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
 		if (!skb) {
 			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
@@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
 		u32 page_offset = frag->page_offset;
 		u32 buf_len = skb_frag_size(frag);
 		dma_addr_t desc_dma;
-		u32 desc_dma_32;
 		u32 pkt_info;
 
 		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
@@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
 			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
 				KNAV_DMA_DESC_RETQ_SHIFT;
 		set_pkt_info(dma_addr, buf_len, 0, ndesc);
-		desc_dma_32 = (u32)desc_dma;
-		set_words(&desc_dma_32, 1, &pdesc->next_desc);
+		set_words(&desc_dma, 1, &pdesc->next_desc);
 		pkt_len += buf_len;
 		if (pdesc != desc)
 			knav_pool_desc_map(netcp->tx_pool, pdesc,
@@ -1194,10 +1172,7 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
 	}
 
 	set_words(&tmp, 1, &desc->packet_info);
-	tmp = lower_32_bits((uintptr_t)&skb);
-	set_words(&tmp, 1, &desc->pad[0]);
-	tmp = upper_32_bits((uintptr_t)&skb);
-	set_words(&tmp, 1, &desc->pad[1]);
+	set_words((u32 *)&skb, 1, &desc->pad[0]);
 
 	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
 		tmp = tx_pipe->switch_to_port;
-- 
2.7.0





-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Franklin S Cooper Jr.]

On 02/03/2016 08:21 AM, Grygorii Strashko wrote:
> Hi Arnd,
>
> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>>>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>>>> Yes. Here is a boot log on the latest master with the below
>>>>>>> three patches reverted.
>>>>>>> http://pastebin.com/W7RWSHpE (Working)
>>>>>>>
>>>>>>> I reverted these three patches. The two latest patches seem
>>>>>>> to be trying to correct/expand upon the last patch on this list.
>>>>>>>
>>>>>>> commit 958d104e3d40eef5148c402887138f6594ff7e1e
>>>>>>> netcp: fix regression in receive processing
>>>>>>>
>>>>>>> commit 9dd2d6c5c9755b160fe0111bcdad9491676feea8
>>>>>>> netcp: add more __le32 annotations
>>>>>>>
>>>>>>> commit 899077791403ff7a2d8cfaa87bd1a82d729463e2
>>>>>>> netcp: try to reduce type confusion in descriptors
>>>>>>>
>>>>>> The middle patch should have no effect on generated code, so I'm ignoring
>>>>>> that for now.
>>>>>>
>>>>>> The next thing to rule out is an endianess bug. I assume you
>>>>>> are running this on with a little-endian kernel, correct? If
>>>>>> you are running big-endian, the base assumption that the driver needs
>>>>>> to swap the data was flawed and that portion needs to be done.
>>>>>>
>>>>>> If you are running little-endian 32-bit, please try the partial
>>>>>> revert below, which just undoes the attempt to make it work with
>>>>>> 64-bit kernels.
>>>>> Keystone 2 devices are little-endian 32-bit devices.
>>>> I meant the kernel you are running on it, not the hardware.
>>>> You should always be able to run both a big-endian kernel and
>>>> a littl-endian kernel on any ARMv7 machine, and a couple of
>>>> platforms use 64-bit physical addresses even on 32-bit machines
>>>> (with the normal 32-bit instruction set).
>>> I'm not sure if Keystone 2 devices support this or if we
>>> have support for this. I'll have to double check.
>>>> I wasn't completely sure if there are already keystone-derived
>>>> products with 64-bit CPU cores, but I guess the driver would
>>>> fail really badly on those (with or without the patch).
>>>>
>>>>> This partial revert fixes the boot problem for me.
>>>> Ok.
>>>>
>>>>
>>>> I tried to create a smaller version and stumbled over
>>>> a typo, maybe that's the whole problem. Can you try this one:
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
>>>> index c61d66d38634..8490804416dd 100644
>>>> --- a/drivers/net/ethernet/ti/netcp_core.c
>>>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>>>> @@ -167,7 +167,7 @@ static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *des
>>>>   {
>>>>   	desc->pad[0] = cpu_to_le32(pad0);
>>>>   	desc->pad[1] = cpu_to_le32(pad1);
>>>> -	desc->pad[2] = cpu_to_le32(pad1);
>>>> +	desc->pad[2] = cpu_to_le32(pad2);
>>>>   }
>>>>   
>>>>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>>>
>>>>
>>> So only making this change on the latest master with no
>>> other changes I see the boot problem again.
> yep. I can confirm that.
>
> Also, I'm today came up with the similar fix that you've proposed before in this thread.
> So, Could we move forward this way?
>
>
> From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Wed, 3 Feb 2016 15:11:40 +0200
> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
>
> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
> introduces a regression in Kernel 4.5-rc1 as it breaks
> get/set_pad_info() functionality.
>
> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
> store DMA/MEM buffer pointer and buffer size respectively. And in both
> cases the pointer type size is 32 bit regardless of LAPE enabled or
> not.
> 			!LAPE	LPAE
> sizeof(void*)		32bit	32bit
> sizeof(dma_addr_t) 	32bit	32bit
> sizeof(phys_addr_t) 	32bit	64bit
>
> Unfortunately, above commit changed buffer's pointers save/restore
> code (get/set_pad_info()) and added intermediate conversation to u64
> which causes TI NETCP driver crash in RX/TX path due to "Unable to
> handle kernel NULL pointer" exception.
>
> Hence, fix it by partially reverting above commit and restoring
> get/set_pad_info() functionality as it was before.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/net/ethernet/ti/netcp_core.c | 63 +++++++++++-------------------------
>  1 file changed, 19 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d..c8eafc6 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -117,20 +117,10 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>  	*ndesc = le32_to_cpu(desc->next_desc);
>  }
>  
> -static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
> +static void get_pad_info(u32 *pad0, u32 *pad1, struct knav_dma_desc *desc)
>  {
>  	*pad0 = le32_to_cpu(desc->pad[0]);
>  	*pad1 = le32_to_cpu(desc->pad[1]);
> -	*pad2 = le32_to_cpu(desc->pad[2]);
> -}
> -
> -static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
> -{
> -	u64 pad64;
> -
> -	pad64 = le32_to_cpu(desc->pad[0]) +
> -		((u64)le32_to_cpu(desc->pad[1]) << 32);
> -	*padptr = (void *)(uintptr_t)pad64;
>  }
>  
>  static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>  	desc->packet_info = cpu_to_le32(pkt_info);
>  }
>  
> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>  {
>  	desc->pad[0] = cpu_to_le32(pad0);
>  	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
>  }
>  
>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  	dma_addr_t dma_desc, dma_buf;
>  	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>  	void *buf_ptr;
> -	u32 pad[2];
>  	u32 tmp;
>  
>  	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  			break;
>  		}
>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
> -		get_pad_ptr(&buf_ptr, ndesc);
> +		get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);
>  		dma_unmap_page(netcp->dev, dma_buf, PAGE_SIZE, DMA_FROM_DEVICE);
>  		__free_page(buf_ptr);
>  		knav_pool_desc_put(netcp->rx_pool, desc);
>  	}
> -
> -	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
> -	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info((u32 *)&buf_ptr, &buf_len, desc);
>  
>  	if (buf_ptr)
>  		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
> @@ -639,8 +625,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	dma_addr_t dma_desc, dma_buff;
>  	struct netcp_packet p_info;
>  	struct sk_buff *skb;
> -	u32 pad[2];
>  	void *org_buf_ptr;
> +	u32 tmp;
>  
>  	dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz);
>  	if (!dma_desc)
> @@ -653,8 +639,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	}
>  
>  	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
> -	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
> -	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info((u32 *)&org_buf_ptr, &org_buf_len, desc);
>  
>  	if (unlikely(!org_buf_ptr)) {
>  		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
> @@ -679,7 +664,6 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	/* Fill in the page fragment list */
>  	while (dma_desc) {
>  		struct page *page;
> -		void *ptr;
>  
>  		ndesc = knav_pool_desc_unmap(netcp->rx_pool, dma_desc, dma_sz);
>  		if (unlikely(!ndesc)) {
> @@ -688,8 +672,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  		}
>  
>  		get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc);
> -		get_pad_ptr(&ptr, ndesc);
> -		page = ptr;
> +		get_pad_info((u32 *)&page, &tmp, ndesc);
>  
>  		if (likely(dma_buff && buf_len && page)) {
>  			dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE,
> @@ -767,6 +750,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
>  	unsigned int buf_len, dma_sz;
>  	dma_addr_t dma;
>  	void *buf_ptr;
> +	u32 tmp;
>  
>  	/* Allocate descriptor */
>  	while ((dma = knav_queue_pop(netcp->rx_fdq[fdq], &dma_sz))) {
> @@ -777,7 +761,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
>  		}
>  
>  		get_org_pkt_info(&dma, &buf_len, desc);
> -		get_pad_ptr(&buf_ptr, desc);
> +		get_pad_info((u32 *)&buf_ptr, &tmp, desc);
>  
>  		if (unlikely(!dma)) {
>  			dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n");
> @@ -829,7 +813,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  	struct page *page;
>  	dma_addr_t dma;
>  	void *bufptr;
> -	u32 pad[3];
> +	u32 pad[2];
>  
>  	/* Allocate descriptor */
>  	hwdesc = knav_pool_desc_get(netcp->rx_pool);
> @@ -846,7 +830,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>  
>  		bufptr = netdev_alloc_frag(primary_buf_len);
> -		pad[2] = primary_buf_len;
> +		pad[1] = primary_buf_len;
>  
>  		if (unlikely(!bufptr)) {
>  			dev_warn_ratelimited(netcp->ndev_dev,
> @@ -858,9 +842,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		if (unlikely(dma_mapping_error(netcp->dev, dma)))
>  			goto fail;
>  
> -		pad[0] = lower_32_bits((uintptr_t)bufptr);
> -		pad[1] = upper_32_bits((uintptr_t)bufptr);
> -
> +		pad[0] = (u32)bufptr;
>  	} else {
>  		/* Allocate a secondary receive queue entry */
>  		page = alloc_page(GFP_ATOMIC | GFP_DMA | __GFP_COLD);
> @@ -870,9 +852,8 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		}
>  		buf_len = PAGE_SIZE;
>  		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
> -		pad[0] = lower_32_bits(dma);
> -		pad[1] = upper_32_bits(dma);
> -		pad[2] = 0;
> +		pad[0] = (u32)page;
> +		pad[1] = 0;
>  	}
>  
>  	desc_info =  KNAV_DMA_DESC_PS_INFO_IN_DESC;
> @@ -882,7 +863,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>  		    KNAV_DMA_DESC_RETQ_SHIFT;
>  	set_org_pkt_info(dma, buf_len, hwdesc);
> -	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
> +	set_pad_info(pad[0], pad[1], hwdesc);
>  	set_desc_info(desc_info, pkt_info, hwdesc);
>  
>  	/* Push to FDQs */
> @@ -971,11 +952,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>  					  unsigned int budget)
>  {
>  	struct knav_dma_desc *desc;
> -	void *ptr;
>  	struct sk_buff *skb;
>  	unsigned int dma_sz;
>  	dma_addr_t dma;
>  	int pkts = 0;
> +	u32 tmp;
>  
>  	while (budget--) {
>  		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
> @@ -988,8 +969,7 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>  			continue;
>  		}
>  
> -		get_pad_ptr(&ptr, desc);
> -		skb = ptr;
> +		get_pad_info((u32 *)&skb, &tmp, desc);
>  		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
>  		if (!skb) {
>  			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
> @@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  		u32 page_offset = frag->page_offset;
>  		u32 buf_len = skb_frag_size(frag);
>  		dma_addr_t desc_dma;
> -		u32 desc_dma_32;
>  		u32 pkt_info;
>  
>  		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>  				KNAV_DMA_DESC_RETQ_SHIFT;
>  		set_pkt_info(dma_addr, buf_len, 0, ndesc);
> -		desc_dma_32 = (u32)desc_dma;
> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>  		pkt_len += buf_len;
>  		if (pdesc != desc)
>  			knav_pool_desc_map(netcp->tx_pool, pdesc,
> @@ -1194,10 +1172,7 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>  	}
>  
>  	set_words(&tmp, 1, &desc->packet_info);
> -	tmp = lower_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[0]);
> -	tmp = upper_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[1]);
> +	set_words((u32 *)&skb, 1, &desc->pad[0]);
>  
>  	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>  		tmp = tx_pipe->switch_to_port;

This worked for me.

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
> > On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
> >>>
> >> So only making this change on the latest master with no
> >> other changes I see the boot problem again.
> 
> yep. I can confirm that.
> 
> Also, I'm today came up with the similar fix that you've proposed before in this thread.
> So, Could we move forward this way?

I still think it would be good to actually understand what the actual
problem was.

> From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Wed, 3 Feb 2016 15:11:40 +0200
> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
> 
> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
> introduces a regression in Kernel 4.5-rc1 as it breaks
> get/set_pad_info() functionality.
> 
> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
> store DMA/MEM buffer pointer and buffer size respectively. And in both
> cases the pointer type size is 32 bit regardless of LAPE enabled or
> not.
> 			!LAPE	LPAE
> sizeof(void*)		32bit	32bit
> sizeof(dma_addr_t) 	32bit	32bit
> sizeof(phys_addr_t) 	32bit	64bit
> 

This looks wrong: I was getting the build warnings originally
because of 64-bit dma_addr_t, and that should be the only way that
this driver can operate, because in some configurations on keystone
there is no memory below 4GB, and there is no dma-ranges property
in the DT that shifts around the start of the DMA addresses.

This doesn't all fit together yet, maybe you have a better idea
of what is going on.

I don't think we should ever have a platform that has dma_addr_t
and phys_addr_t be different.

> Unfortunately, above commit changed buffer's pointers save/restore
> code (get/set_pad_info()) and added intermediate conversation to u64
> which causes TI NETCP driver crash in RX/TX path due to "Unable to
> handle kernel NULL pointer" exception.

The conversion is not what made it crash, it must be a bug in the
conversion ;-)

> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>  	desc->packet_info = cpu_to_le32(pkt_info);
>  }
>  
> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>  {
>  	desc->pad[0] = cpu_to_le32(pad0);
>  	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
>  }

As in my earlier patch, the line you are removing here was clearly
broken, but evidently that is not the only bug.

>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  	dma_addr_t dma_desc, dma_buf;
>  	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>  	void *buf_ptr;
> -	u32 pad[2];
>  	u32 tmp;
>  
>  	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  			break;
>  		}
>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
> -		get_pad_ptr(&buf_ptr, ndesc);
> +		get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);

I'd prefer not to put code like this back, as this cannot possibly
do the right thing on a 64-bit architecture.


> @@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  		u32 page_offset = frag->page_offset;
>  		u32 buf_len = skb_frag_size(frag);
>  		dma_addr_t desc_dma;
> -		u32 desc_dma_32;
>  		u32 pkt_info;
>  
>  		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>  			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>  				KNAV_DMA_DESC_RETQ_SHIFT;
>  		set_pkt_info(dma_addr, buf_len, 0, ndesc);
> -		desc_dma_32 = (u32)desc_dma;
> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>  		pkt_len += buf_len;
>  		if (pdesc != desc)
>  			knav_pool_desc_map(netcp->tx_pool, pdesc,

This is clearly broken on big-endian kernels with 64-bit dma_addr_t, so even
if we revert the rest I think this part has to stay.

I have another version for testing below. That removes the logic that
splits and reassembles the 64-bit values, but leaves the other changes
in place. Can you try this?

	Arnd


diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index c61d66d38634..cda19f2401c1 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -120,16 +120,14 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
 static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
 {
 	*pad0 = le32_to_cpu(desc->pad[0]);
-	*pad1 = le32_to_cpu(desc->pad[1]);
-	*pad2 = le32_to_cpu(desc->pad[2]);
+	*pad2 = le32_to_cpu(desc->pad[1]);
 }
 
 static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
 {
 	u64 pad64;
 
-	pad64 = le32_to_cpu(desc->pad[0]) +
-		((u64)le32_to_cpu(desc->pad[1]) << 32);
+	pad64 = le32_to_cpu(desc->pad[0]);
 	*padptr = (void *)(uintptr_t)pad64;
 }
 
@@ -166,8 +164,7 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
 static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
 {
 	desc->pad[0] = cpu_to_le32(pad0);
-	desc->pad[1] = cpu_to_le32(pad1);
-	desc->pad[2] = cpu_to_le32(pad1);
+	desc->pad[1] = cpu_to_le32(pad2);
 }
 
 static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
@@ -581,7 +578,7 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 	dma_addr_t dma_desc, dma_buf;
 	unsigned int buf_len, dma_sz = sizeof(*ndesc);
 	void *buf_ptr;
-	u32 pad[2];
+	u32 pad;
 	u32 tmp;
 
 	get_words(&dma_desc, 1, &desc->next_desc);
@@ -599,8 +596,8 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 		knav_pool_desc_put(netcp->rx_pool, desc);
 	}
 
-	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
-	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
+	get_pad_info(&pad, NULL, &buf_len, desc);
+	buf_ptr = (void *)(uintptr_t)(pad);
 
 	if (buf_ptr)
 		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
@@ -653,8 +650,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	}
 
 	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
-	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
-	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
+	get_pad_info(&pad[0], NULL, &org_buf_len, desc);
+	org_buf_ptr = (void *)(uintptr_t)(pad[0]);
 
 	if (unlikely(!org_buf_ptr)) {
 		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
@@ -858,8 +855,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		if (unlikely(dma_mapping_error(netcp->dev, dma)))
 			goto fail;
 
-		pad[0] = lower_32_bits((uintptr_t)bufptr);
-		pad[1] = upper_32_bits((uintptr_t)bufptr);
+		pad[0] = (uintptr_t)bufptr;
 
 	} else {
 		/* Allocate a secondary receive queue entry */
@@ -870,8 +866,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 		buf_len = PAGE_SIZE;
 		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
-		pad[0] = lower_32_bits(dma);
-		pad[1] = upper_32_bits(dma);
+		pad[0] = (u32)(dma);
 		pad[2] = 0;
 	}
 
@@ -882,7 +877,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
 		    KNAV_DMA_DESC_RETQ_SHIFT;
 	set_org_pkt_info(dma, buf_len, hwdesc);
-	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
+	set_pad_info(pad[0], 0, pad[2], hwdesc);
 	set_desc_info(desc_info, pkt_info, hwdesc);
 
 	/* Push to FDQs */
@@ -1194,10 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
 	}
 
 	set_words(&tmp, 1, &desc->packet_info);
-	tmp = lower_32_bits((uintptr_t)&skb);
+	tmp = (uintptr_t)&skb;
 	set_words(&tmp, 1, &desc->pad[0]);
-	tmp = upper_32_bits((uintptr_t)&skb);
-	set_words(&tmp, 1, &desc->pad[1]);
 
 	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
 		tmp = tx_pipe->switch_to_port;

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>>>
>>>> So only making this change on the latest master with no
>>>> other changes I see the boot problem again.
>>
>> yep. I can confirm that.
>>
>> Also, I'm today came up with the similar fix that you've proposed before in this thread.
>> So, Could we move forward this way?
>
> I still think it would be good to actually understand what the actual
> problem was.
>
>>  From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>> Date: Wed, 3 Feb 2016 15:11:40 +0200
>> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
>>
>> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
>> introduces a regression in Kernel 4.5-rc1 as it breaks
>> get/set_pad_info() functionality.
>>
>> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
>> store DMA/MEM buffer pointer and buffer size respectively. And in both
>> cases the pointer type size is 32 bit regardless of LAPE enabled or
>> not.
>> 			!LAPE	LPAE
>> sizeof(void*)		32bit	32bit
>> sizeof(dma_addr_t) 	32bit	32bit
>> sizeof(phys_addr_t) 	32bit	64bit
>>
>
> This looks wrong: I was getting the build warnings originally
> because of 64-bit dma_addr_t, and that should be the only way that
> this driver can operate, because in some configurations on keystone
> there is no memory below 4GB, and there is no dma-ranges property
> in the DT that shifts around the start of the DMA addresses.

see keystone.dtsi:
	soc {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "ti,keystone","simple-bus";
		interrupt-parent = <&gic>;
		ranges = <0x0 0x0 0x0 0xc0000000>;
		dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

config:

CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
CONFIG_PHYS_ADDR_T_64BIT=y

and

#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
typedef u64 dma_addr_t;
#else
typedef u32 dma_addr_t;
#endif

Above is valid configuration for Keystone 2 with LPAE=y

>
> This doesn't all fit together yet, maybe you have a better idea
> of what is going on.
>
> I don't think we should ever have a platform that has dma_addr_t
> and phys_addr_t be different.

This is a valid case.

>
>> Unfortunately, above commit changed buffer's pointers save/restore
>> code (get/set_pad_info()) and added intermediate conversation to u64
>> which causes TI NETCP driver crash in RX/TX path due to "Unable to
>> handle kernel NULL pointer" exception.
>
> The conversion is not what made it crash, it must be a bug in the
> conversion ;-)
>
>> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>>   	desc->packet_info = cpu_to_le32(pkt_info);
>>   }
>>
>> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>>   {
>>   	desc->pad[0] = cpu_to_le32(pad0);
>>   	desc->pad[1] = cpu_to_le32(pad1);
>> -	desc->pad[2] = cpu_to_le32(pad1);
>>   }
>
> As in my earlier patch, the line you are removing here was clearly
> broken, but evidently that is not the only bug.
>
>>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>   	dma_addr_t dma_desc, dma_buf;
>>   	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>>   	void *buf_ptr;
>> -	u32 pad[2];
>>   	u32 tmp;
>>
>>   	get_words(&dma_desc, 1, &desc->next_desc);
>> @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>   			break;
>>   		}
>>   		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
>> -		get_pad_ptr(&buf_ptr, ndesc);
>> +		get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);
>
> I'd prefer not to put code like this back, as this cannot possibly
> do the right thing on a 64-bit architecture.
>
>
>> @@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>   		u32 page_offset = frag->page_offset;
>>   		u32 buf_len = skb_frag_size(frag);
>>   		dma_addr_t desc_dma;
>> -		u32 desc_dma_32;
>>   		u32 pkt_info;
>>
>>   		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
>> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>   			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>>   				KNAV_DMA_DESC_RETQ_SHIFT;
>>   		set_pkt_info(dma_addr, buf_len, 0, ndesc);
>> -		desc_dma_32 = (u32)desc_dma;
>> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
>> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>>   		pkt_len += buf_len;
>>   		if (pdesc != desc)
>>   			knav_pool_desc_map(netcp->tx_pool, pdesc,
>
> This is clearly broken on big-endian kernels with 64-bit dma_addr_t, so even
> if we revert the rest I think this part has to stay.

Keystone 2 has 32-bit dma_addr_t

>
> I have another version for testing below. That removes the logic that
> splits and reassembles the 64-bit values, but leaves the other changes
> in place. Can you try this?

Sry, but we really don't need this logic for KS2 ;)


>
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..cda19f2401c1 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -120,16 +120,14 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>   static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
>   {
>   	*pad0 = le32_to_cpu(desc->pad[0]);
> -	*pad1 = le32_to_cpu(desc->pad[1]);
> -	*pad2 = le32_to_cpu(desc->pad[2]);
> +	*pad2 = le32_to_cpu(desc->pad[1]);
>   }
>
>   static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
>   {
>   	u64 pad64;
>
> -	pad64 = le32_to_cpu(desc->pad[0]) +
> -		((u64)le32_to_cpu(desc->pad[1]) << 32);
> +	pad64 = le32_to_cpu(desc->pad[0]);
>   	*padptr = (void *)(uintptr_t)pad64;
>   }
>
> @@ -166,8 +164,7 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>   static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>   {
>   	desc->pad[0] = cpu_to_le32(pad0);
> -	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
> +	desc->pad[1] = cpu_to_le32(pad2);
>   }
>
>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +578,7 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>   	dma_addr_t dma_desc, dma_buf;
>   	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>   	void *buf_ptr;
> -	u32 pad[2];
> +	u32 pad;
>   	u32 tmp;
>
>   	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -599,8 +596,8 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>   		knav_pool_desc_put(netcp->rx_pool, desc);
>   	}
>
> -	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
> -	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad, NULL, &buf_len, desc);
> +	buf_ptr = (void *)(uintptr_t)(pad);
>
>   	if (buf_ptr)
>   		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
> @@ -653,8 +650,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>   	}
>
>   	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
> -	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
> -	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad[0], NULL, &org_buf_len, desc);
> +	org_buf_ptr = (void *)(uintptr_t)(pad[0]);
>
>   	if (unlikely(!org_buf_ptr)) {
>   		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
> @@ -858,8 +855,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   		if (unlikely(dma_mapping_error(netcp->dev, dma)))
>   			goto fail;
>
> -		pad[0] = lower_32_bits((uintptr_t)bufptr);
> -		pad[1] = upper_32_bits((uintptr_t)bufptr);
> +		pad[0] = (uintptr_t)bufptr;
>
>   	} else {
>   		/* Allocate a secondary receive queue entry */
> @@ -870,8 +866,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   		}
>   		buf_len = PAGE_SIZE;
>   		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
> -		pad[0] = lower_32_bits(dma);
> -		pad[1] = upper_32_bits(dma);
> +		pad[0] = (u32)(dma);
>   		pad[2] = 0;
>   	}
>
> @@ -882,7 +877,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>   		    KNAV_DMA_DESC_RETQ_SHIFT;
>   	set_org_pkt_info(dma, buf_len, hwdesc);
> -	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
> +	set_pad_info(pad[0], 0, pad[2], hwdesc);
>   	set_desc_info(desc_info, pkt_info, hwdesc);
>
>   	/* Push to FDQs */
> @@ -1194,10 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>   	}
>
>   	set_words(&tmp, 1, &desc->packet_info);
> -	tmp = lower_32_bits((uintptr_t)&skb);
> +	tmp = (uintptr_t)&skb;
>   	set_words(&tmp, 1, &desc->pad[0]);
> -	tmp = upper_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[1]);
>
>   	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>   		tmp = tx_pipe->switch_to_port;
>


-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Murali Karicheri]

On 02/03/2016 11:31 AM, Grygorii Strashko wrote:
> On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
>> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>>>>
>>>>> So only making this change on the latest master with no
>>>>> other changes I see the boot problem again.
>>>
>>> yep. I can confirm that.
>>>
>>> Also, I'm today came up with the similar fix that you've proposed before in this thread.
>>> So, Could we move forward this way?
>>
>> I still think it would be good to actually understand what the actual
>> problem was.
>>
>>>  From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
>>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>> Date: Wed, 3 Feb 2016 15:11:40 +0200
>>> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
>>>
>>> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
>>> introduces a regression in Kernel 4.5-rc1 as it breaks
>>> get/set_pad_info() functionality.
>>>
>>> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
>>> store DMA/MEM buffer pointer and buffer size respectively. And in both
>>> cases the pointer type size is 32 bit regardless of LAPE enabled or
>>> not.
>>>             !LAPE    LPAE
>>> sizeof(void*)        32bit    32bit
>>> sizeof(dma_addr_t)     32bit    32bit
>>> sizeof(phys_addr_t)     32bit    64bit
>>>
>>
>> This looks wrong: I was getting the build warnings originally
>> because of 64-bit dma_addr_t, and that should be the only way that
>> this driver can operate, because in some configurations on keystone
>> there is no memory below 4GB, and there is no dma-ranges property
>> in the DT that shifts around the start of the DMA addresses.
> 
> see keystone.dtsi:
>     soc {
>         #address-cells = <1>;
>         #size-cells = <1>;
>         compatible = "ti,keystone","simple-bus";
>         interrupt-parent = <&gic>;
>         ranges = <0x0 0x0 0x0 0xc0000000>;
>         dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> config:
> 
> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
> CONFIG_PHYS_ADDR_T_64BIT=y
> 
> and
> 
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
> 
> Above is valid configuration for Keystone 2 with LPAE=y
> 
>>
>> This doesn't all fit together yet, maybe you have a better idea
>> of what is going on.
>>
>> I don't think we should ever have a platform that has dma_addr_t
>> and phys_addr_t be different.
> 
> This is a valid case.
> 
>>
>>> Unfortunately, above commit changed buffer's pointers save/restore
>>> code (get/set_pad_info()) and added intermediate conversation to u64
>>> which causes TI NETCP driver crash in RX/TX path due to "Unable to
>>> handle kernel NULL pointer" exception.
>>
>> The conversion is not what made it crash, it must be a bug in the
>> conversion ;-)
>>
>>> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>>>       desc->packet_info = cpu_to_le32(pkt_info);
>>>   }
>>>
>>> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>>> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>>>   {
>>>       desc->pad[0] = cpu_to_le32(pad0);
>>>       desc->pad[1] = cpu_to_le32(pad1);
>>> -    desc->pad[2] = cpu_to_le32(pad1);
>>>   }
>>
>> As in my earlier patch, the line you are removing here was clearly
>> broken, but evidently that is not the only bug.
>>
>>>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>>> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>>       dma_addr_t dma_desc, dma_buf;
>>>       unsigned int buf_len, dma_sz = sizeof(*ndesc);
>>>       void *buf_ptr;
>>> -    u32 pad[2];
>>>       u32 tmp;
>>>
>>>       get_words(&dma_desc, 1, &desc->next_desc);
>>> @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>>               break;
>>>           }
>>>           get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
>>> -        get_pad_ptr(&buf_ptr, ndesc);
>>> +        get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);
>>
>> I'd prefer not to put code like this back, as this cannot possibly
>> do the right thing on a 64-bit architecture.
>>
>>
>>> @@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>>           u32 page_offset = frag->page_offset;
>>>           u32 buf_len = skb_frag_size(frag);
>>>           dma_addr_t desc_dma;
>>> -        u32 desc_dma_32;
>>>           u32 pkt_info;
>>>
>>>           dma_addr = dma_map_page(dev, page, page_offset, buf_len,
>>> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>>               (netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>>>                   KNAV_DMA_DESC_RETQ_SHIFT;
>>>           set_pkt_info(dma_addr, buf_len, 0, ndesc);
>>> -        desc_dma_32 = (u32)desc_dma;
>>> -        set_words(&desc_dma_32, 1, &pdesc->next_desc);
>>> +        set_words(&desc_dma, 1, &pdesc->next_desc);
>>>           pkt_len += buf_len;
>>>           if (pdesc != desc)
>>>               knav_pool_desc_map(netcp->tx_pool, pdesc,
>>
>> This is clearly broken on big-endian kernels with 64-bit dma_addr_t, so even
>> if we revert the rest I think this part has to stay.
> 
> Keystone 2 has 32-bit dma_addr_t
> 
>>
>> I have another version for testing below. That removes the logic that
>> splits and reassembles the 64-bit values, but leaves the other changes
>> in place. Can you try this?
> 
> Sry, but we really don't need this logic for KS2 ;)
> 
Just replied as well in my response.

Yes Agree. Currently we don't have a use case for Big endian support either.

Murali
> 
>>
>> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
>> index c61d66d38634..cda19f2401c1 100644
>> --- a/drivers/net/ethernet/ti/netcp_core.c
>> +++ b/drivers/net/ethernet/ti/netcp_core.c
>> @@ -120,16 +120,14 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>>   static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
>>   {
>>       *pad0 = le32_to_cpu(desc->pad[0]);
>> -    *pad1 = le32_to_cpu(desc->pad[1]);
>> -    *pad2 = le32_to_cpu(desc->pad[2]);
>> +    *pad2 = le32_to_cpu(desc->pad[1]);
>>   }
>>
>>   static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
>>   {
>>       u64 pad64;
>>
>> -    pad64 = le32_to_cpu(desc->pad[0]) +
>> -        ((u64)le32_to_cpu(desc->pad[1]) << 32);
>> +    pad64 = le32_to_cpu(desc->pad[0]);
>>       *padptr = (void *)(uintptr_t)pad64;
>>   }
>>
>> @@ -166,8 +164,7 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>>   static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>>   {
>>       desc->pad[0] = cpu_to_le32(pad0);
>> -    desc->pad[1] = cpu_to_le32(pad1);
>> -    desc->pad[2] = cpu_to_le32(pad1);
>> +    desc->pad[1] = cpu_to_le32(pad2);
>>   }
>>
>>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>> @@ -581,7 +578,7 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>       dma_addr_t dma_desc, dma_buf;
>>       unsigned int buf_len, dma_sz = sizeof(*ndesc);
>>       void *buf_ptr;
>> -    u32 pad[2];
>> +    u32 pad;
>>       u32 tmp;
>>
>>       get_words(&dma_desc, 1, &desc->next_desc);
>> @@ -599,8 +596,8 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>           knav_pool_desc_put(netcp->rx_pool, desc);
>>       }
>>
>> -    get_pad_info(&pad[0], &pad[1], &buf_len, desc);
>> -    buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
>> +    get_pad_info(&pad, NULL, &buf_len, desc);
>> +    buf_ptr = (void *)(uintptr_t)(pad);
>>
>>       if (buf_ptr)
>>           netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
>> @@ -653,8 +650,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>>       }
>>
>>       get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
>> -    get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
>> -    org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
>> +    get_pad_info(&pad[0], NULL, &org_buf_len, desc);
>> +    org_buf_ptr = (void *)(uintptr_t)(pad[0]);
>>
>>       if (unlikely(!org_buf_ptr)) {
>>           dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
>> @@ -858,8 +855,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>>           if (unlikely(dma_mapping_error(netcp->dev, dma)))
>>               goto fail;
>>
>> -        pad[0] = lower_32_bits((uintptr_t)bufptr);
>> -        pad[1] = upper_32_bits((uintptr_t)bufptr);
>> +        pad[0] = (uintptr_t)bufptr;
>>
>>       } else {
>>           /* Allocate a secondary receive queue entry */
>> @@ -870,8 +866,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>>           }
>>           buf_len = PAGE_SIZE;
>>           dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
>> -        pad[0] = lower_32_bits(dma);
>> -        pad[1] = upper_32_bits(dma);
>> +        pad[0] = (u32)(dma);
>>           pad[2] = 0;
>>       }
>>
>> @@ -882,7 +877,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>>       pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>>               KNAV_DMA_DESC_RETQ_SHIFT;
>>       set_org_pkt_info(dma, buf_len, hwdesc);
>> -    set_pad_info(pad[0], pad[1], pad[2], hwdesc);
>> +    set_pad_info(pad[0], 0, pad[2], hwdesc);
>>       set_desc_info(desc_info, pkt_info, hwdesc);
>>
>>       /* Push to FDQs */
>> @@ -1194,10 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>>       }
>>
>>       set_words(&tmp, 1, &desc->packet_info);
>> -    tmp = lower_32_bits((uintptr_t)&skb);
>> +    tmp = (uintptr_t)&skb;
>>       set_words(&tmp, 1, &desc->pad[0]);
>> -    tmp = upper_32_bits((uintptr_t)&skb);
>> -    set_words(&tmp, 1, &desc->pad[1]);
>>
>>       if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>>           tmp = tx_pipe->switch_to_port;
>>
> 
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Wednesday 03 February 2016 18:31:00 Grygorii Strashko wrote:
> On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
> > On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
> >> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
> >>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
> >
> > This looks wrong: I was getting the build warnings originally
> > because of 64-bit dma_addr_t, and that should be the only way that
> > this driver can operate, because in some configurations on keystone
> > there is no memory below 4GB, and there is no dma-ranges property
> > in the DT that shifts around the start of the DMA addresses.
> 
> see keystone.dtsi:
> 	soc {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		compatible = "ti,keystone","simple-bus";
> 		interrupt-parent = <&gic>;
> 		ranges = <0x0 0x0 0x0 0xc0000000>;
> 		dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

You are right, I totally missed it when I looked again. I thought it
was correct but then couldn't find it in the dts.

> config:
> 
> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
> CONFIG_PHYS_ADDR_T_64BIT=y
> 
> and
> 
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
> 
> Above is valid configuration for Keystone 2 with LPAE=y

Ok, but what do you mean with "should not be defined"? It clearly is
defined in any multiplatform configuration that enables another platform
needing 64-bit dma_addr_t.


	Arnd

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

Hi Arnd,

On 02/03/2016 10:40 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 18:31:00 Grygorii Strashko wrote:
>> On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
>>> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>>>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>
>>> This looks wrong: I was getting the build warnings originally
>>> because of 64-bit dma_addr_t, and that should be the only way that
>>> this driver can operate, because in some configurations on keystone
>>> there is no memory below 4GB, and there is no dma-ranges property
>>> in the DT that shifts around the start of the DMA addresses.
>>
>> see keystone.dtsi:
>> 	soc {
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		compatible = "ti,keystone","simple-bus";
>> 		interrupt-parent = <&gic>;
>> 		ranges = <0x0 0x0 0x0 0xc0000000>;
>> 		dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
>> 		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> You are right, I totally missed it when I looked again. I thought it
> was correct but then couldn't find it in the dts.
> 
>> config:
>>
>> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
>> CONFIG_PHYS_ADDR_T_64BIT=y
>>
>> and
>>
>> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
>> typedef u64 dma_addr_t;
>> #else
>> typedef u32 dma_addr_t;
>> #endif
>>
>> Above is valid configuration for Keystone 2 with LPAE=y
> 
> Ok, but what do you mean with "should not be defined"? It clearly is
> defined in any multiplatform configuration that enables another platform
> needing 64-bit dma_addr_t.
> 

Then we probably have bigger problem :) KS2 will not work as is with
such configuration and not only KS2 - LPAE is enabled for TI DRA7 also.

Problem here is that dma_addr_t is used to fill DMA controllers data or can be 
written directly to register, so all drivers need to be revised which was initially
created for 32-bit HW and with assumption that dma_addr_t is 32-bits.

Also, I'm not sure that it will be possible to support both LE/BE in such case.

Actually, I've tried current multi_v7_defconfig and can see:
# CONFIG_ARCH_PHYS_ADDR_T_64BIT is not set
# CONFIG_PHYS_ADDR_T_64BIT is not set
# CONFIG_ARM_LPAE is not set
What is your "multiplatform configuration" ??

So I propose to fix this regression first (as we both did - revert changes in 
get/set_pad_info()) and have KS2 working again with current version of
defconfig files (keystone_defconfig & multi_v7_defconfig) while this discussion is continuing. 

-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Thursday 04 February 2016 14:19:54 Grygorii Strashko wrote:
> On 02/03/2016 10:40 PM, Arnd Bergmann wrote:
> > On Wednesday 03 February 2016 18:31:00 Grygorii Strashko wrote:
> >> On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
> >>> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
> >>>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
> >>>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
> >> config:
> >>
> >> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
> >> CONFIG_PHYS_ADDR_T_64BIT=y
> >>
> >> and
> >>
> >> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
> >> typedef u64 dma_addr_t;
> >> #else
> >> typedef u32 dma_addr_t;
> >> #endif
> >>
> >> Above is valid configuration for Keystone 2 with LPAE=y
> > 
> > Ok, but what do you mean with "should not be defined"? It clearly is
> > defined in any multiplatform configuration that enables another platform
> > needing 64-bit dma_addr_t.
> > 
> 
> Then we probably have bigger problem :) KS2 will not work as is with
> such configuration and not only KS2 - LPAE is enabled for TI DRA7 also.
> 
> Problem here is that dma_addr_t is used to fill DMA controllers data or can be 
> written directly to register, so all drivers need to be revised which was initially
> created for 32-bit HW and with assumption that dma_addr_t is 32-bits.

Almost everything should work fine. What all other drivers tend to do is
something like

	writel(dma_addr, reg_base + REG_OFFSET);

which works for 64-bit dma_addr_t as long as the upper 32 bits are
always zero, and that should be the case on keystone.

The part that does not work and that originally triggered the
warning I was fixing is

void f(u32 *data)
{
	writel(*data, reg_base + reg_offset);
}

...

	f((u32 *)&dma_addr);

as this driver does. I have not seen the same warning for any
other driver in the kernel, so it is clearly something that
rarely happens, and it still works by chance on little-endian
kernels, just not on big-endian.

> Also, I'm not sure that it will be possible to support both LE/BE in such case.
> 
> Actually, I've tried current multi_v7_defconfig and can see:
> # CONFIG_ARCH_PHYS_ADDR_T_64BIT is not set
> # CONFIG_PHYS_ADDR_T_64BIT is not set
> # CONFIG_ARM_LPAE is not set
> What is your "multiplatform configuration" ??

kernelci is testing multi_v7_defconfig with LPAE and KVM enabled on top.
This breaks all pre-Cortex-A15 platforms (A5, A8 and A9 don't have LPAE)
but should work on any A7/A15/A17 machine.
 
> So I propose to fix this regression first (as we both did - revert changes in 
> get/set_pad_info()) and have KS2 working again with current version of
> defconfig files (keystone_defconfig & multi_v7_defconfig) while this
> discussion is continuing. 

Could you please at least test the last patch I sent? It really shouldn't
be too hard to find the line that was wrong in my patch.

	Arnd

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

On 02/04/2016 03:07 PM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 14:19:54 Grygorii Strashko wrote:
>> On 02/03/2016 10:40 PM, Arnd Bergmann wrote:
>>> On Wednesday 03 February 2016 18:31:00 Grygorii Strashko wrote:
>>>> On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
>>>>> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>>>>>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>>>>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>> config:
>>>>
>>>> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
>>>> CONFIG_PHYS_ADDR_T_64BIT=y
>>>>
>>>> and
>>>>
>>>> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT <--- should not be defined for KS2
>>>> typedef u64 dma_addr_t;
>>>> #else
>>>> typedef u32 dma_addr_t;
>>>> #endif
>>>>
>>>> Above is valid configuration for Keystone 2 with LPAE=y
>>>
>>> Ok, but what do you mean with "should not be defined"? It clearly is
>>> defined in any multiplatform configuration that enables another platform
>>> needing 64-bit dma_addr_t.
>>>
>>
>> Then we probably have bigger problem :) KS2 will not work as is with
>> such configuration and not only KS2 - LPAE is enabled for TI DRA7 also.
>>
>> Problem here is that dma_addr_t is used to fill DMA controllers data or can be
>> written directly to register, so all drivers need to be revised which was initially
>> created for 32-bit HW and with assumption that dma_addr_t is 32-bits.
> 
> Almost everything should work fine. What all other drivers tend to do is
> something like
> 
> 	writel(dma_addr, reg_base + REG_OFFSET);
> 
> which works for 64-bit dma_addr_t as long as the upper 32 bits are
> always zero, and that should be the case on keystone.
> 
> The part that does not work and that originally triggered the
> warning I was fixing is
> 
> void f(u32 *data)
> {
> 	writel(*data, reg_base + reg_offset);
> }
> 
> ...
> 
> 	f((u32 *)&dma_addr);
> 
> as this driver does. I have not seen the same warning for any
> other driver in the kernel, so it is clearly something that
> rarely happens, and it still works by chance on little-endian
> kernels, just not on big-endian.
> 
>> Also, I'm not sure that it will be possible to support both LE/BE in such case.
>>
>> Actually, I've tried current multi_v7_defconfig and can see:
>> # CONFIG_ARCH_PHYS_ADDR_T_64BIT is not set
>> # CONFIG_PHYS_ADDR_T_64BIT is not set
>> # CONFIG_ARM_LPAE is not set
>> What is your "multiplatform configuration" ??
> 
> kernelci is testing multi_v7_defconfig with LPAE and KVM enabled on top.
> This breaks all pre-Cortex-A15 platforms (A5, A8 and A9 don't have LPAE)
> but should work on any A7/A15/A17 machine.
>   
>> So I propose to fix this regression first (as we both did - revert changes in
>> get/set_pad_info()) and have KS2 working again with current version of
>> defconfig files (keystone_defconfig & multi_v7_defconfig) while this
>> discussion is continuing.
> 
> Could you please at least test the last patch I sent? It really shouldn't
> be too hard to find the line that was wrong in my patch.
> 

I don't see any build warnings (in netcp) with my patch + below diff and If I manually enable
ARCH_DMA_ADDR_T_64BIT. 

--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1058,6 +1058,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
                u32 page_offset = frag->page_offset;
                u32 buf_len = skb_frag_size(frag);
                dma_addr_t desc_dma;
+               u32 desc_dma_32;
                u32 pkt_info;
 
                dma_addr = dma_map_page(dev, page, page_offset, buf_len,
@@ -1079,7 +1080,8 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
                        (netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
                                KNAV_DMA_DESC_RETQ_SHIFT;
                set_pkt_info(dma_addr, buf_len, 0, ndesc);
-               set_words(&desc_dma, 1, &pdesc->next_desc);
+               desc_dma_32 = (u32)desc_dma;
+               set_words(&desc_dma_32, 1, &pdesc->next_desc);
                pkt_len += buf_len;
                if (pdesc != desc)
                        knav_pool_desc_map(netcp->tx_pool, pdesc,
 


-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Murali Karicheri]

On 02/03/2016 11:20 AM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>>>
>>>> So only making this change on the latest master with no
>>>> other changes I see the boot problem again.
>>
>> yep. I can confirm that.
>>
>> Also, I'm today came up with the similar fix that you've proposed before in this thread.
>> So, Could we move forward this way?
> 
> I still think it would be good to actually understand what the actual
> problem was.
> 
>> From 8280895f01b33edba303e7374431bef47630f26c Mon Sep 17 00:00:00 2001
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>> Date: Wed, 3 Feb 2016 15:11:40 +0200
>> Subject: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality
>>
>> The commit 899077791403 ("netcp: try to reduce type confusion in descriptors")
>> introduces a regression in Kernel 4.5-rc1 as it breaks
>> get/set_pad_info() functionality.
>>
>> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to
>> store DMA/MEM buffer pointer and buffer size respectively. And in both
>> cases the pointer type size is 32 bit regardless of LAPE enabled or
>> not.
>> 			!LAPE	LPAE
>> sizeof(void*)		32bit	32bit
>> sizeof(dma_addr_t) 	32bit	32bit
>> sizeof(phys_addr_t) 	32bit	64bit
>>
> 
> This looks wrong: I was getting the build warnings originally
> because of 64-bit dma_addr_t, and that should be the only way that
> this driver can operate, because in some configurations on keystone
> there is no memory below 4GB, and there is no dma-ranges property
> in the DT that shifts around the start of the DMA addresses.
Arnd,

Why do think so? I see in arch/arm/boot/dts/keystone.dtsi

        soc {
                #address-cells = <1>;
                #size-cells = <1>;
                compatible = "ti,keystone","simple-bus";
                interrupt-parent = <&gic>;
                ranges = <0x0 0x0 0x0 0xc0000000>;
                dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;

AFAIK, On Keystone, dma address is 32 bit and Physical DDR address is
64 bit (actually 36 bit, LPAE address). The conversion happens based on
pfn_offset which is calculated based on the above dma-range property.

> 
> This doesn't all fit together yet, maybe you have a better idea
> of what is going on.
> 
> I don't think we should ever have a platform that has dma_addr_t
> and phys_addr_t be different.
> 

Not sure why? Keystone is that platform where they are different.

>> Unfortunately, above commit changed buffer's pointers save/restore
>> code (get/set_pad_info()) and added intermediate conversation to u64
>> which causes TI NETCP driver crash in RX/TX path due to "Unable to
>> handle kernel NULL pointer" exception.
> 
> The conversion is not what made it crash, it must be a bug in the
> conversion ;-)
> 
>> @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>>  	desc->packet_info = cpu_to_le32(pkt_info);
>>  }
>>  
>> -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>> +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc)
>>  {
>>  	desc->pad[0] = cpu_to_le32(pad0);
>>  	desc->pad[1] = cpu_to_le32(pad1);
>> -	desc->pad[2] = cpu_to_le32(pad1);
>>  }
> 
> As in my earlier patch, the line you are removing here was clearly
> broken, but evidently that is not the only bug.
> 
>>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
>> @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>  	dma_addr_t dma_desc, dma_buf;
>>  	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>>  	void *buf_ptr;
>> -	u32 pad[2];
>>  	u32 tmp;
>>  
>>  	get_words(&dma_desc, 1, &desc->next_desc);
>> @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>>  			break;
>>  		}
>>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
>> -		get_pad_ptr(&buf_ptr, ndesc);
>> +		get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc);
> 
> I'd prefer not to put code like this back, as this cannot possibly
> do the right thing on a 64-bit architecture.
> 
> 
>> @@ -1078,7 +1058,6 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>  		u32 page_offset = frag->page_offset;
>>  		u32 buf_len = skb_frag_size(frag);
>>  		dma_addr_t desc_dma;
>> -		u32 desc_dma_32;
>>  		u32 pkt_info;
>>  
>>  		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
>> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>  			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>>  				KNAV_DMA_DESC_RETQ_SHIFT;
>>  		set_pkt_info(dma_addr, buf_len, 0, ndesc);
>> -		desc_dma_32 = (u32)desc_dma;
>> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
>> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>>  		pkt_len += buf_len;
>>  		if (pdesc != desc)
>>  			knav_pool_desc_map(netcp->tx_pool, pdesc,
> 
> This is clearly broken on big-endian kernels with 64-bit dma_addr_t, so even
> if we revert the rest I think this part has to stay.
> 
> I have another version for testing below. That removes the logic that
> splits and reassembles the 64-bit values, but leaves the other changes
> in place. Can you try this?
> 
> 	Arnd
> 
> 
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..cda19f2401c1 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -120,16 +120,14 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>  static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
>  {
>  	*pad0 = le32_to_cpu(desc->pad[0]);
> -	*pad1 = le32_to_cpu(desc->pad[1]);
> -	*pad2 = le32_to_cpu(desc->pad[2]);
> +	*pad2 = le32_to_cpu(desc->pad[1]);
>  }
>  
>  static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
>  {
>  	u64 pad64;
>  
> -	pad64 = le32_to_cpu(desc->pad[0]) +
> -		((u64)le32_to_cpu(desc->pad[1]) << 32);
> +	pad64 = le32_to_cpu(desc->pad[0]);
>  	*padptr = (void *)(uintptr_t)pad64;
>  }
>  
> @@ -166,8 +164,7 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>  static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>  {
>  	desc->pad[0] = cpu_to_le32(pad0);
> -	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
> +	desc->pad[1] = cpu_to_le32(pad2);
>  }
>  
>  static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +578,7 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  	dma_addr_t dma_desc, dma_buf;
>  	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>  	void *buf_ptr;
> -	u32 pad[2];
> +	u32 pad;
>  	u32 tmp;
>  
>  	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -599,8 +596,8 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>  		knav_pool_desc_put(netcp->rx_pool, desc);
>  	}
>  
> -	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
> -	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad, NULL, &buf_len, desc);
> +	buf_ptr = (void *)(uintptr_t)(pad);
>  
>  	if (buf_ptr)
>  		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
> @@ -653,8 +650,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>  	}
>  
>  	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
> -	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
> -	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad[0], NULL, &org_buf_len, desc);
> +	org_buf_ptr = (void *)(uintptr_t)(pad[0]);
>  
>  	if (unlikely(!org_buf_ptr)) {
>  		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
> @@ -858,8 +855,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		if (unlikely(dma_mapping_error(netcp->dev, dma)))
>  			goto fail;
>  
> -		pad[0] = lower_32_bits((uintptr_t)bufptr);
> -		pad[1] = upper_32_bits((uintptr_t)bufptr);
> +		pad[0] = (uintptr_t)bufptr;
>  
>  	} else {
>  		/* Allocate a secondary receive queue entry */
> @@ -870,8 +866,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  		}
>  		buf_len = PAGE_SIZE;
>  		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
> -		pad[0] = lower_32_bits(dma);
> -		pad[1] = upper_32_bits(dma);
> +		pad[0] = (u32)(dma);
>  		pad[2] = 0;
>  	}
>  
> @@ -882,7 +877,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>  	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>  		    KNAV_DMA_DESC_RETQ_SHIFT;
>  	set_org_pkt_info(dma, buf_len, hwdesc);
> -	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
> +	set_pad_info(pad[0], 0, pad[2], hwdesc);
>  	set_desc_info(desc_info, pkt_info, hwdesc);
>  
>  	/* Push to FDQs */
> @@ -1194,10 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>  	}
>  
>  	set_words(&tmp, 1, &desc->packet_info);
> -	tmp = lower_32_bits((uintptr_t)&skb);
> +	tmp = (uintptr_t)&skb;
>  	set_words(&tmp, 1, &desc->pad[0]);
> -	tmp = upper_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[1]);
>  
>  	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>  		tmp = tx_pipe->switch_to_port;
> 
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Wednesday 03 February 2016 11:41:40 Murali Karicheri wrote:
> > 
> > This looks wrong: I was getting the build warnings originally
> > because of 64-bit dma_addr_t, and that should be the only way that
> > this driver can operate, because in some configurations on keystone
> > there is no memory below 4GB, and there is no dma-ranges property
> > in the DT that shifts around the start of the DMA addresses.
> Arnd,
> 
> Why do think so? I see in arch/arm/boot/dts/keystone.dtsi
> 
>         soc {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 compatible = "ti,keystone","simple-bus";
>                 interrupt-parent = <&gic>;
>                 ranges = <0x0 0x0 0x0 0xc0000000>;
>                 dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
> 
> AFAIK, On Keystone, dma address is 32 bit and Physical DDR address is
> 64 bit (actually 36 bit, LPAE address). The conversion happens based on
> pfn_offset which is calculated based on the above dma-range property.

My mistake, see my other reply.
 


	Arnd

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

On 02/03/2016 06:20 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 16:21:05 Grygorii Strashko wrote:
>> On 02/03/2016 04:11 PM, Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 07:19 PM, Franklin S Cooper Jr. wrote:
>>>>>
>>>> So only making this change on the latest master with no
>>>> other changes I see the boot problem again.
>>
>> yep. I can confirm that.
>>
>> Also, I'm today came up with the similar fix that you've proposed before in this thread.
>> So, Could we move forward this way?
> 
> I still think it would be good to actually understand what the actual
> problem was.
> 

[...]

>>   		dma_addr = dma_map_page(dev, page, page_offset, buf_len,
>> @@ -1100,8 +1079,7 @@ netcp_tx_map_skb(struct sk_buff *skb, struct netcp_intf *netcp)
>>   			(netcp->tx_compl_qid & KNAV_DMA_DESC_RETQ_MASK) <<
>>   				KNAV_DMA_DESC_RETQ_SHIFT;
>>   		set_pkt_info(dma_addr, buf_len, 0, ndesc);
>> -		desc_dma_32 = (u32)desc_dma;
>> -		set_words(&desc_dma_32, 1, &pdesc->next_desc);
>> +		set_words(&desc_dma, 1, &pdesc->next_desc);
>>   		pkt_len += buf_len;
>>   		if (pdesc != desc)
>>   			knav_pool_desc_map(netcp->tx_pool, pdesc,
> 
> This is clearly broken on big-endian kernels with 64-bit dma_addr_t, so even
> if we revert the rest I think this part has to stay.
> 
> I have another version for testing below. That removes the logic that
> splits and reassembles the 64-bit values, but leaves the other changes
> in place. Can you try this?
> 

Nop. It crashes kernel
   50.244448] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[   50.266219] Unable to handle kernel NULL pointer dereference at virtual address 00000001
[   50.274287] pgd = c0003000
[   50.277007] [00000001] *pgd=80000800004003, *pmd=00000000
[   50.282412] Internal error: Oops: a07 [#1] PREEMPT SMP ARM
[   50.287881] Modules linked in:
[   50.290938] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc2-00179-gad2f022-dirty #30
[   50.300214] Hardware name: Keystone
[   50.303693] task: c07476c0 ti: c0742000 task.ti: c0742000
[   50.309082] PC is at _test_and_set_bit+0x4/0x4c
[   50.313607] LR is at __netif_schedule+0x1c/0x60
[   50.318127] pc : [<c028415c>]    lr : [<c04294f0>]    psr: 20000113
[   50.318127] sp : c0743d68  ip : 00000001  fp : c0743d7c
[   50.329568] r10: c0743e00  r9 : c0744100  r8 : ffff9e75
[   50.334775] r7 : 00000000  r6 : 00000040  r5 : de495b00  r4 : 6d3cdb51
[   50.341282] r3 : 00000001  r2 : c07476c0  r1 : 6d3cdba9  r0 : 00000000
[   50.347790] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[   50.355077] Control: 30c5387d  Table: 1878abc0  DAC: fffffffd
[   50.360803] Process swapper/0 (pid: 0, stack limit = 0xc0742210)
[   50.366790] Stack: (0xc0743d68 to 0xc0744000)
[   50.371137] 3d60:                   d9a16a00 de495b00 c0743d94 c0743d80 c04295a0 c04294e0
[   50.379291] 3d80: de7a9cc0 de495b00 c0743dbc c0743d98 c037396c c0429570 c0061d34 00000010
[   50.387445] 3da0: de7a9d80 de7a9d80 00000040 0000012c c0743ddc c0743dc0 c0375f58 c0373848
[   50.395599] 3dc0: de7a9d80 c0375f3c 00000040 0000012c c0743e3c c0743de0 c042a0cc c0375f48
[   50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 c0741000 debac000
[   50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 00000000 c074408c c0742000
[   50.420061] 3e20: 00000101 00000003 40000003 c0744080 c0743e9c c0743e40 c0026670 c0429ed8
[   50.428215] 3e40: d8722360 c0744808 00200000 c0744100 ffff9e74 c054088c 0000000a c07779c0
[   50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc 0000004e 00000000
[   50.444522] 3e80: 00000000 de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 c0026a38 c0026548
[   50.452675] 3ea0: c073dddc 0000004e c0743edc c0743eb8 c0061d34 c00269c4 c0744808 e080400c
[   50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 c0009438 c0061cd8
[   50.468981] 3ee0: c0010314 60000013 ffffffff c0743f3c c0773aa4 c0773aa4 c0743f64 c0743f08
[   50.477136] 3f00: c0013a80 c0009404 00000000 deba8348 000070c8 c001f880 c0742000 c07444b0
[   50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 c0743f68 c0743f58
[   50.493442] 3f40: c0010310 c0010314 60000013 ffffffff c006d624 c006a15c c0743f74 c0743f68
[   50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 00000002 00000000
[   50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 00000000 c0743ff4 c0743fa8
[   50.517902] 3fa0: c06fad60 c05383e4 ffffffff ffffffff 00000000 c06fa6d8 ffffffff 00000000
[   50.526056] 3fc0: 00000000 c0731a30 00000000 c0777294 c0744484 c0731a2c c0748878 80007000
[   50.534210] 3fe0: 412fc0f4 00000000 00000000 c0743ff8 80008090 c06fa964 00000000 00000000
[   50.542357] Backtrace: 
[   50.544816] [<c04294d4>] (__netif_schedule) from [<c04295a0>] (netif_wake_subqueue+0x3c/0x44)
[   50.553312]  r5:de495b00 r4:d9a16a00
[   50.556909] [<c0429564>] (netif_wake_subqueue) from [<c037396c>] (netcp_process_tx_compl_packets+0x130/0x134)
[   50.566789]  r5:de495b00 r4:de7a9cc0
[   50.570381] [<c037383c>] (netcp_process_tx_compl_packets) from [<c0375f58>] (netcp_tx_poll+0x1c/0x4c)
[   50.579570]  r7:0000012c r6:00000040 r5:de7a9d80 r4:de7a9d80
[   50.585258] [<c0375f3c>] (netcp_tx_poll) from [<c042a0cc>] (net_rx_action+0x200/0x2f8)
[   50.593148]  r7:0000012c r6:00000040 r5:c0375f3c r4:de7a9d80
[   50.598833] [<c0429ecc>] (net_rx_action) from [<c0026670>] (__do_softirq+0x134/0x258)
[   50.606637]  r10:c0744080 r9:40000003 r8:00000003 r7:00000101 r6:c0742000 r5:c074408c
[   50.614486]  r4:00000000
[   50.617023] [<c002653c>] (__do_softirq) from [<c0026a38>] (irq_exit+0x80/0xb8)
[   50.624221]  r10:c07444fc r9:c0773aa4 r8:de408000 r7:00000000 r6:00000000 r5:0000004e
[   50.632069]  r4:c073dddc
[   50.634608] [<c00269b8>] (irq_exit) from [<c0061d34>] (__handle_domain_irq+0x68/0xbc)
[   50.642410]  r5:0000004e r4:c073dddc
[   50.645996] [<c0061ccc>] (__handle_domain_irq) from [<c0009438>] (gic_handle_irq+0x40/0x78)
[   50.654318]  r9:c0773aa4 r8:e0805000 r7:e0804000 r6:c0743f08 r5:e080400c r4:c0744808
[   50.662087] [<c00093f8>] (gic_handle_irq) from [<c0013a80>] (__irq_svc+0x40/0x74)
[   50.669547] Exception stack(0xc0743f08 to 0xc0743f50)
[   50.674586] 3f00:                   00000000 deba8348 000070c8 c001f880 c0742000 c07444b0
[   50.682740] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 c0743f68 c0743f58
[   50.690892] 3f40: c0010310 c0010314 60000013 ffffffff
[   50.695925]  r9:c0773aa4 r8:c0773aa4 r7:c0743f3c r6:ffffffff r5:60000013 r4:c0010314
[   50.703701] [<c00102d4>] (arch_cpu_idle) from [<c00598c0>] (default_idle_call+0x28/0x34)
[   50.711773] [<c0059898>] (default_idle_call) from [<c00599dc>] (cpu_startup_entry+0x110/0x180)
[   50.720366] [<c00598cc>] (cpu_startup_entry) from [<c0538468>] (rest_init+0x90/0x94)
[   50.728083]  r7:00000000 r4:00000002
[   50.731675] [<c05383d8>] (rest_init) from [<c06fad60>] (start_kernel+0x408/0x414)
[   50.739131]  r5:00000000 r4:c0777050
[   50.742716] [<c06fa958>] (start_kernel) from [<80008090>] (0x80008090)
[   50.749227] Code: e3500000 13a00001 e12fff1e e211c003 (15cc1000) 
[   50.755356] ---[ end trace ff02c0c474692a7b ]---



> 	Arnd
> 
> 
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index c61d66d38634..cda19f2401c1 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -120,16 +120,14 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
>   static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc)
>   {
>   	*pad0 = le32_to_cpu(desc->pad[0]);
> -	*pad1 = le32_to_cpu(desc->pad[1]);
> -	*pad2 = le32_to_cpu(desc->pad[2]);
> +	*pad2 = le32_to_cpu(desc->pad[1]);
>   }
>   
>   static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
>   {
>   	u64 pad64;
>   
> -	pad64 = le32_to_cpu(desc->pad[0]) +
> -		((u64)le32_to_cpu(desc->pad[1]) << 32);
> +	pad64 = le32_to_cpu(desc->pad[0]);
>   	*padptr = (void *)(uintptr_t)pad64;
>   }
>   
> @@ -166,8 +164,7 @@ static void set_desc_info(u32 desc_info, u32 pkt_info,
>   static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc)
>   {
>   	desc->pad[0] = cpu_to_le32(pad0);
> -	desc->pad[1] = cpu_to_le32(pad1);
> -	desc->pad[2] = cpu_to_le32(pad1);
> +	desc->pad[1] = cpu_to_le32(pad2);
>   }
>   
>   static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
> @@ -581,7 +578,7 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>   	dma_addr_t dma_desc, dma_buf;
>   	unsigned int buf_len, dma_sz = sizeof(*ndesc);
>   	void *buf_ptr;
> -	u32 pad[2];
> +	u32 pad;
>   	u32 tmp;
>   
>   	get_words(&dma_desc, 1, &desc->next_desc);
> @@ -599,8 +596,8 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
>   		knav_pool_desc_put(netcp->rx_pool, desc);
>   	}
>   
> -	get_pad_info(&pad[0], &pad[1], &buf_len, desc);
> -	buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad, NULL, &buf_len, desc);
> +	buf_ptr = (void *)(uintptr_t)(pad);
>   
>   	if (buf_ptr)
>   		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
> @@ -653,8 +650,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
>   	}
>   
>   	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
> -	get_pad_info(&pad[0], &pad[1], &org_buf_len, desc);
> -	org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32));
> +	get_pad_info(&pad[0], NULL, &org_buf_len, desc);
> +	org_buf_ptr = (void *)(uintptr_t)(pad[0]);
>   
>   	if (unlikely(!org_buf_ptr)) {
>   		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
> @@ -858,8 +855,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   		if (unlikely(dma_mapping_error(netcp->dev, dma)))
>   			goto fail;
>   
> -		pad[0] = lower_32_bits((uintptr_t)bufptr);
> -		pad[1] = upper_32_bits((uintptr_t)bufptr);
> +		pad[0] = (uintptr_t)bufptr;
>   
>   	} else {
>   		/* Allocate a secondary receive queue entry */
> @@ -870,8 +866,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   		}
>   		buf_len = PAGE_SIZE;
>   		dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE);
> -		pad[0] = lower_32_bits(dma);
> -		pad[1] = upper_32_bits(dma);
> +		pad[0] = (u32)(dma);
>   		pad[2] = 0;
>   	}
>   
> @@ -882,7 +877,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   	pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) <<
>   		    KNAV_DMA_DESC_RETQ_SHIFT;
>   	set_org_pkt_info(dma, buf_len, hwdesc);
> -	set_pad_info(pad[0], pad[1], pad[2], hwdesc);
> +	set_pad_info(pad[0], 0, pad[2], hwdesc);
>   	set_desc_info(desc_info, pkt_info, hwdesc);
>   
>   	/* Push to FDQs */
> @@ -1194,10 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>   	}
>   
>   	set_words(&tmp, 1, &desc->packet_info);
> -	tmp = lower_32_bits((uintptr_t)&skb);
> +	tmp = (uintptr_t)&skb;
>   	set_words(&tmp, 1, &desc->pad[0]);
> -	tmp = upper_32_bits((uintptr_t)&skb);
> -	set_words(&tmp, 1, &desc->pad[1]);
>   
>   	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>   		tmp = tx_pipe->switch_to_port;
> 


-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Thursday 04 February 2016 18:25:08 Grygorii Strashko wrote:
> > 
> > I have another version for testing below. That removes the logic that
> > splits and reassembles the 64-bit values, but leaves the other changes
> > in place. Can you try this?
> > 
> 
> Nop. It crashes kernel

Ah. too bad.

>    50.244448] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [   50.266219] Unable to handle kernel NULL pointer dereference at virtual address 00000001
> [   50.274287] pgd = c0003000
> [   50.277007] [00000001] *pgd=80000800004003, *pmd=00000000
> [   50.282412] Internal error: Oops: a07 [#1] PREEMPT SMP ARM
> [   50.287881] Modules linked in:
> [   50.290938] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc2-00179-gad2f022-dirty #30
> [   50.300214] Hardware name: Keystone
> [   50.303693] task: c07476c0 ti: c0742000 task.ti: c0742000
> [   50.309082] PC is at _test_and_set_bit+0x4/0x4c
> [   50.313607] LR is at __netif_schedule+0x1c/0x60
> [   50.318127] pc : [<c028415c>]    lr : [<c04294f0>]    psr: 20000113
> [   50.318127] sp : c0743d68  ip : 00000001  fp : c0743d7c
> [   50.329568] r10: c0743e00  r9 : c0744100  r8 : ffff9e75
> [   50.334775] r7 : 00000000  r6 : 00000040  r5 : de495b00  r4 : 6d3cdb51
> [   50.341282] r3 : 00000001  r2 : c07476c0  r1 : 6d3cdba9  r0 : 00000000
> [   50.347790] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> [   50.355077] Control: 30c5387d  Table: 1878abc0  DAC: fffffffd
> [   50.360803] Process swapper/0 (pid: 0, stack limit = 0xc0742210)
> [   50.366790] Stack: (0xc0743d68 to 0xc0744000)
> [   50.371137] 3d60:                   d9a16a00 de495b00 c0743d94 c0743d80 c04295a0 c04294e0
> [   50.379291] 3d80: de7a9cc0 de495b00 c0743dbc c0743d98 c037396c c0429570 c0061d34 00000010
> [   50.387445] 3da0: de7a9d80 de7a9d80 00000040 0000012c c0743ddc c0743dc0 c0375f58 c0373848
> [   50.395599] 3dc0: de7a9d80 c0375f3c 00000040 0000012c c0743e3c c0743de0 c042a0cc c0375f48
> [   50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 c0741000 debac000
> [   50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 00000000 c074408c c0742000
> [   50.420061] 3e20: 00000101 00000003 40000003 c0744080 c0743e9c c0743e40 c0026670 c0429ed8
> [   50.428215] 3e40: d8722360 c0744808 00200000 c0744100 ffff9e74 c054088c 0000000a c07779c0
> [   50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc 0000004e 00000000
> [   50.444522] 3e80: 00000000 de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 c0026a38 c0026548
> [   50.452675] 3ea0: c073dddc 0000004e c0743edc c0743eb8 c0061d34 c00269c4 c0744808 e080400c
> [   50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 c0009438 c0061cd8
> [   50.468981] 3ee0: c0010314 60000013 ffffffff c0743f3c c0773aa4 c0773aa4 c0743f64 c0743f08
> [   50.477136] 3f00: c0013a80 c0009404 00000000 deba8348 000070c8 c001f880 c0742000 c07444b0
> [   50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 c0743f68 c0743f58
> [   50.493442] 3f40: c0010310 c0010314 60000013 ffffffff c006d624 c006a15c c0743f74 c0743f68
> [   50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 00000002 00000000
> [   50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 00000000 c0743ff4 c0743fa8
> [   50.517902] 3fa0: c06fad60 c05383e4 ffffffff ffffffff 00000000 c06fa6d8 ffffffff 00000000
> [   50.526056] 3fc0: 00000000 c0731a30 00000000 c0777294 c0744484 c0731a2c c0748878 80007000
> [   50.534210] 3fe0: 412fc0f4 00000000 00000000 c0743ff8 80008090 c06fa964 00000000 00000000
> [   50.542357] Backtrace: 
> [   50.544816] [<c04294d4>] (__netif_schedule) from [<c04295a0>] (netif_wake_subqueue+0x3c/0x44)
> [   50.553312]  r5:de495b00 r4:d9a16a00
> [   50.556909] [<c0429564>] (netif_wake_subqueue) from [<c037396c>] (netcp_process_tx_compl_packets+0x130/0x134)
> [   50.566789]  r5:de495b00 r4:de7a9cc0
> [   50.570381] [<c037383c>] (netcp_process_tx_compl_packets) from [<c0375f58>] (netcp_tx_poll+0x1c/0x4c)
> [   50.579570]  r7:0000012c r6:00000040 r5:de7a9d80 r4:de7a9d80
> [   50.585258] [<c0375f3c>] (netcp_tx_poll) from [<c042a0cc>] (net_rx_action+0x200/0x2f8)
> [   50.593148]  r7:0000012c r6:00000040 r5:c0375f3c r4:de7a9d80
> [   50.598833] [<c0429ecc>] (net_rx_action) from [<c0026670>] (__do_softirq+0x134/0x258)
> [   50.606637]  r10:c0744080 r9:40000003 r8:00000003 r7:00000101 r6:c0742000 r5:c074408c
> [   50.614486]  r4:00000000
> [   50.617023] [<c002653c>] (__do_softirq) from [<c0026a38>] (irq_exit+0x80/0xb8)
> [   50.624221]  r10:c07444fc r9:c0773aa4 r8:de408000 r7:00000000 r6:00000000 r5:0000004e
> [   50.632069]  r4:c073dddc
> [   50.634608] [<c00269b8>] (irq_exit) from [<c0061d34>] (__handle_domain_irq+0x68/0xbc)
> [   50.642410]  r5:0000004e r4:c073dddc
> [   50.645996] [<c0061ccc>] (__handle_domain_irq) from [<c0009438>] (gic_handle_irq+0x40/0x78)
> 

This is a different bug now, something is corrupting the skb pointer, probably as a
result of the patch below (which is a subset of what is now applied compared
to the last working version):

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 7e291c04a81a..cda19f2401c1 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -117,10 +117,18 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
+
+static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
+{
+	u64 pad64;
+
+	pad64 = le32_to_cpu(desc->pad[0]);
+	*padptr = (void *)(uintptr_t)pad64;
 }
 
 static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,

@@ -953,11 +966,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 					  unsigned int budget)
 {
 	struct knav_dma_desc *desc;
+	void *ptr;
 	struct sk_buff *skb;
 	unsigned int dma_sz;
 	dma_addr_t dma;
 	int pkts = 0;
-	u32 tmp;
 
 	while (budget--) {
 		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
@@ -970,7 +983,8 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 			continue;
 		}
 
-		get_pad_info((u32 *)&skb, &tmp, desc);
+		get_pad_ptr(&ptr, desc);
+		skb = ptr;
 		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
 		if (!skb) {
 			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
@@ -1173,7 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
 	}
 
 	set_words(&tmp, 1, &desc->packet_info);
-	set_words((u32 *)&skb, 1, &desc->pad[0]);
+	tmp = (uintptr_t)&skb;
+	set_words(&tmp, 1, &desc->pad[0]);
 
 	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
 		tmp = tx_pipe->switch_to_port;


I'm sure it's something obvious and stupid in there, but I just can't
see it and that is very unsatisfying. Do you see where I am going wrong?
Most of all, I want to know it so I don't make the same mistake again
when I patch another driver.

	Arnd

Re: Keystone 2 boards boot failure

[Grygorii Strashko]

hi Arnd,

On 02/05/2016 06:18 PM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 18:25:08 Grygorii Strashko wrote:
>>>
>>> I have another version for testing below. That removes the logic that
>>> splits and reassembles the 64-bit values, but leaves the other changes
>>> in place. Can you try this?
>>>
>>
>> Nop. It crashes kernel
> 
> Ah. too bad.
> 
>>     50.244448] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>> [   50.266219] Unable to handle kernel NULL pointer dereference at virtual address 00000001
>> [   50.274287] pgd = c0003000
>> [   50.277007] [00000001] *pgd=80000800004003, *pmd=00000000
>> [   50.282412] Internal error: Oops: a07 [#1] PREEMPT SMP ARM
>> [   50.287881] Modules linked in:
>> [   50.290938] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc2-00179-gad2f022-dirty #30
>> [   50.300214] Hardware name: Keystone
>> [   50.303693] task: c07476c0 ti: c0742000 task.ti: c0742000
>> [   50.309082] PC is at _test_and_set_bit+0x4/0x4c
>> [   50.313607] LR is at __netif_schedule+0x1c/0x60
>> [   50.318127] pc : [<c028415c>]    lr : [<c04294f0>]    psr: 20000113
>> [   50.318127] sp : c0743d68  ip : 00000001  fp : c0743d7c
>> [   50.329568] r10: c0743e00  r9 : c0744100  r8 : ffff9e75
>> [   50.334775] r7 : 00000000  r6 : 00000040  r5 : de495b00  r4 : 6d3cdb51
>> [   50.341282] r3 : 00000001  r2 : c07476c0  r1 : 6d3cdba9  r0 : 00000000
>> [   50.347790] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
>> [   50.355077] Control: 30c5387d  Table: 1878abc0  DAC: fffffffd
>> [   50.360803] Process swapper/0 (pid: 0, stack limit = 0xc0742210)
>> [   50.366790] Stack: (0xc0743d68 to 0xc0744000)
>> [   50.371137] 3d60:                   d9a16a00 de495b00 c0743d94 c0743d80 c04295a0 c04294e0
>> [   50.379291] 3d80: de7a9cc0 de495b00 c0743dbc c0743d98 c037396c c0429570 c0061d34 00000010
>> [   50.387445] 3da0: de7a9d80 de7a9d80 00000040 0000012c c0743ddc c0743dc0 c0375f58 c0373848
>> [   50.395599] 3dc0: de7a9d80 c0375f3c 00000040 0000012c c0743e3c c0743de0 c042a0cc c0375f48
>> [   50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 c0741000 debac000
>> [   50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 00000000 c074408c c0742000
>> [   50.420061] 3e20: 00000101 00000003 40000003 c0744080 c0743e9c c0743e40 c0026670 c0429ed8
>> [   50.428215] 3e40: d8722360 c0744808 00200000 c0744100 ffff9e74 c054088c 0000000a c07779c0
>> [   50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc 0000004e 00000000
>> [   50.444522] 3e80: 00000000 de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 c0026a38 c0026548
>> [   50.452675] 3ea0: c073dddc 0000004e c0743edc c0743eb8 c0061d34 c00269c4 c0744808 e080400c
>> [   50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 c0009438 c0061cd8
>> [   50.468981] 3ee0: c0010314 60000013 ffffffff c0743f3c c0773aa4 c0773aa4 c0743f64 c0743f08
>> [   50.477136] 3f00: c0013a80 c0009404 00000000 deba8348 000070c8 c001f880 c0742000 c07444b0
>> [   50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 c0743f68 c0743f58
>> [   50.493442] 3f40: c0010310 c0010314 60000013 ffffffff c006d624 c006a15c c0743f74 c0743f68
>> [   50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 00000002 00000000
>> [   50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 00000000 c0743ff4 c0743fa8
>> [   50.517902] 3fa0: c06fad60 c05383e4 ffffffff ffffffff 00000000 c06fa6d8 ffffffff 00000000
>> [   50.526056] 3fc0: 00000000 c0731a30 00000000 c0777294 c0744484 c0731a2c c0748878 80007000
>> [   50.534210] 3fe0: 412fc0f4 00000000 00000000 c0743ff8 80008090 c06fa964 00000000 00000000
>> [   50.542357] Backtrace:
>> [   50.544816] [<c04294d4>] (__netif_schedule) from [<c04295a0>] (netif_wake_subqueue+0x3c/0x44)
>> [   50.553312]  r5:de495b00 r4:d9a16a00
>> [   50.556909] [<c0429564>] (netif_wake_subqueue) from [<c037396c>] (netcp_process_tx_compl_packets+0x130/0x134)
>> [   50.566789]  r5:de495b00 r4:de7a9cc0
>> [   50.570381] [<c037383c>] (netcp_process_tx_compl_packets) from [<c0375f58>] (netcp_tx_poll+0x1c/0x4c)
>> [   50.579570]  r7:0000012c r6:00000040 r5:de7a9d80 r4:de7a9d80
>> [   50.585258] [<c0375f3c>] (netcp_tx_poll) from [<c042a0cc>] (net_rx_action+0x200/0x2f8)
>> [   50.593148]  r7:0000012c r6:00000040 r5:c0375f3c r4:de7a9d80
>> [   50.598833] [<c0429ecc>] (net_rx_action) from [<c0026670>] (__do_softirq+0x134/0x258)
>> [   50.606637]  r10:c0744080 r9:40000003 r8:00000003 r7:00000101 r6:c0742000 r5:c074408c
>> [   50.614486]  r4:00000000
>> [   50.617023] [<c002653c>] (__do_softirq) from [<c0026a38>] (irq_exit+0x80/0xb8)
>> [   50.624221]  r10:c07444fc r9:c0773aa4 r8:de408000 r7:00000000 r6:00000000 r5:0000004e
>> [   50.632069]  r4:c073dddc
>> [   50.634608] [<c00269b8>] (irq_exit) from [<c0061d34>] (__handle_domain_irq+0x68/0xbc)
>> [   50.642410]  r5:0000004e r4:c073dddc
>> [   50.645996] [<c0061ccc>] (__handle_domain_irq) from [<c0009438>] (gic_handle_irq+0x40/0x78)
>>
> 
> This is a different bug now, something is corrupting the skb pointer, probably as a
> result of the patch below (which is a subset of what is now applied compared
> to the last working version):
> 
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index 7e291c04a81a..cda19f2401c1 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -117,10 +117,18 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc,
> +
> +static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc)
> +{
> +	u64 pad64;
> +
> +	pad64 = le32_to_cpu(desc->pad[0]);
> +	*padptr = (void *)(uintptr_t)pad64;
>   }
>   
>   static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
> 
> @@ -953,11 +966,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>   					  unsigned int budget)
>   {
>   	struct knav_dma_desc *desc;
> +	void *ptr;
>   	struct sk_buff *skb;
>   	unsigned int dma_sz;
>   	dma_addr_t dma;
>   	int pkts = 0;
> -	u32 tmp;
>   
>   	while (budget--) {
>   		dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz);
> @@ -970,7 +983,8 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
>   			continue;
>   		}
>   
> -		get_pad_info((u32 *)&skb, &tmp, desc);
> +		get_pad_ptr(&ptr, desc);
> +		skb = ptr;
>   		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
>   		if (!skb) {
>   			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
> @@ -1173,7 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
>   	}
>   
>   	set_words(&tmp, 1, &desc->packet_info);
> -	set_words((u32 *)&skb, 1, &desc->pad[0]);
> +	tmp = (uintptr_t)&skb;
> +	set_words(&tmp, 1, &desc->pad[0]);

&skb is virt address and its size is 32bit even when LPAE=y (phys/dma 64 bit)
so  this is excess conversion to/from u64 ;)
This is from the first look.

>   
>   	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
>   		tmp = tx_pipe->switch_to_port;
> 
> 
> I'm sure it's something obvious and stupid in there, but I just can't
> see it and that is very unsatisfying. Do you see where I am going wrong?
> Most of all, I want to know it so I don't make the same mistake again
> when I patch another driver.
> 

I'm very sorry, but I'll not be able to test it in the nearest future :(
What I could do now is update your/my patch as i mentioned in [1]
and re-send it at the weekend (with your authorship and my signoff).
Do you agree?


[1] https://www.mail-archive.com/netdev@vger.kernel.org/msg95831.html

-- 
regards,
-grygorii

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Friday 05 February 2016 19:11:06 Grygorii Strashko wrote:
> On 02/05/2016 06:18 PM, Arnd Bergmann wrote:
> > On Thursday 04 February 2016 18:25:08 Grygorii Strashko wrote:

> > @@ -1173,7 +1189,8 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp,
> >   	}
> >   
> >   	set_words(&tmp, 1, &desc->packet_info);
> > -	set_words((u32 *)&skb, 1, &desc->pad[0]);
> > +	tmp = (uintptr_t)&skb;
> > +	set_words(&tmp, 1, &desc->pad[0]);
> 
> &skb is virt address and its size is 32bit even when LPAE=y (phys/dma 64 bit)
> so  this is excess conversion to/from u64 ;)
> This is from the first look.

My original patch attempted to fix support for 64-bit CPUs, as no driver
should be written to support only 32-bit CPUs even if you think at this
point that there can never be a 64-bit keystone system.

The half-reverted patch above no longer works correctly for 64-bit CPUs
but it should not actually be wrong on 32-bit CPUs either, unless I'm
missing your point. 

> >   
> >   	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
> >   		tmp = tx_pipe->switch_to_port;
> > 
> > 
> > I'm sure it's something obvious and stupid in there, but I just can't
> > see it and that is very unsatisfying. Do you see where I am going wrong?
> > Most of all, I want to know it so I don't make the same mistake again
> > when I patch another driver.
> > 
> 
> I'm very sorry, but I'll not be able to test it in the nearest future :(
> What I could do now is update your/my patch as i mentioned in [1]
> and re-send it at the weekend (with your authorship and my signoff).
> Do you agree?
> 
> 
> [1] https://www.mail-archive.com/netdev@vger.kernel.org/msg95831.html

Yes, let's do that in the meantime. I can also make sure that that
the driver doesn't build on 64-bit, just in case.

	Arnd

Re: Keystone 2 boards boot failure

[Arnd Bergmann]

On Tuesday 02 February 2016 19:19:13 Franklin S Cooper Jr. wrote:
> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
> > On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
> >> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
> >>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
> >> Keystone 2 devices are little-endian 32-bit devices.
> > I meant the kernel you are running on it, not the hardware.
> > You should always be able to run both a big-endian kernel and
> > a littl-endian kernel on any ARMv7 machine, and a couple of
> > platforms use 64-bit physical addresses even on 32-bit machines
> > (with the normal 32-bit instruction set).
> 
> I'm not sure if Keystone 2 devices support this or if we
> have support for this. I'll have to double check.

Don't worry about it, there is nothing you need to check:

As I said, all ARMv7 *hardware* can be run in either mode, and
that includes Keystone 2.

As for the kernel, it's obvious that nobody tried to run
a Keystone based machine with a big-endian kernel, or they
would have run into the broken network driver.

I believe you also require this patch, unless the firmware is
clever enough to figure out endianess by itself (very unlikely)

diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
index 33eaa037af5a..016ae7644e73 100644
--- a/arch/arm/mach-keystone/keystone.h
+++ b/arch/arm/mach-keystone/keystone.h
@@ -16,7 +16,7 @@
 #ifndef __ASSEMBLER__
 
 extern const struct smp_operations keystone_smp_ops;
-extern void secondary_startup(void);
+extern void keystone_secondary_startup(void);
 extern u32 keystone_cpu_smc(u32 command, u32 cpu, u32 addr);
 extern int keystone_pm_runtime_init(void);
 
diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
index 5665276972ec..c427787f78d1 100644
--- a/arch/arm/mach-keystone/platsmp.c
+++ b/arch/arm/mach-keystone/platsmp.c
@@ -26,7 +26,7 @@
 static int keystone_smp_boot_secondary(unsigned int cpu,
 						struct task_struct *idle)
 {
-	unsigned long start = virt_to_idmap(&secondary_startup);
+	unsigned long start = virt_to_idmap(&keystone_secondary_startup);
 	int error;
 
 	pr_debug("keystone-smp: booting cpu %d, vector %08lx\n",
diff --git a/arch/arm/mach-keystone/smc.S b/arch/arm/mach-keystone/smc.S
index d15de8179fab..3ce858ce426e 100644
--- a/arch/arm/mach-keystone/smc.S
+++ b/arch/arm/mach-keystone/smc.S
@@ -23,6 +23,13 @@
  */
 ENTRY(keystone_cpu_smc)
 	stmfd   sp!, {r4-r11, lr}
+ARM_BE8(setend	le)			@ call SMC as LE
 	smc	#0
+ARM_BE8(setend	be)			@ go back to BE8
 	ldmfd   sp!, {r4-r11, pc}
 ENDPROC(keystone_cpu_smc)
+
+ENTRY(keystone_secondary_startup)
+ARM_BE8(setend	be)			@ go BE8 if entered LE
+	b	secondary_startup
+ENDPROC(keyston_secondary_startup)

It would be nice to give this a go once the network driver problem
is solved.

	Arnd

Re: Keystone 2 boards boot failure

[santosh shilimkar]

On 2/3/2016 8:35 AM, Arnd Bergmann wrote:
> On Tuesday 02 February 2016 19:19:13 Franklin S Cooper Jr. wrote:
>> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>> Keystone 2 devices are little-endian 32-bit devices.
>>> I meant the kernel you are running on it, not the hardware.
>>> You should always be able to run both a big-endian kernel and
>>> a littl-endian kernel on any ARMv7 machine, and a couple of
>>> platforms use 64-bit physical addresses even on 32-bit machines
>>> (with the normal 32-bit instruction set).
>>
>> I'm not sure if Keystone 2 devices support this or if we
>> have support for this. I'll have to double check.
>
I missed this thread so far but noticed after RMK's idmap
changes series.

> Don't worry about it, there is nothing you need to check:
>
> As I said, all ARMv7 *hardware* can be run in either mode, and
> that includes Keystone 2.
>
Right. Keystone isn't special from ARMv7 perspective.

> As for the kernel, it's obvious that nobody tried to run
> a Keystone based machine with a big-endian kernel, or they
> would have run into the broken network driver.
>
> I believe you also require this patch, unless the firmware is
> clever enough to figure out endianess by itself (very unlikely)
>
> diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
> index 33eaa037af5a..016ae7644e73 100644
> --- a/arch/arm/mach-keystone/keystone.h
> +++ b/arch/arm/mach-keystone/keystone.h
> @@ -16,7 +16,7 @@
>   #ifndef __ASSEMBLER__
>
>   extern const struct smp_operations keystone_smp_ops;
> -extern void secondary_startup(void);
> +extern void keystone_secondary_startup(void);
>   extern u32 keystone_cpu_smc(u32 command, u32 cpu, u32 addr);
>   extern int keystone_pm_runtime_init(void);
>
> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
> index 5665276972ec..c427787f78d1 100644
> --- a/arch/arm/mach-keystone/platsmp.c
> +++ b/arch/arm/mach-keystone/platsmp.c
> @@ -26,7 +26,7 @@
>   static int keystone_smp_boot_secondary(unsigned int cpu,
>   						struct task_struct *idle)
>   {
> -	unsigned long start = virt_to_idmap(&secondary_startup);
> +	unsigned long start = virt_to_idmap(&keystone_secondary_startup);
>   	int error;
>
>   	pr_debug("keystone-smp: booting cpu %d, vector %08lx\n",
> diff --git a/arch/arm/mach-keystone/smc.S b/arch/arm/mach-keystone/smc.S
> index d15de8179fab..3ce858ce426e 100644
> --- a/arch/arm/mach-keystone/smc.S
> +++ b/arch/arm/mach-keystone/smc.S
> @@ -23,6 +23,13 @@
>    */
>   ENTRY(keystone_cpu_smc)
>   	stmfd   sp!, {r4-r11, lr}
> +ARM_BE8(setend	le)			@ call SMC as LE
>   	smc	#0
> +ARM_BE8(setend	be)			@ go back to BE8
>   	ldmfd   sp!, {r4-r11, pc}
>   ENDPROC(keystone_cpu_smc)
> +
> +ENTRY(keystone_secondary_startup)
> +ARM_BE8(setend	be)			@ go BE8 if entered LE
> +	b	secondary_startup
> +ENDPROC(keyston_secondary_startup)
>
> It would be nice to give this a go once the network driver problem
> is solved.
>
Big endian kernel has worked on Keystone in past.
Yes, above secondary hook needs to be modified along with
drivers endian macro conversion was what was needed IIRC.

Indeed, it will be good to get the BE working but for 4.5-rcx,
we need to fix the boot problem on priority.

Regards,
Santosh

Re: Keystone 2 boards boot failure

[Murali Karicheri]

On 02/03/2016 12:08 PM, santosh shilimkar wrote:
> On 2/3/2016 8:35 AM, Arnd Bergmann wrote:
>> On Tuesday 02 February 2016 19:19:13 Franklin S Cooper Jr. wrote:
>>> On 02/02/2016 05:26 PM, Arnd Bergmann wrote:
>>>> On Tuesday 02 February 2016 16:59:34 Franklin Cooper wrote:
>>>>> On 02/02/2016 03:26 PM, Arnd Bergmann wrote:
>>>>>> On Tuesday 02 February 2016 15:01:33 Franklin S Cooper Jr. wrote:
>>>>> Keystone 2 devices are little-endian 32-bit devices.
>>>> I meant the kernel you are running on it, not the hardware.
>>>> You should always be able to run both a big-endian kernel and
>>>> a littl-endian kernel on any ARMv7 machine, and a couple of
>>>> platforms use 64-bit physical addresses even on 32-bit machines
>>>> (with the normal 32-bit instruction set).
>>>
>>> I'm not sure if Keystone 2 devices support this or if we
>>> have support for this. I'll have to double check.
>>
> I missed this thread so far but noticed after RMK's idmap
> changes series.
> 
>> Don't worry about it, there is nothing you need to check:
>>
>> As I said, all ARMv7 *hardware* can be run in either mode, and
>> that includes Keystone 2.
>>
> Right. Keystone isn't special from ARMv7 perspective.
> 
>> As for the kernel, it's obvious that nobody tried to run
>> a Keystone based machine with a big-endian kernel, or they
>> would have run into the broken network driver.
>>
>> I believe you also require this patch, unless the firmware is
>> clever enough to figure out endianess by itself (very unlikely)
>>
>> diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
>> index 33eaa037af5a..016ae7644e73 100644
>> --- a/arch/arm/mach-keystone/keystone.h
>> +++ b/arch/arm/mach-keystone/keystone.h
>> @@ -16,7 +16,7 @@
>>   #ifndef __ASSEMBLER__
>>
>>   extern const struct smp_operations keystone_smp_ops;
>> -extern void secondary_startup(void);
>> +extern void keystone_secondary_startup(void);
>>   extern u32 keystone_cpu_smc(u32 command, u32 cpu, u32 addr);
>>   extern int keystone_pm_runtime_init(void);
>>
>> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
>> index 5665276972ec..c427787f78d1 100644
>> --- a/arch/arm/mach-keystone/platsmp.c
>> +++ b/arch/arm/mach-keystone/platsmp.c
>> @@ -26,7 +26,7 @@
>>   static int keystone_smp_boot_secondary(unsigned int cpu,
>>                           struct task_struct *idle)
>>   {
>> -    unsigned long start = virt_to_idmap(&secondary_startup);
>> +    unsigned long start = virt_to_idmap(&keystone_secondary_startup);
>>       int error;
>>
>>       pr_debug("keystone-smp: booting cpu %d, vector %08lx\n",
>> diff --git a/arch/arm/mach-keystone/smc.S b/arch/arm/mach-keystone/smc.S
>> index d15de8179fab..3ce858ce426e 100644
>> --- a/arch/arm/mach-keystone/smc.S
>> +++ b/arch/arm/mach-keystone/smc.S
>> @@ -23,6 +23,13 @@
>>    */
>>   ENTRY(keystone_cpu_smc)
>>       stmfd   sp!, {r4-r11, lr}
>> +ARM_BE8(setend    le)            @ call SMC as LE
>>       smc    #0
>> +ARM_BE8(setend    be)            @ go back to BE8
>>       ldmfd   sp!, {r4-r11, pc}
>>   ENDPROC(keystone_cpu_smc)
>> +
>> +ENTRY(keystone_secondary_startup)
>> +ARM_BE8(setend    be)            @ go BE8 if entered LE
>> +    b    secondary_startup
>> +ENDPROC(keyston_secondary_startup)
>>
>> It would be nice to give this a go once the network driver problem
>> is solved.
>>
> Big endian kernel has worked on Keystone in past.

Yes, this was on a v3.10.x baseline, not in the upstream.

> Yes, above secondary hook needs to be modified along with
> drivers endian macro conversion was what was needed IIRC.
> 

To support BE, it may be more than Netcp driver. Do you recall, what
changes you did to get BE working on Keystone? Is it just NetCP driver?

Murali
> Indeed, it will be good to get the BE working but for 4.5-rcx,
> we need to fix the boot problem on priority.
> 
> Regards,
> Santosh
> 
> 


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: Keystone 2 boards boot failure

[santosh shilimkar]

On 2/3/2016 10:47 AM, Murali Karicheri wrote:
> On 02/03/2016 12:08 PM, santosh shilimkar wrote:
>> On 2/3/2016 8:35 AM, Arnd Bergmann wrote:

[..]

>>> It would be nice to give this a go once the network driver problem
>>> is solved.
>>>
>> Big endian kernel has worked on Keystone in past.
>
> Yes, this was on a v3.10.x baseline, not in the upstream.
>
Thats what I mean in past. That time upstream didn't have
ARM BE patches o.w there was no other depedency.

>> Yes, above secondary hook needs to be modified along with
>> drivers endian macro conversion was what was needed IIRC.
>>
>
> To support BE, it may be more than Netcp driver. Do you recall, what
> changes you did to get BE working on Keystone? Is it just NetCP driver?
>
IIRC it was Navigator (QMSS, DMA), NETCP, SPI and couple of
of more drivers. Driver update was a massive patch done by Prabhu.

Regards,
Santosh

Re: Keystone 2 boards boot failure

[Murali Karicheri]

On 02/03/2016 03:13 PM, santosh shilimkar wrote:
> On 2/3/2016 10:47 AM, Murali Karicheri wrote:
>> On 02/03/2016 12:08 PM, santosh shilimkar wrote:
>>> On 2/3/2016 8:35 AM, Arnd Bergmann wrote:
> 
> [..]
> 
>>>> It would be nice to give this a go once the network driver problem
>>>> is solved.
>>>>
>>> Big endian kernel has worked on Keystone in past.
>>
>> Yes, this was on a v3.10.x baseline, not in the upstream.
>>
> Thats what I mean in past. That time upstream didn't have
> ARM BE patches o.w there was no other depedency.
> 
>>> Yes, above secondary hook needs to be modified along with
>>> drivers endian macro conversion was what was needed IIRC.
>>>
>>
>> To support BE, it may be more than Netcp driver. Do you recall, what
>> changes you did to get BE working on Keystone? Is it just NetCP driver?
>>
> IIRC it was Navigator (QMSS, DMA), NETCP, SPI and couple of
> of more drivers. Driver update was a massive patch done by Prabhu.
> 
Ok. Thanks.

Murali
> Regards,
> Santosh
> 
> 
> 


-- 
Murali Karicheri
Linux Kernel, Keystone