netdev - Re: [PATCH net-next 3/3] net: airoha: Introduce ethernet support for EN7581 SoC

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]

Date: Sun, 2 Jun 2024 19:38:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: netdev@...r.kernel.org, nbd@....name, lorenzo.bianconi83@...il.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, conor@...nel.org,
	linux-arm-kernel@...ts.infradead.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	devicetree@...r.kernel.org, catalin.marinas@....com,
	will@...nel.org, upstream@...oha.com,
	angelogioacchino.delregno@...labora.com,
	benjamin.larsson@...exis.eu
Subject: Re: [PATCH net-next 3/3] net: airoha: Introduce ethernet support for
 EN7581 SoC

> +static int airoha_set_gdma_port(struct airoha_eth *eth, int port, bool enable)
> +{
> +	u32 vip_port, cfg_addr, val = enable ? FE_DP_PPE : FE_DP_DROP;
> +
> +	switch (port) {
> +	case 0:
> +		vip_port = BIT(22);
> +		cfg_addr = REG_GDM3_FWD_CFG;
> +		break;
> +	case 1:
> +		vip_port = BIT(23);
> +		cfg_addr = REG_GDM3_FWD_CFG;
> +		break;
> +	case 2:
> +		vip_port = BIT(25);
> +		cfg_addr = REG_GDM4_FWD_CFG;
> +		break;
> +	case 4:
> +		vip_port = BIT(24);
> +		cfg_addr = REG_GDM4_FWD_CFG;
> +		break;

Please add some #defines for the BIT(), so there is descriptive
names. Please do the same other places you have BIT macros, it makes
the code easier to understand.

> +static int airoha_set_gdma_ports(struct airoha_eth *eth, bool enable)
> +{
> +	const int port_list[] = { 0, 1, 2, 4 };
> +	int i;

Maybe add a comment about port 3?

> +static void airoha_fe_vip_setup(struct airoha_eth *eth)
> +{
> +	airoha_fe_wr(eth, REG_FE_VIP_PATN(3), 0x8863); /* ETH->PPP (0x8863) */

Rather than a comment, use ETH_P_PPP_DISC

> +	airoha_fe_wr(eth, REG_FE_VIP_EN(3), PATN_FCPU_EN_MASK | PATN_EN_MASK);
> +
> +	airoha_fe_wr(eth, REG_FE_VIP_PATN(4), 0xc021); /* PPP->LCP (0xc021) */

PPP_LCP

> +	airoha_fe_wr(eth, REG_FE_VIP_EN(4),
> +		     PATN_FCPU_EN_MASK | FIELD_PREP(PATN_TYPE_MASK, 1) |
> +		     PATN_EN_MASK);
> +
> +	airoha_fe_wr(eth, REG_FE_VIP_PATN(6), 0x8021); /* PPP->IPCP (0x8021) */

PPP_IPCP

etc...

> +static int airoha_qdma_fill_rx_queue(struct airoha_queue *q)
> +{
> +	struct airoha_eth *eth = q->eth;
> +	struct device *dev = eth->net_dev->dev.parent;
> +	int qid = q - &eth->q_rx[0], nframes = 0;

Reverse Christmass tree. Which means you will need to move some of the
assignments into the body of the function.

> +static int airoha_dev_open(struct net_device *dev)
> +{
> +	struct airoha_eth *eth = netdev_priv(dev);
> +	int err;
> +
> +	if (netdev_uses_dsa(dev))
> +		airoha_fe_set(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);
> +	else
> +		airoha_fe_clear(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);

Does this imply the hardware can be used in a situation where it is
not connected to a switch? Does it have an MII and MDIO bus? Could a
PHY be connected? If it can be used as a conventional NIC, we need to
ensure there is a path to use usage without an ABI breakage.

> +static int airoha_register_debugfs(struct airoha_eth *eth)
> +{
> +	eth->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +	if (IS_ERR(eth->debugfs_dir))
> +		return PTR_ERR(eth->debugfs_dir);

No error checking should be performed with debugfs calls. Just keep
going and it will work out O.K.

> +	err = of_get_ethdev_address(np, dev);
> +	if (err) {
> +		if (err == -EPROBE_DEFER)
> +			return err;
> +
> +		eth_hw_addr_random(dev);
> +		dev_err(&pdev->dev, "generated random MAC address %pM\n",
> +			dev->dev_addr);

dev_info() would be better here, since it is not considered an error.

> +	err = airoha_hw_init(eth);
> +	if (err)
> +		return err;
> +
> +	airoha_qdma_start_napi(eth);
> +	err = register_netdev(dev);
> +	if (err)
> +		return err;
> +
> +	err = airoha_register_debugfs(eth);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, eth);

Is this required? As soon as you call register_netdev(), the device is
live and in use. It can be sending the first packets before the
function returns. So if anything needs this connection between the
platform data and the eth, it will not be in place, and bad things
will happen.

> +static inline void airoha_qdma_start_napi(struct airoha_eth *eth)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(eth->q_tx_irq); i++)
> +		napi_enable(&eth->q_tx_irq[i].napi);
> +
> +	airoha_qdma_for_each_q_rx(eth, i)
> +		napi_enable(&eth->q_rx[i].napi);
> +}
> +
> +static inline void airoha_qdma_stop_napi(struct airoha_eth *eth)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(eth->q_tx_irq); i++)
> +		napi_disable(&eth->q_tx_irq[i].napi);
> +
> +	airoha_qdma_for_each_q_rx(eth, i)
> +		napi_disable(&eth->q_rx[i].napi);
> +}

These seem off to be in a header file?

    Andrew

---
pw-bot: cr