| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200819083817.00000a02@intel.com>
Date: Wed, 19 Aug 2020 08:38:17 -0700
From: Jesse Brandeburg <jesse.brandeburg@...el.com>
To: <sundeep.lkml@...il.com>
Cc: <davem@...emloft.net>, <kuba@...nel.org>,
<richardcochran@...il.com>, <netdev@...r.kernel.org>,
<sgoutham@...vell.com>, Zyta Szpak <zyta@...vell.com>,
Subbaraya Sundeep <sbhatta@...vell.com>
Subject: Re: [PATCH v6 net-next 1/3] octeontx2-af: Support to enable/disable
HW timestamping
sundeep.lkml@...il.com wrote:
> From: Zyta Szpak <zyta@...vell.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@...vell.com>
> Signed-off-by: Subbaraya Sundeep <sbhatta@...vell.com>
> Signed-off-by: Sunil Goutham <sgoutham@...vell.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.
*/
> +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.
> + 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?
> 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.
Also, What's up with the magic number? At least you had a comment.
Powered by blists - more mailing lists