Re: [PATCH edk2-platforms v1 07/38] Silicon/Hisilicon/D06: Wait for all disk ready

[Ming <ming.huang@linaro.org>]

在 8/8/2018 8:53 PM, Leif Lindholm 写道:
> On Wed, Aug 08, 2018 at 07:44:36PM +0800, Ming wrote:
>>
>>
>> 在 8/8/2018 5:59 PM, Leif Lindholm 写道:
>>> On Wed, Aug 08, 2018 at 05:02:15PM +0800, Ming wrote:
>>>> 在 8/3/2018 1:36 AM, Leif Lindholm 写道:
>>>>> On Tue, Jul 24, 2018 at 03:08:51PM +0800, Ming Huang wrote:
>>>>>> This patch is relative to D06 SasDxe driver. The SasDxe set a
>>>>>> variable to notice this libray. Here Wait for all disk ready
>>>>>> for 30S at most.
>>>>>>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>>>> ---
>>>>>>  Silicon/Hisilicon/HisiPkg.dec                                               |  1 +
>>>>>>  Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c               | 43 ++++++++++++++++++++
>>>>>>  Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  2 +
>>>>>>  3 files changed, 46 insertions(+)
>>>>>>
>>>>>> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec
>>>>>> index 35bea970ec..b56a6a6af7 100644
>>>>>> --- a/Silicon/Hisilicon/HisiPkg.dec
>>>>>> +++ b/Silicon/Hisilicon/HisiPkg.dec
>>>>>> @@ -45,6 +45,7 @@
>>>>>>  
>>>>>>    gHisiEfiMemoryMapGuid  = {0xf8870015, 0x6994, 0x4b98, {0x95, 0xa2, 0xbd, 0x56, 0xda, 0x91, 0xc0, 0x7f}}
>>>>>>    gVersionInfoHobGuid = {0xe13a14c, 0x859c, 0x4f22, {0x82, 0xbd, 0x18, 0xe, 0xe1, 0x42, 0x12, 0xbf}}
>>>>>> +  gHisiOemVariableGuid = {0xac62b9a5, 0x9939, 0x41d3, {0xff, 0x5c, 0xc5, 0x80, 0x32, 0x7d, 0x9b, 0x29}}
>>>>>>    gOemBootVariableGuid = {0xb7784577, 0x5aaf, 0x4557, {0xa1, 0x99, 0xd4, 0xa4, 0x2f, 0x45, 0x06, 0xf8}}
>>>>>
>>>>> What is the difference between gHisiOemVariableGuid and gOemBootVariableGuid?
>>>>>
>>>>
>>>> Just a guid using to a variable.
>>>> Do you suggest to use the old one?
>>>
>>> Well, I am unsure about the intended scope for either. So I don't
>>> know. How is HisiOem different from Oem? Can you explain the structure?
>>
>> HisiOem and Oem are the same scope, just need a guid for SetVariable.
> 
> The only purpose for the GUID is so that you can have variables of the
> same name in different modules without risking clashes. Sharing the
> same variable GUID across multiple modules is perfectly fine (and
> often preferable) when those modules are closely related.
> 
> But as I think this was only used for the communication with the SAS
> controller driver, I think maybe this one can just go.

OK.

