mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
Cc: Bartosz Golaszewski <brgl@kernel.org>,
	Linus Walleij <linusw@kernel.org>,
	Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>,
	Kent Gibson <warthog618@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Herve Codina <herve.codina@bootlin.com>,
	Jia Jia <physicalmtea@gmail.com>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH 3/6] gpiolib: cdev: do not free line IRQs again after the chip is removed
Date: Tue, 13 Oct 2026 18:44:33 +0300	[thread overview]
Message-ID: <as5R4WDqXfMXkD2L@ashevche-desk.local> (raw)
In-Reply-To: <20261012-gpiolib-chip-removal-v1-3-aambrozewicz@nvidia.com>

On Mon, Oct 12, 2026 at 04:49:57PM +0300, Adrian Ambrozewicz wrote:
> Since commit ec8b6f55b981 ("gpio: free irqs that are still requested
> when the chip is being removed"), gpiochip_remove() calls free_irq() for
> every action still installed on the lines of the chip. The character
> device does not know about this: a line request or v1 line event that
> user space keeps open across the removal still holds the IRQ number and
> calls free_irq() on it again when it is released.
> 
> What that second free_irq() does depends on what happened to the IRQ
> number in the meantime. If the chip disposed of its mapping, the
> descriptor is gone and the call returns early, or, when it races with
> the RCU-deferred free of the descriptor, it touches freed memory, as
> reported with virtio-gpio. If the number was handed out again, for
> instance because the same chip was set up again, the call finds a live
> descriptor without our action and warns:
> 
>   Trying to free already-free IRQ 28
>   WARNING: kernel/irq/manage.c:1881 at free_irq+0x193/0x2d0, CPU#2: gpio-hold/111
>   Call Trace:
>    <TASK>
>    linereq_free+0xd2/0x120
>    linereq_release+0xd/0x20
>    __fput+0xed/0x2a0
>    __x64_sys_close+0x38/0x80
>    do_syscall_64+0x109/0x5d0
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
>    </TASK>
> 
> Stop edge detection from the device notifier that gpiochip_remove()
> already calls into, rather than when the request is released, and only
> free the IRQ there if it still has an action: chips that never mark
> their lines as used for IRQs, which gpiochip_lock_as_irq() leaves to
> the driver, keep theirs past gpiod_free_irqs(). The release path then
> finds no IRQ left to free. The notifier takes the request's
> config_mutex, which is otherwise only taken by the ioctls, so that it
> does not race with a concurrent reconfiguration of the lines.
> 
> Reproduced with gpio-sim: request a line with edge detection, set the
> chip's configfs "live" attribute to 0 while the request is open, set it
> up again and request the same line, then close the first request. Each
> of 30 cycles warns as above, for v2 requests with and without software
> debouncing and for v1 line events; with this change, 100 cycles of each
> pass without a warning.

...

> +/*
> + * gpiochip_remove() frees the interrupts still requested on the lines of the
> + * chip being removed, so once the chip is going away the IRQ may no longer be
> + * ours to free.
> + */
> +static void free_line_irq(unsigned int irq, void *dev_id)
> +{
> +	if (irq_has_action(irq))
> +		free_irq_label(free_irq(irq, dev_id));

In all cases it also checks for irq to be valid, so embed it here and drop
those checks below.

	if (irq && irq_has_action(irq))

> +}

...

>  static void edge_detector_stop(struct line *line)
>  {
>  	if (line->irq) {
> -		free_irq_label(free_irq(line->irq, line));
> +		free_line_irq(line->irq, line);
>  		line->irq = 0;
>  	}

This, for instance, will become

	free_line_irq(line->irq, line);
	line->irq = 0;


...

> +static int linereq_unregistered_notify(struct notifier_block *nb,
> +				       unsigned long action, void *data)
> +{
> +	struct linereq *lr = container_of(nb, struct linereq,
> +					  device_unregistered_nb);

Slightly better to have it

	struct linereq *lr = container_of(nb, struct linereq, device_unregistered_nb);

or

	struct linereq *lr =
		container_of(nb, struct linereq, device_unregistered_nb);

> +	unsigned int i;

Not used outside of the loop.

> +	/*
> +	 * Stop edge detection now rather than when the request is released:
> +	 * by then the IRQ numbers of the removed chip may be in use again.
> +	 */
> +	scoped_guard(mutex, &lr->config_mutex) {
> +		for (i = 0; i < lr->num_lines; i++)

		for (unsigned int i = 0; i < lr->num_lines; i++)

> +			edge_detector_stop(&lr->lines[i]);
> +	}
> +
> +	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
> +
> +	return NOTIFY_OK;
> +}

-- 
With Best Regards,
Andy Shevchenko



  reply	other threads:[~2026-10-13 15:44 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-12 13:49 [PATCH 0/6] gpiolib: fix chip removal with lines still requested from user space Adrian Ambrozewicz
2026-10-12 13:49 ` [PATCH 1/6] gpiolib: wait for pending label frees before cleaning up desc_srcu Adrian Ambrozewicz
2026-10-12 15:46   ` Paul E. McKenney
2026-10-12 20:42   ` Andy Shevchenko
2026-10-12 13:49 ` [PATCH 2/6] gpiolib: free the label of lines released after their chip was removed Adrian Ambrozewicz
2026-10-13 15:38   ` Andy Shevchenko
2026-10-12 13:49 ` [PATCH 3/6] gpiolib: cdev: do not free line IRQs again after the chip is removed Adrian Ambrozewicz
2026-10-13 15:44   ` Andy Shevchenko [this message]
2026-10-12 13:49 ` [PATCH 4/6] gpiolib: cdev: don't sleep in the SRCU read section in read() Adrian Ambrozewicz
2026-10-13 15:48   ` Andy Shevchenko
2026-10-12 13:49 ` [PATCH 5/6] gpiolib: cdev: don't leak the IRQ name when the chip is removed Adrian Ambrozewicz
2026-10-13 15:59   ` Andy Shevchenko
2026-10-12 13:50 ` [PATCH 6/6] gpiolib: let the character device free its IRQs first on chip removal Adrian Ambrozewicz
2026-10-13 16:05   ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=as5R4WDqXfMXkD2L@ashevche-desk.local \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=aambrozewicz@nvidia.com \
    --cc=bartosz.golaszewski@oss.qualcomm.com \
    --cc=brgl@kernel.org \
    --cc=herve.codina@bootlin.com \
    --cc=linusw@kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulmck@kernel.org \
    --cc=physicalmtea@gmail.com \
    --cc=stable@vger.kernel.org \
    --cc=warthog618@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®