| Message ID | 20240702185557.3699991-4-leitao@debian.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: caam: Unembed net_dev | expand |
On Tue, 2 Jul 2024 11:55:53 -0700 Breno Leitao wrote: > +static void free_caam_qi_pcpu_netdev(const cpumask_t *cpus) > +{ > + struct caam_qi_pcpu_priv *priv; > + int i; > + > + for_each_cpu(i, cpus) { > + priv = per_cpu_ptr(&pcpu_qipriv, i); > + free_netdev(priv->net_dev); > + } > +} > + > int caam_qi_init(struct platform_device *caam_pdev) > { > int err, i; > struct device *ctrldev = &caam_pdev->dev, *qidev; > struct caam_drv_private *ctrlpriv; > const cpumask_t *cpus = qman_affine_cpus(); > + cpumask_t clean_mask; > > ctrlpriv = dev_get_drvdata(ctrldev); > qidev = ctrldev; > @@ -743,6 +756,8 @@ int caam_qi_init(struct platform_device *caam_pdev) > return err; > } > > + cpumask_clear(&clean_mask); > + > /* > * Enable the NAPI contexts on each of the core which has an affine > * portal. > @@ -751,10 +766,16 @@ int caam_qi_init(struct platform_device *caam_pdev) > struct caam_qi_pcpu_priv *priv = per_cpu_ptr(&pcpu_qipriv, i); > struct caam_napi *caam_napi = &priv->caam_napi; > struct napi_struct *irqtask = &caam_napi->irqtask; > - struct net_device *net_dev = &priv->net_dev; > + struct net_device *net_dev; > > + net_dev = alloc_netdev_dummy(0); > + if (!net_dev) { > + err = -ENOMEM; > + goto fail; free_netdev() doesn't take NULL, free_caam_qi_pcpu_netdev() will feed it one if we fail here > + } > + cpumask_set_cpu(i, &clean_mask); > + priv->net_dev = net_dev; > net_dev->dev = *qidev; > - INIT_LIST_HEAD(&net_dev->napi_list); > > netif_napi_add_tx_weight(net_dev, irqtask, caam_qi_poll, > CAAM_NAPI_WEIGHT); > @@ -766,16 +787,22 @@ int caam_qi_init(struct platform_device *caam_pdev) > dma_get_cache_alignment(), 0, NULL); > if (!qi_cache) { > dev_err(qidev, "Can't allocate CAAM cache\n"); > - free_rsp_fqs(); > - return -ENOMEM; > + err = -ENOMEM; > + goto fail2; > } > > caam_debugfs_qi_init(ctrlpriv); > > err = devm_add_action_or_reset(qidev, caam_qi_shutdown, ctrlpriv); > if (err) > - return err; > + goto fail2; > > dev_info(qidev, "Linux CAAM Queue I/F driver initialised\n"); > return 0; > + > +fail2: > + free_rsp_fqs(); > +fail: > + free_caam_qi_pcpu_netdev(&clean_mask);
Hello Jakub, On Wed, Jul 03, 2024 at 07:45:33PM -0700, Jakub Kicinski wrote: > On Tue, 2 Jul 2024 11:55:53 -0700 Breno Leitao wrote: > > @@ -751,10 +766,16 @@ int caam_qi_init(struct platform_device *caam_pdev) > > struct caam_qi_pcpu_priv *priv = per_cpu_ptr(&pcpu_qipriv, i); > > struct caam_napi *caam_napi = &priv->caam_napi; > > struct napi_struct *irqtask = &caam_napi->irqtask; > > - struct net_device *net_dev = &priv->net_dev; > > + struct net_device *net_dev; > > > > + net_dev = alloc_netdev_dummy(0); > > + if (!net_dev) { > > + err = -ENOMEM; > > + goto fail; > > free_netdev() doesn't take NULL, free_caam_qi_pcpu_netdev() > will feed it one if we fail here Sorry, I am not sure I followed you. Let me ask a clarifying questions: Do you think that free_netdev() will take NULL ? If that is the case, that *shouldn't* happen, since I have a cpumask that tracks the percpu netdev that got allocated, and only free those percpu-net_device that was properly allocated. Let me simplify the code to make it easy to understand what I had in mind: int caam_qi_init(struct platform_device *caam_pdev) { cpumask_clear(&clean_mask); net_dev = alloc_netdev_dummy(0); if (!net_dev) goto fail; cpumask_set_cpu(i, &clean_mask); fail: free_caam_qi_pcpu_netdev(&clean_mask); } static void free_caam_qi_pcpu_netdev(const cpumask_t *cpus) { for_each_cpu(i, cpus) { priv = per_cpu_ptr(&pcpu_qipriv, i); free_netdev(priv->net_dev); } } So, if alloc_netdev_dummy() fails, then the cpu current cpu will not be set in `clean_mask`, thus, free_caam_qi_pcpu_netdev() will not free it later. Anyway, let me know if I am missing something.
On Thu, 4 Jul 2024 09:15:29 -0700 Breno Leitao wrote: > So, if alloc_netdev_dummy() fails, then the cpu current cpu will not be > set in `clean_mask`, thus, free_caam_qi_pcpu_netdev() will not free it > later. Ah, sorry, I missed the clean mask, my eyes must have pattern matched the loop as for_each_cpu().
diff --git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c index 46a083849a8e..ba8fb5d8a7b2 100644 --- a/drivers/crypto/caam/qi.c +++ b/drivers/crypto/caam/qi.c @@ -57,7 +57,7 @@ struct caam_napi { */ struct caam_qi_pcpu_priv { struct caam_napi caam_napi; - struct net_device net_dev; + struct net_device *net_dev; struct qman_fq *rsp_fq; } ____cacheline_aligned; @@ -144,7 +144,7 @@ static void caam_fq_ern_cb(struct qman_portal *qm, struct qman_fq *fq, { const struct qm_fd *fd; struct caam_drv_req *drv_req; - struct device *qidev = &(raw_cpu_ptr(&pcpu_qipriv)->net_dev.dev); + struct device *qidev = &(raw_cpu_ptr(&pcpu_qipriv)->net_dev->dev); struct caam_drv_private *priv = dev_get_drvdata(qidev); fd = &msg->ern.fd; @@ -530,6 +530,7 @@ static void caam_qi_shutdown(void *data) if (kill_fq(qidev, per_cpu(pcpu_qipriv.rsp_fq, i))) dev_err(qidev, "Rsp FQ kill failed, cpu: %d\n", i); + free_netdev(per_cpu(pcpu_qipriv.net_dev, i)); } qman_delete_cgr_safe(&priv->cgr); @@ -573,7 +574,7 @@ static enum qman_cb_dqrr_result caam_rsp_fq_dqrr_cb(struct qman_portal *p, struct caam_napi *caam_napi = raw_cpu_ptr(&pcpu_qipriv.caam_napi); struct caam_drv_req *drv_req; const struct qm_fd *fd; - struct device *qidev = &(raw_cpu_ptr(&pcpu_qipriv)->net_dev.dev); + struct device *qidev = &(raw_cpu_ptr(&pcpu_qipriv)->net_dev->dev); struct caam_drv_private *priv = dev_get_drvdata(qidev); u32 status; @@ -718,12 +719,24 @@ static void free_rsp_fqs(void) kfree(per_cpu(pcpu_qipriv.rsp_fq, i)); } +static void free_caam_qi_pcpu_netdev(const cpumask_t *cpus) +{ + struct caam_qi_pcpu_priv *priv; + int i; + + for_each_cpu(i, cpus) { + priv = per_cpu_ptr(&pcpu_qipriv, i); + free_netdev(priv->net_dev); + } +} + int caam_qi_init(struct platform_device *caam_pdev) { int err, i; struct device *ctrldev = &caam_pdev->dev, *qidev; struct caam_drv_private *ctrlpriv; const cpumask_t *cpus = qman_affine_cpus(); + cpumask_t clean_mask; ctrlpriv = dev_get_drvdata(ctrldev); qidev = ctrldev; @@ -743,6 +756,8 @@ int caam_qi_init(struct platform_device *caam_pdev) return err; } + cpumask_clear(&clean_mask); + /* * Enable the NAPI contexts on each of the core which has an affine * portal. @@ -751,10 +766,16 @@ int caam_qi_init(struct platform_device *caam_pdev) struct caam_qi_pcpu_priv *priv = per_cpu_ptr(&pcpu_qipriv, i); struct caam_napi *caam_napi = &priv->caam_napi; struct napi_struct *irqtask = &caam_napi->irqtask; - struct net_device *net_dev = &priv->net_dev; + struct net_device *net_dev; + net_dev = alloc_netdev_dummy(0); + if (!net_dev) { + err = -ENOMEM; + goto fail; + } + cpumask_set_cpu(i, &clean_mask); + priv->net_dev = net_dev; net_dev->dev = *qidev; - INIT_LIST_HEAD(&net_dev->napi_list); netif_napi_add_tx_weight(net_dev, irqtask, caam_qi_poll, CAAM_NAPI_WEIGHT); @@ -766,16 +787,22 @@ int caam_qi_init(struct platform_device *caam_pdev) dma_get_cache_alignment(), 0, NULL); if (!qi_cache) { dev_err(qidev, "Can't allocate CAAM cache\n"); - free_rsp_fqs(); - return -ENOMEM; + err = -ENOMEM; + goto fail2; } caam_debugfs_qi_init(ctrlpriv); err = devm_add_action_or_reset(qidev, caam_qi_shutdown, ctrlpriv); if (err) - return err; + goto fail2; dev_info(qidev, "Linux CAAM Queue I/F driver initialised\n"); return 0; + +fail2: + free_rsp_fqs(); +fail: + free_caam_qi_pcpu_netdev(&clean_mask); + return err; }
Embedding net_device into structures prohibits the usage of flexible arrays in the net_device structure. For more details, see the discussion at [1]. Un-embed the net_devices from struct caam_qi_pcpu_priv by converting them into pointers, and allocating them dynamically. Use the leverage alloc_netdev_dummy() to allocate the net_device object at caam_qi_init(). The free of the device occurs at caam_qi_shutdown(). Link: https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/ [1] Signed-off-by: Breno Leitao <leitao@debian.org> --- drivers/crypto/caam/qi.c | 43 ++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-)