RE: [PATCH v17 17/22] media: i2c: maxim-serdes: add MAX96717 driver

[Dayananda, Vivekananda]

  Hi Dumitru,

  I found an issue with serializer teardown during Device Tree overlay removal while testing a downstream setup based on
  this series.

  My setup is a KV260 with a MAX96724 deserializer connected to a Hawk stereo camera. The camera has two AR0234 sensors
  behind one MAX9295D serializer. The sensors and two EEPROMs are described under the serializer’s i2c-gate node, using
  distinct addresses: 0x10, 0x18, 0x30 and 0x55. The deserializer uses I2C ATR for the serializer itself.

  Our application flow uses xmutil/dfx-mgrd to load and unload the camera Device Tree overlay. With the SerDes drivers
  built as modules, removing the overlay caused a NULL pointer dereference in i2c_atr_del_adapter(). The relevant part
  of the trace is:

  Unable to handle kernel NULL pointer dereference at virtual address 00000000000000a0
  Comm: dfx-mgrd

  Call trace:
   i2c_atr_del_adapter+0x3c/0x15c [i2c_atr]
   max_ser_i2c_adapter_deinit+0x68/0x90 [max_serdes]
   max_ser_remove+0x44/0xd0 [max_serdes]
   max96717_remove+0x14/0x20 [max96717]
   ...
   i2c_unregister_device+0xc4/0x128
   of_i2c_notify+0x6c/0x148
   ...
   __of_changeset_revert_notify+0x48/0xd0
   of_overlay_remove+0x124/0x1a0
   ...
   configfs_rmdir+0x278/0x420

  At probe time, max_ser_i2c_adapter_init() finds i2c-gate and initializes priv->mux, leaving priv->atr NULL. However,
  max_ser_i2c_adapter_deinit() looks up the firmware child again to decide which resource to release.

  During overlay removal, the child node has already been detached before the removal notifier reaches the serializer
  driver. The lookup therefore returns NULL, causing teardown to select the ATR path and call i2c_atr_del_adapter(NULL,
  0).

  The affected initialization and teardown functions in our downstream tree match those in this v17 patch.

  My local fix selects the cleanup path using the saved resource pointers. It also adds NULL checks to the cleanup
  helpers and clears the mux pointer after removing its adapters. This lets teardown follow the resource allocated
  during probe even after the firmware hierarchy has been dismantled.

  Could you please fold the following fix into the next revision of the series?

  Signed-off-by: Vivekananda Dayananda vivekana@xxxxxxx (vivekana@xxxxxxx)

  diff --git a/drivers/media/i2c/maxim-serdes/max_ser.c b/drivers/media/i2c/maxim-serdes/max_ser.c
  index ee891f6aa612..1fac47685921 100644
  --- a/drivers/media/i2c/maxim-serdes/max_ser.c
  +++ b/drivers/media/i2c/maxim-serdes/max_ser.c
  @@ -439,10 +439,19 @@ static const struct i2c_atr_ops max_ser_i2c_atr_ops = {

   static void max_ser_i2c_atr_deinit(struct max_ser_priv *priv)
   {
  -     /* Deleting adapters that haven't been added does no harm. */
  +     /* The i2c-gate/mux path never allocates an address translator. */
  +     if (!priv->atr)
  +             return;
  +
  +     /*
  +      * Remove downstream adapters before their parent translator. The ATR
  +      * API also accepts a channel whose adapter was never added, which is
  +      * needed when unwinding a failed adapter initialization.
  +      */
        i2c_atr_del_adapter(priv->atr, 0);

        i2c_atr_delete(priv->atr);
  +     /* Record that no translator remains for a later cleanup call. */
        priv->atr = NULL;
   }

  @@ -478,7 +487,13 @@ static int max_ser_i2c_mux_select(struct i2c_mux_core *mux, u32 chan)

   static void max_ser_i2c_mux_deinit(struct max_ser_priv *priv)
   {
  +     /* The ATR path and a failed mux allocation leave this pointer NULL. */
  +     if (!priv->mux)
  +             return;
  +
        i2c_mux_del_adapters(priv->mux);
  +     /* Do not use a stale mux pointer to select teardown a second time. */
  +     priv->mux = NULL;
   }

   static int max_ser_i2c_mux_init(struct max_ser_priv *priv)
  @@ -507,16 +522,21 @@ static int max_ser_i2c_adapter_init(struct max_ser_priv *priv)

   static void max_ser_i2c_adapter_deinit(struct max_ser_priv *priv)
   {
  -     struct fwnode_handle *fwnode;
  -
  -     fwnode = device_get_named_child_node(priv->dev, "i2c-gate");
  -     if (fwnode) {
  -             fwnode_handle_put(fwnode);
  +     /*
  +      * Probe selects either an i2c-gate mux or an I2C address translator
  +      * (ATR). Teardown must follow that resource choice, not re-derive it
  +      * from firmware: the i2c-gate child may already have disappeared during
  +      * DT overlay removal. Treating its absence as an ATR configuration
  +      * would pass a NULL translator to the ATR cleanup API on a mux device.
  +      *
  +      * Only one adapter type is initialized. Its saved pointer selects the
  +      * cleanup path even while the firmware graph is being dismantled. The
  +      * deinit helpers also tolerate NULL after cleanup or skipped setup.
  +      */
  +     if (priv->mux)
                max_ser_i2c_mux_deinit(priv);
  -             return;
  -     }
  -
  -     max_ser_i2c_atr_deinit(priv);
  +     else
  +             max_ser_i2c_atr_deinit(priv);
   }

   static int max_ser_set_tpg_fmt(struct v4l2_subdev *sd,

  Thanks,
  Vivekananda