linux-kernel - Re: [net-next v5 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

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: <02baf961-b82d-4819-8791-229f7735a1a7@lunn.ch>
Date: Thu, 21 Aug 2025 14:41:09 +0200
From: Andrew Lunn <andrew@...n.ch>
To: David Yang <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next v5 3/3] net: dsa: yt921x: Add support for Motorcomm
 YT921x

> +#define should_unreachable() \
> +	pr_err("%s: !!unreachable %d, please report a bug!\n", \
> +	       __func__, __LINE__)
> +#define consume_retval(res) do { \
> +	if (unlikely(res)) \
> +		pr_err("%s: %i\n", __func__, (res)); \
> +} while (0)
> +

> +static int
> +yt921x_vid_del(struct yt921x_priv *priv, int port, u16 vid)
> +{
> +	struct yt921x_port *pp = &priv->ports[port];
> +	u32 mask;
> +	u32 ctrl;
> +	u32 val;
> +	int res;

...

> +
> +	if (pp->vids_cnt <= 0)
> +		should_unreachable();
> +	else
> +		pp->vids_cnt--;
> +	return 0;

Have you seen other drivers do this? If you are doing something which
other drivers don't do, it is probably wrong.

What you are more likely to see is WARN_ON(pp->vids_cnt <= 0); You
then get a stack trace, to help debug what happened. Kernel developers
know what WARN_ON() does, so it is easy to
understand. should_unreachable() is unique, it is unclear what it
does, making it harder to understand and review.

> +static void
> +yt921x_dsa_port_bridge_leave(struct dsa_switch *ds, int port,
> +			     struct dsa_bridge bridge)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	u16 ports_mask;
> +	int res;
> +
> +	ports_mask = dsa_bridge_ports(ds, bridge.dev);
> +
> +	dev_dbg(dev, "%s: port %d, mask 0x%x\n", __func__, port, ports_mask);
> +
> +	ports_mask |= priv->cpu_ports_mask;
> +
> +	yt921x_smi_acquire(priv);
> +	res = yt921x_bridge_force(priv, ports_mask);
> +	yt921x_smi_release(priv);
> +
> +	consume_retval(res);
> +}

And this is the same. Every driver you look at would just have the if
statement, not a macro.

In order to make drivers easy to maintain, easier to review, easy to
debug, they should all look similar.

	Andrew