RE: [PATCH V5 10/11] [SCSI] aacraid: Replace pci_enable_msix() with pci_enable_msix_range()

Rajinikanth Pandurangan
2015-07-23T18:39:54Z

Hi Tomas,

Thanks for the details. Will merge the patches and resubmit.

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com]
Sent: Thursday, July 23, 2015 7:34 AM
To: Rajinikanth Pandurangan; jbottomley@parallels.com; linux-scsi@vger.kernel.org
Cc: aacraid@pmc-sierra.com; Harry Yang; Rich Bono; Mahesh Rajashekhara; Achim Leubner; Murthy Bhat
Subject: Re: [PATCH V5 10/11] [SCSI] aacraid: Replace pci_enable_msix() with pci_enable_msix_range()

On 23.7.2015 15:42, Rajinikanth Pandurangan wrote:
> Hello Tomas,
>
> Our real intention was to replace pci_enable_msix() with pci_enable_msix_range().
> In V4, I had min as 8 and max as msi_count (which could be max of 32). So range was 2 - 32. But then as you suggested to make separate patch if I wanted to set the min range different, I have created patch 11 in V5.
> In patch 10 of V5, set range as (1 - 32). - I thought this would reflect our original intention.
> After discussed internally, we wanted to have at least minimum of 2 MSIx, so it reflects in patch 11 of V5.
In V4 you had a minimum value of 8 and because AAC_MAX_MSIX also is eight your were limiting it to exact 8, that seemed to me as a clear mistake so I pointed you to that. Your answer was that this functional change is intended (8 as minimum value instead of 2), so I asked you for a new patch for the minimum value change.

In your V5 you again use the original range and thus the split to two patches makes no sense.
As I already wrote, I accepted it as it is (two patches instead of one is not a big problem) If you for some reason will post a new series, please merge 10 and 11.

>
> Please let me know if you still think I missed something.

For for future series
- if you wish the upper limit to be 32
change the definition of AAC_MAX_MSIX in aacraid.h
- change the description of the aac_msi option from the current description it seems that msi-x is unsupported
- and and important point is - you may add the 'reviewed-by' tag only if the person actually reviewed your patch and not just commented or asked you to make some change

Cheers,

tomas

>
> Thanks,
>
> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Thursday, July 23, 2015 5:54 AM
> To: Rajinikanth Pandurangan; jbottomley@parallels.com;
> linux-scsi@vger.kernel.org
> Cc: aacraid@pmc-sierra.com; Harry Yang; Rich Bono; Mahesh
> Rajashekhara; Achim Leubner; Murthy Bhat
> Subject: Re: [PATCH V5 10/11] [SCSI] aacraid: Replace
> pci_enable_msix() with pci_enable_msix_range()
>
> On 22.7.2015 18:49, rajinikanth.pandurangan@pmcs.com wrote:
>> From: Rajinikanth Pandurangan <rajinikanth.pandurangan@pmcs.com>
>>
>> Description:
>> As pci_enable_msix() deprecated, replaced with
>> pci_enable_msix_range()
>>
>> V4 Reviewed/commented by:
>> Tomas Henzl <thenzl@redhat.com>
>>
>> Changes from V4:
>> Changed to 1 as minimum msix range in pci_enable_msix_range() to
>> match with original code.
> I don't this is correct, your original code hasn't allowed a single msi-x line too, it allowed it in an exact same range <2,8> like it is now with 10+11/11 patch applied.
> It looks like you have decided to not change the minimal supported
> value in the end, that means that not two but a single patch is
> preferred. If you for any reason repost your series, please merge
> 10+11 into a single patch.
> I can accept it in the current form too so -
>
> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
>
> Tomas
>
>>
>> Signed-off-by: Rajinikanth Pandurangan
>> <rajinikanth.pandurangan@pmcs.com>
>> ---
>> drivers/scsi/aacraid/comminit.c | 20 ++++++--------------
>> 1 file changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/aacraid/comminit.c
>> b/drivers/scsi/aacraid/comminit.c index b4b6088..a02cfb3 100644
>> --- a/drivers/scsi/aacraid/comminit.c
>> +++ b/drivers/scsi/aacraid/comminit.c
>> @@ -338,7 +338,7 @@ static int aac_comm_init(struct aac_dev * dev)
>>
>> void aac_define_int_mode(struct aac_dev *dev) {
>> - int i, msi_count;
>> + int i, msi_count, min_msix;
>>
>> msi_count = i = 0;
>> /* max. vectors from GET_COMM_PREFERRED_SETTINGS */ @@ -366,22
>> +366,14 @@ void aac_define_int_mode(struct aac_dev *dev)
>>
>> if (msi_count > 1 &&
>> pci_find_capability(dev->pdev, PCI_CAP_ID_MSIX)) {
>> - i = pci_enable_msix(dev->pdev,
>> + min_msix = 1;
>> + i = pci_enable_msix_range(dev->pdev,
>> dev->msixentry,
>> + min_msix,
>> msi_count);
>> - /* Check how many MSIX vectors are allocated */
>> - if (i >= 0) {
>> + if (i > 0) {
>> dev->msi_enabled = 1;
>> - if (i) {
>> - msi_count = i;
>> - if (pci_enable_msix(dev->pdev,
>> - dev->msixentry,
>> - msi_count)) {
>> - dev->msi_enabled = 0;
>> - printk(KERN_ERR "%s%d: MSIX not supported!! Will try MSI 0x%x.\n",
>> - dev->name, dev->id, i);
>> - }
>> - }
>> + msi_count = i;
>> } else {
>> dev->msi_enabled = 0;
>> printk(KERN_ERR "%s%d: MSIX not supported!! Will try MSI
>> 0x%x.\n",
>>
>