Date:   Wed, 22 Dec 2021 08:44:44 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Johan Hovold <johan@kernel.org>,
        Andrey Smirnov <andrew.smirnov@gmail.com>,
        linux-kernel@vger.kernel.org,
        =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
Subject: Re: [PATCH] nvmem: fix unregistering device in nvmem_register()
 error path
Message-ID: <YcLXbPzyhtMnP0YQ@kroah.com>
References: <20211221154550.11455-1-zajec5@gmail.com>
 <YcH7fw5S6aSXswvb@kroah.com>
 <9e94f0fd-e2d5-4d9e-5759-a5f591191785@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9e94f0fd-e2d5-4d9e-5759-a5f591191785@gmail.com>
Precedence: bulk

On Tue, Dec 21, 2021 at 06:46:01PM +0100, Rafał Miłecki wrote:
> On 21.12.2021 17:06, Greg Kroah-Hartman wrote:
> > On Tue, Dec 21, 2021 at 04:45:50PM +0100, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > 1. Drop incorrect put_device() calls
> > > 
> > > If device_register() fails then underlaying device_add() takes care of
> > > calling put_device() if needed. There is no need to do that in a driver.
> > 
> > Did you read the documentation for device_register() that says:
> > 
> >   * NOTE: _Never_ directly free @dev after calling this function, even
> >   * if it returned an error! Always use put_device() to give up the
> >   * reference initialized in this function instead.
> 
> I clearly tried to be too smart and ignored documentation.
> 
> I'd say device_add() behaviour is rather uncommon and a bit unintuitive.
> Most kernel functions are safe to assume to do nothing that requires
> cleanup if they fail.
> 
> E.g. if I call platform_device_register() and it fails I don't need to
> call anything like platform_device_put(). I just free previously
> allocated memory.

And that is wrong.

{sigh}

Seems the author of that function did not read the documentation.  I'll
add "fix platform_device_register()" to my long TODO list.  Arguably, it
should handle this type of failure internally to it, to prevent all
individual drivers from having to handle it.

You also need to handle this type of functionality in your bus call, and
you do, which is good as you do not want everyone who calls
nvmem_register() to also have to do much the same thing.

> When calling device_register() / device_add() it seems device always
> gets partially registered (even if it fails!). Enough to make it safe to
> depend on core subsystem calling .release() after device_put().
> 
> So what initially looks like unbalanced device_put() call is actually
> some device_add() specific magic behaviour ;)

It's documented magic behavior :)

thanks,

greg k-h