[PATCH net-next v3 0/3] crypto: caam: Unembed net_dev

[PATCH net-next v3 0/3] crypto: caam: Unembed net_dev

[Breno Leitao]

This will un-embed the net_device struct from inside other struct, so we
can add flexible array into net_device.

This also enable COMPILE test for FSL_CAAM, as any config option that
depends on ARCH_LAYERSCAPE.

Changelog:
v3:
	* Fix free_netdev() deference per-cpu (Simon)
	* Hide imx8m_machine_match under CONFIG_OF (Jakub)
v2:
        * added a cover letter (Jakub)
        * dropped the patch that makes FSL_DPAA dependent of
          COMPILE_TEST, since it exposes other problems.
v1:
        * https://lore.kernel.org/all/20240624162128.1665620-1-leitao@debian.org/

Breno Leitao (4):
  crypto: caam: Avoid unused variable
  crypto: caam: Make CRYPTO_DEV_FSL_CAAM dependent of COMPILE_TEST
  crypto: caam: Unembed net_dev structure from qi
  crypto: caam: Unembed net_dev structure in dpaa2

 drivers/crypto/caam/Kconfig       |  2 +-
 drivers/crypto/caam/caamalg_qi2.c | 28 +++++++++++++++++---
 drivers/crypto/caam/caamalg_qi2.h |  2 +-
 drivers/crypto/caam/ctrl.c        |  2 ++
 drivers/crypto/caam/qi.c          | 43 +++++++++++++++++++++++++------
 5 files changed, 64 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH net-next v3 1/4] crypto: caam: Avoid unused imx8m_machine_match variable

[Breno Leitao]

If caam module is built without OF support, the compiler returns the
following warning:

	drivers/crypto/caam/ctrl.c:83:34: warning: 'imx8m_machine_match' defined but not used [-Wunused-const-variable=]

imx8m_machine_match is only referenced by of_match_node(), which is set
to NULL if CONFIG_OF is not set, as of commit 5762c20593b6b ("dt: Add
empty of_match_node() macro"):

	#define of_match_node(_matches, _node)  NULL

Do not create imx8m_machine_match if CONFIG_OF is not set.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407011309.cpTuOGdg-lkp@intel.com/
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/crypto/caam/ctrl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index bd418dea586d..d4b39184dbdb 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -80,6 +80,7 @@ static void build_deinstantiation_desc(u32 *desc, int handle)
 	append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);
 }
 
