Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash at qla_mapq_init_qp_cpu_map() #156

Closed
hcbwiz opened this issue Apr 25, 2023 · 3 comments
Closed

Crash at qla_mapq_init_qp_cpu_map() #156

hcbwiz opened this issue Apr 25, 2023 · 3 comments

Comments

@hcbwiz
Copy link

hcbwiz commented Apr 25, 2023

Hi,
This commit:

commit a1ff8c3ae93d793e5f2b79477b33e1bea9b2660a
Author: Gleb Chesnokov <gleb.chesnokov@scst.dev>
Date:   Sun Mar 12 16:25:55 2023 +0300

qla2x00t-32gbit: Select qpair depending on which CPU post_cmd() gets called

In current I/O path, Tx and Rx may not be processed on same CPU. This may
lead to thrashing and optimum performance may not be achieved.

Pick qpair such that Tx and Rx are processed on same CPU.

pci_irq_get_affinity() gets NULL value in qla_mapq_init_qp_cpu_map() if it turns on target mode:

In qla24xx_enable_msix():

	if (USER_CTRL_IRQ(ha) || !ha->mqiobase) {
		/* user wants to control IRQ setting for target mode */
		ret = pci_alloc_irq_vectors(ha->pdev, min_vecs,
		    min((u16)ha->msix_count, (u16)(num_online_cpus() + min_vecs)),
		    PCI_IRQ_MSIX);
	} else
		ret = pci_alloc_irq_vectors_affinity(ha->pdev, min_vecs,
		    min((u16)ha->msix_count, (u16)(num_online_cpus() + min_vecs)),
		    PCI_IRQ_MSIX | PCI_IRQ_AFFINITY,
		    &desc);

This patch try to handle it:

diff --git a/qla2x00t-32gbit/qla_inline.h b/qla2x00t-32gbit/qla_inline.h
index c3f1b3b7..6b2716e5 100644
--- a/qla2x00t-32gbit/qla_inline.h
+++ b/qla2x00t-32gbit/qla_inline.h
@@ -550,11 +550,19 @@ qla_mapq_init_qp_cpu_map(struct qla_hw_data *ha,
        if (!ha->qp_cpu_map)
                return;
        mask = pci_irq_get_affinity(ha->pdev, msix->vector_base0);
+       if (!mask) {
+               goto fallback;
+       }
        qpair->cpuid = cpumask_first(mask);
        for_each_cpu(cpu, mask) {
                ha->qp_cpu_map[cpu] = qpair;
        }
        msix->cpuid = qpair->cpuid;
+
+fallback:
+       for_each_possible_cpu(cpu) {
+               ha->qp_cpu_map[cpu] = NULL;
+       }
 #endif
 }
@lnocturno
Copy link
Contributor

Hi,

Thank you for the report!

I'll try to find time to take a look at the problem next week.

Thanks,
Gleb

lnocturno added a commit that referenced this issue May 11, 2023
When target mode is enabled, the pci_irq_get_affinity() function may return
a NULL value in qla_mapq_init_qp_cpu_map() due to the qla24xx_enable_msix()
code that handles IRQ settings for target mode. This leads to a crash due to
a NULL pointer dereference.

This patch fixes the issue by adding a check for the NULL value returned
by pci_irq_get_affinity() and introducing a 'cpu_mapped' boolean flag to the
qla_qpair structure, ensuring that the qpair's CPU affinity is updated when
it has not been mapped to a CPU.

Fixes: a1ff8c3 ("qla2x00t-32gbit: Select qpair depending on which CPU
post_cmd() gets called")
Fixes: #156
@lnocturno
Copy link
Contributor

Hi,

I have created a patch that is slightly different from yours..
Can you retest the problem with it? You can just use this branch:

https://github.com/SCST-project/scst/tree/qla2x00t-32gbit

Thanks,
Gleb

@hcbwiz
Copy link
Author

hcbwiz commented May 11, 2023

Hi,

I have no equipment to retest it.
However, the patch is okay to me

lnocturno added a commit that referenced this issue Jun 5, 2023
When target mode is enabled, the pci_irq_get_affinity() function may return
a NULL value in qla_mapq_init_qp_cpu_map() due to the qla24xx_enable_msix()
code that handles IRQ settings for target mode. This leads to a crash due
to a NULL pointer dereference.

This patch fixes the issue by adding a check for the NULL value returned by
pci_irq_get_affinity() and introducing a 'cpu_mapped' boolean flag to the
qla_qpair structure, ensuring that the qpair's CPU affinity is updated when
it has not been mapped to a CPU.

Fixes: 1d201c81d4cc ("scsi: qla2xxx: Select qpair depending on which CPU post_cmd() gets called")
Signed-off-by: Gleb Chesnokov <gleb.chesnokov@scst.dev>
Link: https://lore.kernel.org/r/56b416f2-4e0f-b6cf-d6d5-b7c372e3c6a2@scst.dev
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit d54820b22e40 upstream ]

Fixes: #156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants