Re: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000

[Andy Shevchenko]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/