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(-)
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;
}
--
2.43.0On 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().
© 2016 - 2025 Red Hat, Inc.