[{"id":1757227,"web_url":"http://patchwork.ozlabs.org/comment/1757227/","msgid":"<878ti8uh0r.fsf@concordia.ellerman.id.au>","date":"2017-08-25T09:35:16","subject":"Re: [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free\n\twindows","submitter":{"id":46580,"url":"http://patchwork.ozlabs.org/api/people/46580/","name":"Michael Ellerman","email":"mpe@ellerman.id.au"},"content":"Sukadev Bhattiprolu writes:\n> diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c\n> index 3a50d6a..9c12919 100644\n> --- a/arch/powerpc/platforms/powernv/vas-window.c\n> +++ b/arch/powerpc/platforms/powernv/vas-window.c\n> @@ -490,6 +490,76 @@ int init_winctx_regs(struct vas_window *window, struct vas_winctx *winctx)\n> \treturn 0;\n> }\n> \n> +DEFINE_SPINLOCK(vas_ida_lock);\n\nstatic.\n\n> +\n> +void vas_release_window_id(struct ida *ida, int winid)\n\nstatic.\n\n> +{\n> +\tspin_lock(&vas_ida_lock);\n> +\tida_remove(ida, winid);\n> +\tspin_unlock(&vas_ida_lock);\n> +}\n> +\n> +int vas_assign_window_id(struct ida *ida)\n\nstatic.\n\n> +{\n> +\tint rc, winid;\n> +\n> +\trc = ida_pre_get(ida, GFP_KERNEL);\n> +\tif (!rc)\n> +\t\treturn -EAGAIN;\n> +\n> +\tspin_lock(&vas_ida_lock);\n> +\trc = ida_get_new_above(ida, 0, &winid);\n\nIf you're passing 0 you can just use ida_get_new().\n\nOr did you actually want to exclude 0? In which case you should pass 1.\n\n> +\tspin_unlock(&vas_ida_lock);\n> +\n> +\tif (rc)\n> +\t\treturn rc;\n\nYou're supposed to handle EAGAIN I thought.\n\n> +\n> +\tif (winid > VAS_WINDOWS_PER_CHIP) {\n> +\t\tpr_err(\"VAS: Too many (%d) open windows\\n\", winid);\n> +\t\tvas_release_window_id(ida, winid);\n> +\t\treturn -EAGAIN;\n> +\t}\n> +\n> +\treturn winid;\n> +}\n> +\n> +void vas_window_free(struct vas_window *window)\n\nstatic.\n\n> +{\n> +\tint winid = window->winid;\n> +\tstruct vas_instance *vinst = window->vinst;\n> +\n> +\tunmap_winctx_mmio_bars(window);\n> +\tkfree(window);\n> +\n> +\tvas_release_window_id(&vinst->ida, winid);\n> +}\n> +\n> +struct vas_window *vas_window_alloc(struct vas_instance *vinst)\n> +{\n> +\tint winid;\n> +\tstruct vas_window *window;\n> +\n> +\twinid = vas_assign_window_id(&vinst->ida);\n> +\tif (winid < 0)\n> +\t\treturn ERR_PTR(winid);\n> +\n> +\twindow = kzalloc(sizeof(*window), GFP_KERNEL);\n> +\tif (!window)\n> +\t\treturn ERR_PTR(-ENOMEM);\n\nYou leak an id here.\n\nThe error handling would be easier in here if the caller did the alloc,\nor if you split alloc and init, and alloc just did the kzalloc().\n\nOne of the callers even prints \"unable to allocate memory\" if this\nfunction fails, but that's not accurate, there's several failure modes.\n\n> +\n> +\twindow->vinst = vinst;\n> +\twindow->winid = winid;\n> +\n> +\tif (map_winctx_mmio_bars(window))\n> +\t\tgoto out_free;\n> +\n> +\treturn window;\n> +\n> +out_free:\n> +\tkfree(window);\n\nLeak an id here.\n\n> +\treturn ERR_PTR(-ENOMEM);\n> +}\n> +\n> /* stub for now */\n> int vas_win_close(struct vas_window *window)\n> {\n> -- \n> 2.7.4","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org","linuxppc-dev@ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdwzT14H0z9sP5\n\tfor ;\n\tFri, 25 Aug 2017 19:36:53 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdwzT03RmzDrVj\n\tfor ;\n\tFri, 25 Aug 2017 19:36:53 +1000 (AEST)","from ozlabs.org (bilbo.ozlabs.org [103.22.144.67])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xdwxd5BqyzDqMM\n\tfor ;\n\tFri, 25 Aug 2017 19:35:17 +1000 (AEST)","by ozlabs.org (Postfix)\n\tid 3xdwxd44Z1z9sP5; Fri, 25 Aug 2017 19:35:17 +1000 (AEST)","from authenticated.ozlabs.org (localhost [127.0.0.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPSA id 3xdwxd24s1z9sNd;\n\tFri, 25 Aug 2017 19:35:17 +1000 (AEST)"],"From":"Michael Ellerman ","To":"Sukadev Bhattiprolu ","Subject":"Re: [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free\n\twindows","In-Reply-To":"<1503556688-15412-7-git-send-email-sukadev@linux.vnet.ibm.com>","References":"<1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<1503556688-15412-7-git-send-email-sukadev@linux.vnet.ibm.com>","User-Agent":"Notmuch/0.21 (https://notmuchmail.org)","Date":"Fri, 25 Aug 2017 19:35:16 +1000","Message-ID":"<878ti8uh0r.fsf@concordia.ellerman.id.au>","MIME-Version":"1.0","Content-Type":"text/plain","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, mikey@neuling.org, linuxppc-dev@ozlabs.org, \n\tlinux-kernel@vger.kernel.org, apopple@au1.ibm.com, oohall@gmail.com","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1758361,"web_url":"http://patchwork.ozlabs.org/comment/1758361/","msgid":"<20170828045238.GD12907@us.ibm.com>","date":"2017-08-28T04:52:38","subject":"Re: [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free\n\twindows","submitter":{"id":984,"url":"http://patchwork.ozlabs.org/api/people/984/","name":"Sukadev Bhattiprolu","email":"sukadev@linux.vnet.ibm.com"},"content":"Michael Ellerman [mpe@ellerman.id.au] wrote:\n> Sukadev Bhattiprolu writes:\n> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c\n\n> > +\trc = ida_pre_get(ida, GFP_KERNEL);\n> > +\tif (!rc)\n> > +\t\treturn -EAGAIN;\n> > +\n> > +\tspin_lock(&vas_ida_lock);\n> > +\trc = ida_get_new_above(ida, 0, &winid);\n> \n> If you're passing 0 you can just use ida_get_new().\n\nOk.\n\n> \n> Or did you actually want to exclude 0? In which case you should pass 1.\n> \n> > +\tspin_unlock(&vas_ida_lock);\n> > +\n> > +\tif (rc)\n> > +\t\treturn rc;\n> \n> You're supposed to handle EAGAIN I thought.\n\nYes, I will retry the pre_get()\n> \n> > +\n> > +\tif (winid > VAS_WINDOWS_PER_CHIP) {\n> > +\t\tpr_err(\"VAS: Too many (%d) open windows\\n\", winid);\n> > +\t\tvas_release_window_id(ida, winid);\n> > +\t\treturn -EAGAIN;\n> > +\t}\n> > +\n> > +\treturn winid;\n> > +}\n> > +\n> > +void vas_window_free(struct vas_window *window)\n> \n> static.\n\nOk\n\n> \n> > +{\n> > +\tint winid = window->winid;\n> > +\tstruct vas_instance *vinst = window->vinst;\n> > +\n> > +\tunmap_winctx_mmio_bars(window);\n> > +\tkfree(window);\n> > +\n> > +\tvas_release_window_id(&vinst->ida, winid);\n> > +}\n> > +\n> > +struct vas_window *vas_window_alloc(struct vas_instance *vinst)\n> > +{\n> > +\tint winid;\n> > +\tstruct vas_window *window;\n> > +\n> > +\twinid = vas_assign_window_id(&vinst->ida);\n> > +\tif (winid < 0)\n> > +\t\treturn ERR_PTR(winid);\n> > +\n> > +\twindow = kzalloc(sizeof(*window), GFP_KERNEL);\n> > +\tif (!window)\n> > +\t\treturn ERR_PTR(-ENOMEM);\n> \n> You leak an id here.\n\nArgh. Yes.\n\n> \n> The error handling would be easier in here if the caller did the alloc,\n> or if you split alloc and init, and alloc just did the kzalloc().\n\nI was trying to simplify error handling in the callers where they have\nto only deal with one failure now.\n> \n> One of the callers even prints \"unable to allocate memory\" if this\n> function fails, but that's not accurate, there's several failure modes.\n\nYes, will fix that message and the leaks.\n\nThanks,\n\nSuka","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org","linuxppc-dev@ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xgfYc43hVz9s9Y\n\tfor ;\n\tMon, 28 Aug 2017 14:53:56 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xgfYc30tZzDqj2\n\tfor ;\n\tMon, 28 Aug 2017 14:53:56 +1000 (AEST)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xgfXF6gf4zDq5m\n\tfor ;\n\tMon, 28 Aug 2017 14:52:45 +1000 (AEST)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 3xgfXF615gz8t8c\n\tfor ;\n\tMon, 28 Aug 2017 14:52:45 +1000 (AEST)","by ozlabs.org (Postfix)\n\tid 3xgfXF5qP6z9sP3; Mon, 28 Aug 2017 14:52:45 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com\n\t[148.163.156.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xgfXF33mDz9ryv\n\tfor ; Mon, 28 Aug 2017 14:52:45 +1000 (AEST)","from pps.filterd (m0098404.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv7S4nuCJ119471\n\tfor ; Mon, 28 Aug 2017 00:52:43 -0400","from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2cm1x1kqqu-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Mon, 28 Aug 2017 00:52:43 -0400","from localhost\n\tby e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tSun, 27 Aug 2017 22:52:42 -0600","from b03cxnp08025.gho.boulder.ibm.com (9.17.130.17)\n\tby e33.co.us.ibm.com (192.168.1.133) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tSun, 27 Aug 2017 22:52:40 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp08025.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v7S4qe2T66060314; Sun, 27 Aug 2017 21:52:40 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 16B6278037;\n\tSun, 27 Aug 2017 22:52:40 -0600 (MDT)","from suka-w540.localdomain (unknown [9.70.94.25])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id DC1F47803F;\n\tSun, 27 Aug 2017 22:52:39 -0600 (MDT)","by suka-w540.localdomain (Postfix, from userid 1000)\n\tid 7155922528E; Sun, 27 Aug 2017 21:52:38 -0700 (PDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=sukadev@linux.vnet.ibm.com; receiver=)","Date":"Sun, 27 Aug 2017 21:52:38 -0700","From":"Sukadev Bhattiprolu ","To":"Michael Ellerman ","Subject":"Re: [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free\n\twindows","References":"<1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<1503556688-15412-7-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<878ti8uh0r.fsf@concordia.ellerman.id.au>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<878ti8uh0r.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-0008-0000-0000-0000087C201B","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007625; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000225; SDB=6.00908651; UDB=6.00455616;\n\tIPR=6.00688891; \n\tBA=6.00005555; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016893;\n\tXFM=3.00000015; UTC=2017-08-28 04:52:42","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17082804-0009-0000-0000-000043BD6263","Message-Id":"<20170828045238.GD12907@us.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-08-28_02:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=2\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1708280077","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, mikey@neuling.org, linuxppc-dev@ozlabs.org, \n\tlinux-kernel@vger.kernel.org, apopple@au1.ibm.com, oohall@gmail.com","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}}]