On Tue, May 27, 2025 at 08:59:03AM -0700, Shradha Gupta wrote:
> Currently, the MANA driver allocates MSI-X vectors statically based on
> MANA_MAX_NUM_QUEUES and num_online_cpus() values and in some cases ends
> up allocating more vectors than it needs. This is because, by this time
> we do not have a HW channel and do not know how many IRQs should be
> allocated.
>
> To avoid this, we allocate 1 MSI-X vector during the creation of HWC and
> after getting the value supported by hardware, dynamically add the
> remaining MSI-X vectors.
>
> Signed-off-by: Shradha Gupta <shradhagu...@linux.microsoft.com>
> Reviewed-by: Haiyang Zhang <haiya...@microsoft.com>
...
> +static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec)
> +{
> + struct gdma_context *gc = pci_get_drvdata(pdev);
> + struct gdma_irq_context *gic;
> + int *irqs, *start_irqs, irq;
> + unsigned int cpu;
> + int err, i;
> +
> + cpus_read_lock();
> +
> + irqs = kmalloc_array(nvec, sizeof(int), GFP_KERNEL);
> + if (!irqs) {
> err = -ENOMEM;
> - goto free_irq_array;
> + goto free_irq_vector;
> }
>
> for (i = 0; i < nvec; i++) {
> - gic = &gc->irq_contexts[i];
> + gic = kzalloc(sizeof(*gic), GFP_KERNEL);
> + if (!gic) {
> + err = -ENOMEM;
> + goto free_irq;
> + }
> +
> gic->handler = mana_gd_process_eq_events;
> INIT_LIST_HEAD(&gic->eq_list);
> spin_lock_init(&gic->lock);
> @@ -1418,69 +1498,128 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
> snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> i - 1, pci_name(pdev));
>
> - irq = pci_irq_vector(pdev, i);
> - if (irq < 0) {
> - err = irq;
> - goto free_irq;
> + irqs[i] = pci_irq_vector(pdev, i);
> + if (irqs[i] < 0) {
> + err = irqs[i];
> + goto free_current_gic;
> }
>
> - if (!i) {
> - err = request_irq(irq, mana_gd_intr, 0, gic->name, gic);
> - if (err)
> - goto free_irq;
> -
> - /* If number of IRQ is one extra than number of online
> CPUs,
> - * then we need to assign IRQ0 (hwc irq) and IRQ1 to
> - * same CPU.
> - * Else we will use different CPUs for IRQ0 and IRQ1.
> - * Also we are using cpumask_local_spread instead of
> - * cpumask_first for the node, because the node can be
> - * mem only.
> - */
> - if (start_irq_index) {
> - cpu = cpumask_local_spread(i, gc->numa_node);
> - irq_set_affinity_and_hint(irq, cpumask_of(cpu));
> - } else {
> - irqs[start_irq_index] = irq;
> - }
> - } else {
> - irqs[i - start_irq_index] = irq;
> - err = request_irq(irqs[i - start_irq_index],
> mana_gd_intr, 0,
> - gic->name, gic);
> - if (err)
> - goto free_irq;
> - }
> + err = request_irq(irqs[i], mana_gd_intr, 0, gic->name, gic);
> + if (err)
> + goto free_current_gic;
Jumping to free_current_gic will free start_irqs.
However, start_irqs isn't initialised until a few lines below.
Flagged by Smatch.
> +
> + xa_store(&gc->irq_contexts, i, gic, GFP_KERNEL);
> }
>
> - err = irq_setup(irqs, nvec - start_irq_index, gc->numa_node, false);
> + /* If number of IRQ is one extra than number of online CPUs,
> + * then we need to assign IRQ0 (hwc irq) and IRQ1 to
> + * same CPU.
> + * Else we will use different CPUs for IRQ0 and IRQ1.
> + * Also we are using cpumask_local_spread instead of
> + * cpumask_first for the node, because the node can be
> + * mem only.
> + */
> + start_irqs = irqs;
> + if (nvec > num_online_cpus()) {
> + cpu = cpumask_local_spread(0, gc->numa_node);
> + irq_set_affinity_and_hint(irqs[0], cpumask_of(cpu));
> + irqs++;
> + nvec -= 1;
> + }
> +
> + err = irq_setup(irqs, nvec, gc->numa_node, false);
> if (err)
> goto free_irq;
>
> - gc->max_num_msix = nvec;
> - gc->num_msix_usable = nvec;
> cpus_read_unlock();
> - kfree(irqs);
> + kfree(start_irqs);
> return 0;
>
> +free_current_gic:
> + kfree(gic);
> free_irq:
> - for (j = i - 1; j >= 0; j--) {
> - irq = pci_irq_vector(pdev, j);
> - gic = &gc->irq_contexts[j];
> + for (i -= 1; i >= 0; i--) {
> + irq = pci_irq_vector(pdev, i);
> + gic = xa_load(&gc->irq_contexts, i);
> + if (WARN_ON(!gic))
> + continue;
>
> irq_update_affinity_hint(irq, NULL);
> free_irq(irq, gic);
> + xa_erase(&gc->irq_contexts, i);
> + kfree(gic);
> }
>
> - kfree(gc->irq_contexts);
> - gc->irq_contexts = NULL;
> -free_irq_array:
> - kfree(irqs);
> + kfree(start_irqs);
> free_irq_vector:
> cpus_read_unlock();
> - pci_free_irq_vectors(pdev);
> return err;
> }
...
