diff mbox series

[net-next,3/9] octeontx2-pf: Create representor netdev

Message ID 20240416050616.6056-4-gakula@marvell.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Introduce RVU representors | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang fail Errors and warnings before: 937 this patch: 946
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 950 this patch: 957
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Geetha sowjanya April 16, 2024, 5:06 a.m. UTC
Adds initial devlink support to set/get the switchdev mode.
Representor netdevs are created for each rvu devices when
the switch mode is set to 'switchdev'. These netdevs are
be used to control and configure VFs.

Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
 .../marvell/octeontx2/nic/otx2_devlink.c      |  48 ++++++
 .../net/ethernet/marvell/octeontx2/nic/rep.c  | 159 ++++++++++++++++++
 .../net/ethernet/marvell/octeontx2/nic/rep.h  |   2 +
 3 files changed, 209 insertions(+)

Comments

kernel test robot April 17, 2024, 12:31 a.m. UTC | #1
Hi Geetha,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Geetha-sowjanya/octeontx2-pf-Refactoring-RVU-driver/20240416-131052
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240416050616.6056-4-gakula%40marvell.com
patch subject: [net-next PATCH 3/9] octeontx2-pf: Create representor netdev
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240417/202404170853.i93jboPB-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240417/202404170853.i93jboPB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404170853.i93jboPB-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/marvell/octeontx2/nic/rep.c: In function 'rvu_rep_create':
>> drivers/net/ethernet/marvell/octeontx2/nic/rep.c:163:66: warning: '%d' directive output may be truncated writing between 1 and 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
     163 |                 snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
         |                                                                  ^~
   drivers/net/ethernet/marvell/octeontx2/nic/rep.c:163:58: note: directive argument in the range [0, 1023]
     163 |                 snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
         |                                                          ^~~~~~~~~~~
   drivers/net/ethernet/marvell/octeontx2/nic/rep.c:163:17: note: 'snprintf' output between 7 and 20 bytes into a destination of size 16
     163 |                 snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     164 |                          rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
         |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +163 drivers/net/ethernet/marvell/octeontx2/nic/rep.c

   131	
   132	int rvu_rep_create(struct otx2_nic *priv)
   133	{
   134		int rep_cnt = priv->rep_cnt;
   135		struct net_device *ndev;
   136		struct rep_dev *rep;
   137		int rep_id, err;
   138		u16 pcifunc;
   139	
   140		priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
   141		if (!priv->reps)
   142			return -ENOMEM;
   143	
   144		for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
   145			ndev = alloc_etherdev(sizeof(*rep));
   146			if (!ndev) {
   147				dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
   148				err = -ENOMEM;
   149				goto exit;
   150			}
   151	
   152			rep = netdev_priv(ndev);
   153			priv->reps[rep_id] = rep;
   154			rep->mdev = priv;
   155			rep->netdev = ndev;
   156			rep->rep_id = rep_id;
   157	
   158			ndev->min_mtu = OTX2_MIN_MTU;
   159			ndev->max_mtu = priv->hw.max_mtu;
   160			pcifunc = priv->rep_pf_map[rep_id];
   161			rep->pcifunc = pcifunc;
   162	
 > 163			snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
   164				 rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
   165	
   166			eth_hw_addr_random(ndev);
   167			if (register_netdev(ndev)) {
   168				dev_err(priv->dev, "PFVF reprentator registration failed\n");
   169				free_netdev(ndev);
   170				ndev->netdev_ops = NULL;
   171				goto exit;
   172			}
   173		}
   174		err = rvu_rep_napi_init(priv);
   175		if (err)
   176			goto exit;
   177	
   178		return 0;
   179	exit:
   180		rvu_rep_free_netdev(priv);
   181		return err;
   182	}
   183
kernel test robot April 17, 2024, 1:24 a.m. UTC | #2
Hi Geetha,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Geetha-sowjanya/octeontx2-pf-Refactoring-RVU-driver/20240416-131052
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240416050616.6056-4-gakula%40marvell.com
patch subject: [net-next PATCH 3/9] octeontx2-pf: Create representor netdev
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240417/202404170922.RBiIclFT-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240417/202404170922.RBiIclFT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404170922.RBiIclFT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/marvell/octeontx2/nic/rep.c:167:7: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     167 |                 if (register_netdev(ndev)) {
         |                     ^~~~~~~~~~~~~~~~~~~~~
   drivers/net/ethernet/marvell/octeontx2/nic/rep.c:181:9: note: uninitialized use occurs here
     181 |         return err;
         |                ^~~
   drivers/net/ethernet/marvell/octeontx2/nic/rep.c:167:3: note: remove the 'if' if its condition is always false
     167 |                 if (register_netdev(ndev)) {
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
     168 |                         dev_err(priv->dev, "PFVF reprentator registration failed\n");
         |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     169 |                         free_netdev(ndev);
         |                         ~~~~~~~~~~~~~~~~~~
     170 |                         ndev->netdev_ops = NULL;
         |                         ~~~~~~~~~~~~~~~~~~~~~~~~
     171 |                         goto exit;
         |                         ~~~~~~~~~~
     172 |                 }
         |                 ~
   drivers/net/ethernet/marvell/octeontx2/nic/rep.c:137:17: note: initialize the variable 'err' to silence this warning
     137 |         int rep_id, err;
         |                        ^
         |                         = 0
   1 warning generated.


vim +167 drivers/net/ethernet/marvell/octeontx2/nic/rep.c

   131	
   132	int rvu_rep_create(struct otx2_nic *priv)
   133	{
   134		int rep_cnt = priv->rep_cnt;
   135		struct net_device *ndev;
   136		struct rep_dev *rep;
   137		int rep_id, err;
   138		u16 pcifunc;
   139	
   140		priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
   141		if (!priv->reps)
   142			return -ENOMEM;
   143	
   144		for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
   145			ndev = alloc_etherdev(sizeof(*rep));
   146			if (!ndev) {
   147				dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
   148				err = -ENOMEM;
   149				goto exit;
   150			}
   151	
   152			rep = netdev_priv(ndev);
   153			priv->reps[rep_id] = rep;
   154			rep->mdev = priv;
   155			rep->netdev = ndev;
   156			rep->rep_id = rep_id;
   157	
   158			ndev->min_mtu = OTX2_MIN_MTU;
   159			ndev->max_mtu = priv->hw.max_mtu;
   160			pcifunc = priv->rep_pf_map[rep_id];
   161			rep->pcifunc = pcifunc;
   162	
   163			snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
   164				 rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
   165	
   166			eth_hw_addr_random(ndev);
 > 167			if (register_netdev(ndev)) {
   168				dev_err(priv->dev, "PFVF reprentator registration failed\n");
   169				free_netdev(ndev);
   170				ndev->netdev_ops = NULL;
   171				goto exit;
   172			}
   173		}
   174		err = rvu_rep_napi_init(priv);
   175		if (err)
   176			goto exit;
   177	
   178		return 0;
   179	exit:
   180		rvu_rep_free_netdev(priv);
   181		return err;
   182	}
   183
