From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85DF2C32772 for ; Tue, 23 Aug 2022 15:14:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 232DB6B0074; Tue, 23 Aug 2022 11:14:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1E2276B0075; Tue, 23 Aug 2022 11:14:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0837C8D0001; Tue, 23 Aug 2022 11:14:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id EFC1D6B0074 for ; Tue, 23 Aug 2022 11:14:42 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id CD654AB2BC for ; Tue, 23 Aug 2022 15:14:42 +0000 (UTC) X-FDA: 79831204404.01.266FE65 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf17.hostedemail.com (Postfix) with ESMTP id 5323F40003 for ; Tue, 23 Aug 2022 15:14:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661267680; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Rat9a6NaBolAbjqzAgthWD8czlnUo9KIJObHQLn33Ng=; b=JsSQQRLm0VxD+rbJNo2QKDYy1gjKQBJ5a34fXT1cFhu2A8GvA8q+m5Cq5pOvFQGZ6KuKp8 nOGhhJUNJ33iKjbSy1kEDDXQZwGyQ96B2adVUWbzEpyInxYuLWgq1W4NWNoVtvVrMyR8OJ 4k4Y9G69zDHcOFWwZgYu5uP2xHK6cq0= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-462-GZqrdkMkP2aOQSYs0DyTYw-1; Tue, 23 Aug 2022 11:14:35 -0400 X-MC-Unique: GZqrdkMkP2aOQSYs0DyTYw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A7D223C0D85C; Tue, 23 Aug 2022 15:14:34 +0000 (UTC) Received: from localhost (ovpn-12-31.pek2.redhat.com [10.72.12.31]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 009FC2026D4C; Tue, 23 Aug 2022 15:14:33 +0000 (UTC) Date: Tue, 23 Aug 2022 23:14:30 +0800 From: Baoquan He To: Christophe Leroy Cc: "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "akpm@linux-foundation.org" , "hch@infradead.org" , "agordeev@linux.ibm.com" , "wangkefeng.wang@huawei.com" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename Message-ID: References: <20220820003125.353570-1-bhe@redhat.com> <20220820003125.353570-2-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=JsSQQRLm; spf=pass (imf17.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1661267681; a=rsa-sha256; cv=none; b=2N/FkwM+HvIidbxeWAVCoQjXrKDmLhZCdFaTEggHTyGnjp72Ut1ptnqCuHorNSeGkpH8gS yt0wj7VTJ0StvNdW7JAUdcLqNRNMntxLRvjKWPPRwDWtmyXi2XFZ1kBhB94+6aI3rqXlrK BP8fFs4pAeQqPjVjdweT3AsGhDbtUvA= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1661267681; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Rat9a6NaBolAbjqzAgthWD8czlnUo9KIJObHQLn33Ng=; b=Boqj2DmdafdIqgdI1RaUROJGEskWZvOOYOwNVYXbDYcBl7fkeCH/9Q6qqQeSDt+Holvo3G rFSxEqH3CA/KeSSCKNQUIvqc+0NLYaOwHIMZmDw/x7NyF82CcIrWMbMLm/9r0Xi9yIv8Jm 4UAMkIFM42MviRq0z3yZJKO1XJ6JNeo= Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=JsSQQRLm; spf=pass (imf17.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 5323F40003 X-Stat-Signature: prgsknck6jqt6hh8xs56obb1m5g6xomw X-HE-Tag: 1661267681-269419 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 08/23/22 at 05:24am, Christophe Leroy wrote: > > > Le 23/08/2022 à 02:20, Baoquan He a écrit : > > On 08/22/22 at 06:25am, Christophe Leroy wrote: > >> > >> > >> Le 20/08/2022 à 02:31, Baoquan He a écrit : > >>> In some architectures, there are ARCH specifici io address mapping > >>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64, > >>> openrisc, s390, sh. > >>> > >>> In oder to convert them to take GENERIC_IOREMAP method, we need change > >>> the return value of hook ioremap_allowed() and iounmap_allowed(). > >>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect > >>> their current behaviour. > > > > Thanks for reviewing. > > > >> > >> Please don't just say you need to change the return value. Explain why. > > > > The 1st paragraph and the sentence 'In oder to convert them to take > > GENERIC_IOREMAP method' tell the reason, no? > > What I would like to read is _why_ you need to change the return value > in order to convert to GENERIC_IOREMAP I rephrase the log as below, it's OK to you? Or please help check and tell what I need to improve to better explain the reason. ==== The current io[re|un]map_allowed() hooks are used to check if the io[re|un]map() actions are qualified to proceed when taking GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map() will return NULL. On some architectures like arc, ia64, openris, s390, sh, there are ARCH specific io address mapping to translate the passed in physical address to io address when calling ioremap(). In order to convert these architectures to take GENERIC_IOREMAP way to ioremap(), we need change the return value of hook ioremap_allowed() and iounmap_allowed(). With the change, we can move the architecture specific io address mapping into ioremap_allowed() hook, and give the mapped io address out to let ioremap_prot() return it. While at it, rename the hooks to arch_ioremap() and arch_iounmap() to reflect their new behaviour. ==== > > > > > > >> > >> And why does it need a name change ? The new name suggests that what was > >> simply a check function becomes now a function doing the job. Is that > >> the intention ? > > > > Yes, it's not a simple checking any more. It could do io address mapping > > inside arch_ioremap(), and could modify the passed in 'phys_addr' and > > 'prot' in patch 2. The ioremap_allowed() isn't appropriate to reflect > > those. > > Fair enough, then all this needs to be explained in the commit message. Sure. After we decide the hooks, I will update the log accordingly. > > > > >> > >> > >>> > >>> === > >>> arch_ioremap() return a bool, > >> > >> It is not a bool. A bool is either true or false. > > > > Thanks, I forgot to update this accordingly. > > > >> > >>> - IS_ERR means return an error > >>> - NULL means continue to remap > >>> - a non-NULL, non-IS_ERR pointer is returned directly > >>> arch_iounmap() return a bool, > >> > >> Same here, not a bool either. > > > > And this place. > >> > >>> - 0 means continue to vunmap > >>> - error code means skip vunmap and return directly > >>> > >>> This is taken from Kefeng's below old patch. Christoph suggested the > >>> return value because he foresaw the doablity of converting to take > >>> GENERIC_IOREMAP on more architectures. > >>> - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap() > >>> - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u > >>> > >>> While at it, the invocation of arch_ioremap() need be moved to the > >>> beginning of ioremap_prot() because architectures like sh, openrisc, > >>> ia64, need do the ARCH specific io address mapping on the original > >>> physical address. And in the later patch, the address fix up code > >>> in arch_ioremap() also need be done on the original addre on some > >>> architectures. > >>> > >>> This is preparation for later patch, no functionality change. > >> > >> No functionnal change, really ? > > > > You mean the new arch_ioremap() owning different definition or the > > invocation of arch_ioremap() moved up is functional change? Now I am > > not sure about the latter one, may need update my knowledge base. > > Both indeed. I understand that this first step is not changing much to > the logic, but I think the simple fact to change the arguments and name > are some how a functionnal change. OK, I thought the function interface change is not related to functional change. I will remove the 'no functionality change' sentence to avoid misleading. Thanks.