From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
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 03/12] powerpc/vas: Define vas_init() and vas_exit()
Date: Thu, 24 Aug 2017 14:43:05 -0700 [thread overview]
Message-ID: <20170824214305.GB6310@us.ibm.com> (raw)
In-Reply-To: <87pobluqt8.fsf@concordia.ellerman.id.au>
Michael Ellerman [mpe@ellerman.id.au] wrote:
> Hi Suka,
>
> Comments inline ...
>
> Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes:
> > diff --git a/Documentation/devicetree/bindings/powerpc/ibm,vas.txt b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt
> > new file mode 100644
> > index 0000000..0e3111d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/powerpc/ibm,vas.txt
> > @@ -0,0 +1,24 @@
> > +* IBM Powerpc Virtual Accelerator Switchboard (VAS)
> > +
> > +VAS is a hardware mechanism that allows kernel subsystems and user processes
> > +to directly submit compression and other requests to Nest accelerators (NX)
> > +or other coprocessors functions.
> > +
> > +Required properties:
> > +- compatible : should be "ibm,vas" or "ibm,power9-vas"
>
> The driver doesn't look for the latter.
Ok. I have removed it from this list of required properties
>
> > +- ibm,vas-id : A unique identifier for each instance of VAS in the system
>
> What is this?
Like the ibm,chip-id, but in the future, there could be more than one instance
of VAS per chip, so firmware assigns a unique id to each instance of VAS.
>
> > +- reg : Should contain 4 pairs of 64-bit fields specifying the Hypervisor
> > + window context start and length, OS/User window context start and length,
> > + "Paste address" start and length, "Paste window id" start bit and number
> > + of bits)
> > +- name : "vas"
>
> I don't think the name is normally included in the binding, and in fact
> there's no reason why the name is important, so I'd be inclined to drop that.
Ok. I dropped it.
>
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3c41902..abc235f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6425,6 +6425,14 @@ F: drivers/crypto/nx/nx.*
> > F: drivers/crypto/nx/nx_csbcpb.h
> > F: drivers/crypto/nx/nx_debugfs.h
> >
> > +IBM Power Virtual Accelerator Switchboard
> > +M: Sukadev Bhattiprolu
> > +L: linuxppc-dev@lists.ozlabs.org
> > +S: Supported
> > +F: arch/powerpc/platforms/powernv/vas*
> > +F: arch/powerpc/include/asm/vas.h
> > +F: arch/powerpc/include/uapi/asm/vas.h
>
> That's not in the right place, the file is sorted alphabetically.
ah, fixed.
>
> V comes after L.
>
> > diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig
> > index 6a6f4ef..f565454 100644
> > --- a/arch/powerpc/platforms/powernv/Kconfig
> > +++ b/arch/powerpc/platforms/powernv/Kconfig
> > @@ -30,3 +30,17 @@ config OPAL_PRD
> > help
> > This enables the opal-prd driver, a facility to run processor
> > recovery diagnostics on OpenPower machines
> > +
> > +config PPC_VAS
> > + bool "IBM Virtual Accelerator Switchboard (VAS)"
>
> ^ bool, so never a module.
yes, it should be built in.
>
> > + depends on PPC_POWERNV && PPC_64K_PAGES
> > + default n
>
> It should be default y.
>
> I know the usual advice is to make things 'default n', but this has
> fairly tight depends already, so y is OK IMO.
Ok.
>
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > new file mode 100644
> > index 0000000..556156b
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -0,0 +1,183 @@
> > +/*
> > + * Copyright 2016 IBM Corp.
>
> 2016-2017.
Ok.
>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
>
> #define pr_fmt(fmt) "vas: " fmt
Ok
>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/export.h>
> > +#include <linux/types.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of.h>
> > +
> > +#include "vas.h"
> > +
> > +static bool init_done;
> > +LIST_HEAD(vas_instances);
>
> Can be static.
Yes
>
> > +
> > +static int init_vas_instance(struct platform_device *pdev)
> > +{
> > + int rc, vasid;
> > + struct vas_instance *vinst;
> > + struct device_node *dn = pdev->dev.of_node;
> > + struct resource *res;
>
> struct device_node *dn = pdev->dev.of_node;
> struct vas_instance *vinst;
> struct resource *res;
> int rc, vasid;
>
> Petty I know, but much prettier :)
I usually go the opposite way (shortest first) so I have done that here also.
For newer files I will invert the tree.
>
> > +
> > + rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
> > + if (rc) {
> > + pr_err("VAS: No ibm,vas-id property for %s?\n", pdev->name);
>
> With the pr_fmt() above you don't need VAS: on the front of all these.
Ok
>
> > + return -ENODEV;
> > + }
> > +
> > + if (pdev->num_resources != 4) {
> > + pr_err("VAS: Unexpected DT configuration for [%s, %d]\n",
> > + pdev->name, vasid);
> > + return -ENODEV;
> > + }
> > +
> > + vinst = kcalloc(1, sizeof(*vinst), GFP_KERNEL);
>
> kzalloc() would be more normal given there's only 1.
Yes.
>
> > + if (!vinst)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&vinst->node);
> > + ida_init(&vinst->ida);
> > + mutex_init(&vinst->mutex);
> > + vinst->vas_id = vasid;
> > + vinst->pdev = pdev;
> > +
> > + res = &pdev->resource[0];
> > + vinst->hvwc_bar_start = res->start;
> > + vinst->hvwc_bar_len = res->end - res->start + 1;
> > +
> > + res = &pdev->resource[1];
> > + vinst->uwc_bar_start = res->start;
> > + vinst->uwc_bar_len = res->end - res->start + 1;
>
> You have vinst->pdev, why do you need to copy all those?
>
> I don't see the lens even used.
I have dropped the len fields. Kept the starts for now as it might
be easier to understand what the field is.
>
> > + res = &pdev->resource[2];
> > + vinst->paste_base_addr = res->start;
> > +
> > + res = &pdev->resource[3];
> > + vinst->paste_win_id_shift = 63 - res->end;
>
> ????
>
> What if res->end is INT_MAX ?
Good question. I have added a check for res->end exceeding 62 but
am not sure how else to error check this or, for that matter, the
res->start fields that we get from skiboot.
>
> > + pr_devel("VAS: Initialized instance [%s, %d], paste_base 0x%llx, "
> > + "paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> > + vinst->paste_base_addr, vinst->paste_win_id_shift);
> > +
> > + vinst->ready = true;
>
> I don't see ready used.
Yes, we don't need it now. I have dropped it.
>
> It also shouldn't be necessary, it's not ready unless it's in the list,
> and if it's in the list then it's ready.
>
> If you're actually concerned about concurrent usage then you need a
> memory barrier here to order the stores to the vinst struct vs the
> addition to the list. And you need a matching barrier on the read side.
>
> > + list_add(&vinst->node, &vas_instances);
> > +
> > + dev_set_drvdata(&pdev->dev, vinst);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Although this is read/used multiple times, it is written to only
> > + * during initialization.
> > + */
> > +struct vas_instance *find_vas_instance(int vasid)
> > +{
> > + struct list_head *ent;
> > + struct vas_instance *vinst;
> > +
> > + list_for_each(ent, &vas_instances) {
> > + vinst = list_entry(ent, struct vas_instance, node);
> > + if (vinst->vas_id == vasid)
> > + return vinst;
> > + }
>
> There's no locking here, or any reference counting of the instances. see
The vas_instances list is populated at startup and never really modified
(besides, the vas_exit() code which never gets called and has now been
dropped). I was trying to use vas_initialized() and avoid locking in
find_vas_instance().
I have now dropped vas_initialized() and added a lock here - this is
used in the window setup but not in copy/paste path, so the lock should
not matter much.
>
> > + pr_devel("VAS: Instance %d not found\n", vasid);
> > + return NULL;
> > +}
> > +
> > +bool vas_initialized(void)
> > +{
> > + return init_done;
> > +}
> > +
> > +static int vas_probe(struct platform_device *pdev)
> > +{
> > + if (!pdev || !pdev->dev.of_node)
> > + return -ENODEV;
>
> Neither of those can happen, or if they did it would be a BUG.
Ok. Changed to BUG_ON.
>
> > + return init_vas_instance(pdev);
> > +}
> > +
> > +static void free_inst(struct vas_instance *vinst)
> > +{
> > + list_del(&vinst->node);
> > + kfree(vinst);
>
> And here you just delete and the free the instance, even though you have
> handed out pointers to the instance above in find_vas_instance().
>
> So that needs work.
>
> Is there any hardware cleanup required before we delete the instance? Or
> do we just leave any windows there?
>
> Seems like to begin with you should probably just not support remove.
Yes, I have dropped support for the remove()
>
> > +static int vas_remove(struct platform_device *pdev)
> > +{
> > + struct vas_instance *vinst;
> > +
> > + vinst = dev_get_drvdata(&pdev->dev);
> > +
> > + pr_devel("VAS: Removed instance [%s, %d]\n", pdev->name,
> > + vinst->vas_id);
> > + free_inst(vinst);
> > +
> > + return 0;
> > +}
> > +static const struct of_device_id powernv_vas_match[] = {
> > + { .compatible = "ibm,vas",},
> > + {},
> > +};
> > +
> > +static struct platform_driver vas_driver = {
> > + .driver = {
> > + .name = "vas",
> > + .of_match_table = powernv_vas_match,
> > + },
> > + .probe = vas_probe,
> > + .remove = vas_remove,
> > +};
> > +
> > +module_platform_driver(vas_driver);
>
> Can't be a module.
Yes, its now built-in and not a module anymore.
>
> > +
> > +int vas_init(void)
> > +{
> > + int found = 0;
> > + struct device_node *dn;
> > +
> > + for_each_compatible_node(dn, NULL, "ibm,vas") {
> > + of_platform_device_create(dn, NULL, NULL);
> > + found++;
> > + }
> > +
> > + if (!found)
> > + return -ENODEV;
> > +
> > + pr_devel("VAS: Found %d instances\n", found);
> > + init_done = true;
>
> What does init_done mean?
>
> The way it's currently written it just means there were some ibm,vas
> nodes in the device tree. But it doesn't say that we actually probed
> them correctly and created vas instances for them.
>
> So it doesn't really tell you much.
>
> Two of the callers of vas_initialized() immediately do a
> find_instance(), so they don't really need to call it at all, the lack
> of any instances will suffice.
>
> The other two callers are vas_copy_crb() and vas_paste_crb(). The only
> use of the former is in nx which doesn't check the return code.
>
> But both should never be called unless we allocated a window anyway.
>
> In short it seems unecessary.
Yes, I have dropped vas_initialized().
>
> > +
> > + return 0;
> > +}
> > +
> > +void vas_exit(void)
> > +{
> > + struct list_head *ent;
> > + struct vas_instance *vinst;
> > +
> > + list_for_each(ent, &vas_instances) {
> > + vinst = list_entry(ent, struct vas_instance, node);
> > + of_platform_depopulate(&vinst->pdev->dev);
> > + }
> > +
> > + init_done = false;
> > +}
> > +
> > +module_init(vas_init);
> > +module_exit(vas_exit);
> > +MODULE_DESCRIPTION("Bare metal IBM Virtual Accelerator Switchboard");
> > +MODULE_AUTHOR("Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>");
> > +MODULE_LICENSE("GPL");
>
> Can't be a module.
>
> Just a device_initcall() should work for init, you shouldn't need
> vas_exit() or any of the other bits.
Yes, fixed.
>
> cheers
next prev parent reply other threads:[~2017-08-24 21:43 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 6:37 [PATCH v7 00/12] Enable VAS Sukadev Bhattiprolu
2017-08-24 6:37 ` [PATCH v7 01/12] powerpc/vas: Define macros, register fields and structures Sukadev Bhattiprolu
2017-08-25 9:47 ` Michael Ellerman
2017-08-24 6:37 ` [PATCH v7 02/12] Move GET_FIELD/SET_FIELD to vas.h Sukadev Bhattiprolu
2017-08-24 6:37 ` [PATCH v7 03/12] powerpc/vas: Define vas_init() and vas_exit() Sukadev Bhattiprolu
2017-08-24 11:51 ` Michael Ellerman
2017-08-24 21:43 ` Sukadev Bhattiprolu [this message]
2017-08-24 6:38 ` [PATCH v7 04/12] powerpc/vas: Define helpers to access MMIO regions Sukadev Bhattiprolu
2017-08-25 3:38 ` Michael Ellerman
2017-08-28 4:36 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 05/12] powerpc/vas: Define helpers to init window context Sukadev Bhattiprolu
2017-08-25 9:25 ` Michael Ellerman
2017-08-28 4:44 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 06/12] powerpc/vas: Define helpers to alloc/free windows Sukadev Bhattiprolu
2017-08-25 9:35 ` Michael Ellerman
2017-08-28 4:52 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 07/12] powerpc/vas: Define vas_win_paste_addr() Sukadev Bhattiprolu
2017-08-25 9:36 ` Michael Ellerman
2017-08-24 6:38 ` [PATCH v7 08/12] powerpc/vas: Define vas_win_id() Sukadev Bhattiprolu
2017-08-25 9:37 ` Michael Ellerman
2017-08-28 4:53 ` Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 09/12] powerpc/vas: Define vas_rx_win_open() interface Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 10/12] powerpc/vas: Define vas_win_close() interface Sukadev Bhattiprolu
2017-08-25 10:02 ` Michael Ellerman
2017-08-28 5:14 ` Sukadev Bhattiprolu
2017-08-28 11:43 ` Michael Ellerman
2017-08-24 6:38 ` [PATCH v7 11/12] powerpc/vas: Define vas_tx_win_open() Sukadev Bhattiprolu
2017-08-24 6:38 ` [PATCH v7 12/12] powerpc/vas: Define copy/paste interfaces Sukadev Bhattiprolu
2017-08-25 10:56 ` Michael Ellerman
2017-08-28 5:20 ` Sukadev Bhattiprolu
2017-08-28 11:45 ` Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170824214305.GB6310@us.ibm.com \
--to=sukadev@linux.vnet.ibm.com \
--cc=apopple@au1.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=hbabu@us.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@ozlabs.org \
--cc=mikey@neuling.org \
--cc=mpe@ellerman.id.au \
--cc=oohall@gmail.com \
--cc=stewart@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.