The following 2 NFC control operations (via generic netlink) were added:
- Dev_up, which turn on the NFC controller (this operation may take a few seconds, as it can download new FW to the NFC controller)
- Dev_down, which turn off the NFC controller
Signed-off-by: Ilan Elias <[email protected]>
---
drivers/nfc/pn533.c | 2 +
include/linux/nfc.h | 6 ++++
include/net/nfc.h | 3 ++
net/nfc/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++
net/nfc/netlink.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++
net/nfc/nfc.h | 4 +++
6 files changed, 148 insertions(+), 0 deletions(-)
diff --git a/drivers/nfc/pn533.c b/drivers/nfc/pn533.c
index c77e054..d931e9c 100644
--- a/drivers/nfc/pn533.c
+++ b/drivers/nfc/pn533.c
@@ -1445,6 +1445,8 @@ static int pn533_set_configuration(struct pn533 *dev, u8 cfgitem, u8 *cfgdata,
}
struct nfc_ops pn533_nfc_ops = {
+ .dev_up = NULL,
+ .dev_down = NULL,
.start_poll = pn533_start_poll,
.stop_poll = pn533_stop_poll,
.activate_target = pn533_activate_target,
diff --git a/include/linux/nfc.h b/include/linux/nfc.h
index 330a4c5..2056969 100644
--- a/include/linux/nfc.h
+++ b/include/linux/nfc.h
@@ -39,6 +39,10 @@
*
* @NFC_CMD_GET_DEVICE: request information about a device (requires
* %NFC_ATTR_DEVICE_INDEX) or dump request to get a list of all nfc devices
+ * @NFC_CMD_DEV_UP: turn on the nfc device
+ * (requires %NFC_ATTR_DEVICE_INDEX)
+ * @NFC_CMD_DEV_DOWN: turn off the nfc device
+ * (requires %NFC_ATTR_DEVICE_INDEX)
* @NFC_CMD_START_POLL: start polling for targets using the given protocols
* (requires %NFC_ATTR_DEVICE_INDEX and %NFC_ATTR_PROTOCOLS)
* @NFC_CMD_STOP_POLL: stop polling for targets (requires
@@ -56,6 +60,8 @@
enum nfc_commands {
NFC_CMD_UNSPEC,
NFC_CMD_GET_DEVICE,
+ NFC_CMD_DEV_UP,
+ NFC_CMD_DEV_DOWN,
NFC_CMD_START_POLL,
NFC_CMD_STOP_POLL,
NFC_CMD_GET_TARGET,
diff --git a/include/net/nfc.h b/include/net/nfc.h
index cc01303..5ee60de 100644
--- a/include/net/nfc.h
+++ b/include/net/nfc.h
@@ -48,6 +48,8 @@ typedef void (*data_exchange_cb_t)(void *context, struct sk_buff *skb,
int err);
struct nfc_ops {
+ int (*dev_up)(struct nfc_dev *dev);
+ int (*dev_down)(struct nfc_dev *dev);
int (*start_poll)(struct nfc_dev *dev, u32 protocols);
void (*stop_poll)(struct nfc_dev *dev);
int (*activate_target)(struct nfc_dev *dev, u32 target_idx,
@@ -78,6 +80,7 @@ struct nfc_dev {
int targets_generation;
spinlock_t targets_lock;
struct device dev;
+ bool dev_up;
bool polling;
struct nfc_genl_data genl_data;
u32 supported_protocols;
diff --git a/net/nfc/core.c b/net/nfc/core.c
index b6fd4e1..b9cc7b9 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -53,6 +53,75 @@ int nfc_printk(const char *level, const char *format, ...)
EXPORT_SYMBOL(nfc_printk);
/**
+ * nfc_dev_up - turn on the NFC device
+ *
+ * @dev: The nfc device to be turned on
+ *
+ * The device remains up until the nfc_dev_down function is called.
+ */
+int nfc_dev_up(struct nfc_dev *dev)
+{
+ int rc = 0;
+
+ nfc_dbg("dev_name=%s", dev_name(&dev->dev));
+
+ device_lock(&dev->dev);
+
+ if (!device_is_registered(&dev->dev)) {
+ rc = -ENODEV;
+ goto error;
+ }
+
+ if (dev->dev_up) {
+ rc = -EALREADY;
+ goto error;
+ }
+
+ if (dev->ops->dev_up)
+ rc = dev->ops->dev_up(dev);
+
+ if (!rc)
+ dev->dev_up = true;
+
+error:
+ device_unlock(&dev->dev);
+ return rc;
+}
+
+/**
+ * nfc_dev_down - turn off the NFC device
+ *
+ * @dev: The nfc device to be turned off
+ */
+int nfc_dev_down(struct nfc_dev *dev)
+{
+ int rc = 0;
+
+ nfc_dbg("dev_name=%s", dev_name(&dev->dev));
+
+ device_lock(&dev->dev);
+
+ if (!device_is_registered(&dev->dev)) {
+ rc = -ENODEV;
+ goto error;
+ }
+
+ if (!dev->dev_up) {
+ rc = -EINVAL;
+ goto error;
+ }
+
+ if (dev->ops->dev_down)
+ dev->ops->dev_down(dev);
+
+ dev->dev_up = false;
+
+error:
+ device_unlock(&dev->dev);
+ return rc;
+}
+
+/**
* nfc_start_poll - start polling for nfc targets
*
* @dev: The nfc device that must start polling
diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
index ccdff79..4e572b4 100644
--- a/net/nfc/netlink.c
+++ b/net/nfc/netlink.c
@@ -367,6 +367,60 @@ out_putdev:
return rc;
}
+static int nfc_genl_dev_up(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nfc_dev *dev;
+ int rc;
+ u32 idx;
+
+ nfc_dbg("entry");
+
+ if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
+ return -EINVAL;
+
+ idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
+
+ dev = nfc_get_device(idx);
+ if (!dev)
+ return -ENODEV;
+
+ mutex_lock(&dev->genl_data.genl_data_mutex);
+
+ rc = nfc_dev_up(dev);
+
+ mutex_unlock(&dev->genl_data.genl_data_mutex);
+
+ nfc_put_device(dev);
+ return rc;
+}
+
+static int nfc_genl_dev_down(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nfc_dev *dev;
+ int rc;
+ u32 idx;
+
+ nfc_dbg("entry");
+
+ if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
+ return -EINVAL;
+
+ idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
+
+ dev = nfc_get_device(idx);
+ if (!dev)
+ return -ENODEV;
+
+ mutex_lock(&dev->genl_data.genl_data_mutex);
+
+ rc = nfc_dev_down(dev);
+
+ mutex_unlock(&dev->genl_data.genl_data_mutex);
+
+ nfc_put_device(dev);
+ return rc;
+}
+
static int nfc_genl_start_poll(struct sk_buff *skb, struct genl_info *info)
{
struct nfc_dev *dev;
@@ -441,6 +495,16 @@ static struct genl_ops nfc_genl_ops[] = {
.policy = nfc_genl_policy,
},
{
+ .cmd = NFC_CMD_DEV_UP,
+ .doit = nfc_genl_dev_up,
+ .policy = nfc_genl_policy,
+ },
+ {
+ .cmd = NFC_CMD_DEV_DOWN,
+ .doit = nfc_genl_dev_down,
+ .policy = nfc_genl_policy,
+ },
+ {
.cmd = NFC_CMD_START_POLL,
.doit = nfc_genl_start_poll,
.policy = nfc_genl_policy,
diff --git a/net/nfc/nfc.h b/net/nfc/nfc.h
index aaf9832..1a877de 100644
--- a/net/nfc/nfc.h
+++ b/net/nfc/nfc.h
@@ -101,6 +101,10 @@ static inline void nfc_device_iter_exit(struct class_dev_iter *iter)
class_dev_iter_exit(iter);
}
+int nfc_dev_up(struct nfc_dev *dev);
+
+int nfc_dev_down(struct nfc_dev *dev);
+
int nfc_start_poll(struct nfc_dev *dev, u32 protocols);
int nfc_stop_poll(struct nfc_dev *dev);
--
1.7.0.4
HI Ilan,
On Thu, Aug 18, 2011 at 3:43 AM, Elias, Ilan <[email protected]> wrote:
>> What happens if the driver is polling for targets or even with a
>> remote target activated? The nfc_dev structure has some control to
>> track the driver state (e.g. dev->polling) that must have their values
>> updated. Otherwise, the next start_poll() call would fail with EBUSY.
> You're correct, of course.
> When dev_down is called and we have some active operation (e.g. polling,
> active target), there are 2 approaches:
> 1) stop internally any active operations and update internal states
> (e.g. dev->polling). Please note that if we have an active data exchange,
> the callback might not be called at all.
> 2) simply return EBUSY and let the upper layers handle it.
We don't use to interrupt a running command when a new one arrives.
However, if dev_down/dev_up commands are intending to be used also as
a reset procedure, the first approach is more suitable.
Until now we didn't face any situation that required a reset
procedure. So i would go with the second approach.
> With both approaches, we'll have to keep track of active target state
> (in addition to dev->polling). Which option do you prefer?
Yes, that's true. Something like dev->remote_activated will be needed.
Regards,
Aloisio
Hi Ilan,
On Wed, Aug 17, 2011 at 3:33 AM, Elias, Ilan <[email protected]> wrote:
> Hi Aloisio, Lauro, Samuel,
>
>>
>> The following 2 NFC control operations (via generic netlink) were added:
>> ? ? ? - Dev_up, which turn on the NFC controller (this operation may take
>> a ? ? few seconds, as it can download new FW to the NFC controller)
>> ? ? ? - Dev_down, which turn off the NFC controller
>>
> Please note that in the file core.c, the new flag 'dev_up' is not checked in the nfc core API (e.g. in the function nfc_start_poll).
> We should discuss whether the dev_up/dev_down operations will be used in all devices (i.e. also PN533) - this is my preference.
I agree. In order to abstract the NFC device type that is being used,
dev_up must be called before start_poll to all devices. The driver
must be in charge of deciding if something must be done at dev_up
time.
Regards,
Aloisio
Hi Aloisio, Lauro, Samuel,
>
> The following 2 NFC control operations (via generic netlink) were added:
> - Dev_up, which turn on the NFC controller (this operation may take
> a few seconds, as it can download new FW to the NFC controller)
> - Dev_down, which turn off the NFC controller
>
Please note that in the file core.c, the new flag 'dev_up' is not checked in the nfc core API (e.g. in the function nfc_start_poll).
We should discuss whether the dev_up/dev_down operations will be used in all devices (i.e. also PN533) - this is my preference.
If so, I'll add a check for the 'dev_up' flag in the nfc core API.
Otherwise, I'll check the state at a lower layer, i.e. the NCI layer.
Please advise.
Thanks & BR,
Ilan
Hi Aloisio,
Thanks for your comments.
> > +int nfc_dev_down(struct nfc_dev *dev)
> > +{
> > + ? ? ? int rc = 0;
> > +
> > + ? ? ? nfc_dbg("dev_name=%s", dev_name(&dev->dev));
> > +
> > + ? ? ? device_lock(&dev->dev);
> > +
> > + ? ? ? if (!device_is_registered(&dev->dev)) {
> > + ? ? ? ? ? ? ? rc = -ENODEV;
> > + ? ? ? ? ? ? ? goto error;
> > + ? ? ? }
> > +
> > + ? ? ? if (!dev->dev_up) {
> > + ? ? ? ? ? ? ? rc = -EINVAL;
> > + ? ? ? ? ? ? ? goto error;
> > + ? ? ? }
>
> I think -EALREADY would fit better.
Agreed.
> > +
> > + ? ? ? if (dev->ops->dev_down)
> > + ? ? ? ? ? ? ? dev->ops->dev_down(dev);
> > +
> > + ? ? ? dev->dev_up = false;
>
> What happens if the driver is polling for targets or even with a
> remote target activated? The nfc_dev structure has some control to
> track the driver state (e.g. dev->polling) that must have their values
> updated. Otherwise, the next start_poll() call would fail with EBUSY.
You're correct, of course.
When dev_down is called and we have some active operation (e.g. polling,
active target), there are 2 approaches:
1) stop internally any active operations and update internal states
(e.g. dev->polling). Please note that if we have an active data exchange,
the callback might not be called at all.
2) simply return EBUSY and let the upper layers handle it.
With both approaches, we'll have to keep track of active target state
(in addition to dev->polling). Which option do you prefer?
> > +static int nfc_genl_dev_up(struct sk_buff *skb, struct
> genl_info *info)
> > +{
> > + ? ? ? struct nfc_dev *dev;
> > + ? ? ? int rc;
> > + ? ? ? u32 idx;
> > +
> > + ? ? ? nfc_dbg("entry");
> > +
> > + ? ? ? if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
> > + ? ? ? ? ? ? ? return -EINVAL;
> > +
> > + ? ? ? idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
> > +
> > + ? ? ? dev = nfc_get_device(idx);
> > + ? ? ? if (!dev)
> > + ? ? ? ? ? ? ? return -ENODEV;
> > +
> > + ? ? ? mutex_lock(&dev->genl_data.genl_data_mutex);
> > +
> > + ? ? ? rc = nfc_dev_up(dev);
> > +
> > + ? ? ? mutex_unlock(&dev->genl_data.genl_data_mutex);
>
> This mutex is used to protect the variable dev->genl_data.poll_req_pid
> in cases when the netlink socket is closed in parallel with start_poll
> or stop_poll calls. As you are not accessing poll_req_pid, you don't
> need to care about this mutex.
Agreed.
>
> > +static int nfc_genl_dev_down(struct sk_buff *skb, struct
> genl_info *info)
> > +{
> > + ? ? ? struct nfc_dev *dev;
> > + ? ? ? int rc;
> > + ? ? ? u32 idx;
> > +
> > + ? ? ? nfc_dbg("entry");
> > +
> > + ? ? ? if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
> > + ? ? ? ? ? ? ? return -EINVAL;
> > +
> > + ? ? ? idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
> > +
> > + ? ? ? dev = nfc_get_device(idx);
> > + ? ? ? if (!dev)
> > + ? ? ? ? ? ? ? return -ENODEV;
> > +
> > + ? ? ? mutex_lock(&dev->genl_data.genl_data_mutex);
> > +
> > + ? ? ? rc = nfc_dev_down(dev);
> > +
> > + ? ? ? mutex_unlock(&dev->genl_data.genl_data_mutex);
>
> The same as above.
Agreed.
Thanks & BR,
Ilan
Hi Ilan,
On Tue, Aug 16, 2011 at 2:35 PM, Elias, Ilan <[email protected]> wrote:
> +/**
> + * nfc_dev_down - turn off the NFC device
> + *
> + * @dev: The nfc device to be turned off
> + */
> +int nfc_dev_down(struct nfc_dev *dev)
> +{
> + ? ? ? int rc = 0;
> +
> + ? ? ? nfc_dbg("dev_name=%s", dev_name(&dev->dev));
> +
> + ? ? ? device_lock(&dev->dev);
> +
> + ? ? ? if (!device_is_registered(&dev->dev)) {
> + ? ? ? ? ? ? ? rc = -ENODEV;
> + ? ? ? ? ? ? ? goto error;
> + ? ? ? }
> +
> + ? ? ? if (!dev->dev_up) {
> + ? ? ? ? ? ? ? rc = -EINVAL;
> + ? ? ? ? ? ? ? goto error;
> + ? ? ? }
I think -EALREADY would fit better.
> +
> + ? ? ? if (dev->ops->dev_down)
> + ? ? ? ? ? ? ? dev->ops->dev_down(dev);
> +
> + ? ? ? dev->dev_up = false;
What happens if the driver is polling for targets or even with a
remote target activated? The nfc_dev structure has some control to
track the driver state (e.g. dev->polling) that must have their values
updated. Otherwise, the next start_poll() call would fail with EBUSY.
> +static int nfc_genl_dev_up(struct sk_buff *skb, struct genl_info *info)
> +{
> + ? ? ? struct nfc_dev *dev;
> + ? ? ? int rc;
> + ? ? ? u32 idx;
> +
> + ? ? ? nfc_dbg("entry");
> +
> + ? ? ? if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
> +
> + ? ? ? dev = nfc_get_device(idx);
> + ? ? ? if (!dev)
> + ? ? ? ? ? ? ? return -ENODEV;
> +
> + ? ? ? mutex_lock(&dev->genl_data.genl_data_mutex);
> +
> + ? ? ? rc = nfc_dev_up(dev);
> +
> + ? ? ? mutex_unlock(&dev->genl_data.genl_data_mutex);
This mutex is used to protect the variable dev->genl_data.poll_req_pid
in cases when the netlink socket is closed in parallel with start_poll
or stop_poll calls. As you are not accessing poll_req_pid, you don't
need to care about this mutex.
> +static int nfc_genl_dev_down(struct sk_buff *skb, struct genl_info *info)
> +{
> + ? ? ? struct nfc_dev *dev;
> + ? ? ? int rc;
> + ? ? ? u32 idx;
> +
> + ? ? ? nfc_dbg("entry");
> +
> + ? ? ? if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
> + ? ? ? ? ? ? ? return -EINVAL;
> +
> + ? ? ? idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
> +
> + ? ? ? dev = nfc_get_device(idx);
> + ? ? ? if (!dev)
> + ? ? ? ? ? ? ? return -ENODEV;
> +
> + ? ? ? mutex_lock(&dev->genl_data.genl_data_mutex);
> +
> + ? ? ? rc = nfc_dev_down(dev);
> +
> + ? ? ? mutex_unlock(&dev->genl_data.genl_data_mutex);
The same as above.
Regards,
Aloisio