> 
>>>>>>    gEfiHisiSocControllerGuid = {0xee369cc3, 0xa743, 0x5382, {0x75, 0x64, 0x53, 0xe4, 0x31, 0x19, 0x38, 0x35}}
>>>>>>  
>>>>>> diff --git a/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c b/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c
>>>>>> index 7dd5ba615c..f7536bfea3 100644
>>>>>> --- a/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c
>>>>>> +++ b/Silicon/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c
>>>>>> @@ -20,6 +20,7 @@
>>>>>>  #include <Library/BmcConfigBootLib.h>
>>>>>>  #include <Library/DevicePathLib.h>
>>>>>>  #include <Library/PcdLib.h>
>>>>>> +#include <Library/TimerLib.h>
>>>>>>  #include <Library/UefiBootManagerLib.h>
>>>>>>  #include <Library/UefiLib.h>
>>>>>>  #include <Protocol/DevicePath.h>
>>>>>> @@ -554,6 +555,47 @@ PlatformBootManagerBeforeConsole (
>>>>>>    PlatformRegisterOptionsAndKeys ();
>>>>>>  }
>>>>>>  
>>>>>> +STATIC
>>>>>> +VOID
>>>>>> +WaitForDiskReady (
>>>>>> +  )
>>>>>> +{
>>>>>> +  EFI_STATUS                Status;
>>>>>> +  UINT32                    Index;
>>>>>> +  UINTN                     DataSize;
>>>>>> +  UINT32                    DiskInfo;
>>>>>> +  UINT8                     IsFinished;
>>>>>> +
>>>>>> +  Status = EFI_NOT_FOUND;
>>>>>> +  DataSize = sizeof (UINT32);
>>>>>> +  // Wait for 30 seconds at most.
>>>>>> +  for (Index=0; Index<30; Index++) {
>>>>>
>>>>> Spaces around '=' and '<'.
>>>>>
>>>>>> +    Status = gRT->GetVariable (
>>>>>> +                    L"SASDiskInfo",
>>>>>> +                    &gHisiOemVariableGuid,
>>>>>> +                    NULL,
>>>>>> +                    &DataSize,
>>>>>> +                    &DiskInfo
>>>>>> +                    );
>>>>>
>>>>> Wait...
>>>>> Are we synchronizing against the storage controller driver using an
>>>>> environment variable and looping over it for 30 seconds?
>>>>>
>>>> D06:
>>>> For using hard disk backboard, some disk need 15 seconds to ready.
>>>> Actually, wait less 15 seconds here(minus the time from end of SAS
>>>> driver to here).
>>>> For using expander, wait less 6 seconds here(minus the time from end of SAS
>>>> driver to here).
>>>>
>>>> D03/D05:
>>>> no waiting here.
>>>>
>>>> Maybe I should change 30 to 15, it will never loop over for 30 seconds.
>>>>
>>>>> That can't go in.
>>>>> Please look into implementing an event in the SAS driver which you can
>>>>> wait for here.
>>>>
>>>> I don't really understand what differece between implementing with variable
>>>> and implementing with event.
>>>
>>> One is using the mechanism explicitly designed for this sort of
>>> thing. (And hence making the code a lot more clear and avoiding
>>> unintended side effects.)
>>> It also mean you would wait exactly as long as you needed, and not
>>> need to worry about how long.
>>
>> In this case, it don't want to wait exactly time, the time depand on
>> SAS driver and which backboard is used. After optimizing SAS driver,
>> wait time is reduced. Here,30 just for avoiding dead loop.
> 
> Which is exactly what an event is for.
> At the point where the SAS controller driver is currently updating the
> variable, it can instead call SignalEvent ().
> 
> The WaitForDiskReady () function can be changed to wait/check for that event.
> 

As current solution(using variables) have run stable, I prefer don't change
SAS driver and this wait function. Is it OK?

>>>>> Why not call WaitForDiskReady() before EfiBootManagerConnectAll () in
>>>>> PlatformBootManagerAfterConsole ()?
>>>>>
>>>>
>>>> The Sas controller of D06 is a pci device, SAS driver (Start functio) run after
>>>> pci enumeration. WaitForDiskReady should be called after SAS driver and before
>>>> creating boot options.
>>>
>>> OK, but EfiBootManagerConnectAll () is a bit of a sledge hammer, and
>>> will affect boot times.
>>>
>>> The SAS controller is onboard (so always present), right?
>>> If so, you could connect it manually.
>>
>> Yes, it's always present.
>> I intend to optimize SAS driver in future. It's not enouth time to do
>> this before 18.08.
>> Thanks.
> 
> OK.
> 
> /
>     Leif
>