Re: [PATCH v6 net-next 1/3] octeontx2-af: Support to enable/disable HW timestamping

[sundeep subbaraya <sundeep.lkml@gmail.com>]

Hi,

On Wed, Aug 19, 2020 at 9:08 PM Jesse Brandeburg
<jesse.brandeburg@intel.com> wrote:
>
> sundeep.lkml@gmail.com wrote:
>
> > From: Zyta Szpak <zyta@marvell.com>
> >
> > Four new mbox messages ids and handler are added in order to
> > enable or disable timestamping procedure on tx and rx side.
> > Additionally when PTP is enabled, the packet parser must skip
> > over 8 bytes and start analyzing packet data there. To make NPC
> > profiles work seemlesly PTR_ADVANCE of IKPU is set so that
> > parsing can be done as before when all data pointers
> > are shifted by 8 bytes automatically.
> >
> > Signed-off-by: Zyta Szpak <zyta@marvell.com>
> > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
> > Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
>
>
> I know these patches are already acked by a couple of people in v4, but
> I have a few more minor concerns that I'd like you to consider listed
> below. Up to DaveM whether he wants to apply without the fixes I
> mention.
>
>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/af/cgx.c    | 29 ++++++++++++
> >  drivers/net/ethernet/marvell/octeontx2/af/cgx.h    |  4 ++
> >  drivers/net/ethernet/marvell/octeontx2/af/mbox.h   |  4 ++
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu.h    |  1 +
> >  .../net/ethernet/marvell/octeontx2/af/rvu_cgx.c    | 54 ++++++++++++++++++++++
> >  .../net/ethernet/marvell/octeontx2/af/rvu_nix.c    | 52 +++++++++++++++++++++
> >  .../net/ethernet/marvell/octeontx2/af/rvu_npc.c    | 27 +++++++++++
> >  7 files changed, 171 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> > index a4e65da..8f17e26 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> > @@ -468,6 +468,35 @@ static void cgx_lmac_pause_frm_config(struct cgx *cgx, int lmac_id, bool enable)
> >       }
> >  }
> >
>
> Generally what I'd like to see is that you have a comment here in
> kernel doc format, I suppose your driver probably doesn't have any of
> these, but it is particularly important to describe what each function
> is meant to do especially when it is a symbol callable from other
> files/modules. Something like:
>
> /**
>  * cgx_lmac_ptp_config - enable or disable timestamping
>  * @cgxd: driver context
>  * @lmac_id: ID used to get register offset
>  * @enable: true if timestamping should be enabled, false if not
>  *
>  * Here would be a multi-line description of what this function does
>  * and if it has a return value, what it's for.
>  */
>
I agree but we have lot of non static functions because of mbox handlers
and adding kernel doc for all those didn't look good. We try best to use
proper function names.

> > +void cgx_lmac_ptp_config(void *cgxd, int lmac_id, bool enable)
> > +{
> > +     struct cgx *cgx = cgxd;
> > +     u64 cfg;
> > +
> > +     if (!cgx)
> > +             return;
> > +
> <snip>
>
> > +int rvu_mbox_handler_nix_lf_ptp_tx_enable(struct rvu *rvu, struct msg_req *req,
> > +                                       struct msg_rsp *rsp)
> > +{
> > +     struct rvu_hwinfo *hw = rvu->hw;
> > +     u16 pcifunc = req->hdr.pcifunc;
> > +     struct rvu_block *block;
> > +     int blkaddr;
> > +     int nixlf;
> > +     u64 cfg;
> > +
> > +     blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
> > +     if (blkaddr < 0)
> > +             return NIX_AF_ERR_AF_LF_INVALID;
> > +
> > +     block = &hw->block[blkaddr];
> > +     nixlf = rvu_get_lf(rvu, block, pcifunc, 0);
> > +     if (nixlf < 0)
> > +             return NIX_AF_ERR_AF_LF_INVALID;
> > +
> > +     cfg = rvu_read64(rvu, blkaddr, NIX_AF_LFX_TX_CFG(nixlf));
> > +     cfg |= BIT_ULL(32);
>
> I'm not super excited by the magic numbers here, without even a
> comment, you should make a define for bit 32, and not leave me guessing
> if this is the "enable" bit or is for something else.
>
Because of the huge number of registers and bit definitions we are
avoiding adding macros for simpler cases like the one above.

> > +     rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_CFG(nixlf), cfg);
> > +
> > +     return 0;
> > +}
> > +
> > +int rvu_mbox_handler_nix_lf_ptp_tx_disable(struct rvu *rvu, struct msg_req *req,
> > +                                        struct msg_rsp *rsp)
> > +{
> > +     struct rvu_hwinfo *hw = rvu->hw;
> > +     u16 pcifunc = req->hdr.pcifunc;
> > +     struct rvu_block *block;
> > +     int blkaddr;
> > +     int nixlf;
> > +     u64 cfg;
> > +
> > +     blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
> > +     if (blkaddr < 0)
> > +             return NIX_AF_ERR_AF_LF_INVALID;
> > +
> > +     block = &hw->block[blkaddr];
> > +     nixlf = rvu_get_lf(rvu, block, pcifunc, 0);
> > +     if (nixlf < 0)
> > +             return NIX_AF_ERR_AF_LF_INVALID;
> > +
> > +     cfg = rvu_read64(rvu, blkaddr, NIX_AF_LFX_TX_CFG(nixlf));
> > +     cfg &= ~BIT_ULL(32);
> > +     rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_CFG(nixlf), cfg);
> > +
> > +     return 0;
> > +}
> > +
>
> Is this and the function above exactly the same 20+ lines of code
> with a one line difference? Before you passed an "enable" bool to
> another function, why the difference here?
>
Agreed. I will modify it.

> >  int rvu_mbox_handler_nix_lso_format_cfg(struct rvu *rvu,
> >                                       struct nix_lso_format_cfg *req,
> >                                       struct nix_lso_format_cfg_rsp *rsp)
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > index 0a21408..8179bbe 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > @@ -27,6 +27,7 @@
> >  #define NIXLF_PROMISC_ENTRY  2
> >
> >  #define NPC_PARSE_RESULT_DMAC_OFFSET 8
> > +#define NPC_HW_TSTAMP_OFFSET         8
> >
> >  static void npc_mcam_free_all_entries(struct rvu *rvu, struct npc_mcam *mcam,
> >                                     int blkaddr, u16 pcifunc);
> > @@ -61,6 +62,32 @@ int rvu_npc_get_pkind(struct rvu *rvu, u16 pf)
> >       return -1;
> >  }
> >
> > +int npc_config_ts_kpuaction(struct rvu *rvu, int pf, u16 pcifunc, bool en)
> > +{
> > +     int pkind, blkaddr;
> > +     u64 val;
> > +
> > +     pkind = rvu_npc_get_pkind(rvu, pf);
> > +     if (pkind < 0) {
> > +             dev_err(rvu->dev, "%s: pkind not mapped\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, pcifunc);
> > +     if (blkaddr < 0) {
> > +             dev_err(rvu->dev, "%s: NPC block not implemented\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +     val = rvu_read64(rvu, blkaddr, NPC_AF_PKINDX_ACTION0(pkind));
> > +     val &= ~0xff00000ULL; /* Zero ptr advance field */
>
> Please don't use trailing comments *ever* in a code section, the only
> place it is marginally ok, is in structure definitions.
>
Okay will fix it.
> Also, What's up with the magic number? At least you had a comment.
>
>
Sure will fix it.

Thanks,
Sundeep