From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762294AbbA3ONo (ORCPT ); Fri, 30 Jan 2015 09:13:44 -0500 Received: from outbound-smtp02.blacknight.com ([81.17.249.8]:44861 "EHLO outbound-smtp02.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752222AbbA3ONn (ORCPT ); Fri, 30 Jan 2015 09:13:43 -0500 Message-ID: <54CB9191.4070207@nexus-software.ie> Date: Fri, 30 Jan 2015 14:13:37 +0000 From: "Bryan O'Donoghue" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Andy Shevchenko CC: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , "dvhart@infradead.org" , "Ong, Boon Leong" , "linux-kernel@vger.kernel.org" Subject: Re: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000 References: <1422587139-32139-1-git-send-email-pure.logic@nexus-software.ie> <1422587139-32139-2-git-send-email-pure.logic@nexus-software.ie> <54CB7F5F.2070308@nexus-software.ie> <54CB7FB9.3010806@nexus-software.ie> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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