Re: [linusw-pinctrl:devel 30/31] drivers/pinctrl/pinctrl-rockchip.c:3683 rockchip_pinconf_set() error: we previously assumed 'gpio->direction_output' could be null (see line 3644)

[Heiko Stuebner <heiko@sntech.de>]

Am Dienstag, 20. Januar 2026, 07:25:00 Mitteleuropäische Normalzeit schrieb Dan Carpenter:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
> head:   76d415763bae9488dd2b923b1348ce6f26c1f0ae
> commit: e2c58cbe3aff49fe201e81ee5f651294e313ec74 [30/31] pinctrl: rockchip: Simplify locking with scoped_guard()
> config: sh-randconfig-r073-20260119 (https://download.01.org/0day-ci/archive/20260120/202601200057.P0Lr7NDg-lkp@intel.com/config)
> compiler: sh4-linux-gcc (GCC) 15.2.0
> smatch version: v0.5.0-8985-g2614ff1a
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202601200057.P0Lr7NDg-lkp@intel.com/
> 
> smatch warnings:
> drivers/pinctrl/pinctrl-rockchip.c:3683 rockchip_pinconf_set() error: we previously assumed 'gpio->direction_output' could be null (see line 3644)
> 
> vim +3683 drivers/pinctrl/pinctrl-rockchip.c
> 
> d3e5116119bd02e Heiko Stübner       2013-06-10  3622  static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> 03b054e9696c3cb Sherman Yin         2013-08-27  3623  				unsigned long *configs, unsigned num_configs)
> d3e5116119bd02e Heiko Stübner       2013-06-10  3624  {
> d3e5116119bd02e Heiko Stübner       2013-06-10  3625  	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> d3e5116119bd02e Heiko Stübner       2013-06-10  3626  	struct rockchip_pin_bank *bank = pin_to_bank(info, pin);
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3627  	struct gpio_chip *gpio = &bank->gpio_chip;
> 03b054e9696c3cb Sherman Yin         2013-08-27  3628  	enum pin_config_param param;
> 58957d2edfa19e9 Mika Westerberg     2017-01-23  3629  	u32 arg;
> 03b054e9696c3cb Sherman Yin         2013-08-27  3630  	int i;
> 03b054e9696c3cb Sherman Yin         2013-08-27  3631  	int rc;
> 03b054e9696c3cb Sherman Yin         2013-08-27  3632  
> 03b054e9696c3cb Sherman Yin         2013-08-27  3633  	for (i = 0; i < num_configs; i++) {
> 03b054e9696c3cb Sherman Yin         2013-08-27  3634  		param = pinconf_to_config_param(configs[i]);
> 03b054e9696c3cb Sherman Yin         2013-08-27  3635  		arg = pinconf_to_config_argument(configs[i]);
> d3e5116119bd02e Heiko Stübner       2013-06-10  3636  
> 203a83112e097a5 Linus Walleij       2025-09-05  3637  		if (param == PIN_CONFIG_LEVEL || param == PIN_CONFIG_INPUT_ENABLE) {
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3638  			/*
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3639  			 * Check for gpio driver not being probed yet.
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3640  			 * The lock makes sure that either gpio-probe has completed
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3641  			 * or the gpio driver hasn't probed yet.
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3642  			 */
> e2c58cbe3aff49f Krzysztof Kozlowski 2026-01-18  3643  			scoped_guard(mutex, &bank->deferred_lock) {
> 8ce5ef645468502 Caleb Connolly      2022-03-28 @3644  				if (!gpio || !gpio->direction_output) {
>                                                                                               ^^^^^^^^^^^^^^^^^^^^^^
> Is this check necessary?

I think the culprit is 
commit 8ce5ef645468 ("pinctrl/rockchip: support deferring other gpio params")

Which moved the original check up here for more generalization, but moved
it out of its original switch block where the break in line 3650 would end
the block, up here where actually a contnue would be necessary?

In its original place, the "break" would exit the switch block and thus
continue to the next entry in the for loop.


> e2c58cbe3aff49f Krzysztof Kozlowski 2026-01-18  3645  					rc = rockchip_pinconf_defer_pin(bank,
> e2c58cbe3aff49f Krzysztof Kozlowski 2026-01-18  3646  									pin - bank->pin_base,
> e2c58cbe3aff49f Krzysztof Kozlowski 2026-01-18  3647  									param, arg);
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3648  					if (rc)
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3649  						return rc;
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3650  					break;

now it looks like this break should be a continue, I think.
Because right now it exits the whole loop, if I'm not blind.


> 8ce5ef645468502 Caleb Connolly      2022-03-28  3651  				}
> e2c58cbe3aff49f Krzysztof Kozlowski 2026-01-18  3652  			}
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3653  		}
> 8ce5ef645468502 Caleb Connolly      2022-03-28  3654  
> d3e5116119bd02e Heiko Stübner       2013-06-10  3655  		switch (param) {
> d3e5116119bd02e Heiko Stübner       2013-06-10  3656  		case PIN_CONFIG_BIAS_DISABLE:
> 03b054e9696c3cb Sherman Yin         2013-08-27  3657  			rc =  rockchip_set_pull(bank, pin - bank->pin_base,
> 03b054e9696c3cb Sherman Yin         2013-08-27  3658  				param);
> 03b054e9696c3cb Sherman Yin         2013-08-27  3659  			if (rc)
> 03b054e9696c3cb Sherman Yin         2013-08-27  3660  				return rc;
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3661  			break;
> d3e5116119bd02e Heiko Stübner       2013-06-10  3662  		case PIN_CONFIG_BIAS_PULL_UP:
> d3e5116119bd02e Heiko Stübner       2013-06-10  3663  		case PIN_CONFIG_BIAS_PULL_DOWN:
> d3e5116119bd02e Heiko Stübner       2013-06-10  3664  		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> 6ca5274d1d1258b Heiko Stübner       2013-10-16  3665  		case PIN_CONFIG_BIAS_BUS_HOLD:
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3666  			if (!rockchip_pinconf_pull_valid(info->ctrl, param))
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3667  				return -ENOTSUPP;
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3668  
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3669  			if (!arg)
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3670  				return -EINVAL;
> 44b6d93043ab677 Heiko Stübner       2013-06-16  3671  
> 03b054e9696c3cb Sherman Yin         2013-08-27  3672  			rc = rockchip_set_pull(bank, pin - bank->pin_base,
> 03b054e9696c3cb Sherman Yin         2013-08-27  3673  				param);
> 03b054e9696c3cb Sherman Yin         2013-08-27  3674  			if (rc)
> 03b054e9696c3cb Sherman Yin         2013-08-27  3675  				return rc;
> d3e5116119bd02e Heiko Stübner       2013-06-10  3676  			break;
> 203a83112e097a5 Linus Walleij       2025-09-05  3677  		case PIN_CONFIG_LEVEL:
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3678  			rc = rockchip_set_mux(bank, pin - bank->pin_base,
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3679  					      RK_FUNC_GPIO);
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3680  			if (rc != RK_FUNC_GPIO)
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3681  				return -EINVAL;
> 9ce9a02039de72e Jianqun Xu          2021-08-16  3682  
> 9ce9a02039de72e Jianqun Xu          2021-08-16 @3683  			rc = gpio->direction_output(gpio, pin - bank->pin_base,
>                                                                              ^^^^^^^^^^^^^^^^^^^^^^
> Unchecked dereference.  This is old code so it's presumably okay.  I
> think this warning is triggering now because of changes in Smatch.

The block from above should already make sure that this is only accessed
if the gpio pointer is valid, so this should okay.


Heiko