979868 - B2G NFC: support UICC-based card-emulation as per ISO/IEC 14443 Type A&B

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Ming Yin]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Comment 1

[Ming Yin]

For NFC payment user story(ucid:NFC12), the NFCd SHALL configure the NFC controller to support Card-emulation as per ISO/IEC 14443 Type A and Type B PICC.  

Additionally,following requirements need to be taken into consideration. 
1) Card Emulation mode SHALL be enabled as soon as the NFC hardware is turned on;
2) The default routing for the card emulation mode SHALL be via the UICC; 
3) UICC SHALL be configured as default secure element;
3) Automatic and continuous switch between card emulation and reader mode, based on optimized power consumption.

Need to check whether we need to support Card Emulation in standby and battery off/low modes!

Comment 2

[Ken Chang[:kenkai]]

We should support card emulation in standby and battery low mode, but not in battery off mode. If we do it in battery off mode, user may be confused in power consumption. The other thing is that we may need vendor to support this. since phone needs a hardware wake-up mechanism while being in standby mode.

Comment 3

[Ming Yin]

From business perspective,  "support for battery-off in card emulation mode" is actually a very appreciated and unique feature, especially for the mobile phone based contactless/NFC solutions, such as payment and ticketing. Imagining following scenario, a user bought an public transport ticket from online portal and the ticket is downloaded and stored in the secure element(SIM) of his mobile phone. In normal case, going through the gate means tap-and-go with phone. so far so good!
Sometimes, we are all likely to encounter another situation.  "Oops, battery is flat, but what is with my ticket?" In such an embarrassed situation, battery-off mode support will be very appreciated. 

From technical perspective, Ken, you are right. Not every nfc chip supports battery-off mode. I had checked some relative new NFC chip products from NXP and Broadcom.
for example, PN544,  PN547, BCM20793. They support battery-off in Card Emulation mode.

I would suggest to keep this requirement, maybe with 2nd. priority?  Standby and battery-low support are prio-1.

Comment 4

[Ming Yin]

From DT perspective, the NFC card emulation shall be also possible under following "abnormal" cases.
1) screen-off
2) screen-locked
3) battery-low(phone switched off)
4) battery-off(phone switched off, NFC/SIM powered by the field of external reader.)
5) flight-mode

Comment 5

[Ken Chang[:kenkai]]

Currently, I wonder if we need a special option being different from the option of NFC enable for NFC payment. For example, "touch to pay" or something else. After enabling "touch to pay", device knows that the NFC chip can not be turn off. Or do you see any related option or mechanism implemented on other devices?

Comment 6

[Ming Yin]

In general, we can enable the card emulation in several ways.

Option-1: The NFC card emulation is by default turned on, and switch between card emulation and reader mode will be done automatically;

Option-2: The NFC card emulation is by default turned off, as you suggested, it will be manually switched on/off by user via system setting, e.g., such as your suggested "touch to pay" menu;

Option-3: There is an API available for the system application to enable/disable card emulation mode;


Currently, all MNOs' customized NFC Android Handsets adopt the option-1; 
Android Kitkat has used option-2 and has only ability to query card emulation mode status via API; 
In our proposed secure element management API, we proposed also a possibility for option-3 below.

/**
   * Sets the Card Emulation Support mode. This interface is
   * used by privileged content to notify nfc hardware to
   * support card emulation mode, say even in low power mode.
   *
   * @param type
   *        One of CE_MODE_DEVICE_* (or) masked values of CE_MODE_DEVICE_*
   */
  DOMRequest setCardEmulationMode(long mode);

The related bug can be found here. https://bugzilla.mozilla.org/attachment.cgi?id=8374418

Comment 7

[Sandip Kamat]

