linux-kernel - Re: [PATCH v2 2/2] i2c: i2c-smbus: fwnode_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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4bf2c0bf-27bf-f4d1-f72a-0f82c53f650c@fi.rohmeurope.com>
Date:   Thu, 27 Oct 2022 05:40:20 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Akhil R <akhilrajeev@...dia.com>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "jdelvare@...e.de" <jdelvare@...e.de>
Subject: Re: [PATCH v2 2/2] i2c: i2c-smbus: fwnode_irq_get_byname() return
 value fix

On 10/25/22 19:30, Andy Shevchenko wrote:
> On Tue, Oct 25, 2022 at 06:12:11PM +0300, Matti Vaittinen wrote:
>> The fwnode_irq_get_byname() was changed to not return 0 upon failure so
>> return value check can be adjusted to reflect the change.
> 
> ...
> 
>> Depends on the mentioned return value change which is in patch 1/2. The
>> return value change does also cause a functional change here. Eg. when
>> IRQ mapping fails, the fwnode_irq_get_byname() no longer returns zero.
>> This will cause also the probe here to return nonzero failure. I guess
>> this is desired behaviour.
> 
> The entire error handling there looks suspicious.
> 
> The 'struct i2c_smbus_alert_setup' description says:
> 
>   "If irq is not specified, the smbus_alert driver doesn't take care of
>    interrupt handling. In that case it is up to the I2C bus driver to
>    either handle the interrupts or to poll for alerts."
> 
> So, the question is, shouldn't we just drop the check completely?

I don't really know what this means. Does it mean that if IRQ is not 
provided, the driver needs to take care of alerts (in which case the 
check here is very valid because IRQ is required for smbus_alert 
driver). Or does it mean that only the IRQ handling is omitted while the 
smbus_alert driver should do all the other stuff (what ever that is) as 
usual. In this case this check indeed feels wrong.

I would appreciate someone with more insight to this driver to take a 
look at it.

Yours
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~