Re: [Patch v2 net-next 1/7] octeontx2-af: forward error correction configuration

Hi Willem,

-----Original Message-----
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Sent: Thursday, January 28, 2021 1:45 AM
To: Hariprasad Kelam <redacted>
Cc: Network Development <redacted>; LKML <linux-
kernel@vger.kernel.org>; David Miller [off-list ref]; Jakub
Kicinski [off-list ref]; Sunil Kovvuri Goutham
[off-list ref]; Linu Cherian [off-list ref];
Geethasowjanya Akula [off-list ref]; Jerin Jacob Kollanukkaran
[off-list ref]; Subbaraya Sundeep Bhatta [off-list ref];
Christina Jacob [off-list ref]
Subject: [EXT] Re: [Patch v2 net-next 1/7] octeontx2-af: forward error
correction configuration

External Email

----------------------------------------------------------------------
On Wed, Jan 27, 2021 at 4:05 AM Hariprasad Kelam [off-list ref]
wrote:

quoted

From: Christina Jacob <redacted>

CGX block supports forward error correction modes baseR and RS. This
patch adds support to set encoding mode and to read
corrected/uncorrected block counters

Adds new mailbox handlers set_fec to configure encoding modes and
fec_stats to read counters and also increase mbox timeout to accomdate
firmware command response timeout.

Along with new CGX_CMD_SET_FEC command add other commands to

sync with

quoted

kernel enum list with firmware.

Signed-off-by: Christina Jacob <redacted>
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
Signed-off-by: Hariprasad Kelam <redacted>
---
 drivers/net/ethernet/marvell/octeontx2/af/cgx.c    | 74

++++++++++++++++++++++

quoted

 drivers/net/ethernet/marvell/octeontx2/af/cgx.h    |  7 ++
 .../net/ethernet/marvell/octeontx2/af/cgx_fw_if.h  | 17 ++++-
 drivers/net/ethernet/marvell/octeontx2/af/mbox.h   | 22 ++++++-
 .../net/ethernet/marvell/octeontx2/af/rvu_cgx.c    | 33 ++++++++++
 5 files changed, 151 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c

b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
index 84a9123..5489dab 100644

--- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c

@@ -340,6 +340,58 @@ int cgx_get_tx_stats(void *cgxd, int lmac_id, int

idx, u64 *tx_stat)

quoted

        return 0;
 }

+static int cgx_set_fec_stats_count(struct cgx_link_user_info *linfo)
+{
+       if (linfo->fec) {
+               switch (linfo->lmac_type_id) {
+               case LMAC_MODE_SGMII:
+               case LMAC_MODE_XAUI:
+               case LMAC_MODE_RXAUI:
+               case LMAC_MODE_QSGMII:
+                       return 0;
+               case LMAC_MODE_10G_R:
+               case LMAC_MODE_25G_R:
+               case LMAC_MODE_100G_R:
+               case LMAC_MODE_USXGMII:
+                       return 1;
+               case LMAC_MODE_40G_R:
+                       return 4;
+               case LMAC_MODE_50G_R:
+                       if (linfo->fec == OTX2_FEC_BASER)
+                               return 2;
+                       else
+                               return 1;
+               }
+       }
+       return 0;

may consider inverting the condition, to remove one level of indentation.

Agreed. Will fix in next version.

quoted

+int cgx_set_fec(u64 fec, int cgx_id, int lmac_id) {
+       u64 req = 0, resp;
+       struct cgx *cgx;
+       int err = 0;
+
+       cgx = cgx_get_pdata(cgx_id);
+       if (!cgx)
+               return -ENXIO;
+
+       req = FIELD_SET(CMDREG_ID, CGX_CMD_SET_FEC, req);
+       req = FIELD_SET(CMDSETFEC, fec, req);
+       err = cgx_fwi_cmd_generic(req, &resp, cgx, lmac_id);
+       if (!err) {
+               cgx->lmac_idmap[lmac_id]->link_info.fec =
+                       FIELD_GET(RESP_LINKSTAT_FEC, resp);
+               return cgx->lmac_idmap[lmac_id]->link_info.fec;
+       }
+       return err;

Prefer keeping the success path linear and return early if (err) in explicit
branch. This also aids branch prediction.

Agreed. Will fix this in next version.
 

quoted

+int rvu_mbox_handler_cgx_fec_stats(struct rvu *rvu,
+                                  struct msg_req *req,
+                                  struct cgx_fec_stats_rsp *rsp) {
+       int pf = rvu_get_pf(req->hdr.pcifunc);
+       u8 cgx_idx, lmac;
+       int err = 0;
+       void *cgxd;
+
+       if (!is_cgx_config_permitted(rvu, req->hdr.pcifunc))
+               return -EPERM;
+       rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_idx, &lmac);
+
+       cgxd = rvu_cgx_pdata(cgx_idx, rvu);
+       err = cgx_get_fec_stats(cgxd, lmac, rsp);
+       return err;

no need for variable err

Agreed will fix this in next version.

Thanks,
Hariprasad k