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

[Andrew Lunn]

> +#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