Messages in this thread | | | Date | Fri, 30 Jan 2015 14:13:37 +0000 | From | Bryan O'Donoghue <> | Subject | Re: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000 |
| |
On 30/01/15 14:04, Andy Shevchenko wrote: >> 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 should be clearer :)
Will change so that -ENOMEM won't be returned. ENOMEM transmits no useful information...
> >> 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?
No we don't if index >= 0 base and size are ignored => teardown by index.
I'll make that clear in a comment with the refactored statement above. -ENOMEM is redundant
-- BOD
| |