diff mbox

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

Message ID 1437583757-5449-11-git-send-email-rajinikanth.pandurangan@pmcs.com (mailing list archive)
State New, archived
Headers show

Commit Message

rajinikanth.pandurangan@pmcs.com July 22, 2015, 4:49 p.m. UTC
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.

Signed-off-by: Rajinikanth Pandurangan <rajinikanth.pandurangan@pmcs.com>
---
 drivers/scsi/aacraid/comminit.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

Tomas Henzl July 23, 2015, 12:54 p.m. UTC | #1
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",
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
rajinikanth.pandurangan@pmcs.com July 23, 2015, 1:42 p.m. UTC | #2
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.

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

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",
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
rajinikanth.pandurangan@pmcs.com July 23, 2015, 1:52 p.m. UTC | #3
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 8 - 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.

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

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",
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomas Henzl July 23, 2015, 2:33 p.m. UTC | #4
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",
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
rajinikanth.pandurangan@pmcs.com July 23, 2015, 6:39 p.m. UTC | #5
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",
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mahesh Rajashekhara Aug. 11, 2015, 6:01 a.m. UTC | #6
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@redhat.com] 
Sent: Thursday, July 23, 2015 8:04 PM
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",
>>
> 

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

Patch

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",