* set_cpus_allowed_ptr() usage in FREESCALE CAAM
@ 2018-10-05 12:54 Sebastian Andrzej Siewior
2018-10-08 11:09 ` [PATCH] crypto: caam/qi - simplify CGR allocation, freeing Horia Geantă
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-10-05 12:54 UTC (permalink / raw)
To: Horia Geantă
Cc: Aymen Sghaier, linux-crypto, linux-kernel, Peter Zijlstra,
Thomas Gleixner
Hi,
this block:
|int caam_qi_shutdown(struct device *qidev)
| {
| struct cpumask old_cpumask = current->cpus_allowed;
…
| /*
| * QMan driver requires CGRs to be deleted from same CPU from where they
| * were instantiated. Hence we get the module removal execute from the
| * same CPU from where it was originally inserted.
| */
| set_cpus_allowed_ptr(current, get_cpu_mask(mod_init_cpu));
…
| /* Now that we're done with the CGRs, restore the cpus allowed mask */
| set_cpus_allowed_ptr(current, &old_cpumask);
in drivers/crypto/caam/qi.c needs to go. I saw it twice in the driver.
set_cpus_allowed_ptr() is not intended for this kind of thing.
What you want is to use work_on_cpu_safe() instead. It takes also a CPU
as an argument. You need to check the error code of the function if it
worked because the CPU may have gone offline. This functions also
ensures that the CPU does not vanish in the middle of the work.
Also please check the error code in both cases of the function because
it may fail if the CPU is not online.
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] crypto: caam/qi - simplify CGR allocation, freeing 2018-10-05 12:54 set_cpus_allowed_ptr() usage in FREESCALE CAAM Sebastian Andrzej Siewior @ 2018-10-08 11:09 ` Horia Geantă 2018-10-09 17:11 ` Sebastian Andrzej Siewior 2018-10-17 6:21 ` Herbert Xu 0 siblings, 2 replies; 6+ messages in thread From: Horia Geantă @ 2018-10-08 11:09 UTC (permalink / raw) To: Herbert Xu Cc: Sebastian Andrzej Siewior, David S. Miller, Aymen Sghaier, Li Yang, Roy Pledge, Madalin Bucur, Peter Zijlstra, Thomas Gleixner, linux-crypto, linux-kernel CGRs (Congestion Groups) have to be freed by the same CPU that initialized them. This is why currently the driver takes special measures; however, using set_cpus_allowed_ptr() is incorrect - as reported by Sebastian. Instead of the generic solution of replacing set_cpus_allowed_ptr() with work_on_cpu_safe(), we use the qman_delete_cgr_safe() QBMan API instead of qman_delete_cgr() - which internally takes care of proper CGR deletion. Link: https://lkml.kernel.org/r/20181005125443.dfhd2asqktm22ney@linutronix.de Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Horia Geantă <horia.geanta@nxp.com> --- drivers/crypto/caam/qi.c | 43 ++++--------------------------------------- drivers/crypto/caam/qi.h | 2 +- 2 files changed, 5 insertions(+), 40 deletions(-) diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c index 67f7f8c42c93..b84e6c8b1e13 100644 --- a/drivers/crypto/caam/qi.c +++ b/drivers/crypto/caam/qi.c @@ -83,13 +83,6 @@ EXPORT_SYMBOL(caam_congested); static u64 times_congested; #endif -/* - * CPU from where the module initialised. This is required because QMan driver - * requires CGRs to be removed from same CPU from where they were originally - * allocated. - */ -static int mod_init_cpu; - /* * This is a a cache of buffers, from which the users of CAAM QI driver * can allocate short (CAAM_QI_MEMCACHE_SIZE) buffers. It's faster than @@ -492,12 +485,11 @@ void caam_drv_ctx_rel(struct caam_drv_ctx *drv_ctx) } EXPORT_SYMBOL(caam_drv_ctx_rel); -int caam_qi_shutdown(struct device *qidev) +void caam_qi_shutdown(struct device *qidev) { - int i, ret; + int i; struct caam_qi_priv *priv = dev_get_drvdata(qidev); const cpumask_t *cpus = qman_affine_cpus(); - struct cpumask old_cpumask = current->cpus_allowed; for_each_cpu(i, cpus) { struct napi_struct *irqtask; @@ -510,26 +502,12 @@ int caam_qi_shutdown(struct device *qidev) dev_err(qidev, "Rsp FQ kill failed, cpu: %d\n", i); } - /* - * QMan driver requires CGRs to be deleted from same CPU from where they - * were instantiated. Hence we get the module removal execute from the - * same CPU from where it was originally inserted. - */ - set_cpus_allowed_ptr(current, get_cpu_mask(mod_init_cpu)); - - ret = qman_delete_cgr(&priv->cgr); - if (ret) - dev_err(qidev, "Deletion of CGR failed: %d\n", ret); - else - qman_release_cgrid(priv->cgr.cgrid); + qman_delete_cgr_safe(&priv->cgr); + qman_release_cgrid(priv->cgr.cgrid); kmem_cache_destroy(qi_cache); - /* Now that we're done with the CGRs, restore the cpus allowed mask */ - set_cpus_allowed_ptr(current, &old_cpumask); - platform_device_unregister(priv->qi_pdev); - return ret; } static void cgr_cb(struct qman_portal *qm, struct qman_cgr *cgr, int congested) @@ -718,22 +696,11 @@ int caam_qi_init(struct platform_device *caam_pdev) struct device *ctrldev = &caam_pdev->dev, *qidev; struct caam_drv_private *ctrlpriv; const cpumask_t *cpus = qman_affine_cpus(); - struct cpumask old_cpumask = current->cpus_allowed; static struct platform_device_info qi_pdev_info = { .name = "caam_qi", .id = PLATFORM_DEVID_NONE }; - /* - * QMAN requires CGRs to be removed from same CPU+portal from where it - * was originally allocated. Hence we need to note down the - * initialisation CPU and use the same CPU for module exit. - * We select the first CPU to from the list of portal owning CPUs. - * Then we pin module init to this CPU. - */ - mod_init_cpu = cpumask_first(cpus); - set_cpus_allowed_ptr(current, get_cpu_mask(mod_init_cpu)); - qi_pdev_info.parent = ctrldev; qi_pdev_info.dma_mask = dma_get_mask(ctrldev); qi_pdev = platform_device_register_full(&qi_pdev_info); @@ -795,8 +762,6 @@ int caam_qi_init(struct platform_device *caam_pdev) return -ENOMEM; } - /* Done with the CGRs; restore the cpus allowed mask */ - set_cpus_allowed_ptr(current, &old_cpumask); #ifdef CONFIG_DEBUG_FS debugfs_create_file("qi_congested", 0444, ctrlpriv->ctl, ×_congested, &caam_fops_u64_ro); diff --git a/drivers/crypto/caam/qi.h b/drivers/crypto/caam/qi.h index bc421da5ad8b..f93c9c7ed430 100644 --- a/drivers/crypto/caam/qi.h +++ b/drivers/crypto/caam/qi.h @@ -173,7 +173,7 @@ int caam_drv_ctx_update(struct caam_drv_ctx *drv_ctx, u32 *sh_desc); void caam_drv_ctx_rel(struct caam_drv_ctx *drv_ctx); int caam_qi_init(struct platform_device *pdev); -int caam_qi_shutdown(struct device *dev); +void caam_qi_shutdown(struct device *dev); /** * qi_cache_alloc - Allocate buffers from CAAM-QI cache -- 2.16.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: caam/qi - simplify CGR allocation, freeing 2018-10-08 11:09 ` [PATCH] crypto: caam/qi - simplify CGR allocation, freeing Horia Geantă @ 2018-10-09 17:11 ` Sebastian Andrzej Siewior 2018-10-25 14:05 ` Horia Geanta 2018-10-17 6:21 ` Herbert Xu 1 sibling, 1 reply; 6+ messages in thread From: Sebastian Andrzej Siewior @ 2018-10-09 17:11 UTC (permalink / raw) To: Horia Geantă Cc: Herbert Xu, David S. Miller, Aymen Sghaier, Li Yang, Roy Pledge, Madalin Bucur, Peter Zijlstra, Thomas Gleixner, linux-crypto, linux-kernel On 2018-10-08 14:09:37 [+0300], Horia Geantă wrote: > CGRs (Congestion Groups) have to be freed by the same CPU that > initialized them. > This is why currently the driver takes special measures; however, using > set_cpus_allowed_ptr() is incorrect - as reported by Sebastian. > > Instead of the generic solution of replacing set_cpus_allowed_ptr() with > work_on_cpu_safe(), we use the qman_delete_cgr_safe() QBMan API instead > of qman_delete_cgr() - which internally takes care of proper CGR > deletion. > > Link: https://lkml.kernel.org/r/20181005125443.dfhd2asqktm22ney@linutronix.de > Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> Oh. No more usage of set_cpus_allowed_ptr(). Wonderful. Thank you. Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> for that. Now that you shifted my attention to qman_delete_cgr_safe(). Could you please use work_on_cpu_safe() here instead smp_call_function_single() with preempt_disable() around it? Now, what is the problem with the CPU limitation? Is this a HW limitation that you can access the registers from a certain CPU? This still fails (silently) if the CPU is missing, right? If you can't get around it, you could block the CPU from going offline. You could register a HP notifier cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, … and the function would return -EINVAL if this is the special CPU. The other thing would be forbid rmmod. This *could* work but if I remember correctly, an explicit unbind can't be stopped. Sebastian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: caam/qi - simplify CGR allocation, freeing 2018-10-09 17:11 ` Sebastian Andrzej Siewior @ 2018-10-25 14:05 ` Horia Geanta 2018-10-29 8:56 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 6+ messages in thread From: Horia Geanta @ 2018-10-25 14:05 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Roy Pledge Cc: Herbert Xu, David S. Miller, Aymen Sghaier, Leo Li, Madalin-cristian Bucur, Peter Zijlstra, Thomas Gleixner, linux-crypto, linux-kernel On 10/9/2018 8:11 PM, Sebastian Andrzej Siewior wrote: > On 2018-10-08 14:09:37 [+0300], Horia Geantă wrote: >> CGRs (Congestion Groups) have to be freed by the same CPU that >> initialized them. >> This is why currently the driver takes special measures; however, using >> set_cpus_allowed_ptr() is incorrect - as reported by Sebastian. >> >> Instead of the generic solution of replacing set_cpus_allowed_ptr() with >> work_on_cpu_safe(), we use the qman_delete_cgr_safe() QBMan API instead >> of qman_delete_cgr() - which internally takes care of proper CGR >> deletion. >> >> Link: https://lkml.kernel.org/r/20181005125443.dfhd2asqktm22ney@linutronix.de >> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > > Oh. No more usage of set_cpus_allowed_ptr(). Wonderful. Thank you. > Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > for that. > > Now that you shifted my attention to qman_delete_cgr_safe(). > Could you please use work_on_cpu_safe() here instead > smp_call_function_single() with preempt_disable() around it? > > Now, what is the problem with the CPU limitation? Is this a HW > limitation that you can access the registers from a certain CPU? > Roy confirmed the CPU limitation should actually be removed, there is nothing in HW requiring it. A clean-up patch will follow. Thanks, Horia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: caam/qi - simplify CGR allocation, freeing 2018-10-25 14:05 ` Horia Geanta @ 2018-10-29 8:56 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 6+ messages in thread From: Sebastian Andrzej Siewior @ 2018-10-29 8:56 UTC (permalink / raw) To: Horia Geanta Cc: Roy Pledge, Herbert Xu, David S. Miller, Aymen Sghaier, Leo Li, Madalin-cristian Bucur, Peter Zijlstra, Thomas Gleixner, linux-crypto, linux-kernel On 2018-10-25 14:05:32 [+0000], Horia Geanta wrote: > > Now, what is the problem with the CPU limitation? Is this a HW > > limitation that you can access the registers from a certain CPU? > > > Roy confirmed the CPU limitation should actually be removed, there is nothing in > HW requiring it. > A clean-up patch will follow. good. Thank you. > Thanks, > Horia Sebastian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: caam/qi - simplify CGR allocation, freeing 2018-10-08 11:09 ` [PATCH] crypto: caam/qi - simplify CGR allocation, freeing Horia Geantă 2018-10-09 17:11 ` Sebastian Andrzej Siewior @ 2018-10-17 6:21 ` Herbert Xu 1 sibling, 0 replies; 6+ messages in thread From: Herbert Xu @ 2018-10-17 6:21 UTC (permalink / raw) To: Horia Geantă Cc: Sebastian Andrzej Siewior, David S. Miller, Aymen Sghaier, Li Yang, Roy Pledge, Madalin Bucur, Peter Zijlstra, Thomas Gleixner, linux-crypto, linux-kernel On Mon, Oct 08, 2018 at 02:09:37PM +0300, Horia Geantă wrote: > CGRs (Congestion Groups) have to be freed by the same CPU that > initialized them. > This is why currently the driver takes special measures; however, using > set_cpus_allowed_ptr() is incorrect - as reported by Sebastian. > > Instead of the generic solution of replacing set_cpus_allowed_ptr() with > work_on_cpu_safe(), we use the qman_delete_cgr_safe() QBMan API instead > of qman_delete_cgr() - which internally takes care of proper CGR > deletion. > > Link: https://lkml.kernel.org/r/20181005125443.dfhd2asqktm22ney@linutronix.de > Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > --- > drivers/crypto/caam/qi.c | 43 ++++--------------------------------------- > drivers/crypto/caam/qi.h | 2 +- > 2 files changed, 5 insertions(+), 40 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-29 17:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-05 12:54 set_cpus_allowed_ptr() usage in FREESCALE CAAM Sebastian Andrzej Siewior 2018-10-08 11:09 ` [PATCH] crypto: caam/qi - simplify CGR allocation, freeing Horia Geantă 2018-10-09 17:11 ` Sebastian Andrzej Siewior 2018-10-25 14:05 ` Horia Geanta 2018-10-29 8:56 ` Sebastian Andrzej Siewior 2018-10-17 6:21 ` Herbert Xu
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.