bna: fix error handling of bnad_get_flash_partition_by_offset()

PATCH In review Dan Carpenter 3 participants 5 messages Feb 9, 2012 – Feb 10, 2012

Subsystems (4)

Networking Network drivers Kernel janitors Brocade bna 10 gigabit Ethernet driver

lore

Share & tools

lore

Dan Carpenter

author

2012-02-09 10:49 UTC #0 lore raw

PATCH 1/1 bna: fix error handling of bnad_get_flash_partition_by_offset()

The current error handling doesn't work because we flash_part is a u32 so the checks for negative error codes don't work. I considered making things signed but I don't know the hardware enough to say if that's a problem. Really, we don't use the error codes so just returning zero for all problems is fine.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/ethernet/brocade/bna/bnad_ethtool.c +4 -6

index a27c601..ab753d7 100644
@@ -946,7 +946,7 @@ bnad_get_flash_partition_by_offset(struct bnad *bnad, u32 offset,
 
 	flash_attr = kzalloc(sizeof(struct bfa_flash_attr), GFP_KERNEL);
 	if (!flash_attr)
-		return -ENOMEM;
+		return 0;
 
 	fcomp.bnad = bnad;
 	fcomp.comp_status = 0;

@@ -958,7 +958,7 @@ bnad_get_flash_partition_by_offset(struct bnad *bnad, u32 offset,
 	if (ret != BFA_STATUS_OK) {
 		spin_unlock_irqrestore(&bnad->bna_lock, flags);
 		kfree(flash_attr);
-		goto out_err;
+		return 0;
 	}
 	spin_unlock_irqrestore(&bnad->bna_lock, flags);
 	wait_for_completion(&fcomp.comp);

@@ -978,8 +978,6 @@ bnad_get_flash_partition_by_offset(struct bnad *bnad, u32 offset,
 	}
 	kfree(flash_attr);
 	return flash_part;
-out_err:
-	return -EINVAL;
 }
 
 static int

@@ -1006,7 +1004,7 @@ bnad_get_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom,
 	/* Query the flash partition based on the offset */
 	flash_part = bnad_get_flash_partition_by_offset(bnad,
 				eeprom->offset, &base_offset);
-	if (flash_part <= 0)
+	if (flash_part = 0)
 		return -EFAULT;
 
 	fcomp.bnad = bnad;

@@ -1048,7 +1046,7 @@ bnad_set_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom,
 	/* Query the flash partition based on the offset */
 	flash_part = bnad_get_flash_partition_by_offset(bnad,
 				eeprom->offset, &base_offset);
-	if (flash_part <= 0)
+	if (flash_part = 0)
 		return -EFAULT;
 
 	fcomp.bnad = bnad;

David Miller

maintainer

2012-02-09 20:43 UTC #1 lore raw

From: Dan Carpenter <[email protected]>
Date: Thu, 9 Feb 2012 13:49:34 +0300

Quoted 7 lines

The current error handling doesn't work because we flash_part is a u32 so the checks for negative error codes don't work. I considered making things signed but I don't know the hardware enough to say if that's a problem. Really, we don't use the error codes so just returning zero for all problems is fine.
Signed-off-by Dan Carpenter <[email protected]>

Looks good, applied, thanks Dan.

Rasesh Mody

2012-02-10 01:53 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 8 lines

From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, February 09, 2012 2:50 AM
The current error handling doesn't work because we flash_part is a u32 so the checks for negative error codes don't work. I considered making things signed but I don't know the hardware enough to say if that's a problem. Really, we don't use the error codes so just returning zero for all problems is fine.

Hi Dan,

We can't return 0 from the bnad_get_flash_partition_by_offset() on error as the flash partition 0 is a optrom partition. Also we got comments to return proper Linux error codes as ethtool application expects so.

What we can do is change the return type of the bnad_get_flash_partition_by_offset() to int.

Thanks,
Rasesh

Dan Carpenter

author

2012-02-10 07:13 UTC #3 lore raw

Quoted 9 lines · Rasesh Mody

Quoted 8 lines
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, February 09, 2012 2:50 AM
The current error handling doesn't work because we flash_part is a u32 so the checks for negative error codes don't work. I considered making things signed but I don't know the hardware enough to say if that's a problem. Really, we don't use the error codes so just returning zero for all problems is fine.

Quoted 6 lines

Hi Dan,
We can't return 0 from the bnad_get_flash_partition_by_offset() on error as the flash partition 0 is a optrom partition. Also we got comments to return proper Linux error codes as ethtool application expects so.

It's already treated as an error. A return value of zero means the user gets a return value of -EFAULT. I'm slightly confused by your email.

My patch was already merged into git. Can you just send a patch which does what you want? I don't know the subsystem well enough to say how you want zero returns to be handled if the original code was not correct.

regards,
dan carpenter

Rasesh Mody

2012-02-10 22:38 UTC #4 lore raw

Quoted 10 lines

From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, February 09, 2012 11:16 PM
Quoted 5 lines · Rasesh Mody
We can't return 0 from the bnad_get_flash_partition_by_offset() on error as the flash partition 0 is a optrom partition. Also we got comments to return proper Linux error codes as ethtool application expects so.

Quoted 7 lines

It's already treated as an error. A return value of zero means the user gets a return value of -EFAULT. I'm slightly confused by your email.
My patch was already merged into git. Can you just send a patch which does what you want? I don't know the subsystem well enough to say how you want zero returns to be handled if the original code was not correct.

I would like to take back my prior statement, we can return 0 from the bnad_get_flash_partition_by_offset on error. Flash partition 1 and not 0 is actually optrom, so there will be no need to handle zero returns differently. Zero returns should be treated as an error and reported as -EFAULTS as its being done in current implementation. Apologies for the confusion.

Regards,
Rasesh