Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 28 Aug 2022 17:22:58 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc:     Shreeya Patel <shreeya.patel@collabora.com>, lars@metafoo.de,
        krisman@collabora.com, kernel@collabora.com,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: light: tsl2583: Fix module unloading
Message-ID: <20220828172258.4a78152f@jic23-huawei>
In-Reply-To: <793d9ca8-47af-2f3d-6079-e0bbb8e7d898@collabora.com>
References: <20220825092048.136973-1-shreeya.patel@collabora.com>
        <793d9ca8-47af-2f3d-6079-e0bbb8e7d898@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 25 Aug 2022 21:53:09 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> Hi,
> 
> On 8/25/22 12:20, Shreeya Patel wrote:
> > tsl2583 uses devm_iio_device_register() function and
> > calling iio_device_unregister() in remove breaks the
> > module unloading.
> > Fix this by removing call to iio_device_unregister()
> > from tsl2583_remove().
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>  
> 
> Stable and fixes tags are missing

Stable tags often added by maintainer when applying the patch
rather than at submission: can take into account how they want to
manage stable backports - some maintainers prefer to run a bunch
of tests before asking for patches to be added to stable. I don't
do that, but I don't always tag fixes for stable if I think there
is some risk associated.

Obviously this one is fine for stable material though and I would
have tagged v2 whilst applying ;)
> 
> > ---
> >  drivers/iio/light/tsl2583.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c
> > index 82662dab87c0..36c25f79e6a6 100644
> > --- a/drivers/iio/light/tsl2583.c
> > +++ b/drivers/iio/light/tsl2583.c
> > @@ -878,8 +878,6 @@ static int tsl2583_remove(struct i2c_client *client)
> >  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> >  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> >  
> > -	iio_device_unregister(indio_dev);
> > -
> >  	pm_runtime_disable(&client->dev);
> >  	pm_runtime_set_suspended(&client->dev);
> >    
> 
> Driver removal sequence should be opposite to the registration order.
> Could be better not to use the devm in this case

Agreed. Simplest fix is indeed to do what v2 does.

Nicer tidy up after that is perhaps to switch to
devm_pm_runtime_enable()
which tidies up nicely on that and register a
separate callback with devm_add_action_or_reset() to deal with
the power down.  However, I'm struggling to follow how the device
is powered up in the first place (particularly if we disable runtime
pm to make the flow simpler). 

I think the driver may only "work" today by merit of letting it 
autosuspend then resuming. 

Jonathan


>