linux-kernel - Re: [PATCH] i2c: core: helper function to detect slave mode

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f751ffbe-f9f8-c5bd-3b1a-2edd91152a78@mleia.com>
Date:   Tue, 17 Jan 2017 01:14:17 +0200
From:   Vladimir Zapolskiy <vz@...ia.com>
To:     Luis Oliveira <Luis.Oliveira@...opsys.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Wolfram Sang <wsa@...-dreams.de>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-i2c@...r.kernel.org, devicetree <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ramiro.Oliveira@...opsys.com, Joao Pinto <Joao.Pinto@...opsys.com>,
        CARLOS.PALMINHA@...opsys.com
Subject: Re: [PATCH] i2c: core: helper function to detect slave mode

Hello Luis,

On 01/16/2017 12:32 PM, Luis Oliveira wrote:
> On 12-Jan-17 17:01, Andy Shevchenko wrote:
>> On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
>>> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
>>>> On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@...ia.com>
>>>> wrote:
>>>>> On 01/07/2017 12:45 AM, Andy Shevchenko wrote:
>>
>>>>>> +             }
>>>>>>>> +     } else if (IS_BUILTIN(CONFIG_ACPI) &&
>>>>>>>> ACPI_HANDLE(dev)) {
>>>>>>>> +             dev_dbg(dev, "ACPI slave is not supported
>>>>>>>> yet\n");
>>>>>>>> +     }
>>>>>>>
>>>>>>> If so, then it might be better to drop else-if stub for now.
>>>>>>
>>>>>> Please, don't.
>>>>>>
>>>>>
>>>>> Why do you ask for this stub to be added?
>>>>
>>>> 1. Exactly the reason you asked above. Here is the code which has
>>>> built differently on different platforms. x86 usually is not using
>>>> CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
>>>> existing examples.
>>>
>>> From the context by the stub I mean dev_dbg() in
>>> i2c_slave_mode_detect()
>>> function, I don't see a connection to GPIO library, please clarify.
>>
>> I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.
> 
> I can prepare a V3 and remove it if that's the decision.
> 

from my review comments you may find that your v2 change contains a bug
plus it misses a minor but desirable code optimization. You may get more
review comments then, for example it is not obvious that on a platform
with both CONFIG_ACPI and CONFIG_OF enabled there should be an exclusive
selection of only one of two possible branches as in your code etc.

The discussed IS_BUILTIN() and dev_dbg() code chunks are other ones, for
both of them you may find my review comments and Andy's responses, it's
up to you as the author to make any updates or keep the code as is,
taking into account any expressed concerns and concerns about concerns
the maintainer will make a decision.

--
With best wishes,
Vladimir