RE: [EXTERNAL] Re: [net-next PATCH v2 3/9] octeontx2-pf: Create representor netdev

From: Geethasowjanya Akula
Date: Tue Apr 23 2024 - 11:27:04 EST




> -----Original Message-----
> From: Jiri Pirko <jiri@xxxxxxxxxxx>
> Sent: Monday, April 22, 2024 10:34 PM
> To: Geethasowjanya Akula <gakula@xxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> davem@xxxxxxxxxxxxx; pabeni@xxxxxxxxxx; edumazet@xxxxxxxxxx; Sunil
> Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [net-next PATCH v2 3/9] octeontx2-pf: Create
> representor netdev
> ----------------------------------------------------------------------
> Mon, Apr 22, 2024 at 11:53:55AM CEST, gakula@xxxxxxxxxxx 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@xxxxxxxxxxx>
> >---
> > .../marvell/octeontx2/nic/otx2_devlink.c | 47 ++++++
> > .../net/ethernet/marvell/octeontx2/nic/rep.c | 154 ++++++++++++++++++
> > .../net/ethernet/marvell/octeontx2/nic/rep.h | 2 +
> > 3 files changed, 203 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..60156c7ebe45 100644
> >--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> >+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c
> >@@ -76,7 +76,52 @@ 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;
> >+
> >+ switch (mode) {
> >+ case DEVLINK_ESWITCH_MODE_LEGACY:
> >+ rvu_rep_destroy(pfvf);
> >+ break;
> >+ case DEVLINK_ESWITCH_MODE_SWITCHDEV:
> >+ rvu_rep_create(pfvf);
>
>
> rvu_rep_create() returns int. Return that from this function. Also, pass extack
> and let rvu_rep_create() fill up the message in case of any error.
Ack.
>
>
> >+ break;
> >+ default:
> >+ return -EINVAL;
> >+ }
> >+ pfvf->esw_mode = mode;
> >+ 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 +157,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 +169,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 0e19b938d197..e2571728cb0d 100644
> >--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> >+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> >@@ -28,6 +28,154 @@ 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);
> >+}
> >+
> >+void rvu_rep_destroy(struct otx2_nic *priv) {
> >+ struct rep_dev *rep;
> >+ int rep_id;
> >+
> >+ rvu_rep_free_cq_rsrc(priv);
> >+ for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
> >+ rep = priv->reps[rep_id];
> >+ unregister_netdev(rep->netdev);
> >+ free_netdev(rep->netdev);
> >+ }
> >+}
> >+
> >+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);
>
> Please avoid dmesg prints like this, use extack instead.
> Please follow this with the functions you call, like rvu_rep_napi_init()
>
Ack. Will address it in the next version.
>
> >+ 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%d", rep_id,
> >+ rvu_get_pf(pcifunc));
> >+
> >+ eth_hw_addr_random(ndev);
> >+ err = register_netdev(ndev);
> >+ if (err) {
> >+ dev_err(priv->dev, "PFVF reprentator registration
> failed\n");
> >+ goto exit;
> >+ }
> >+ }
> >+ err = rvu_rep_napi_init(priv);
> >+ if (err)
> >+ goto exit;
> >+
> >+ return 0;
> >+exit:
> >+ while (--rep_id >= 0) {
> >+ rep = priv->reps[rep_id];
> >+ unregister_netdev(rep->netdev);
> >+ free_netdev(rep->netdev);
> >+ }
> >+ return err;
> >+}
> >+
> > static int rvu_rep_rsrc_free(struct otx2_nic *priv) {
> > struct otx2_qset *qset = &priv->qset; @@ -163,6 +311,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:
> >@@ -184,6 +336,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
> >
> >