+#ifdef CONFIG_OF
 static const struct of_device_id imx8m_machine_match[] = {
 	{ .compatible = "fsl,imx8mm", },
 	{ .compatible = "fsl,imx8mn", },
@@ -88,6 +89,7 @@ static const struct of_device_id imx8m_machine_match[] = {
 	{ .compatible = "fsl,imx8ulp", },
 	{ }
 };
+#endif
 
 /*
  * run_descriptor_deco0 - runs a descriptor on DECO0, under direct control of
-- 
2.43.0

[PATCH net-next v3 2/4] crypto: caam: Make CRYPTO_DEV_FSL_CAAM dependent of COMPILE_TEST

[Breno Leitao]

As most of the drivers that depend on ARCH_LAYERSCAPE, make
CRYPTO_DEV_FSL_CAAM depend on COMPILE_TEST for compilation and testing.

    # grep -r depends.\*ARCH_LAYERSCAPE.\*COMPILE_TEST | wc -l
    29

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/crypto/caam/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
index c631f99e415f..05210a0edb8a 100644
--- a/drivers/crypto/caam/Kconfig
+++ b/drivers/crypto/caam/Kconfig
@@ -10,7 +10,7 @@ config CRYPTO_DEV_FSL_CAAM_AHASH_API_DESC
 
 config CRYPTO_DEV_FSL_CAAM
 	tristate "Freescale CAAM-Multicore platform driver backend"
-	depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE
+	depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE || COMPILE_TEST
 	select SOC_BUS
 	select CRYPTO_DEV_FSL_CAAM_COMMON
 	imply FSL_MC_BUS
-- 
2.43.0

[PATCH net-next v3 3/4] crypto: caam: Unembed net_dev structure from qi

[Breno Leitao]

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.0

[PATCH net-next v3 4/4] crypto: caam: Unembed net_dev structure in dpaa2

[Breno Leitao]

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 dpaa2_caam_priv_per_cpu by
converting them into pointers, and allocating them dynamically. Use the
leverage alloc_netdev_dummy() to allocate the net_device object at
dpaa2_dpseci_setup().

The free of the device occurs at dpaa2_dpseci_disable().

Link: https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/ [1]
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/crypto/caam/caamalg_qi2.c | 28 +++++++++++++++++++++++++---
 drivers/crypto/caam/caamalg_qi2.h |  2 +-
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
index a4f6884416a0..207dc422785a 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -4990,11 +4990,23 @@ static int dpaa2_dpseci_congestion_setup(struct dpaa2_caam_priv *priv,
 	return err;
 }
 
+static void free_dpaa2_pcpu_netdev(struct dpaa2_caam_priv *priv, const cpumask_t *cpus)
+{
+	struct dpaa2_caam_priv_per_cpu *ppriv;
+	int i;
+
+	for_each_cpu(i, cpus) {
+		ppriv = per_cpu_ptr(priv->ppriv, i);
+		free_netdev(ppriv->net_dev);
+	}
+}
+
 static int __cold dpaa2_dpseci_setup(struct fsl_mc_device *ls_dev)
 {
 	struct device *dev = &ls_dev->dev;
 	struct dpaa2_caam_priv *priv;
 	struct dpaa2_caam_priv_per_cpu *ppriv;
+	cpumask_t clean_mask;
 	int err, cpu;
 	u8 i;
 
@@ -5073,6 +5085,7 @@ static int __cold dpaa2_dpseci_setup(struct fsl_mc_device *ls_dev)
 		}
 	}
 
+	cpumask_clear(&clean_mask);
 	i = 0;
 	for_each_online_cpu(cpu) {
 		u8 j;
@@ -5096,15 +5109,23 @@ static int __cold dpaa2_dpseci_setup(struct fsl_mc_device *ls_dev)
 			priv->rx_queue_attr[j].fqid,
 			priv->tx_queue_attr[j].fqid);
 
-		ppriv->net_dev.dev = *dev;
-		INIT_LIST_HEAD(&ppriv->net_dev.napi_list);
-		netif_napi_add_tx_weight(&ppriv->net_dev, &ppriv->napi,
+		ppriv->net_dev = alloc_netdev_dummy(0);
+		if (!ppriv->net_dev) {
+			err = -ENOMEM;
+			goto err_alloc_netdev;
+		}
+		cpumask_set_cpu(cpu, &clean_mask);
+		ppriv->net_dev->dev = *dev;
+
+		netif_napi_add_tx_weight(ppriv->net_dev, &ppriv->napi,
 					 dpaa2_dpseci_poll,
 					 DPAA2_CAAM_NAPI_WEIGHT);
 	}
 
 	return 0;
 
+err_alloc_netdev:
+	free_dpaa2_pcpu_netdev(priv, &clean_mask);
 err_get_rx_queue:
 	dpaa2_dpseci_congestion_free(priv);
 err_get_vers:
@@ -5153,6 +5174,7 @@ static int __cold dpaa2_dpseci_disable(struct dpaa2_caam_priv *priv)
 		ppriv = per_cpu_ptr(priv->ppriv, i);
 		napi_disable(&ppriv->napi);
 		netif_napi_del(&ppriv->napi);
+		free_netdev(ppriv->net_dev);
 	}
 
 	return 0;
diff --git a/drivers/crypto/caam/caamalg_qi2.h b/drivers/crypto/caam/caamalg_qi2.h
index abb502bb675c..61d1219a202f 100644
--- a/drivers/crypto/caam/caamalg_qi2.h
+++ b/drivers/crypto/caam/caamalg_qi2.h
@@ -81,7 +81,7 @@ struct dpaa2_caam_priv {
  */
 struct dpaa2_caam_priv_per_cpu {
 	struct napi_struct napi;
-	struct net_device net_dev;
+	struct net_device *net_dev;
 	int req_fqid;
 	int rsp_fqid;
 	int prio;
-- 
2.43.0

RE: [EXT] [PATCH net-next v3 4/4] crypto: caam: Unembed net_dev structure in dpaa2

[Gaurav Jain]

Hi Breno

> -----Original Message-----
> From: Breno Leitao <leitao@debian.org>
> Sent: Wednesday, July 3, 2024 12:26 AM
> To: kuba@kernel.org; Horia Geanta <horia.geanta@nxp.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>; linux-
> crypto@vger.kernel.org; Herbert Xu <herbert@gondor.apana.org.au>; David S.
> Miller <davem@davemloft.net>
> Cc: horms@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] [PATCH net-next v3 4/4] crypto: caam: Unembed net_dev
> structure in dpaa2
>
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
>
>
> 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 dpaa2_caam_priv_per_cpu by converting
> them into pointers, and allocating them dynamically. Use the leverage
> alloc_netdev_dummy() to allocate the net_device object at
> dpaa2_dpseci_setup().
>
> The free of the device occurs at dpaa2_dpseci_disable().
>
> Link:
> https://lore.kernel/
> .org%2Fall%2F20240229225910.79e224cf%40kernel.org%2F&data=05%7C02%7
> Cgaurav.jain%40nxp.com%7C5748b86d20dc4be03e0b08dc9ac8bfae%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638555434196331223%7CUnknow
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
> iLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=LtDEO2Ko6WFFfip9iyj%2BQycgsBE
> LG3barb9byRSxQCg%3D&reserved=0 [1]
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/crypto/caam/caamalg_qi2.c | 28 +++++++++++++++++++++++++---
> drivers/crypto/caam/caamalg_qi2.h |  2 +-
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg_qi2.c
> b/drivers/crypto/caam/caamalg_qi2.c
> index a4f6884416a0..207dc422785a 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
> @@ -4990,11 +4990,23 @@ static int dpaa2_dpseci_congestion_setup(struct
> dpaa2_caam_priv *priv,
>         return err;
>  }
>
> +static void free_dpaa2_pcpu_netdev(struct dpaa2_caam_priv *priv, const
> +cpumask_t *cpus) {
> +       struct dpaa2_caam_priv_per_cpu *ppriv;
> +       int i;
> +
> +       for_each_cpu(i, cpus) {
> +               ppriv = per_cpu_ptr(priv->ppriv, i);
> +               free_netdev(ppriv->net_dev);
> +       }
> +}
> +
>  static int __cold dpaa2_dpseci_setup(struct fsl_mc_device *ls_dev)  {
>         struct device *dev = &ls_dev->dev;
>         struct dpaa2_caam_priv *priv;
>         struct dpaa2_caam_priv_per_cpu *ppriv;
> +       cpumask_t clean_mask;
>         int err, cpu;
>         u8 i;
>
> @@ -5073,6 +5085,7 @@ static int __cold dpaa2_dpseci_setup(struct
> fsl_mc_device *ls_dev)
>                 }
>         }
>
> +       cpumask_clear(&clean_mask);
>         i = 0;
>         for_each_online_cpu(cpu) {
>                 u8 j;
> @@ -5096,15 +5109,23 @@ static int __cold dpaa2_dpseci_setup(struct
> fsl_mc_device *ls_dev)
>                         priv->rx_queue_attr[j].fqid,
>                         priv->tx_queue_attr[j].fqid);
>
> -               ppriv->net_dev.dev = *dev;
> -               INIT_LIST_HEAD(&ppriv->net_dev.napi_list);

