linux-kernel - Re: [PATCH v2 05/13] i2c: acpi: Return error pointers from i2c_acpi_new

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: <98674437-9374-32d4-4b25-dedd0cad5bea@redhat.com>
Date:   Tue, 27 Nov 2018 10:16:25 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Darren Hart <dvhart@...radead.org>,
        platform-driver-x86@...r.kernel.org,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        linux-acpi@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
        Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 05/13] i2c: acpi: Return error pointers from
 i2c_acpi_new_device()

Hi,

On 27-11-18 10:04, Mika Westerberg wrote:
> On Mon, Nov 26, 2018 at 05:08:50PM +0200, Andy Shevchenko wrote:
>> The caller would like to know the reason why the i2c_acpi_new_device() fails.
>> For example, if adapter is not available, it might be in the future and we
>> would like to re-probe the clients again. But at the same time we would like to
>> bail out if the error seems unrecoverable, such as out of memory condition.
>> To achieve this, return error pointer in some cases.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
>> ---
>>   drivers/i2c/i2c-core-acpi.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>> index 32affd3fa8bd..af4b5bd5d973 100644
>> --- a/drivers/i2c/i2c-core-acpi.c
>> +++ b/drivers/i2c/i2c-core-acpi.c
>> @@ -387,6 +387,7 @@ struct notifier_block i2c_acpi_notifier = {
>>    * Also see i2c_new_device, which this function calls to create the i2c-client.
>>    *
>>    * Returns a pointer to the new i2c-client, or NULL if the adapter is not found.
>> + * In some cases might return an error pointer.
> 
> I would rather make it return error pointer always. Then the caller can
> just check for IS_ERR() and not need to deal with the possible NULL. It
> is also more consistent that way than saying "some cases might return an
> error pointer" (but some cases you get NULL or even pointer to the
> created object) ;-)

Good one, that will allow for a nice cleanup of the callers, we can make
i2c_acpi_new_device return -EPROBE_DEFER when the i2c_acpi_find_adapter_by_handle()
call fails, which is exactly the case when we want to defer.

One problem is that i2c_new_device() currently simply returns NULL on all
errors. Andy, you could take a look how much work it is to make that return
an ERR_PTR too, or just check its return value and return ERR_PTR(-ENXIO) if
it fails for now...

Regards,

Hans