linux-kernel - Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20161015111608.GF13900@localhost>
Date:   Sat, 15 Oct 2016 13:16:08 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Axel Haslam <haslam_axel@...jectara.com>,
        Johan Hovold <johan@...oldconsulting.com>,
        Sandeep Patil <patil_sandeep@...jectara.com>,
        Rui Miguel Silva <rui.silva@...aro.org>
Subject: Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

On Mon, Oct 10, 2016 at 10:39:32AM +0200, Linus Walleij wrote:
> Greybus GPIO seems to reimplement the already existing generic
> gpiolib irqchip. Also use gpiochip_get_data(). Also use
> devm_gpiochip_add_data().
> 
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> Cc: Axel Haslam <haslam_axel@...jectara.com>
> Cc: Johan Hovold <johan@...oldconsulting.com>
> Cc: Sandeep Patil <patil_sandeep@...jectara.com>
> Cc: Rui Miguel Silva <rui.silva@...aro.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> ChangeLog v1->v2:
> - Add the hunk adding select GPIOLIB_IRQCHIP to Kconfig,
>   sorry for sending patches too late in the night.
> 
> Greybus folks: please look at this. I expect something like this
> to be applied before migrating from staging, I can't test it
> obviously.

Yeah, ripping out the inline gpiolib-irqchip backport has been on the
TODO list for quite some time now. This was used to support some ancient
vendor kernels, which we no longer need to worry about. Thanks for
taking a stab at it.

But please split this up in two patches for the gpiolib-irqchip
conversion and the switch to using the new gpiochip_add_data()
interface.

>  static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  			 const struct gbphy_device_id *id)
>  {
> @@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	gpio->get = gb_gpio_get;
>  	gpio->set = gb_gpio_set;
>  	gpio->set_debounce = gb_gpio_set_debounce;
> -	gpio->to_irq = gb_gpio_to_irq;
>  	gpio->base = -1;		/* Allocate base dynamically */
>  	gpio->ngpio = ggc->line_max + 1;
>  	gpio->can_sleep = true;
> @@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	if (ret)
>  		goto exit_line_free;
>  
> -	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
> -				   handle_level_irq, IRQ_TYPE_NONE);
> +	ret = devm_gpiochip_add_data(&connection->bundle->dev,

As Viresh already noted, this should use the gbphy_dev device that is
being probed rather than the parent bundle device.

> +				     gpio, ggc);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add irq chip: %d\n", ret);
> +			"failed to add gpio chip: %d\n", ret);
>  		goto exit_line_free;
>  	}
>  
> -	ret = gpiochip_add(gpio);
> +	ret = gpiochip_irqchip_add(gpio, irqc,
> +				   0, handle_level_irq,
> +				   IRQ_TYPE_NONE);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add gpio chip: %d\n", ret);
> -		goto exit_gpio_irqchip_remove;
> +			"failed to add gpio irqchip: %d\n", ret);
> +		goto exit_line_free;
>  	}
>  
>  	gbphy_runtime_put_autosuspend(gbphy_dev);
>  	return 0;
>  
> -exit_gpio_irqchip_remove:
> -	gb_gpio_irqchip_remove(ggc);

You need to deregister the gpiochip here, before parts of our state is
freed.

>  exit_line_free:
>  	kfree(ggc->lines);
>  exit_connection_disable:

And the connection should really have been disabled before freeing lines
as well (separate prior bug).

> @@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
>  		gbphy_runtime_get_noresume(gbphy_dev);
>  
>  	gb_connection_disable_rx(connection);
> -	gpiochip_remove(&ggc->chip);
> -	gb_gpio_irqchip_remove(ggc);

This is similarly problematic, as again parts of our private state is
released below while the gpiochip is still registered.

I suggest simply not using the devres interface in probe.

>  	gb_connection_disable(connection);
>  	gb_connection_destroy(connection);
>  	kfree(ggc->lines);

Thanks,
Johan