From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751180AbdH1EgP (ORCPT ); Mon, 28 Aug 2017 00:36:15 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:54792 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbdH1EgO (ORCPT ); Mon, 28 Aug 2017 00:36:14 -0400 Date: Sun, 27 Aug 2017 21:36:07 -0700 From: Sukadev Bhattiprolu To: Michael Ellerman Cc: Benjamin Herrenschmidt , mikey@neuling.org, stewart@linux.vnet.ibm.com, apopple@au1.ibm.com, hbabu@us.ibm.com, oohall@gmail.com, linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions References: <1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com> <1503556688-15412-5-git-send-email-sukadev@linux.vnet.ibm.com> <87efs0uxj4.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87efs0uxj4.fsf@concordia.ellerman.id.au> X-Operating-System: Linux 2.0.32 on an i486 User-Agent: Mutt/1.7.1 (2016-10-04) X-TM-AS-GCONF: 00 x-cbid: 17082804-0044-0000-0000-000003840072 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007625; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000225; SDB=6.00908645; UDB=6.00455613; IPR=6.00688886; BA=6.00005555; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016893; XFM=3.00000015; UTC=2017-08-28 04:36:11 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17082804-0045-0000-0000-000007B218C4 Message-Id: <20170828043607.GB12907@us.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-28_01:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708280073 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michael Ellerman [mpe@ellerman.id.au] wrote: > Hi Suka, > > Comments inline. > > Sukadev Bhattiprolu writes: > > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > > index 6156fbe..a3a705a 100644 > > --- a/arch/powerpc/platforms/powernv/vas-window.c > > +++ b/arch/powerpc/platforms/powernv/vas-window.c > > @@ -9,9 +9,182 @@ > > > > #include > > #include > > +#include > > +#include > > > > #include "vas.h" > > > > +/* > > + * Compute the paste address region for the window @window using the > > + * ->paste_base_addr and ->paste_win_id_shift we got from device tree. > > + */ > > +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len) > > +{ > > + uint64_t base, shift; > > Please use the kernel types, so u64 here. Ok. > > > + int winid; > > + > > + base = window->vinst->paste_base_addr; > > + shift = window->vinst->paste_win_id_shift; > > + winid = window->winid; > > + > > + *addr = base + (winid << shift); > > + if (len) > > + *len = PAGE_SIZE; > > Having multiple output parameters makes for a pretty awkward API. Is it > really necesssary given len is a constant PAGE_SIZE anyway. > > If you didn't return len, then you could just make the function return > the addr, and you wouldn't need any output parameters. I agree, I went back and forth on it. I was trying to avoid callers making assumptions on the size. But since there are just a couple of places, I guess we could have them assume PAGE_SIZE. > > One of the callers that passes len is unmap_paste_region(), but that > is a bit odd. It would be more natural I think if once a window is > mapped it knows its size. Or if the mapping will always just be one page > then we can just know that. Agree, since the len values are constant I was trying to avoid saving them in each of the 64K windows - so the compute during unmap. Will change to assume PAGE_SIZE. Also agree with other comments here.