Re: [PATCH v3] drm/bridge: adv7511: Fix cec clock EPROBE_DEFER handling
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(_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 mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/bridge: adv7511: Fix cec clock EPROBE_DEFER handling
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 > --- > 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); > >
[PATCH v3] drm/bridge: adv7511: Fix cec clock EPROBE_DEFER handling
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 --- 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(_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 =