Re: [PATCH net-next 01/10] net: libwx: Add irq flow functions

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * 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