On 12/1/16, 2:25 AM, "Christoph Hellwig" <h...@infradead.org> wrote:



>> -    pci_disable_msix(ha->pdev);
>> +    pci_free_irq_vectors(ha->pdev);
>
>Please make the switch to pci_alloc_irq_vectors / pci_free_irq_vectors
>a se[arate patch.

Ack. We’ll split up patch. 

>
>> +    ret = pci_alloc_irq_vectors(ha->pdev,
>> +            MIN_MSIX_COUNT, ha->msix_count, PCI_IRQ_MSIX);
>
>And for proper blk-mq support you must use PCI_IRQ_AFFINITY to get
>the affinity right.

Ack. 

>
>>              /* Recalculate queue values */
>> -            if (ql2xmqsupport) {
>> +            if (ha->mqiobase && ql2xmqsupport) {
>
>Where is that change coming from?

This change was introduced in previous patch. 
>
>>                      cpus = num_online_cpus();
>>                      ha->max_req_queues = (ha->msix_count - 1 > cpus) ?
>>                              (cpus + 1) : (ha->msix_count - 1);
>> -                    ha->max_rsp_queues = ha->max_req_queues;
>>  
>>                      /* ATIOQ needs 1 vector. That's 1 less QPair */
>>                      if (QLA_TGT_MODE_ENABLED())
>>                              ha->max_req_queues--;
>
>And don't do your own look at online cpus and queue spreading, that's
>what PCI_IRQ_AFFINITY and blk_mq_pci_map_queues are for.

We’ll update patch with the recommendation. 

>
>>                  "MSI: Enabled.\n");
>> @@ -3273,11 +3261,10 @@ struct qla_init_msix_entry {
>>  
>>      if (ha->flags.msix_enabled)
>>              qla24xx_disable_msix(ha);
>> -    else if (ha->flags.msi_enabled) {
>> -            free_irq(ha->pdev->irq, rsp);
>> -            pci_disable_msi(ha->pdev);
>> -    } else
>> -            free_irq(ha->pdev->irq, rsp);
>> +    else {
>> +            free_irq(pci_irq_vector(ha->pdev, 0), rsp);
>> +            pci_free_irq_vectors(ha->pdev);
>> +    }
>
>Please also kill off qla24xx_disable_msix, and have a single
>callsite that iterates over all vectors and finally calls
>pci_free_irq_vectors.

Ack. We’ll update in revised patch.

>
>> -    if (ql2xmqsupport) {
>> +    if (ql2xmqsupport && ha->max_qpairs) {
>
>Where does this come from?

This was introduced in earlier patch 

>

Reply via email to