1009483 - [NFC] Emulator support send deactivate notification

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

NFC testcase test_nfc_manager_tech_discovered.js fail after bug 1008109 landing.

The reason is that libnfc-nci is waiting for deactivate notification but emulator will not send it after receiving deactivate command.

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

test_nfc_peer.js and test_nfc_peer_sendndef.js are also failing

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb. 9]

For now the work-around is rollback nfcd to 6b4fb9da3baf141330f261d1721f9e12a2fe7d47.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

IRC log 


<tzimmermann> pong
<tzimmermann> you always ping me while i'm at lunch :D
<DimiLee> haha
<DimiLee> have a question
<tzimmermann> how can i help you?
<DimiLee> are you available now ?
<tzimmermann> yes
<DimiLee> i create a bug 1009438
<tzimmermann> sure?
<tzimmermann> https://bugzilla.mozilla.org/show_bug.cgi?id=1009438
<DimiLee> bug 1009483 
<DimiLee> sorry
<tzimmermann> i'm currently looking at that bug
<tzimmermann> what's your question?
<DimiLee> to support send notification after processing deactivate command
<DimiLee> it seems related to a discussion we had before about register a callback function
<tzimmermann> yes. i need to take a look at the source code to be sure, but it sounds like the callback we where talking about
<DimiLee> yes
<tzimmermann> i'm a bit surprised that an extra deactivate_ntf is required, but the spec clearly says so
<DimiLee> ya, libnfc-nci  waiting for the notification
<DimiLee> and if there is no notification he will force to call deactivate again
<DimiLee> and emulator is already in idle state, so when in idle state we received another deactivate command it will trigger the assertion
<tzimmermann> hmm, i could imagine that this is the cause of bug 1000063
<tzimmermann> as i mentioned last time, we have to wait until the response has been received before we can generate the notification
<DimiLee> yes, i forget where you suggest me to check and call the callback
<DimiLee> so that's my question
<tzimmermann> in hw/goldfish_nfc.c
<tzimmermann> there are two functions: goldfish_nfc_send_dta and goldfish_nfc_send_ntf
<tzimmermann> both should get a new argument with the callback function
<tzimmermann> the functions should be called from within goldfish_nfc_process_ctrl
<DimiLee> so check after CTRL_RESP_RCV ?
<tzimmermann> for dta in the CTRL_DATA_RCV_ branch, for ntf in the CTRL_NTFN_RCV branch. for responses, we need a different mechanism to set the callback function
<tzimmermann> the difference with responses is that they are generated by the command handler
<DimiLee> sorry i don't get it, in this case, the notification should be registered when process command
<tzimmermann> right, but we should handle all three cases: data, notification and response. maybe nfc_process_nci_msg and nfc_process_hci_msg could return the function pointer in an extra argument
<tzimmermann> see goldfish_nfc.c:132
<tzimmermann> currently, the response get's stored in s->resp and is then delivered to the guest system
<DimiLee> yes
<tzimmermann> we could extend this call
<tzimmermann> with an extra argument for returning the callback pointer and maybe a pointer to user data
<DimiLee> yes
<tzimmermann> do you think that's sound?
<DimiLee> sounds good, but in this  case , we should call the callback function after CTRL_RESP_RCV ?
<tzimmermann> exactly.
<DimiLee> ok i will try to implement it tomorrow, seems this is related to test case fail. the priority is high
<tzimmermann> the arguments for these callback functions should be the user data and s->ntfn
<DimiLee> ok
<tzimmermann> i'm sure there are details that will come up during the implementation
<DimiLee> ya, i will ask you for help ! 
<DimiLee> when you are not in lunch
<tzimmermann> :D
<tzimmermann> thanks a lot for working on this problem! i have a conference this week during afternoons, so my time is a bit limited
<DimiLee> no problem, just send me a mail if you have any suggest about how to implement this
<tzimmermann> ok

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

