Patchwork netxen: Error return off by one in 'netxen_nic_set_pauseparam()'.

login
register
mail settings
Submitter santosh nayak
Date June 20, 2012, 7:27 a.m.
Message ID <1340177259-14083-1-git-send-email-santoshprasadnayak@gmail.com>
Download mbox | patch
Permalink /patch/165907/
State Accepted
Delegated to: David Miller
Headers show

Comments

santosh nayak - June 20, 2012, 7:27 a.m.
From: Santosh Nayak <santoshprasadnayak@gmail.com>

There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
from zero. 
Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"

Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
Destination tree "linux-next"


 .../ethernet/qlogic/netxen/netxen_nic_ethtool.c    |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Dan Carpenter - June 20, 2012, 7:44 a.m.
On Wed, Jun 20, 2012 at 12:57:39PM +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
> from zero. 
> Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"
> 

I don't know this code well enough to say if you are right or not,
but what about for port == NETXEN_NIU_MAX_XG_PORTS a few lines later
in both functions?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter - June 20, 2012, 7:57 a.m.
On Wed, Jun 20, 2012 at 12:57:39PM +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
> from zero. 
> Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"
> 

So my understanding is that you are guessing on this based on the
fact that, "Who counts from 0-4 inclusive?".  I can't argue with
that logic.

Looking some more at NETXEN_NIU_MAX_XG_PORTS.  It is used off by one
4 times and it's used correctly 1 time.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
santosh nayak - June 20, 2012, 7:59 a.m.
On Wed, Jun 20, 2012 at 1:14 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Jun 20, 2012 at 12:57:39PM +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
>> from zero.
>> Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"
>>
>
> I don't know this code well enough to say if you are right or not,
> but what about for port == NETXEN_NIU_MAX_XG_PORTS a few lines later
> in both functions?


I think "for port == NETXEN_NIU_MAX_XG_PORTS"  error should be returned.


@Rajesh,

Can you please comment on it ?


regards
santosh

>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajesh Borundia - June 20, 2012, 9:51 a.m.

santosh nayak - June 20, 2012, 10:16 a.m.
On Wed, Jun 20, 2012 at 3:21 PM, Rajesh Borundia
<rajesh.borundia@qlogic.com> wrote:
> _______________________________________
> From: santosh prasad nayak [santoshprasadnayak@gmail.com]
> Sent: Wednesday, June 20, 2012 1:29 PM
> To: Dan Carpenter; Rajesh Borundia
> Cc: Sony Chacko; netdev; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] netxen: Error return off by one in 'netxen_nic_set_pauseparam()'.
>
> On Wed, Jun 20, 2012 at 1:14 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> On Wed, Jun 20, 2012 at 12:57:39PM +0530, santosh nayak wrote:
>>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>>
>>> There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
>>> from zero.
>>> Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"
>>>
>>
>> I don't know this code well enough to say if you are right or not,
>> but what about for port == NETXEN_NIU_MAX_XG_PORTS a few lines later
>> in both functions?
>
>
> I think "for port == NETXEN_NIU_MAX_XG_PORTS"  error should be returned.
>
>
> @Rajesh,
>
> Can you please comment on it ?
>
>
> regards
> santosh
>
>>
>> regards,
>> dan carpenter
>>
>
> Yes error should be returned for  both port == NETXEN_NIU_MAX_XG_PORTS and
> port ==  NETXEN_NIU_MAX_GBE_PORTS.


Ok.

The current patch is for GBE port.
For XG port I will send another patch.

regards
santosh


>
>
> Rajesh
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 25, 2012, 10:27 p.m.
From: santosh nayak <santoshprasadnayak@gmail.com>
Date: Wed, 20 Jun 2012 12:57:39 +0530

> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> There are 'NETXEN_NIU_MAX_GBE_PORTS'  GBE ports. Port indexing starts
> from zero. 
> Hence we should also return error for "port == NETXEN_NIU_MAX_GBE_PORTS"
> 
> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>

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

Patch

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
index 3973040..d4f179f 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
@@ -489,7 +489,7 @@  netxen_nic_get_pauseparam(struct net_device *dev,
 	int port = adapter->physical_port;
 
 	if (adapter->ahw.port_type == NETXEN_NIC_GBE) {
-		if ((port < 0) || (port > NETXEN_NIU_MAX_GBE_PORTS))
+		if ((port < 0) || (port >= NETXEN_NIU_MAX_GBE_PORTS))
 			return;
 		/* get flow control settings */
 		val = NXRD32(adapter, NETXEN_NIU_GB_MAC_CONFIG_0(port));
@@ -534,7 +534,7 @@  netxen_nic_set_pauseparam(struct net_device *dev,
 	int port = adapter->physical_port;
 	/* read mode */
 	if (adapter->ahw.port_type == NETXEN_NIC_GBE) {
-		if ((port < 0) || (port > NETXEN_NIU_MAX_GBE_PORTS))
+		if ((port < 0) || (port >= NETXEN_NIU_MAX_GBE_PORTS))
 			return -EIO;
 		/* set flow control */
 		val = NXRD32(adapter, NETXEN_NIU_GB_MAC_CONFIG_0(port));