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

sundeep.lkml@gmail.com wrote:

From: Zyta Szpak <redacted>

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 <redacted>
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.

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?

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.