From: Gary R Hook Subject: Re: [PATCH v3 RESEND 3/5] crypto: cpp - Abstract interrupt registeration Date: Mon, 3 Jul 2017 10:50:51 -0500 Message-ID: References: <20170629165406.13463-1-brijesh.singh@amd.com> <20170629165406.13463-4-brijesh.singh@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: "herbert@gondor.apana.org.au" , "davem@davemloft.net" To: "Singh, Brijesh" , "linux-crypto@vger.kernel.org" , "Lendacky, Thomas" Return-path: Received: from mail-by2nam03on0064.outbound.protection.outlook.com ([104.47.42.64]:1897 "EHLO NAM03-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752639AbdGCPvN (ORCPT ); Mon, 3 Jul 2017 11:51:13 -0400 In-Reply-To: <20170629165406.13463-4-brijesh.singh@amd.com> Sender: linux-crypto-owner@vger.kernel.org List-ID: On 06/29/2017 11:54 AM, Singh, Brijesh wrote: > The CCP and PSP devices part of AMD Secure Procesor may share the same > interrupt. Hence we expand the SP device to register a common interrupt > handler and provide functions to CCP and PSP devices to register their > interrupt callback which will be invoked upon interrupt. > > Signed-off-by: Brijesh Singh Acked-by: Gary R Hook > --- > drivers/crypto/ccp/ccp-dev-v3.c | 6 +-- > drivers/crypto/ccp/ccp-dev-v5.c | 7 ++- > drivers/crypto/ccp/ccp-dev.c | 3 +- > drivers/crypto/ccp/ccp-dev.h | 2 - > drivers/crypto/ccp/ccp-pci.c | 103 > +++++++++++------------------------- > drivers/crypto/ccp/ccp-platform.c | 57 ++++++++++---------- > drivers/crypto/ccp/sp-dev.c | 107 > ++++++++++++++++++++++++++++++++++++++ > drivers/crypto/ccp/sp-dev.h | 16 +++++- > 8 files changed, 186 insertions(+), 115 deletions(-) > > diff --git a/drivers/crypto/ccp/ccp-dev-v3.c > b/drivers/crypto/ccp/ccp-dev-v3.c > index 57179034..695fde8 100644 > --- a/drivers/crypto/ccp/ccp-dev-v3.c > +++ b/drivers/crypto/ccp/ccp-dev-v3.c > @@ -453,7 +453,7 @@ static int ccp_init(struct ccp_device *ccp) > iowrite32(ccp->qim, ccp->io_regs + IRQ_STATUS_REG); > > /* Request an irq */ > - ret = ccp->get_irq(ccp); > + ret = sp_request_ccp_irq(ccp->sp, ccp_irq_handler, ccp->name, ccp); > if (ret) { > dev_err(dev, "unable to allocate an IRQ\n"); > goto e_pool; > @@ -510,7 +510,7 @@ static int ccp_init(struct ccp_device *ccp) > if (ccp->cmd_q[i].kthread) > kthread_stop(ccp->cmd_q[i].kthread); > > - ccp->free_irq(ccp); > + sp_free_ccp_irq(ccp->sp, ccp); > > e_pool: > for (i = 0; i < ccp->cmd_q_count; i++) > @@ -549,7 +549,7 @@ static void ccp_destroy(struct ccp_device *ccp) > if (ccp->cmd_q[i].kthread) > kthread_stop(ccp->cmd_q[i].kthread); > > - ccp->free_irq(ccp); > + sp_free_ccp_irq(ccp->sp, ccp); > > for (i = 0; i < ccp->cmd_q_count; i++) > dma_pool_destroy(ccp->cmd_q[i].dma_pool); > diff --git a/drivers/crypto/ccp/ccp-dev-v5.c > b/drivers/crypto/ccp/ccp-dev-v5.c > index 8ed2b37..b0391f0 100644 > --- a/drivers/crypto/ccp/ccp-dev-v5.c > +++ b/drivers/crypto/ccp/ccp-dev-v5.c > @@ -880,7 +880,7 @@ static int ccp5_init(struct ccp_device *ccp) > > dev_dbg(dev, "Requesting an IRQ...\n"); > /* Request an irq */ > - ret = ccp->get_irq(ccp); > + ret = sp_request_ccp_irq(ccp->sp, ccp5_irq_handler, ccp->name, ccp); > if (ret) { > dev_err(dev, "unable to allocate an IRQ\n"); > goto e_pool; > @@ -986,7 +986,7 @@ static int ccp5_init(struct ccp_device *ccp) > kthread_stop(ccp->cmd_q[i].kthread); > > e_irq: > - ccp->free_irq(ccp); > + sp_free_ccp_irq(ccp->sp, ccp); > > e_pool: > for (i = 0; i < ccp->cmd_q_count; i++) > @@ -1036,7 +1036,7 @@ static void ccp5_destroy(struct ccp_device *ccp) > if (ccp->cmd_q[i].kthread) > kthread_stop(ccp->cmd_q[i].kthread); > > - ccp->free_irq(ccp); > + sp_free_ccp_irq(ccp->sp, ccp); > > for (i = 0; i < ccp->cmd_q_count; i++) { > cmd_q = &ccp->cmd_q[i]; > @@ -1105,7 +1105,6 @@ static const struct ccp_actions ccp5_actions = { > .init = ccp5_init, > .destroy = ccp5_destroy, > .get_free_slots = ccp5_get_free_slots, > - .irqhandler = ccp5_irq_handler, > }; > > const struct ccp_vdata ccpv5a = { > diff --git a/drivers/crypto/ccp/ccp-dev.c b/drivers/crypto/ccp/ccp-dev.c > index 8a1674a..7c751bf 100644 > --- a/drivers/crypto/ccp/ccp-dev.c > +++ b/drivers/crypto/ccp/ccp-dev.c > @@ -599,8 +599,7 @@ int ccp_dev_init(struct sp_device *sp) > goto e_err; > } > > - ccp->get_irq = sp->get_irq; > - ccp->free_irq = sp->free_irq; > + ccp->use_tasklet = sp->use_tasklet; > > ccp->io_regs = sp->io_map + ccp->vdata->offset; > if (ccp->vdata->setup) > diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h > index ca44821..193f309 100644 > --- a/drivers/crypto/ccp/ccp-dev.h > +++ b/drivers/crypto/ccp/ccp-dev.h > @@ -351,8 +351,6 @@ struct ccp_device { > /* Bus specific device information > */ > void *dev_specific; > - int (*get_irq)(struct ccp_device *ccp); > - void (*free_irq)(struct ccp_device *ccp); > unsigned int qim; > unsigned int irq; > bool use_tasklet; > diff --git a/drivers/crypto/ccp/ccp-pci.c b/drivers/crypto/ccp/ccp-pci.c > index ab2df96..b29a093 100644 > --- a/drivers/crypto/ccp/ccp-pci.c > +++ b/drivers/crypto/ccp/ccp-pci.c > @@ -28,67 +28,37 @@ > > #define MSIX_VECTORS 2 > > -struct ccp_msix { > - u32 vector; > - char name[16]; > -}; > - > struct ccp_pci { > int msix_count; > - struct ccp_msix msix[MSIX_VECTORS]; > + struct msix_entry msix_entry[MSIX_VECTORS]; > }; > > -static int ccp_get_msix_irqs(struct ccp_device *ccp) > +static int ccp_get_msix_irqs(struct sp_device *sp) > { > - struct sp_device *sp = ccp->sp; > struct ccp_pci *ccp_pci = sp->dev_specific; > - struct device *dev = ccp->dev; > + struct device *dev = sp->dev; > struct pci_dev *pdev = to_pci_dev(dev); > - struct msix_entry msix_entry[MSIX_VECTORS]; > - unsigned int name_len = sizeof(ccp_pci->msix[0].name) - 1; > int v, ret; > > - for (v = 0; v < ARRAY_SIZE(msix_entry); v++) > - msix_entry[v].entry = v; > + for (v = 0; v < ARRAY_SIZE(ccp_pci->msix_entry); v++) > + ccp_pci->msix_entry[v].entry = v; > > - ret = pci_enable_msix_range(pdev, msix_entry, 1, v); > + ret = pci_enable_msix_range(pdev, ccp_pci->msix_entry, 1, v); > if (ret < 0) > return ret; > > ccp_pci->msix_count = ret; > - for (v = 0; v < ccp_pci->msix_count; v++) { > - /* Set the interrupt names and request the irqs */ > - snprintf(ccp_pci->msix[v].name, name_len, "%s-%u", > - sp->name, v); > - ccp_pci->msix[v].vector = msix_entry[v].vector; > - ret = request_irq(ccp_pci->msix[v].vector, > - ccp->vdata->perform->irqhandler, > - 0, ccp_pci->msix[v].name, ccp); > - if (ret) { > - dev_notice(dev, "unable to allocate MSI-X IRQ > (%d)\n", > - ret); > - goto e_irq; > - } > - } > - ccp->use_tasklet = true; > + sp->use_tasklet = true; > > + sp->psp_irq = ccp_pci->msix_entry[0].vector; > + sp->ccp_irq = (ccp_pci->msix_count > 1) ? > ccp_pci->msix_entry[1].vector > + : > ccp_pci->msix_entry[0].vector; > return 0; > - > -e_irq: > - while (v--) > - free_irq(ccp_pci->msix[v].vector, dev); > - > - pci_disable_msix(pdev); > - > - ccp_pci->msix_count = 0; > - > - return ret; > } > > -static int ccp_get_msi_irq(struct ccp_device *ccp) > +static int ccp_get_msi_irq(struct sp_device *sp) > { > - struct sp_device *sp = ccp->sp; > - struct device *dev = ccp->dev; > + struct device *dev = sp->dev; > struct pci_dev *pdev = to_pci_dev(dev); > int ret; > > @@ -96,35 +66,24 @@ static int ccp_get_msi_irq(struct ccp_device *ccp) > if (ret) > return ret; > > - ccp->irq = pdev->irq; > - ret = request_irq(ccp->irq, ccp->vdata->perform->irqhandler, 0, > - sp->name, ccp); > - if (ret) { > - dev_notice(dev, "unable to allocate MSI IRQ (%d)\n", ret); > - goto e_msi; > - } > - ccp->use_tasklet = true; > + sp->ccp_irq = pdev->irq; > + sp->psp_irq = pdev->irq; > > return 0; > - > -e_msi: > - pci_disable_msi(pdev); > - > - return ret; > } > > -static int ccp_get_irqs(struct ccp_device *ccp) > +static int ccp_get_irqs(struct sp_device *sp) > { > - struct device *dev = ccp->dev; > + struct device *dev = sp->dev; > int ret; > > - ret = ccp_get_msix_irqs(ccp); > + ret = ccp_get_msix_irqs(sp); > if (!ret) > return 0; > > /* Couldn't get MSI-X vectors, try MSI */ > dev_notice(dev, "could not enable MSI-X (%d), trying MSI\n", ret); > - ret = ccp_get_msi_irq(ccp); > + ret = ccp_get_msi_irq(sp); > if (!ret) > return 0; > > @@ -134,23 +93,19 @@ static int ccp_get_irqs(struct ccp_device *ccp) > return ret; > } > > -static void ccp_free_irqs(struct ccp_device *ccp) > +static void ccp_free_irqs(struct sp_device *sp) > { > - struct sp_device *sp = ccp->sp; > struct ccp_pci *ccp_pci = sp->dev_specific; > - struct device *dev = ccp->dev; > + struct device *dev = sp->dev; > struct pci_dev *pdev = to_pci_dev(dev); > > - if (ccp_pci->msix_count) { > - while (ccp_pci->msix_count--) > - free_irq(ccp_pci->msix[ccp_pci->msix_count].vector, > - ccp); > + if (ccp_pci->msix_count) > pci_disable_msix(pdev); > - } else if (ccp->irq) { > - free_irq(ccp->irq, ccp); > + else if (sp->psp_irq) > pci_disable_msi(pdev); > - } > - ccp->irq = 0; > + > + sp->ccp_irq = 0; > + sp->psp_irq = 0; > } > > static int ccp_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *id) > @@ -178,8 +133,6 @@ static int ccp_pci_probe(struct pci_dev *pdev, const > struct pci_device_id *id) > dev_err(dev, "missing driver data\n"); > goto e_err; > } > - sp->get_irq = ccp_get_irqs; > - sp->free_irq = ccp_free_irqs; > > ret = pcim_enable_device(pdev); > if (ret) { > @@ -208,6 +161,10 @@ static int ccp_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *id) > goto e_err; > } > > + ret = ccp_get_irqs(sp); > + if (ret) > + goto e_err; > + > pci_set_master(pdev); > > ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48)); > @@ -245,6 +202,8 @@ static void ccp_pci_remove(struct pci_dev *pdev) > > sp_destroy(sp); > > + ccp_free_irqs(sp); > + > dev_notice(dev, "disabled\n"); > } > > diff --git a/drivers/crypto/ccp/ccp-platform.c > b/drivers/crypto/ccp/ccp-platform.c > index b6bb22d..5228dd7 100644 > --- a/drivers/crypto/ccp/ccp-platform.c > +++ b/drivers/crypto/ccp/ccp-platform.c > @@ -30,6 +30,7 @@ > > struct ccp_platform { > int coherent; > + unsigned int irq_count; > }; > > static const struct acpi_device_id ccp_acpi_match[]; > @@ -59,45 +60,39 @@ static struct sp_dev_vdata > *ccp_get_acpi_version(struct platform_device *pdev) > return NULL; > } > > -static int ccp_get_irq(struct ccp_device *ccp) > +static int ccp_get_irqs(struct sp_device *sp) > { > - struct device *dev = ccp->dev; > + struct ccp_platform *ccp_platform = sp->dev_specific; > + struct device *dev = sp->dev; > struct platform_device *pdev = to_platform_device(dev); > + unsigned int i, count; > int ret; > > - ret = platform_get_irq(pdev, 0); > - if (ret < 0) > - return ret; > + for (i = 0, count = 0; i < pdev->num_resources; i++) { > + struct resource *res = &pdev->resource[i]; > > - ccp->irq = ret; > - ret = request_irq(ccp->irq, ccp->vdata->perform->irqhandler, 0, > - ccp->name, ccp); > - if (ret) { > - dev_notice(dev, "unable to allocate IRQ (%d)\n", ret); > - return ret; > + if (resource_type(res) == IORESOURCE_IRQ) > + count++; > } > > - return 0; > -} > + ccp_platform->irq_count = count; > > -static int ccp_get_irqs(struct ccp_device *ccp) > -{ > - struct device *dev = ccp->dev; > - int ret; > - > - ret = ccp_get_irq(ccp); > - if (!ret) > - return 0; > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + return ret; > > - /* Couldn't get an interrupt */ > - dev_notice(dev, "could not enable interrupts (%d)\n", ret); > + sp->psp_irq = ret; > + if (count == 1) { > + sp->ccp_irq = ret; > + } else { > + ret = platform_get_irq(pdev, 1); > + if (ret < 0) > + return ret; > > - return ret; > -} > + sp->ccp_irq = ret; > + } > > -static void ccp_free_irqs(struct ccp_device *ccp) > -{ > - free_irq(ccp->irq, ccp); > + return 0; > } > > static int ccp_platform_probe(struct platform_device *pdev) > @@ -126,8 +121,6 @@ static int ccp_platform_probe(struct platform_device > *pdev) > dev_err(dev, "missing driver data\n"); > goto e_err; > } > - sp->get_irq = ccp_get_irqs; > - sp->free_irq = ccp_free_irqs; > > ior = platform_get_resource(pdev, IORESOURCE_MEM, 0); > sp->io_map = devm_ioremap_resource(dev, ior); > @@ -154,6 +147,10 @@ static int ccp_platform_probe(struct > platform_device *pdev) > goto e_err; > } > > + ret = ccp_get_irqs(sp); > + if (ret) > + goto e_err; > + > dev_set_drvdata(dev, sp); > > ret = sp_init(sp); > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > index d1dda38..44e76e5 100644 > --- a/drivers/crypto/ccp/sp-dev.c > +++ b/drivers/crypto/ccp/sp-dev.c > @@ -64,6 +64,113 @@ static void sp_del_device(struct sp_device *sp) > write_unlock_irqrestore(&sp_unit_lock, flags); > } > > +static irqreturn_t sp_irq_handler(int irq, void *data) > +{ > + struct sp_device *sp = data; > + > + if (sp->ccp_irq_handler) > + sp->ccp_irq_handler(irq, sp->ccp_irq_data); > + > + if (sp->psp_irq_handler) > + sp->psp_irq_handler(irq, sp->psp_irq_data); > + > + return IRQ_HANDLED; > +} > + > +int sp_request_ccp_irq(struct sp_device *sp, irq_handler_t handler, > + const char *name, void *data) > +{ > + int ret; > + > + if ((sp->psp_irq == sp->ccp_irq) && sp->dev_vdata->psp_vdata) { > + /* Need a common routine to manage all interrupts */ > + sp->ccp_irq_data = data; > + sp->ccp_irq_handler = handler; > + > + if (!sp->irq_registered) { > + ret = request_irq(sp->ccp_irq, sp_irq_handler, 0, > + sp->name, sp); > + if (ret) > + return ret; > + > + sp->irq_registered = true; > + } > + } else { > + /* Each sub-device can manage it's own interrupt */ > + ret = request_irq(sp->ccp_irq, handler, 0, name, data); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +int sp_request_psp_irq(struct sp_device *sp, irq_handler_t handler, > + const char *name, void *data) > +{ > + int ret; > + > + if ((sp->psp_irq == sp->ccp_irq) && sp->dev_vdata->ccp_vdata) { > + /* Need a common routine to manage all interrupts */ > + sp->psp_irq_data = data; > + sp->psp_irq_handler = handler; > + > + if (!sp->irq_registered) { > + ret = request_irq(sp->psp_irq, sp_irq_handler, 0, > + sp->name, sp); > + if (ret) > + return ret; > + > + sp->irq_registered = true; > + } > + } else { > + /* Each sub-device can manage it's own interrupt */ > + ret = request_irq(sp->psp_irq, handler, 0, name, data); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +void sp_free_ccp_irq(struct sp_device *sp, void *data) > +{ > + if ((sp->psp_irq == sp->ccp_irq) && sp->dev_vdata->psp_vdata) { > + /* Using common routine to manage all interrupts */ > + if (!sp->psp_irq_handler) { > + /* Nothing else using it, so free it */ > + free_irq(sp->ccp_irq, sp); > + > + sp->irq_registered = false; > + } > + > + sp->ccp_irq_handler = NULL; > + sp->ccp_irq_data = NULL; > + } else { > + /* Each sub-device can manage it's own interrupt */ > + free_irq(sp->ccp_irq, data); > + } > +} > + > +void sp_free_psp_irq(struct sp_device *sp, void *data) > +{ > + if ((sp->psp_irq == sp->ccp_irq) && sp->dev_vdata->ccp_vdata) { > + /* Using common routine to manage all interrupts */ > + if (!sp->ccp_irq_handler) { > + /* Nothing else using it, so free it */ > + free_irq(sp->psp_irq, sp); > + > + sp->irq_registered = false; > + } > + > + sp->psp_irq_handler = NULL; > + sp->psp_irq_data = NULL; > + } else { > + /* Each sub-device can manage it's own interrupt */ > + free_irq(sp->psp_irq, data); > + } > +} > + > /** > * sp_alloc_struct - allocate and initialize the sp_device struct > * > diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h > index 189e57e..3520da4 100644 > --- a/drivers/crypto/ccp/sp-dev.h > +++ b/drivers/crypto/ccp/sp-dev.h > @@ -68,9 +68,15 @@ struct sp_device { > unsigned int axcache; > > bool irq_registered; > + bool use_tasklet; > > - int (*get_irq)(struct ccp_device *ccp); > - void (*free_irq)(struct ccp_device *ccp); > + unsigned int ccp_irq; > + irq_handler_t ccp_irq_handler; > + void *ccp_irq_data; > + > + unsigned int psp_irq; > + irq_handler_t psp_irq_handler; > + void *psp_irq_data; > > void *ccp_data; > void *psp_data; > @@ -90,6 +96,12 @@ struct sp_device *sp_get_master(void); > > int sp_suspend(struct sp_device *sp, pm_message_t state); > int sp_resume(struct sp_device *sp); > +int sp_request_ccp_irq(struct sp_device *sp, irq_handler_t handler, > + const char *name, void *data); > +void sp_free_ccp_irq(struct sp_device *sp, void *data); > +int sp_request_psp_irq(struct sp_device *sp, irq_handler_t handler, > + const char *name, void *data); > +void sp_free_psp_irq(struct sp_device *sp, void *data); > > #ifdef CONFIG_CRYPTO_DEV_SP_CCP > > -- > 2.9.4 >