[PATCH v3 1/4] kernel: vrx518_tc: fix RX desc phys to virt mapping

[Sergey Ryazanov]

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 at 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 at 3e8.eu> # VDSL link
>> Reported-and-tested-by: nebibigon93 at yandex.ru # ADSL link
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a at 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