Updated patch after Arnd's comments.
Waldek
Waldemar Rymarkiewicz (1):
NFC: Driver for Inside Secure MicroRead NFC chip
Documentation/nfc/nfc-microread.txt | 46 ++++
drivers/nfc/Kconfig | 10 +
drivers/nfc/Makefile | 1 +
drivers/nfc/microread.c | 465 +++++++++++++++++++++++++++++++++++
include/linux/nfc/microread.h | 31 +++
5 files changed, 553 insertions(+), 0 deletions(-)
create mode 100644 Documentation/nfc/nfc-microread.txt
create mode 100644 drivers/nfc/microread.c
create mode 100644 include/linux/nfc/microread.h
Add new driver for MicroRead NFC chip connected to i2c bus.
See Documentation/nfc/nfc-microread.txt.
Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
---
Documentation/nfc/nfc-microread.txt | 46 ++++
drivers/nfc/Kconfig | 10 +
drivers/nfc/Makefile | 1 +
drivers/nfc/microread.c | 465 +++++++++++++++++++++++++++++++++++
include/linux/nfc/microread.h | 31 +++
5 files changed, 553 insertions(+), 0 deletions(-)
create mode 100644 Documentation/nfc/nfc-microread.txt
create mode 100644 drivers/nfc/microread.c
create mode 100644 include/linux/nfc/microread.h
diff --git a/Documentation/nfc/nfc-microread.txt b/Documentation/nfc/nfc-microread.txt
new file mode 100644
index 0000000..e15c49c
--- /dev/null
+++ b/Documentation/nfc/nfc-microread.txt
@@ -0,0 +1,46 @@
+Kernel driver for Inside Secure MicroRead Near Field Communication chip
+
+General
+-------
+
+microread is the RF chip that uses Near Field Communication (NFC) for proximity
+transactions and interactions.
+
+Please visit Inside Secure webside for microread specification:
+http://www.insidesecure.com/eng/Products/NFC-Products/MicroRead
+
+
+The Driver
+----------
+
+The microread driver can be found under drivers/nfc/microread.c and it's
+compiled as a module named "microread".
+
+The driver supports i2c interface only.
+
+The userspace application can use /dev/microread device to control the chip by
+HCI messages. In a typical scenario application will open() /dev/microread,
+reset the chip useing ioctl() interface then poll() the device to check if
+writing/reaing is possible.Finally, will read/write data (HCI) from/to the chip.
+
+Note that only one application can use the /dev/microread at the time.
+
+The driver waits for microread chip interrupt which occures if there are some
+data to be read. Then, poll() will indicate that fact to the userspace.
+
+The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.
+
+
+Platform Data
+-------------
+
+The below platform data should be set when configuring board.
+
+struct microread_nfc_platform_data {
+ unsigned int rst_gpio;
+ unsigned int irq_gpio;
+ unsigned int ioh_gpio;
+ int (*request_hardware) (struct i2c_client *client);
+ void (*release_hardware) (void);
+};
+
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
index ea15800..a805615 100644
--- a/drivers/nfc/Kconfig
+++ b/drivers/nfc/Kconfig
@@ -26,5 +26,15 @@ config PN544_NFC
To compile this driver as a module, choose m here. The module will
be called pn544.
+config MICROREAD_NFC
+ tristate "MICROREAD NFC driver"
+ depends on I2C
+ default n
+ ---help---
+ Say yes if you want Inside Secure Microread Near Field Communication
+ driver. This is for i2c connected version. If unsure, say N here.
+
+ To compile this driver as a module, choose m here. The module will
+ be called microread.
endif # NFC_DEVICES
diff --git a/drivers/nfc/Makefile b/drivers/nfc/Makefile
index a4efb16..974f5cb 100644
--- a/drivers/nfc/Makefile
+++ b/drivers/nfc/Makefile
@@ -3,3 +3,4 @@
#
obj-$(CONFIG_PN544_NFC) += pn544.o
+obj-$(CONFIG_MICROREAD_NFC) += microread.o
diff --git a/drivers/nfc/microread.c b/drivers/nfc/microread.c
new file mode 100644
index 0000000..bcbdbad
--- /dev/null
+++ b/drivers/nfc/microread.c
@@ -0,0 +1,465 @@
+/*
+ * Driver for Microread NFC chip
+ *
+ * Copyright (C) 2011 Tieto Poland
+ *
+ * Author: Waldemar Rymarkiewicz <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/poll.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/nfc/microread.h>
+#include <linux/uaccess.h>
+#include <linux/pm.h>
+
+#define MICROREAD_DEV_NAME "microread"
+#define MICROREAD_MAX_BUF_SIZE 0x20
+
+#define MICROREAD_STATE_BUSY 0x00
+#define MICROREAD_STATE_READY 0x01
+
+struct microread_info {
+ struct i2c_client *i2c_dev;
+ struct miscdevice mdev;
+
+ u8 state;
+ u8 irq_state;
+ int irq;
+ u16 buflen;
+ u8 *buf;
+ wait_queue_head_t rx_waitq;
+ struct mutex rx_mutex;
+ struct mutex mutex;
+};
+
+static void microread_reset_hw(struct microread_nfc_platform_data *pdata)
+{
+ gpio_set_value(pdata->rst_gpio, 1);
+ usleep_range(1000, 2000);
+ gpio_set_value(pdata->rst_gpio, 0);
+ usleep_range(5000, 10000);
+}
+
+static u8 calc_lrc(u8 *buf, int len)
+{
+ int i;
+ u8 lrc = 0;
+ for (i = 0; i < len; i++)
+ lrc = lrc ^ buf[i];
+ return lrc;
+}
+
+static inline int microread_is_busy(struct microread_info *info)
+{
+ return (info->state == MICROREAD_STATE_BUSY);
+}
+
+static int microread_open(struct inode *inode, struct file *file)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+ int ret = 0;
+
+ dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
+
+ mutex_lock(&info->mutex);
+
+ if (microread_is_busy(info)) {
+ dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
+ info);
+ ret = -EBUSY;
+ goto done;
+ }
+
+ info->state = MICROREAD_STATE_BUSY;
+done:
+ mutex_unlock(&info->mutex);
+ return ret;
+}
+
+static int microread_release(struct inode *inode, struct file *file)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+
+ dev_vdbg(&client->dev, "%s: info: %p, client %p\n", __func__, info,
+ client);
+
+ mutex_lock(&info->mutex);
+ info->state = MICROREAD_STATE_READY;
+ mutex_unlock(&info->mutex);
+
+ return 0;
+}
+
+static ssize_t microread_read(struct file *file, char __user *buf, size_t count,
+ loff_t *f_pos)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+ struct microread_nfc_platform_data *pdata =
+ dev_get_platdata(&client->dev);
+ int ret;
+ u8 len, lrc;
+
+ dev_dbg(&client->dev, "%s: info: %p, size: %d (bytes).", __func__,
+ info, count);
+ mutex_lock(&info->mutex);
+
+ if (!info->irq_state && !gpio_get_value(pdata->irq_gpio)) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ goto done;
+ }
+
+ if (wait_event_interruptible(info->rx_waitq,
+ (info->irq_state == 1))) {
+ ret = -EINTR;
+ goto done;
+ }
+ }
+
+ if (count > info->buflen) {
+ dev_err(&client->dev, "%s: no enough space in read buffer.",
+ __func__);
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ mutex_lock(&info->rx_mutex);
+ ret = i2c_master_recv(client, info->buf, info->buflen);
+ info->irq_state = 0;
+ mutex_unlock(&info->rx_mutex);
+
+ if (ret < 0) {
+ dev_err(&client->dev, "%s: i2c read (data) failed (err %d).",
+ __func__, ret);
+ ret = -EREMOTEIO;
+ goto done;
+ }
+
+#ifdef VERBOSE_DEBUG
+ print_hex_dump(KERN_DEBUG, "rx: ", DUMP_PREFIX_NONE,
+ 16, 1, info->buf, ret, false);
+#endif
+ len = info->buf[0];
+
+ lrc = calc_lrc(info->buf, len + 1);
+ if (lrc != info->buf[len + 1]) {
+ dev_err(&client->dev, "%s: incorrect i2c frame.", __func__);
+ ret = -EFAULT;
+ goto done;
+ }
+
+ ret = len + 2;
+
+ if (copy_to_user(buf, info->buf, len + 2)) {
+ dev_err(&client->dev, "%s: error copying to user.", __func__);
+ ret = -EFAULT;
+ goto done;
+ }
+done:
+ mutex_unlock(&info->mutex);
+
+ return ret;
+}
+
+static ssize_t microread_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *f_pos)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+ int ret;
+ u16 len;
+
+ dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", __func__,
+ info, count);
+
+ if (count > info->buflen) {
+ dev_err(&client->dev, "%s: no enought space in TX buffer.",
+ __func__);
+ return -EINVAL;
+ }
+
+ len = min_t(u16, count, info->buflen);
+
+ mutex_lock(&info->mutex);
+ if (copy_from_user(info->buf, buf, len)) {
+ dev_err(&client->dev, "%s: error copying from user.",
+ __func__);
+ ret = -EFAULT;
+ goto done;
+ }
+
+#ifdef VERBOSE_DEBUG
+ print_hex_dump(KERN_DEBUG, "tx: ", DUMP_PREFIX_NONE, 16, 1, info->buf,
+ len, false);
+#endif
+ ret = i2c_master_send(client, info->buf, len);
+ if (ret < 0)
+ dev_err(&client->dev, "%s: i2c write failed (err %d).",
+ __func__, ret);
+
+done:
+ mutex_unlock(&info->mutex);
+ return ret;
+
+}
+
+static unsigned int microread_poll(struct file *file, poll_table *wait)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+ int ret = (POLLOUT | POLLWRNORM);
+
+ dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
+ client);
+
+ mutex_lock(&info->mutex);
+ poll_wait(file, &info->rx_waitq, wait);
+
+ mutex_lock(&info->rx_mutex);
+ if (info->irq_state)
+ ret |= (POLLIN | POLLRDNORM);
+ mutex_unlock(&info->rx_mutex);
+ mutex_unlock(&info->mutex);
+
+ return ret;
+}
+
+static long microread_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct microread_info *info = container_of(file->private_data,
+ struct microread_info, mdev);
+ struct i2c_client *client = info->i2c_dev;
+ struct microread_nfc_platform_data *pdata =
+ dev_get_platdata(&client->dev);
+ int ret = 0;
+
+ dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, info, cmd);
+
+ mutex_lock(&info->mutex);
+
+ switch (cmd) {
+ case MICROREAD_IOC_CONFIGURE:
+ case MICROREAD_IOC_CONNECT:
+ goto done;
+
+ case MICROREAD_IOC_RESET:
+ microread_reset_hw(pdata);
+ goto done;
+
+ default:
+ dev_err(&client->dev, "%s; not supported ioctl 0x%x", __func__,
+ cmd);
+ ret = -ENOIOCTLCMD;
+ goto done;
+ }
+
+done:
+ mutex_unlock(&info->mutex);
+ return ret;
+}
+
+const struct file_operations microread_fops = {
+ .owner = THIS_MODULE,
+ .open = microread_open,
+ .release = microread_release,
+ .read = microread_read,
+ .write = microread_write,
+ .poll = microread_poll,
+ .unlocked_ioctl = microread_ioctl,
+};
+
+static irqreturn_t microread_irq(int irq, void *dev)
+{
+ struct microread_info *info = dev;
+ struct i2c_client *client = info->i2c_dev;
+
+ dev_dbg(&client->dev, "irq: info %p client %p ", info, client);
+
+ if (irq != client->irq)
+ return IRQ_NONE;
+
+ mutex_lock(&info->rx_mutex);
+ info->irq_state = 1;
+ mutex_unlock(&info->rx_mutex);
+
+ wake_up_interruptible(&info->rx_waitq);
+
+ return IRQ_HANDLED;
+}
+
+static int __devinit microread_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct microread_info *info;
+ struct microread_nfc_platform_data *pdata =
+ dev_get_platdata(&client->dev);
+ int ret = 0;
+
+ dev_dbg(&client->dev, "%s: client %p", __func__, client);
+
+ if (!pdata) {
+ dev_err(&client->dev, "%s: client %p: missing platform data.",
+ __func__, client);
+ return -EINVAL;
+ }
+
+ info = kzalloc(sizeof(struct microread_info), GFP_KERNEL);
+ if (!info) {
+ dev_err(&client->dev, "%s: can't allocate microread_info.",
+ __func__);
+ return -ENOMEM;
+ }
+
+ info->buflen = (u16) MICROREAD_MAX_BUF_SIZE;
+ info->buf = kzalloc(info->buflen, GFP_KERNEL);
+ if (!info->buf) {
+ dev_err(&client->dev, "%s: can't allocate buf. (len %d)",
+ __func__, info->buflen);
+ ret = -ENOMEM;
+ goto free_info;
+ }
+
+ mutex_init(&info->mutex);
+ mutex_init(&info->rx_mutex);
+ init_waitqueue_head(&info->rx_waitq);
+ i2c_set_clientdata(client, info);
+ info->i2c_dev = client;
+ info->irq_state = 0;
+ info->state = MICROREAD_STATE_READY;
+
+ ret = request_threaded_irq(client->irq, NULL, microread_irq,
+ IRQF_SHARED|IRQF_TRIGGER_RISING, MICROREAD_DRIVER_NAME, info);
+ if (ret) {
+ dev_err(&client->dev, "%s: can't request irq. (ret %d, irq %d)",
+ __func__, ret, client->irq);
+ goto free_buf;
+ }
+
+ info->mdev.minor = MISC_DYNAMIC_MINOR;
+ info->mdev.name = MICROREAD_DEV_NAME;
+ info->mdev.fops = µread_fops;
+ info->mdev.parent = &client->dev;
+
+ ret = misc_register(&info->mdev);
+ if (ret < 0) {
+ dev_err(&client->dev, "%s: register chr dev failed (ret %d)",
+ __func__, ret);
+ goto free_irq;
+ }
+
+ dev_info(&client->dev, "Probed.[/dev/%s]", MICROREAD_DEV_NAME);
+ return 0;
+
+free_irq:
+ free_irq(client->irq, info);
+free_buf:
+ kfree(info->buf);
+free_info:
+ kfree(info);
+
+ dev_info(&client->dev, "Not probed.");
+ return ret;
+}
+
+static __devexit int microread_remove(struct i2c_client *client)
+{
+ struct microread_info *info = i2c_get_clientdata(client);
+
+ dev_dbg(&client->dev, "%s", __func__);
+
+ misc_deregister(&info->mdev);
+ free_irq(client->irq, info);
+ kfree(info->buf);
+ kfree(info);
+
+ dev_info(&client->dev, "%s: Removed.", __func__);
+ return 0;
+}
+
+static int microread_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+ return -ENOSYS;
+}
+
+static int microread_resume(struct i2c_client *client)
+{
+ return -ENOSYS;
+}
+
+static struct i2c_device_id microread_id[] = {
+ { MICROREAD_DRIVER_NAME, 0},
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, microread_id);
+
+static struct i2c_driver microread_driver = {
+ .driver = {
+ .name = MICROREAD_DRIVER_NAME,
+ },
+ .probe = microread_probe,
+ .remove = __devexit_p(microread_remove),
+ .suspend = microread_suspend,
+ .resume = microread_resume,
+ .id_table = microread_id,
+};
+
+static int __init microread_init(void)
+{
+ int ret;
+
+ pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+
+ ret = i2c_add_driver(µread_driver);
+ if (ret) {
+ pr_err(MICROREAD_DRIVER_NAME ": driver registration failed");
+ return ret;
+ }
+ pr_info(MICROREAD_DRIVER_NAME ": Loaded.");
+ return 0;
+}
+
+static void __exit microread_exit(void)
+{
+ pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+ i2c_del_driver(µread_driver);
+ pr_info(MICROREAD_DRIVER_NAME ": Unloaded");
+}
+
+module_init(microread_init);
+module_exit(microread_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Waldemar Rymarkiewicz <[email protected]>");
+MODULE_DESCRIPTION("Driver for Microread NFC chip");
diff --git a/include/linux/nfc/microread.h b/include/linux/nfc/microread.h
new file mode 100644
index 0000000..a0ad68e
--- /dev/null
+++ b/include/linux/nfc/microread.h
@@ -0,0 +1,31 @@
+#ifndef _MICROREAD_H
+#define _MICROREAD_H
+
+#include <linux/i2c.h>
+
+#define MICROREAD_DRIVER_NAME "microread"
+
+#define MICROREAD_IOC_MAGIC 'o'
+#define MICROREAD_IOC_MAX_CONFIG_LENGTH 16
+
+struct microread_ioc_configure {
+ int length;
+ unsigned char buffer[MICROREAD_IOC_MAX_CONFIG_LENGTH];
+};
+
+/* ioctl cmds */
+#define MICROREAD_IOC_CONFIGURE _IOW(MICROREAD_IOC_MAGIC, 0, \
+ struct microread_ioc_configure)
+#define MICROREAD_IOC_CONNECT _IO(MICROREAD_IOC_MAGIC, 1)
+#define MICROREAD_IOC_RESET _IO(MICROREAD_IOC_MAGIC, 2)
+
+#ifdef __KERNEL__
+/* board config platform data for microread */
+struct microread_nfc_platform_data {
+ unsigned int rst_gpio;
+ unsigned int irq_gpio;
+ unsigned int ioh_gpio;
+};
+#endif /* __KERNEL__ */
+
+#endif /* _MICROREAD_H */
--
1.7.1
On Fri, 18 Mar 2011 11:40:24 +0100
Waldemar Rymarkiewicz <[email protected]> wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
Ok we now have two devices and they have differing APIs and own device
names and both from the same company. This has to stop right now and the
existing one wants sorting out accordingly (while you are it fix the fact
any user can blow the kernel log away in that one by issuing bogus ioctls
at it, thats not a good thing)
NAK to this in its current form.
If we are going to have multiple nfc devices (and we are) then we need
a /dev/nfc%d device range to dump them in and we need some API
commonality.
Your API seems fairly sane (except your nfc-microread.txt needs to
document or point properly to the HCI messages in question
> +The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.
And a reset is a generic sort of interface so we should probably have
NFC_IOC_RESET to go with /dev/nfc%d naming.
> +static int microread_open(struct inode *inode, struct file *file)
> +{
> + struct microread_info *info = container_of(file->private_data,
> + struct microread_info, mdev);
> + struct i2c_client *client = info->i2c_dev;
> + int ret = 0;
> +
> + dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
> +
> + mutex_lock(&info->mutex);
> +
> + if (microread_is_busy(info)) {
> + dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
> + info);
So a process spinning trying to open it can spew all over the log - looks
bogus to me (similar problems in the existing driver)
> + if (count > info->buflen) {
> + dev_err(&client->dev, "%s: no enough space in read buffer.",
> + __func__);
> + ret = -ENOMEM;
> + goto done;
More bogus log spewing and an odd error code for good measure
> + lrc = calc_lrc(info->buf, len + 1);
> + if (lrc != info->buf[len + 1]) {
> + dev_err(&client->dev, "%s: incorrect i2c frame.", __func__);
> + ret = -EFAULT;
> + goto done;
So I can also spew all over the log by putting a deliberately busted
sender next to it.
> + }
> +
> + ret = len + 2;
> +
> + if (copy_to_user(buf, info->buf, len + 2)) {
> + dev_err(&client->dev, "%s: error copying to user.", __func__);
> + ret = -EFAULT;
And another one.
> +static ssize_t microread_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *f_pos)
> +{
> + struct microread_info *info = container_of(file->private_data,
> + struct microread_info, mdev);
> + struct i2c_client *client = info->i2c_dev;
> + int ret;
> + u16 len;
> +
> + dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", __func__,
> + info, count);
> +
> + if (count > info->buflen) {
> + dev_err(&client->dev, "%s: no enought space in TX buffer.",
> + __func__);
> + return -EINVAL;
> + }
And another
> +
> + len = min_t(u16, count, info->buflen);
> +
> + mutex_lock(&info->mutex);
> + if (copy_from_user(info->buf, buf, len)) {
> + dev_err(&client->dev, "%s: error copying from user.",
> + __func__);
Etc - these all want cleaning up
> +static unsigned int microread_poll(struct file *file, poll_table *wait)
> +{
> + struct microread_info *info = container_of(file->private_data,
> + struct microread_info, mdev);
> + struct i2c_client *client = info->i2c_dev;
> + int ret = (POLLOUT | POLLWRNORM);
> +
> + dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
> + client);
> +
> + mutex_lock(&info->mutex);
> + poll_wait(file, &info->rx_waitq, wait);
> +
> + mutex_lock(&info->rx_mutex);
> + if (info->irq_state)
> + ret |= (POLLIN | POLLRDNORM);
> + mutex_unlock(&info->rx_mutex);
> + mutex_unlock(&info->mutex);
What is the intended behaviour on a reset while I am polling ?
> +static long microread_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct microread_info *info = container_of(file->private_data,
> + struct microread_info, mdev);
> + struct i2c_client *client = info->i2c_dev;
> + struct microread_nfc_platform_data *pdata =
> + dev_get_platdata(&client->dev);
> + int ret = 0;
> +
> + dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, info, cmd);
> +
> + mutex_lock(&info->mutex);
> +
> + switch (cmd) {
> + case MICROREAD_IOC_CONFIGURE:
> + case MICROREAD_IOC_CONNECT:
> + goto done;
Ermm nope.. why do we have do nothing ioctls ?
> + case MICROREAD_IOC_RESET:
> + microread_reset_hw(pdata);
> + default:
> + dev_err(&client->dev, "%s; not supported ioctl 0x%x", __func__,
> + cmd);
And more spewage
> +static irqreturn_t microread_irq(int irq, void *dev)
> +{
> + struct microread_info *info = dev;
> + struct i2c_client *client = info->i2c_dev;
> +
> + dev_dbg(&client->dev, "irq: info %p client %p ", info, client);
> +
> + if (irq != client->irq)
> + return IRQ_NONE;
How can this occur - why is this test needed ????
> +
> + mutex_lock(&info->rx_mutex);
> + info->irq_state = 1;
> + mutex_unlock(&info->rx_mutex);
Would it not be lighter to use atomic bit ops ?
> + info->mdev.minor = MISC_DYNAMIC_MINOR;
> + info->mdev.name = MICROREAD_DEV_NAME;
> + info->mdev.fops = µread_fops;
> + info->mdev.parent = &client->dev;
> +
> + ret = misc_register(&info->mdev);
> + if (ret < 0) {
> + dev_err(&client->dev, "%s: register chr dev failed (ret %d)",
> + __func__, ret);
> + goto free_irq;
> + }
And at this point you want a thing to hand out nfc%d names not to use
misc device with random per device API. The same app ought to be able to
work with many nfc readers.
> +static int microread_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> + return -ENOSYS;
> +}
> +
> +static int microread_resume(struct i2c_client *client)
> +{
> + return -ENOSYS;
> +}
So why provide them ??
Alan
Hi Waldemar,
On Fri, Mar 18, 2011 at 11:40:24AM +0100, Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
>
> See Documentation/nfc/nfc-microread.txt.
>
> Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
Besides the stuff Alan mentioned...
> +struct microread_nfc_platform_data {
> + unsigned int rst_gpio;
> + unsigned int irq_gpio;
> + unsigned int ioh_gpio;
... you should request and setup the GPIOs before using them.
> +free_irq:
> + free_irq(client->irq, info);
> +free_buf:
> + kfree(info->buf);
> +free_info:
> + kfree(info);
> +
> + dev_info(&client->dev, "Not probed.");
> + return ret;
> +}
When respinning, you could consider using managed devices (devm_*);
this error path could completely go then.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
>
> See Documentation/nfc/nfc-microread.txt.
As I said in my first review and Alan also pointed out now, the
most important change will be to add a common NFC core layer,
before adding more hardware drivers.
Also, regarding the user interface, we need to be really sure
that this is the best way of talking to NFC devices. The interface
you have today is a simple character device read/write kind,
which may be the best thing if the protocol stack on top is
really simple and there is never the need to have multiple
applications talking to different endpoints on the wireless
interface, and if there are no protocol headers being
send over the character device interface.
Otherwise, a better interface is probably to add a new network
socket family and abstract the protocol layers in the kernel.
Can you explain in more depth what the kind of data is on
this interface?
A few more things I noticed when reading through the driver
again:
> +const struct file_operations microread_fops = {
> + .owner = THIS_MODULE,
> + .open = microread_open,
> + .release = microread_release,
> + .read = microread_read,
> + .write = microread_write,
> + .poll = microread_poll,
> + .unlocked_ioctl = microread_ioctl,
> +};
As I said, all the file operations need to move to the core module.
Also, this is missing a compat_ioctl function.
> +static irqreturn_t microread_irq(int irq, void *dev)
> +{
> + struct microread_info *info = dev;
> + struct i2c_client *client = info->i2c_dev;
> +
> + dev_dbg(&client->dev, "irq: info %p client %p ", info, client);
> +
> + if (irq != client->irq)
> + return IRQ_NONE;
> +
> + mutex_lock(&info->rx_mutex);
> + info->irq_state = 1;
> + mutex_unlock(&info->rx_mutex);
> +
> + wake_up_interruptible(&info->rx_waitq);
> +
> + return IRQ_HANDLED;
> +}
You cannot take a mutex from interrupt context, that may
cause deadlocks.
> diff --git a/include/linux/nfc/microread.h b/include/linux/nfc/microread.h
> new file mode 100644
> index 0000000..a0ad68e
> --- /dev/null
> +++ b/include/linux/nfc/microread.h
Please split this header file into an include/linux/nfc.h file
and the file with the platform data in include/linux/platform_data/microread.h
Do not define any ioctls that are not used.
Arnd
On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> > +
> > + mutex_lock(&info->rx_mutex);
> > + info->irq_state = 1;
> > + mutex_unlock(&info->rx_mutex);
> > +
> > + wake_up_interruptible(&info->rx_waitq);
> > +
> > + return IRQ_HANDLED;
> > +}
> You cannot take a mutex from interrupt context, that may
> cause deadlocks.
This is a threaded IRQ handler so mutexes are fine.
On Friday 18 March 2011, Mark Brown wrote:
> On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> > On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
>
> > > +
> > > + mutex_lock(&info->rx_mutex);
> > > + info->irq_state = 1;
> > > + mutex_unlock(&info->rx_mutex);
> > > +
> > > + wake_up_interruptible(&info->rx_waitq);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
>
> > You cannot take a mutex from interrupt context, that may
> > cause deadlocks.
>
> This is a threaded IRQ handler so mutexes are fine.
Ah, right. I've never seen one of these used in the field,
so I didn't think of this.
Looking at the mutexes though: The read function does
not hold the rx_mutex when reading the irq_state
variable, so that is potentially racy.
The read function seems to have another problem regarding
the user space buffer: it bails out if the provided buffer
is larger than the available data, which is pointless,
but it does not check if the user buffer is too short.
Arnd
Alan,
>Ok we now have two devices and they have differing APIs and
>own device names and both from the same company. This has to
>stop right now and the existing one wants sorting out
>accordingly (while you are it fix the fact any user can blow
>the kernel log away in that one by issuing bogus ioctls at it,
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then
>we need a /dev/nfc%d device range to dump them in and we need
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset
>the hardware.
>
>And a reset is a generic sort of interface so we should
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.
I see your point and agree with your and Arnd's opinion on that concept. Will, then, think how to approach to this and try to propose a resonable skeleton first.
>> + if (microread_is_busy(info)) {
>> + dev_err(&client->dev, "%s: info %p: device is
>busy.", __func__,
>> + info);
>
>So a process spinning trying to open it can spew all over the
>log - looks bogus to me (similar problems in the existing driver)
All potential spewing logs have been removed.
>
>What is the intended behaviour on a reset while I am polling ?
Good question :|. I will answare soon.
>
>Ermm nope.. why do we have do nothing ioctls ?
>
onfc stack requires those ones, but they are only valid for a specific test enviroment.
This should not be a case for driver and the stack should care about it if it needs this. Then will remove it.
>> + if (irq != client->irq)
>> + return IRQ_NONE;
>
>How can this occur - why is this test needed ????
It's not needed. Removed.
>> +
>> + mutex_lock(&info->rx_mutex);
>> + info->irq_state = 1;
>> + mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?
Do you mean in order to remove rx_mutex?
mutex_lock(&info->rx_mutex);
atomic_set(info->irq_state ,1);
mutex_unlock(&info->rx_mutex);
looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
mutex_lock(&info->rx_mutex);
ret = i2c_master_recv(client, info->buf, info->buflen);
info->irq_state = 0;
mutex_unlock(&info->rx_mutex);
>> + info->mdev.minor = MISC_DYNAMIC_MINOR;
>> + info->mdev.name = MICROREAD_DEV_NAME;
>> + info->mdev.fops = µread_fops;
>> + info->mdev.parent = &client->dev;
>> +
>> + ret = misc_register(&info->mdev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "%s: register chr dev
>failed (ret %d)",
>> + __func__, ret);
>> + goto free_irq;
>> + }
>
>And at this point you want a thing to hand out nfc%d names not
>to use misc device with random per device API. The same app
>ought to be able to work with many nfc readers.
Sure. Will fix this as well.
>> +static int microread_suspend(struct i2c_client *client,
>pm_message_t
>> +mesg) {
>> + return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> + return -ENOSYS;
>> +}
>
>So why provide them ??
I've supposed to implement later on (need more hw specific input on that topic). Anyway...
I can add this when it's completed at all. Removed for now.
>-----Original Message-----
>From: Alan Cox [mailto:[email protected]]
>Sent: Friday, March 18, 2011 12:04 PM
>To: Rymarkiewicz Waldemar
>Cc: [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]
>Subject: Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
>
>On Fri, 18 Mar 2011 11:40:24 +0100
>Waldemar Rymarkiewicz <[email protected]> wrote:
>
>> Add new driver for MicroRead NFC chip connected to i2c bus.
>
>Ok we now have two devices and they have differing APIs and
>own device names and both from the same company. This has to
>stop right now and the existing one wants sorting out
>accordingly (while you are it fix the fact any user can blow
>the kernel log away in that one by issuing bogus ioctls at it,
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then
>we need a /dev/nfc%d device range to dump them in and we need
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset
>the hardware.
>
>And a reset is a generic sort of interface so we should
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.
>
>> +static int microread_open(struct inode *inode, struct file *file) {
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret = 0;
>> +
>> + dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
>> +
>> + mutex_lock(&info->mutex);
>> +
>> + if (microread_is_busy(info)) {
>> + dev_err(&client->dev, "%s: info %p: device is
>busy.", __func__,
>> + info);
>
>So a process spinning trying to open it can spew all over the
>log - looks bogus to me (similar problems in the existing driver)
>
>
>
>> + if (count > info->buflen) {
>> + dev_err(&client->dev, "%s: no enough space in
>read buffer.",
>> + __func__);
>> + ret = -ENOMEM;
>> + goto done;
>
>More bogus log spewing and an odd error code for good measure
>
>> + lrc = calc_lrc(info->buf, len + 1);
>> + if (lrc != info->buf[len + 1]) {
>> + dev_err(&client->dev, "%s: incorrect i2c
>frame.", __func__);
>> + ret = -EFAULT;
>> + goto done;
>
>So I can also spew all over the log by putting a deliberately
>busted sender next to it.
>
>> + }
>> +
>> + ret = len + 2;
>> +
>> + if (copy_to_user(buf, info->buf, len + 2)) {
>> + dev_err(&client->dev, "%s: error copying to
>user.", __func__);
>> + ret = -EFAULT;
>
>And another one.
>
>> +static ssize_t microread_write(struct file *file, const
>char __user *buf,
>> + size_t count, loff_t *f_pos)
>> +{
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret;
>> + u16 len;
>> +
>> + dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).",
>__func__,
>> + info, count);
>> +
>> + if (count > info->buflen) {
>> + dev_err(&client->dev, "%s: no enought space in
>TX buffer.",
>> + __func__);
>> + return -EINVAL;
>> + }
>
>And another
>
>> +
>> + len = min_t(u16, count, info->buflen);
>> +
>> + mutex_lock(&info->mutex);
>> + if (copy_from_user(info->buf, buf, len)) {
>> + dev_err(&client->dev, "%s: error copying from user.",
>> + __func__);
>
>Etc - these all want cleaning up
>
>
>> +static unsigned int microread_poll(struct file *file, poll_table
>> +*wait) {
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret = (POLLOUT | POLLWRNORM);
>> +
>> + dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
>> + client);
>> +
>> + mutex_lock(&info->mutex);
>> + poll_wait(file, &info->rx_waitq, wait);
>> +
>> + mutex_lock(&info->rx_mutex);
>> + if (info->irq_state)
>> + ret |= (POLLIN | POLLRDNORM);
>> + mutex_unlock(&info->rx_mutex);
>> + mutex_unlock(&info->mutex);
>
>What is the intended behaviour on a reset while I am polling ?
>
>> +static long microread_ioctl(struct file *file, unsigned int cmd,
>> +
>unsigned long arg)
>> +{
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + struct microread_nfc_platform_data *pdata =
>> + dev_get_platdata(&client->dev);
>> + int ret = 0;
>> +
>> + dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__,
>info, cmd);
>> +
>> + mutex_lock(&info->mutex);
>> +
>> + switch (cmd) {
>> + case MICROREAD_IOC_CONFIGURE:
>> + case MICROREAD_IOC_CONNECT:
>> + goto done;
>
>Ermm nope.. why do we have do nothing ioctls ?
>
>> + case MICROREAD_IOC_RESET:
>> + microread_reset_hw(pdata);
>
>> + default:
>> + dev_err(&client->dev, "%s; not supported ioctl
>0x%x", __func__,
>> + cmd);
>
>And more spewage
>
>
>> +static irqreturn_t microread_irq(int irq, void *dev) {
>> + struct microread_info *info = dev;
>> + struct i2c_client *client = info->i2c_dev;
>> +
>> + dev_dbg(&client->dev, "irq: info %p client %p ", info, client);
>> +
>> + if (irq != client->irq)
>> + return IRQ_NONE;
>
>How can this occur - why is this test needed ????
>
>> +
>> + mutex_lock(&info->rx_mutex);
>> + info->irq_state = 1;
>> + mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?
>
>
>> + info->mdev.minor = MISC_DYNAMIC_MINOR;
>> + info->mdev.name = MICROREAD_DEV_NAME;
>> + info->mdev.fops = µread_fops;
>> + info->mdev.parent = &client->dev;
>> +
>> + ret = misc_register(&info->mdev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "%s: register chr dev
>failed (ret %d)",
>> + __func__, ret);
>> + goto free_irq;
>> + }
>
>And at this point you want a thing to hand out nfc%d names not
>to use misc device with random per device API. The same app
>ought to be able to work with many nfc readers.
>
>> +static int microread_suspend(struct i2c_client *client,
>pm_message_t
>> +mesg) {
>> + return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> + return -ENOSYS;
>> +}
>
>So why provide them ??
>
>
>Alan
>-
On Friday 18 March 2011, [email protected] wrote:
> >
> >Ermm nope.. why do we have do nothing ioctls ?
> >
>
> onfc stack requires those ones, but they are only valid for a specific test enviroment.
> This should not be a case for driver and the stack should care about it if it needs
> this. Then will remove it.
The way this normally works is to figure out the correct
way to implement the kernel driver first and then write
a user space stack around it, not the other way round...
> >> +
> >> + mutex_lock(&info->rx_mutex);
> >> + info->irq_state = 1;
> >> + mutex_unlock(&info->rx_mutex);
> >
> >Would it not be lighter to use atomic bit ops ?
>
> Do you mean in order to remove rx_mutex?
>
> mutex_lock(&info->rx_mutex);
> atomic_set(info->irq_state ,1);
> mutex_unlock(&info->rx_mutex);
>
> looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
>
> mutex_lock(&info->rx_mutex);
> ret = i2c_master_recv(client, info->buf, info->buflen);
> info->irq_state = 0;
> mutex_unlock(&info->rx_mutex);
What Alan meant was set_bit/test_and_clear_bit here, not atomic_*().
As I mentioned in the other mail, the way you check the flag is
probably still racy with the current mutex use.
Note that the interrupt handler doesn't do much at all, so it's probably
a good idea to use a regular (non-threaded) handler here to reduce
the overhead, once you have removed the mutex.
Arnd
Wolfram,
>> + unsigned int irq_gpio;
>> + unsigned int ioh_gpio;
>
>... you should request and setup the GPIOs before using them.
I assume the board specific code will do so.
Waldek-
> >Would it not be lighter to use atomic bit ops ?
>
> Do you mean in order to remove rx_mutex?
>
> mutex_lock(&info->rx_mutex);
> atomic_set(info->irq_state ,1);
> mutex_unlock(&info->rx_mutex);
>
> looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
>
> mutex_lock(&info->rx_mutex);
> ret = i2c_master_recv(client, info->buf, info->buflen);
> info->irq_state = 0;
> mutex_unlock(&info->rx_mutex);
>
I was thinking clear_bit/test_and_set_bit rather than atomic_t operations.
ie in the IRQ
clear_bit(0, &info->irq_state);
in the main path
if (test_and_set_bit(0, &info->state))
i2c_master_recv(...)
but if the mutex is needed anyway it doesn't help make the code saner.
>I was thinking clear_bit/test_and_set_bit rather than atomic_t
>operations.
>
I see, then I will for it.
Waldek-
On Fri, Mar 18, 2011 at 05:08:39PM +0200, [email protected] wrote:
> Wolfram,
> >> + unsigned int irq_gpio;
> >> + unsigned int ioh_gpio;
> >... you should request and setup the GPIOs before using them.
> I assume the board specific code will do so.
The usual pattern is that the code which uses the GPIO requests it,
otherwise all the users just end up having to replicate the setup and
teardown code for them.
On Fri, 18 Mar 2011 11:40:24 +0100 Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
>
> See Documentation/nfc/nfc-microread.txt.
>
> Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
> ---
> Documentation/nfc/nfc-microread.txt | 46 ++++
> drivers/nfc/Kconfig | 10 +
> drivers/nfc/Makefile | 1 +
> drivers/nfc/microread.c | 465 +++++++++++++++++++++++++++++++++++
> include/linux/nfc/microread.h | 31 +++
> 5 files changed, 553 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/nfc/nfc-microread.txt
> create mode 100644 drivers/nfc/microread.c
> create mode 100644 include/linux/nfc/microread.h
>
> diff --git a/Documentation/nfc/nfc-microread.txt b/Documentation/nfc/nfc-microread.txt
> new file mode 100644
> index 0000000..e15c49c
> --- /dev/null
> +++ b/Documentation/nfc/nfc-microread.txt
> @@ -0,0 +1,46 @@
> +Kernel driver for Inside Secure MicroRead Near Field Communication chip
> +
> +General
> +-------
> +
> +microread is the RF chip that uses Near Field Communication (NFC) for proximity
> +transactions and interactions.
> +
> +Please visit Inside Secure webside for microread specification:
website (or web site)
> +http://www.insidesecure.com/eng/Products/NFC-Products/MicroRead
> +
> +
> +The Driver
> +----------
> +
> +The microread driver can be found under drivers/nfc/microread.c and it's
> +compiled as a module named "microread".
> +
> +The driver supports i2c interface only.
> +
> +The userspace application can use /dev/microread device to control the chip by
> +HCI messages. In a typical scenario application will open() /dev/microread,
> +reset the chip useing ioctl() interface then poll() the device to check if
using
> +writing/reaing is possible.Finally, will read/write data (HCI) from/to the chip.
reading is possible. Finally, it will
> +
> +Note that only one application can use the /dev/microread at the time.
> +
> +The driver waits for microread chip interrupt which occures if there are some
occurs
> +data to be read. Then, poll() will indicate that fact to the userspace.
> +
> +The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.
_RESET) to reset
> +
> +
> +Platform Data
> +-------------
> +
> +The below platform data should be set when configuring board.
> +
> +struct microread_nfc_platform_data {
> + unsigned int rst_gpio;
> + unsigned int irq_gpio;
> + unsigned int ioh_gpio;
> + int (*request_hardware) (struct i2c_client *client);
> + void (*release_hardware) (void);
> +};
> +
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
Randy,
>> +data to be read. Then, poll() will indicate that fact to
>the userspace.
>> +
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset
>the hardware.
>
> _RESET) to reset
>
Thanks, all are now fixed.
Waldek
>> +free_irq:
>> + free_irq(client->irq, info);
>> +free_buf:
>> + kfree(info->buf);
>> +free_info:
>> + kfree(info);
>> +
>> + dev_info(&client->dev, "Not probed.");
>> + return ret;
>> +}
>
>When respinning, you could consider using managed devices
>(devm_*); this error path could completely go then.
Well, to be honest I did't know about that 'feature' :), but now I see it's pretty nice. Will start to use it.
Thanks,
Waldek
On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> > Add new driver for MicroRead NFC chip connected to i2c bus.
> >
> > See Documentation/nfc/nfc-microread.txt.
>
> As I said in my first review and Alan also pointed out now, the
> most important change will be to add a common NFC core layer,
> before adding more hardware drivers.
>
> Also, regarding the user interface, we need to be really sure
> that this is the best way of talking to NFC devices. The interface
> you have today is a simple character device read/write kind,
> which may be the best thing if the protocol stack on top is
> really simple and there is never the need to have multiple
> applications talking to different endpoints on the wireless
> interface, and if there are no protocol headers being
> send over the character device interface.
>
> Otherwise, a better interface is probably to add a new network
> socket family and abstract the protocol layers in the kernel.
Yes, NFC seems to be a good fit for a new socket family. Especially if we ever
want to have a proper NFC p2p support from the kernel.
Sending HCI commands should be done through a dedicated netlink socket too.
I am currently strting to work on such solution, and I hope to be able to come
up with a basic prototype for it in a few weeks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
>Yes, NFC seems to be a good fit for a new socket family.
>Especially if we ever want to have a proper NFC p2p support
>from the kernel.
>Sending HCI commands should be done through a dedicated
>netlink socket too.
>
>I am currently strting to work on such solution, and I hope to
>be able to come up with a basic prototype for it in a few weeks.
What about common drivers interface in this case. Should we go for common /dev/nfcX interface as well?
Waldek-
On Tuesday 29 March 2011, [email protected] wrote:
> >Yes, NFC seems to be a good fit for a new socket family.
> >Especially if we ever want to have a proper NFC p2p support
> >from the kernel.
> >Sending HCI commands should be done through a dedicated
> >netlink socket too.
> >
> >I am currently strting to work on such solution, and I hope to
> >be able to come up with a basic prototype for it in a few weeks.
>
> What about common drivers interface in this case.
> Should we go for common /dev/nfcX interface as well?
I fear there can only be one. A good implementation of a socket
interface would mean that there is no need for a character device.
The difference between the two is where you keep the common
NFC logic:
If you have a character device, it will be like a serial port
connecting to a modem. Any higher-level protocols live in the
user space and are limited to a single application then, which
is required to have appropriate priviledges to open the device.
In contrast, a socket implementation puts the protocol
stack into the kernel, which requires much more kernel code
but almost no user space library code, aside from perhaps
a small shim layer. It makes it possible to have multiple
applications and/or users concurrently use NFC to make connections
to separate endpoints. Since sockets have no implicit permission
handling, the kernel code then needs to implement a way to enforce
policy.
I still don't understand enough about NFC to judge which of
the two is better suited for the problem, but my feeling is
that a socket based implementation would be better if you expect
a lot of people to use it, while the main advantage of the
character device is its simplicity, so that would be preferred
if you only expect a very small set of possible applications
for this.
Arnd
> The difference between the two is where you keep the common
> NFC logic:
I think I'd disagree on that
>
> If you have a character device, it will be like a serial port
> connecting to a modem. Any higher-level protocols live in the
> user space and are limited to a single application then, which
> is required to have appropriate priviledges to open the device.
A socket is just an API just as a file, you can put the stack in either
place in either case.
> character device is its simplicity, so that would be preferred
> if you only expect a very small set of possible applications
> for this.
NFC is not particularly performance dependant so having a lot of the
stack in a daemon isn't really going to hurt anything too much on a
client embedded device/phone.
The bigger question is probably what it needs to look like the other end
- ie the server side embedded devices doing transactions.
On Tuesday 29 March 2011, Alan Cox wrote:
> > The difference between the two is where you keep the common
> > NFC logic:
>
> I think I'd disagree on that
> >
> > If you have a character device, it will be like a serial port
> > connecting to a modem. Any higher-level protocols live in the
> > user space and are limited to a single application then, which
> > is required to have appropriate priviledges to open the device.
>
> A socket is just an API just as a file, you can put the stack in either
> place in either case.
Fair enough. Obviously the character device that we've been talking
about here does not include the stack, but that would be another option.
As you say, using a socket with a dumb interface is also possible,
but that sounds to me like a combination of all the disadvantages.
> > character device is its simplicity, so that would be preferred
> > if you only expect a very small set of possible applications
> > for this.
>
> NFC is not particularly performance dependant so having a lot of the
> stack in a daemon isn't really going to hurt anything too much on a
> client embedded device/phone.
>
> The bigger question is probably what it needs to look like the other end
> - ie the server side embedded devices doing transactions.
I was under the impression that NFC was peer-to-peer, so the driver
would already be handling both sides potentially.
Arnd
> I was under the impression that NFC was peer-to-peer, so the driver
> would already be handling both sides potentially.
Your security requirements each side are going to be very different. A
phone or handheld device is generally single user at a time, the other
end may well be interacting with many devices at once each with their own
security context and potentially handling sensitive data (payment
authorisations for example).
Alan
On Tuesday 29 March 2011, Alan Cox wrote:
> > I was under the impression that NFC was peer-to-peer, so the driver
> > would already be handling both sides potentially.
>
> Your security requirements each side are going to be very different. A
> phone or handheld device is generally single user at a time, the other
> end may well be interacting with many devices at once each with their own
> security context and potentially handling sensitive data (payment
> authorisations for example).
Yes, good point.
Arnd
On Tue, Mar 29, 2011 at 01:05:02PM +0200, Arnd Bergmann wrote:
> On Tuesday 29 March 2011, [email protected] wrote:
> > >Yes, NFC seems to be a good fit for a new socket family.
> > >Especially if we ever want to have a proper NFC p2p support
> > >from the kernel.
> > >Sending HCI commands should be done through a dedicated
> > >netlink socket too.
> > >
> > >I am currently strting to work on such solution, and I hope to
> > >be able to come up with a basic prototype for it in a few weeks.
> >
> > What about common drivers interface in this case.
> > Should we go for common /dev/nfcX interface as well?
>
> I fear there can only be one. A good implementation of a socket
> interface would mean that there is no need for a character device.
My idea of an initial NFC subsystem architecture was actually the following
one:
- A core NFC layer against which NFC drivers would register.
- A netlink socket for handling the HCI commands. That would put a big part of
the NFC HCI layer in kernel land and could potentially simplify the existing
NFC stacks.
- A socket family for the LLCP abstraction, a.k.a. NFC peer to peer mode.
We can start working on the first 2 items and leave the last one as a future
enhancement, since what NFC is currently mostly used for is tag
reading/writing and smartcard emulation.
Basically, we'd replace the current character device option with a netlink
one, allowing for a single kernel entry point for multiple applications
willing to do NFC.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
>My idea of an initial NFC subsystem architecture was actually
>the following
>one:
>- A core NFC layer against which NFC drivers would register.
>- A netlink socket for handling the HCI commands. That would
>put a big part of the NFC HCI layer in kernel land and could
>potentially simplify the existing NFC stacks.
Shouldn't be better to add a new AF_NFC sock family and then register new LLCP and HCI protocols?
Waldek-
On Thursday 31 March 2011, [email protected] wrote:
> >My idea of an initial NFC subsystem architecture was actually
> >the following
> >one:
> >- A core NFC layer against which NFC drivers would register.
> >- A netlink socket for handling the HCI commands. That would
> >put a big part of the NFC HCI layer in kernel land and could
> >potentially simplify the existing NFC stacks.
>
> Shouldn't be better to add a new AF_NFC sock family and then
> register new LLCP and HCI protocols?
That depends on what the HCI protocol really looks like, e.g.
is it's related to the LLCP data at all or not.
Another option might be to control the HCI over setsockopts,
but the netlink socket sounds more flexible for this.
Arnd
Hi Waldemar,
On Thu, Mar 31, 2011 at 05:42:21PM +0300, [email protected] wrote:
> >My idea of an initial NFC subsystem architecture was actually
> >the following
> >one:
> >- A core NFC layer against which NFC drivers would register.
> >- A netlink socket for handling the HCI commands. That would
> >put a big part of the NFC HCI layer in kernel land and could
> >potentially simplify the existing NFC stacks.
>
> Shouldn't be better to add a new AF_NFC sock family and then register new LLCP and HCI protocols?
>
That's the Bluetooth approach, and it works just fine as well.
However, the netlink option for sending commands and receiving answers to/from
a subsystem sounds more appropriate, at least to me.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Hi Samuel,
>That's the Bluetooth approach, and it works just fine as well.
>However, the netlink option for sending commands and receiving
>answers to/from a subsystem sounds more appropriate, at least to me.
I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
Waldek-
On Thu, Mar 31, 2011 at 06:24:50PM +0300, [email protected] wrote:
> Hi Samuel,
>
> >That's the Bluetooth approach, and it works just fine as well.
> >However, the netlink option for sending commands and receiving
> >answers to/from a subsystem sounds more appropriate, at least to me.
>
> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>
Have a look at the wireless nl80211.c implementation. It's a good example for
that sort of usage.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
Hi all,
On Thu, Mar 31, 2011 at 12:30 PM, Samuel Ortiz <[email protected]> wrote:
> On Thu, Mar 31, 2011 at 06:24:50PM +0300, [email protected] wrote:
>> Hi Samuel,
>>
>> >That's the Bluetooth approach, and it works just fine as well.
>> >However, the netlink option for sending commands and receiving
>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>
>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>
> Have a look at the wireless nl80211.c implementation. It's a good example for
> that sort of usage.
We are finalizing a NFC stack prototype and we decided to use generic
netlink. But during the implementation we found a disadvantage of this
approach.
While implementing data_exchange through netlink, it was not possible
to avoid memcpy as the skbuff coming from netlink can contain more
than one message.
We didn't find any implementation that uses netlink to exchage data.
As in the nl80211, the netlink is used only for control commands.
We plan to publish for comments this prototype early next week. Then
we plan to implement a version using AF_NFC.
Regards,
Aloisio Almeida Jr
INdT - Instituto Nokia de Tecnologia
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Friday 01 April 2011 20:19:30 Aloisio Almeida wrote:
> We are finalizing a NFC stack prototype and we decided to use generic
> netlink. But during the implementation we found a disadvantage of this
> approach.
>
> While implementing data_exchange through netlink, it was not possible
> to avoid memcpy as the skbuff coming from netlink can contain more
> than one message.
I don't think anyone in this thread suggested using netlink for user
data.
> We didn't find any implementation that uses netlink to exchage data.
> As in the nl80211, the netlink is used only for control commands.
Yes, that is the normal way to do it.
> We plan to publish for comments this prototype early next week. Then
> we plan to implement a version using AF_NFC.
Sounds good.
Arnd
On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <[email protected]> wrote:
> On Thu, Mar 31, 2011 at 06:24:50PM +0300, [email protected] wrote:
>> Hi Samuel,
>>
>> >That's the Bluetooth approach, and it works just fine as well.
>> >However, the netlink option for sending commands and receiving
>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>
>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>
> Have a look at the wireless nl80211.c implementation. It's a good example for
> that sort of usage.
So, why was the network interface 'ala eth0 or the netlink interface dropped?
I saw these patches recently "NFC: add nfc subsystem core" - which
seem to add just the device /dev/nfc - am I correct ?
Or missing something ?
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
On Mon, Jun 6, 2011 at 3:22 PM, Pavan Savoy <[email protected]> wrote:
> On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <[email protected]> wrote:
>> On Thu, Mar 31, 2011 at 06:24:50PM +0300, [email protected] wrote:
>>> Hi Samuel,
>>>
>>> >That's the Bluetooth approach, and it works just fine as well.
>>> >However, the netlink option for sending commands and receiving
>>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>>
>>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>>
>> Have a look at the wireless nl80211.c implementation. It's a good example for
>> that sort of usage.
>
> So, why was the network interface 'ala eth0 or the netlink interface dropped?
> I saw these patches recently "NFC: add nfc subsystem core" - which
> seem to add just the device /dev/nfc - am I correct ?
> Or missing something ?
Yes apparently I did miss a whole of patches in the series.
So, for anyone who is interested on the list - this is the proposed
architecture,
http://code.openbossa.org/?p=nfc/linux-nfc.git;a=blob;f=Documentation/networking/nfc.txt;h=e17ac191ca958235db99da23786c46c4c60082e9;hb=f8d3349352063626240ad1c1a15e2361dab81ef4
>> Cheers,
>> Samuel.
>>
>> --
>> Intel Open Source Technology Centre
>> http://oss.intel.com/
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>>
>>
>
On Mon, Jun 6, 2011 at 5:30 PM, Pavan Savoy <[email protected]> wrote:
> Yes apparently I did miss a whole of patches in the series.
> So, for anyone who is interested on the list - this is the proposed
> architecture,
>
> http://code.openbossa.org/?p=nfc/linux-nfc.git;a=blob;f=Documentation/networking/nfc.txt;h=e17ac191ca958235db99da23786c46c4c60082e9;hb=f8d3349352063626240ad1c1a15e2361dab81ef4
Yes, the patch series was submitted to linux-wireless and there is an
on going discussion over there.
Aloisio
Hi Pavan,
On Mon, Jun 06, 2011 at 03:22:01PM -0500, Pavan Savoy wrote:
> On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <[email protected]> wrote:
> > On Thu, Mar 31, 2011 at 06:24:50PM +0300, [email protected] wrote:
> >> Hi Samuel,
> >>
> >> >That's the Bluetooth approach, and it works just fine as well.
> >> >However, the netlink option for sending commands and receiving
> >> >answers to/from a subsystem sounds more appropriate, at least to me.
> >>
> >> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
> >>
> > Have a look at the wireless nl80211.c implementation. It's a good example for
> > that sort of usage.
>
> So, why was the network interface 'ala eth0 or the netlink interface dropped?
The netlink interface was not dropped. See:
http://marc.info/?l=linux-wireless&m=130705125228073&w=2
> I saw these patches recently "NFC: add nfc subsystem core" - which
> seem to add just the device /dev/nfc - am I correct ?
No, the /dev/nfc interface is gone with this subsystem. Instead you will talk
to an AF_NFC socket, or to the NFC generic netlink socket for some NFC high
level commands and events. See:
http://marc.info/?l=linux-wireless&m=130705125728090&w=2
for more details.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/