napi_list is not needed anymore? There is no mention in commit.

Regards
Gaurav Jain
> -               netif_napi_add_tx_weight(&ppriv->net_dev, &ppriv->napi,
> +               ppriv->net_dev = alloc_netdev_dummy(0);
> +               if (!ppriv->net_dev) {
> +                       err = -ENOMEM;
> +                       goto err_alloc_netdev;
> +               }
> +               cpumask_set_cpu(cpu, &clean_mask);
> +               ppriv->net_dev->dev = *dev;
> +
> +               netif_napi_add_tx_weight(ppriv->net_dev, &ppriv->napi,
>                                          dpaa2_dpseci_poll,
>                                          DPAA2_CAAM_NAPI_WEIGHT);
>         }
>
>         return 0;
>
> +err_alloc_netdev:
> +       free_dpaa2_pcpu_netdev(priv, &clean_mask);
>  err_get_rx_queue:
>         dpaa2_dpseci_congestion_free(priv);
>  err_get_vers:
> @@ -5153,6 +5174,7 @@ static int __cold dpaa2_dpseci_disable(struct
> dpaa2_caam_priv *priv)
>                 ppriv = per_cpu_ptr(priv->ppriv, i);
>                 napi_disable(&ppriv->napi);
>                 netif_napi_del(&ppriv->napi);
> +               free_netdev(ppriv->net_dev);
>         }
>
>         return 0;
> diff --git a/drivers/crypto/caam/caamalg_qi2.h
> b/drivers/crypto/caam/caamalg_qi2.h
> index abb502bb675c..61d1219a202f 100644
> --- a/drivers/crypto/caam/caamalg_qi2.h
> +++ b/drivers/crypto/caam/caamalg_qi2.h
> @@ -81,7 +81,7 @@ struct dpaa2_caam_priv {
>   */
>  struct dpaa2_caam_priv_per_cpu {
>         struct napi_struct napi;
> -       struct net_device net_dev;
> +       struct net_device *net_dev;
>         int req_fqid;
>         int rsp_fqid;
>         int prio;
> --
> 2.43.0

Re: [EXT] [PATCH net-next v3 4/4] crypto: caam: Unembed net_dev structure in dpaa2

[Breno Leitao]

Hello Gaurav,

On Wed, Jul 03, 2024 at 05:45:23AM +0000, Gaurav Jain wrote:

> > @@ -5096,15 +5109,23 @@ static int __cold dpaa2_dpseci_setup(struct
> > fsl_mc_device *ls_dev)
> >                         priv->rx_queue_attr[j].fqid,
> >                         priv->tx_queue_attr[j].fqid);
> >
> > -               ppriv->net_dev.dev = *dev;
> > -               INIT_LIST_HEAD(&ppriv->net_dev.napi_list);

> napi_list is not needed anymore? There is no mention in commit.

Good question. This allocation is now done in the alloc_netdev_dummy()
path. This is the code path:

alloc_netdev_dummy()->alloc_netdev()->alloc_netdev_mqs() which calls:

	INIT_LIST_HEAD(&dev->napi_list);

So, napi_list is initialized when the net_device interface is allocated.

Re: [PATCH net-next v3 3/4] crypto: caam: Unembed net_dev structure from qi

[Jakub Kicinski]

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);

