| Message ID | 20180411063855.5451-2-alistair@popple.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a1409adac748f0db655e096521bbe6904aadeb98 |
| Headers | show |
| Series | [1/2] powernv/npu: Add lock to prevent race in concurrent context init/destroy | expand |
On Wed, 11 Apr 2018, Alistair Popple wrote: > There is a single npu context per set of callback parameters. Callers > should be prevented from overwriting existing callback values so instead > return an error if different parameters are passed. > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > --- > arch/powerpc/include/asm/powernv.h | 2 +- > arch/powerpc/platforms/powernv/npu-dma.c | 16 +++++++++++++--- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h > index dc5f6a5d4575..362ea12a4501 100644 > --- a/arch/powerpc/include/asm/powernv.h > +++ b/arch/powerpc/include/asm/powernv.h > @@ -15,7 +15,7 @@ > extern void powernv_set_nmmu_ptcr(unsigned long ptcr); > extern struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > unsigned long flags, > - struct npu_context *(*cb)(struct npu_context *, void *), > + void (*cb)(struct npu_context *, void *), > void *priv); > extern void pnv_npu2_destroy_context(struct npu_context *context, > struct pci_dev *gpdev); > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > index cb77162f4e7a..193f43ea3fbc 100644 > --- a/arch/powerpc/platforms/powernv/npu-dma.c > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > @@ -407,7 +407,7 @@ struct npu_context { > bool nmmu_flush; > > /* Callback to stop translation requests on a given GPU */ > - struct npu_context *(*release_cb)(struct npu_context *, void *); > + void (*release_cb)(struct npu_context *context, void *priv); > > /* > * Private pointer passed to the above callback for usage by > @@ -705,7 +705,7 @@ static const struct mmu_notifier_ops nv_nmmu_notifier_ops = { > */ > struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > unsigned long flags, > - struct npu_context *(*cb)(struct npu_context *, void *), > + void (*cb)(struct npu_context *, void *), > void *priv) > { > int rc; > @@ -763,8 +763,18 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, > */ > spin_lock(&npu_context_lock); > npu_context = mm->context.npu_context; > - if (npu_context) > + if (npu_context) { > + if (npu_context->release_cb != cb || > + npu_context->priv != priv) { > + spin_unlock(&npu_context_lock); > + opal_npu_destroy_context(nphb->opal_id, mm->context.id, > + PCI_DEVID(gpdev->bus->number, > + gpdev->devfn)); > + return ERR_PTR(-EINVAL); > + } > + > WARN_ON(!kref_get_unless_zero(&npu_context->kref)); > + } > spin_unlock(&npu_context_lock); > > if (!npu_context) { > -- > 2.11.0 > > Reviewed-by: Mark Hairgrove <mhairgrove@nvidia.com> Tested-by: Mark Hairgrove <mhairgrove@nvidia.com>
On Fri, Apr 13, 2018 at 12:02 PM, Mark Hairgrove <mhairgrove@nvidia.com> wrote: > > > On Wed, 11 Apr 2018, Alistair Popple wrote: > >> There is a single npu context per set of callback parameters. Callers >> should be prevented from overwriting existing callback values so instead >> return an error if different parameters are passed. >> >> Signed-off-by: Alistair Popple <alistair@popple.id.au> >> --- >> arch/powerpc/include/asm/powernv.h | 2 +- >> arch/powerpc/platforms/powernv/npu-dma.c | 16 +++++++++++++--- >> 2 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h >> index dc5f6a5d4575..362ea12a4501 100644 >> --- a/arch/powerpc/include/asm/powernv.h >> +++ b/arch/powerpc/include/asm/powernv.h >> @@ -15,7 +15,7 @@ >> extern void powernv_set_nmmu_ptcr(unsigned long ptcr); >> extern struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, >> unsigned long flags, >> - struct npu_context *(*cb)(struct npu_context *, void *), >> + void (*cb)(struct npu_context *, void *), >> void *priv); >> extern void pnv_npu2_destroy_context(struct npu_context *context, >> struct pci_dev *gpdev); >> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c >> index cb77162f4e7a..193f43ea3fbc 100644 >> --- a/arch/powerpc/platforms/powernv/npu-dma.c >> +++ b/arch/powerpc/platforms/powernv/npu-dma.c >> @@ -407,7 +407,7 @@ struct npu_context { >> bool nmmu_flush; >> >> /* Callback to stop translation requests on a given GPU */ >> - struct npu_context *(*release_cb)(struct npu_context *, void *); >> + void (*release_cb)(struct npu_context *context, void *priv); >> >> /* >> * Private pointer passed to the above callback for usage by >> @@ -705,7 +705,7 @@ static const struct mmu_notifier_ops nv_nmmu_notifier_ops = { >> */ >> struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, >> unsigned long flags, >> - struct npu_context *(*cb)(struct npu_context *, void *), >> + void (*cb)(struct npu_context *, void *), >> void *priv) >> { >> int rc; >> @@ -763,8 +763,18 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, >> */ >> spin_lock(&npu_context_lock); >> npu_context = mm->context.npu_context; >> - if (npu_context) >> + if (npu_context) { >> + if (npu_context->release_cb != cb || >> + npu_context->priv != priv) { >> + spin_unlock(&npu_context_lock); >> + opal_npu_destroy_context(nphb->opal_id, mm->context.id, >> + PCI_DEVID(gpdev->bus->number, >> + gpdev->devfn)); >> + return ERR_PTR(-EINVAL); >> + } >> + >> WARN_ON(!kref_get_unless_zero(&npu_context->kref)); >> + } >> spin_unlock(&npu_context_lock); >> >> if (!npu_context) { >> -- >> 2.11.0 >> >> > > Reviewed-by: Mark Hairgrove <mhairgrove@nvidia.com> > Tested-by: Mark Hairgrove <mhairgrove@nvidia.com> > Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Sorry, forgot to include:
Fixes: 1ab66d1fbada ("powerpc/powernv: Introduce address translation services for Nvlink2")
Thanks
On Wednesday, 11 April 2018 4:38:55 PM AEST Alistair Popple wrote:
> There is a single npu context per set of callback parameters. Callers
> should be prevented from overwriting existing callback values so instead
> return an error if different parameters are passed.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
> arch/powerpc/include/asm/powernv.h | 2 +-
> arch/powerpc/platforms/powernv/npu-dma.c | 16 +++++++++++++---
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h
> index dc5f6a5d4575..362ea12a4501 100644
> --- a/arch/powerpc/include/asm/powernv.h
> +++ b/arch/powerpc/include/asm/powernv.h
> @@ -15,7 +15,7 @@
> extern void powernv_set_nmmu_ptcr(unsigned long ptcr);
> extern struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
> unsigned long flags,
> - struct npu_context *(*cb)(struct npu_context *, void *),
> + void (*cb)(struct npu_context *, void *),
> void *priv);
> extern void pnv_npu2_destroy_context(struct npu_context *context,
> struct pci_dev *gpdev);
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index cb77162f4e7a..193f43ea3fbc 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -407,7 +407,7 @@ struct npu_context {
> bool nmmu_flush;
>
> /* Callback to stop translation requests on a given GPU */
> - struct npu_context *(*release_cb)(struct npu_context *, void *);
> + void (*release_cb)(struct npu_context *context, void *priv);
>
> /*
> * Private pointer passed to the above callback for usage by
> @@ -705,7 +705,7 @@ static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {
> */
> struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
> unsigned long flags,
> - struct npu_context *(*cb)(struct npu_context *, void *),
> + void (*cb)(struct npu_context *, void *),
> void *priv)
> {
> int rc;
> @@ -763,8 +763,18 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
> */
> spin_lock(&npu_context_lock);
> npu_context = mm->context.npu_context;
> - if (npu_context)
> + if (npu_context) {
> + if (npu_context->release_cb != cb ||
> + npu_context->priv != priv) {
> + spin_unlock(&npu_context_lock);
> + opal_npu_destroy_context(nphb->opal_id, mm->context.id,
> + PCI_DEVID(gpdev->bus->number,
> + gpdev->devfn));
> + return ERR_PTR(-EINVAL);
> + }
> +
> WARN_ON(!kref_get_unless_zero(&npu_context->kref));
> + }
> spin_unlock(&npu_context_lock);
>
> if (!npu_context) {
>
diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h index dc5f6a5d4575..362ea12a4501 100644 --- a/arch/powerpc/include/asm/powernv.h +++ b/arch/powerpc/include/asm/powernv.h @@ -15,7 +15,7 @@ extern void powernv_set_nmmu_ptcr(unsigned long ptcr); extern struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, unsigned long flags, - struct npu_context *(*cb)(struct npu_context *, void *), + void (*cb)(struct npu_context *, void *), void *priv); extern void pnv_npu2_destroy_context(struct npu_context *context, struct pci_dev *gpdev); diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index cb77162f4e7a..193f43ea3fbc 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -407,7 +407,7 @@ struct npu_context { bool nmmu_flush; /* Callback to stop translation requests on a given GPU */ - struct npu_context *(*release_cb)(struct npu_context *, void *); + void (*release_cb)(struct npu_context *context, void *priv); /* * Private pointer passed to the above callback for usage by @@ -705,7 +705,7 @@ static const struct mmu_notifier_ops nv_nmmu_notifier_ops = { */ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, unsigned long flags, - struct npu_context *(*cb)(struct npu_context *, void *), + void (*cb)(struct npu_context *, void *), void *priv) { int rc; @@ -763,8 +763,18 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, */ spin_lock(&npu_context_lock); npu_context = mm->context.npu_context; - if (npu_context) + if (npu_context) { + if (npu_context->release_cb != cb || + npu_context->priv != priv) { + spin_unlock(&npu_context_lock); + opal_npu_destroy_context(nphb->opal_id, mm->context.id, + PCI_DEVID(gpdev->bus->number, + gpdev->devfn)); + return ERR_PTR(-EINVAL); + } + WARN_ON(!kref_get_unless_zero(&npu_context->kref)); + } spin_unlock(&npu_context_lock); if (!npu_context) {
There is a single npu context per set of callback parameters. Callers should be prevented from overwriting existing callback values so instead return an error if different parameters are passed. Signed-off-by: Alistair Popple <alistair@popple.id.au> --- arch/powerpc/include/asm/powernv.h | 2 +- arch/powerpc/platforms/powernv/npu-dma.c | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-)