RE: [xdp-hints] Re: [Intel-wired-lan] [PATCH iwl-next, v3 2/2] igc: Add Tx hardware timestamp request for AF_XDP zero-copy packet

[Song, Yoong Siang]

On Thursday, March 7, 2024 9:39 PM, Kurt Kanzenbach <kurt@xxxxxxxxxxxxx> wrote:
>Hi Maciej,
>
>On Wed Mar 06 2024, Maciej Fijalkowski wrote:
>> On Sun, Mar 03, 2024 at 04:32:25PM +0800, Song Yoong Siang wrote:
>>> -	tstamp->skb = NULL;
>>> +	/* Copy the tx hardware timestamp into xdp metadata or skb */
>>> +	if (tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK)
>>
>> I believe this should also be protected with xp_tx_metadata_enabled()
>> check. We recently had following bugfix, PTAL:
>>
>> https://lore.kernel.org/bpf/20240222-stmmac_xdp-v2-1-
>4beee3a037e4@xxxxxxxxxxxxx/
>>
>> I'll take a deeper look at patch tomorrow, might be the case that you've
>> addressed that or you were aware of this issue but anyways wanted to bring
>> it up. Just check that you don't break standard XDP/AF_XDP traffic :)
>
>This one doesn't crash standard AF_XDP traffic. Don't know why, but it
>seems to work.
>
>Thanks,
>Kurt

Thanks Maciej and Kurt for the comments.
In stmmac, xsk_tx_metadata_complete() is called by generic tx complete irq,
thus causing the NULL pointer issue.
In igc, xsk_tx_metadata_complete() is called by ptp irq,
and we use  tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK
as a flag to check whether XDP Tx metadata is used. Thus other data path won't
call into xsk_tx_metadata_complete().
Even it work, but I think I should still add xp_tx_metadata_enabled() checking
for safety measure. Any thoughts?

In case Maciej have more comments, I will wait few days before I add the checking in v4.

Btw, thank for fixing the issue on stmmac. 

Thanks & Regards
Siang