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.whitchu...@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.pinch...@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:

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to