| Message ID | 20250112140911.1702-2-ryazanov.s.a@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Hauke Mehrtens |
| Headers | show |
| Series | ipq40xx: fritzbox 7530: fix ADSL/ATM support | expand |
On 1/12/25 15:09, Sergey Ryazanov wrote: > It looks like VRX518 returns phys addr of data buffer in the 'data_ptr' > field of the RX descriptor and an actual data offset within the buffer > in the 'byte_off' field. In order to map the phys address back to > virtual we need the original phys address of the allocated buffer. > > In the same driver applies offset to phys address before the mapping, > what leads to WARN_ON triggering in plat_mem_virt() function with > subsequent kernel panic: > > WARNING: CPU: 0 PID: 0 at .../sw_plat.c:764 0xbf306cd0 [vrx518_tc@8af9f5d0+0x25000] > ... > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = aff5701e > [00000000] *pgd=00000000 > Internal error: Oops: 5 [#1] SMP ARM > > Noticed in ATM mode, when chip always returns byte_off = 4. > > In order to fix the issue, pass the phys address to plat_mem_virt() as > is and apply byte_off later on mapped virtual address when copying RXed > data into the skb. > > Run tested with FRITZ!Box 7530 on both ADSL and VDSL (thanks Jan) links. > > Fixes: 474bbe23b7 ("kernel: add Intel/Lantiq VRX518 TC driver") > Tested-by: Jan Hoffmann <jan@3e8.eu> # VDSL link > Reported-and-tested-by: nebibigon93@yandex.ru # ADSL link > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > --- > package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch > index edc97998b7..0d97ec4d5f 100644 > --- a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch > +++ b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch > @@ -855,7 +855,7 @@ This replaces it by a basic working implementation. > - continue; > + > + // this seems to be a pointer to a DS PKT buffer > -+ phyaddr = desc->data_ptr + desc->byte_off; > ++ phyaddr = desc->data_ptr; > + ptr = plat_mem_virt(phyaddr); > + > + len = desc->data_len; > @@ -871,7 +871,7 @@ This replaces it by a basic working implementation. > - ring_idx_inc(rxout, idx); > + > + dst = skb_put(skb, len); > -+ memcpy(dst, ptr, len); > ++ memcpy(dst, ptr + desc->byte_off, len); > + > + priv->tc_ops.recv(g_plat_priv->netdev, skb); > + Is the len over the full buffer including the byte_off? If it does not contain the byte_off you should add it to the len in the dma_sync_single_range_for_cpu() call. Hauke
Hi Hauke, sorry for the delayed response, see the answer below. On 18.01.2025 22:05, Hauke Mehrtens wrote: > On 1/12/25 15:09, Sergey Ryazanov wrote: >> It looks like VRX518 returns phys addr of data buffer in the 'data_ptr' >> field of the RX descriptor and an actual data offset within the buffer >> in the 'byte_off' field. In order to map the phys address back to >> virtual we need the original phys address of the allocated buffer. >> >> In the same driver applies offset to phys address before the mapping, >> what leads to WARN_ON triggering in plat_mem_virt() function with >> subsequent kernel panic: >> >> WARNING: CPU: 0 PID: 0 at .../sw_plat.c:764 0xbf306cd0 >> [vrx518_tc@8af9f5d0+0x25000] >> ... >> Unable to handle kernel NULL pointer dereference at virtual address >> 00000000 >> pgd = aff5701e >> [00000000] *pgd=00000000 >> Internal error: Oops: 5 [#1] SMP ARM >> >> Noticed in ATM mode, when chip always returns byte_off = 4. >> >> In order to fix the issue, pass the phys address to plat_mem_virt() as >> is and apply byte_off later on mapped virtual address when copying RXed >> data into the skb. >> >> Run tested with FRITZ!Box 7530 on both ADSL and VDSL (thanks Jan) links. >> >> Fixes: 474bbe23b7 ("kernel: add Intel/Lantiq VRX518 TC driver") >> Tested-by: Jan Hoffmann <jan@3e8.eu> # VDSL link >> Reported-and-tested-by: nebibigon93@yandex.ru # ADSL link >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> --- >> package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch >> b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch >> index edc97998b7..0d97ec4d5f 100644 >> --- a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch >> +++ b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch >> @@ -855,7 +855,7 @@ This replaces it by a basic working implementation. >> - continue; >> + >> + // this seems to be a pointer to a DS PKT buffer >> -+ phyaddr = desc->data_ptr + desc->byte_off; >> ++ phyaddr = desc->data_ptr; >> + ptr = plat_mem_virt(phyaddr); >> + >> + len = desc->data_len; >> @@ -871,7 +871,7 @@ This replaces it by a basic working implementation. >> - ring_idx_inc(rxout, idx); >> + >> + dst = skb_put(skb, len); >> -+ memcpy(dst, ptr, len); >> ++ memcpy(dst, ptr + desc->byte_off, len); >> + >> + priv->tc_ops.recv(g_plat_priv->netdev, skb); >> + > > Is the len over the full buffer including the byte_off? Nope. According to my observations, the descriptor length contains only the packet length. > If it does not contain the byte_off you should add it to the len in the > dma_sync_single_range_for_cpu() call. Nice catch! To sync the whole area we need the sum of the data offset and the data length. Will update and send the V4. -- Sergey
diff --git a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch index edc97998b7..0d97ec4d5f 100644 --- a/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch +++ b/package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch @@ -855,7 +855,7 @@ This replaces it by a basic working implementation. - continue; + + // this seems to be a pointer to a DS PKT buffer -+ phyaddr = desc->data_ptr + desc->byte_off; ++ phyaddr = desc->data_ptr; + ptr = plat_mem_virt(phyaddr); + + len = desc->data_len; @@ -871,7 +871,7 @@ This replaces it by a basic working implementation. - ring_idx_inc(rxout, idx); + + dst = skb_put(skb, len); -+ memcpy(dst, ptr, len); ++ memcpy(dst, ptr + desc->byte_off, len); + + priv->tc_ops.recv(g_plat_priv->netdev, skb); +
It looks like VRX518 returns phys addr of data buffer in the 'data_ptr' field of the RX descriptor and an actual data offset within the buffer in the 'byte_off' field. In order to map the phys address back to virtual we need the original phys address of the allocated buffer. In the same driver applies offset to phys address before the mapping, what leads to WARN_ON triggering in plat_mem_virt() function with subsequent kernel panic: WARNING: CPU: 0 PID: 0 at .../sw_plat.c:764 0xbf306cd0 [vrx518_tc@8af9f5d0+0x25000] ... Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = aff5701e [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Noticed in ATM mode, when chip always returns byte_off = 4. In order to fix the issue, pass the phys address to plat_mem_virt() as is and apply byte_off later on mapped virtual address when copying RXed data into the skb. Run tested with FRITZ!Box 7530 on both ADSL and VDSL (thanks Jan) links. Fixes: 474bbe23b7 ("kernel: add Intel/Lantiq VRX518 TC driver") Tested-by: Jan Hoffmann <jan@3e8.eu> # VDSL link Reported-and-tested-by: nebibigon93@yandex.ru # ADSL link Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> --- package/kernel/lantiq/vrx518_tc/patches/200-swplat.patch | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)