diff mbox series

net: rfkill: gpio: set GPIO direction

Message ID	20231206131336.3099727-1-r.czerwinski@pengutronix.de
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; From: Rouven Czerwinski <r.czerwinski@pengutronix.de> To: Josua Mayer <josua@solid-run.com>, Johannes Berg <johannes.berg@intel.com>, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: kernel@pengutronix.de, Rouven Czerwinski <r.czerwinski@pengutronix.de>, stable@vger.kernel.org, Johannes Berg <johannes@sipsolutions.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com> Subject: [PATCH] net: rfkill: gpio: set GPIO direction Date: Wed, 6 Dec 2023 14:13:35 +0100 Message-Id: <20231206131336.3099727-1-r.czerwinski@pengutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	net: rfkill: gpio: set GPIO direction \| net: rfkill: gpio: set GPIO direction

Commit Message

Rouven Czerwinski Dec. 6, 2023, 1:13 p.m. UTC

  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

Comments

Johannes Berg Dec. 6, 2023, 1:16 p.m. UTC | #1

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

Rouven Czerwinski Dec. 6, 2023, 1:24 p.m. UTC | #2

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

Philipp Zabel Dec. 6, 2023, 2:35 p.m. UTC | #3

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

kernel test robot Dec. 6, 2023, 9:41 p.m. UTC | #4

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 mbox series

Patch

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);