diff mbox

[edk2,1/4] MdeModulePkg: Change the default IPv4 config policy

Message ID CAD0U-hJQZMgOPime2q3RkS3LSZxShGeGEwGJKdN4NcQMr807CQ@mail.gmail.com
State New
Headers show

Commit Message

Ryan Harkin June 22, 2016, 9:32 a.m. UTC 
On 22 June 2016 at 09:45, Wu, Jiaxin <jiaxin.wu@intel.com> wrote:
> Hi Ryan,

> I have root cause the issue:

>


Wow!  You found it!  I'm very pleased about that :-)


> Let me analyze this problem for you according below steps:

> 1 .First PXE boot, then boot to shell;

> 2. ifconfig -s eth0 dhcp (Success);

> 3. Reboot and do PXE, then boot to shell;----> The issue is enrolled by here!!!!

>

> On this reboot, policy is DHCP (Changed by step2). So, Ip4Dxe driver will try to get one IP address from DHCP server automatically. Before it get the IP address successfully, the IpSb->State will be always in IP4_SERVICE_STARTED status until the Instance->Dhcp4Event is triggered, then it can be changed to IP4_SERVICE_CONFIGED. But the DHCP process will be interrupted by PXE boot, which will change the policy to static, and the Instance->Dhcp4Event will be also closed directly. However, current implementation doesn't update the IpSb->State to IP4_SERVICE_UNSTARTED status in such case. So, failure happened!

>


That makes perfect sense.


> Why not happened in simulator platform? Because Ip4Dxe driver finished the DHCP process (DORA) and the IpSb->State has been changed to IP4_SERVICE_CONFIGED before PXE set the policy to DHCP.

>


Ah, a timing problem!


> 4. ifconfig -s eth0 dhcp (Platform failed to get IP address no matter how many times retried.)

>

>

> The corresponding patch will be send out later to fix this issue, so, please help to verify it.

>


Thanks a lot for looking at this.  I'm happy to test a fix.

Perhaps you could also consider adding this diff into your fix?  It's
slightly more efficient in the case with no error, but I think it's a
little bit easier to follow the flow and intention of the code with my
diff.

But it's ok if you don't like it, it's your code.



Regards,
Ryan.





>

> Thanks and Best Regard!

> Jiaxin

>

>

>> -----Original Message-----

>> From: Wu, Jiaxin

>> Sent: Wednesday, June 22, 2016 11:35 AM

>> To: 'Ryan Harkin' <ryan.harkin@linaro.org>

>> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; 'edk2-

>> devel-01' <edk2-devel@lists.01.org>

>> Subject: RE: [edk2] [Patch 1/4] MdeModulePkg: Change the default IPv4

>> config policy

>>

>> Hi Ryan,

>> I can reproduce the issue on my real platform now, thank you for your

>> reporting. I will find the root cause and fix it. If have any process, I will inform

>> you.

>>

>> Thanks again.

>> Jiaxin

>>

>> > -----Original Message-----

>> > From: Wu, Jiaxin

>> > Sent: Wednesday, June 22, 2016 10:41 AM

>> > To: Ryan Harkin <ryan.harkin@linaro.org>

>> > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;

>> > edk2-

>> > devel-01 <edk2-devel@lists.01.org>

>> > Subject: RE: [edk2] [Patch 1/4] MdeModulePkg: Change the default IPv4

>> > config policy

>> >

>> > Ryan,

>> >

>> > > There could be a bug here, I suppose.  Why is

>> > > NetLibCreateServiceChild returning EFI_UNSUPPORTED?  I guess I need

>> > > to investigate this some

>> > more.

>> > >

>> > > Continuing, at this point Status is still EFI_UNSUPPORTED, so this

>> > > is caught line 958 and we return from the function.  That looks like

>> > > a bug.  If not, it could do with a comment explaining why we've just

>> > > handled the EFI_UNSUPPORTED error without returning, yet we return

>> > here anyway.

>> >

>> > Ip4Dxe driver will try to get one IP address from DHCP server

>> > automatically if policy is DHCP, so DHCPv4 Service Binding protocol is

