Re: [PATCH] staging: zsmalloc: Ensure handle is never 0 on success

[Nitin Gupta <ngupta@vflare.org>]

On Tue, Nov 5, 2013 at 5:17 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> Hi Olav,
>
>
> On 11/05/2013 04:54 PM, Olav Haugan wrote:
>>
>> zsmalloc encodes a handle using the page pfn and an object
>> index. On some hardware platforms the pfn could be 0 and this
>> causes the encoded handle to be 0 which is interpreted as an
>> allocation failure.
>>
>> To prevent this false error we ensure that the encoded handle
>> will not be 0 when allocation succeeds.
>>
>> Change-Id: Ifff930dcf254915b497aec5cb36f152a5e5365d6
>> Signed-off-by: Olav Haugan <ohaugan@codeaurora.org>
>> ---
>>   drivers/staging/zsmalloc/zsmalloc-main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/zsmalloc/zsmalloc-main.c
>> b/drivers/staging/zsmalloc/zsmalloc-main.c
>> index 523b937..0e32c0f 100644
>> --- a/drivers/staging/zsmalloc/zsmalloc-main.c
>> +++ b/drivers/staging/zsmalloc/zsmalloc-main.c
>> @@ -441,7 +441,7 @@ static void *obj_location_to_handle(struct page *page,
>> unsigned long obj_idx)
>>         }
>>
>>         handle = page_to_pfn(page) << OBJ_INDEX_BITS;
>> -       handle |= (obj_idx & OBJ_INDEX_MASK);
>> +       handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
>
>
> As suggestion you could use a macro instead of hardcoded 1.
>
> I am not familiar with this code, but if it's a valid test to verify if
> the resulting address is page aligned, you might want to set this
> offset macro to a page aligned value as well.
>
>

Using a hardcoded 1 looks fine in this case. But the patch description
should also be added as a comment for this function. Otherwise, the patch
looks good to me.

>>
>>         return (void *)handle;
>>   }
>> @@ -451,7 +451,7 @@ static void obj_handle_to_location(unsigned long
>> handle, struct page **page,
>>                                 unsigned long *obj_idx)
>>   {
>>         *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
>> -       *obj_idx = handle & OBJ_INDEX_MASK;
>> +       *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
>
>
> Ditto.
>

Thanks,
Nitin