linux-kernel - Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240826110150.5f2e5c72@jic23-huawei>
Date: Mon, 26 Aug 2024 11:01:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, lars@...afoo.de,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 ang.iglesiasg@...il.com, linus.walleij@...aro.org,
 biju.das.jz@...renesas.com, javier.carrasco.cruz@...il.com,
 semen.protsenko@...aro.org, 579lpy@...il.com, ak@...klinger.de,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 6/7] iio: pressure: bmp280: Add data ready trigger
 support

On Sat, 24 Aug 2024 14:02:22 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote:  
> > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> > > a trigger for when there are data ready in the sensor for pick up.
> > > 
> > > This use case is used along with NORMAL_MODE in the sensor, which allows
> > > the sensor to do consecutive measurements depending on the ODR rate value.
> > > 
> > > The trigger pin can be configured to be open-drain or push-pull and either
> > > rising or falling edge.
> > > 
> > > No support is added yet for interrupts for FIFO, WATERMARK and out of range
> > > values.  
> > 
> > ...
> >   
> > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > > +				  const struct iio_trigger_ops *trigger_ops,
> > > +				  int (*int_config)(struct bmp280_data *data),  
> >   
> > > +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))  
> > 
> > irq_handler_t
> >   
> 
> But the function returns an irqreturn_t type, no?
irq_handler_t is a typdef for the full function signature.
It will still return irqreturn_t 
> 
> > ...
> >   
> > > +	fwnode = dev_fwnode(data->dev);
> > > +	if (!fwnode)
> > > +		return -ENODEV;  
> > 
> > Why do you need this? The below will fail anyway.  
> 
> Because If I don't make this check then fwnode might be garbage and I will
> pass garbage to the fwnode_irq_get() function. Or do I miss something?
It checks for NULL which is all it can actually be and returns a suitable
error code if it is.

> 
> >   
> > > +	irq = fwnode_irq_get(fwnode, 0);
> > > +	if (!irq)  
> > 
> > Are you sure this is correct check?
> >   
> Well, I think yes, because the function return either the Linux IRQ number
> on success or a negative errno on failure.
> 
> https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

Indeed, so if (irq < 0)
		return dev_err_probe(data->dev, irq, ...)

	carry on as valid irq.
your error check if returning only if irq == 0 which never
happens (due to catch for that in the code you link).
Negative values are true, so !-EINVAL == false
for example.
>