| Message ID | 20240813-smatch-clock-v1-1-664c84295b1c@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Fix last smatch warnings | expand |
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@chromium.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. 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 >
Hi Dan On Tue, 13 Aug 2024 at 14:54, Dan Carpenter <dan.carpenter@linaro.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@chromium.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 > >
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); + 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; }
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@chromium.org> --- drivers/media/i2c/ar0521.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)