Re: [PATCH net-next v3 3/4] crypto: caam: Unembed net_dev structure from qi

[Breno Leitao]

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.

Re: [PATCH net-next v3 3/4] crypto: caam: Unembed net_dev structure from qi

[Jakub Kicinski]

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().

Re: [PATCH net-next v3 0/3] crypto: caam: Unembed net_dev

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  2 Jul 2024 11:55:50 -0700 you wrote:
> This will un-embed the net_device struct from inside other struct, so we
> can add flexible array into net_device.
> 
> This also enable COMPILE test for FSL_CAAM, as any config option that
> depends on ARCH_LAYERSCAPE.
> 
> Changelog:
> v3:
> 	* Fix free_netdev() deference per-cpu (Simon)
> 	* Hide imx8m_machine_match under CONFIG_OF (Jakub)
> v2:
>         * added a cover letter (Jakub)
>         * dropped the patch that makes FSL_DPAA dependent of
>           COMPILE_TEST, since it exposes other problems.
> v1:
>         * https://lore.kernel.org/all/20240624162128.1665620-1-leitao@debian.org/
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/4] crypto: caam: Avoid unused imx8m_machine_match variable
    https://git.kernel.org/netdev/net-next/c/9b5c33b1a3b7
  - [net-next,v3,2/4] crypto: caam: Make CRYPTO_DEV_FSL_CAAM dependent of COMPILE_TEST
    https://git.kernel.org/netdev/net-next/c/beba3771d9e0
  - [net-next,v3,3/4] crypto: caam: Unembed net_dev structure from qi
    https://git.kernel.org/netdev/net-next/c/82c81e740def
  - [net-next,v3,4/4] crypto: caam: Unembed net_dev structure in dpaa2
    https://git.kernel.org/netdev/net-next/c/0e1a4d427f58

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v3 1/4] crypto: caam: Avoid unused imx8m_machine_match variable

[Horia Geanta]

