Re: [PATCH] bnx2x: fix assignment of signed expression to unsigned variable

On Sat, Mar 23, 2013 at 10:56:47PM +0300, Dan Carpenter wrote:

On Sat, Mar 23, 2013 at 11:52:55AM -0700, Kumar Amit Mehta wrote:

quoted

fix for incorrect assignment of signed expression to unsigned variable.

This fix isn't right.

quoted

Signed-off-by: Kumar Amit Mehta <redacted>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
index 5682054..b90533a 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c

@@ -2139,12 +2139,12 @@ static u8 bnx2x_dcbnl_get_cap(struct net_device *netdev, int capid, u8 *cap)
 			break;
 		default:
 			BNX2X_ERR("Non valid capability ID\n");
-			rval = -EINVAL;
+			rval = EINVAL;
 			break;
 		}
 	} else {
 		DP(BNX2X_MSG_DCB, "DCB disabled\n");
-		rval = -EINVAL;
+		rval = EINVAL;

This function is called from dcbnl_getcap().  It returns zero on
success, but it's not clear what it should return on failure.  But
certainly it's not a positive EINVAL.

quoted

 	}
 
 	DP(BNX2X_MSG_DCB, "capid %d:%x\n", capid, *cap);

@@ -2154,7 +2154,7 @@ static u8 bnx2x_dcbnl_get_cap(struct net_device *netdev, int capid, u8 *cap)
 static int bnx2x_dcbnl_get_numtcs(struct net_device *netdev, int tcid, u8 *num)
 {
 	struct bnx2x *bp = netdev_priv(netdev);
-	u8 rval = 0;
+	s8 rval = 0;

Just use "int" here.

quoted

 
 	DP(BNX2X_MSG_DCB, "tcid %d\n", tcid);
 

@@ -2188,7 +2188,7 @@ static int bnx2x_dcbnl_set_numtcs(struct net_device *netdev, int tcid, u8 num)
 	return -EINVAL;
 }
 
-static u8  bnx2x_dcbnl_get_pfc_state(struct net_device *netdev)
+static u8 bnx2x_dcbnl_get_pfc_state(struct net_device *netdev)
 {
 	struct bnx2x *bp = netdev_priv(netdev);
 	DP(BNX2X_MSG_DCB, "state = %d\n", bp->dcbx_local_feat.pfc.enabled);

@@ -2282,7 +2282,7 @@ static int bnx2x_set_admin_app_up(struct bnx2x *bp, u8 idtype, u16 idval, u8 up)
 	else {
 		/* app table is full */
 		BNX2X_ERR("Application table is too large\n");
-		return -EBUSY;
+		return EBUSY;
 	}

The fix for this is more involved.  The function prototype should
be changed to return an int.  The caller should be updated to check
for errors.

I wonder, why these callbacks are restricting the lower layers to return
only unsigned values, However Updating the prototype doesn't seem a good idea as
then it would require changes in various existing drivers.