Hi Vincent,

Thank you for the patch.

On Tue, Mar 31, 2020 at 04:16:29PM +0200, 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.

Lovely :-S

[ 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

drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 30 +++++++------------- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 11 +++++-- 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index a20a45c0b353..4b0fee32be21 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) { 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);

@@ -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);
• /* Ensure that adv7511_remove() doesn't attempt to disable it again. */

Would it make sense to call devm_clk_put() here to already release the clock ? Apart from that,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• 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;

• return 0;

} diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 9e13e466e72c..ebc548e23ece 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1122,6 +1122,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");

@@ -1226,8 +1235,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: