The VGA connector in ast is supposed to work without I2C. Currently, this isn't correctly implemented in several places. Fix this. Also add managed cleanup of the I2C code, and fail if the connector setup fail.
Tested on AST2100 hardware.
Thomas Zimmermann (3): drm/ast: Handle failed I2C initialization gracefully drm/ast: Convert I2C code to managed cleanup drm/ast: Fail if connector initialization fails
drivers/gpu/drm/ast/ast_mode.c | 52 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 25 deletions(-)
base-commit: 6a8f90ec433e2f5de5fc16d7a4839771b7027cc0 -- 2.34.0
I2C initialization is allowed to fail. In this case, create a connector without DDC adapter. The current code would dereference a NULL pointer.
Reading the modes from the connector is supposed to work without I2C adapter. Add the respective test.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 1e30eaeb0e1b..3f0c8c1f9777 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1226,7 +1226,7 @@ static int ast_get_modes(struct drm_connector *connector) else kfree(edid); } - if (!flags) + if (!flags && ast_connector->i2c) edid = drm_get_edid(connector, &ast_connector->i2c->adapter); if (edid) { drm_connector_update_edid_property(&ast_connector->base, edid); @@ -1332,10 +1332,13 @@ static int ast_connector_init(struct drm_device *dev) if (!ast_connector->i2c) drm_err(dev, "failed to add ddc bus for connector\n");
- drm_connector_init_with_ddc(dev, connector, - &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c->adapter); + if (ast_connector->i2c) + drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA, + &ast_connector->i2c->adapter); + else + drm_connector_init(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA);
drm_connector_helper_add(connector, &ast_connector_helper_funcs);
Hi Thomas,
url: https://github.com/0day-ci/linux/commits/Thomas-Zimmermann/ast-Fix-I2C-corne... base: 6a8f90ec433e2f5de5fc16d7a4839771b7027cc0 config: i386-randconfig-m021-20211203 (https://download.01.org/0day-ci/archive/20211204/202112042126.N4Qr3RiC-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com Reported-by: Dan Carpenter dan.carpenter@oracle.com
smatch warnings: drivers/gpu/drm/ast/ast_mode.c:1231 ast_get_modes() error: uninitialized symbol 'edid'. drivers/gpu/drm/ast/ast_mode.c:1232 ast_get_modes() warn: passing freed memory 'edid'
vim +/edid +1231 drivers/gpu/drm/ast/ast_mode.c
312fec1405dd54 Dave Airlie 2012-02-29 1209 static int ast_get_modes(struct drm_connector *connector) 312fec1405dd54 Dave Airlie 2012-02-29 1210 { 312fec1405dd54 Dave Airlie 2012-02-29 1211 struct ast_connector *ast_connector = to_ast_connector(connector); fa7dbd7688849d Thomas Zimmermann 2020-06-17 1212 struct ast_private *ast = to_ast_private(connector->dev); 312fec1405dd54 Dave Airlie 2012-02-29 1213 struct edid *edid; ^^^^^^^^^^^^^^^^^^ 312fec1405dd54 Dave Airlie 2012-02-29 1214 int ret; 83c6620bae3f14 Dave Airlie 2014-03-28 1215 bool flags = false; 6c9bd4432b2527 Gregory Williams 2021-07-30 1216 83c6620bae3f14 Dave Airlie 2014-03-28 1217 if (ast->tx_chip_type == AST_TX_DP501) { 83c6620bae3f14 Dave Airlie 2014-03-28 1218 ast->dp501_maxclk = 0xff; 83c6620bae3f14 Dave Airlie 2014-03-28 1219 edid = kmalloc(128, GFP_KERNEL); 83c6620bae3f14 Dave Airlie 2014-03-28 1220 if (!edid) 83c6620bae3f14 Dave Airlie 2014-03-28 1221 return -ENOMEM; 312fec1405dd54 Dave Airlie 2012-02-29 1222 83c6620bae3f14 Dave Airlie 2014-03-28 1223 flags = ast_dp501_read_edid(connector->dev, (u8 *)edid); 83c6620bae3f14 Dave Airlie 2014-03-28 1224 if (flags) 83c6620bae3f14 Dave Airlie 2014-03-28 1225 ast->dp501_maxclk = ast_get_dp501_max_clk(connector->dev); 83c6620bae3f14 Dave Airlie 2014-03-28 1226 else 83c6620bae3f14 Dave Airlie 2014-03-28 1227 kfree(edid); ^^^^^^^^^^^
83c6620bae3f14 Dave Airlie 2014-03-28 1228 } 75bd8f71712a8a Thomas Zimmermann 2021-12-01 1229 if (!flags && ast_connector->i2c)
If "flags" is false then "edid" is either freed or uninitialized. So if "ast_connector->i2c" also is false then that's going to cause problems. Hopefully that part of the condition can just be removed?
if (!flags) edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
312fec1405dd54 Dave Airlie 2012-02-29 1230 edid = drm_get_edid(connector, &ast_connector->i2c->adapter); 312fec1405dd54 Dave Airlie 2012-02-29 @1231 if (edid) { c555f02371c338 Daniel Vetter 2018-07-09 @1232 drm_connector_update_edid_property(&ast_connector->base, edid); 312fec1405dd54 Dave Airlie 2012-02-29 1233 ret = drm_add_edid_modes(connector, edid); 993dcb05e47e35 Jani Nikula 2012-08-15 1234 kfree(edid); 312fec1405dd54 Dave Airlie 2012-02-29 1235 return ret; 6c9bd4432b2527 Gregory Williams 2021-07-30 1236 } c555f02371c338 Daniel Vetter 2018-07-09 1237 drm_connector_update_edid_property(&ast_connector->base, NULL); 312fec1405dd54 Dave Airlie 2012-02-29 1238 return 0; 312fec1405dd54 Dave Airlie 2012-02-29 1239 }
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Dec 01, 2021 at 04:30:58PM +0100, Thomas Zimmermann wrote:
I2C initialization is allowed to fail. In this case, create a connector without DDC adapter. The current code would dereference a NULL pointer.
Reading the modes from the connector is supposed to work without I2C adapter. Add the respective test.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
Release the I2C adapter as part of the DRM device cleanup.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 36 +++++++++++++++------------------- 1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 3f0c8c1f9777..a84dced95440 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -40,6 +40,7 @@ #include <drm/drm_gem_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -48,7 +49,6 @@ #include "ast_tables.h"
static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev); -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
static inline void ast_load_palette_index(struct ast_private *ast, u8 index, u8 red, u8 green, @@ -1300,14 +1300,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector, return flags; }
-static void ast_connector_destroy(struct drm_connector *connector) -{ - struct ast_connector *ast_connector = to_ast_connector(connector); - - ast_i2c_destroy(ast_connector->i2c); - drm_connector_cleanup(connector); -} - static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { .get_modes = ast_get_modes, .mode_valid = ast_mode_valid, @@ -1316,7 +1308,7 @@ static const struct drm_connector_helper_funcs ast_connector_helper_funcs = { static const struct drm_connector_funcs ast_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = ast_connector_destroy, + .destroy = drm_connector_cleanup, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; @@ -1493,6 +1485,14 @@ static void set_data(void *i2c_priv, int data) } }
+static void ast_i2c_release(struct drm_device *dev, void *res) +{ + struct ast_i2c_chan *i2c = res; + + i2c_del_adapter(&i2c->adapter); + kfree(i2c); +} + static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) { struct ast_i2c_chan *i2c; @@ -1521,19 +1521,15 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev) ret = i2c_bit_add_bus(&i2c->adapter); if (ret) { drm_err(dev, "Failed to register bit i2c\n"); - goto out_free; + goto out_kfree; }
+ ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c); + if (ret) + return NULL; return i2c; -out_free: - kfree(i2c); - return NULL; -}
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c) -{ - if (!i2c) - return; - i2c_del_adapter(&i2c->adapter); +out_kfree: kfree(i2c); + return NULL; }
On Wed, Dec 01, 2021 at 04:30:59PM +0100, Thomas Zimmermann wrote:
Release the I2C adapter as part of the DRM device cleanup.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
Update the connector code to fail if the connector could not be initialized. The current code just ignored the error and failed later when the connector was supposed to be used.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index a84dced95440..c988991cad33 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1319,18 +1319,21 @@ static int ast_connector_init(struct drm_device *dev) struct ast_connector *ast_connector = &ast->connector; struct drm_connector *connector = &ast_connector->base; struct drm_encoder *encoder = &ast->encoder; + int ret;
ast_connector->i2c = ast_i2c_create(dev); if (!ast_connector->i2c) drm_err(dev, "failed to add ddc bus for connector\n");
if (ast_connector->i2c) - drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA, - &ast_connector->i2c->adapter); + ret = drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA, + &ast_connector->i2c->adapter); else - drm_connector_init(dev, connector, &ast_connector_funcs, - DRM_MODE_CONNECTOR_VGA); + ret = drm_connector_init(dev, connector, &ast_connector_funcs, + DRM_MODE_CONNECTOR_VGA); + if (ret) + return ret;
drm_connector_helper_add(connector, &ast_connector_helper_funcs);
On Wed, Dec 01, 2021 at 04:31:00PM +0100, Thomas Zimmermann wrote:
Update the connector code to fail if the connector could not be initialized. The current code just ignored the error and failed later when the connector was supposed to be used.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Maxime Ripard -
Thomas Zimmermann