If adv7511's devm_clk_get() for the cec clock returns -EPROBE_DEFER, we end up in an infinite probe loop. This happens:
(1) adv7511's probe is called.
(2) adv7511's probe adds some secondary i2c devices which bind to the dummy driver and thus call driver_deferred_probe_trigger() and increment deferred_trigger_count (see driver_bound()).
(3) adv7511's probe returns -EPROBE_DEFER, and since the deferred_trigger_count has changed during the probe call, driver_deferred_probe_trigger() is called immediately (see really_probe()) and adv7511's probe is scheduled.
(4) Goto step 1.
[ 61.972915] really_probe: bus: 'i2c': really_probe: probing driver adv7511 with device 0-0039 [ 61.992734] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-003f [ 61.993343] driver_bound: driver: 'dummy': driver_bound: bound to device '0-003f' [ 61.993626] really_probe: bus: 'i2c': really_probe: bound device 0-003f to driver dummy [ 61.995604] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-0038 [ 61.996381] driver_bound: driver: 'dummy': driver_bound: bound to device '0-0038' [ 61.996663] really_probe: bus: 'i2c': really_probe: bound device 0-0038 to driver dummy [ 61.998651] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-003c [ 61.999222] driver_bound: driver: 'dummy': driver_bound: bound to device '0-003c' [ 61.999496] really_probe: bus: 'i2c': really_probe: bound device 0-003c to driver dummy [ 62.010050] really_probe: i2c 0-0039: Driver adv7511 requests probe deferral [ 62.011380] really_probe: bus: 'platform': really_probe: probing driver pwm-clock with device clock-cec [ 62.012812] really_probe: platform clock-cec: Driver pwm-clock requests probe deferral [ 62.024679] really_probe: bus: 'i2c': really_probe: probing driver adv7511 with device 0-0039
Fix this by calling devm_clk_get() before registering the secondary devices.
Signed-off-by: Vincent Whitchurch vincent.whitchurch@axis.com --- v3: Make adv7511_cec_init() return void. v2: Add devm_clk_put() in error path.
drivers/gpu/drm/bridge/adv7511/adv7511.h | 5 ++- drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 34 ++++++++------------ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 ++++++--- 3 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..05a66149b186 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -380,17 +380,16 @@ struct adv7511 { };
#ifdef CONFIG_DRM_I2C_ADV7511_CEC -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); #else -static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +static inline void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { unsigned int offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, ADV7511_CEC_CTRL_POWER_DOWN); - return 0; } #endif
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index a20a45c0b353..ee0ed4fb67c1 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c @@ -286,28 +286,17 @@ static const struct cec_adap_ops adv7511_cec_adap_ops = { .adap_transmit = adv7511_cec_adap_transmit, };
-static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511) -{ - adv7511->cec_clk = devm_clk_get(dev, "cec"); - if (IS_ERR(adv7511->cec_clk)) { - int ret = PTR_ERR(adv7511->cec_clk); - - adv7511->cec_clk = NULL; - return ret; - } - clk_prepare_enable(adv7511->cec_clk); - adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk); - return 0; -} - -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { unsigned int offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0; - int ret = adv7511_cec_parse_dt(dev, adv7511); + int ret;
- if (ret) - goto err_cec_parse_dt; + if (!adv7511->cec_clk) + goto err_cec_no_clock; + + clk_prepare_enable(adv7511->cec_clk); + adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS); @@ -334,7 +323,7 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) ret = cec_register_adapter(adv7511->cec_adap, dev); if (ret) goto err_cec_register; - return 0; + return;
err_cec_register: cec_delete_adapter(adv7511->cec_adap); @@ -342,8 +331,11 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) err_cec_alloc: dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", ret); -err_cec_parse_dt: + clk_disable_unprepare(adv7511->cec_clk); + devm_clk_put(dev, adv7511->cec_clk); + /* Ensure that adv7511_remove() doesn't attempt to disable it again. */ + adv7511->cec_clk = NULL; +err_cec_no_clock: regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, ADV7511_CEC_CTRL_POWER_DOWN); - return ret == -EPROBE_DEFER ? ret : 0; } diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 87b58c1acff4..8d8df6116082 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1128,6 +1128,15 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) return ret;
+ adv7511->cec_clk = devm_clk_get(dev, "cec"); + if (IS_ERR(adv7511->cec_clk)) { + ret = PTR_ERR(adv7511->cec_clk); + if (ret == -EPROBE_DEFER) + return ret; + + adv7511->cec_clk = NULL; + } + ret = adv7511_init_regulators(adv7511); if (ret) { dev_err(dev, "failed to init regulators\n"); @@ -1218,9 +1227,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (adv7511->type == ADV7511) adv7511_set_link_config(adv7511, &link_config);
- ret = adv7511_cec_init(dev, adv7511); - if (ret) - goto err_unregister_cec; + adv7511_cec_init(dev, adv7511);
adv7511->bridge.funcs = &adv7511_bridge_funcs; adv7511->bridge.of_node = dev->of_node; @@ -1232,8 +1239,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
err_unregister_cec: i2c_unregister_device(adv7511->i2c_cec); - if (adv7511->cec_clk) - clk_disable_unprepare(adv7511->cec_clk); err_i2c_unregister_packet: i2c_unregister_device(adv7511->i2c_packet); err_i2c_unregister_edid:
On 14.05.2020 13:30, Vincent Whitchurch wrote:
If adv7511's devm_clk_get() for the cec clock returns -EPROBE_DEFER, we end up in an infinite probe loop. This happens:
(1) adv7511's probe is called.
(2) adv7511's probe adds some secondary i2c devices which bind to the dummy driver and thus call driver_deferred_probe_trigger() and increment deferred_trigger_count (see driver_bound()).
(3) adv7511's probe returns -EPROBE_DEFER, and since the deferred_trigger_count has changed during the probe call, driver_deferred_probe_trigger() is called immediately (see really_probe()) and adv7511's probe is scheduled.
(4) Goto step 1.
[ 61.972915] really_probe: bus: 'i2c': really_probe: probing driver adv7511 with device 0-0039 [ 61.992734] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-003f [ 61.993343] driver_bound: driver: 'dummy': driver_bound: bound to device '0-003f' [ 61.993626] really_probe: bus: 'i2c': really_probe: bound device 0-003f to driver dummy [ 61.995604] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-0038 [ 61.996381] driver_bound: driver: 'dummy': driver_bound: bound to device '0-0038' [ 61.996663] really_probe: bus: 'i2c': really_probe: bound device 0-0038 to driver dummy [ 61.998651] really_probe: bus: 'i2c': really_probe: probing driver dummy with device 0-003c [ 61.999222] driver_bound: driver: 'dummy': driver_bound: bound to device '0-003c' [ 61.999496] really_probe: bus: 'i2c': really_probe: bound device 0-003c to driver dummy [ 62.010050] really_probe: i2c 0-0039: Driver adv7511 requests probe deferral [ 62.011380] really_probe: bus: 'platform': really_probe: probing driver pwm-clock with device clock-cec [ 62.012812] really_probe: platform clock-cec: Driver pwm-clock requests probe deferral [ 62.024679] really_probe: bus: 'i2c': really_probe: probing driver adv7511 with device 0-0039
Fix this by calling devm_clk_get() before registering the secondary devices.
Signed-off-by: Vincent Whitchurch vincent.whitchurch@axis.com
v3: Make adv7511_cec_init() return void. v2: Add devm_clk_put() in error path.
drivers/gpu/drm/bridge/adv7511/adv7511.h | 5 ++- drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 34 ++++++++------------ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 ++++++--- 3 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index a9bb734366ae..05a66149b186 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -380,17 +380,16 @@ struct adv7511 { };
#ifdef CONFIG_DRM_I2C_ADV7511_CEC -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); #else -static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +static inline void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { unsigned int offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, ADV7511_CEC_CTRL_POWER_DOWN);
- return 0; } #endif
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index a20a45c0b353..ee0ed4fb67c1 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c @@ -286,28 +286,17 @@ static const struct cec_adap_ops adv7511_cec_adap_ops = { .adap_transmit = adv7511_cec_adap_transmit, };
-static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511) -{
- adv7511->cec_clk = devm_clk_get(dev, "cec");
- if (IS_ERR(adv7511->cec_clk)) {
int ret = PTR_ERR(adv7511->cec_clk);adv7511->cec_clk = NULL;return ret;- }
- clk_prepare_enable(adv7511->cec_clk);
- adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
- return 0;
-}
-int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { unsigned int offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
- int ret = adv7511_cec_parse_dt(dev, adv7511);
- int ret;
- if (ret)
goto err_cec_parse_dt;
if (!adv7511->cec_clk)
goto err_cec_no_clock;clk_prepare_enable(adv7511->cec_clk);
adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS);
@@ -334,7 +323,7 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) ret = cec_register_adapter(adv7511->cec_adap, dev); if (ret) goto err_cec_register;
- return 0;
return;
err_cec_register: cec_delete_adapter(adv7511->cec_adap);
@@ -342,8 +331,11 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) err_cec_alloc: dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", ret); -err_cec_parse_dt:
- clk_disable_unprepare(adv7511->cec_clk);
- devm_clk_put(dev, adv7511->cec_clk);
- /* Ensure that adv7511_remove() doesn't attempt to disable it again. */
- adv7511->cec_clk = NULL;
Are you sure these three lines above are necessary? devm mechanism should free the clock and with failed probe remove should not be called.
Beside this:
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
Regards
Andrzej
+err_cec_no_clock: regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, ADV7511_CEC_CTRL_POWER_DOWN);
- return ret == -EPROBE_DEFER ? ret : 0; }
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 87b58c1acff4..8d8df6116082 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1128,6 +1128,15 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) return ret;
- adv7511->cec_clk = devm_clk_get(dev, "cec");
- if (IS_ERR(adv7511->cec_clk)) {
ret = PTR_ERR(adv7511->cec_clk);if (ret == -EPROBE_DEFER)return ret;adv7511->cec_clk = NULL;- }
- ret = adv7511_init_regulators(adv7511); if (ret) { dev_err(dev, "failed to init regulators\n");
@@ -1218,9 +1227,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (adv7511->type == ADV7511) adv7511_set_link_config(adv7511, &link_config);
- ret = adv7511_cec_init(dev, adv7511);
- if (ret)
goto err_unregister_cec;
adv7511_cec_init(dev, adv7511);
adv7511->bridge.funcs = &adv7511_bridge_funcs; adv7511->bridge.of_node = dev->of_node;
@@ -1232,8 +1239,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
err_unregister_cec: i2c_unregister_device(adv7511->i2c_cec);
- if (adv7511->cec_clk)
clk_disable_unprepare(adv7511->cec_clk);
On Thu, Jul 02, 2020 at 04:22:34PM +0200, Andrzej Hajda wrote:
On 14.05.2020 13:30, Vincent Whitchurch wrote:
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index a20a45c0b353..ee0ed4fb67c1 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c @@ -286,28 +286,17 @@ static const struct cec_adap_ops adv7511_cec_adap_ops = { .adap_transmit = adv7511_cec_adap_transmit, };
-static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511) -{
- adv7511->cec_clk = devm_clk_get(dev, "cec");
- if (IS_ERR(adv7511->cec_clk)) {
int ret = PTR_ERR(adv7511->cec_clk);adv7511->cec_clk = NULL;return ret;- }
- clk_prepare_enable(adv7511->cec_clk);
- adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
- return 0;
-}
-int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { unsigned int offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
- int ret = adv7511_cec_parse_dt(dev, adv7511);
- int ret;
- if (ret)
goto err_cec_parse_dt;
if (!adv7511->cec_clk)
goto err_cec_no_clock;clk_prepare_enable(adv7511->cec_clk);
adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS);
@@ -334,7 +323,7 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) ret = cec_register_adapter(adv7511->cec_adap, dev); if (ret) goto err_cec_register;
- return 0;
return;
err_cec_register: cec_delete_adapter(adv7511->cec_adap);
@@ -342,8 +331,11 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) err_cec_alloc: dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", ret); -err_cec_parse_dt:
- clk_disable_unprepare(adv7511->cec_clk);
- devm_clk_put(dev, adv7511->cec_clk);
- /* Ensure that adv7511_remove() doesn't attempt to disable it again. */
- adv7511->cec_clk = NULL;
Are you sure these three lines above are necessary? devm mechanism should free the clock and with failed probe remove should not be called.
This driver ignores all failures in CEC registration/initialisation and does not fail the probe for them. I find that odd, but it seems to be done like this on purpose (see commit 1b6fba458c0a2e8513 "drm/bridge: adv7511/33: Fix adv7511_cec_init() failure handling") and my patch preserves that behaviour.
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Vincent Whitchurch