>> > required for this operation. If DHCPv4 Service Binding protocol is not

>> > ready, the Ip4Dxe should register a notify to wait this Service

>> > Binding protocol ready. We can't guarantee DHCPv4 Service has been

>> > started before Ip4 Service. So, EFI_UNSUPPORTED status check for

>> > NetLibCreateServiceChild is reasonable and correct here.

>> > According your below test patch, you allow Ip4StartAutoConfig()

>> > function continue the steps even  DHCPv4 Service is not ready, this is

>> > not allowed because no DHCP instance has been created, then ASSERT

>> > happened. You don't need to do that. You can see the

>> > Ip4Config2OnDhcp4SbInstalled() notify function, once the DHCPv4

>> > service binding protocol is installed in the system, Ip4StartAutoConfig() will

>> be recalled again.

>> >

>> > > > I don't have wireshark set up, but I'll try to get it working and

>> > > > send you a trace.

>> > > >

>> > > > However, I suspect that DHCP service (Is that the DORA you

>> > > > mentioned?) is not being restarted for some reason, so in that

>> > > > case, no packets would be sent out.

>> >

>> > For this confusion, the packet captured from wireshark can help us

>> > know whether there is any packet out or not.

>> >

>> > Thanks.

>> > Jiaxin

>> >

>> >

>> > > -----Original Message-----

>> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf

>> > > Of Ryan Harkin

>> > > Sent: Wednesday, June 22, 2016 2:12 AM

>> > > To: Wu, Jiaxin <jiaxin.wu@intel.com>

>> > > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;

>> > > edk2-

>> > > devel-01 <edk2-devel@lists.01.org>

>> > > Subject: Re: [edk2] [Patch 1/4] MdeModulePkg: Change the default

>> > > IPv4 config policy

>> > >

>> > > Hi Jiaxin,

>> > >

>> > > On 21 June 2016 at 12:20, Ryan Harkin <ryan.harkin@linaro.org> wrote:

>> > > > Hi Jiaxin,

>> > > >

>> > > > On 21 June 2016 at 12:10, Wu, Jiaxin <jiaxin.wu@intel.com> wrote:

>> > > >> Hi Ryan,

>> > > >>

>> > > >> I can't reproduce your issue on NT32, I will try it in a real platform later.

>> > > >>

>> > > >

>> > > > I am unable to reproduce the problem on my "FVP models" (a type of

>> > > > emulator) either.  The models use a different ethernet device than

>> > > > my hardware.

>> > > >

>> > > >

>> > > >>> On first boot, policy is static, it gets changed to DHCP, "ifconfig -s

>> > > >>> eth0 dhcp" works.   On reboot, policy is dhcp, PXE changes it a couple

>> > > >>> of times, then drops to the shell with is set to static again.

>> > > >>> "ifconfig -s eth0 dhcp" fails no matter how many times I reboot.

>> > > >>

>> > > >> First PXE boot --> ifconfig -s eth0 dhcp (Success) --> reboot,

>> > > >> then PXE --> ifconfig -s eth0 dhcp (Your platform failed here,

>> > > >> but my parts is always in a success no matter how many times I

>> > > >> tried.)

>> > > >>

>> > > >>> It's only when I "ifconfig -s eth0 static ...." then "ifconfig

>> > > >>> -s

>> > > >>> eth0 dhcp" that DHCP obtains an IP address again.

>> > > >>>

>> > > >>> It could easily be a bug in the LAN9118 driver too, but there's

>> > > >>> a bug somewhere, because it isn't working as we expect  :-O

>> > > >>

>> > > >> So, to help us analyze problem, could you sent me one wireshark

>> > > >> packet

>> > > for the above steps?

>> > > >>

>> > > >

>> > > > I don't have wireshark set up, but I'll try to get it working and

>> > > > send you a trace.

>> > > >

>> > > > However, I suspect that DHCP service (Is that the DORA you

>> > > > mentioned?) is not being restarted for some reason, so in that

>> > > > case, no packets would be sent out.

>> > > >

