Hi Ilan,
On Tue, 2011-08-16 at 19:36 +0200, Elias, Ilan wrote:
> The NFC Controller Interface (NCI) is a standard communication protocol between an NFC Controller (NFCC) and a Device Host (DH), defined by the NFC Forum.
>
> Signed-off-by: Ilan Elias <ilane@xxxxxx>
> ---
> include/net/nfc/nci.h | 313 ++++++++++++++++++
> include/net/nfc/nci_core.h | 177 +++++++++++
> net/nfc/Kconfig | 2 +
> net/nfc/Makefile | 1 +
> net/nfc/nci/Kconfig | 10 +
> net/nfc/nci/Makefile | 7 +
> net/nfc/nci/nci_core.c | 750 ++++++++++++++++++++++++++++++++++++++++++++
> net/nfc/nci/nci_data.c | 252 +++++++++++++++
> net/nfc/nci/nci_lib.c | 94 ++++++
> net/nfc/nci/nci_ntf.c | 264 ++++++++++++++++
> net/nfc/nci/nci_rsp.c | 226 +++++++++++++
> 11 files changed, 2096 insertions(+), 0 deletions(-)
> create mode 100644 include/net/nfc/nci.h
> create mode 100644 include/net/nfc/nci_core.h
> create mode 100644 net/nfc/nci/Kconfig
> create mode 100644 net/nfc/nci/Makefile
> create mode 100644 net/nfc/nci/nci_core.c
> create mode 100644 net/nfc/nci/nci_data.c
> create mode 100644 net/nfc/nci/nci_lib.c
> create mode 100644 net/nfc/nci/nci_ntf.c
> create mode 100644 net/nfc/nci/nci_rsp.c
In general, I prefer the nci/core.c, nci/data.c, etc naming scheme as opposed to the more redundant one you're using here.
> +/* NCI Discovery Types */
> +#define NCI_DISCOVERY_TYPE_POLL_A_PASSIVE 0x00
Identation fix.
> +#define NCI_DISCOVERY_TYPE_POLL_B_PASSIVE 0x01
> +#define NCI_DISCOVERY_TYPE_POLL_F_PASSIVE 0x02
> +#define NCI_DISCOVERY_TYPE_POLL_A_ACTIVE 0x03
> +#define NCI_DISCOVERY_TYPE_POLL_F_ACTIVE 0x05
> +#define NCI_DISCOVERY_TYPE_WAKEUP_A_PASSIVE 0x06
> +#define NCI_DISCOVERY_TYPE_WAKEUP_B_PASSIVE 0x07
> +#define NCI_DISCOVERY_TYPE_WAKEUP_A_ACTIVE 0x09
> +#define NCI_DISCOVERY_TYPE_LISTEN_A_PASSIVE 0x80
> +#define NCI_DISCOVERY_TYPE_LISTEN_B_PASSIVE 0x81
> +#define NCI_DISCOVERY_TYPE_LISTEN_F_PASSIVE 0x82
> +#define NCI_DISCOVERY_TYPE_LISTEN_A_ACTIVE 0x83
> +#define NCI_DISCOVERY_TYPE_LISTEN_F_ACTIVE 0x85
> +#define NCI_MT_DATA_PKT 0x00
Ditto.
> +#define NCI_MT_CMD_PKT 0x01
> +#define NCI_MT_RSP_PKT 0x02
> +#define NCI_MT_NTF_PKT 0x03
> +/* Reserved for drivers usage */
> +#define NCI_SKB_RESERVE 4
Do we expect all drivers to need at most 4 bytes ?
> +#define nci_req_lock(ndev) mutex_lock(&ndev->req_lock)
> +#define nci_req_unlock(ndev) mutex_unlock(&ndev->req_lock)
This kind of abstraction doesn't bring much to the table. I'd prefer to
use the mutex API directly.
> +/* Execute request and wait for completion. */
> +static int __nci_request(struct nci_dev *ndev,
> + void (*req)(struct nci_dev *ndev, unsigned long opt),
> + unsigned long opt,
> + __u32 timeout)
> +{
> + DECLARE_WAITQUEUE(wait, current);
> + int rc = 0;
> +
> + ndev->req_status = NCI_REQ_PEND;
> +
> + add_wait_queue(&ndev->req_wait_q, &wait);
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + req(ndev, opt);
> + schedule_timeout(timeout);
> +
> + remove_wait_queue(&ndev->req_wait_q, &wait);
> +
> + if (signal_pending(current))
> + return -EINTR;
> +
> + switch (ndev->req_status) {
> + case NCI_REQ_DONE:
> + rc = nci_to_errno(ndev->req_result);
> + break;
> +
> + case NCI_REQ_CANCELED:
> + rc = -ndev->req_result;
> + break;
> +
> + default:
> + rc = -ETIMEDOUT;
> + break;
> + }
> +
> + ndev->req_status = ndev->req_result = 0;
> +
> + return rc;
> +}
So here, I wonder: Can't we just use simple completion and the
wait_for_completion_* APIs ? One completion structure per ndev and you
should be fine. Especially since you seem to be serializing (as
expected) the requests below.
> +/* Send NCI command */
> +int nci_send_cmd(struct nci_dev *ndev, __u16 opcode, __u8 plen, void *payload)
> +{
> + struct nci_ctrl_hdr *hdr;
> + struct sk_buff *skb;
> +
> + nfc_dbg("entry, opcode 0x%x, plen %d", opcode, plen);
> +
> + skb = nci_skb_alloc((NCI_CTRL_HDR_SIZE + plen), GFP_KERNEL);
> + if (!skb) {
> + nfc_err("no memory for command");
> + return -ENOMEM;
> + }
> +
> + hdr = (struct nci_ctrl_hdr *) skb_put(skb, NCI_CTRL_HDR_SIZE);
> + hdr->gid = nci_opcode_gid(opcode);
> + hdr->oid = nci_opcode_oid(opcode);
> + hdr->plen = plen;
> +
> + nci_mt_set((__u8 *)hdr, NCI_MT_CMD_PKT);
> + nci_pbf_set((__u8 *)hdr, NCI_PBF_LAST);
> +
> + if (plen)
> + memcpy(skb_put(skb, plen), payload, plen);
> +
> + skb->dev = (void *) ndev;
> +
> + skb_queue_tail(&ndev->cmd_q, skb);
> + tasklet_schedule(&ndev->cmd_task);
> +
> + return 0;
> +}
So I have 2 comments on this one:
1) Do we have a compelling reason for deferring the command queue
handling ? In which case can you queue a command while the previous one
is not completed yet ?
2) If we do, we certainly don't want a tasklet for that. Using a tasklet
puts some additional constraints on what we can call for it, the
tradeoff being a supposedly lower latency. I don't think we need it,
workqueues (for cmd, tx and rx) would be just fine.
> +/* Send NCI data */
> +int nci_send_data(struct nci_dev *ndev, __u8 conn_id, struct sk_buff *skb)
> +{
> + int rc = 0;
> +
> + nfc_dbg("entry, conn_id 0x%x, plen %d", conn_id, skb->len);
> +
> + /* check if the packet need to be fragmented */
> + if (skb->len <= ndev->max_pkt_payload_size) {
> + /* no need to fragment packet */
> +
> + /* reserve header space for nci and driver */
> + if (skb_cow_head(skb, (NCI_DATA_HDR_SIZE + NCI_SKB_RESERVE))) {
I think this kind of reallocations could be avoided with a couple
patches that I will post soon. We could basically tell the NFC core in
advance which head room we need.
See
http://git.kernel.org/?p=linux/kernel/git/sameo/nfc-2.6.git;a=commit;h=ba108daaec65f5d02daaf9084fa5f8f2375d0d92
Cheers,
Samuel.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
