Re: [PATCH 2/3] media: i2c: Add Sony IMX681 sensor driver

[Sergey Lebedev]

Thank you - four of the five are real, and I have checked each against the
patch rather than agreeing on sight. All four are fixed in v2, which follows
this message. The author is away until 28 September, so v2 comes from me; where
a fix reverses something he wrote deliberately I have said so and given the
reasoning rather than leaving the question hanging for three weeks.

Confirmed
=========

1. devm_regulator_bulk_get() ignored. probe() logs a dev_dbg and continues, and
   imx681_power_on() then calls regulator_bulk_enable() on an array nothing
   filled in. Your use-after-free reading is the sharper one and it holds:
   _regulator_bulk_get() unwinds with "while (--i >= 0)
   regulator_put(consumers[i].consumer)" and leaves the pointers where they
   are, so a get that fails partway puts freed regulators into supplies[]
   rather than NULLs. The likely failure is -EPROBE_DEFER, which is ordinary.
   On this machine INT3472 hands out dummies so the get succeeds, which is
   exactly why testing did not find it. v2 makes it fatal, via dev_err_probe(),
   which is also the right handling for the -EPROBE_DEFER case.

2. pm_runtime_get_if_active(). It returns 1, 0, or a negative errno, and
   "if (!...)" only catches the 0. An error therefore falls through to issue
   I2C to a possibly-suspended sensor and then pm_runtime_put()s a reference it
   never took. v2 tests it as "if (pm_runtime_get_if_active(dev) <= 0)". The
   kerneldoc is explicit that on -EINVAL "the usage_count will remain
   unmodified", so the put is unbalanced and not merely redundant.

3. Missing pm_runtime_put_noidle() on the probe error path. The reference
   taken by pm_runtime_get_noresume() is never released, and
   pm_runtime_set_suspended() is called while it is still held. v2 adds it at
   error_pm.

Not a bug, and I think demonstrably so
======================================

The NULL-state dereference in imx681_set_pad_format(). The subdev core reaches
.set_fmt through v4l2_subdev_call_state_active(), which always supplies a
state, and the drivers around it agree: imx415, imx355, ov02c10 and ov08x40
all call v4l2_subdev_state_get_format(state, ...) with no NULL check between
them. If the core can hand a NULL state to .set_fmt then this is a
subsystem-wide problem rather than one driver's, and I would rather hear that
from a maintainer than paper over it here.

The gain clash, which is the interesting one
============================================

Correct, and worse than it looks. The analogue handler writes
IMX681_REG_DIGITAL_GAIN on *every* call, not only above code 960: above 960 it
writes the spillover it computed, and at or below 960 it writes the 1.0x it
initialised the local to. V4L2_CID_DIGITAL_GAIN writes the same register
directly. So setting digital gain to 4x and then moving analogue gain anywhere
at all puts 1x back into 0x020E - no error returned, and the DIGITAL_GAIN
control still reporting 4x.

The v1 cover asked whether the range should be split honestly between the two
controls. I have stopped asking and done it, because the two turn out to be one
question: clustering would stop the controls overwriting each other, but it
would then need an invented rule for what analogue 1020 together with digital
2x means, and there is no honest answer - they are two names for one register.
So v2 advertises ANALOGUE_GAIN as 0..960, the 1x..16x the analogue stage
actually does, and DIGITAL_GAIN owns 0x020E alone.

Measured rather than argued, by reading 0x020E over i2c while the sensor
streams. It has to be while streaming: at stream start
__v4l2_ctrl_handler_setup() applies the controls in creation order, analogue
before digital, so digital wins and the clash is invisible. It bites a live AE
loop, which is the case that matters.

                                     v1 module     v2 module
  start                              0x0100        0x0100
  after digital_gain = 4x            0x0400        0x0400
  after analogue_gain 500 -> 700     0x0100  <--   0x0400
  DIGITAL_GAIN control reads         1024          1024

Both modules were built from the same tree and swapped with rmmod/insmod, so
nothing but the driver differs between the columns. The last row is the point:
the control reports 4x in both, and in v1 the register disagrees with it
silently.

For Andre, on your return
=========================

Two of these touch choices that look deliberate in your code, so they are yours
to overrule.

The regulator handling. Your comment says "Continue without regulators -
INT3472 may handle power", and v2 makes that failure fatal instead. My reading
is that the get does not fail for supplies the firmware leaves undescribed - it
hands out dummies, which is where this machine's "supply dvdd not found, using
dummy regulator" comes from, and the camera still probes with the change in
place. If you wrote the comment because you had actually seen the get fail on
some machine, then I have removed a workaround you needed: say so and it goes
back in v3.

The gain range. I have split it rather than holding v2 for three weeks:
ANALOGUE_GAIN stops at 960 and no longer touches the digital register. Your own
define carried the comment "combined analog+digital", and V4L2 has no control
meaning combined gain, so I believe this is what the ABI asks for - but it is
your design and you can have it back in v3.

And one you did not ask for: the series lists you as M: for SONY IMX681 in
MAINTAINERS. That is the convention for a driver's author, but it is also a
standing obligation to everyone who reports a bug against it later, so it
should be your choice and not mine. It is already in v2 because leaving a new
driver with no maintainer entry is worse; say the word and it becomes me, or
both of us.

I ran an independent review of the same file before writing this, and it found
the same four and nothing else.

One thing worth saying plainly, since v1's cover letter listed its testing at
some length. That list - applies to media/next, correct authorship under git
am, checkpatch, dt_binding_check, a W=1 build, booted on the hardware with the
camera streaming - would not have found a single one of these four. They live
on error paths this board never takes and in a control interaction that needs
both controls driven while streaming. Testing that the happy path works is not
review, and I should not have been as satisfied with that list as I was.

Sergey