>> > > > I don't know how the DHCP service is handled or started, but I'll

>> > > > try and find the points in the code where it's started and stopped

>> > > > and add some trace to see if there are any errors.  Any

>> > > > suggestions of where to place some DEBUG would be welcome.

>> > > >

>> > >

>> > > I decided to add DEBUG statements to function Ip4StartAutoConfig in

>> > > Ip4Config2Impl.c.  I appended my DEBUG to the end of each line, so I

>> > > didn't change the line numbering when compared to the upstream

>> > > version, currently last modified here:

>> > > 364f4efa444150df3f074f563374dce1e153adc6

>> > >

>> > > The debug code I added to each branch in the function looks like this:

>> > >

>> > > DEBUG ((EFI_D_ERROR, "%a:%d Instance->Policy=%d\n",

>> __FUNCTION__,

>> > > __LINE__, Instance->Policy));

>> > >

>> > > Therefore, I can trace which branches the code is taking.

>> > >

>> > > On first boot, as expected, I can see that Ip4StartAutoConfig is not

>> > > called at all.  The default policy is static, so I'd expect that.

>> > >

>> > > Then, when I configure DHCP from Shell, I see the following debug:

>> > >

>> > > Ip4StartAutoConfig:921 Instance->Policy=0

>> > > Ip4StartAutoConfig:1032 Instance->Policy=0

>> > >

>> > > This tells me that NetLibCreateServiceChild, gBS->OpenProtocol and

>> > > Dhcp4->Configure succeeded and the DHCP process was started.

>> > >

>> > > That all looks good.  So I reset the board and see this debug on boot:

>> > >

>> > > Warning: LAN9118 Driver in stopped state

>> > > Ip4StartAutoConfig:921 Instance->Policy=1

>> > > Ip4StartAutoConfig:943 Instance->Policy=1

>> > > Ip4StartAutoConfig:947 Instance->Policy=1

>> > > Ip4StartAutoConfig:958 Instance->Policy=1

>> > > Ip4StartAutoConfig:921 Instance->Policy=1

>> > > Ip4StartAutoConfig:943 Instance->Policy=1

>> > > Ip4StartAutoConfig:958 Instance->Policy=1

>> > > Ip4StartAutoConfig:921 Instance->Policy=1

>> > > Ip4StartAutoConfig:962 Instance->Policy=1

>> > > Ip4StartAutoConfig:1032 Instance->Policy=1

>> > > EhcExecTransfer: transfer failed with 2

>> > > EhcControlTransfer: error - Device Error, transfer - 2 Press ENTER

>> > > to boot now or any other key to show the Boot Menu in 8 seconds

>> > >

>> > > Booting EFI USB Device

>> > > Booting EFI Misc Device

>> > > Booting EFI Misc Device 1

>> > > Booting EFI Network

>> > > ..PXE-E99: Unexpected network error.

>> > > Booting EFI Internal Shell

>> > > add-symbol-file

>> > >

>> >

>> /working/platforms/uefi/edk2/Build/ArmJuno/DEBUG_GCC49/AARCH64/Sh

>> > > ellPkg/Application/Shell/Shell/DEBUG/Shell.dll

>> > > 0xF86C4000

>> > > Loading driver at 0x000F86C3000 EntryPoint=0x000F86C4000 Shell.efi

>> > >

>> > > Shell> ifconfig -s eth0 dhcp

>> > > Ip4StartAutoConfig:921 Instance->Policy=0

>> > > Ip4StartAutoConfig:924 Instance->Policy=0

>> > >

>> > >

>> > > So I can see that on boot, because the saved policy is DHCP, the

>> > > code taking a few error branches.

>> > >

>> > > First off "NetLibCreateServiceChild" returns EFI_UNSUPPORTED.  The

>> > > comments say this means "No DHCPv4 Service Binding protocol,

>> > > register a notify.".  Dhcp4SbNotifyEvent is NULL, so we create a notify

>> event.

>> > >

>> > > There could be a bug here, I suppose.  Why is

>> > > NetLibCreateServiceChild returning EFI_UNSUPPORTED?  I guess I need