Kalesh Anakkur Purayil April 17, 2024, 4:28 a.m. UTC | #3
On Tue, Apr 16, 2024 at 10:37 AM Geetha sowjanya <gakula@marvell.com> wrote:
>
> Adds initial devlink support to set/get the switchdev mode.
> Representor netdevs are created for each rvu devices when
> the switch mode is set to 'switchdev'. These netdevs are
> be used to control and configure VFs.
>
> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
> ---
>  .../marvell/octeontx2/nic/otx2_devlink.c      |  48 ++++++
>  .../net/ethernet/marvell/octeontx2/nic/rep.c  | 159 ++++++++++++++++++
>  .../net/ethernet/marvell/octeontx2/nic/rep.h  |   2 +
>  3 files changed, 209 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> index 4e1130496573..f4f5f5d93c7e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> @@ -76,7 +76,53 @@ static const struct devlink_param otx2_dl_params[] = {
>                              otx2_dl_mcam_count_validate),
>  };
>
> +#ifdef CONFIG_RVU_ESWITCH
> +static int otx2_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
> +{
> +       struct otx2_devlink *otx2_dl = devlink_priv(devlink);
> +       struct otx2_nic *pfvf = otx2_dl->pfvf;
> +
> +       if (!is_rep_dev(pfvf->pdev))
> +               return -EOPNOTSUPP;
> +
> +       *mode = pfvf->esw_mode;
> +
> +       return 0;
> +}
> +
> +static int otx2_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
> +                                        struct netlink_ext_ack *extack)
> +{
> +       struct otx2_devlink *otx2_dl = devlink_priv(devlink);
> +       struct otx2_nic *pfvf = otx2_dl->pfvf;
> +
> +       if (!is_rep_dev(pfvf->pdev))
> +               return -EOPNOTSUPP;
> +
> +       if (pfvf->esw_mode == mode)
> +               return 0;
> +
> +       pfvf->esw_mode = mode;
[Kalesh] Move this assignment after the switch block. Else, you will
end up updating pfvf->esw_mode to an unsupported mode even though
drover returns error.
> +       switch (mode) {
> +       case DEVLINK_ESWITCH_MODE_LEGACY:
> +               rvu_rep_destroy(pfvf);
> +               break;
> +       case DEVLINK_ESWITCH_MODE_SWITCHDEV:
> +               rvu_rep_create(pfvf);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static const struct devlink_ops otx2_devlink_ops = {
> +#ifdef CONFIG_RVU_ESWITCH
> +       .eswitch_mode_get = otx2_devlink_eswitch_mode_get,
> +       .eswitch_mode_set = otx2_devlink_eswitch_mode_set,
> +#endif
>  };
>
>  int otx2_register_dl(struct otx2_nic *pfvf)
> @@ -112,6 +158,7 @@ int otx2_register_dl(struct otx2_nic *pfvf)
>         devlink_free(dl);
>         return err;
>  }
> +EXPORT_SYMBOL(otx2_register_dl);
>
>  void otx2_unregister_dl(struct otx2_nic *pfvf)
>  {
> @@ -123,3 +170,4 @@ void otx2_unregister_dl(struct otx2_nic *pfvf)
>                                   ARRAY_SIZE(otx2_dl_params));
>         devlink_free(dl);
>  }
> +EXPORT_SYMBOL(otx2_unregister_dl);
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index b892a7fe3ddc..fd55ef40c934 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -28,6 +28,159 @@ MODULE_DESCRIPTION(DRV_STRING);
>  MODULE_LICENSE("GPL");
>  MODULE_DEVICE_TABLE(pci, rvu_rep_id_table);
>
> +static int rvu_rep_napi_init(struct otx2_nic *priv)
> +{
> +       struct otx2_cq_poll *cq_poll = NULL;
> +       struct otx2_qset *qset = &priv->qset;
> +       struct otx2_hw *hw = &priv->hw;
> +       int err = 0, qidx, vec;
> +       char *irq_name;
> +
> +       qset->napi = kcalloc(hw->cint_cnt, sizeof(*cq_poll), GFP_KERNEL);
> +       if (!qset->napi)
> +               return -ENOMEM;
> +
> +       /* Register NAPI handler */
> +       for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
> +               cq_poll = &qset->napi[qidx];
> +               cq_poll->cint_idx = qidx;
> +               cq_poll->cq_ids[CQ_RX] =
> +                       (qidx <  hw->rx_queues) ? qidx : CINT_INVALID_CQ;
> +               cq_poll->cq_ids[CQ_TX] = (qidx < hw->tx_queues) ?
> +                                         qidx + hw->rx_queues : CINT_INVALID_CQ;
> +               cq_poll->cq_ids[CQ_XDP] = CINT_INVALID_CQ;
> +               cq_poll->cq_ids[CQ_QOS] = CINT_INVALID_CQ;
> +
> +               cq_poll->dev = (void *)priv;
> +               netif_napi_add(priv->reps[qidx]->netdev, &cq_poll->napi,
> +                              otx2_napi_handler);
> +               napi_enable(&cq_poll->napi);
> +       }
> +       /* Register CQ IRQ handlers */
> +       vec = hw->nix_msixoff + NIX_LF_CINT_VEC_START;
> +       for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
> +               irq_name = &hw->irq_name[vec * NAME_SIZE];
> +
> +               snprintf(irq_name, NAME_SIZE, "rep%d-rxtx-%d", qidx, qidx);
> +
> +               err = request_irq(pci_irq_vector(priv->pdev, vec),
> +                                 otx2_cq_intr_handler, 0, irq_name,
> +                                 &qset->napi[qidx]);
> +               if (err) {
> +                       dev_err(priv->dev,
> +                               "RVU REP IRQ registration failed for CQ%d\n", qidx);
> +                       goto err_free_cints;
> +               }
> +               vec++;
> +
> +               /* Enable CQ IRQ */
> +               otx2_write64(priv, NIX_LF_CINTX_INT(qidx), BIT_ULL(0));
> +               otx2_write64(priv, NIX_LF_CINTX_ENA_W1S(qidx), BIT_ULL(0));
> +       }
> +       priv->flags &= ~OTX2_FLAG_INTF_DOWN;
> +       return 0;
> +
> +err_free_cints:
> +       otx2_free_cints(priv, qidx);
> +       otx2_disable_napi(priv);
> +       return err;
> +}
> +
> +static void rvu_rep_free_cq_rsrc(struct otx2_nic *priv)
> +{
> +       struct otx2_cq_poll *cq_poll = NULL;
> +       struct otx2_qset *qset = &priv->qset;
> +       int qidx, vec;
> +
> +       /* Cleanup CQ NAPI and IRQ */
> +       vec = priv->hw.nix_msixoff + NIX_LF_CINT_VEC_START;
> +       for (qidx = 0; qidx < priv->hw.cint_cnt; qidx++) {
> +               /* Disable interrupt */
> +               otx2_write64(priv, NIX_LF_CINTX_ENA_W1C(qidx), BIT_ULL(0));
> +
> +               synchronize_irq(pci_irq_vector(priv->pdev, vec));
> +
> +               cq_poll = &qset->napi[qidx];
> +               napi_synchronize(&cq_poll->napi);
> +               vec++;
> +       }
> +       otx2_free_cints(priv, priv->hw.cint_cnt);
> +       otx2_disable_napi(priv);
> +}
> +
> +static void rvu_rep_free_netdev(struct otx2_nic *priv)
> +{
> +       struct rep_dev *rep;
> +       int rep_id;
> +
> +       for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
> +               rep = priv->reps[rep_id];
> +               if (rep && rep->netdev->netdev_ops) {
> +                       unregister_netdev(rep->netdev);
> +                       free_netdev(rep->netdev);
> +               }
> +       }
> +       devm_kfree(priv->dev, priv->reps);
> +}
> +
> +void rvu_rep_destroy(struct otx2_nic *priv)
> +{
> +       rvu_rep_free_cq_rsrc(priv);
> +       rvu_rep_free_netdev(priv);
> +}
> +
> +int rvu_rep_create(struct otx2_nic *priv)
> +{
> +       int rep_cnt = priv->rep_cnt;
> +       struct net_device *ndev;
> +       struct rep_dev *rep;
> +       int rep_id, err;
> +       u16 pcifunc;
> +
> +       priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
> +       if (!priv->reps)
> +               return -ENOMEM;
> +
> +       for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
> +               ndev = alloc_etherdev(sizeof(*rep));
> +               if (!ndev) {
> +                       dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
> +                       err = -ENOMEM;
> +                       goto exit;
> +               }
> +
> +               rep = netdev_priv(ndev);
> +               priv->reps[rep_id] = rep;
> +               rep->mdev = priv;
> +               rep->netdev = ndev;
> +               rep->rep_id = rep_id;
> +
> +               ndev->min_mtu = OTX2_MIN_MTU;
> +               ndev->max_mtu = priv->hw.max_mtu;
> +               pcifunc = priv->rep_pf_map[rep_id];
> +               rep->pcifunc = pcifunc;
> +
> +               snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
> +                        rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
> +
> +               eth_hw_addr_random(ndev);
> +               if (register_netdev(ndev)) {
> +                       dev_err(priv->dev, "PFVF reprentator registration failed\n");
> +                       free_netdev(ndev);
> +                       ndev->netdev_ops = NULL;
[Kalesh] update "rc" with the return status of register_netdev()
> +                       goto exit;
> +               }
> +       }
> +       err = rvu_rep_napi_init(priv);
> +       if (err)
> +               goto exit;
> +
> +       return 0;
> +exit:
> +       rvu_rep_free_netdev(priv);
> +       return err;
> +}
> +
>  static int rvu_rep_rsrc_free(struct otx2_nic *priv)
>  {
>         struct otx2_qset *qset = &priv->qset;
> @@ -162,6 +315,10 @@ static int rvu_rep_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>         if (err)
>                 goto err_detach_rsrc;
>
> +       err = otx2_register_dl(priv);
> +       if (err)
> +               goto err_detach_rsrc;
> +
>         return 0;
>
>  err_detach_rsrc:
> @@ -183,6 +340,8 @@ static void rvu_rep_remove(struct pci_dev *pdev)
>  {
>         struct otx2_nic *priv = pci_get_drvdata(pdev);
>
> +       otx2_unregister_dl(priv);
> +       rvu_rep_destroy(priv);
>         rvu_rep_rsrc_free(priv);
>         otx2_detach_resources(&priv->mbox);
>         if (priv->hw.lmt_info)
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> index 30cce17eb48b..be6c939e5cba 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> @@ -29,4 +29,6 @@ static inline bool is_rep_dev(struct pci_dev *pdev)
>         return pdev->device == PCI_DEVID_RVU_REP;
>  }
>
> +int rvu_rep_create(struct otx2_nic *priv);
> +void rvu_rep_destroy(struct otx2_nic *priv);
>  #endif /* REP_H */
> --
> 2.25.1
>
>
Geetha sowjanya April 17, 2024, 1:43 p.m. UTC | #4
Hi Kalesh,



> -----Original Message-----
> From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
> Sent: Wednesday, April 17, 2024 9:58 AM
> To: Geethasowjanya Akula <gakula@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kuba@kernel.org;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Sunil
> Kovvuri Goutham <sgoutham@marvell.com>; Subbaraya Sundeep Bhatta
> <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>
> Subject: [EXTERNAL] Re: [net-next PATCH 3/9] octeontx2-pf: Create
> representor netdev
> ----------------------------------------------------------------------
> On Tue, Apr 16, 2024 at 10:37 AM Geetha sowjanya <gakula@marvell.com>
> wrote:
> >
> > Adds initial devlink support to set/get the switchdev mode.
> > Representor netdevs are created for each rvu devices when
> > the switch mode is set to 'switchdev'. These netdevs are
> > be used to control and configure VFs.
> >
> > Signed-off-by: Geetha sowjanya <gakula@marvell.com>
> > ---
> >  .../marvell/octeontx2/nic/otx2_devlink.c      |  48 ++++++
> >  .../net/ethernet/marvell/octeontx2/nic/rep.c  | 159 ++++++++++++++++++
> >  .../net/ethernet/marvell/octeontx2/nic/rep.h  |   2 +
> >  3 files changed, 209 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> > index 4e1130496573..f4f5f5d93c7e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> > @@ -76,7 +76,53 @@ static const struct devlink_param otx2_dl_params[] =
> {
> >                              otx2_dl_mcam_count_validate),
> >  };
> >
> > +#ifdef CONFIG_RVU_ESWITCH
> > +static int otx2_devlink_eswitch_mode_get(struct devlink *devlink, u16
> *mode)
> > +{
> > +       struct otx2_devlink *otx2_dl = devlink_priv(devlink);
> > +       struct otx2_nic *pfvf = otx2_dl->pfvf;
> > +
> > +       if (!is_rep_dev(pfvf->pdev))
> > +               return -EOPNOTSUPP;
> > +
> > +       *mode = pfvf->esw_mode;
> > +
> > +       return 0;
> > +}
> > +
> > +static int otx2_devlink_eswitch_mode_set(struct devlink *devlink, u16
> mode,
> > +                                        struct netlink_ext_ack *extack)
> > +{
> > +       struct otx2_devlink *otx2_dl = devlink_priv(devlink);
> > +       struct otx2_nic *pfvf = otx2_dl->pfvf;
> > +
> > +       if (!is_rep_dev(pfvf->pdev))
> > +               return -EOPNOTSUPP;
> > +
> > +       if (pfvf->esw_mode == mode)
> > +               return 0;
> > +
> > +       pfvf->esw_mode = mode;
> [Kalesh] Move this assignment after the switch block. Else, you will
> end up updating pfvf->esw_mode to an unsupported mode even though
> drover returns error.
Ack.
> > +       switch (mode) {
> > +       case DEVLINK_ESWITCH_MODE_LEGACY:
> > +               rvu_rep_destroy(pfvf);
> > +               break;
> > +       case DEVLINK_ESWITCH_MODE_SWITCHDEV:
> > +               rvu_rep_create(pfvf);
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> >  static const struct devlink_ops otx2_devlink_ops = {
> > +#ifdef CONFIG_RVU_ESWITCH
> > +       .eswitch_mode_get = otx2_devlink_eswitch_mode_get,
> > +       .eswitch_mode_set = otx2_devlink_eswitch_mode_set,
> > +#endif
> >  };
> >
> >  int otx2_register_dl(struct otx2_nic *pfvf)
> > @@ -112,6 +158,7 @@ int otx2_register_dl(struct otx2_nic *pfvf)
> >         devlink_free(dl);
> >         return err;
> >  }
> > +EXPORT_SYMBOL(otx2_register_dl);
> >
> >  void otx2_unregister_dl(struct otx2_nic *pfvf)
> >  {
> > @@ -123,3 +170,4 @@ void otx2_unregister_dl(struct otx2_nic *pfvf)
> >                                   ARRAY_SIZE(otx2_dl_params));
> >         devlink_free(dl);
> >  }
> > +EXPORT_SYMBOL(otx2_unregister_dl);
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > index b892a7fe3ddc..fd55ef40c934 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > @@ -28,6 +28,159 @@ MODULE_DESCRIPTION(DRV_STRING);
> >  MODULE_LICENSE("GPL");
> >  MODULE_DEVICE_TABLE(pci, rvu_rep_id_table);
> >
> > +static int rvu_rep_napi_init(struct otx2_nic *priv)
> > +{
> > +       struct otx2_cq_poll *cq_poll = NULL;
> > +       struct otx2_qset *qset = &priv->qset;
> > +       struct otx2_hw *hw = &priv->hw;
> > +       int err = 0, qidx, vec;
> > +       char *irq_name;
> > +
> > +       qset->napi = kcalloc(hw->cint_cnt, sizeof(*cq_poll), GFP_KERNEL);
> > +       if (!qset->napi)
> > +               return -ENOMEM;
> > +
> > +       /* Register NAPI handler */
> > +       for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
> > +               cq_poll = &qset->napi[qidx];
> > +               cq_poll->cint_idx = qidx;
> > +               cq_poll->cq_ids[CQ_RX] =
> > +                       (qidx <  hw->rx_queues) ? qidx : CINT_INVALID_CQ;
> > +               cq_poll->cq_ids[CQ_TX] = (qidx < hw->tx_queues) ?
> > +                                         qidx + hw->rx_queues : CINT_INVALID_CQ;
> > +               cq_poll->cq_ids[CQ_XDP] = CINT_INVALID_CQ;
> > +               cq_poll->cq_ids[CQ_QOS] = CINT_INVALID_CQ;
> > +
> > +               cq_poll->dev = (void *)priv;
> > +               netif_napi_add(priv->reps[qidx]->netdev, &cq_poll->napi,
> > +                              otx2_napi_handler);
> > +               napi_enable(&cq_poll->napi);
> > +       }
> > +       /* Register CQ IRQ handlers */
> > +       vec = hw->nix_msixoff + NIX_LF_CINT_VEC_START;
> > +       for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
> > +               irq_name = &hw->irq_name[vec * NAME_SIZE];
> > +
> > +               snprintf(irq_name, NAME_SIZE, "rep%d-rxtx-%d", qidx, qidx);
> > +
> > +               err = request_irq(pci_irq_vector(priv->pdev, vec),
> > +                                 otx2_cq_intr_handler, 0, irq_name,
> > +                                 &qset->napi[qidx]);
> > +               if (err) {
> > +                       dev_err(priv->dev,
> > +                               "RVU REP IRQ registration failed for CQ%d\n", qidx);
> > +                       goto err_free_cints;
> > +               }
> > +               vec++;
> > +
> > +               /* Enable CQ IRQ */
> > +               otx2_write64(priv, NIX_LF_CINTX_INT(qidx), BIT_ULL(0));
> > +               otx2_write64(priv, NIX_LF_CINTX_ENA_W1S(qidx), BIT_ULL(0));
> > +       }
> > +       priv->flags &= ~OTX2_FLAG_INTF_DOWN;
> > +       return 0;
> > +
> > +err_free_cints:
> > +       otx2_free_cints(priv, qidx);
> > +       otx2_disable_napi(priv);
> > +       return err;
> > +}
> > +
> > +static void rvu_rep_free_cq_rsrc(struct otx2_nic *priv)
> > +{
> > +       struct otx2_cq_poll *cq_poll = NULL;
> > +       struct otx2_qset *qset = &priv->qset;
> > +       int qidx, vec;
> > +
> > +       /* Cleanup CQ NAPI and IRQ */
> > +       vec = priv->hw.nix_msixoff + NIX_LF_CINT_VEC_START;
> > +       for (qidx = 0; qidx < priv->hw.cint_cnt; qidx++) {
> > +               /* Disable interrupt */
> > +               otx2_write64(priv, NIX_LF_CINTX_ENA_W1C(qidx), BIT_ULL(0));
> > +
> > +               synchronize_irq(pci_irq_vector(priv->pdev, vec));
> > +
> > +               cq_poll = &qset->napi[qidx];
> > +               napi_synchronize(&cq_poll->napi);
> > +               vec++;
> > +       }
> > +       otx2_free_cints(priv, priv->hw.cint_cnt);
> > +       otx2_disable_napi(priv);
> > +}
> > +
> > +static void rvu_rep_free_netdev(struct otx2_nic *priv)
> > +{
> > +       struct rep_dev *rep;
> > +       int rep_id;
> > +
> > +       for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
> > +               rep = priv->reps[rep_id];
> > +               if (rep && rep->netdev->netdev_ops) {
> > +                       unregister_netdev(rep->netdev);
> > +                       free_netdev(rep->netdev);
> > +               }
> > +       }
> > +       devm_kfree(priv->dev, priv->reps);
> > +}
> > +
> > +void rvu_rep_destroy(struct otx2_nic *priv)
> > +{
> > +       rvu_rep_free_cq_rsrc(priv);
> > +       rvu_rep_free_netdev(priv);
> > +}
> > +
> > +int rvu_rep_create(struct otx2_nic *priv)
> > +{
> > +       int rep_cnt = priv->rep_cnt;
> > +       struct net_device *ndev;
> > +       struct rep_dev *rep;
> > +       int rep_id, err;
> > +       u16 pcifunc;
> > +
> > +       priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev),
> GFP_KERNEL);
> > +       if (!priv->reps)
> > +               return -ENOMEM;
> > +
> > +       for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
> > +               ndev = alloc_etherdev(sizeof(*rep));
> > +               if (!ndev) {
> > +                       dev_err(priv->dev, "PFVF representor:%d creation failed\n",
> rep_id);
> > +                       err = -ENOMEM;
> > +                       goto exit;
> > +               }
> > +
> > +               rep = netdev_priv(ndev);
> > +               priv->reps[rep_id] = rep;
> > +               rep->mdev = priv;
> > +               rep->netdev = ndev;
> > +               rep->rep_id = rep_id;
> > +
> > +               ndev->min_mtu = OTX2_MIN_MTU;
> > +               ndev->max_mtu = priv->hw.max_mtu;
> > +               pcifunc = priv->rep_pf_map[rep_id];
> > +               rep->pcifunc = pcifunc;
> > +
> > +               snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
> > +                        rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
> > +
> > +               eth_hw_addr_random(ndev);
> > +               if (register_netdev(ndev)) {
> > +                       dev_err(priv->dev, "PFVF reprentator registration failed\n");
> > +                       free_netdev(ndev);
> > +                       ndev->netdev_ops = NULL;
> [Kalesh] update "rc" with the return status of register_netdev()
Ack.  Thanks for reviewing. Will submit the v2 with the changes. 
> > +                       goto exit;
> > +               }
> > +       }
> > +       err = rvu_rep_napi_init(priv);
> > +       if (err)
> > +               goto exit;
> > +
> > +       return 0;
> > +exit:
> > +       rvu_rep_free_netdev(priv);
> > +       return err;
> > +}
> > +
> >  static int rvu_rep_rsrc_free(struct otx2_nic *priv)
> >  {
> >         struct otx2_qset *qset = &priv->qset;
> > @@ -162,6 +315,10 @@ static int rvu_rep_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> >         if (err)
> >                 goto err_detach_rsrc;
> >
> > +       err = otx2_register_dl(priv);
> > +       if (err)
> > +               goto err_detach_rsrc;
> > +
> >         return 0;
> >
> >  err_detach_rsrc:
> > @@ -183,6 +340,8 @@ static void rvu_rep_remove(struct pci_dev *pdev)
> >  {
> >         struct otx2_nic *priv = pci_get_drvdata(pdev);
> >
> > +       otx2_unregister_dl(priv);
> > +       rvu_rep_destroy(priv);
> >         rvu_rep_rsrc_free(priv);
> >         otx2_detach_resources(&priv->mbox);
> >         if (priv->hw.lmt_info)
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> > index 30cce17eb48b..be6c939e5cba 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
> > @@ -29,4 +29,6 @@ static inline bool is_rep_dev(struct pci_dev *pdev)
> >         return pdev->device == PCI_DEVID_RVU_REP;
> >  }
> >
> > +int rvu_rep_create(struct otx2_nic *priv);
> > +void rvu_rep_destroy(struct otx2_nic *priv);
> >  #endif /* REP_H */
> > --
> > 2.25.1
> >
> >
> 
> 
> --
> Regards,
> Kalesh A P
Dan Carpenter April 17, 2024, 3:24 p.m. UTC | #5
Hi Geetha,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Geetha-sowjanya/octeontx2-pf-Refactoring-RVU-driver/20240416-131052
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240416050616.6056-4-gakula%40marvell.com
patch subject: [net-next PATCH 3/9] octeontx2-pf: Create representor netdev
config: alpha-randconfig-r081-20240417 (https://download.01.org/0day-ci/archive/20240417/202404172208.4REfSKKS-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404172208.4REfSKKS-lkp@intel.com/

New smatch warnings:
drivers/net/ethernet/marvell/octeontx2/nic/rep.c:170 rvu_rep_create() error: dereferencing freed memory 'ndev'

vim +/ndev +170 drivers/net/ethernet/marvell/octeontx2/nic/rep.c

f9a5b510759eeb Geetha sowjanya 2024-04-16  131  
f9a5b510759eeb Geetha sowjanya 2024-04-16  132  int rvu_rep_create(struct otx2_nic *priv)
f9a5b510759eeb Geetha sowjanya 2024-04-16  133  {
f9a5b510759eeb Geetha sowjanya 2024-04-16  134  	int rep_cnt = priv->rep_cnt;
f9a5b510759eeb Geetha sowjanya 2024-04-16  135  	struct net_device *ndev;
f9a5b510759eeb Geetha sowjanya 2024-04-16  136  	struct rep_dev *rep;
f9a5b510759eeb Geetha sowjanya 2024-04-16  137  	int rep_id, err;
f9a5b510759eeb Geetha sowjanya 2024-04-16  138  	u16 pcifunc;
f9a5b510759eeb Geetha sowjanya 2024-04-16  139  
f9a5b510759eeb Geetha sowjanya 2024-04-16  140  	priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
f9a5b510759eeb Geetha sowjanya 2024-04-16  141  	if (!priv->reps)
f9a5b510759eeb Geetha sowjanya 2024-04-16  142  		return -ENOMEM;
f9a5b510759eeb Geetha sowjanya 2024-04-16  143  
f9a5b510759eeb Geetha sowjanya 2024-04-16  144  	for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
f9a5b510759eeb Geetha sowjanya 2024-04-16  145  		ndev = alloc_etherdev(sizeof(*rep));
f9a5b510759eeb Geetha sowjanya 2024-04-16  146  		if (!ndev) {
f9a5b510759eeb Geetha sowjanya 2024-04-16  147  			dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
f9a5b510759eeb Geetha sowjanya 2024-04-16  148  			err = -ENOMEM;
f9a5b510759eeb Geetha sowjanya 2024-04-16  149  			goto exit;
f9a5b510759eeb Geetha sowjanya 2024-04-16  150  		}
f9a5b510759eeb Geetha sowjanya 2024-04-16  151  
f9a5b510759eeb Geetha sowjanya 2024-04-16  152  		rep = netdev_priv(ndev);
f9a5b510759eeb Geetha sowjanya 2024-04-16  153  		priv->reps[rep_id] = rep;
f9a5b510759eeb Geetha sowjanya 2024-04-16  154  		rep->mdev = priv;
f9a5b510759eeb Geetha sowjanya 2024-04-16  155  		rep->netdev = ndev;
f9a5b510759eeb Geetha sowjanya 2024-04-16  156  		rep->rep_id = rep_id;
f9a5b510759eeb Geetha sowjanya 2024-04-16  157  
f9a5b510759eeb Geetha sowjanya 2024-04-16  158  		ndev->min_mtu = OTX2_MIN_MTU;
f9a5b510759eeb Geetha sowjanya 2024-04-16  159  		ndev->max_mtu = priv->hw.max_mtu;
f9a5b510759eeb Geetha sowjanya 2024-04-16  160  		pcifunc = priv->rep_pf_map[rep_id];
f9a5b510759eeb Geetha sowjanya 2024-04-16  161  		rep->pcifunc = pcifunc;
f9a5b510759eeb Geetha sowjanya 2024-04-16  162  
f9a5b510759eeb Geetha sowjanya 2024-04-16  163  		snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
f9a5b510759eeb Geetha sowjanya 2024-04-16  164  			 rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
f9a5b510759eeb Geetha sowjanya 2024-04-16  165  
f9a5b510759eeb Geetha sowjanya 2024-04-16  166  		eth_hw_addr_random(ndev);
f9a5b510759eeb Geetha sowjanya 2024-04-16  167  		if (register_netdev(ndev)) {

err = register_netdev(ndev);
if (err) {

f9a5b510759eeb Geetha sowjanya 2024-04-16  168  			dev_err(priv->dev, "PFVF reprentator registration failed\n");
f9a5b510759eeb Geetha sowjanya 2024-04-16  169  			free_netdev(ndev);
                                                                                    ^^^^
freed

f9a5b510759eeb Geetha sowjanya 2024-04-16 @170  			ndev->netdev_ops = NULL;
                                                                        ^^^^^^^^^^^^^^^^^^^^^^^
Use after free

f9a5b510759eeb Geetha sowjanya 2024-04-16  171  			goto exit;
f9a5b510759eeb Geetha sowjanya 2024-04-16  172  		}
f9a5b510759eeb Geetha sowjanya 2024-04-16  173  	}
f9a5b510759eeb Geetha sowjanya 2024-04-16  174  	err = rvu_rep_napi_init(priv);
f9a5b510759eeb Geetha sowjanya 2024-04-16  175  	if (err)
f9a5b510759eeb Geetha sowjanya 2024-04-16  176  		goto exit;
f9a5b510759eeb Geetha sowjanya 2024-04-16  177  
f9a5b510759eeb Geetha sowjanya 2024-04-16  178  	return 0;
f9a5b510759eeb Geetha sowjanya 2024-04-16  179  exit:
f9a5b510759eeb Geetha sowjanya 2024-04-16  180  	rvu_rep_free_netdev(priv);

rvu_rep_free_netdev() also calls free_netdev() so it's a double free.  I
would normally write this as:

exit:
	while (--rep_id >= 0) {
		unregister_netdev(priv->reps[rep_id]);
		free_netdev(priv->reps[rep_id]);
	}

	return err;

When you write it that way then rvu_rep_free_netdev() can be made easier
as well:

static void rvu_rep_free_netdev(struct otx2_nic *priv)
{
	int rep_id;

	for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
		unregister_netdev(priv->reps[rep_id]);
		free_netdev(priv->reps[rep_id]);
	}
}

There should be no need to call devm_kfree(priv->dev, priv->reps);.

f9a5b510759eeb Geetha sowjanya 2024-04-16 @181  	return err;
f9a5b510759eeb Geetha sowjanya 2024-04-16  182  }
Dan Carpenter April 17, 2024, 3:36 p.m. UTC | #6
On Wed, Apr 17, 2024 at 06:24:13PM +0300, Dan Carpenter wrote:
> f9a5b510759eeb Geetha sowjanya 2024-04-16  132  int rvu_rep_create(struct otx2_nic *priv)
> f9a5b510759eeb Geetha sowjanya 2024-04-16  133  {
> f9a5b510759eeb Geetha sowjanya 2024-04-16  134  	int rep_cnt = priv->rep_cnt;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  135  	struct net_device *ndev;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  136  	struct rep_dev *rep;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  137  	int rep_id, err;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  138  	u16 pcifunc;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  139  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  140  	priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
> f9a5b510759eeb Geetha sowjanya 2024-04-16  141  	if (!priv->reps)
> f9a5b510759eeb Geetha sowjanya 2024-04-16  142  		return -ENOMEM;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  143  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  144  	for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
> f9a5b510759eeb Geetha sowjanya 2024-04-16  145  		ndev = alloc_etherdev(sizeof(*rep));
> f9a5b510759eeb Geetha sowjanya 2024-04-16  146  		if (!ndev) {
> f9a5b510759eeb Geetha sowjanya 2024-04-16  147  			dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
> f9a5b510759eeb Geetha sowjanya 2024-04-16  148  			err = -ENOMEM;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  149  			goto exit;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  150  		}
> f9a5b510759eeb Geetha sowjanya 2024-04-16  151  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  152  		rep = netdev_priv(ndev);
> f9a5b510759eeb Geetha sowjanya 2024-04-16  153  		priv->reps[rep_id] = rep;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  154  		rep->mdev = priv;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  155  		rep->netdev = ndev;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  156  		rep->rep_id = rep_id;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  157  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  158  		ndev->min_mtu = OTX2_MIN_MTU;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  159  		ndev->max_mtu = priv->hw.max_mtu;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  160  		pcifunc = priv->rep_pf_map[rep_id];
> f9a5b510759eeb Geetha sowjanya 2024-04-16  161  		rep->pcifunc = pcifunc;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  162  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  163  		snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
> f9a5b510759eeb Geetha sowjanya 2024-04-16  164  			 rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
> f9a5b510759eeb Geetha sowjanya 2024-04-16  165  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  166  		eth_hw_addr_random(ndev);
> f9a5b510759eeb Geetha sowjanya 2024-04-16  167  		if (register_netdev(ndev)) {
> 
> err = register_netdev(ndev);
> if (err) {
> 
> f9a5b510759eeb Geetha sowjanya 2024-04-16  168  			dev_err(priv->dev, "PFVF reprentator registration failed\n");
> f9a5b510759eeb Geetha sowjanya 2024-04-16  169  			free_netdev(ndev);
>                                                                                     ^^^^
> freed
> 
> f9a5b510759eeb Geetha sowjanya 2024-04-16 @170  			ndev->netdev_ops = NULL;
>                                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> Use after free
> 
> f9a5b510759eeb Geetha sowjanya 2024-04-16  171  			goto exit;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  172  		}
> f9a5b510759eeb Geetha sowjanya 2024-04-16  173  	}
> f9a5b510759eeb Geetha sowjanya 2024-04-16  174  	err = rvu_rep_napi_init(priv);
> f9a5b510759eeb Geetha sowjanya 2024-04-16  175  	if (err)
> f9a5b510759eeb Geetha sowjanya 2024-04-16  176  		goto exit;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  177  
> f9a5b510759eeb Geetha sowjanya 2024-04-16  178  	return 0;
> f9a5b510759eeb Geetha sowjanya 2024-04-16  179  exit:
> f9a5b510759eeb Geetha sowjanya 2024-04-16  180  	rvu_rep_free_netdev(priv);
> 
> rvu_rep_free_netdev() also calls free_netdev() so it's a double free.

Actually the rep->netdev->netdev_ops check in rvu_rep_free_netdev() was
supposed to prevent the double free.  But since rep->netdev is already
freed, then it's another use after free.  You could use a different flag
instead of rep->netdev->netdev_ops to mean "don't free this".  But
really, it's just better to write it how I have suggested.

My patch adds some duplicate code but when you remove the conditions in
rvu_rep_free_netdev() and the "ndev->netdev_ops = NULL" assignment, then
overall it's fewer lines of code this way.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
index 4e1130496573..f4f5f5d93c7e 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
@@ -76,7 +76,53 @@  static const struct devlink_param otx2_dl_params[] = {
 			     otx2_dl_mcam_count_validate),
 };
 
+#ifdef CONFIG_RVU_ESWITCH
+static int otx2_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
+{
+	struct otx2_devlink *otx2_dl = devlink_priv(devlink);
+	struct otx2_nic *pfvf = otx2_dl->pfvf;
+
+	if (!is_rep_dev(pfvf->pdev))
+		return -EOPNOTSUPP;
+
+	*mode = pfvf->esw_mode;
+
+	return 0;
+}
+
+static int otx2_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
+					 struct netlink_ext_ack *extack)
+{
+	struct otx2_devlink *otx2_dl = devlink_priv(devlink);
+	struct otx2_nic *pfvf = otx2_dl->pfvf;
+
+	if (!is_rep_dev(pfvf->pdev))
+		return -EOPNOTSUPP;
+
+	if (pfvf->esw_mode == mode)
+		return 0;
+
+	pfvf->esw_mode = mode;
+	switch (mode) {
+	case DEVLINK_ESWITCH_MODE_LEGACY:
+		rvu_rep_destroy(pfvf);
+		break;
+	case DEVLINK_ESWITCH_MODE_SWITCHDEV:
+		rvu_rep_create(pfvf);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+#endif
+
 static const struct devlink_ops otx2_devlink_ops = {
+#ifdef CONFIG_RVU_ESWITCH
+	.eswitch_mode_get = otx2_devlink_eswitch_mode_get,
+	.eswitch_mode_set = otx2_devlink_eswitch_mode_set,
+#endif
 };
 
 int otx2_register_dl(struct otx2_nic *pfvf)
@@ -112,6 +158,7 @@  int otx2_register_dl(struct otx2_nic *pfvf)
 	devlink_free(dl);
 	return err;
 }
+EXPORT_SYMBOL(otx2_register_dl);
 
 void otx2_unregister_dl(struct otx2_nic *pfvf)
 {
@@ -123,3 +170,4 @@  void otx2_unregister_dl(struct otx2_nic *pfvf)
 				  ARRAY_SIZE(otx2_dl_params));
 	devlink_free(dl);
 }
+EXPORT_SYMBOL(otx2_unregister_dl);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
index b892a7fe3ddc..fd55ef40c934 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
@@ -28,6 +28,159 @@  MODULE_DESCRIPTION(DRV_STRING);
 MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(pci, rvu_rep_id_table);
 
+static int rvu_rep_napi_init(struct otx2_nic *priv)
+{
+	struct otx2_cq_poll *cq_poll = NULL;
+	struct otx2_qset *qset = &priv->qset;
+	struct otx2_hw *hw = &priv->hw;
+	int err = 0, qidx, vec;
+	char *irq_name;
+
+	qset->napi = kcalloc(hw->cint_cnt, sizeof(*cq_poll), GFP_KERNEL);
+	if (!qset->napi)
+		return -ENOMEM;
+
+	/* Register NAPI handler */
+	for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
+		cq_poll = &qset->napi[qidx];
+		cq_poll->cint_idx = qidx;
+		cq_poll->cq_ids[CQ_RX] =
+			(qidx <  hw->rx_queues) ? qidx : CINT_INVALID_CQ;
+		cq_poll->cq_ids[CQ_TX] = (qidx < hw->tx_queues) ?
+					  qidx + hw->rx_queues : CINT_INVALID_CQ;
+		cq_poll->cq_ids[CQ_XDP] = CINT_INVALID_CQ;
+		cq_poll->cq_ids[CQ_QOS] = CINT_INVALID_CQ;
+
+		cq_poll->dev = (void *)priv;
+		netif_napi_add(priv->reps[qidx]->netdev, &cq_poll->napi,
+			       otx2_napi_handler);
+		napi_enable(&cq_poll->napi);
+	}
+	/* Register CQ IRQ handlers */
+	vec = hw->nix_msixoff + NIX_LF_CINT_VEC_START;
+	for (qidx = 0; qidx < hw->cint_cnt; qidx++) {
+		irq_name = &hw->irq_name[vec * NAME_SIZE];
+
+		snprintf(irq_name, NAME_SIZE, "rep%d-rxtx-%d", qidx, qidx);
+
+		err = request_irq(pci_irq_vector(priv->pdev, vec),
+				  otx2_cq_intr_handler, 0, irq_name,
+				  &qset->napi[qidx]);
+		if (err) {
+			dev_err(priv->dev,
+				"RVU REP IRQ registration failed for CQ%d\n", qidx);
+			goto err_free_cints;
+		}
+		vec++;
+
+		/* Enable CQ IRQ */
+		otx2_write64(priv, NIX_LF_CINTX_INT(qidx), BIT_ULL(0));
+		otx2_write64(priv, NIX_LF_CINTX_ENA_W1S(qidx), BIT_ULL(0));
+	}
+	priv->flags &= ~OTX2_FLAG_INTF_DOWN;
+	return 0;
+
+err_free_cints:
+	otx2_free_cints(priv, qidx);
+	otx2_disable_napi(priv);
+	return err;
+}
+
+static void rvu_rep_free_cq_rsrc(struct otx2_nic *priv)
+{
+	struct otx2_cq_poll *cq_poll = NULL;
+	struct otx2_qset *qset = &priv->qset;
+	int qidx, vec;
+
+	/* Cleanup CQ NAPI and IRQ */
+	vec = priv->hw.nix_msixoff + NIX_LF_CINT_VEC_START;
+	for (qidx = 0; qidx < priv->hw.cint_cnt; qidx++) {
+		/* Disable interrupt */
+		otx2_write64(priv, NIX_LF_CINTX_ENA_W1C(qidx), BIT_ULL(0));
+
+		synchronize_irq(pci_irq_vector(priv->pdev, vec));
+
+		cq_poll = &qset->napi[qidx];
+		napi_synchronize(&cq_poll->napi);
+		vec++;
+	}
+	otx2_free_cints(priv, priv->hw.cint_cnt);
+	otx2_disable_napi(priv);
+}
+
+static void rvu_rep_free_netdev(struct otx2_nic *priv)
+{
+	struct rep_dev *rep;
+	int rep_id;
+
+	for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
+		rep = priv->reps[rep_id];
+		if (rep && rep->netdev->netdev_ops) {
+			unregister_netdev(rep->netdev);
+			free_netdev(rep->netdev);
+		}
+	}
+	devm_kfree(priv->dev, priv->reps);
+}
+
+void rvu_rep_destroy(struct otx2_nic *priv)
+{
+	rvu_rep_free_cq_rsrc(priv);
+	rvu_rep_free_netdev(priv);
+}
+
+int rvu_rep_create(struct otx2_nic *priv)
+{
+	int rep_cnt = priv->rep_cnt;
+	struct net_device *ndev;
+	struct rep_dev *rep;
+	int rep_id, err;
+	u16 pcifunc;
+
+	priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
+	if (!priv->reps)
+		return -ENOMEM;
+
+	for (rep_id = 0; rep_id < rep_cnt; rep_id++) {
+		ndev = alloc_etherdev(sizeof(*rep));
+		if (!ndev) {
+			dev_err(priv->dev, "PFVF representor:%d creation failed\n", rep_id);
+			err = -ENOMEM;
+			goto exit;
+		}
+
+		rep = netdev_priv(ndev);
+		priv->reps[rep_id] = rep;
+		rep->mdev = priv;
+		rep->netdev = ndev;
+		rep->rep_id = rep_id;
+
+		ndev->min_mtu = OTX2_MIN_MTU;
+		ndev->max_mtu = priv->hw.max_mtu;
+		pcifunc = priv->rep_pf_map[rep_id];
+		rep->pcifunc = pcifunc;
+
+		snprintf(ndev->name, sizeof(ndev->name), "r%dp%dv%d", rep_id,
+			 rvu_get_pf(pcifunc), (pcifunc & RVU_PFVF_FUNC_MASK));
+
+		eth_hw_addr_random(ndev);
+		if (register_netdev(ndev)) {
+			dev_err(priv->dev, "PFVF reprentator registration failed\n");
+			free_netdev(ndev);
+			ndev->netdev_ops = NULL;
+			goto exit;
+		}
+	}
+	err = rvu_rep_napi_init(priv);
+	if (err)
+		goto exit;
+
+	return 0;
+exit:
+	rvu_rep_free_netdev(priv);
+	return err;
+}
+
 static int rvu_rep_rsrc_free(struct otx2_nic *priv)
 {
 	struct otx2_qset *qset = &priv->qset;
@@ -162,6 +315,10 @@  static int rvu_rep_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (err)
 		goto err_detach_rsrc;
 
+	err = otx2_register_dl(priv);
+	if (err)
+		goto err_detach_rsrc;
+
 	return 0;
 
 err_detach_rsrc:
@@ -183,6 +340,8 @@  static void rvu_rep_remove(struct pci_dev *pdev)
 {
 	struct otx2_nic *priv = pci_get_drvdata(pdev);
 
+	otx2_unregister_dl(priv);
+	rvu_rep_destroy(priv);
 	rvu_rep_rsrc_free(priv);
 	otx2_detach_resources(&priv->mbox);
 	if (priv->hw.lmt_info)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
index 30cce17eb48b..be6c939e5cba 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
@@ -29,4 +29,6 @@  static inline bool is_rep_dev(struct pci_dev *pdev)
 	return pdev->device == PCI_DEVID_RVU_REP;
 }
 
+int rvu_rep_create(struct otx2_nic *priv);
+void rvu_rep_destroy(struct otx2_nic *priv);
 #endif /* REP_H */