On 7/2/2024 9:56 PM, Breno Leitao wrote:
> If caam module is built without OF support, the compiler returns the
> following warning:
> 
> 	drivers/crypto/caam/ctrl.c:83:34: warning: 'imx8m_machine_match' defined but not used [-Wunused-const-variable=]
> 
> imx8m_machine_match is only referenced by of_match_node(), which is set
> to NULL if CONFIG_OF is not set, as of commit 5762c20593b6b ("dt: Add
> empty of_match_node() macro"):
> 
> 	#define of_match_node(_matches, _node)  NULL
> 
> Do not create imx8m_machine_match if CONFIG_OF is not set.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407011309.cpTuOGdg-lkp@intel.com/
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/crypto/caam/ctrl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index bd418dea586d..d4b39184dbdb 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -80,6 +80,7 @@ static void build_deinstantiation_desc(u32 *desc, int handle)
>  	append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);
>  }
>  
> +#ifdef CONFIG_OF
>  static const struct of_device_id imx8m_machine_match[] = {
>  	{ .compatible = "fsl,imx8mm", },
>  	{ .compatible = "fsl,imx8mn", },
> @@ -88,6 +89,7 @@ static const struct of_device_id imx8m_machine_match[] = {
>  	{ .compatible = "fsl,imx8ulp", },
>  	{ }
>  };
> +#endif
>  
Shouldn't using __maybe_unused instead of the ifdeffery be preferred
in this case?

I know my comment comes late (patch has been merged to net-next), sorry for this.
Still I'd like to clarify this for future.

Thanks,
Horia

Re: [PATCH net-next v3 1/4] crypto: caam: Avoid unused imx8m_machine_match variable

[Breno Leitao]

Hello Horia,

On Fri, Jul 05, 2024 at 10:11:40AM +0000, Horia Geanta wrote:
> On 7/2/2024 9:56 PM, Breno Leitao wrote:

> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index bd418dea586d..d4b39184dbdb 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -80,6 +80,7 @@ static void build_deinstantiation_desc(u32 *desc, int handle)
> >  	append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);
> >  }
> >  
> > +#ifdef CONFIG_OF
> >  static const struct of_device_id imx8m_machine_match[] = {
> >  	{ .compatible = "fsl,imx8mm", },
> >  	{ .compatible = "fsl,imx8mn", },
> > @@ -88,6 +89,7 @@ static const struct of_device_id imx8m_machine_match[] = {
> >  	{ .compatible = "fsl,imx8ulp", },
> >  	{ }
> >  };
> > +#endif

> Shouldn't using __maybe_unused instead of the ifdeffery be preferred
> in this case?

That is an option as well. Not sure if it makes any difference, tho.

If you prefer __maybe_unused, I am more than happy to send a follow-up
patch to convert the #ifdef to __maybe_unused. Up to you.

Thanks!

Re: [PATCH net-next v3 1/4] crypto: caam: Avoid unused imx8m_machine_match variable

[Horia Geanta]

On 7/8/2024 2:29 PM, Breno Leitao wrote:
> Hello Horia,
> 
> On Fri, Jul 05, 2024 at 10:11:40AM +0000, Horia Geanta wrote:
>> On 7/2/2024 9:56 PM, Breno Leitao wrote:
> 
>>> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
>>> index bd418dea586d..d4b39184dbdb 100644
>>> --- a/drivers/crypto/caam/ctrl.c
>>> +++ b/drivers/crypto/caam/ctrl.c
>>> @@ -80,6 +80,7 @@ static void build_deinstantiation_desc(u32 *desc, int handle)
>>>  	append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);
>>>  }
>>>  
>>> +#ifdef CONFIG_OF
>>>  static const struct of_device_id imx8m_machine_match[] = {
>>>  	{ .compatible = "fsl,imx8mm", },
>>>  	{ .compatible = "fsl,imx8mn", },
>>> @@ -88,6 +89,7 @@ static const struct of_device_id imx8m_machine_match[] = {
>>>  	{ .compatible = "fsl,imx8ulp", },
>>>  	{ }
>>>  };
>>> +#endif
> 
>> Shouldn't using __maybe_unused instead of the ifdeffery be preferred
>> in this case?
> 
> That is an option as well. Not sure if it makes any difference, tho.
> 
In general, I prefer avoiding preprocessor conditionals.
This seems to be suggested also here:
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

> If you prefer __maybe_unused, I am more than happy to send a follow-up
> patch to convert the #ifdef to __maybe_unused. Up to you.
> 
Nah, please don't bother.

Thanks,
Horia