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: [email protected]
Signed-off-by: Rouven Czerwinski <[email protected]>
---
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
--
2.39.2
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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: [email protected]
Signed-off-by: Rouven Czerwinski <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Rouven Czerwinski <[email protected]>
Reviewed-by: Simon Horman <[email protected]>