>> > > to investigate this some

>> > more.

>> > >

>> > > Continuing, at this point Status is still EFI_UNSUPPORTED, so this

>> > > is caught line 958 and we return from the function.  That looks like

>> > > a bug.  If not, it could do with a comment explaining why we've just

>> > > handled the EFI_UNSUPPORTED error without returning, yet we return

>> > here anyway.

>> > >

>> > > I applied this diff to fix the suspected bug:

>> > >

>> > > diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c

>> > > b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c

>> > > index a1de443..fb8a46a 100644

>> > > --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c

>> > > +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c

>> > > @@ -953,9 +953,7 @@ Ip4StartAutoConfig (

>> > >                                         &Instance->Registration

>> > >                                         );

>> > >      }

>> > > -  }

>> > > -

>> > > -  if (EFI_ERROR (Status)) {

>> > > +  } else if (EFI_ERROR (Status)) {

>> > >      return Status;

>> > >    }

>> > >

>> > >

>> > > Now, after first boot, I see this output when I run "ifconfig -s eth0 dhcp".

>> > > Basically the same as before:

>> > >

>> > > Shell> ifconfig -s eth0 dhcp

>> > > Shell> tAutoConfig:921 Instance->Policy=0

>> > > Ip4StartAutoConfig:1030 Instance->Policy=0

>> > >

>> > > Then, I reset the board and then it ASSERTs:

>> > >

>> > > Warning: LAN9118 Driver in stopped state

>> > > Ip4StartAutoConfig:921 Instance->Policy=1

>> > > Ip4StartAutoConfig:943 Instance->Policy=1

>> > > Ip4StartAutoConfig:947 Instance->Policy=1

>> > > Ip4StartAutoConfig:960 Instance->Policy=1

>> > >

>> > > ASSERT_EFI_ERROR (Status = Invalid Parameter) ASSERT [Ip4Dxe]

>> > >

>> >

>> /working/platforms/uefi/edk2/MdeModulePkg/Universal/Network/Ip4Dxe/

>> > > Ip4Config2Impl.c(972):

>> > > !EFI_ERROR (Status)

>> > >

>> > > This is because gBS->OpenProtocol returns Invalid Parameter.

>> > >

>> > > So it looks like my diff wasn't a bug fix at all.  Or, there is another bug?

>> > >

>> > > What do you think?

>> > >

>> > > Regards,

>> > > Ryan.

>> > > _______________________________________________

>> > > edk2-devel mailing list

>> > > edk2-devel@lists.01.org

>> > > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
index d0fa132..7757718 100644
--- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
+++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
@@ -940,22 +940,21 @@  Ip4StartAutoConfig (
              &Instance->Dhcp4Handle
              );

-  if (Status == EFI_UNSUPPORTED) {
-    //
-    // No DHCPv4 Service Binding protocol, register a notify.
-    //
-    if (Instance->Dhcp4SbNotifyEvent == NULL) {
-      Instance->Dhcp4SbNotifyEvent = EfiCreateProtocolNotifyEvent (
-                                       &gEfiDhcp4ServiceBindingProtocolGuid,
-                                       TPL_CALLBACK,
-                                       Ip4Config2OnDhcp4SbInstalled,
-                                       (VOID *) Instance,
-                                       &Instance->Registration
-                                       );
-    }
-  }
-
   if (EFI_ERROR (Status)) {
+    if (Status == EFI_UNSUPPORTED) {^M
+      //^M
+      // No DHCPv4 Service Binding protocol, register a notify.^M
+      //^M
+      if (Instance->Dhcp4SbNotifyEvent == NULL) {^M
+        Instance->Dhcp4SbNotifyEvent = EfiCreateProtocolNotifyEvent (^M
+
&gEfiDhcp4ServiceBindingProtocolGuid,^M
+                                         TPL_CALLBACK,^M
+                                         Ip4Config2OnDhcp4SbInstalled,^M
+                                         (VOID *) Instance,^M
+                                         &Instance->Registration^M
+                                         );^M
+      }^M
+    }^M
     return Status;
   }