linux-kernel - Re: [PATCH v8 2/8] iio: cdc: ad7150: relax return value check for IRQ get

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: <20230801185454.0d115aaa@jic23-huawei>
Date:   Tue, 1 Aug 2023 18:54:54 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/8] iio: cdc: ad7150: relax return value check for
 IRQ get

On Tue, 1 Aug 2023 15:02:47 +0300
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> fwnode_irq_get[_byname]() were changed to not return 0 anymore. The
> special error case where device-tree based IRQ mapping fails can't no
> longer be reliably detected from this return value. This yields a
> functional change in the driver where the mapping failure is treated as
> an error.
> 
> The mapping failure can occur for example when the device-tree IRQ
> information translation call-back(s) (xlate) fail, IRQ domain is not
> found, IRQ type conflicts, etc. In most cases this indicates an error in
> the device-tree and special handling is not really required.
> 
> One more thing to note is that ACPI APIs do not return zero for any
> failures so this special handling did only apply on device-tree based
> systems.
> 
> Drop the special handling for DT mapping failures as these can no longer
> be separated from other errors at driver side. Change all failures in
> IRQ getting to be handled by continuing without the events instead of
> aborting the probe upon certain errors.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Again, fiddled tags so I don't have RB and SoB

Applied to the togreg branch of iio.git and pushed out as testing for
all the normal reasons.

Thanks,

Jonathan

> 
> ---
> Revision history:
> v5 => v6:
>  - Never abort the probe when IRQ getting fails but continue without
>    events.
> 
> Please note that I don't have the hardware to test this change.
> Furthermore, testing this type of device-tree error cases is not
> trivial, as the question we probably dive in is "what happens with the
> existing users who have errors in the device-tree". Answering to this
> question is not simple.
> 
> The patch changing the fwnode_irq_get() got merged during 5.4:
> https://lore.kernel.org/all/fb7241d3-d1d1-1c37-919b-488d6d007484@gmail.com/
> This is a clean-up as agreed.
> ---
>  drivers/iio/cdc/ad7150.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/cdc/ad7150.c b/drivers/iio/cdc/ad7150.c
> index d656d2f12755..4c03b9e834b8 100644
> --- a/drivers/iio/cdc/ad7150.c
> +++ b/drivers/iio/cdc/ad7150.c
> @@ -541,6 +541,7 @@ static int ad7150_probe(struct i2c_client *client)
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	struct ad7150_chip_info *chip;
>  	struct iio_dev *indio_dev;
> +	bool use_irq = true;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> @@ -561,14 +562,13 @@ static int ad7150_probe(struct i2c_client *client)
>  
>  	chip->interrupts[0] = fwnode_irq_get(dev_fwnode(&client->dev), 0);
>  	if (chip->interrupts[0] < 0)
> -		return chip->interrupts[0];
> -	if (id->driver_data == AD7150) {
> +		use_irq = false;
> +	else if (id->driver_data == AD7150) {
>  		chip->interrupts[1] = fwnode_irq_get(dev_fwnode(&client->dev), 1);
>  		if (chip->interrupts[1] < 0)
> -			return chip->interrupts[1];
> +			use_irq = false;
>  	}
> -	if (chip->interrupts[0] &&
> -	    (id->driver_data == AD7151 || chip->interrupts[1])) {
> +	if (use_irq) {
>  		irq_set_status_flags(chip->interrupts[0], IRQ_NOAUTOEN);
>  		ret = devm_request_threaded_irq(&client->dev,
>  						chip->interrupts[0],