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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for your review comments, Tomas

Rajinikanth Pandurangan is no longer with PMC-Sierra now. I have taken care your review comments in V6 patch set. 

Thanks,
Mahesh
 

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@xxxxxxxxxx] 
Sent: Thursday, July 23, 2015 8:04 PM
To: Rajinikanth Pandurangan; jbottomley@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx
Cc: aacraid@xxxxxxxxxxxxxx; 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@xxxxxxxxxx]
> Sent: Thursday, July 23, 2015 5:54 AM
> To: Rajinikanth Pandurangan; jbottomley@xxxxxxxxxxxxx; 
> linux-scsi@xxxxxxxxxxxxxxx
> Cc: aacraid@xxxxxxxxxxxxxx; 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@xxxxxxxx wrote:
>> From: Rajinikanth Pandurangan <rajinikanth.pandurangan@xxxxxxxx>
>>
>> Description:
>>         As pci_enable_msix() deprecated, replaced with
>> pci_enable_msix_range()
>>
>> V4 Reviewed/commented by:
>> 	Tomas Henzl <thenzl@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
> 
> Tomas
> 
>>
>> Signed-off-by: Rajinikanth Pandurangan 
>> <rajinikanth.pandurangan@xxxxxxxx>
>> ---
>>  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",
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux