netdev - Re: [RFC net-next 01/15] net: cpc: add base skeleton driver

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: <17a103da-b01a-44cc-b1e4-5a4f606ed4a8@lunn.ch>
Date: Mon, 12 May 2025 04:13:19 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Damien Riégel <damien.riegel@...abs.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"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>,
	Silicon Labs Kernel Team <linux-devel@...abs.com>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next 01/15] net: cpc: add base skeleton driver

> +/**
> + * cpc_interface_register() - Register CPC interface.
> + * @intf: CPC device to register.
> + *
> + * Context: Process context.
> + *
> + * Return: 0 if successful, otherwise a negative error code.
> + */
> +int cpc_interface_register(struct cpc_interface *intf)
> +{
> +	int err;
> +
> +	err = device_add(&intf->dev);
> +	if (err)
> +		return err;
> +
> +	return 0;

I guess this will change in a later patch, but maybe not. This should
just be

	return device_add(&intf->dev);

> +}
> +
> +/**
> + * cpc_interface_unregister() - Unregister a CPC interface.
> + * @intf: CPC device to unregister.
> + *
> + * Context: Process context.
> + */
> +void cpc_interface_unregister(struct cpc_interface *intf)
> +{
> +	device_del(&intf->dev);
> +	cpc_interface_put(intf);
> +}

It seems odd that unregister is not a mirror of register?

> +/**
> + * cpc_interface_get() - Get a reference to interface and return its pointer.
> + * @intf: Interface to get.
> + *
> + * Return: Interface pointer with its reference counter incremented, or %NULL.
> + */
> +static inline struct cpc_interface *cpc_interface_get(struct cpc_interface *intf)
> +{
> +	if (!intf || !get_device(&intf->dev))
> +		return NULL;
> +	return intf;
> +}

What is the use case for passing in NULL?

> +
> +/**
> + * cpc_interface_put() - Release reference to an interface.
> + * @intf: CPC interface
> + *
> + * Context: Process context.
> + */
> +static inline void cpc_interface_put(struct cpc_interface *intf)
> +{
> +	if (intf)
> +		put_device(&intf->dev);
> +}
> +
> +/**
> + * cpc_interface_get_priv() - Get driver data associated with this interface.
> + * @intf: Interface pointer.
> + *
> + * Return: Driver data, set at allocation via cpc_interface_alloc().
> + */
> +static inline void *cpc_interface_get_priv(struct cpc_interface *intf)
> +{
> +	if (!intf)
> +		return NULL;
> +	return dev_get_drvdata(&intf->dev);
> +}

What is the use case for passing in NULL?

To me, this is hiding bugs. It seems better to let the kernel opp so
you find out where you lost your intf.

	Andrew