[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y86kDphvyHj21IxK@lunn.ch>
Date: Mon, 23 Jan 2023 16:13:18 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next 01/10] net: libwx: Add irq flow functions
> +/**
> + * wx_acquire_msix_vectors - acquire MSI-X vectors
> + * @wx: board private structure
> + *
> + * Attempts to acquire a suitable range of MSI-X vector interrupts. Will
> + * return a negative error code if unable to acquire MSI-X vectors for any
> + * reason.
> + */
> +static int wx_acquire_msix_vectors(struct wx *wx)
> +{
> + struct irq_affinity affd = {0, };
> + int nvecs, i;
> +
> + nvecs = min_t(int, num_online_cpus(), wx->mac.max_msix_vectors);
> +
> + wx->msix_entries = kcalloc(nvecs,
> + sizeof(struct msix_entry),
> + GFP_KERNEL);
> + if (!wx->msix_entries)
> + return -ENOMEM;
> + * wx_set_interrupt_capability - set MSI-X or MSI if supported
> + * @wx: board private structure to initialize
> + *
> + * Attempt to configure the interrupts using the best available
> + * capabilities of the hardware and the kernel.
> + **/
> +static void wx_set_interrupt_capability(struct wx *wx)
> +{
> + int nvecs;
> +
> + /* We will try to get MSI-X interrupts first */
> + if (!wx_acquire_msix_vectors(wx))
> + return;
This could return -ENOMEM. You should return that up the call stack.
I would suggest you make this check more specific. Success, or real
errors, return here, with 0 or -errcode. If it indicates MSI-X are not
available then do the fallback.
> +
> + wx->num_rx_queues = 1;
> + wx->num_tx_queues = 1;
> + wx->num_q_vectors = 1;
> +
> + /* minmum one for queue, one for misc*/
> + nvecs = 1;
> + nvecs = pci_alloc_irq_vectors(wx->pdev, nvecs,
> + nvecs, PCI_IRQ_MSI);
> + if (nvecs < 0)
> + wx_err(wx, "Failed to allocate MSI interrupts. Error: %d\n", nvecs);
If you don't have MSI-X or MSI interrupt, is the device usable? I
would expect some fatal error handling here.
> +/**
> + * wx_cache_ring_rss - Descriptor ring to register mapping for RSS
> + * @wx: board private structure to initialize
> + *
> + * Cache the descriptor ring offsets for RSS, ATR, FCoE, and SR-IOV.
> + *
> + **/
> +static bool wx_cache_ring_rss(struct wx *wx)
> +{
> + u16 i;
> +
> + for (i = 0; i < wx->num_rx_queues; i++)
> + wx->rx_ring[i]->reg_idx = i;
> +
> + for (i = 0; i < wx->num_tx_queues; i++)
> + wx->tx_ring[i]->reg_idx = i;
> +
> + return true;
What is the point of returning true. This cannot fail, so make it
void.
Andrew
Powered by blists - more mailing lists