Re: Keystone 2 boards boot failure
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
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
Re: Keystone 2 boards boot failure
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.28] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready >> [ 50.266219] Unable to handle kernel NULL pointer dereference at virtual >> address 0001 >> [ 50.274287] pgd = c0003000 >> [ 50.277007] [0001] *pgd=884003, *pmd= >> [ 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: GW >> 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 : []lr : []psr: 2113 >> [ 50.318127] sp : c0743d68 ip : 0001 fp : c0743d7c >> [ 50.329568] r10: c0743e00 r9 : c0744100 r8 : 9e75 >> [ 50.334775] r7 : r6 : 0040 r5 : de495b00 r4 : 6d3cdb51 >> [ 50.341282] r3 : 0001 r2 : c07476c0 r1 : 6d3cdba9 r0 : >> [ 50.347790] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment >> kernel >> [ 50.355077] Control: 30c5387d Table: 1878abc0 DAC: fffd >> [ 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 0010 >> [ 50.387445] 3da0: de7a9d80 de7a9d80 0040 012c c0743ddc c0743dc0 >> c0375f58 c0373848 >> [ 50.395599] 3dc0: de7a9d80 c0375f3c 0040 012c c0743e3c c0743de0 >> c042a0cc c0375f48 >> [ 50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 >> c0741000 debac000 >> [ 50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 >> c074408c c0742000 >> [ 50.420061] 3e20: 0101 0003 4003 c0744080 c0743e9c c0743e40 >> c0026670 c0429ed8 >> [ 50.428215] 3e40: d8722360 c0744808 0020 c0744100 9e74 c054088c >> 000a c07779c0 >> [ 50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc >> 004e >> [ 50.444522] 3e80: de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 >> c0026a38 c0026548 >> [ 50.452675] 3ea0: c073dddc 004e c0743edc c0743eb8 c0061d34 c00269c4 >> c0744808 e080400c >> [ 50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 >> c0009438 c0061cd8 >> [ 50.468981] 3ee0: c0010314 6013 c0743f3c c0773aa4 c0773aa4 >> c0743f64 c0743f08 >> [ 50.477136] 3f00: c0013a80 c0009404 deba8348 70c8 c001f880 >> c0742000 c07444b0 >> [ 50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 >> c0743f68 c0743f58 >> [ 50.493442] 3f40: c0010310 c0010314 6013 c006d624 c006a15c >> c0743f74 c0743f68 >> [ 50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 >> 0002 >> [ 50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 >> c0743ff4 c0743fa8 >> [ 50.517902] 3fa0: c06fad60 c05383e4 c06fa6d8 >> >> [ 50.526056] 3fc0: c0731a30 c0777294 c0744484 c0731a2c >> c0748878 80007000 >> [ 50.534210] 3fe0: 412fc0f4 c0743ff8 80008090 c06fa964 >> >> [ 50.542357] Backtrace: >> [ 50.544816] [] (__netif_schedule) from [] >> (netif_wake_subqueue+0x3c/0x44) >> [ 50.553312] r5:de495b00 r4:d9a16a00 >> [ 50.556909] [] (netif_wake_subqueue) from [] >> (netcp_process_tx_compl_packets+0x130/0x134) >> [ 50.566789] r5:de495b00 r4:de7a9cc0 >> [ 50.570381] [] (netcp_process_tx_compl_packets) from >> [] (netcp_tx_poll+0x1c/0x4c) >> [ 50.579570] r7:012c r6:0040 r5:de7a9d80 r4:de7a9d80 >> [ 50.585258] [] (netcp_tx_poll) from [] >> (net_rx_action+0x200/0x2f8) >> [ 50.593148] r7:012c r6:0040 r5:c0375f3c r4:de7a9d80 >> [ 50.598833] [] (net_rx_action) from [] >> (__do_softirq+0x134/0x258) >> [ 50.606637] r10:c0744080 r9:4003 r8:0003 r7:0101 r6:c0742000 >> r5:c074408c >> [ 50.614486] r4: >> [ 50.617023] [] (__do_softirq) from [] >> (irq_exit+0x80/0xb8) >> [ 50.624221] r10:c07444fc r9:c0773aa4 r8:de408000 r7: r6: >> r5:004e >> [ 50.632069] r4:c073dddc >> [ 50.634608] [] (irq_exit) from [] >> (__handle_domain_irq+0x68/0xbc) >> [ 50.642410] r5:004e r4:c073dddc >> [ 50.645996] [] (__handle_domain_irq) from [] >> (gic_handle_irq+0x40/0x78) >> > > This is a differen
Re: Keystone 2 boards boot failure
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.28] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready > [ 50.266219] Unable to handle kernel NULL pointer dereference at virtual > address 0001 > [ 50.274287] pgd = c0003000 > [ 50.277007] [0001] *pgd=884003, *pmd= > [ 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: GW > 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 : []lr : []psr: 2113 > [ 50.318127] sp : c0743d68 ip : 0001 fp : c0743d7c > [ 50.329568] r10: c0743e00 r9 : c0744100 r8 : 9e75 > [ 50.334775] r7 : r6 : 0040 r5 : de495b00 r4 : 6d3cdb51 > [ 50.341282] r3 : 0001 r2 : c07476c0 r1 : 6d3cdba9 r0 : > [ 50.347790] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment > kernel > [ 50.355077] Control: 30c5387d Table: 1878abc0 DAC: fffd > [ 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 0010 > [ 50.387445] 3da0: de7a9d80 de7a9d80 0040 012c c0743ddc c0743dc0 > c0375f58 c0373848 > [ 50.395599] 3dc0: de7a9d80 c0375f3c 0040 012c c0743e3c c0743de0 > c042a0cc c0375f48 > [ 50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 > c0741000 debac000 > [ 50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 > c074408c c0742000 > [ 50.420061] 3e20: 0101 0003 4003 c0744080 c0743e9c c0743e40 > c0026670 c0429ed8 > [ 50.428215] 3e40: d8722360 c0744808 0020 c0744100 9e74 c054088c > 000a c07779c0 > [ 50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc > 004e > [ 50.444522] 3e80: de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 > c0026a38 c0026548 > [ 50.452675] 3ea0: c073dddc 004e c0743edc c0743eb8 c0061d34 c00269c4 > c0744808 e080400c > [ 50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 > c0009438 c0061cd8 > [ 50.468981] 3ee0: c0010314 6013 c0743f3c c0773aa4 c0773aa4 > c0743f64 c0743f08 > [ 50.477136] 3f00: c0013a80 c0009404 deba8348 70c8 c001f880 > c0742000 c07444b0 > [ 50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 > c0743f68 c0743f58 > [ 50.493442] 3f40: c0010310 c0010314 6013 c006d624 c006a15c > c0743f74 c0743f68 > [ 50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 > 0002 > [ 50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 > c0743ff4 c0743fa8 > [ 50.517902] 3fa0: c06fad60 c05383e4 c06fa6d8 > > [ 50.526056] 3fc0: c0731a30 c0777294 c0744484 c0731a2c > c0748878 80007000 > [ 50.534210] 3fe0: 412fc0f4 c0743ff8 80008090 c06fa964 > > [ 50.542357] Backtrace: > [ 50.544816] [] (__netif_schedule) from [] > (netif_wake_subqueue+0x3c/0x44) > [ 50.553312] r5:de495b00 r4:d9a16a00 > [ 50.556909] [] (netif_wake_subqueue) from [] > (netcp_process_tx_compl_packets+0x130/0x134) > [ 50.566789] r5:de495b00 r4:de7a9cc0 > [ 50.570381] [] (netcp_process_tx_compl_packets) from > [] (netcp_tx_poll+0x1c/0x4c) > [ 50.579570] r7:012c r6:0040 r5:de7a9d80 r4:de7a9d80 > [ 50.585258] [] (netcp_tx_poll) from [] > (net_rx_action+0x200/0x2f8) > [ 50.593148] r7:012c r6:0040 r5:c0375f3c r4:de7a9d80 > [ 50.598833] [] (net_rx_action) from [] > (__do_softirq+0x134/0x258) > [ 50.606637] r10:c0744080 r9:4003 r8:0003 r7:0101 r6:c0742000 > r5:c074408c > [ 50.614486] r4: > [ 50.617023] [] (__do_softirq) from [] > (irq_exit+0x80/0xb8) > [ 50.624221] r10:c07444fc r9:c0773aa4 r8:de408000 r7: r6: > r5:004e > [ 50.632069] r4:c073dddc > [ 50.634608] [] (irq_exit) from [] > (__handle_domain_irq+0x68/0xbc) > [ 50.642410] r5:004e r4:c073dddc > [ 50.645996] [] (__handle_domain_irq) from [] > (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 wor
Re: Keystone 2 boards boot failure
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
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.28] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready [ 50.266219] Unable to handle kernel NULL pointer dereference at virtual address 0001 [ 50.274287] pgd = c0003000 [ 50.277007] [0001] *pgd=884003, *pmd= [ 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: GW 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 : []lr : []psr: 2113 [ 50.318127] sp : c0743d68 ip : 0001 fp : c0743d7c [ 50.329568] r10: c0743e00 r9 : c0744100 r8 : 9e75 [ 50.334775] r7 : r6 : 0040 r5 : de495b00 r4 : 6d3cdb51 [ 50.341282] r3 : 0001 r2 : c07476c0 r1 : 6d3cdba9 r0 : [ 50.347790] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel [ 50.355077] Control: 30c5387d Table: 1878abc0 DAC: fffd [ 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 0010 [ 50.387445] 3da0: de7a9d80 de7a9d80 0040 012c c0743ddc c0743dc0 c0375f58 c0373848 [ 50.395599] 3dc0: de7a9d80 c0375f3c 0040 012c c0743e3c c0743de0 c042a0cc c0375f48 [ 50.403752] 3de0: c0743e9c c06635f8 c0744b1c c0744b1c c0773c46 1e46b000 c0741000 debac000 [ 50.411907] 3e00: c0743e00 c0743e00 c0743e08 c0743e08 de408000 c074408c c0742000 [ 50.420061] 3e20: 0101 0003 4003 c0744080 c0743e9c c0743e40 c0026670 c0429ed8 [ 50.428215] 3e40: d8722360 c0744808 0020 c0744100 9e74 c054088c 000a c07779c0 [ 50.436369] 3e60: c073d2c8 c0744080 c0743e40 c0742000 c0744808 c073dddc 004e [ 50.444522] 3e80: de408000 c0773aa4 c07444fc c0743eb4 c0743ea0 c0026a38 c0026548 [ 50.452675] 3ea0: c073dddc 004e c0743edc c0743eb8 c0061d34 c00269c4 c0744808 e080400c [ 50.460828] 3ec0: c0743f08 e0804000 e0805000 c0773aa4 c0743f04 c0743ee0 c0009438 c0061cd8 [ 50.468981] 3ee0: c0010314 6013 c0743f3c c0773aa4 c0773aa4 c0743f64 c0743f08 [ 50.477136] 3f00: c0013a80 c0009404 deba8348 70c8 c001f880 c0742000 c07444b0 [ 50.485289] 3f20: c073d324 c0743f78 c0773aa4 c0773aa4 c07444fc c0743f64 c0743f68 c0743f58 [ 50.493442] 3f40: c0010310 c0010314 6013 c006d624 c006a15c c0743f74 c0743f68 [ 50.501595] 3f60: c00598c0 c00102e0 c0743f8c c0743f78 c00599dc c00598a4 0002 [ 50.509749] 3f80: c0743fa4 c0743f90 c0538468 c00598d8 c0777050 c0743ff4 c0743fa8 [ 50.517902] 3fa0: c06fad60 c05383e4 c06fa6d8 [ 50.526056] 3fc0: c0731a30 c0777294 c0744484 c0731a2c c0748878 80007000 [ 50.534210] 3fe0: 412fc0f4 c0743ff8 80008090 c06fa964 [ 50.542357] Backtrace: [ 50.544816] [] (__netif_schedule) from [] (netif_wake_subqueue+0x3c/0x44) [ 50.553312] r5:de495b00 r4:d9a16a00 [ 50.556909] [] (neti
Re: Keystone 2 boards boot failure
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
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 0xc000>;
>> dma-ranges = <0x8000 0x8 0x 0x8000>;
>> ^^^
>
> 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
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 0xc000>;
> dma-ranges = <0x8000 0x8 0x 0x8000>;
>
> 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
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 0xc000>;
> dma-ranges = <0x8000 0x8 0x 0x8000>;
> ^^^
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
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
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(setendle)@ call SMC as LE
>> smc#0
>> +ARM_BE8(setendbe)@ go back to BE8
>> ldmfd sp!, {r4-r11, pc}
>> ENDPROC(keystone_cpu_smc)
>> +
>> +ENTRY(keystone_secondary_startup)
>> +ARM_BE8(setendbe)@ go BE8 if entered LE
>> +bsecondary_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
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
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
>>> 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.
>>> !LAPELPAE
>>> sizeof(void*)32bit32bit
>>> sizeof(dma_addr_t) 32bit32bit
>>> sizeof(phys_addr_t) 32bit64bit
>>>
>>
>> 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 0xc000>;
> dma-ranges = <0x8000 0x8 0x 0x8000>;
> ^^^
>
> 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_
Re: Keystone 2 boards boot failure
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
>> 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 0xc000>;
dma-ranges = <0x8000 0x8 0x 0x8000>;
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)de
Re: Keystone 2 boards boot failure
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
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
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 0xc000>;
dma-ranges = <0x8000 0x8 0x 0x8000>;
^^^
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;
Re: Keystone 2 boards boot failure
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
> 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 +
Re: Keystone 2 boards boot failure
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
> 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
> ---
> 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_i
Re: Keystone 2 boards boot failure
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
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
---
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
*
Re: Keystone 2 boards boot failure
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
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
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
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;
> - vo
Re: Keystone 2 boards boot failure
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_le
Re: Keystone 2 boards boot failure
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
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
Keystone 2 boards boot failure
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)