We had additional discussion with Dimi on IRC. The temporary workaround which Dimi proposed is to rollback to commit 83e1560bef0988bbdec49d15cb8eef402857d4ef (Date: Mon Apr 28 17:49:28 2014 +0800).
I've tested this and all 5 tests are passing.

Comment 5

[Dimi Lee [:dimi]]

Attachment: WIP Patch v1

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8422370 [details] [diff] [review]
WIP Patch v1

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

Hi Thomas,
  This is a WIP patch, it works ok for this bug but i have not yet done lots of tests on it and the naming may still need to be refine.
But I would like you to provide some suggestion first to make sure it fit the design, thanks!

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422370 [details] [diff] [review]
WIP Patch v1

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

Hi Dimi,

can you please split the patch into a set of smaller changes for the next review. It's hard to see which changes belong together. I suggest one patch for

  - nfc_create_rf_intf_deactivate_ntf,
  - one for adding the callback infrastructure, and
  - one for creating an actual notification on deactivation.

::: hw/goldfish_nfc.c
@@ +66,4 @@
>      uint8_t reserved2[2556]; /* ceil to 4096 */
>  
>      struct nfc_device nfc;
> +    void* callback;

Please store the callback as function pointer, not raw address.

@@ +328,5 @@
>      return 0;
>  }
> +
> +void
> +goldfish_nfc_check_pending_ntfn()

Maybe 'handle_pending_cb.'

'Notification' in that context is not a good name, because it can easily be confused with NCI notification. The callback should be more generic IMHO.

The whole message handling seems to have too many parameters. I suggest to add a new interface for this, as sketched out here:

/* the buffer-type for the delivery callback
 */
enum nfc_buf_type {
  NO_BUF,
  NTFN_BUF,
  DATA_BUF
};

/* supplied to process_{nci,hci}_message
 */
struct nfc_delivery_cb {
  enum nfc_buf_type type;
  void* user_data;
  ssize_t (*cb)(void* /*user_data*/, union_nci_packet*)
};

/* called from within process_nci_message  to setup the supplied delivery callback
 */
int
nfc_delivery_cb_setup(struct nfc_delivery_cb* cb, enum nfc_buf_type type, func, user_data);

/* called after delivering a response to handle the set-up delivery callback
 */
int
nfc_delivery_cb_handle(struct nfc_delivery_cb* cb, union nci_packet*);

This should encapsulate the delivery callback well enough for our purposes.

::: hw/nfc-nci.h
@@ +147,5 @@
>      MAX_NCI_PAYLOAD_LENGTH = 256
>  };
>  
> +enum {
> +    NUMBER_OF_SUPPORTED_NCI_RF_INTERFACES = 2

That's a property of the emulated NFC chip, no the NCI protocol. so it should be located  in nfc.h.

@@ +557,4 @@
>                                   union nci_packet* ntf);
>  
>  size_t
> +nfc_create_rf_intf_deactivate_ntf(enum nci_rf_deactivation_type type,

Just 'nfc_create_rf_deactivate_ntf'.

Comment 8

[Dimi Lee [:dimi]]

Attachment: Part1. Add callback infrastructure for NFC v1

Comment 9

[Dimi Lee [:dimi]]

Attachment: Part2. Support create deactivate notification v1

Comment 10

[Dimi Lee [:dimi]]

Attachment: Part3. Send deactivate notification v1

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422993 [details] [diff] [review]
Part2. Support create deactivate notification v1

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

Looks good.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422991 [details] [diff] [review]
Part1. Add callback infrastructure for NFC v1

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

::: hw/goldfish_nfc.c
@@ +137,5 @@
> +        memset(s->ntfn, 0, maxlen);
> +        res = s->cb.func(&s->cb.data, (union nci_packet*)s->ntfn);
> +
> +        goldfish_set_ntfn(s, res);
> +    }

Don't forget the other cases.

