Re: [PATCH] gpio: mpc8xxx: Add shutdown function. - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Hui Song <hui.song_1@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>, Li Yang <leoyang.li@nxp.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH] gpio: mpc8xxx: Add shutdown function.
Date: Thu, 9 Apr 2020 10:37:52 +0100	[thread overview]
Message-ID: <20200409093752.GV25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <20200409090259.13126-1-hui.song_1@nxp.com>

On Thu, Apr 09, 2020 at 05:02:59PM +0800, Hui Song wrote:
> From: Song Hui <hui.song_1@nxp.com>
> 
> When the kexec command is executed, the memory area will be re-paginated.
> The shutdown function needed to make interrupt handler to be NULL.If
> not, an interrupt will be generated during this period. When the interrupt
> handler is executed,the handler function address changed and crash will
> occur.

I still don't think this is adequate.

When kexec takes effect, the existing kernel is shutdown and replaced
with the new kernel, and it completely takes over the system.  The
dynamically allocated structures (which include pointers for the parent
interrupt for mpc8xxx) are irrelevant, as they will need to be
reallocated.  It feels like you're just putting a band-aid over a real
problem.

I think rather than producing a patch, you need to give details of the
crash you are seeing.  Probably having the kernel message log across
the kexec and into the new kernel up to the point where the crash occurs
would be a good idea.

> 
> Signed-off-by: Song Hui <hui.song_1@nxp.com>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 604dfec..a24e6c5 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -446,9 +446,21 @@ static int mpc8xxx_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int mpc8xxx_shutdown(struct platform_device *pdev)
> +{
> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = platform_get_drvdata(pdev);
> +
> +	if (mpc8xxx_gc->irq) {
> +		irq_set_chained_handler_and_data(mpc8xxx_gc->irqn, NULL, NULL);
> +		irq_domain_remove(mpc8xxx_gc->irq);
> +	}
> +
> +	return 0;
> +}
>  static struct platform_driver mpc8xxx_plat_driver = {
>  	.probe		= mpc8xxx_probe,
>  	.remove		= mpc8xxx_remove,
> +	.shutdown	= mpc8xxx_shutdown,
>  	.driver		= {
>  		.name = "gpio-mpc8xxx",
>  		.of_match_table	= mpc8xxx_gpio_ids,
> -- 
> 2.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up