| Message ID | 20231206131336.3099727-1-r.czerwinski@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: rfkill: gpio: set GPIO direction | expand |
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-gpio-set-GPIO-direction/20231206-211525 base: 994d5c58e50e91bb02c7be4a91d5186292a895c8 patch link: https://lore.kernel.org/r/20231206131336.3099727-1-r.czerwinski%40pengutronix.de patch subject: [PATCH] net: rfkill: gpio: set GPIO direction config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231207/202312070522.71CNBJ25-lkp@intel.com/config) 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@intel.com/reproduce) 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
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);
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(+) base-commit: 994d5c58e50e91bb02c7be4a91d5186292a895c8