linux-kernel - Re: [PATCH v3 1/7] driver core: auxiliary bus: add device creation helpers

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: <1jikp8xx01.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 17 Feb 2025 19:10:54 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Dave Ertman <david.m.ertman@...el.com>,  Ira Weiny
 <ira.weiny@...el.com>,  "Rafael J. Wysocki" <rafael@...nel.org>,  Stephen
 Boyd <sboyd@...nel.org>,  Arnd Bergmann <arnd@...db.de>,  Danilo Krummrich
 <dakr@...nel.org>,  Conor Dooley <conor.dooley@...rochip.com>,  Daire
 McNamara <daire.mcnamara@...rochip.com>,  Philipp Zabel
 <p.zabel@...gutronix.de>,  Douglas Anderson <dianders@...omium.org>,
  Andrzej Hajda <andrzej.hajda@...el.com>,  Neil Armstrong
 <neil.armstrong@...aro.org>,  Robert Foss <rfoss@...nel.org>,  Laurent
 Pinchart <Laurent.pinchart@...asonboard.com>,  Jonas Karlman
 <jonas@...boo.se>,  Jernej Skrabec <jernej.skrabec@...il.com>,  Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>,  Maxime Ripard
 <mripard@...nel.org>,  Thomas Zimmermann <tzimmermann@...e.de>,  David
 Airlie <airlied@...il.com>,  Simona Vetter <simona@...ll.ch>,  Hans de
 Goede <hdegoede@...hat.com>,  Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>,  Bryan O'Donoghue
 <bryan.odonoghue@...aro.org>,  Vladimir Kondratiev
 <vladimir.kondratiev@...ileye.com>,  Gregory CLEMENT
 <gregory.clement@...tlin.com>,  Théo Lebrun
 <theo.lebrun@...tlin.com>,
  Michael Turquette <mturquette@...libre.com>,  Abel Vesa
 <abelvesa@...nel.org>,  Peng Fan <peng.fan@....com>,  Shawn Guo
 <shawnguo@...nel.org>,  Sascha Hauer <s.hauer@...gutronix.de>,
  Pengutronix Kernel Team <kernel@...gutronix.de>,  Fabio Estevam
 <festevam@...il.com>,  Kevin Hilman <khilman@...libre.com>,  Martin
 Blumenstingl <martin.blumenstingl@...glemail.com>,
  linux-kernel@...r.kernel.org,  linux-riscv@...ts.infradead.org,
  dri-devel@...ts.freedesktop.org,  platform-driver-x86@...r.kernel.org,
  linux-mips@...r.kernel.org,  linux-clk@...r.kernel.org,
  imx@...ts.linux.dev,  linux-arm-kernel@...ts.infradead.org,
  linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH v3 1/7] driver core: auxiliary bus: add device creation
 helpers

On Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:

[...]

>> 
>> >
>> >> +							int id)
>> >> +{
>> >> +	struct auxiliary_device *auxdev;
>> >> +	int ret;
>> >> +
>> >> +	auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
>> >> +	if (!auxdev)
>> >> +		return ERR_PTR(-ENOMEM);
>> >
>> > Ick, who cares what the error value really is?  Why not just do NULL or
>> > a valid pointer?  That makes the caller much simpler to handle, right?
>> >
>> 
>> Sure why not

I have tried the 'NULL or valid' approach. In the consumers,
which mostly return an integer from their various init function, I got
this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't
really pick one.

It is actually easier to pass something along.

>> 
>> >> +
>> >> +	auxdev->id = id;
>> >> +	auxdev->name = devname;
>> >> +	auxdev->dev.parent = dev;
>> >> +	auxdev->dev.platform_data = platform_data;
>> >> +	auxdev->dev.release = auxiliary_device_release;
>> >> +	device_set_of_node_from_dev(&auxdev->dev, dev);
>> >> +
>> >> +	ret = auxiliary_device_init(auxdev);
>> >
>> > Only way this will fail is if you forgot to set parent or a valid name.
>> > So why not check for devname being non-NULL above this?
>> 
>> If auxiliary_device_init() ever changes it would be easy to forget to
>> update that and lead to something nasty to debug, don't you think ?
>
> Yes, this is being more defensive, I take back my objection, thanks.
>
>> >> +	if (ret) {
>> >> +		kfree(auxdev);
>> >> +		return ERR_PTR(ret);
>> >> +	}
>> >> +
>> >> +	ret = __auxiliary_device_add(auxdev, modname);
>> >> +	if (ret) {
>> >> +		/*
>> >> +		 * NOTE: It may look odd but auxdev should not be freed
>> >> +		 * here. auxiliary_device_uninit() calls device_put()
>> >> +		 * which call the device release function, freeing auxdev.
>> >> +		 */
>> >> +		auxiliary_device_uninit(auxdev);
>> >
>> > Yes it is odd, are you SURE you should be calling device_del() on the
>> > device if this fails?  auxiliary_device_uninit(), makes sense so why not
>> > just call that here?
>> 
>> I'm confused ... I am call auxiliary_device_uninit() here. What do you
>> mean ? 
>
> Oh wow, I got this wrong, sorry, I was thinking you were calling
> auxiliary_device_destroy().  Nevermind, ugh, it was a long day...
>

No worries

> thanks,
>
> greg k-h

-- 
Jerome