@@ +337,3 @@
>      maxlen = MIN(sizeof(s->ntfn), MAX_NCI_PAYLOAD_LENGTH);
>      memset(s->ntfn, 0, maxlen);
>      res = create(data, &s->nfc, maxlen, (union nci_packet*)s->ntfn);

Please keep the (res < 0) test here.

::: hw/nfc.c
@@ +79,5 @@
>  }
> +
> +void
> +nfc_delivery_cb_setup(struct nfc_delivery_cb* cb, enum nfc_buf_type type,
> +                      ssize_t (*func)(const struct nfc_cb_param*, union nci_packet*))

User data is missing.

@@ +91,5 @@
> +ssize_t
> +nfc_delivery_cb_handle(const struct nfc_cb_param* param, union nci_packet* pkt)
> +{
> +    ssize_t res;
> +

When I said 'handle function' in the last review, I meant the function that executes the call-back function, not the callback itself.

And please don't use a central function for implementing callbacks. See my review of patch 3.

::: hw/nfc.h
@@ +61,5 @@
> +
> +/* supplied to process_{nci,hci}_message */
> +struct nfc_delivery_cb {
> +    enum nfc_buf_type type;
> +    struct nfc_cb_param data;

User data should be a pointer of type void*. The supplied callback function knowns how to interpret it.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422994 [details] [diff] [review]
Part3. Send deactivate notification v1

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

::: hw/nfc-nci.c
@@ +707,5 @@
> +    cb->data.oid = NCI_OID_RF_DEACTIVATED_NTF;
> +    cb->data.ntf.deact.type = payload->type;
> +    cb->data.ntf.deact.reason = NCI_RF_DEACT_DH_REQUEST;
> +
> +    nfc_delivery_cb_setup(cb, NTFN_BUF, nfc_delivery_cb_handle);

Instead of storing the parameters in cb->data and passing nfc_delivery_cb_handle, please write a function for settings up the notification and pass this function. I mentioned that user data is void*. For this simple case, you can cast packet->type to (void*) and pass it that way.

::: hw/nfc.h
@@ +67,1 @@
>  };

See my other comments about why this structure should go.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dimi,

Thanks a lot, that's making good progress. It's just missing a cleanup of the callback itself and some smaller nits. If you're not going to implement delivery callbacks for the goldfish send functions, please open a new bug for it.

Comment 15

[Dimi Lee [:dimi]]

Attachment: Part4. Merge set status code to one function v1

Comment 16

[Dimi Lee [:dimi]]

Attachment: Part1. Add callback infrastructure for NFC v2

Comment 17

[Dimi Lee [:dimi]]

Attachment: Part2. Support create deactivate notification v2

Comment 18

[Dimi Lee [:dimi]]

Attachment: Part3. Send deactivate notification v2

Comment 19

[Dimi Lee [:dimi]]

Attachment: Part2. Support create deactivate notification v2

Hi Thomas,
 Please help review this part again because after checking NCI spec, there is only certain case require send deactivate notification.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8423780 [details] [diff] [review]
Part4. Merge set status code to one function v1

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

r+ with nits.

::: hw/goldfish_nfc.c
@@ +105,4 @@
>  }
>  
>  static void
> +goldfish_set_status(struct nfc_state* s, int status, ssize_t res)

Should better be named 'goldfish_nfc_set_status'. 'Res' only makes sense in the context of the caller, so 'ssize_t res' should be 'int set' with 'set' being interpreted as a boolean.

@@ +146,2 @@
>  
> +      goldfish_set_status(s, STATUS_NCI_RESP, res);

Pass '!!res' or 'res > 0' as final argument.

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8423780 [details] [diff] [review]
Part4. Merge set status code to one function v1

When I apply the patches, this is actually part 1.

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8423782 [details] [diff] [review]
Part1. Add callback infrastructure for NFC v2

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

