[PATCH] NFC: Add Core support to generate tag lost event

[PATCH] NFC: Add Core support to generate tag lost event

[Eric Lapuyade]

Some HW/drivers get notifications when a tag moves out of the radio field.
This notification is now forwarded to user space through netlink.

Signed-off-by: Eric Lapuyade <eric.lapuyade@intel.com>
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
---
 include/net/nfc/nfc.h |    5 +++
 net/nfc/core.c        |   84 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletions(-)

diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
index 7273ff1..313d00f 100644
--- a/include/net/nfc/nfc.h
+++ b/include/net/nfc/nfc.h
@@ -62,6 +62,7 @@ struct nfc_ops {
 	int (*data_exchange)(struct nfc_dev *dev, u32 target_idx,
 			     struct sk_buff *skb, data_exchange_cb_t cb,
 			     void *cb_context);
+	int (*check_presence)(struct nfc_dev *dev, u32 target_idx);
 };
 
 #define NFC_TARGET_IDX_ANY -1
@@ -107,6 +108,10 @@ struct nfc_dev {
 	int tx_headroom;
 	int tx_tailroom;
 
+	struct timer_list check_pres_timer;
+	struct workqueue_struct *check_pres_wq;
+	struct work_struct check_pres_work;
+
 	struct nfc_ops *ops;
 };
 #define to_nfc_dev(_dev) container_of(_dev, struct nfc_dev, dev)
diff --git a/net/nfc/core.c b/net/nfc/core.c
index e9936d9..a580926 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -33,6 +33,8 @@
 
 #define VERSION "0.1"
 
+#define NFC_CHECK_PRES_FREQ_MS	2000
+
 int nfc_devlist_generation;
 DEFINE_MUTEX(nfc_devlist_mutex);
 
@@ -289,9 +291,14 @@ int nfc_activate_target(struct nfc_dev *dev, u32 target_idx, u32 protocol)
 	}
 
 	rc = dev->ops->activate_target(dev, target_idx, protocol);
-	if (!rc)
+	if (!rc) {
 		dev->activated_target_idx = target_idx;
 
+		if (dev->ops->check_presence)
+			mod_timer(&dev->check_pres_timer, jiffies +
+				  msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
+	}
+
 error:
 	device_unlock(&dev->dev);
 	return rc;
@@ -317,6 +324,9 @@ int nfc_deactivate_target(struct nfc_dev *dev, u32 target_idx)
 		goto error;
 	}
 
+	if (dev->ops->check_presence)
+		del_timer_sync(&dev->check_pres_timer);
+
 	dev->ops->deactivate_target(dev, target_idx);
 	dev->activated_target_idx = NFC_TARGET_IDX_NONE;
 
@@ -352,8 +362,27 @@ int nfc_data_exchange(struct nfc_dev *dev, u32 target_idx, struct sk_buff *skb,
 		goto error;
 	}
 
+	if (dev->activated_target_idx == NFC_TARGET_IDX_NONE) {
+		rc = -ENOTCONN;
+		kfree_skb(skb);
+		goto error;
+	}
+
+	if (target_idx != dev->activated_target_idx) {
+		rc = -EADDRNOTAVAIL;
+		kfree_skb(skb);
+		goto error;
+	}
+
+	if (dev->ops->check_presence)
+		del_timer_sync(&dev->check_pres_timer);
+
 	rc = dev->ops->data_exchange(dev, target_idx, skb, cb, cb_context);
 
+	if (!rc && dev->ops->check_presence)
+		mod_timer(&dev->check_pres_timer, jiffies +
+			  msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
+
 error:
 	device_unlock(&dev->dev);
 	return rc;
@@ -517,11 +546,46 @@ static void nfc_release(struct device *d)
 
 	pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
+	if (dev->ops->check_presence) {
+		del_timer_sync(&dev->check_pres_timer);
+		destroy_workqueue(dev->check_pres_wq);
+	}
+
 	nfc_genl_data_exit(&dev->genl_data);
 	kfree(dev->targets);
 	kfree(dev);
 }
 
