[edk2] [Patch] MdeModulePkg: Remove TransmitReceive() and ActiveChild dependency

[Jiaxin Wu]

Fix git 59a8cfd4 (SVN r17869) removes DHCP4.TransmitReceive()and DORA
process dependency, but it updated TransmitReceive() to take the ownership
of DhcpSb->ActiveChild but never release it. This will break the retransmit
and lease time out counter of DORA.
To fix that, TransmitReceive() doesn't need to be the ActiveChild, and the
timer routine should be updated to handle the TransmitReceive specially.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <***@intel.com>
---
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c | 1 +
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c | 6 +++++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h | 4 +++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 18 ++++++++++++++----
4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
index 9253df2..c9c3a4a 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
@@ -505,10 +505,11 @@ DhcpInitProtocol (
Instance->Signature = DHCP_PROTOCOL_SIGNATURE;
CopyMem (&Instance->Dhcp4Protocol, &mDhcp4ProtocolTemplate, sizeof (Instance->Dhcp4Protocol));
InitializeListHead (&Instance->Link);
Instance->Handle = NULL;
Instance->Service = DhcpSb;
+ Instance->IsTransmitReceive = FALSE;
Instance->InDestroy = FALSE;
Instance->CompletionEvent = NULL;
Instance->RenewRebindEvent = NULL;
Instance->Token = NULL;
Instance->UdpIo = NULL;
diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
index 821dfbb..7c7ed1f 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
@@ -1492,11 +1492,10 @@ EfiDhcp4TransmitReceive (
return EFI_INVALID_PARAMETER;
}

Instance = DHCP_INSTANCE_FROM_THIS (This);
DhcpSb = Instance->Service;
- DhcpSb->ActiveChild = Instance;

if (Instance->Token != NULL) {
//
// The previous call to TransmitReceive is not finished.
//
@@ -1525,10 +1524,15 @@ EfiDhcp4TransmitReceive (
}

OldTpl = gBS->RaiseTPL (TPL_CALLBACK);

//
+ // Set the InTransmitReceive flag.
+ //
+ Instance->IsTransmitReceive = TRUE;
+
+ //
// Save the token and the timeout value.
//
Instance->Token = Token;
Instance->Timeout = Token->TimeoutValue;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
index 44213cf..615e006 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
@@ -4,11 +4,11 @@
RFC 2131: Dynamic Host Configuration Protocol
RFC 2132: DHCP Options and BOOTP Vendor Extensions
RFC 1534: Interoperation Between DHCP and BOOTP
RFC 3396: Encoding Long Options in DHCP.

-Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
http://opensource.org/licenses/bsd-license.php

@@ -61,10 +61,12 @@ struct _DHCP_PROTOCOL {
EFI_DHCP4_PROTOCOL Dhcp4Protocol;
LIST_ENTRY Link;
EFI_HANDLE Handle;
DHCP_SERVICE *Service;

+ BOOLEAN IsTransmitReceive;
+
BOOLEAN InDestroy;

EFI_EVENT CompletionEvent;
EFI_EVENT RenewRebindEvent;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
index 995bbcf..583e5bd 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
@@ -1516,10 +1516,12 @@ EFIAPI
DhcpOnTimerTick (
IN EFI_EVENT Event,
IN VOID *Context
)
{
+ LIST_ENTRY *Entry;
+ LIST_ENTRY *Next;
DHCP_SERVICE *DhcpSb;
DHCP_PROTOCOL *Instance;
EFI_STATUS Status;

DhcpSb = (DHCP_SERVICE *) Context;
@@ -1663,14 +1665,22 @@ DhcpOnTimerTick (
}
}
}

ON_EXIT:
- if ((Instance != NULL) && (Instance->Token != NULL)) {
- Instance->Timeout--;
- if (Instance->Timeout == 0) {
- PxeDhcpDone (Instance);
+ //
+ // Iterate through all the DhcpSb Children.
+ //
+ NET_LIST_FOR_EACH_SAFE (Entry, Next, &DhcpSb->Children) {
+ Instance = NET_LIST_USER_STRUCT (Entry, DHCP_PROTOCOL, Link);
+
+ if ((Instance != NULL) && (Instance->Token != NULL) && \
+ (Instance->IsTransmitReceive || Instance == DhcpSb->ActiveChild)) {
+ Instance->Timeout--;
+ if (Instance->Timeout == 0) {
+ PxeDhcpDone (Instance);
+ }
}
}

return ;

[Fu, Siyuan]

Hi, Jiaxin

1. Some of the code path in DhcpOnTimerTick won't goto ON_EXIT, for example in
if (DhcpSb->ExtraRefresh != 0) {
return ;
}
Or
goto END_SESSION;
Should these case also need be considered to counter the TransmitReceive timer tick?

2. You didn't set the IsTransmitReceive flag to FALSE after timeout, and I also think this flag is not needed, just check Instance->Token != NULL is enough. Also "|| Instance == DhcpSb->ActiveChild" makes no sense, the TimeOut is only for a transmitreceive token, why should it also count a ActiveChild?

Siyuan

-----Original Message-----
From: Wu, Jiaxin
Sent: Wednesday, July 15, 2015 11:53 AM
To: edk2-***@lists.sourceforge.net; Fu, Siyuan
Subject: [Patch] MdeModulePkg: Remove TransmitReceive() and ActiveChild dependency

Fix git 59a8cfd4 (SVN r17869) removes DHCP4.TransmitReceive()and DORA process dependency, but it updated TransmitReceive() to take the ownership of DhcpSb->ActiveChild but never release it. This will break the retransmit and lease time out counter of DORA.
To fix that, TransmitReceive() doesn't need to be the ActiveChild, and the timer routine should be updated to handle the TransmitReceive specially.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <***@intel.com>
---
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c | 1 +
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c | 6 +++++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h | 4 +++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 18 ++++++++++++++----
4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
index 9253df2..c9c3a4a 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
@@ -505,10 +505,11 @@ DhcpInitProtocol (
Instance->Signature = DHCP_PROTOCOL_SIGNATURE;
CopyMem (&Instance->Dhcp4Protocol, &mDhcp4ProtocolTemplate, sizeof (Instance->Dhcp4Protocol));
InitializeListHead (&Instance->Link);
Instance->Handle = NULL;
Instance->Service = DhcpSb;
+ Instance->IsTransmitReceive = FALSE;
Instance->InDestroy = FALSE;
Instance->CompletionEvent = NULL;
Instance->RenewRebindEvent = NULL;
Instance->Token = NULL;
Instance->UdpIo = NULL;
diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
index 821dfbb..7c7ed1f 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
@@ -1492,11 +1492,10 @@ EfiDhcp4TransmitReceive (
return EFI_INVALID_PARAMETER;
}

Instance = DHCP_INSTANCE_FROM_THIS (This);
DhcpSb = Instance->Service;
- DhcpSb->ActiveChild = Instance;

if (Instance->Token != NULL) {
//
// The previous call to TransmitReceive is not finished.
//
@@ -1525,10 +1524,15 @@ EfiDhcp4TransmitReceive (
}

OldTpl = gBS->RaiseTPL (TPL_CALLBACK);

//
+ // Set the InTransmitReceive flag.
+ //
+ Instance->IsTransmitReceive = TRUE;
+
+ //
// Save the token and the timeout value.
//
Instance->Token = Token;
Instance->Timeout = Token->TimeoutValue;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
index 44213cf..615e006 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
@@ -4,11 +4,11 @@
RFC 2131: Dynamic Host Configuration Protocol
RFC 2132: DHCP Options and BOOTP Vendor Extensions
RFC 1534: Interoperation Between DHCP and BOOTP
RFC 3396: Encoding Long Options in DHCP.

-Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php

@@ -61,10 +61,12 @@ struct _DHCP_PROTOCOL {
EFI_DHCP4_PROTOCOL Dhcp4Protocol;
LIST_ENTRY Link;
EFI_HANDLE Handle;
DHCP_SERVICE *Service;

+ BOOLEAN IsTransmitReceive;
+
BOOLEAN InDestroy;

EFI_EVENT CompletionEvent;
EFI_EVENT RenewRebindEvent;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
index 995bbcf..583e5bd 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
@@ -1516,10 +1516,12 @@ EFIAPI
DhcpOnTimerTick (
IN EFI_EVENT Event,
IN VOID *Context
)
{
+ LIST_ENTRY *Entry;
+ LIST_ENTRY *Next;
DHCP_SERVICE *DhcpSb;
DHCP_PROTOCOL *Instance;
EFI_STATUS Status;

DhcpSb = (DHCP_SERVICE *) Context;
@@ -1663,14 +1665,22 @@ DhcpOnTimerTick (
}
}
}

ON_EXIT:
- if ((Instance != NULL) && (Instance->Token != NULL)) {
- Instance->Timeout--;
- if (Instance->Timeout == 0) {
- PxeDhcpDone (Instance);
+ //
+ // Iterate through all the DhcpSb Children.
+ //
+ NET_LIST_FOR_EACH_SAFE (Entry, Next, &DhcpSb->Children) {
+ Instance = NET_LIST_USER_STRUCT (Entry, DHCP_PROTOCOL, Link);
+
+ if ((Instance != NULL) && (Instance->Token != NULL) && \
+ (Instance->IsTransmitReceive || Instance == DhcpSb->ActiveChild)) {
+ Instance->Timeout--;
+ if (Instance->Timeout == 0) {
+ PxeDhcpDone (Instance);
+ }
}
}

return ;

--
1.9.5.msysgit.1

[Wu, Jiaxin]

Hi Siyuan,
You are right, IsTransmitReceive flag looks redundant.

Thanks.
Jiaxin



-----Original Message-----
From: Fu, Siyuan
Sent: Thursday, July 16, 2015 10:47 AM
To: Wu, Jiaxin; edk2-***@lists.sourceforge.net
Subject: RE: [Patch] MdeModulePkg: Remove TransmitReceive() and ActiveChild dependency

Hi, Jiaxin

1. Some of the code path in DhcpOnTimerTick won't goto ON_EXIT, for example in
if (DhcpSb->ExtraRefresh != 0) {
return ;
}
Or
goto END_SESSION;
Should these case also need be considered to counter the TransmitReceive timer tick?

2. You didn't set the IsTransmitReceive flag to FALSE after timeout, and I also think this flag is not needed, just check Instance->Token != NULL is enough. Also "|| Instance == DhcpSb->ActiveChild" makes no sense, the TimeOut is only for a transmitreceive token, why should it also count a ActiveChild?

Siyuan

-----Original Message-----
From: Wu, Jiaxin
Sent: Wednesday, July 15, 2015 11:53 AM
To: edk2-***@lists.sourceforge.net; Fu, Siyuan
Subject: [Patch] MdeModulePkg: Remove TransmitReceive() and ActiveChild dependency

Fix git 59a8cfd4 (SVN r17869) removes DHCP4.TransmitReceive()and DORA process dependency, but it updated TransmitReceive() to take the ownership of DhcpSb->ActiveChild but never release it. This will break the retransmit and lease time out counter of DORA.
To fix that, TransmitReceive() doesn't need to be the ActiveChild, and the timer routine should be updated to handle the TransmitReceive specially.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <***@intel.com>
---
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c | 1 +
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c | 6 +++++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h | 4 +++-
MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c | 18 ++++++++++++++----
4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
index 9253df2..c9c3a4a 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Driver.c
@@ -505,10 +505,11 @@ DhcpInitProtocol (
Instance->Signature = DHCP_PROTOCOL_SIGNATURE;
CopyMem (&Instance->Dhcp4Protocol, &mDhcp4ProtocolTemplate, sizeof (Instance->Dhcp4Protocol));
InitializeListHead (&Instance->Link);
Instance->Handle = NULL;
Instance->Service = DhcpSb;
+ Instance->IsTransmitReceive = FALSE;
Instance->InDestroy = FALSE;
Instance->CompletionEvent = NULL;
Instance->RenewRebindEvent = NULL;
Instance->Token = NULL;
Instance->UdpIo = NULL;
diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
index 821dfbb..7c7ed1f 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.c
@@ -1492,11 +1492,10 @@ EfiDhcp4TransmitReceive (
return EFI_INVALID_PARAMETER;
}

Instance = DHCP_INSTANCE_FROM_THIS (This);
DhcpSb = Instance->Service;
- DhcpSb->ActiveChild = Instance;

if (Instance->Token != NULL) {
//
// The previous call to TransmitReceive is not finished.
//
@@ -1525,10 +1524,15 @@ EfiDhcp4TransmitReceive (
}

OldTpl = gBS->RaiseTPL (TPL_CALLBACK);

//
+ // Set the InTransmitReceive flag.
+ //
+ Instance->IsTransmitReceive = TRUE;
+
+ //
// Save the token and the timeout value.
//
Instance->Token = Token;
Instance->Timeout = Token->TimeoutValue;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
index 44213cf..615e006 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Impl.h
@@ -4,11 +4,11 @@
RFC 2131: Dynamic Host Configuration Protocol
RFC 2132: DHCP Options and BOOTP Vendor Extensions
RFC 1534: Interoperation Between DHCP and BOOTP
RFC 3396: Encoding Long Options in DHCP.

-Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php

@@ -61,10 +61,12 @@ struct _DHCP_PROTOCOL {
EFI_DHCP4_PROTOCOL Dhcp4Protocol;
LIST_ENTRY Link;
EFI_HANDLE Handle;
DHCP_SERVICE *Service;

+ BOOLEAN IsTransmitReceive;
+
BOOLEAN InDestroy;

EFI_EVENT CompletionEvent;
EFI_EVENT RenewRebindEvent;

diff --git a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
index 995bbcf..583e5bd 100644
--- a/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
+++ b/MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Io.c
@@ -1516,10 +1516,12 @@ EFIAPI
DhcpOnTimerTick (
IN EFI_EVENT Event,
IN VOID *Context
)
{
+ LIST_ENTRY *Entry;
+ LIST_ENTRY *Next;
DHCP_SERVICE *DhcpSb;
DHCP_PROTOCOL *Instance;
EFI_STATUS Status;

DhcpSb = (DHCP_SERVICE *) Context;
@@ -1663,14 +1665,22 @@ DhcpOnTimerTick (
}
}
}

ON_EXIT:
- if ((Instance != NULL) && (Instance->Token != NULL)) {
- Instance->Timeout--;
- if (Instance->Timeout == 0) {
- PxeDhcpDone (Instance);
+ //
+ // Iterate through all the DhcpSb Children.
+ //
+ NET_LIST_FOR_EACH_SAFE (Entry, Next, &DhcpSb->Children) {
+ Instance = NET_LIST_USER_STRUCT (Entry, DHCP_PROTOCOL, Link);
+
+ if ((Instance != NULL) && (Instance->Token != NULL) && \
+ (Instance->IsTransmitReceive || Instance == DhcpSb->ActiveChild)) {
+ Instance->Timeout--;
+ if (Instance->Timeout == 0) {
+ PxeDhcpDone (Instance);
+ }
}
}

return ;

--
1.9.5.msysgit.1