linux-kernel - Re: [PATCH i2c-next v3 2/3] i2c: aspeed: Add 'aspeed,timeout' DT property reading code

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: <37ae7a11-44b6-88e5-0f4d-c97c70334ad4@linux.intel.com>
Date:   Thu, 27 Sep 2018 10:41:16 -0700
From:   Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
To:     Joel Stanley <joel@....id.au>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Andrew Jeffery <andrew@...id.au>, linux-i2c@...r.kernel.org,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-aspeed@...ts.ozlabs.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        jarkko.nikula@...ux.intel.com,
        James Feist <james.feist@...ux.intel.com>,
        Vernon Mauery <vernon.mauery@...ux.intel.com>
Subject: Re: [PATCH i2c-next v3 2/3] i2c: aspeed: Add 'aspeed,timeout' DT
 property reading code

Hi Joel,

On 9/26/2018 8:11 PM, Joel Stanley wrote:
> On Thu, 27 Sep 2018 at 01:58, Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com> wrote:
>>
>> +/* Timeout */
>> +#define ASPEED_I2C_BUS_TIMEOUT_US_DEFAULT              (5 * 1000 * 1000)
> 
> The 5 seconds time out is way too long. On a system that doesn't have
> functional i2c, this holds up boot for a long time as most i2c client
> drivers try to initialise their device and fail. I realise you're not
> changing the value, but we should pick a better default. 1 second?
> Half a second?
> 

I agree with you. We could probably use 1 second as default which can
cover the most of general cases. If so, we don't need to make the
default setting in this driver because i2c-core-base will default
adap->timeout to 1 second if the value is 0 when a driver registers an
adapter. Will fix this code.

>>
>> +       ret = of_property_read_u32(pdev->dev.of_node, "aspeed,timeout",
>> +                                  &timeout_us);
> 
> Can we make this binding generic? It's not specific to aspeed's
> hardware. Getting the value could even part of the i2c core.
> 
> I read the previous thread with Wolfram. I think this would still fit
> with what Wolfram suggested, but please forgive my jetlagged brain if
> I've missed something.
> 

It followed the way of the existing i2c-mpc driver uses 'fsl,timeout'
for the same purpose. Though, I also want to make it as a generic as you
suggested like 'timeout' in milliseconds unit, not in microseconds unit.
If making it a generic property is acceptable, I'll fix it too.

Wolfram, can you please share your thought on it?

Thanks,
Jae