| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87a500zt46.fsf@redhat.com>
Date: Tue, 02 Dec 2025 18:33:13 +0100
From: Paolo Valerio <pvalerio@...hat.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
netdev@...r.kernel.org
Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>, Claudiu Beznea
<claudiu.beznea@...on.dev>, Andrew Lunn <andrew+netdev@...n.ch>, "David
S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Lorenzo
Bianconi <lorenzo@...nel.org>, Thomas Petazzoni
<thomas.petazzoni@...tlin.com>, Grégory Clement
<gregory.clement@...tlin.com>, Benoît Monin
<benoit.monin@...tlin.com>
Subject: Re: [PATCH RFC net-next 5/6] cadence: macb/gem: make tx path skb
agnostic
On 27 Nov 2025 at 03:49:13 PM, Théo Lebrun <theo.lebrun@...tlin.com> wrote:
> On Wed Nov 19, 2025 at 2:53 PM CET, Paolo Valerio wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 2f665260a84d..67bb98d3cb00 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -964,19 +964,27 @@ struct macb_dma_desc_ptp {
>> #define MACB_PP_HEADROOM XDP_PACKET_HEADROOM
>> #define MACB_MAX_PAD (MACB_PP_HEADROOM + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>>
>> -/* struct macb_tx_skb - data about an skb which is being transmitted
>> - * @skb: skb currently being transmitted, only set for the last buffer
>> - * of the frame
>> +enum macb_tx_buff_type {
>> + MACB_TYPE_SKB,
>> + MACB_TYPE_XDP_TX,
>> + MACB_TYPE_XDP_NDO,
>> +};
>> +
>> +/* struct macb_tx_buff - data about an skb or xdp frame which is being transmitted
>> + * @data: pointer to skb or xdp frame being transmitted, only set
>> + * for the last buffer for sk_buff
>> * @mapping: DMA address of the skb's fragment buffer
>> * @size: size of the DMA mapped buffer
>> * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
>> * false when buffer was mapped with dma_map_single()
>> + * @type: type of buffer (MACB_TYPE_SKB, MACB_TYPE_XDP_TX, MACB_TYPE_XDP_NDO)
>> */
>> -struct macb_tx_skb {
>> - struct sk_buff *skb;
>> - dma_addr_t mapping;
>> - size_t size;
>> - bool mapped_as_page;
>> +struct macb_tx_buff {
>> + void *data;
>> + dma_addr_t mapping;
>> + size_t size;
>> + bool mapped_as_page;
>> + enum macb_tx_buff_type type;
>> };
>
> Here as well, reviewing would be helped by moving the tx_skb to tx_buff
> renaming to a separate commit that has no functional change.
>
> As said in [0], I am not a fan of the field name `data`.
> Let's discuss it there.
>
> [0]: https://lore.kernel.org/all/DEITSIO441QL.X81MVLL3EIV4@bootlin.com/
>
sure
>> -static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budget)
>> +static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
>> + int budget)
>> {
>> - if (tx_skb->mapping) {
>> - if (tx_skb->mapped_as_page)
>> - dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
>> - tx_skb->size, DMA_TO_DEVICE);
>> + if (tx_buff->mapping) {
>> + if (tx_buff->mapped_as_page)
>> + dma_unmap_page(&bp->pdev->dev, tx_buff->mapping,
>> + tx_buff->size, DMA_TO_DEVICE);
>> else
>> - dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
>> - tx_skb->size, DMA_TO_DEVICE);
>> - tx_skb->mapping = 0;
>> + dma_unmap_single(&bp->pdev->dev, tx_buff->mapping,
>> + tx_buff->size, DMA_TO_DEVICE);
>> + tx_buff->mapping = 0;
>> }
>>
>> - if (tx_skb->skb) {
>> - napi_consume_skb(tx_skb->skb, budget);
>> - tx_skb->skb = NULL;
>> + if (tx_buff->data) {
>> + if (tx_buff->type != MACB_TYPE_SKB)
>> + netdev_err(bp->dev, "BUG: Unexpected tx buffer type while unmapping (%d)",
>> + tx_buff->type);
>> + napi_consume_skb(tx_buff->data, budget);
>> + tx_buff->data = NULL;
>> }
>
> This code does not make much sense by itself. We check `tx_buff->type !=
> MACB_TYPE_SKB` but call napi_consume_skb() in all cases. I remember it
> changes in the next commit.
>
>> @@ -1069,16 +1073,23 @@ static void macb_tx_error_task(struct work_struct *work)
>>
>> desc = macb_tx_desc(queue, tail);
>> ctrl = desc->ctrl;
>> - tx_skb = macb_tx_skb(queue, tail);
>> - skb = tx_skb->skb;
>> + tx_buff = macb_tx_buff(queue, tail);
>> +
>> + if (tx_buff->type != MACB_TYPE_SKB)
>> + netdev_err(bp->dev, "BUG: Unexpected tx buffer type (%d)",
>> + tx_buff->type);
>> + skb = tx_buff->data;
>
> Same here: `tx_buff->type != MACB_TYPE_SKB` does not make sense if we
> keep on going with the SKB case anyways.
>
>>
>> if (ctrl & MACB_BIT(TX_USED)) {
>> /* skb is set for the last buffer of the frame */
>> while (!skb) {
>> - macb_tx_unmap(bp, tx_skb, 0);
>> + macb_tx_unmap(bp, tx_buff, 0);
>> tail++;
>> - tx_skb = macb_tx_skb(queue, tail);
>> - skb = tx_skb->skb;
>> + tx_buff = macb_tx_buff(queue, tail);
>> + if (tx_buff->type != MACB_TYPE_SKB)
>> + netdev_err(bp->dev, "BUG: Unexpected tx buffer type (%d)",
>> + tx_buff->type);
>> + skb = tx_buff->data;
>
> Same.
>
yeah, I'll revisit this and the ones above
>> @@ -5050,7 +5066,7 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>> netif_stop_queue(dev);
>>
>> /* Store packet information (to free when Tx completed) */
>> - lp->rm9200_txq[desc].skb = skb;
>> + lp->rm9200_txq[desc].data = skb;
>> lp->rm9200_txq[desc].size = skb->len;
>> lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,
>> skb->len, DMA_TO_DEVICE);
>
> We might want to assign `lp->rm9200_txq[desc].type` here, to ensure all
> `struct macb_tx_buff` instances are fully initialised.
>
good catch
> Thanks,
>
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Powered by blists - more mailing lists