linux-kernel - Re: [PATCH v5 3/3] i2c: designware: Add doorbell support for Mendocino

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: <a9993ba0-ddfb-5ed3-c997-e9eea797cfc3@amd.com>
Date:   Wed, 22 Mar 2023 12:12:19 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Mark Hasemeyer <markhas@...omium.org>
Cc:     Grzegorz Bernacki <gjb@...ihalf.com>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Jan Dabros <jsd@...ihalf.com>, Held Felix <Felix.Held@....com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] i2c: designware: Add doorbell support for
 Mendocino


On 3/22/23 12:01, Mark Hasemeyer wrote:
>> +static int psp_send_i2c_req_mendocino(enum psp_i2c_req_type i2c_req_type)
>> +{
>> +       int status, ret;
>> +
>> +       ret = read_poll_timeout(psp_ring_platform_doorbell, status,
>> +                               (status != -EBUSY),
>> +                               PSP_I2C_REQ_RETRY_DELAY_US,
>> +                               PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US,
>> +                               0, i2c_req_type);
>> +       if (ret)
>> +               dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n",
>> +                       (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ?
>> +                       "release" : "acquire");
>> +
>> +       return ret ? ret : status;
>> +}
> I think we need the value of the PSP_CMDRESP_STS field returned to the caller
> and its status checked like in psp_send_i2c_req_cezanne. Otherwise the function
> won't continue to poll when the PSP_I2C_REQ_STS_BUS_BUSY bit is set.

In that case it looks like psp_send_check_i2c_req isn't handling this 
properly
for Cezanne either in this series.  I think psp_send_platform_access_msg
returning -EIO is going to mean that check_i2c_req_sts never gets run.

So either psp_send_platform_access_msg should return 0 for that case
(expecting caller to investigate more closely) or psp_send_check_i2c_req
needs to special case -EIO for more investigation.

I lean upon the latter unless you have a strong opinion.

>
> FYI - there's a test on ChromeOS to stress test I2C bus arbitration:
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/hwsec/tpm_contest.go
> I can try to run it assuming the ToT kernel runs on skyrim.

I would expect ToT should run fine on Skyrim.  I'll adjust as you suggested
if you can please test it.