From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CB5C3223C177E for ; Wed, 21 Feb 2018 02:53:58 -0800 (PST) Received: by mail-io0-x242.google.com with SMTP id t126so1708701iof.4 for ; Wed, 21 Feb 2018 02:59:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=CPiRiALZYaOw558yafXNPfY7JB0ySw9Ysgi18i7Vb0E=; b=MbfX4ovpxzsPb7czbtKl4fY7OSuNEXExH8OVz/rIK2V8Jzlba/jyOR2S+XZjT6BMwX jLcJTX/4dIVRQxjoZCYbLLKzTQ8u/XgtHq/NsXPVIFiKXbFfdLEuQ6C0qsdKTLV5zG7s HXS21JX1O5Pg91J97xQbqTsMla0CsoQ03qEHk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=CPiRiALZYaOw558yafXNPfY7JB0ySw9Ysgi18i7Vb0E=; b=ODURHpIi2n8XrPAIOvmBpcgiFuciTsuVSBmvFUao+SdHSdAaV4giXB3IqgOWVky0Ve 7XhrYykN7PvtQcgOoP8+z8PdnCe7Z0+Z4ElEGrSzXNg++kOUQ5gFEQZe56i9gF2SJsTn 3DTLUCx4mrnD9isodUxkMJFxh34ZOxE53ay4oj01ogWj5RnuKGAzVPMZGa71ljz2Djo/ 8XlZ0V01Mlhh7NhqpqySYIRDkyOerzi+FJyDgDB0UXLGGjIz85bkA5hfMOWzjX2+AHq8 iInGkZVR9dBWCVTRaI/G3VS2uNvAZkD40erVNFg8C+J1f6KynYaY89upX6+XgdmZvJx5 eqmA== X-Gm-Message-State: APf1xPB0OUEFiSiN1vsAW/x1ILMx/CTuXBODAwQeYWqdh3YNjc2KINGB 0D/ztfX6ddEY+ZDHEsEqazQFrHBNYZ/KScuXzGq0xohpwo0= X-Google-Smtp-Source: AG47ELsJ1i/YRHE+MDa+HaMz256lRrDuDnTDgar2y4Ux0NKuPobxoLXR1JrpLGYvsmYnuoUcIuP8jw0AhE54ctjxzLM= X-Received: by 10.107.151.74 with SMTP id z71mr3349443iod.277.1519210797239; Wed, 21 Feb 2018 02:59:57 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Wed, 21 Feb 2018 02:59:56 -0800 (PST) In-Reply-To: <20180221105422.pwnwieugy6tsdp7z@bivouac.eciton.net> References: <20180221101918.16088-1-ard.biesheuvel@linaro.org> <20180221102448.d5liacahtcofcbm7@bivouac.eciton.net> <20180221105422.pwnwieugy6tsdp7z@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 21 Feb 2018 10:59:56 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacerI2cDxe: fix TPL handling bug X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Feb 2018 10:53:59 -0000 Content-Type: text/plain; charset="UTF-8" On 21 February 2018 at 10:54, Leif Lindholm wrote: > On Wed, Feb 21, 2018 at 10:44:05AM +0000, Ard Biesheuvel wrote: >> On 21 February 2018 at 10:24, Leif Lindholm wrote: >> > On Wed, Feb 21, 2018 at 10:19:18AM +0000, Ard Biesheuvel wrote: >> >> Currently, SynQuacerI2cStartRequest() increases the TPL to TPL_HIGH_LEVEL >> >> while accessing the I2C controller hardware, but fails to restore the TPL >> >> to the original level if the call to SynQuacerI2cMasterStart() fails, and >> >> returns right away. Given the TPL_HIGH_LEVEL implies that interrupts are >> >> disabled, this results in a complete system hang. So instead, break out >> >> of the loop, so that the TPL restore will occur before leaving the function. >> >> >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> >> Signed-off-by: Ard Biesheuvel >> >> --- >> >> Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c >> >> index 46c512a20151..b2318a6f5a8c 100644 >> >> --- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c >> >> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c >> >> @@ -324,7 +324,7 @@ SynQuacerI2cStartRequest ( >> >> >> >> Status = SynQuacerI2cMasterStart (I2c, SlaveAddress, Op); >> >> if (EFI_ERROR (Status)) { >> >> - return Status; >> >> + break; >> >> } >> >> >> >> Status = WaitForInterrupt (I2c); >> > >> > This change also causes >> > >> > // Stop the transfer >> > MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, 0); >> > >> > to be executed in the faulting case. >> > >> > Which at the very least looks quirky. >> > >> >> I don't think it is unreasonable in general to stop any ongoing >> transfer no matter how we leave this function. And in this particular >> case, one of the failure modes is F_I2C_BCR_MSS being set without the >> bus being busy as a result, and so clearing BCR is actually rather >> appropriate here. > > So stopping an ongoing transfer even if none have been started (as is > possible here) is a non-issue? Well, there are two ways SynQuacerI2cMasterStart() can fail: - the bus is busy and no transfer was started by *this* master - an attempt to start a transfer was made but the bus is not busy as a result The former case can only occur in the presence of multiple masters. The latter can probably only occur if some kind of corruption occurs on the bus (I'm not sure, and this part originates in the Fujitsu code) In the latter case, F_I2C_BCR_MSS will be asserted, and de-asserting it is the correct thing to do. > If so, I have no objection (but would > appreciate that mentioned where the "// Stop the transfer" comment > currently is). > > (("// Force bus state to idle, terminating any ongoing transfer"))? > I can fix that, sure. > And could commit message mention this change in behaviour please? > OK