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

[Sergey Lebedev]

Correct, and it is a regression I introduced between v1 and v2. The v2 cover
letter also offered to spread the same mistake across six more drivers, so
that offer needs withdrawing before anything else.

Withdrawing the cleanup offer
=============================

The cover said the "!" form is in gc05a2, gc08a3, imx283, lm3560, og0ve1b and
ov6211 and offered a cleanup converting them. Please do not take that up, and
if anyone has begun, stop: converting them to "<= 0" would break each of them
exactly as it broke this driver.

The reasoning behind the offer was wrong in a specific way worth naming. I
compared against i915, xe, ipa, ivpu and arm-smmu, which do test "> 0" or
"<= 0" - but they sit where CONFIG_PM is effectively mandatory, so the third
return never reaches them. drivers/media/i2c builds without PM, and that is
the whole of the difference.

The finding
===========

Without CONFIG_PM the helper is, unconditionally:

  static inline int pm_runtime_get_if_active(struct device *dev)
  {
          return -EINVAL;
  }

v1's "if (!...)" fell through on that and applied the control, which is the
right thing to do: with no runtime PM the sensor is powered from probe and
never suspended. v2's "<= 0" returns 0 instead and drops it - exposure, gain,
blanking, test pattern, all of them, with nothing reported to the caller.

v3 will use the shape ov64a40 already has:

          pm_status = pm_runtime_get_if_active(imx681->dev);
          if (!pm_status)
                  return 0;
          ...
          if (pm_status > 0)
                  pm_runtime_put(imx681->dev);

0 skips, 1 applies and puts, -EINVAL applies and does not put. That answers the
first review's finding - the put of a reference never taken - without losing
the case v1 had right by accident.

ov64a40 is the only fully correct caller of this helper in drivers/media/i2c.
I looked at it when the first review landed, wrote it down as a variant of the
same thing, and did not read its second half. Had I done so, v2 would have
carried this.

If a cleanup for the other six is still wanted, that is the shape it should
take, and I will send one only if a maintainer asks.

v3 follows this message. Respinning within hours is not something I would
normally do, but nobody has reviewed v2 yet and the alternative is asking
people to read code I already know is wrong. The change is ten lines, six of
them comment, and 1/3 and 3/3 are byte-identical to v2.

The other finding, on 3/3, which is not ours
============================================

The same run flagged a High issue in ipu-bridge and marked it pre-existing. It
is - our 3/3 is two lines of table entry - but I checked it rather than waving
it past, and it looks real. Sakari, Dan: this is yours rather than mine.

The reference. sensor->adev = ACPI_PTR(acpi_dev_get(adev)) takes a second
reference to the ACPI device. On "goto err_free_swnodes" only the iterator's is
dropped, by acpi_dev_put(adev), and bridge->n_sensors has not been incremented
yet, so ipu_bridge_unregister_sensors() does not walk this sensor either.
Narrow in practice: ipu_bridge_instantiate_ivsc() returns 0 immediately when
there is no csi_dev, so reaching it needs a machine with an IVSC.

The fwnode pointers look wider to me. primary->secondary is assigned in
ipu_bridge_connect_sensor(), and set_secondary_fwnode(sensor->csi_dev, fwnode)
in ipu_bridge_instantiate_ivsc(). Neither is cleared anywhere in the file,
while ipu_bridge_unregister_sensors() calls
software_node_unregister_node_group() on the nodes they point at. The file
walks that chain itself, in ipu_bridge_check_fwnode_graph(fwnode->secondary).
That is the ordinary unbind path rather than an error path.

No patch from me for either: I have no IVSC machine and no way to force the
failure, and an untested lifetime change in someone else's driver is worth
less than a clear report.

Sergey