From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 9 Mar 2015 14:34:01 +0200 From: Vlad Dogaru To: Jonathan Cameron Cc: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , Daniel Baluta , Roberta Dobrescu , "linux-iio@vger.kernel.org" , "octavian.purdila@intel.com" , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , kernel@pengutronix.de Subject: Re: [PATCH] iio: accel: mma9551: Check gpiod_to_irq return value Message-ID: <20150309123400.GB32609@vdogaru> References: <1425292752-7768-1-git-send-email-roberta.dobrescu@gmail.com> <20150302161503.GE7865@pengutronix.de> <20150303070232.GC27074@vdogaru> <54FC2C83.5090802@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 In-Reply-To: <54FC2C83.5090802@kernel.org> List-ID: On Sun, Mar 08, 2015 at 11:03:31AM +0000, Jonathan Cameron wrote: > On 03/03/15 07:02, Vlad Dogaru wrote: > > On Mon, Mar 02, 2015 at 05:15:03PM +0100, Uwe Kleine-König wrote: > >> Hello, > >> > >> On Mon, Mar 02, 2015 at 04:09:52PM +0200, Daniel Baluta wrote: > >>> On Mon, Mar 2, 2015 at 12:39 PM, Roberta Dobrescu > >>> wrote: > >>>> The return value of gpiod_to_irq should be checked before giving > >>>> it to devm_request_threaded_irq in order to not pass an error > >>>> code in case it fails. > >> nothing really bad should happen because request_irq with a negative irq > >> parameter just returns an error I think. So it's not urgent, but still a > >> good idea to fix. > >> > >>>> Signed-off-by: Roberta Dobrescu > >>>> Reviewed-by: Vlad Dogaru > >> It's good habit to point out the commit that introduced the problem. In > >> this case this would be: > >> > >> Fixes: c78b91716340 ("iio: add driver for Freescale MMA9551L") > Vlad, do you want to respin taking the comments into account, or as Uwe > put it, it's worth having as is so should I consider it as it stands? I got slightly confused in my previous mail because the code was pasted from the sx9500 driver. That's the one I said I was working on. The original mma9551 patch looks good as it is, please apply it. Acked-by: Vlad Dogaru Thanks, Vlad > >> > >>>> --- > >>>> gpiod_to_irq also appears in the following drivers: > >>>> * drivers/iio/accel/bmc150-accel.c > >>>> * drivers/iio/accel/kxcjk-1013.c > >>>> * drivers/iio/accel/mma9553.c > >>>> * drivers/iio/gyro/bmg160.c > >>>> * drivers/iio/imu/kmx61.c > >>>> * drivers/iio/proximity/sx9500.c, > >>>> > >>>> something like this: > >>>> > >>>> > >>>> ret = gpiod_to_irq(gpio); > >>>> > >>>> dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret); > >>>> > >>>> return ret; > >>>> > >>>> > >>>> The return value of the functions containing the above code is checked, > >>>> so the only problem would be that the debug message would contain a wrong > >>>> value for irq in case gpiod_to_irq fails. So it doesn't affects much. > >> Still worth fixing, isn't it? Also the error isn't handled, but ignored, > >> like: > >> > >> if (client->irq <= 0) > >> client->irq = sx9500_gpio_probe(client, data); > >> > >> if (client->irq > 0) { > >> ret = devm_request_threaded_irq(.... > >> > >> but if an irq is specified (be it by means of a "normal" irq or by > >> specifying a gpio in the device tree/acpi tables) I expect the driver to > >> fail probing instead of just behaving as if no irq would be available. > > > > If there is no IRQ available this device would still be able to do raw > > reads, although I admit I have not tested this. > > > >> I don't know how this was tested, but I wonder further about > >> > >> #define SX9500_GPIO_NAME "sx9500_gpio" > >> ... > >> devm_gpiod_get_index(dev, SX9500_GPIO_NAME, 0, GPIOD_IN); > >> > >> If I understand the code correctly that is supposed to look for > >> "sx9500_gpio-gpio" in the ACPI data. Is this really correct? > > > > I'm in the process of changing this, will post some patches soon. This > > should include failing to probe if an IRQ is not found. > > > > At the time I wrote the driver, I wasn't using Device Tree and ACPI had > > no support for _DSD (or at least I wasn't aware of it), so the name did > > not matter, only the index. > > > > Thanks for the > > > >>>> drivers/iio/accel/mma9551.c | 6 +++++- > >>>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/iio/accel/mma9551.c b/drivers/iio/accel/mma9551.c > >>>> index 46c3835..b6f3041 100644 > >>>> --- a/drivers/iio/accel/mma9551.c > >>>> +++ b/drivers/iio/accel/mma9551.c > >>>> @@ -428,7 +428,11 @@ static int mma9551_gpio_probe(struct iio_dev *indio_dev) > >>>> if (ret) > >>>> return ret; > >>>> > >>>> - data->irqs[i] = gpiod_to_irq(gpio); > >>>> + ret = gpiod_to_irq(gpio); > >>>> + if (ret < 0) > >>>> + return ret; > >> I wonder if you should handle 0 as error, too. But even as is: > >> > >> Acked-by: Uwe Kleine-König > >> > >> Thanks > >> Uwe > >> > >> -- > >> Pengutronix e.K. | Uwe Kleine-König | > >> Industrial Linux Solutions | http://www.pengutronix.de/ | >