Re: [net-next,03/10] idpf: move queue resources to idpf_q_vec_rsrc structure

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
idpf: move queue resources to idpf_q_vec_rsrc structure

This patch moves TX and RX queue resources from struct idpf_vport to
the newly introduced idpf_q_vec_rsrc structure as part of a refactoring
effort.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 48557c67dda8..25c15d2990d6 100644

[ ... ]

> @@ -438,9 +440,10 @@ static void idpf_rx_buf_rel_all(struct idpf_rx_queue *rxq)
>   *
>   * Free a specific rx queue resources
>   */
> -static void idpf_rx_desc_rel(struct idpf_rx_queue *rxq, struct device *dev,
> -			     u32 model)
> +static void idpf_rx_desc_rel(struct idpf_rx_queue *rxq, u32 model)
>  {
> +	struct device *dev;
> +
>  	if (!rxq)
>  		return;
>

[ ... ]

>  	if (!rxq->desc_ring)
>  		return;
>
> +	dev = &rxq->q_vector->vport->adapter->pdev->dev;
                 ^^^^^^^^^

Is this safe when q_vector has not been initialized yet? Looking at
idpf_vport_open(), the call sequence is:

  idpf_vport_queues_alloc()  -> allocates desc_ring
  idpf_vport_queue_ids_init() -> can fail, goes to queues_rel
  idpf_vport_intr_init()      -> sets q_vector via
                                 idpf_vport_intr_map_vector_to_qs()

If idpf_vport_queue_ids_init() fails, the error path jumps to queues_rel
which calls idpf_vport_queues_rel() -> idpf_rx_desc_rel_all() ->
idpf_rx_desc_rel(). At this point desc_ring is allocated but q_vector
is still NULL, causing a NULL pointer dereference here.

The old code passed dev as a parameter derived from vport->adapter->pdev->dev
which was always valid.

> 	dmam_free_coherent(dev, rxq->size, rxq->desc_ring, rxq->dma);
>  	rxq->desc_ring = NULL;
>  }

> @@ -467,9 +471,10 @@ static void idpf_rx_desc_rel(struct idpf_rx_queue *rxq, struct device *dev,
>   * @bufq: buffer queue to clean the resources from
>   * @dev: device to free DMA memory
>   */
> -static void idpf_rx_desc_rel_bufq(struct idpf_buf_queue *bufq,
> -				  struct device *dev)
> +static void idpf_rx_desc_rel_bufq(struct idpf_buf_queue *bufq)
>  {
> +	struct device *dev;
> +
>  	if (!bufq)
>  		return;
>

[ ... ]

>  	if (!bufq->split_buf)
>  		return;
>
> +	dev = &bufq->q_vector->vport->adapter->pdev->dev;
                 ^^^^^^^^^

The same concern applies here. bufq->q_vector may be NULL if we reach
this cleanup path before idpf_vport_intr_init() has been called.

> 	dma_free_coherent(dev, bufq->size, bufq->split_buf, bufq->dma);
>  	bufq->split_buf = NULL;
>  }