diff mbox series

[1/3] kernel: vrx518_tc: fix RX desc phys to virt mapping

Message ID 20250111225027.16396-2-ryazanov.s.a@gmail.com
State Superseded
Delegated to: Hauke Mehrtens
Headers show
Series ipq40xx: fritzbox 7530: fix ADSL/ATM support | expand

Commit Message

Sergey Ryazanov Jan. 11, 2025, 10:50 p.m. UTC
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.

Fixes: 474bbe23b7 ("kernel: add Intel/Lantiq VRX518 TC driver")
Reported-and-tested-by: nebibigon93@yandex.ru
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(-)

Comments

Jan Hoffmann Jan. 12, 2025, 12:20 a.m. UTC | #1
On 11.01.25 at 23:50, 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.

VDSL still works with this patch.

Tested-by: Jan Hoffmann <jan@3e8.eu>


Maybe the PKG_RELEASE should be increased? (Although it looks like it 
also wasn't done for the last few changes to the package.)

> 
> Run tested with FRITZ!Box 7530.
> 
> Fixes: 474bbe23b7 ("kernel: add Intel/Lantiq VRX518 TC driver")
> Reported-and-tested-by: nebibigon93@yandex.ru
> 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);
>   +
Sergey Ryazanov Jan. 12, 2025, 12:40 a.m. UTC | #2
Hi Jan,

On 12.01.2025 02:20, Jan Hoffmann wrote:
> On 11.01.25 at 23:50, 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.
> 
> VDSL still works with this patch.
> 
> Tested-by: Jan Hoffmann <jan@3e8.eu>

Thanks a lot! This patch with VDSL link worried me most of all.

> Maybe the PKG_RELEASE should be increased? (Although it looks like it 
> also wasn't done for the last few changes to the package.)

Nice catch! Going to send V2 with one more dedicated patch updating 
PKG_RELEASE.

--
Sergey
diff mbox series

Patch

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);
 +