Re: [PATCH net-next v5 11/15] net: macb: change function signatures to take contexts

Replies to Sashiko reviews (only those that aren't about pre-existing
issues).

On this patch, only from sashiko.dev:
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com

quoted

@@ -186,40 +188,42 @@ static struct macb_tx_skb *macb_tx_skb(struct macb_queue *queue,
 {
  struct macb_txq *txq = macb_txq(queue);

- return &txq->skb[macb_tx_ring_wrap(queue->bp, index)];
+ return &txq->skb[macb_tx_ring_wrap(queue->bp->ctx, index)];

[Severity: Medium]
Will this continue to dynamically evaluate queue->bp->ctx instead of
accepting the context parameter? It looks like this leaves a mixed reference
which might defeat the refactoring goal of avoiding bugs from using bp->ctx
instead of ctx.

The main goal of refactoring to take ctx (+ queue index) is that, on
transition states, we cannot mess up the buffers we access.

The macb_tx_skb() function is by definition a function that is only
called in places where the interface must be up and running. Changing
its signature would require passing a queue index and that means a tiny
bit more code in the 11 calling locations without any safety benefit.

quoted

@@ -1620,6 +1631,8 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
       int budget)
 {
+ unsigned int q = queue - queue->bp->queues;
+ struct macb_context *ctx = queue->bp->ctx;
  struct macb_rxq *rxq = macb_rxq(queue);
  struct macb *bp = queue->bp;

[Severity: Medium]
Can this cause a mismatch in size when processing descriptors later in
the function? Since dma_unmap_single() uses bp->ctx->rx_buffer_size
instead of ctx->rx_buffer_size:

gem_rx() {
    ...
    dma_unmap_single(&bp->pdev->dev, addr,
                     bp->ctx->rx_buffer_size, DMA_FROM_DEVICE);

Could this lead to wrong DMA unmap sizes if a concurrent swap modifies
bp->ctx?

I like this review: our function signature bp to ctx rework is aimed at
avoiding those exact bugs. But here, no possible issue. We don't allow
concurrent swaps.

gem_rx() can only be called when a single context exists. Only functions
which might be called at a timeframe where two contexts might be alive
need to be careful about the context they use.

quoted

@@ -2358,14 +2381,14 @@ static unsigned int macb_tx_map(struct macb *bp,
  do {
     i--;
     tx_skb = macb_tx_skb(queue, i);
-    desc = macb_tx_desc(queue, i);
+    desc = macb_tx_desc(ctx, q, i);

     ctrl = (u32)tx_skb->size;
     if (eof) {
        ctrl |= MACB_BIT(TX_LAST);
        eof = 0;
     }
-    if (unlikely(macb_tx_ring_wrap(bp, i) ==
+    if (unlikely(macb_tx_ring_wrap(ctx, i) ==
           bp->ctx->tx_ring_size - 1))
        ctrl |= MACB_BIT(TX_WRAP);

[Severity: Medium]
Does mixing the explicit ctx and bp->ctx in the same bounds calculation
create a time-of-check to time-of-use race? If the context sizes ever differ
due to a concurrent context swap, could this lead to buffer overflows by
failing to set TX_WRAP?

Again, same reasoning. macb_tx_map() cannot be called in a timeframe
where two contexts are alive.

I'll clarify the commit message to avoid the confusion.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com