[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiDSCu6BUdJt1sjmbOq2s4KU1JUN9D+HfnvSG6OqMhD-dKekw@mail.gmail.com>
Date: Tue, 13 Aug 2024 15:01:37 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Krzysztof HaĆasa <khalasa@...p.pl>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Sylwester Nawrocki <s.nawrocki@...sung.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Kevin Hilman <khilman@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-staging@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/6] media: ar0521: Refactor ar0521_power_off()
Hi Dan
On Tue, 13 Aug 2024 at 14:54, Dan Carpenter <dan.carpenter@...aro.org> wrote:
>
> On Tue, Aug 13, 2024 at 12:13:48PM +0000, Ricardo Ribalda wrote:
> > Factor out all the power off logic, except the clk_disable_unprepare(),
> > to a new function __ar0521_power_off().
> >
> > This allows ar0521_power_on() to explicitly clean-out the clock during
> > the error-path.
> >
> > The following smatch warning is fixed:
> > drivers/media/i2c/ar0521.c:912 ar0521_power_on() warn: 'sensor->extclk' from clk_prepare_enable() not released on lines: 912.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
>
> It's better to just ignore false positives... The problem here is that
> Smatch can't track that to_ar0521_dev(dev_get_drvdata(dev))->sensor is the same
> as sensor. What I could do is say that "this frees *something* unknown" so
> let's silence the warning."
>
> The problem is that check_unwind.c is not very granular. It just marks things
> as allocated and freed. I could make it more granular so the free and the
> alloc have to match. Or we could match based on the type. This frees a
> "struct ar0521_dev" so mark all those as freed in the caller.
>
> > drivers/media/i2c/ar0521.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index 09331cf95c62..2c528db31ba6 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -835,7 +835,7 @@ static const struct initial_reg {
> > be(0x0707)), /* 3F44: couple k factor 2 */
> > };
> >
> > -static int ar0521_power_off(struct device *dev)
> > +static void __ar0521_power_off(struct device *dev)
> > {
> > struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > @@ -850,6 +850,16 @@ static int ar0521_power_off(struct device *dev)
> > if (sensor->supplies[i])
> > regulator_disable(sensor->supplies[i]);
> > }
> > +}
> > +
> > +static int ar0521_power_off(struct device *dev)
> > +{
> > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > + struct ar0521_dev *sensor = to_ar0521_dev(sd);
> > +
> > + clk_disable_unprepare(sensor->extclk);
> > + __ar0521_power_off(dev);
>
> You had intended to remove the clk_disable_unprepare() from __ar0521_power_off()
> but forgot so these are double unprepares.
Ups, thanks for catching it :)
Will send as v2, unless you fix it in smatch first ;P
Regards!
>
> regards,
> dan carpenter
>
> > +
> > return 0;
> > }
> >
> > @@ -908,7 +918,8 @@ static int ar0521_power_on(struct device *dev)
> >
> > return 0;
> > off:
> > - ar0521_power_off(dev);
> > + clk_disable_unprepare(sensor->extclk);
> > + __ar0521_power_off(dev);
> > return ret;
> > }
> >
> >
> > --
> > 2.46.0.76.ge559c4bf1a-goog
> >
--
Ricardo Ribalda
Powered by blists - more mailing lists