netdev - Re: [PATCH net-next 01/13] net: enetc: add initial netc-lib driver to support NTMP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <829ccd93-8b4e-4dd9-bd15-58d345797aca@lunn.ch>
Date: Fri, 3 Jan 2025 17:20:35 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, vladimir.oltean@....com, xiaoning.wang@....com,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, christophe.leroy@...roup.eu,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linuxppc-dev@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org,
	imx@...ts.linux.dev
Subject: Re: [PATCH net-next 01/13] net: enetc: add initial netc-lib driver
 to support NTMP

> +#define NTMP_FILL_CRD(crd, tblv, qa, ua) \
> +({ \
> +	typeof(crd) _crd = (crd); \
> +	(_crd)->update_act = cpu_to_le16(ua); \
> +	(_crd)->tblv_qact = NTMP_TBLV_QACT(tblv, qa); \
> +})
> +
> +#define NTMP_FILL_CRD_EID(req, tblv, qa, ua, eid) \
> +({ \
> +	typeof(req) _req = (req); \
> +	NTMP_FILL_CRD(&(_req)->crd, tblv, qa, ua); \
> +	(_req)->entry_id = cpu_to_le32(eid); \
> +})


These are pretty complex for #defines. Can they be made into
functions? That will get you type checking, finding bugs where
parameters are swapped.

> +int netc_setup_cbdr(struct device *dev, int cbd_num,
> +		    struct netc_cbdr_regs *regs,
> +		    struct netc_cbdr *cbdr)
> +{
> +	int size;
> +
> +	size = cbd_num * sizeof(union netc_cbd) + NTMP_BASE_ADDR_ALIGN;
> +
> +	cbdr->addr_base = dma_alloc_coherent(dev, size, &cbdr->dma_base,
> +					     GFP_KERNEL);
> +	if (!cbdr->addr_base)
> +		return -ENOMEM;
> +
> +	cbdr->dma_size = size;
> +	cbdr->bd_num = cbd_num;
> +	cbdr->regs = *regs;
> +
> +	/* The base address of the Control BD Ring must be 128 bytes aligned */
> +	cbdr->dma_base_align =  ALIGN(cbdr->dma_base,  NTMP_BASE_ADDR_ALIGN);
> +	cbdr->addr_base_align = PTR_ALIGN(cbdr->addr_base,
> +					  NTMP_BASE_ADDR_ALIGN);
> +
> +	cbdr->next_to_clean = 0;
> +	cbdr->next_to_use = 0;
> +	spin_lock_init(&cbdr->ring_lock);
> +
> +	/* Step 1: Configure the base address of the Control BD Ring */
> +	netc_write(cbdr->regs.bar0, lower_32_bits(cbdr->dma_base_align));
> +	netc_write(cbdr->regs.bar1, upper_32_bits(cbdr->dma_base_align));
> +
> +	/* Step 2: Configure the producer index register */
> +	netc_write(cbdr->regs.pir, cbdr->next_to_clean);
> +
> +	/* Step 3: Configure the consumer index register */
> +	netc_write(cbdr->regs.cir, cbdr->next_to_use);
> +
> +	/* Step4: Configure the number of BDs of the Control BD Ring */
> +	netc_write(cbdr->regs.lenr, cbdr->bd_num);
> +
> +	/* Step 5: Enable the Control BD Ring */
> +	netc_write(cbdr->regs.mr, NETC_CBDR_MR_EN);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netc_setup_cbdr);

I assume there is a version 3 in development, which will need a
different library, or at least different symbols. Maybe you should
think about the naming issues now?

> diff --git a/include/linux/fsl/ntmp.h b/include/linux/fsl/ntmp.h
> new file mode 100644
> index 000000000000..7cf322a1c8e3
> --- /dev/null
> +++ b/include/linux/fsl/ntmp.h
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/* Copyright 2025 NXP */
> +#ifndef __NETC_NTMP_H
> +#define __NETC_NTMP_H

Does this header need to be global? What else will use it outside of
drivers/net/ethernet/freescale/enetc?

	Andrew