Fix the undefined usage of the GPIO consumer API after retrieving the GPIO description with GPIO_ASIS. The API documentation mentions that GPIO_ASIS won't set a GPIO direction and requires the user to set a direction before using the GPIO.
This can be confirmed on i.MX6 hardware, where rfkill-gpio is no longer able to enabled/disable a device, presumably because the GPIO controller was never configured for the output direction.
Fixes: b2f750c3a80b ("net: rfkill: gpio: prevent value glitch during probe") Cc: stable@vger.kernel.org Signed-off-by: Rouven Czerwinski r.czerwinski@pengutronix.de --- net/rfkill/rfkill-gpio.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index 5a81505fba9ac..3d9ae696397cf 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -126,6 +126,16 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -EINVAL; }
+ if (rfkill->reset_gpio) + ret = gpiod_direction_output(rfkill->reset_gpio, true); + if (ret) + return ret; + + if (rfkill->shutdown_gpio) + ret = gpiod_direction_output(rfkill->shutdown_gpio, true); + if (ret) + return ret; + rfkill->rfkill_dev = rfkill_alloc(rfkill->name, &pdev->dev, rfkill->type, &rfkill_gpio_ops, rfkill);
base-commit: 994d5c58e50e91bb02c7be4a91d5186292a895c8
On Wed, 2023-12-06 at 14:13 +0100, Rouven Czerwinski wrote:
+++ b/net/rfkill/rfkill-gpio.c @@ -126,6 +126,16 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -EINVAL; }
- if (rfkill->reset_gpio)
ret = gpiod_direction_output(rfkill->reset_gpio, true);- if (ret)
return ret;- if (rfkill->shutdown_gpio)
ret = gpiod_direction_output(rfkill->shutdown_gpio, true);- if (ret)
return ret;
That's weird, you need ret to be inside the if. It's even entirely uninitialized if you don't have ACPI, if you don't have reset/shutdown.
johannes
Hi Johannes,
On Wed, 2023-12-06 at 14:16 +0100, Johannes Berg wrote:
On Wed, 2023-12-06 at 14:13 +0100, Rouven Czerwinski wrote:
+++ b/net/rfkill/rfkill-gpio.c @@ -126,6 +126,16 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -EINVAL; }
- if (rfkill->reset_gpio)
ret = gpiod_direction_output(rfkill->reset_gpio,true);
- if (ret)
return ret;- if (rfkill->shutdown_gpio)
ret = gpiod_direction_output(rfkill-shutdown_gpio, true);
- if (ret)
return ret;That's weird, you need ret to be inside the if. It's even entirely uninitialized if you don't have ACPI, if you don't have reset/shutdown.
Thanks for the review, you are totally right, I didn't look at the ret initialization. I moved it inside the if for v2.
Thanks, Rouven
Hi Rouven,
On Mi, 2023-12-06 at 14:24 +0100, Rouven Czerwinski wrote:
Hi Johannes,
On Wed, 2023-12-06 at 14:16 +0100, Johannes Berg wrote:
On Wed, 2023-12-06 at 14:13 +0100, Rouven Czerwinski wrote:
+++ b/net/rfkill/rfkill-gpio.c @@ -126,6 +126,16 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -EINVAL; }
- if (rfkill->reset_gpio)
ret = gpiod_direction_output(rfkill->reset_gpio,true);
- if (ret)
return ret;- if (rfkill->shutdown_gpio)
ret = gpiod_direction_output(rfkill-shutdown_gpio, true);
- if (ret)
return ret;That's weird, you need ret to be inside the if. It's even entirely uninitialized if you don't have ACPI, if you don't have reset/shutdown.
Thanks for the review, you are totally right, I didn't look at the ret initialization. I moved it inside the if for v2.
The if-block is not required at all, gpiod_direction_output(NULL, ...) will just return 0 from VALIDATE_DESC().
regards Philipp
Hi Rouven,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 994d5c58e50e91bb02c7be4a91d5186292a895c8]
url: https://github.com/intel-lab-lkp/linux/commits/Rouven-Czerwinski/net-rfkill-... base: 994d5c58e50e91bb02c7be4a91d5186292a895c8 patch link: https://lore.kernel.org/r/20231206131336.3099727-1-r.czerwinski%40pengutroni... patch subject: [PATCH] net: rfkill: gpio: set GPIO direction config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231207/202312070522.71CNBJ25-lkp@i...) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231207/202312070522.71CNBJ25-lkp@i...)
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 | Closes: https://lore.kernel.org/oe-kbuild-all/202312070522.71CNBJ25-lkp@intel.com/
All warnings (new ones prefixed by >>):
net/rfkill/rfkill-gpio.c:129:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
129 | if (rfkill->reset_gpio) | ^~~~~~~~~~~~~~~~~~ net/rfkill/rfkill-gpio.c:131:6: note: uninitialized use occurs here 131 | if (ret) | ^~~ net/rfkill/rfkill-gpio.c:129:2: note: remove the 'if' if its condition is always true 129 | if (rfkill->reset_gpio) | ^~~~~~~~~~~~~~~~~~~~~~~ 130 | ret = gpiod_direction_output(rfkill->reset_gpio, true); | ~~~~~~~~~~~~~~~~ net/rfkill/rfkill-gpio.c:82:9: note: initialize the variable 'ret' to silence this warning 82 | int ret; | ^ | = 0 1 warning generated.
vim +129 net/rfkill/rfkill-gpio.c
74 75 static int rfkill_gpio_probe(struct platform_device *pdev) 76 { 77 struct rfkill_gpio_data *rfkill; 78 struct gpio_desc *gpio; 79 const char *name_property; 80 const char *type_property; 81 const char *type_name; 82 int ret; 83 84 rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL); 85 if (!rfkill) 86 return -ENOMEM; 87 88 if (dev_of_node(&pdev->dev)) { 89 name_property = "label"; 90 type_property = "radio-type"; 91 } else { 92 name_property = "name"; 93 type_property = "type"; 94 } 95 device_property_read_string(&pdev->dev, name_property, &rfkill->name); 96 device_property_read_string(&pdev->dev, type_property, &type_name); 97 98 if (!rfkill->name) 99 rfkill->name = dev_name(&pdev->dev); 100 101 rfkill->type = rfkill_find_type(type_name); 102 103 if (ACPI_HANDLE(&pdev->dev)) { 104 ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill); 105 if (ret) 106 return ret; 107 } 108 109 rfkill->clk = devm_clk_get(&pdev->dev, NULL); 110 111 gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_ASIS); 112 if (IS_ERR(gpio)) 113 return PTR_ERR(gpio); 114 115 rfkill->reset_gpio = gpio; 116 117 gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_ASIS); 118 if (IS_ERR(gpio)) 119 return PTR_ERR(gpio); 120 121 rfkill->shutdown_gpio = gpio; 122 123 /* Make sure at-least one GPIO is defined for this instance */ 124 if (!rfkill->reset_gpio && !rfkill->shutdown_gpio) { 125 dev_err(&pdev->dev, "invalid platform data\n"); 126 return -EINVAL; 127 } 128
129 if (rfkill->reset_gpio)
130 ret = gpiod_direction_output(rfkill->reset_gpio, true); 131 if (ret) 132 return ret; 133 134 if (rfkill->shutdown_gpio) 135 ret = gpiod_direction_output(rfkill->shutdown_gpio, true); 136 if (ret) 137 return ret; 138 139 rfkill->rfkill_dev = rfkill_alloc(rfkill->name, &pdev->dev, 140 rfkill->type, &rfkill_gpio_ops, 141 rfkill); 142 if (!rfkill->rfkill_dev) 143 return -ENOMEM; 144 145 ret = rfkill_register(rfkill->rfkill_dev); 146 if (ret < 0) 147 goto err_destroy; 148 149 platform_set_drvdata(pdev, rfkill); 150 151 dev_info(&pdev->dev, "%s device registered.\n", rfkill->name); 152 153 return 0; 154 155 err_destroy: 156 rfkill_destroy(rfkill->rfkill_dev); 157 158 return ret; 159 } 160
Fix the undefined usage of the GPIO consumer API after retrieving the GPIO description with GPIO_ASIS. The API documentation mentions that GPIO_ASIS won't set a GPIO direction and requires the user to set a direction before using the GPIO.
This can be confirmed on i.MX6 hardware, where rfkill-gpio is no longer able to enabled/disable a device, presumably because the GPIO controller was never configured for the output direction.
Fixes: b2f750c3a80b ("net: rfkill: gpio: prevent value glitch during probe") Cc: stable@vger.kernel.org Signed-off-by: Rouven Czerwinski r.czerwinski@pengutronix.de --- v2: - remove the if clauses, the gpiod_direction_* functions can handle NULL gpio descriptors.
net/rfkill/rfkill-gpio.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index 5a81505fba9ac..4e32d659524e0 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -126,6 +126,14 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -EINVAL; }
+ ret = gpiod_direction_output(rfkill->reset_gpio, true); + if (ret) + return ret; + + ret = gpiod_direction_output(rfkill->shutdown_gpio, true); + if (ret) + return ret; + rfkill->rfkill_dev = rfkill_alloc(rfkill->name, &pdev->dev, rfkill->type, &rfkill_gpio_ops, rfkill);
base-commit: 994d5c58e50e91bb02c7be4a91d5186292a895c8
On Thu, Dec 07, 2023 at 08:58:36AM +0100, Rouven Czerwinski wrote:
Fix the undefined usage of the GPIO consumer API after retrieving the GPIO description with GPIO_ASIS. The API documentation mentions that GPIO_ASIS won't set a GPIO direction and requires the user to set a direction before using the GPIO.
This can be confirmed on i.MX6 hardware, where rfkill-gpio is no longer able to enabled/disable a device, presumably because the GPIO controller was never configured for the output direction.
Fixes: b2f750c3a80b ("net: rfkill: gpio: prevent value glitch during probe") Cc: stable@vger.kernel.org Signed-off-by: Rouven Czerwinski r.czerwinski@pengutronix.de
Reviewed-by: Simon Horman horms@kernel.org
linux-stable-mirror@lists.linaro.org
-
Johannes Berg -
kernel test robot -
Philipp Zabel -
Rouven Czerwinski -
Simon Horman