lkml.org 
[lkml]   [2015]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000
From
> On 30/01/15 12:03, Andy Shevchenko wrote:

>> When CONFIG_DEBUGFS=n, you will get error pointer here, which is not NULL.
>>
>> So, the proper check is
>> if (IS_ERR())
>> return PTR_ERR();
>> if (!file)
>> return -ENOMEM;
>
> Yeah I saw that. Also saw that most other code doesn't bother trapping
> those return values - so skipped it.
> No issue adding.

Ok.

>>> + } else {
>>> + reg = i;
>>
>> Do we go always through all IMRs and choose the last one?
>> If no, break is missed here.
>
> Yep - we always choose the last one.

OK.

>>> + ret = imr_check_params(base, size);
>>> + if (ret == -EINVAL || (ret == -ENOMEM && reg == -1))
>>
>> reg base size (0 correct, 1 wrong):
>>
>> 0 0 0 — which should be used? what is the priority?
>> 0 x 1 — index
>> 0 1 x — index
>> 1 0 0 — address
>> 1 0 1 — an error
>> 1 1 1 — an error
>>
>> Thus, could it be simpler? Like
>> if (reg < 0 && ret) ?
>
> ret will be EINVAL for unaligned base or size
> ret will be ENOMEM when reg == -1 and size == 0

ENOMEM only when size == 0.

I have read the function description. From my point of view there is
no difference what is in (base, size) if we have index set.

> I could probably write it like this to make it clearer
> (ret == -EINVAL || (reg == -1 && size == 0)
> return -EINVAL;
>
> traps unaligned input - for address range tear-down
> traps zero sized - for address range tear-down
>
> Allows index based teardown i.e. reg >= 0

Again, seems a logic mistake here, or description of function is not correct.
Do we care what (base, size) if index is set? If so, why is it not
mentioned properly in the description?

Cite youself "If you specify an index on its own the base and size
parameters are ignored."
It's clearly fits to what I propose. Or I missed something?

>>> + pr_warn("debugfs register failed!\n");
>>
>> Do we actually need this? Or move it to debug level.
>
> It was your suggestion @ a previous review ....

Yes, I suggested that is not a fatal error. Since we don't check (as
above) for CONFIG_DEBUGFS=n, we get the warning only if failed to
create a file. So, probably it's fine to leave like this.

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2015-01-30 15:21    [W:0.042 / U:1.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site