+static void nfc_check_pres_work(struct work_struct *work)
+{
+	struct nfc_dev *dev = container_of(work, struct nfc_dev,
+					   check_pres_work);
+	int rc;
+
+	device_lock(&dev->dev);
+
+	if (dev->activated_target_idx != NFC_TARGET_IDX_NONE &&
+	    timer_pending(&dev->check_pres_timer) == 0) {
+		rc = dev->ops->check_presence(dev, dev->activated_target_idx);
+		if (!rc) {
+			mod_timer(&dev->check_pres_timer, jiffies +
+				  msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
+		} else {
+			nfc_target_lost(dev, dev->activated_target_idx);
+			dev->activated_target_idx = NFC_TARGET_IDX_NONE;
+		}
+	}
+
+	device_unlock(&dev->dev);
+}
+
+static void nfc_check_pres_timeout(unsigned long data)
+{
+	struct nfc_dev *dev = (struct nfc_dev *)data;
+
+	queue_work(dev->check_pres_wq, &dev->check_pres_work);
+}
+
 struct class nfc_class = {
 	.name = "nfc",
 	.dev_release = nfc_release,
@@ -589,6 +653,24 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
 
 	dev->activated_target_idx = NFC_TARGET_IDX_NONE;
 
+	if (ops->check_presence) {
+		char name[32];
+		init_timer(&dev->check_pres_timer);
+		dev->check_pres_timer.data = (unsigned long)dev;
+		dev->check_pres_timer.function = nfc_check_pres_timeout;
+
+		INIT_WORK(&dev->check_pres_work, nfc_check_pres_work);
+		snprintf(name, sizeof(name), "nfc%d_check_pres_wq", dev->idx);
+		dev->check_pres_wq = alloc_workqueue(name, WQ_NON_REENTRANT |
+						     WQ_UNBOUND |
+						     WQ_MEM_RECLAIM, 1);
+		if (dev->check_pres_wq == NULL) {
+			kfree(dev);
+			return NULL;
+		}
+	}
+
+
 	return dev;
 }
 EXPORT_SYMBOL(nfc_allocate_device);
-- 
1.7.6.5

Re: [PATCH] NFC: Add Core support to generate tag lost event

[Samuel Ortiz]

Hi Eric,

On Fri, Mar 23, 2012 at 12:19:53PM +0100, Eric Lapuyade wrote:
> Some HW/drivers get notifications when a tag moves out of the radio field.
> This notification is now forwarded to user space through netlink.
Thanks, applied to my nfc-next branch.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH] NFC: Add Core support to generate tag lost event

[Lauro Ramos Venancio]

2012/3/23 Eric Lapuyade <eric.lapuyade@linux.intel.com>:
> Some HW/drivers get notifications when a tag moves out of the radio field.
> This notification is now forwarded to user space through netlink.

I think to put a timer in the NFC core is not the best solution for
this problem.
This solution is not suitable for devices that receive an event when
the tag moves out of the field.
It would be better the NFC core to receive this as an event and to
move the timer to the device driver.

Regards,

Lauro

