* Re: [PATCH] net: rfkill: gpio: set GPIO direction
2023-12-06 13:13 [PATCH] net: rfkill: gpio: set GPIO direction Rouven Czerwinski
@ 2023-12-06 13:16 ` Johannes Berg
2023-12-06 13:24 ` Rouven Czerwinski
2023-12-06 21:41 ` kernel test robot
2023-12-07 7:58 ` [PATCH v2] " Rouven Czerwinski
2 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2023-12-06 13:16 UTC (permalink / raw)
To: Rouven Czerwinski, Josua Mayer, linux-wireless, netdev,
linux-kernel
Cc: kernel, stable, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: rfkill: gpio: set GPIO direction
2023-12-06 13:16 ` Johannes Berg
@ 2023-12-06 13:24 ` Rouven Czerwinski
2023-12-06 14:35 ` Philipp Zabel
0 siblings, 1 reply; 7+ messages in thread
From: Rouven Czerwinski @ 2023-12-06 13:24 UTC (permalink / raw)
To: Johannes Berg, Josua Mayer, linux-wireless, netdev, linux-kernel
Cc: stable, Eric Dumazet, kernel, Jakub Kicinski, Paolo Abeni,
David S. Miller
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: rfkill: gpio: set GPIO direction
2023-12-06 13:24 ` Rouven Czerwinski
@ 2023-12-06 14:35 ` Philipp Zabel
0 siblings, 0 replies; 7+ messages in thread
From: Philipp Zabel @ 2023-12-06 14:35 UTC (permalink / raw)
To: Rouven Czerwinski, Johannes Berg, Josua Mayer, linux-wireless,
netdev, linux-kernel
Cc: stable, Eric Dumazet, kernel, Jakub Kicinski, Paolo Abeni,
David S. Miller
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: rfkill: gpio: set GPIO direction
2023-12-06 13:13 [PATCH] net: rfkill: gpio: set GPIO direction Rouven Czerwinski
2023-12-06 13:16 ` Johannes Berg
@ 2023-12-06 21:41 ` kernel test robot
2023-12-07 7:58 ` [PATCH v2] " Rouven Czerwinski
2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-12-06 21:41 UTC (permalink / raw)
To: Rouven Czerwinski, Josua Mayer, Johannes Berg, linux-wireless,
netdev, linux-kernel
Cc: llvm, oe-kbuild-all, kernel, Rouven Czerwinski, stable,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2] net: rfkill: gpio: set GPIO direction
2023-12-06 13:13 [PATCH] net: rfkill: gpio: set GPIO direction Rouven Czerwinski
2023-12-06 13:16 ` Johannes Berg
2023-12-06 21:41 ` kernel test robot
@ 2023-12-07 7:58 ` Rouven Czerwinski
2023-12-12 9:54 ` Simon Horman
2 siblings, 1 reply; 7+ messages in thread
From: Rouven Czerwinski @ 2023-12-07 7:58 UTC (permalink / raw)
To: Johannes Berg, Josua Mayer, linux-wireless, netdev, linux-kernel
Cc: pza, Rouven Czerwinski, stable, Johannes Berg, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
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
--
2.39.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] net: rfkill: gpio: set GPIO direction
2023-12-07 7:58 ` [PATCH v2] " Rouven Czerwinski
@ 2023-12-12 9:54 ` Simon Horman
0 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2023-12-12 9:54 UTC (permalink / raw)
To: Rouven Czerwinski
Cc: Johannes Berg, Josua Mayer, linux-wireless, netdev, linux-kernel,
pza, stable, Johannes Berg, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
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>
^ permalink raw reply [flat|nested] 7+ messages in thread