Re: [PATCH v2] hwmon: (sht15) Stop deferred work before releasing its resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwmon: (sht15) IRQF_NO_AUTOEN in sht15_probe() breaks gpiod_direction_output()
--

commit 0765de729006e4101f713c8613f8a3c9ed6e7d2e
Author: Myeonghun Pak <mhun512@gmail.com>

hwmon: (sht15) Stop deferred work before releasing its resources

This commit switches the SHT15 driver from devm_request_irq() to
request_irq() and free_irq(), adds IRQF_NO_AUTOEN, and drains
read_work and update_supply_work during probe failure and device removal.

> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 7bb2105d1ae22..c95acec7ae2f8 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
[ ... ]
> @@ -976,25 +979,31 @@ static int sht15_probe(struct platform_device *pdev)
>  		goto err_release_reg;
>  	}
>  
> -	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->data),
> -			       sht15_interrupt_fired,
> -			       IRQF_TRIGGER_FALLING,
> -			       "sht15 data",
> -			       data);
> -	if (ret)
> +	data->irq = gpiod_to_irq(data->data);
> +	if (data->irq < 0) {
> +		ret = data->irq;
> +		goto err_release_reg;
> +	}
> +
> +	ret = request_irq(data->irq, sht15_interrupt_fired,
> +			  IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
> +			  "sht15 data", data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get irq for data line\n");
>  		goto err_release_reg;
> -	disable_irq_nosync(gpiod_to_irq(data->data));
> +	}
> +	/* IRQ remains disabled until a measurement starts. */
>  	ret = sht15_connection_reset(data);
>  	if (ret)
> -		goto err_release_reg;
> +		goto err_free_irq;

[Severity: High]
Does replacing disable_irq_nosync() with IRQF_NO_AUTOEN cause
sht15_connection_reset() to fail with -EIO on probe?

During request_irq(), __setup_irq() activates the IRQ and invokes
gpiochip_lock_as_irq() in drivers/gpio/gpiolib.c, which unconditionally sets
both GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_IRQ_IS_ENABLED on data->data:

drivers/gpio/gpiolib.c:gpiochip_lock_as_irq() {
	...
	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);
	...
}

When IRQF_NO_AUTOEN is passed, __setup_irq() in kernel/irq/manage.c skips
irq_startup() and only sets desc->depth = 1 without calling irq_disable() or
gpiochip_disable_irq():

kernel/irq/manage.c:__setup_irq() {
	...
	if (!(new->flags & IRQF_NO_AUTOEN) &&
	    irq_settings_can_autoenable(desc)) {
		irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
	} else if (!per_cpu_devid) {
		WARN_ON_ONCE(new->flags & IRQF_SHARED);
		/* Undo nested disables: */
		desc->depth = 1;
	}
	...
}

Previously, calling disable_irq_nosync() after requesting the IRQ invoked
irq_disable() -> gpiochip_disable_irq(), which cleared
GPIOD_FLAG_IRQ_IS_ENABLED. With IRQF_NO_AUTOEN, GPIOD_FLAG_IRQ_IS_ENABLED
remains set when sht15_connection_reset() calls
gpiod_direction_output(data->data, 1):

drivers/gpio/gpiolib.c:gpiod_direction_output_nonotify() {
	...
	/* GPIOs used for enabled IRQs shall not be set as output */
	if (test_bit(GPIOD_FLAG_USED_AS_IRQ, &flags) &&
	    test_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &flags)) {
		gpiod_err(desc,
			  "%s: tried to set a GPIO tied to an IRQ as output\n",
			  __func__);
		return -EIO;
	}
	...
}

Will this cause gpiod_direction_output() in sht15_connection_reset() (and in
sht15_transmission_start() before the first enable_irq()) to return -EIO
whenever data->data is backed by a gpiolib irqchip?

[ ... ]

ওয়াট

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019234402.805759-1-mhun512@gmail.com?part=1