[{"id":1756270,"web_url":"http://patchwork.ozlabs.org/comment/1756270/","msgid":"<87pobluqt8.fsf@concordia.ellerman.id.au>","date":"2017-08-24T11:51:31","subject":"Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit()","submitter":{"id":46580,"url":"http://patchwork.ozlabs.org/api/people/46580/","name":"Michael Ellerman","email":"mpe@ellerman.id.au"},"content":"Hi Suka,\n\nComments inline ...\n\nSukadev Bhattiprolu writes:\n> diff --git a/Documentation/devicetree/bindings/powerpc/ibm,vas.txt b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt\n> new file mode 100644\n> index 0000000..0e3111d\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt\n> @@ -0,0 +1,24 @@\n> +* IBM Powerpc Virtual Accelerator Switchboard (VAS)\n> +\n> +VAS is a hardware mechanism that allows kernel subsystems and user processes\n> +to directly submit compression and other requests to Nest accelerators (NX)\n> +or other coprocessors functions.\n> +\n> +Required properties:\n> +- compatible : should be \"ibm,vas\" or \"ibm,power9-vas\"\n\nThe driver doesn't look for the latter.\n\n> +- ibm,vas-id : A unique identifier for each instance of VAS in the system\n\nWhat is this?\n\n> +- reg : Should contain 4 pairs of 64-bit fields specifying the Hypervisor\n> + window context start and length, OS/User window context start and length,\n> + \"Paste address\" start and length, \"Paste window id\" start bit and number\n> + of bits)\n> +- name : \"vas\"\n\nI don't think the name is normally included in the binding, and in fact\nthere's no reason why the name is important, so I'd be inclined to drop that.\n\n> diff --git a/MAINTAINERS b/MAINTAINERS\n> index 3c41902..abc235f 100644\n> --- a/MAINTAINERS\n> +++ b/MAINTAINERS\n> @@ -6425,6 +6425,14 @@ F:\tdrivers/crypto/nx/nx.*\n> F:\tdrivers/crypto/nx/nx_csbcpb.h\n> F:\tdrivers/crypto/nx/nx_debugfs.h\n> \n> +IBM Power Virtual Accelerator Switchboard\n> +M:\tSukadev Bhattiprolu\n> +L:\tlinuxppc-dev@lists.ozlabs.org\n> +S:\tSupported\n> +F:\tarch/powerpc/platforms/powernv/vas*\n> +F:\tarch/powerpc/include/asm/vas.h\n> +F:\tarch/powerpc/include/uapi/asm/vas.h\n\nThat's not in the right place, the file is sorted alphabetically.\n\nV comes after L.\n\n> diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig\n> index 6a6f4ef..f565454 100644\n> --- a/arch/powerpc/platforms/powernv/Kconfig\n> +++ b/arch/powerpc/platforms/powernv/Kconfig\n> @@ -30,3 +30,17 @@ config OPAL_PRD\n> \thelp\n> \t This enables the opal-prd driver, a facility to run processor\n> \t recovery diagnostics on OpenPower machines\n> +\n> +config PPC_VAS\n> +\tbool \"IBM Virtual Accelerator Switchboard (VAS)\"\n\n^ bool, so never a module.\n\n> +\tdepends on PPC_POWERNV && PPC_64K_PAGES\n> +\tdefault n\n\nIt should be default y.\n\nI know the usual advice is to make things 'default n', but this has\nfairly tight depends already, so y is OK IMO.\n\n> diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c\n> new file mode 100644\n> index 0000000..556156b\n> --- /dev/null\n> +++ b/arch/powerpc/platforms/powernv/vas.c\n> @@ -0,0 +1,183 @@\n> +/*\n> + * Copyright 2016 IBM Corp.\n\n2016-2017.\n\n> + *\n> + * This program is free software; you can redistribute it and/or\n> + * modify it under the terms of the GNU General Public License\n> + * as published by the Free Software Foundation; either version\n> + * 2 of the License, or (at your option) any later version.\n> + */\n\n#define pr_fmt(fmt) \"vas: \" fmt\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"vas.h\"\n> +\n> +static bool init_done;\n> +LIST_HEAD(vas_instances);\n\nCan be static.\n\n> +\n> +static int init_vas_instance(struct platform_device *pdev)\n> +{\n> +\tint rc, vasid;\n> +\tstruct vas_instance *vinst;\n> +\tstruct device_node *dn = pdev->dev.of_node;\n> +\tstruct resource *res;\n\n\tstruct device_node *dn = pdev->dev.of_node;\n\tstruct vas_instance *vinst;\n\tstruct resource *res;\n\tint rc, vasid;\n\nPetty I know, but much prettier :)\n\n> +\n> +\trc = of_property_read_u32(dn, \"ibm,vas-id\", &vasid);\n> +\tif (rc) {\n> +\t\tpr_err(\"VAS: No ibm,vas-id property for %s?\\n\", pdev->name);\n\nWith the pr_fmt() above you don't need VAS: on the front of all these.\n\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tif (pdev->num_resources != 4) {\n> +\t\tpr_err(\"VAS: Unexpected DT configuration for [%s, %d]\\n\",\n> +\t\t\t\tpdev->name, vasid);\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tvinst = kcalloc(1, sizeof(*vinst), GFP_KERNEL);\n\nkzalloc() would be more normal given there's only 1.\n\n> +\tif (!vinst)\n> +\t\treturn -ENOMEM;\n> +\n> +\tINIT_LIST_HEAD(&vinst->node);\n> +\tida_init(&vinst->ida);\n> +\tmutex_init(&vinst->mutex);\n> +\tvinst->vas_id = vasid;\n> +\tvinst->pdev = pdev;\n> +\n> +\tres = &pdev->resource[0];\n> +\tvinst->hvwc_bar_start = res->start;\n> +\tvinst->hvwc_bar_len = res->end - res->start + 1;\n> +\n> +\tres = &pdev->resource[1];\n> +\tvinst->uwc_bar_start = res->start;\n> +\tvinst->uwc_bar_len = res->end - res->start + 1;\n\nYou have vinst->pdev, why do you need to copy all those?\n\nI don't see the lens even used.\n\n> +\tres = &pdev->resource[2];\n> +\tvinst->paste_base_addr = res->start;\n> +\n> +\tres = &pdev->resource[3];\n> +\tvinst->paste_win_id_shift = 63 - res->end;\n\n????\n\nWhat if res->end is INT_MAX ?\n\n> +\tpr_devel(\"VAS: Initialized instance [%s, %d], paste_base 0x%llx, \"\n> +\t\t\t\"paste_win_id_shift 0x%llx\\n\", pdev->name, vasid,\n> +\t\t\tvinst->paste_base_addr, vinst->paste_win_id_shift);\n> +\n> +\tvinst->ready = true;\n\nI don't see ready used.\n\nIt also shouldn't be necessary, it's not ready unless it's in the list,\nand if it's in the list then it's ready.\n\nIf you're actually concerned about concurrent usage then you need a\nmemory barrier here to order the stores to the vinst struct vs the\naddition to the list. And you need a matching barrier on the read side.\n\n> +\tlist_add(&vinst->node, &vas_instances);\n> +\n> +\tdev_set_drvdata(&pdev->dev, vinst);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * Although this is read/used multiple times, it is written to only\n> + * during initialization.\n> + */\n> +struct vas_instance *find_vas_instance(int vasid)\n> +{\n> +\tstruct list_head *ent;\n> +\tstruct vas_instance *vinst;\n> +\n> +\tlist_for_each(ent, &vas_instances) {\n> +\t\tvinst = list_entry(ent, struct vas_instance, node);\n> +\t\tif (vinst->vas_id == vasid)\n> +\t\t\treturn vinst;\n> +\t}\n\nThere's no locking here, or any reference counting of the instances. see \n\n> +\tpr_devel(\"VAS: Instance %d not found\\n\", vasid);\n> +\treturn NULL;\n> +}\n> +\n> +bool vas_initialized(void)\n> +{\n> +\treturn init_done;\n> +}\n> +\n> +static int vas_probe(struct platform_device *pdev)\n> +{\n> +\tif (!pdev || !pdev->dev.of_node)\n> +\t\treturn -ENODEV;\n\nNeither of those can happen, or if they did it would be a BUG.\n\n> +\treturn init_vas_instance(pdev);\n> +}\n> +\n> +static void free_inst(struct vas_instance *vinst)\n> +{\n> +\tlist_del(&vinst->node);\n> +\tkfree(vinst);\n\nAnd here you just delete and the free the instance, even though you have\nhanded out pointers to the instance above in find_vas_instance().\n\nSo that needs work.\n\nIs there any hardware cleanup required before we delete the instance? Or\ndo we just leave any windows there?\n\nSeems like to begin with you should probably just not support remove.\n\n> +static int vas_remove(struct platform_device *pdev)\n> +{\n> +\tstruct vas_instance *vinst;\n> +\n> +\tvinst = dev_get_drvdata(&pdev->dev);\n> +\n> +\tpr_devel(\"VAS: Removed instance [%s, %d]\\n\", pdev->name,\n> +\t\t\t\tvinst->vas_id);\n> +\tfree_inst(vinst);\n> +\n> +\treturn 0;\n> +}\n> +static const struct of_device_id powernv_vas_match[] = {\n> +\t{ .compatible = \"ibm,vas\",},\n> +\t{},\n> +};\n> +\n> +static struct platform_driver vas_driver = {\n> +\t.driver = {\n> +\t\t.name = \"vas\",\n> +\t\t.of_match_table = powernv_vas_match,\n> +\t},\n> +\t.probe = vas_probe,\n> +\t.remove = vas_remove,\n> +};\n> +\n> +module_platform_driver(vas_driver);\n\nCan't be a module.\n\n> +\n> +int vas_init(void)\n> +{\n> +\tint found = 0;\n> +\tstruct device_node *dn;\n> +\n> +\tfor_each_compatible_node(dn, NULL, \"ibm,vas\") {\n> +\t\tof_platform_device_create(dn, NULL, NULL);\n> +\t\tfound++;\n> +\t}\n> +\n> +\tif (!found)\n> +\t\treturn -ENODEV;\n> +\n> +\tpr_devel(\"VAS: Found %d instances\\n\", found);\n> +\tinit_done = true;\n\nWhat does init_done mean?\n\nThe way it's currently written it just means there were some ibm,vas\nnodes in the device tree. But it doesn't say that we actually probed\nthem correctly and created vas instances for them.\n\nSo it doesn't really tell you much.\n\nTwo of the callers of vas_initialized() immediately do a\nfind_instance(), so they don't really need to call it at all, the lack\nof any instances will suffice.\n\nThe other two callers are vas_copy_crb() and vas_paste_crb(). The only\nuse of the former is in nx which doesn't check the return code.\n\nBut both should never be called unless we allocated a window anyway.\n\nIn short it seems unecessary.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +void vas_exit(void)\n> +{\n> +\tstruct list_head *ent;\n> +\tstruct vas_instance *vinst;\n> +\n> +\tlist_for_each(ent, &vas_instances) {\n> +\t\tvinst = list_entry(ent, struct vas_instance, node);\n> +\t\tof_platform_depopulate(&vinst->pdev->dev);\n> +\t}\n> +\n> +\tinit_done = false;\n> +}\n> +\n> +module_init(vas_init);\n> +module_exit(vas_exit);\n> +MODULE_DESCRIPTION(\"Bare metal IBM Virtual Accelerator Switchboard\");\n> +MODULE_AUTHOR(\"Sukadev Bhattiprolu \");\n> +MODULE_LICENSE(\"GPL\");\n\nCan't be a module.\n\nJust a device_initcall() should work for init, you shouldn't need\nvas_exit() or any of the other bits.\n\ncheers","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 3xdN2j6fGcz9s9Y\n\tfor ;\n\tThu, 24 Aug 2017 21:52:45 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdN2j5mNFzDqF9\n\tfor ;\n\tThu, 24 Aug 2017 21:52:45 +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 3xdN1J3kSPzDrJY\n\tfor ;\n\tThu, 24 Aug 2017 21:51:32 +1000 (AEST)","by ozlabs.org (Postfix)\n\tid 3xdN1J2yhbz9sCZ; Thu, 24 Aug 2017 21:51:32 +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 3xdN1J1Hg1z9s9Y;\n\tThu, 24 Aug 2017 21:51:32 +1000 (AEST)"],"From":"Michael Ellerman ","To":"Sukadev Bhattiprolu ","Subject":"Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit()","In-Reply-To":"<1503556688-15412-4-git-send-email-sukadev@linux.vnet.ibm.com>","References":"<1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<1503556688-15412-4-git-send-email-sukadev@linux.vnet.ibm.com>","User-Agent":"Notmuch/0.21 (https://notmuchmail.org)","Date":"Thu, 24 Aug 2017 21:51:31 +1000","Message-ID":"<87pobluqt8.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":1756833,"web_url":"http://patchwork.ozlabs.org/comment/1756833/","msgid":"<20170824214305.GB6310@us.ibm.com>","date":"2017-08-24T21:43:05","subject":"Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit()","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> Hi Suka,\n> \n> Comments inline ...\n> \n> Sukadev Bhattiprolu writes:\n> > diff --git a/Documentation/devicetree/bindings/powerpc/ibm,vas.txt b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt\n> > new file mode 100644\n> > index 0000000..0e3111d\n> > --- /dev/null\n> > +++ b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt\n> > @@ -0,0 +1,24 @@\n> > +* IBM Powerpc Virtual Accelerator Switchboard (VAS)\n> > +\n> > +VAS is a hardware mechanism that allows kernel subsystems and user processes\n> > +to directly submit compression and other requests to Nest accelerators (NX)\n> > +or other coprocessors functions.\n> > +\n> > +Required properties:\n> > +- compatible : should be \"ibm,vas\" or \"ibm,power9-vas\"\n> \n> The driver doesn't look for the latter.\n\nOk. I have removed it from this list of required properties\n\n> \n> > +- ibm,vas-id : A unique identifier for each instance of VAS in the system\n> \n> What is this?\n\nLike the ibm,chip-id, but in the future, there could be more than one instance\nof VAS per chip, so firmware assigns a unique id to each instance of VAS.\n> \n> > +- reg : Should contain 4 pairs of 64-bit fields specifying the Hypervisor\n> > + window context start and length, OS/User window context start and length,\n> > + \"Paste address\" start and length, \"Paste window id\" start bit and number\n> > + of bits)\n> > +- name : \"vas\"\n> \n> I don't think the name is normally included in the binding, and in fact\n> there's no reason why the name is important, so I'd be inclined to drop that.\n\nOk. I dropped it.\n\n> \n> > diff --git a/MAINTAINERS b/MAINTAINERS\n> > index 3c41902..abc235f 100644\n> > --- a/MAINTAINERS\n> > +++ b/MAINTAINERS\n> > @@ -6425,6 +6425,14 @@ F:\tdrivers/crypto/nx/nx.*\n> > F:\tdrivers/crypto/nx/nx_csbcpb.h\n> > F:\tdrivers/crypto/nx/nx_debugfs.h\n> > \n> > +IBM Power Virtual Accelerator Switchboard\n> > +M:\tSukadev Bhattiprolu\n> > +L:\tlinuxppc-dev@lists.ozlabs.org\n> > +S:\tSupported\n> > +F:\tarch/powerpc/platforms/powernv/vas*\n> > +F:\tarch/powerpc/include/asm/vas.h\n> > +F:\tarch/powerpc/include/uapi/asm/vas.h\n> \n> That's not in the right place, the file is sorted alphabetically.\n\nah, fixed.\n> \n> V comes after L.\n> \n> > diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig\n> > index 6a6f4ef..f565454 100644\n> > --- a/arch/powerpc/platforms/powernv/Kconfig\n> > +++ b/arch/powerpc/platforms/powernv/Kconfig\n> > @@ -30,3 +30,17 @@ config OPAL_PRD\n> > \thelp\n> > \t This enables the opal-prd driver, a facility to run processor\n> > \t recovery diagnostics on OpenPower machines\n> > +\n> > +config PPC_VAS\n> > +\tbool \"IBM Virtual Accelerator Switchboard (VAS)\"\n> \n> ^ bool, so never a module.\n\nyes, it should be built in.\n\n> \n> > +\tdepends on PPC_POWERNV && PPC_64K_PAGES\n> > +\tdefault n\n> \n> It should be default y.\n> \n> I know the usual advice is to make things 'default n', but this has\n> fairly tight depends already, so y is OK IMO.\n\nOk.\n\n> \n> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c\n> > new file mode 100644\n> > index 0000000..556156b\n> > --- /dev/null\n> > +++ b/arch/powerpc/platforms/powernv/vas.c\n> > @@ -0,0 +1,183 @@\n> > +/*\n> > + * Copyright 2016 IBM Corp.\n> \n> 2016-2017.\n\nOk.\n\n> \n> > + *\n> > + * This program is free software; you can redistribute it and/or\n> > + * modify it under the terms of the GNU General Public License\n> > + * as published by the Free Software Foundation; either version\n> > + * 2 of the License, or (at your option) any later version.\n> > + */\n> \n> #define pr_fmt(fmt) \"vas: \" fmt\n\nOk\n> \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"vas.h\"\n> > +\n> > +static bool init_done;\n> > +LIST_HEAD(vas_instances);\n> \n> Can be static.\n\nYes\n\n> \n> > +\n> > +static int init_vas_instance(struct platform_device *pdev)\n> > +{\n> > +\tint rc, vasid;\n> > +\tstruct vas_instance *vinst;\n> > +\tstruct device_node *dn = pdev->dev.of_node;\n> > +\tstruct resource *res;\n> \n> \tstruct device_node *dn = pdev->dev.of_node;\n> \tstruct vas_instance *vinst;\n> \tstruct resource *res;\n> \tint rc, vasid;\n> \n> Petty I know, but much prettier :)\n\nI usually go the opposite way (shortest first) so I have done that here also.\nFor newer files I will invert the tree.\n\n> \n> > +\n> > +\trc = of_property_read_u32(dn, \"ibm,vas-id\", &vasid);\n> > +\tif (rc) {\n> > +\t\tpr_err(\"VAS: No ibm,vas-id property for %s?\\n\", pdev->name);\n> \n> With the pr_fmt() above you don't need VAS: on the front of all these.\n\nOk\n\n> \n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\n> > +\tif (pdev->num_resources != 4) {\n> > +\t\tpr_err(\"VAS: Unexpected DT configuration for [%s, %d]\\n\",\n> > +\t\t\t\tpdev->name, vasid);\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\n> > +\tvinst = kcalloc(1, sizeof(*vinst), GFP_KERNEL);\n> \n> kzalloc() would be more normal given there's only 1.\n\nYes.\n\n> \n> > +\tif (!vinst)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tINIT_LIST_HEAD(&vinst->node);\n> > +\tida_init(&vinst->ida);\n> > +\tmutex_init(&vinst->mutex);\n> > +\tvinst->vas_id = vasid;\n> > +\tvinst->pdev = pdev;\n> > +\n> > +\tres = &pdev->resource[0];\n> > +\tvinst->hvwc_bar_start = res->start;\n> > +\tvinst->hvwc_bar_len = res->end - res->start + 1;\n> > +\n> > +\tres = &pdev->resource[1];\n> > +\tvinst->uwc_bar_start = res->start;\n> > +\tvinst->uwc_bar_len = res->end - res->start + 1;\n> \n> You have vinst->pdev, why do you need to copy all those?\n> \n> I don't see the lens even used.\n\nI have dropped the len fields. Kept the starts for now as it might\nbe easier to understand what the field is.\n\n> \n> > +\tres = &pdev->resource[2];\n> > +\tvinst->paste_base_addr = res->start;\n> > +\n> > +\tres = &pdev->resource[3];\n> > +\tvinst->paste_win_id_shift = 63 - res->end;\n> \n> ????\n> \n> What if res->end is INT_MAX ?\n\nGood question. I have added a check for res->end exceeding 62 but\nam not sure how else to error check this or, for that matter, the\nres->start fields that we get from skiboot.\n\n> \n> > +\tpr_devel(\"VAS: Initialized instance [%s, %d], paste_base 0x%llx, \"\n> > +\t\t\t\"paste_win_id_shift 0x%llx\\n\", pdev->name, vasid,\n> > +\t\t\tvinst->paste_base_addr, vinst->paste_win_id_shift);\n> > +\n> > +\tvinst->ready = true;\n> \n> I don't see ready used.\n\nYes, we don't need it now. I have dropped it.\n\n> \n> It also shouldn't be necessary, it's not ready unless it's in the list,\n> and if it's in the list then it's ready.\n> \n> If you're actually concerned about concurrent usage then you need a\n> memory barrier here to order the stores to the vinst struct vs the\n> addition to the list. And you need a matching barrier on the read side.\n> \n> > +\tlist_add(&vinst->node, &vas_instances);\n> > +\n> > +\tdev_set_drvdata(&pdev->dev, vinst);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/*\n> > + * Although this is read/used multiple times, it is written to only\n> > + * during initialization.\n> > + */\n> > +struct vas_instance *find_vas_instance(int vasid)\n> > +{\n> > +\tstruct list_head *ent;\n> > +\tstruct vas_instance *vinst;\n> > +\n> > +\tlist_for_each(ent, &vas_instances) {\n> > +\t\tvinst = list_entry(ent, struct vas_instance, node);\n> > +\t\tif (vinst->vas_id == vasid)\n> > +\t\t\treturn vinst;\n> > +\t}\n> \n> There's no locking here, or any reference counting of the instances. see \n\nThe vas_instances list is populated at startup and never really modified\n(besides, the vas_exit() code which never gets called and has now been\ndropped). I was trying to use vas_initialized() and avoid locking in\nfind_vas_instance(). \n\nI have now dropped vas_initialized() and added a lock here - this is\nused in the window setup but not in copy/paste path, so the lock should\nnot matter much.\n\n> \n> > +\tpr_devel(\"VAS: Instance %d not found\\n\", vasid);\n> > +\treturn NULL;\n> > +}\n> > +\n> > +bool vas_initialized(void)\n> > +{\n> > +\treturn init_done;\n> > +}\n> > +\n> > +static int vas_probe(struct platform_device *pdev)\n> > +{\n> > +\tif (!pdev || !pdev->dev.of_node)\n> > +\t\treturn -ENODEV;\n> \n> Neither of those can happen, or if they did it would be a BUG.\n\nOk. Changed to BUG_ON.\n> \n> > +\treturn init_vas_instance(pdev);\n> > +}\n> > +\n> > +static void free_inst(struct vas_instance *vinst)\n> > +{\n> > +\tlist_del(&vinst->node);\n> > +\tkfree(vinst);\n> \n> And here you just delete and the free the instance, even though you have\n> handed out pointers to the instance above in find_vas_instance().\n> \n> So that needs work.\n> \n> Is there any hardware cleanup required before we delete the instance? Or\n> do we just leave any windows there?\n> \n> Seems like to begin with you should probably just not support remove.\n\nYes, I have dropped support for the remove()\n> \n> > +static int vas_remove(struct platform_device *pdev)\n> > +{\n> > +\tstruct vas_instance *vinst;\n> > +\n> > +\tvinst = dev_get_drvdata(&pdev->dev);\n> > +\n> > +\tpr_devel(\"VAS: Removed instance [%s, %d]\\n\", pdev->name,\n> > +\t\t\t\tvinst->vas_id);\n> > +\tfree_inst(vinst);\n> > +\n> > +\treturn 0;\n> > +}\n> > +static const struct of_device_id powernv_vas_match[] = {\n> > +\t{ .compatible = \"ibm,vas\",},\n> > +\t{},\n> > +};\n> > +\n> > +static struct platform_driver vas_driver = {\n> > +\t.driver = {\n> > +\t\t.name = \"vas\",\n> > +\t\t.of_match_table = powernv_vas_match,\n> > +\t},\n> > +\t.probe = vas_probe,\n> > +\t.remove = vas_remove,\n> > +};\n> > +\n> > +module_platform_driver(vas_driver);\n> \n> Can't be a module.\n\nYes, its now built-in and not a module anymore.\n> \n> > +\n> > +int vas_init(void)\n> > +{\n> > +\tint found = 0;\n> > +\tstruct device_node *dn;\n> > +\n> > +\tfor_each_compatible_node(dn, NULL, \"ibm,vas\") {\n> > +\t\tof_platform_device_create(dn, NULL, NULL);\n> > +\t\tfound++;\n> > +\t}\n> > +\n> > +\tif (!found)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tpr_devel(\"VAS: Found %d instances\\n\", found);\n> > +\tinit_done = true;\n> \n> What does init_done mean?\n> \n> The way it's currently written it just means there were some ibm,vas\n> nodes in the device tree. But it doesn't say that we actually probed\n> them correctly and created vas instances for them.\n> \n> So it doesn't really tell you much.\n> \n> Two of the callers of vas_initialized() immediately do a\n> find_instance(), so they don't really need to call it at all, the lack\n> of any instances will suffice.\n> \n> The other two callers are vas_copy_crb() and vas_paste_crb(). The only\n> use of the former is in nx which doesn't check the return code.\n> \n> But both should never be called unless we allocated a window anyway.\n> \n> In short it seems unecessary.\n\nYes, I have dropped vas_initialized().\n\n> \n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void vas_exit(void)\n> > +{\n> > +\tstruct list_head *ent;\n> > +\tstruct vas_instance *vinst;\n> > +\n> > +\tlist_for_each(ent, &vas_instances) {\n> > +\t\tvinst = list_entry(ent, struct vas_instance, node);\n> > +\t\tof_platform_depopulate(&vinst->pdev->dev);\n> > +\t}\n> > +\n> > +\tinit_done = false;\n> > +}\n> > +\n> > +module_init(vas_init);\n> > +module_exit(vas_exit);\n> > +MODULE_DESCRIPTION(\"Bare metal IBM Virtual Accelerator Switchboard\");\n> > +MODULE_AUTHOR(\"Sukadev Bhattiprolu \");\n> > +MODULE_LICENSE(\"GPL\");\n> \n> Can't be a module.\n> \n> Just a device_initcall() should work for init, you shouldn't need\n> vas_exit() or any of the other bits.\n\nYes, fixed.\n\n> \n> cheers","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 3xdd9l6VCVz9sRV\n\tfor ;\n\tFri, 25 Aug 2017 07:44:43 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdd9l5NlJzDrRg\n\tfor ;\n\tFri, 25 Aug 2017 07:44:43 +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 3xdd8V5SNwzDqZ1\n\tfor ;\n\tFri, 25 Aug 2017 07:43:38 +1000 (AEST)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 3xdd8V4tjSz8tCH\n\tfor ;\n\tFri, 25 Aug 2017 07:43:38 +1000 (AEST)","by ozlabs.org (Postfix)\n\tid 3xdd8V4gKNz9t2v; Fri, 25 Aug 2017 07:43:38 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\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 3xdd8V11Kcz9sRV\n\tfor ; Fri, 25 Aug 2017 07:43:37 +1000 (AEST)","from pps.filterd (m0098414.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv7OLgboC042692\n\tfor ; Thu, 24 Aug 2017 17:43:35 -0400","from e15.ny.us.ibm.com (e15.ny.us.ibm.com [129.33.205.205])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2cj4r27ku1-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Thu, 24 Aug 2017 17:43:34 -0400","from localhost\n\tby e15.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tThu, 24 Aug 2017 17:43:11 -0400","from b01cxnp23034.gho.pok.ibm.com (9.57.198.29)\n\tby e15.ny.us.ibm.com (146.89.104.202) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 24 Aug 2017 17:43:08 -0400","from b01ledav006.gho.pok.ibm.com (b01ledav006.gho.pok.ibm.com\n\t[9.57.199.111])\n\tby b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP\n\tid v7OLh7rI25297128; Thu, 24 Aug 2017 21:43:07 GMT","from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id EBFF9AC04A;\n\tThu, 24 Aug 2017 17:43:29 -0400 (EDT)","from suka-w540.localdomain (unknown [9.70.94.25])\n\tby b01ledav006.gho.pok.ibm.com (Postfix) with ESMTP id A1733AC03F;\n\tThu, 24 Aug 2017 17:43:29 -0400 (EDT)","by suka-w540.localdomain (Postfix, from userid 1000)\n\tid 8017522927F; Thu, 24 Aug 2017 14:43:05 -0700 (PDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=sukadev@linux.vnet.ibm.com; receiver=)","Date":"Thu, 24 Aug 2017 14:43:05 -0700","From":"Sukadev Bhattiprolu ","To":"Michael Ellerman ","Subject":"Re: [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit()","References":"<1503556688-15412-1-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<1503556688-15412-4-git-send-email-sukadev@linux.vnet.ibm.com>\n\t<87pobluqt8.fsf@concordia.ellerman.id.au>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<87pobluqt8.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":"17082421-0036-0000-0000-0000025E3E25","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007605; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000225; SDB=6.00907138; UDB=6.00454734;\n\tIPR=6.00687334; \n\tBA=6.00005552; 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.00016852;\n\tXFM=3.00000015; UTC=2017-08-24 21:43:09","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17082421-0037-0000-0000-0000418BECE5","Message-Id":"<20170824214305.GB6310@us.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-08-24_08:, , 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-1708240334","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"}}]