(In reply to Ming Yin from comment #6)
> In general, we can enable the card emulation in several ways.
> 
> Option-1: The NFC card emulation is by default turned on, and switch between
> card emulation and reader mode will be done automatically;
> 
> Option-2: The NFC card emulation is by default turned off, as you suggested,
> it will be manually switched on/off by user via system setting, e.g., such
> as your suggested "touch to pay" menu;
> 
> Option-3: There is an API available for the system application to
> enable/disable card emulation mode;
> 
> 
> Currently, all MNOs' customized NFC Android Handsets adopt the option-1; 
> Android Kitkat has used option-2 and has only ability to query card
> emulation mode status via API; 
> In our proposed secure element management API, we proposed also a
> possibility for option-3 below.
> 
> /**
>    * Sets the Card Emulation Support mode. This interface is
>    * used by privileged content to notify nfc hardware to
>    * support card emulation mode, say even in low power mode.
>    *
>    * @param type
>    *        One of CE_MODE_DEVICE_* (or) masked values of CE_MODE_DEVICE_*
>    */
>   DOMRequest setCardEmulationMode(long mode);
> 
> The related bug can be found here.
> https://bugzilla.mozilla.org/attachment.cgi?id=8374418

I would suggest to implement option 2 and 3 for FxOS.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

remove whiteboard.
ucid:NFC12 should be bug 979152. The dependent works don't need to have the ucid tagged.
FT:RIL is not necessary given that the component is NFC which is monitored under RIL team already.

Comment 9

[Ken Chang[:kenkai]]

I think bug 979152 blocks this bug.

Comment 10

[Ken Chang[:kenkai]]

After discussing with dimi, bug 959434, 979152, and 984207 doesn't need to block this bug. Dimi wants to take this bug.

Comment 11

[Dimi Lee [:dimi]]

Attachment: Part1. Add secure element interfaces v1

Comment 12

[Dimi Lee [:dimi]]

Attachment: Part2. Secure element function implementation v1

Comment 13

[Dimi Lee [:dimi]]

Attachment: Part3. Integrate secure element function into Nfc v1

Comment 14

[Dimi Lee [:dimi]]

Attachment: Part4 Enable SE function by default v1

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8424666 [details] [diff] [review]
Part1. Add secure element interfaces v1

Review of attachment 8424666 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nci/NfcManager.cpp
@@ +413,5 @@
> +  ALOGD("%s: enter; ", __FUNCTION__);
> +  bool stat = false;
> +
> +  ALOGD("%s: exit", __FUNCTION__);
> +  return stat;

remove log.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8424667 [details] [diff] [review]
Part2. Secure element function implementation v1

Review of attachment 8424667 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nci/SecureElement.cpp
@@ +11,5 @@
> +#define LOG_TAG "NfcNci"
> +#include <cutils/log.h>
> +
> +SecureElement SecureElement::sSecElem;
> +const char* SecureElement::APP_NAME = "nfc";

is this used?

@@ +50,5 @@
> +{
> +  tNFA_STATUS nfaStat;
> +  unsigned long num = 0;
> +
> +  ALOGD("%s: enter", __FUNCTION__);

remove log.

@@ +58,5 @@
> +    mActiveSeOverride = num;
> +  }
> +  ALOGD("%s: Active SE override: 0x%X", __FUNCTION__, mActiveSeOverride);
> +
> +  mActiveEeHandle = NFA_HANDLE_INVALID;

Is this neccesary? 
We already do this in cstor and finalize()?

Same for below members.

@@ +112,5 @@
> +  ALOGD("%s: enter; mbNewEE=%d, mActualNumEe=%d", __FUNCTION__, mbNewEE, mActualNumEe);
> +  tNFA_STATUS nfaStat = NFA_STATUS_FAILED;
> +
> +  // If mbNewEE is true then there is new EE info.
> +  if (mbNewEE) {

bail out early.

@@ +116,5 @@
> +  if (mbNewEE) {
> +    mActualNumEe = MAX_NUM_EE;
> +
> +    if ((nfaStat = NFA_EeGetInfo(&mActualNumEe, mEeInfo)) != NFA_STATUS_OK) {
> +      ALOGE ("%s: fail get info; error=0x%X", __FUNCTION__, nfaStat);

ditto.

@@ +177,5 @@
> +{
> +  AutoMutex mutex(mMutex);
> +  if (mRfFieldIsOn) {
> +    return true;
> +  }

add an extra line.

@@ +183,5 @@
> +  int ret = clock_gettime(CLOCK_MONOTONIC, &now);
> +  if (ret == -1) {
> +    ALOGE("isRfFieldOn(): clock_gettime failed");
> +    return false;
> +  }

add an extra line

@@ +190,5 @@
> +    // was turned off, still return ON.
> +    return true;
> +  } else {
> +    return false;
> +  }

return (TimeDiff(mLastRfFieldToggle, now) < 50);

@@ +309,5 @@
> +  ALOGD("%s: enter; mActiveEeHandle=0x%X", __FUNCTION__, mActiveEeHandle);
> +
> +  if (!mIsInit) {
> +    ALOGE ("%s: not init", __FUNCTION__);
> +    goto TheEnd;

return early instead of using 'goto'

@@ +366,5 @@
> +  if (isActivated) {
> +    //e->CallVoidMethod (mNativeData->manager, android::gCachedNfcManagerNotifySeListenActivated);
> +  } else {
> +    //e->CallVoidMethod (mNativeData->manager, android::gCachedNfcManagerNotifySeListenDeactivated);
> +  }

remove dummy code.

@@ +368,5 @@
> +  } else {
> +    //e->CallVoidMethod (mNativeData->manager, android::gCachedNfcManagerNotifySeListenDeactivated);
> +  }
> +
> +  ALOGD("%s: exit", __FUNCTION__);

remove log

@@ +391,5 @@
> +    //e->CallVoidMethod (mNativeData->manager, android::gCachedNfcManagerNotifySeFieldDeactivated);
> +  }
> +  mMutex.unlock();
> +
> +  ALOGD("%s: exit", __FUNCTION__);

remove dummy code and log

@@ +560,5 @@
> +{
> +  ALOGD("%s: event=0x%X", __FUNCTION__, event);
> +  switch (event)
> +  {
> +  case NFA_EE_REGISTER_EVT:

nit, In other files, the style is

switch () {
  case ...:

@@ +628,5 @@
> +              __FUNCTION__, action.ee_handle, action.trigger);
> +      } else if (action.trigger == NFC_EE_TRIG_RF_TECHNOLOGY) {
> +        ALOGD("%s: NFA_EE_ACTION_EVT; h=0x%X; trigger=rf tech (0x%X)",
> +              __FUNCTION__, action.ee_handle, action.trigger);
> +      } else {

These 3 else (if) doesn't do anything?

@@ +712,5 @@
> +      return "Connected/Inactive";
> +    case NFC_NFCEE_STATUS_REMOVED:
> +      return "Removed";
> +  }
> +  return "?? Unknown ??";

move this to default: ..

::: src/nci/SecureElement.h
@@ +166,5 @@
> +
> +private:
> +  static const unsigned int MAX_RESPONSE_SIZE = 1024;
> +  enum RouteSelection {NoRoute, DefaultRoute, SecElemRoute};
> +  static const int MAX_NUM_EE = 5;    //max number of EE's

Use consistent style for //
usually
...;  // BlahBlahBlah.
same for below.

@@ +261,5 @@
> +  /**
> +   * Convert status code to status text.
> +   *
> +   * @param  status Status code
> +   * @return None

Move static funcs together.
The return value is None or const char* ?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8424668 [details] [diff] [review]
Part3. Integrate secure element function into Nfc v1

Review of attachment 8424668 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't have 8 lines of context.
It's not easy to review this patch.

::: src/nci/NfcManager.cpp
@@ +425,5 @@
> +
> +  stat = SecureElement::getInstance().activate();
> +  if (stat) {
> +    SecureElement::getInstance().routeToSecureElement();
> +  }

add an extra line

@@ +464,5 @@
> +  sIsSecElemSelected = false;
> +
> +  //if controller is not routing to sec elems AND there is no pipe connected,
> +  //then turn off the sec elems
> +  if (SecureElement::getInstance().isBusy() == false) {

if (!x)

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8424669 [details] [diff] [review]
Part4 Enable SE function by default v1

Review of attachment 8424669 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't have 8 lines of context either.

::: src/NfcService.cpp
@@ +604,5 @@
>        code = enableDiscovery();
> +      if (code != NFC_SUCCESS) {
> +        goto TheEnd;
> +      }
> +      code = enableSE();

We didn't check 'code'?

@@ +698,5 @@
>    return NFC_SUCCESS;
>  }
>  
> +// TODO: Emulator doesn't support SE now.
> +//       So always return sucess to pass testcase.

We already return true in doSelectSecureElement,
why do we still always return success here?

@@ +703,5 @@
> +NfcErrorCode NfcService::enableSE()
> +{
> +  ALOGD("Enable secure element");
> +
> +  sNfcManager->doSelectSecureElement();

We didn't check return value?

Comment 19

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> Comment on attachment 8424667 [details] [diff] [review]
> Part2. Secure element function implementation v1
> 
> Review of attachment 8424667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: src/nci/SecureElement.cpp
> @@ +11,5 @@
> > +#define LOG_TAG "NfcNci"
> > +#include <cutils/log.h>
> > +
> > +SecureElement SecureElement::sSecElem;
> > +const char* SecureElement::APP_NAME = "nfc";
> 
> is this used?
> 
It is used for routing, i'll remove it for this patch

> 
> @@ +58,5 @@
> > +    mActiveSeOverride = num;
> > +  }
> > +  ALOGD("%s: Active SE override: 0x%X", __FUNCTION__, mActiveSeOverride);
> > +
> > +  mActiveEeHandle = NFA_HANDLE_INVALID;
> 
> Is this neccesary? 
> We already do this in cstor and finalize()?
> 
> Same for below members.
> 
SecureElement is a single instance and in the design logic it can be init -> finalize -> init -> ...
some variables are reset in initialize but not reset in finalize, and some variables are reset in both
initialize and finalize. To be honestly, I am not sure why android implements in this way(porting this from android 4.3).

Do you suggest we follow original design or move all reset into initialize function.
I have checked the code it is ok to move all reset to initialize or finalize.

> 
> switch () {
>   case ...:
> 
> @@ +628,5 @@
> > +              __FUNCTION__, action.ee_handle, action.trigger);
> > +      } else if (action.trigger == NFC_EE_TRIG_RF_TECHNOLOGY) {
> > +        ALOGD("%s: NFA_EE_ACTION_EVT; h=0x%X; trigger=rf tech (0x%X)",
> > +              __FUNCTION__, action.ee_handle, action.trigger);
> > +      } else {
> 
> These 3 else (if) doesn't do anything?
> 
Yes, it only prints the debug message. Do you suggest removing it ?

Comment 20

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #18)
> Comment on attachment 8424669 [details] [diff] [review]
> Part4 Enable SE function by default v1
> 
> Review of attachment 8424669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't have 8 lines of context either.
> 
> ::: src/NfcService.cpp
> @@ +604,5 @@
> >        code = enableDiscovery();
> > +      if (code != NFC_SUCCESS) {
> > +        goto TheEnd;
> > +      }
> > +      code = enableSE();
> 
> We didn't check 'code'?
> 
Next line of enableSE is TheEnd.


> @@ +698,5 @@
> >    return NFC_SUCCESS;
> >  }
> >  
> > +// TODO: Emulator doesn't support SE now.
> > +//       So always return sucess to pass testcase.
> 
> We already return true in doSelectSecureElement,
> why do we still always return success here?
> 
No, in doSelectSecureElement it returns the result of activate SE.

> @@ +703,5 @@
> > +NfcErrorCode NfcService::enableSE()
> > +{
> > +  ALOGD("Enable secure element");
> > +
> > +  sNfcManager->doSelectSecureElement();
> 
> We didn't check return value?

I'll add debug message for error case, and return the result when emulator support it.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #19)
> Do you suggest we follow original design or move all reset into initialize
> function.
> I have checked the code it is ok to move all reset to initialize or finalize.
> 
move all reset into initialize function seems simpler to me.

> Yes, it only prints the debug message. Do you suggest removing it ?
remove it.

Comment 22

[Dimi Lee [:dimi]]

Attachment: Part1. Add secure element interfaces v2

Comment 23

[Dimi Lee [:dimi]]

Attachment: Part2. Secure element function implementation v2

Comment 24

[Dimi Lee [:dimi]]

Attachment: Part3. Integrate secure element function into Nfc v2

Comment 25

[Dimi Lee [:dimi]]

Attachment: Part4 Enable SE function by default v2

Comment 26

[Sandip Kamat]

Quick Note: Currently all NFC payment user stories are covered under: Bug 979152, Bug 979154, Bug 979157, Bug 979158, 884478. All the dependencies for those should be marked in those meta bugs.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8426009 [details] [diff] [review]
Part2. Secure element function implementation v2

Review of attachment 8426009 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nci/SecureElement.cpp
@@ +46,5 @@
> +}
> +
> +bool SecureElement::initialize()
> +{
> +  tNFA_STATUS nfaStat;

Declare local variables as near to their use as possible.
i.e. Move into the block of NFA_EeRegister

@@ +56,5 @@
> +  }
> +  ALOGD("%s: Active SE override: 0x%X", __FUNCTION__, mActiveSeOverride);
> +
> +  mActiveEeHandle = NFA_HANDLE_INVALID;
> +

rm empty line

@@ +84,5 @@
> +    mEeRegisterEvent.wait();
> +  }
> +
> +  mIsInit = true;
> +  ALOGD("%s: exit", __FUNCTION__);

remove log

@@ +98,5 @@
> +
> +  mIsInit       = false;
> +  mActualNumEe  = 0;
> +
> +  ALOGD ("%s: exit", __FUNCTION__);

ditto.

@@ +115,5 @@
> +  mActualNumEe = MAX_NUM_EE;
> +
> +  if ((nfaStat = NFA_EeGetInfo(&mActualNumEe, mEeInfo)) != NFA_STATUS_OK) {
> +    ALOGE("%s: fail get info; error=0x%X", __FUNCTION__, nfaStat);
> +    mActualNumEe = 0;

bail out early.

@@ +120,5 @@
> +  } else {
> +    mbNewEE = false;
> +
> +    ALOGD("%s: num EEs discovered: %u", __FUNCTION__, mActualNumEe);
> +    if (mActualNumEe != 0) {

'if' is not necessary.

@@ +208,5 @@
> +
> +  // Get Fresh EE info.
> +  if (!getEeInfo()) {
> +    return;
> +  }

if (a||b||C)

@@ +225,5 @@
> +}
> +
> +bool SecureElement::activate()
> +{
> +  tNFA_STATUS nfaStat = NFA_STATUS_FAILED;

declare when using.

@@ +249,5 @@
> +
> +  uint16_t overrideEeHandle = 0;
> +  if (mActiveSeOverride) {
> +    overrideEeHandle = NFA_HANDLE_GROUP_EE | mActiveSeOverride;
> +  }

overrideEeHandle = mActiveSeOverride ?
                   NFA_HANDLE_GROUP_EE | mActiveSeOverride :
                   0;

@@ +261,5 @@
> +  //activate every discovered secure element
> +  for (int index = 0; index < mActualNumEe; index++) {
> +    tNFA_EE_INFO& eeItem = mEeInfo[index];
> +    if ((eeItem.ee_handle == EE_HANDLE_0xF3) || (eeItem.ee_handle == EE_HANDLE_0xF4) ||
> +        (eeItem.ee_handle == EE_HANDLE_0x01) || (eeItem.ee_handle == EE_HANDLE_0x02)) {

bail out early.
if (a && b && c && d) {
  continue;
}

or add a function to do the handle check.

@@ +262,5 @@
> +  for (int index = 0; index < mActualNumEe; index++) {
> +    tNFA_EE_INFO& eeItem = mEeInfo[index];
> +    if ((eeItem.ee_handle == EE_HANDLE_0xF3) || (eeItem.ee_handle == EE_HANDLE_0xF4) ||
> +        (eeItem.ee_handle == EE_HANDLE_0x01) || (eeItem.ee_handle == EE_HANDLE_0x02)) {
> +      if (overrideEeHandle && (overrideEeHandle != eeItem.ee_handle)) {

'if (overrideEeHandle)' is not neccesary.

@@ +281,5 @@
> +          if (eeItem.ee_status == NFC_NFCEE_STATUS_ACTIVE) {
> +            numActivatedEe++;
> +          } else {
> +            ALOGE("%s: NFA_EeModeSet failed; error=0x%X", __FUNCTION__, nfaStat);
> +          }

the position of else if wrong?

@@ +290,5 @@
> +
> +  mActiveEeHandle = getDefaultEeHandle();
> +  if (mActiveEeHandle == NFA_HANDLE_INVALID) {
> +    ALOGE("%s: ee handle not found", __FUNCTION__);
> +  }

the ALOGE should be done in caller.

@@ +308,5 @@
> +    return retval;
> +  }
> +
> +  //if the controller is routing to sec elems or piping,
> +  //then the secure element cannot be deactivated

nit, add sapce after //

@@ +309,5 @@
> +  }
> +
> +  //if the controller is routing to sec elems or piping,
> +  //then the secure element cannot be deactivated
> +  if ((mCurrentRouteSelection == SecElemRoute) || mIsPiping) {

if (isBusy())

@@ +317,5 @@
> +
> +  if (mActiveEeHandle == NFA_HANDLE_INVALID) {
> +    ALOGE("%s: invalid EE handle", __FUNCTION__);
> +    return retval;
> +  }

group these 'if' checks

@@ +334,5 @@
> +  if (aidBufferLen == 0) {
> +    return;
> +  }
> +
> +  const uint16_t tlvMaxLen = aidBufferLen + 10;

where does 10 come from ? const it.

@@ +347,5 @@
> +
> +  // TODO: Implement notify.
> +TheEnd:
> +  delete []tlv;
> +  ALOGD("%s: exit", __FUNCTION__);

remove log.

@@ +373,5 @@
> +  if (isActive) {
> +    mRfFieldIsOn = true;
> +  } else {
> +    mRfFieldIsOn = false;
> +  }

mRfFieldIsOn = isActive.

@@ +390,5 @@
> +    // There is no good choice here...
> +  }
> +  mMutex.unlock();
> +
> +  ALOGD("%s: exit", __FUNCTION__);

remove log.

@@ +399,5 @@
> +  ALOGD("%s:  Status: %u   Num EE: %u", __FUNCTION__, info.status, info.num_ee);
> +
> +  SyncEventGuard guard(mUiccInfoEvent);
> +  memcpy(&mUiccInfo, &info, sizeof(mUiccInfo));
> +  for (uint8_t xx = 0; xx < info.num_ee; xx++) {

use 'i' as index.

@@ +420,5 @@
> +  mCurrentRouteSelection = selection;
> +  adjustProtocolRoutes(selection);
> +  adjustTechnologyRoutes(selection);
> +
> +TheEnd:

remove label.

@@ +422,5 @@
> +  adjustTechnologyRoutes(selection);
> +
> +TheEnd:
> +  NFA_EeUpdateNow(); //apply new routes now.
> +  ALOGD("%s: exit", __FUNCTION__);

remove log.

@@ +448,5 @@
> +
> +  /**
> +   * delete route to every sec elem
> +   */
> +  for (int i=0; i < mActualNumEe; i++) {

nit, int i = 0;

@@ +469,5 @@
> +  if (true) {
> +    tNFA_HANDLE eeHandle = NFA_EE_HANDLE_DH;
> +    if (routeSelection == SecElemRoute) {
> +      eeHandle = mActiveEeHandle;
> +    }

eeHandle = (routeSelection == SecElemRoute) ? mActiveEeHandle : NFA_EE_HANDLE_DH;

@@ +505,5 @@
> +
> +  /**
> +   * delete route to every sec elem.
> +   */
> +  for (int i=0; i < mActualNumEe; i++) {

ditto

@@ +525,5 @@
> +   */
> +  if (true) {
> +    tNFA_HANDLE eeHandle = NFA_EE_HANDLE_DH;
> +    if (routeSelection == SecElemRoute)
> +        eeHandle = mActiveEeHandle;

ditto

@@ +532,5 @@
> +    nfaStat = NFA_EeSetDefaultTechRouting(eeHandle, techMask, 0, 0);
> +    if (nfaStat == NFA_STATUS_OK)
> +      mRoutingEvent.wait ();
> +    else
> +      ALOGE("%s: fail route to EE; error=0x%X", __FUNCTION__, nfaStat);

nit, brace.

@@ +598,5 @@
> +          //if app-init operation is successful;
> +          //app_init.data[] contains two bytes, which are the status codes of the event;
> +          //app_init.data[] does not contain an APDU response;
> +          //see EMV Contactless Specification for Payment Systems; Book B; Entry Point Specification;
> +          //version 2.1; March 2011; section 3.3.3.5;

nit. space after //

@@ +611,5 @@
> +    case NFA_EE_DISCOVER_REQ_EVT:
> +      {
> +        ALOGD("%s: NFA_EE_DISCOVER_REQ_EVT; status=0x%X; num ee=%u",
> +              __FUNCTION__, eventData->discover_req.status, eventData->discover_req.num_ee);
> +        sSecElem.storeUiccInfo (eventData->discover_req);

nit, extra space after function name.

@@ +743,5 @@
> +      ALOGE("%s: fail to start UICC listen", __FUNCTION__);
> +    }
> +  }
> +
> +  ALOGD("%s: exit; ok=%u", __FUNCTION__, retval);

remove log.

@@ +774,5 @@
> +    } else {
> +      ALOGE("%s: fail to stop UICC listen", __FUNCTION__);
> +    }
> +  } else {
> +    retval = true;

should be retval = false?

::: src/nci/SecureElement.h
@@ +34,5 @@
> +  bool initialize();
> +
> +  /**
> +   * Release all resources.
> +   *

extra line

@@ +46,5 @@
> +   */
> +  void getListOfEeHandles(std::vector<uint32_t>& listSe);
> +
> +  /**
> +   * Turn on the secure element.

Activate.

@@ +53,5 @@
> +   */
> +  bool activate();
> +
> +  /**
> +   *  Turn off the secure element.

Deactivate. There's an extra space in front.

@@ +55,5 @@
> +
> +  /**
> +   *  Turn off the secure element.
> +   *
> +   * @return

True if ok.

@@ +65,5 @@
> +   * in listen mode
> +   *
> +   * @return None.
> +   */
> +  void notifyListenModeState(bool isActivated);;

double semicolon,

And put those callbacks together.

@@ +75,5 @@
> +   */
> +  void notifyRfFieldEvent(bool isActive);
> +
> +  /**
> +   * Resets the field status.

RF

@@ +96,5 @@
> +   * @param  aid Buffer contains application ID.
> +   * @param  aidLen Length of application ID.
> +   * @return None.
> +   */
> +  void notifyTransactionListenersOfAid (const uint8_t* aid, uint8_t aidLen);

extra space after function name

@@ +106,5 @@
> +   * @param  tlv type-length-value encoded in Basic Encoding Rule.
> +   * @param  tlvLen Length tlv.
> +   * @return None.
> +   */
> +  void notifyTransactionListenersOfTlv (const uint8_t* tlv, uint8_t tlvLen);

I cannot find the definition of this func.

@@ +157,5 @@
> +   */
> +  bool isActivatedInListenMode();
> +
> +  /**
> +   * Can be used to determine if the SE is in an RF field.

is activated.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8426010 [details] [diff] [review]
Part3. Integrate secure element function into Nfc v2

Review of attachment 8426010 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nci/NfcManager.cpp
@@ +435,5 @@
>  }
>  
>  bool NfcManager::doDeselectSecureElement()
>  {
>    bool stat = false;

Have a betting naming, status, or result.
Same for other functions.

@@ +436,5 @@
>  
>  bool NfcManager::doDeselectSecureElement()
>  {
>    bool stat = false;
> +  bool bRestartDiscovery = false;

make local variable naming shorter, and don't introduce Hungrean naming.

@@ +459,5 @@
> +  stat = SecureElement::getInstance().routeToDefault();
> +  sIsSecElemSelected = false;
> +
> +  //if controller is not routing to sec elems AND there is no pipe connected,
> +  //then turn off the sec elems

nit, space after //

@@ +466,5 @@
> +  }
> +
> +TheEnd:
> +  if (bRestartDiscovery) {
> +    startRfDiscovery (true);

nit, extra space after func name.

@@ +690,5 @@
>          if (status == NFA_STATUS_OK) {
>            ALOGD("%s: Disabled RF field events", __FUNCTION__);
>          } else {
>            ALOGE("%s: NFA_SetConfig fail, error = 0x%X", __FUNCTION__, status);
>          }

Use a log for 'status', instead of using if-else, same for other places.

@@ +721,5 @@
>  
>        // If RF is activated for what we think is a Secure Element transaction
>        // and it is deactivated to either IDLE or DISCOVERY mode, notify w/event.
>        if ((eventData->deactivated.type == NFA_DEACTIVATE_TYPE_IDLE)
>         || (eventData->deactivated.type == NFA_DEACTIVATE_TYPE_DISCOVERY)) {

nit, move || to the end of last line.

Comment 29

[Dimi Lee [:dimi]]

Attachment: Part2. Secure element function implementation v3

Comment 30

[Dimi Lee [:dimi]]

Attachment: Part3. Integrate secure element function into Nfc v3

Comment 31

[Dimi Lee [:dimi]]

Attachment: Part4 Enable SE function by default v3

Comment 32

[Dimi Lee [:dimi]]

Attachment: pull request to mozilla-b2g/platform_system_nfcd

Hi Yoshi,
  I merge the patch into one pull request, could you help commit it ? thanks.

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/mozilla-b2g/platform_system_nfcd/commit/56e7fe044cb6a0ad689b96ad7a3636d65e769b34