This series fixes various small issues in the drivers, and adds a few things (a couple of pixel formats and a debugging feature).
It also takes a few steps in adding more i2c read/write error handlings to the drivers, but covers only the easy places.
Adding error handling to all reads/writes needs more thinking, perhaps adding a "ret" parameter to the calls, similar to the cci_* functions, or perhaps adding helpers for writing multiple registers from a given table. Also, in some places rolling back from an error will require work.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- Changes in v3: - Include bitfield.h for FIELD_PREP() - Cc stable for relevant fixes - Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboar...
Changes in v2: - Address comments from Andy - Add two new patches: - media: i2c: ds90ub960: Fix shadowing of local variables - media: i2c: ds90ub960: Use HZ_PER_MHZ - Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboar...
--- Tomi Valkeinen (15): media: i2c: ds90ub9x3: Fix extra fwnode_handle_put() media: i2c: ds90ub960: Fix UB9702 refclk register access media: i2c: ds90ub960: Fix use of non-existing registers on UB9702 media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702 media: i2c: ds90ub960: Fix UB9702 VC map media: i2c: ds90ub960: Use HZ_PER_MHZ media: i2c: ds90ub960: Add support for I2C_RX_ID media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats media: i2c: ds90ub953: Clear CRC errors in ub953_log_status() media: i2c: ds90ub960: Drop unused indirect block define media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks() media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq() media: i2c: ds90ub913: Add error handling to ub913_hw_init() media: i2c: ds90ub953: Add error handling for i2c reads/writes media: i2c: ds90ub960: Fix shadowing of local variables
drivers/media/i2c/ds90ub913.c | 26 ++++-- drivers/media/i2c/ds90ub953.c | 56 +++++++++---- drivers/media/i2c/ds90ub960.c | 186 ++++++++++++++++++++++++++++-------------- 3 files changed, 187 insertions(+), 81 deletions(-) --- base-commit: adc218676eef25575469234709c2d87185ca223a change-id: 20241004-ub9xx-fixes-bba80dc48627
Best regards,
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as part of their remove process, and if the driver is removed multiple times, eventually leads to put "overflow", possibly causing memory corruption or crash.
The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching"), which changed the code related to the sd.fwnode, but missed removing these fwnode_handle_put() calls.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching") --- drivers/media/i2c/ds90ub913.c | 1 - drivers/media/i2c/ds90ub953.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c index 8eed4a200fd8..b5375d736629 100644 --- a/drivers/media/i2c/ds90ub913.c +++ b/drivers/media/i2c/ds90ub913.c @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub913_v4l2_nf_unregister(priv); v4l2_subdev_cleanup(&priv->sd); - fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index 16f88db14981..10daecf6f457 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub953_v4l2_notifier_unregister(priv); v4l2_subdev_cleanup(&priv->sd); - fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
Huomenta,
On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as part of their remove process, and if the driver is removed multiple times, eventually leads to put "overflow", possibly causing memory
This is, in fact, an extra put. It'll lead to underflow, not overflow. I'd expect removing it once would be already too much.
corruption or crash.
The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching"), which changed the code related to the sd.fwnode, but missed removing these fwnode_handle_put() calls.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
drivers/media/i2c/ds90ub913.c | 1 - drivers/media/i2c/ds90ub953.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c index 8eed4a200fd8..b5375d736629 100644 --- a/drivers/media/i2c/ds90ub913.c +++ b/drivers/media/i2c/ds90ub913.c @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub913_v4l2_nf_unregister(priv); v4l2_subdev_cleanup(&priv->sd);
- fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity);
} diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index 16f88db14981..10daecf6f457 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub953_v4l2_notifier_unregister(priv); v4l2_subdev_cleanup(&priv->sd);
- fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity);
}
Hi,
On 09/12/2024 11:09, Sakari Ailus wrote:
Huomenta,
On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as part of their remove process, and if the driver is removed multiple times, eventually leads to put "overflow", possibly causing memory
This is, in fact, an extra put. It'll lead to underflow, not overflow.
Well, there are too many puts, so "put overflow" =). I don't think underflow is the right word here either, but it's probably better than overflow. Maybe I'll reword it so that it doesn't have a flow at all.
I'd expect removing it once would be already too much.
True, but there's something keeping some refs to the fwnode even without these drivers (otherwise they'd be freed when these drivers are not around), so the issue shows only when those refs are taken out by the puts in these drivers.
Tomi
corruption or crash.
The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching"), which changed the code related to the sd.fwnode, but missed removing these fwnode_handle_put() calls.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
drivers/media/i2c/ds90ub913.c | 1 - drivers/media/i2c/ds90ub953.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c index 8eed4a200fd8..b5375d736629 100644 --- a/drivers/media/i2c/ds90ub913.c +++ b/drivers/media/i2c/ds90ub913.c @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub913_v4l2_nf_unregister(priv); v4l2_subdev_cleanup(&priv->sd);
- fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index 16f88db14981..10daecf6f457 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub953_v4l2_notifier_unregister(priv); v4l2_subdev_cleanup(&priv->sd);
- fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
Moi,
On Mon, Dec 09, 2024 at 11:46:45AM +0200, Tomi Valkeinen wrote:
Hi,
On 09/12/2024 11:09, Sakari Ailus wrote:
Huomenta,
On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as part of their remove process, and if the driver is removed multiple times, eventually leads to put "overflow", possibly causing memory
This is, in fact, an extra put. It'll lead to underflow, not overflow.
Well, there are too many puts, so "put overflow" =). I don't think underflow is the right word here either, but it's probably better than overflow. Maybe I'll reword it so that it doesn't have a flow at all.
Sound good.
I'd expect removing it once would be already too much.
True, but there's something keeping some refs to the fwnode even without these drivers (otherwise they'd be freed when these drivers are not around), so the issue shows only when those refs are taken out by the puts in these drivers.
Port nodes perhaps?
UB9702 has the refclk freq register at a different offset than UB960, but the code uses the UB960's offset for both chips. Fix this.
The refclk freq is only used for a debug print, so there's no functional change here.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") --- drivers/media/i2c/ds90ub960.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index ffe5f25f8647..b1e848678218 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -352,6 +352,8 @@
#define UB960_SR_I2C_RX_ID(n) (0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */
+#define UB9702_SR_REFCLK_FREQ 0x3d + /* Indirect register blocks */ #define UB960_IND_TARGET_PAT_GEN 0x00 #define UB960_IND_TARGET_RX_ANA(n) (0x01 + (n)) @@ -3837,7 +3839,10 @@ static int ub960_enable_core_hw(struct ub960_data *priv) if (ret) goto err_pd_gpio;
- ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); + if (priv->hw_data->is_ub9702) + ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq); + else + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); if (ret) goto err_pd_gpio;
UB9702 doesn't have the registers for SP and EQ. Adjust the code in ub960_rxport_wait_locks() to not use those registers for UB9702. As these values are only used for a debug print here, there's no functional change.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") --- drivers/media/i2c/ds90ub960.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index b1e848678218..24198b803eff 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -1577,16 +1577,24 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
ub960_rxport_read16(priv, nport, UB960_RR_RX_FREQ_HIGH, &v);
- ret = ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); - if (ret) - return ret; + if (priv->hw_data->is_ub9702) { + dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n", + nport, (v * 1000000ULL) >> 8); + } else { + ret = ub960_rxport_get_strobe_pos(priv, nport, + &strobe_pos); + if (ret) + return ret;
- ret = ub960_rxport_get_eq_level(priv, nport, &eq_level); - if (ret) - return ret; + ret = ub960_rxport_get_eq_level(priv, nport, &eq_level); + if (ret) + return ret;
- dev_dbg(dev, "\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n", - nport, strobe_pos, eq_level, (v * 1000000ULL) >> 8); + dev_dbg(dev, + "\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n", + nport, strobe_pos, eq_level, + (v * 1000000ULL) >> 8); + } }
return 0;
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") --- drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, };
+static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv, + unsigned int nport) +{ + struct device *dev = &priv->client->dev; + u8 eq_level; + s8 strobe_pos; + u8 v = 0; + + /* Strobe */ + + ub960_read(priv, UB960_XR_AEQ_CTL1, &v); + + dev_info(dev, "\t%s strobe\n", + (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" : + "Manual"); + + if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) { + ub960_read(priv, UB960_XR_SFILTER_CFG, &v); + + dev_info(dev, "\tStrobe range [%d, %d]\n", + ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7, + ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7); + } + + ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); + + dev_info(dev, "\tStrobe pos %d\n", strobe_pos); + + /* EQ */ + + ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v); + + dev_info(dev, "\t%s EQ\n", + (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" : + "Adaptive"); + + if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) { + ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v); + + dev_info(dev, "\tEQ range [%u, %u]\n", + (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf, + (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf); + } + + if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0) + dev_info(dev, "\tEQ level %u\n", eq_level); +} + static int ub960_log_status(struct v4l2_subdev *sd) { struct ub960_data *priv = sd_to_ub960(sd); @@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; - u8 eq_level; - s8 strobe_pos; unsigned int i;
dev_info(dev, "RX %u\n", nport); @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd) ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v); dev_info(dev, "\tcsi_err_counter %u\n", v);
- /* Strobe */ - - ub960_read(priv, UB960_XR_AEQ_CTL1, &v); - - dev_info(dev, "\t%s strobe\n", - (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" : - "Manual"); - - if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) { - ub960_read(priv, UB960_XR_SFILTER_CFG, &v); - - dev_info(dev, "\tStrobe range [%d, %d]\n", - ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7, - ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7); - } - - ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); - - dev_info(dev, "\tStrobe pos %d\n", strobe_pos); - - /* EQ */ - - ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v); - - dev_info(dev, "\t%s EQ\n", - (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" : - "Adaptive"); - - if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) { - ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v); - - dev_info(dev, "\tEQ range [%u, %u]\n", - (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf, - (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf); - } - - if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0) - dev_info(dev, "\tEQ level %u\n", eq_level); + if (!priv->hw_data->is_ub9702) + ub960_log_status_ub960_sp_eq(priv, nport);
/* GPIOs */ for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
The driver uses a static CSI-2 virtual channel mapping where all virtual channels from an RX port are mapped to a virtual channel number matching the RX port number.
The UB960 and UB9702 have different registers for the purpose, and the UB9702 version is not correct. Each of the VC_ID_MAP registers do not contain a single mapping, as the driver currently thinks, but two.
This can cause received VCs other than 0 to be mapped in a wrong way.
Fix this by writing both mappings to each register.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") --- drivers/media/i2c/ds90ub960.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 94c8acf171b4..bfffa14e2049 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2533,7 +2533,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, for (i = 0; i < 8; i++) ub960_rxport_write(priv, nport, UB960_RR_VC_ID_MAP(i), - nport); + (nport << 4) | nport); }
break;
Hi Tomi,
On Dec 04, 2024 at 13:05:14 +0200, Tomi Valkeinen wrote:
This series fixes various small issues in the drivers, and adds a few things (a couple of pixel formats and a debugging feature).
It also takes a few steps in adding more i2c read/write error handlings to the drivers, but covers only the easy places.
Adding error handling to all reads/writes needs more thinking, perhaps adding a "ret" parameter to the calls, similar to the cci_* functions, or perhaps adding helpers for writing multiple registers from a given table. Also, in some places rolling back from an error will require work.
With the minor comment addressed, for the series:
Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
Changes in v3:
- Include bitfield.h for FIELD_PREP()
- Cc stable for relevant fixes
- Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboar...
Changes in v2:
- Address comments from Andy
- Add two new patches:
- media: i2c: ds90ub960: Fix shadowing of local variables
- media: i2c: ds90ub960: Use HZ_PER_MHZ
- Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboar...
Tomi Valkeinen (15): media: i2c: ds90ub9x3: Fix extra fwnode_handle_put() media: i2c: ds90ub960: Fix UB9702 refclk register access media: i2c: ds90ub960: Fix use of non-existing registers on UB9702 media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702 media: i2c: ds90ub960: Fix UB9702 VC map media: i2c: ds90ub960: Use HZ_PER_MHZ media: i2c: ds90ub960: Add support for I2C_RX_ID media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats media: i2c: ds90ub953: Clear CRC errors in ub953_log_status() media: i2c: ds90ub960: Drop unused indirect block define media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks() media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq() media: i2c: ds90ub913: Add error handling to ub913_hw_init() media: i2c: ds90ub953: Add error handling for i2c reads/writes media: i2c: ds90ub960: Fix shadowing of local variables
drivers/media/i2c/ds90ub913.c | 26 ++++-- drivers/media/i2c/ds90ub953.c | 56 +++++++++---- drivers/media/i2c/ds90ub960.c | 186 ++++++++++++++++++++++++++++-------------- 3 files changed, 187 insertions(+), 81 deletions(-)
base-commit: adc218676eef25575469234709c2d87185ca223a change-id: 20241004-ub9xx-fixes-bba80dc48627
Best regards,
Tomi Valkeinen tomi.valkeinen@ideasonboard.com
linux-stable-mirror@lists.linaro.org
-
Jai Luthra -
Sakari Ailus -
Tomi Valkeinen