The patch has more than one place where we accidentally return
positive numbers on success. I'm going to point one place, can you
please find and fix the others?
On Thu, Oct 27, 2011 at 07:40:53PM -0400, Kevin McKinney wrote:
> --- a/drivers/staging/bcm/nvm.c
> +++ b/drivers/staging/bcm/nvm.c
> @@ -443,7 +443,7 @@ static INT BeceemFlashBulkRead(
> {
> UINT uiIndex = 0;
> UINT uiBytesToRead = uiNumBytes;
> - INT Status = 0;
> + int Status;
> UINT uiPartOffset = 0;
>
> if(Adapter->device_removed )
> @@ -469,8 +469,8 @@ static INT BeceemFlashBulkRead(
> uiBytesToRead = MAX_RW_SIZE - (uiOffset%MAX_RW_SIZE);
> uiBytesToRead = MIN(uiNumBytes,uiBytesToRead);
>
> - if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
> - {
> + Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead);
> + if (Status < 0) {
> Status = -1;
> Adapter->SelectedChip = RESET_CHIP_SELECT;
> return Status;
> @@ -488,8 +488,8 @@ static INT BeceemFlashBulkRead(
>
> uiBytesToRead = MIN(uiNumBytes,MAX_RW_SIZE);
>
> - if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
> - {
> + Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead);
> + if (Status < 0) {
> Status = -1;
> break;
> }
Status will be positive at the end of this function.
Btw, you could probably get rid of the "Status = -1;" assignment from
the original code. -1 is the wrong error code here and your new code
is preserving error codes.
InterfaceAdapterInit() also has a problem caused by a positive
"retval". There is at least one other place I have not mentioned
because I am a jerk. There may be more than one because I have not
audited everything completely.
Part of the problem I think is that we have Status and retval in the
same function. The names of those things mean exactly the same thing
and they are storing similar data. Maybe the thing is to say that
rdm() always returns to a variable called count.
Example 1:
count = rdm();
if (count < 0)
return count;
Example 2:
count = rdm();
if (count < 0) {
retval = count;
break;
}
Example 3:
The ioctl version is the only place where we care about
positive values of count.
count = rdm();
if (count < 0) {
retval = count;
} else {
if (copy_to_user(foo, bar, count))
retval = -EFAULT;
}
I think that would be less error prone.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