::: hw/goldfish_nfc.c
@@ +128,5 @@
> +    }
> +
> +    if (s->cb.type == NTFN_BUF) {
> +        maxlen = MIN(sizeof(s->ntfn), MAX_NCI_PAYLOAD_LENGTH);
> +        memset(s->ntfn, 0, maxlen);

Memset'ing here wastes CPU cyles. We should maybe just remove similar memsets in other places.

@@ +135,5 @@
> +        goldfish_set_status(s, STATUS_NCI_NTFN, res);
> +
> +    } else if (s->cb.type == DATA_BUF) {
> +        maxlen = MIN(sizeof(s->data), MAX_NCI_PAYLOAD_LENGTH);
> +        memset(s->data, 0, maxlen);

Same here.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8423784 [details] [diff] [review]
Part3. Send deactivate notification v2

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

::: hw/nfc-nci.c
@@ +703,4 @@
>          nfc_res[i].id = 0;
>      }
>  
> +    data = qemu_mallocz(sizeof(struct nfc_deactivate_ntf_param));

No need to clear the allocated memory. Use |qemu_malloc| instead.

::: hw/nfc.c
@@ +89,5 @@
>      cb->func = func;
>  }
>  
> +ssize_t
> +nfc_delivery_deactivate_cmd_cb(void* data, union nci_packet* pkt)

Please move this function and nfc_deactivate_ntf_param to nfc-nci, next to the deactivate_cmd.

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8424650 [details] [diff] [review]
Part2. Support create deactivate notification v2

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

::: hw/nfc-nci.c
@@ +657,4 @@
>      unsigned long bits;
>      enum nfc_rfst rfst;
>      size_t i;
> +    bool sendNTF = true;

C doesn't have bool before C99. It's common to use int with 0/1 being false/true. And please name the variable |send_ntf|.

Comment 25

[Dimi Lee [:dimi]]

Attachment: Part2. Support create deactivate notification v3

This patch is already r+ before and nothing change

Comment 26

[Dimi Lee [:dimi]]

Attachment: Part3. Send deactivate notification v3

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8424681 [details] [diff] [review]
Part3. Send deactivate notification v3

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

::: hw/nfc-nci.c
@@ +657,4 @@
>      unsigned long bits;
>      enum nfc_rfst rfst;
>      size_t i;
> +    bool sendNTF = true;

The same as in the previous review: C doesn't have bool before C99. It's common to use int with 0/1 being false/true. And please name the variable |send_ntf|.

@@ +716,5 @@
>  
> +    if (sendNTF) {
> +        struct nfc_deactivate_ntf_param* data;
> +
> +        data = qemu_mallocz(sizeof(struct nfc_deactivate_ntf_param));

This should be qemu_malloc. We don't need to allocate cleared memory.

@@ +720,5 @@
> +        data = qemu_mallocz(sizeof(struct nfc_deactivate_ntf_param));
> +        data->type = payload->type;
> +        data->reason = NCI_RF_DEACT_DH_REQUEST;
> +
> +        nfc_delivery_cb_setup(cb, NTFN_BUF, (void*)data,

No need to cast :)

::: hw/nfc.c
@@ +89,5 @@
>      cb->func = func;
>  }
>  
> +ssize_t
> +nfc_delivery_deactivate_cmd_cb(void* data, union nci_packet* pkt)

Move this function to nfc-nci.c, next to deactivate_cmd.

::: hw/nfc.h
@@ +54,4 @@
>      uint8_t config_id_value[128];
>  };
>  
> +struct nfc_deactivate_ntf_param {

Move this data structure to nfc-nci.c, next to deactivate_cmd.

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dimi,

this looks nice besides some minor nits. Thanks a lot for the patch set.

Comment 29

[Dimi Lee [:dimi]]

Attachment: pull request for mozilla-b2g/platform_external_qemu

pull request to platform_external_qemu with all nits fixed.
And pass local nfc test cases

Comment 30

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/platform_external_qemu/commit/acbdbe5196f06589da05c3bd1f14b27a6cae69c2