>
> Signed-off-by: Eric Lapuyade <eric.lapuyade@intel.com>
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  include/net/nfc/nfc.h |    5 +++
>  net/nfc/core.c        |   84 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
> index 7273ff1..313d00f 100644
> --- a/include/net/nfc/nfc.h
> +++ b/include/net/nfc/nfc.h
> @@ -62,6 +62,7 @@ struct nfc_ops {
>        int (*data_exchange)(struct nfc_dev *dev, u32 target_idx,
>                             struct sk_buff *skb, data_exchange_cb_t cb,
>                             void *cb_context);
> +       int (*check_presence)(struct nfc_dev *dev, u32 target_idx);
>  };
>
>  #define NFC_TARGET_IDX_ANY -1
> @@ -107,6 +108,10 @@ struct nfc_dev {
>        int tx_headroom;
>        int tx_tailroom;
>
> +       struct timer_list check_pres_timer;
> +       struct workqueue_struct *check_pres_wq;
> +       struct work_struct check_pres_work;
> +
>        struct nfc_ops *ops;
>  };
>  #define to_nfc_dev(_dev) container_of(_dev, struct nfc_dev, dev)
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index e9936d9..a580926 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -33,6 +33,8 @@
>
>  #define VERSION "0.1"
>
> +#define NFC_CHECK_PRES_FREQ_MS 2000
> +
>  int nfc_devlist_generation;
>  DEFINE_MUTEX(nfc_devlist_mutex);
>
> @@ -289,9 +291,14 @@ int nfc_activate_target(struct nfc_dev *dev, u32 target_idx, u32 protocol)
>        }
>
>        rc = dev->ops->activate_target(dev, target_idx, protocol);
> -       if (!rc)
> +       if (!rc) {
>                dev->activated_target_idx = target_idx;
>
> +               if (dev->ops->check_presence)
> +                       mod_timer(&dev->check_pres_timer, jiffies +
> +                                 msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
> +       }
> +
>  error:
>        device_unlock(&dev->dev);
>        return rc;
> @@ -317,6 +324,9 @@ int nfc_deactivate_target(struct nfc_dev *dev, u32 target_idx)
>                goto error;
>        }
>
> +       if (dev->ops->check_presence)
> +               del_timer_sync(&dev->check_pres_timer);
> +
>        dev->ops->deactivate_target(dev, target_idx);
>        dev->activated_target_idx = NFC_TARGET_IDX_NONE;
>
> @@ -352,8 +362,27 @@ int nfc_data_exchange(struct nfc_dev *dev, u32 target_idx, struct sk_buff *skb,
>                goto error;
>        }
>
> +       if (dev->activated_target_idx == NFC_TARGET_IDX_NONE) {
> +               rc = -ENOTCONN;
> +               kfree_skb(skb);
> +               goto error;
> +       }
> +
> +       if (target_idx != dev->activated_target_idx) {
> +               rc = -EADDRNOTAVAIL;
> +               kfree_skb(skb);
> +               goto error;
> +       }
> +
> +       if (dev->ops->check_presence)
> +               del_timer_sync(&dev->check_pres_timer);
> +
>        rc = dev->ops->data_exchange(dev, target_idx, skb, cb, cb_context);
>
> +       if (!rc && dev->ops->check_presence)
> +               mod_timer(&dev->check_pres_timer, jiffies +
> +                         msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
> +
>  error:
>        device_unlock(&dev->dev);
>        return rc;
> @@ -517,11 +546,46 @@ static void nfc_release(struct device *d)
>
>        pr_debug("dev_name=%s\n", dev_name(&dev->dev));
>
> +       if (dev->ops->check_presence) {
> +               del_timer_sync(&dev->check_pres_timer);
> +               destroy_workqueue(dev->check_pres_wq);
> +       }
> +
>        nfc_genl_data_exit(&dev->genl_data);
>        kfree(dev->targets);
>        kfree(dev);
>  }
>
> +static void nfc_check_pres_work(struct work_struct *work)
> +{
> +       struct nfc_dev *dev = container_of(work, struct nfc_dev,
> +                                          check_pres_work);
> +       int rc;
> +
> +       device_lock(&dev->dev);
> +
> +       if (dev->activated_target_idx != NFC_TARGET_IDX_NONE &&
> +           timer_pending(&dev->check_pres_timer) == 0) {
> +               rc = dev->ops->check_presence(dev, dev->activated_target_idx);
> +               if (!rc) {
> +                       mod_timer(&dev->check_pres_timer, jiffies +
> +                                 msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
> +               } else {
> +                       nfc_target_lost(dev, dev->activated_target_idx);
> +                       dev->activated_target_idx = NFC_TARGET_IDX_NONE;
> +               }
> +       }
> +
> +       device_unlock(&dev->dev);
> +}
> +
> +static void nfc_check_pres_timeout(unsigned long data)
> +{
> +       struct nfc_dev *dev = (struct nfc_dev *)data;
> +
> +       queue_work(dev->check_pres_wq, &dev->check_pres_work);
> +}
> +
>  struct class nfc_class = {
>        .name = "nfc",
>        .dev_release = nfc_release,
> @@ -589,6 +653,24 @@ struct nfc_dev *nfc_allocate_device(struct nfc_ops *ops,
>
>        dev->activated_target_idx = NFC_TARGET_IDX_NONE;
>
> +       if (ops->check_presence) {
> +               char name[32];
> +               init_timer(&dev->check_pres_timer);
> +               dev->check_pres_timer.data = (unsigned long)dev;
> +               dev->check_pres_timer.function = nfc_check_pres_timeout;
> +
> +               INIT_WORK(&dev->check_pres_work, nfc_check_pres_work);
> +               snprintf(name, sizeof(name), "nfc%d_check_pres_wq", dev->idx);
> +               dev->check_pres_wq = alloc_workqueue(name, WQ_NON_REENTRANT |
> +                                                    WQ_UNBOUND |
> +                                                    WQ_MEM_RECLAIM, 1);
> +               if (dev->check_pres_wq == NULL) {
> +                       kfree(dev);
> +                       return NULL;
> +               }
> +       }
> +
> +
>        return dev;
>  }
>  EXPORT_SYMBOL(nfc_allocate_device);
> --
> 1.7.6.5
>

Re: [PATCH] NFC: Add Core support to generate tag lost event

[Samuel Ortiz]

Hi Lauro,

On Fri, 2012-03-23 at 14:11 -0300, Lauro Ramos Venancio wrote:
> 2012/3/23 Eric Lapuyade <eric.lapuyade@linux.intel.com>:
> > Some HW/drivers get notifications when a tag moves out of the radio field.
> > This notification is now forwarded to user space through netlink.
> 
> I think to put a timer in the NFC core is not the best solution for
> this problem.
> This solution is not suitable for devices that receive an event when
> the tag moves out of the field.
I don't know of such device. HCI based devices don't get such event,
neither does the pn53x USB ones.
Ilan, does NCI provide such event ?

Cheers,
Samuel.

RE: [PATCH] NFC: Add Core support to generate tag lost event

[Elias, Ilan]

Hi Samuel,
 
> > > Some HW/drivers get notifications when a tag moves out of 
> the radio field.
> > > This notification is now forwarded to user space through netlink.
> > 
> > I think to put a timer in the NFC core is not the best solution for
> > this problem.
> > This solution is not suitable for devices that receive an event when
> > the tag moves out of the field.
> I don't know of such device. HCI based devices don't get such event,
> neither does the pn53x USB ones.
> Ilan, does NCI provide such event ?
With Frame I/F (e.g. JEWEL, MIFARE), the NCI doesn't provide any such event.
With ISO-DEP I/F (e.g. ISO14443) and NFC-DEP I/F (i.e. P2P), the NCI provide such an event only when the host sends data: the event CORE_INTERFACE_ERROR_NTF (with status RF_TRANSMISSION_ERROR or RF_PROTOCOL_ERROR or RF_TIMEOUT_ERROR) is sent to the host to indicate that the controller encountered an error while sending the data to the tag.

Thanks & BR,
Ilan

Re: [PATCH] NFC: Add Core support to generate tag lost event

[Samuel Ortiz]

On Fri, 2012-03-23 at 14:11 -0300, Lauro Ramos Venancio wrote:
> 2012/3/23 Eric Lapuyade <eric.lapuyade@linux.intel.com>:
> > Some HW/drivers get notifications when a tag moves out of the radio field.
> > This notification is now forwarded to user space through netlink.
> 
> I think to put a timer in the NFC core is not the best solution for
> this problem.
> This solution is not suitable for devices that receive an event when
> the tag moves out of the field.
One more thing: If the device provides such capability, the driver can
then call nfc_target_lost() directly, and not register a
presence_check() ops. Without such an ops, the timer won't get started,
obviously.
presence_check() is for drivers that are limited to asking the firmware
about the target being on the field or not.

Cheers,
Samuel.

Re: [PATCH] NFC: Add Core support to generate tag lost event

[Lauro Ramos Venancio]

HI Samuel,

2012/3/23 Samuel Ortiz <samuel@sortiz.org>:
> On Fri, 2012-03-23 at 14:11 -0300, Lauro Ramos Venancio wrote:
>> 2012/3/23 Eric Lapuyade <eric.lapuyade@linux.intel.com>:
>> > Some HW/drivers get notifications when a tag moves out of the radio field.
>> > This notification is now forwarded to user space through netlink.
>>
>> I think to put a timer in the NFC core is not the best solution for
>> this problem.
>> This solution is not suitable for devices that receive an event when
>> the tag moves out of the field.
> One more thing: If the device provides such capability, the driver can
> then call nfc_target_lost() directly, and not register a
> presence_check() ops. Without such an ops, the timer won't get started,
> obviously.
> presence_check() is for drivers that are limited to asking the firmware
> about the target being on the field or not.
>
 I am fine with this kind of solution. My concern is that, maybe, it
shouldn't be put in the core.
We should avoid having an event that is sometimes generated by the
device driver and sometimes generated by the core.
My suggestion is to make the check_presence the first helper function
in a "nfc-utils". The device drivers would be able to decide to use or
not the nfc-utils helpers. The nfc core would be cleaner and would be
easier to anyone understand what must be implemented in a new device
driver.
This is just a suggestion. I will no oppose if you decide to keep the
current implementation.

Regards,
Lauro

RE: [PATCH] NFC: Add Core support to generate tag lost event

[Elias, Ilan]

Hi Eric, 

> +static void nfc_check_pres_work(struct work_struct *work)
> +{
> +	struct nfc_dev *dev = container_of(work, struct nfc_dev,
> +					   check_pres_work);
> +	int rc;
> +
> +	device_lock(&dev->dev);
> +
> +	if (dev->activated_target_idx != NFC_TARGET_IDX_NONE &&
> +	    timer_pending(&dev->check_pres_timer) == 0) {
> +		rc = dev->ops->check_presence(dev, 
> dev->activated_target_idx);
> +		if (!rc) {
> +			mod_timer(&dev->check_pres_timer, jiffies +
> +				  
> msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
> +		} else {
> +			nfc_target_lost(dev, dev->activated_target_idx);
I'm unable to find the definition of the function nfc_target_lost.

Thanks & BR,
Ilan

RE: [PATCH] NFC: Add Core support to generate tag lost event

[Samuel Ortiz]

Hi Ilan,

On Mon, 2012-03-26 at 09:48 +0000, Elias, Ilan wrote:
> Hi Eric, 
> 
> > +static void nfc_check_pres_work(struct work_struct *work)
> > +{
> > +	struct nfc_dev *dev = container_of(work, struct nfc_dev,
> > +					   check_pres_work);
> > +	int rc;
> > +
> > +	device_lock(&dev->dev);
> > +
> > +	if (dev->activated_target_idx != NFC_TARGET_IDX_NONE &&
> > +	    timer_pending(&dev->check_pres_timer) == 0) {
> > +		rc = dev->ops->check_presence(dev, 
> > dev->activated_target_idx);
> > +		if (!rc) {
> > +			mod_timer(&dev->check_pres_timer, jiffies +
> > +				  
> > msecs_to_jiffies(NFC_CHECK_PRES_FREQ_MS));
> > +		} else {
> > +			nfc_target_lost(dev, dev->activated_target_idx);
> I'm unable to find the definition of the function nfc_target_lost.
This patch is based on my nfc-next branch, where nfc_target_lost is
implemented. I'm waiting for John to take patches for the next merge
window before sending him a pull request for that.

Cheers,
Samuel.