Is high_memory check in omap2.c for OneNAND is sufficient?

Is high_memory check in omap2.c for OneNAND is sufficient?

[Singh, Vimal]

Hi,

There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
Snippet:
----------------------
        if (buf >= high_memory) {
                struct page *p1;
----------------------
This check seems not sufficient. There should be a check for upper boundary too.
Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
Then again, function 'dma_cache_maint' will report bug.

Snippet:
--------------
void dma_cache_maint(const void *start, size_t size, int direction)
{
        const void *end = start + size;

        BUG_ON(!virt_addr_valid(start) || !virt_addr_valid(end - 1));
-------------

Should we replace "if (buf >= high_memory)" with "if (buf >= high_memory || (buf + count -1) >= high_memory)"...?

---
vimal

Re: Is high_memory check in omap2.c for OneNAND is sufficient?

[Adrian Hunter]

Singh, Vimal wrote:
> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
> Snippet:
> ----------------------
>         if (buf >= high_memory) {
>                 struct page *p1;
> ----------------------
> This check seems not sufficient. There should be a check for upper boundary too.
> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.

AFAIK it is not possible to allocate memory that crosses the high_memory boundary.

RE: Is high_memory check in omap2.c for OneNAND is sufficient?

[Singh, Vimal]

From: Adrian Hunter [adrian.hunter@nokia.com]
> Singh, Vimal wrote:
> > There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
> > Snippet:
> > ----------------------
> >         if (buf >= high_memory) {
> >                 struct page *p1;
> > ----------------------
> > This check seems not sufficient. There should be a check for upper boundary too.
> > Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
> 
> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.

Do you mean 'buf' can not cross 'high_memory' boundary?
But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.

Re: Is high_memory check in omap2.c for OneNAND is sufficient?

[Adrian Hunter]

Singh, Vimal wrote:
> From: Adrian Hunter [adrian.hunter@nokia.com]
>> Singh, Vimal wrote:
>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>> Snippet:
>>> ----------------------
>>>         if (buf >= high_memory) {
>>>                 struct page *p1;
>>> ----------------------
>>> This check seems not sufficient. There should be a check for upper boundary too.
>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
> 
> Do you mean 'buf' can not cross 'high_memory' boundary?

Yes.

> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.

Is it possible that 'buf' or 'count' is wrong?

RE: Is high_memory check in omap2.c for OneNAND is sufficient?

[Singh, Vimal]

From: Adrian Hunter [adrian.hunter@nokia.com]
>Singh, Vimal wrote:
>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>> Singh, Vimal wrote:
>>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>>> Snippet:
>>>> ----------------------
>>>>         if (buf >= high_memory) {
>>>>                 struct page *p1;
>>>> ----------------------
>>>> This check seems not sufficient. There should be a check for upper boundary too.
>>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
>>
>> Do you mean 'buf' can not cross 'high_memory' boundary?
>
>Yes.
If so, then I wonder why above check is present in code...

>
>> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.
>
>Is it possible that 'buf' or 'count' is wrong?
I do not think so...

Re: Is high_memory check in omap2.c for OneNAND is sufficient?

[Adrian Hunter]

Singh, Vimal wrote:
> From: Adrian Hunter [adrian.hunter@nokia.com]
>> Singh, Vimal wrote:
>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>> Singh, Vimal wrote:
>>>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>>>> Snippet:
>>>>> ----------------------
>>>>>         if (buf >= high_memory) {
>>>>>                 struct page *p1;
>>>>> ----------------------
>>>>> This check seems not sufficient. There should be a check for upper boundary too.
>>>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>>>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
>>> Do you mean 'buf' can not cross 'high_memory' boundary?
>> Yes.
> If so, then I wonder why above check is present in code...
> 

Which check?

>>> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.
>> Is it possible that 'buf' or 'count' is wrong?
> I do not think so...
> 

RE: Is high_memory check in omap2.c for OneNAND is sufficient?

[Singh, Vimal]

From: Adrian Hunter [adrian.hunter@nokia.com]
>Singh, Vimal wrote:
>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>> Singh, Vimal wrote:
>>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>>> Singh, Vimal wrote:
>>>>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>>>>> Snippet:
>>>>>> ----------------------
>>>>>>         if (buf >= high_memory) {
>>>>>>                 struct page *p1;
>>>>>> ----------------------
>>>>>> This check seems not sufficient. There should be a check for upper boundary too.
>>>>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>>>>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
>>>> Do you mean 'buf' can not cross 'high_memory' boundary?
>>> Yes.
>> If so, then I wonder why above check is present in code...
>>
>
>Which check?
>

I mean, if 'buf' can cross 'high_memory' boundary then why do we nedd "if (buf >= high_memory)" check and re-manipulating 'buf' in code (in drivers/mtd/onenand/omap2.c).

I am really confuse now... when this particular condition will occur.
 

>>>> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.
>>> Is it possible that 'buf' or 'count' is wrong?
>> I do not think so...
>>

Re: Is high_memory check in omap2.c for OneNAND is sufficient?

[Adrian Hunter]

ext Singh, Vimal wrote:
> From: Adrian Hunter [adrian.hunter@nokia.com]
>> Singh, Vimal wrote:
>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>> Singh, Vimal wrote:
>>>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>>>> Singh, Vimal wrote:
>>>>>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>>>>>> Snippet:
>>>>>>> ----------------------
>>>>>>>         if (buf >= high_memory) {
>>>>>>>                 struct page *p1;
>>>>>>> ----------------------
>>>>>>> This check seems not sufficient. There should be a check for upper boundary too.
>>>>>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>>>>>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
>>>>> Do you mean 'buf' can not cross 'high_memory' boundary?
>>>> Yes.
>>> If so, then I wonder why above check is present in code...
>>>
>> Which check?
>>
> 
> I mean, if 'buf' can cross 'high_memory' boundary then why do we nedd "if (buf >= high_memory)" check and re-manipulating 'buf' in code (in drivers/mtd/onenand/omap2.c).
> 
> I am really confuse now... when this particular condition will occur.
>  
> 

buf can be below high_memory but in that case buf + count cannot be high_memory or above.

The addresses below high_memory are mapped differently to those above high_memory.

If you kmalloc, you get an address below high_memory.

If you vmalloc, you get an address above high_memory.

Never does a memory allocation cross above and below high_memory.

That is my understanding (on OMAP at least), but I am far from a guru, so please correct me if I am wrong.

>>>>> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.
>>>> Is it possible that 'buf' or 'count' is wrong?
>>> I do not think so...
>>>
> 
> 
> 

RE: Is high_memory check in omap2.c for OneNAND is sufficient?

[Singh, Vimal]

Thanks Andrian... this helped me a lot...
________________________________________
From: Adrian Hunter [adrian.hunter@nokia.com]
Sent: Thursday, March 26, 2009 2:25 PM
To: Singh, Vimal
Cc: linux-mtd@lists.infradead.org
Subject: Re: Is high_memory check in omap2.c for OneNAND is sufficient?

ext Singh, Vimal wrote:
> From: Adrian Hunter [adrian.hunter@nokia.com]
>> Singh, Vimal wrote:
>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>> Singh, Vimal wrote:
>>>>> From: Adrian Hunter [adrian.hunter@nokia.com]
>>>>>> Singh, Vimal wrote:
>>>>>>> There is check for 'high_memory' in 'drivers/mtd/onenand/omap2.c', always before doing 'dma_map_single'.
>>>>>>> Snippet:
>>>>>>> ----------------------
>>>>>>>         if (buf >= high_memory) {
>>>>>>>                 struct page *p1;
>>>>>>> ----------------------
>>>>>>> This check seems not sufficient. There should be a check for upper boundary too.
>>>>>>> Thinking scenario when 'buf' is less than 'high_memory', but somewhere near to it, and 'count' is big enough to beyond 'high_memory'.
>>>>>> AFAIK it is not possible to allocate memory that crosses the high_memory boundary.
>>>>> Do you mean 'buf' can not cross 'high_memory' boundary?
>>>> Yes.
>>> If so, then I wonder why above check is present in code...
>>>
>> Which check?
>>
>
> I mean, if 'buf' can cross 'high_memory' boundary then why do we nedd "if (buf >= high_memory)" check and re-manipulating 'buf' in code (in drivers/mtd/onenand/omap2.c).
>
> I am really confuse now... when this particular condition will occur.
>
>

buf can be below high_memory but in that case buf + count cannot be high_memory or above.

The addresses below high_memory are mapped differently to those above high_memory.

If you kmalloc, you get an address below high_memory.

If you vmalloc, you get an address above high_memory.

Never does a memory allocation cross above and below high_memory.

That is my understanding (on OMAP at least), but I am far from a guru, so please correct me if I am wrong.

>>>>> But then I have seen a case where it was crossing that and BUG was reported by function 'dma_cache_maint'.
>>>> Is it possible that 'buf' or 'count' is wrong?
>>> I do not think so...
>>>
>
>
>