Hi Dan,
On Fri, Oct 28, 2011 at 4:27 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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.
I have located these errors and I will fix them.
> 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.
Yes, I will remove this.
> 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;
> }
In each of the above solutions, shouldn't we set count =
STATUS_SUCCESS when count is greater then 0?
count = rdm();
if (count < 0) {
retval = count;
.................................
.................................
} else {
retval = STATUS_SUCCESS;
.........................................
.........................................
}
Thanks,
Kevin
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
