| Message ID | 20210123092425.11434-2-bongsu.jeon@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add nci suit and virtual nci device driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 2 of 2 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sat, 23 Jan 2021 18:24:24 +0900 Bongsu Jeon wrote: > From: Bongsu Jeon <bongsu.jeon@samsung.com> > > NCI virtual device simulates a NCI device to the user. It can be used to > validate the NCI module and applications. This driver supports > communication between the virtual NCI device and NCI module. > > Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com> > +static bool virtual_ncidev_check_enabled(void) > +{ > + bool ret = true; > + > + mutex_lock(&nci_mutex); > + if (state != virtual_ncidev_enabled) > + ret = false; > + mutex_unlock(&nci_mutex); > + > + return ret; This can be simplified like: bool ret; mutex_lock() ret = state == virtual_ncidev_enabled; mutex_unlock() return ret; > +} > + > +static int virtual_nci_open(struct nci_dev *ndev) > +{ > + return 0; > +} > + > +static int virtual_nci_close(struct nci_dev *ndev) > +{ > + mutex_lock(&nci_mutex); > + if (send_buff) > + kfree_skb(send_buff); kfree_skb() handles NULL, no need for the if, you can always call kfree_skb() here > + send_buff = NULL; > + mutex_unlock(&nci_mutex); > + > + return 0; > +} > + > +static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb) > +{ > + if (virtual_ncidev_check_enabled() == false) > + return 0; Shouldn't you check this _under_ the lock below? Otherwise there is a small window between check and use of send_buff > + mutex_lock(&nci_mutex); > + if (send_buff) { > + mutex_unlock(&nci_mutex); > + return -1; > + } > + send_buff = skb_copy(skb, GFP_KERNEL); > + mutex_unlock(&nci_mutex); > + > + return 0; > +} > + > +static struct nci_ops virtual_nci_ops = { > + .open = virtual_nci_open, > + .close = virtual_nci_close, > + .send = virtual_nci_send > +}; > + > +static ssize_t virtual_ncidev_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + size_t actual_len; > + > + mutex_lock(&nci_mutex); > + if (!send_buff) { > + mutex_unlock(&nci_mutex); > + return 0; > + } > + > + actual_len = min_t(size_t, count, send_buff->len); > + > + if (copy_to_user(buf, send_buff->data, actual_len)) { > + mutex_unlock(&nci_mutex); > + return -EFAULT; > + } > + > + skb_pull(send_buff, actual_len); > + if (send_buff->len == 0) { > + consume_skb(send_buff); > + send_buff = NULL; > + } > + mutex_unlock(&nci_mutex); > + > + return actual_len; > +} > + > +static ssize_t virtual_ncidev_write(struct file *file, > + const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct sk_buff *skb; > + > + skb = alloc_skb(count, GFP_KERNEL); > + if (!skb) > + return -ENOMEM; > + > + if (copy_from_user(skb_put(skb, count), buf, count)) { > + kfree_skb(skb); > + return -EFAULT; > + } > + > + nci_recv_frame(ndev, skb); > + return count; > +} > + > +static int virtual_ncidev_open(struct inode *inode, struct file *file) > +{ > + int ret = 0; > + > + mutex_lock(&nci_mutex); > + if (state != virtual_ncidev_disabled) { > + mutex_unlock(&nci_mutex); > + return -EBUSY; > + } > + > + ndev = nci_allocate_device(&virtual_nci_ops, VIRTUAL_NFC_PROTOCOLS, > + 0, 0); > + if (!ndev) { > + mutex_unlock(&nci_mutex); > + return -ENOMEM; > + } > + > + ret = nci_register_device(ndev); > + if (ret < 0) { > + nci_free_device(ndev); > + mutex_unlock(&nci_mutex); > + return ret; > + } > + state = virtual_ncidev_enabled; > + mutex_unlock(&nci_mutex); > + > + return 0; > +} > + > +static int virtual_ncidev_close(struct inode *inode, struct file *file) > +{ > + mutex_lock(&nci_mutex); > + > + if (state == virtual_ncidev_enabled) { > + state = virtual_ncidev_disabling; > + mutex_unlock(&nci_mutex); > + > + nci_unregister_device(ndev); > + nci_free_device(ndev); > + > + mutex_lock(&nci_mutex); > + } > + > + state = virtual_ncidev_disabled; > + mutex_unlock(&nci_mutex); > + > + return 0; > +} > + > +static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd, > + unsigned long arg) > +{ > + if (cmd == IOCTL_GET_NCIDEV_IDX) { > + struct nfc_dev *nfc_dev = ndev->nfc_dev; > + void __user *p = (void __user *)arg; > + > + if (copy_to_user(p, &nfc_dev->idx, sizeof(nfc_dev->idx))) > + return -EFAULT; > + } else { > + return -ENOTTY; > + } > + > + return 0; Please flip the condition and return early. I think I suggested this already: { struct nfc_dev *nfc_dev = ndev->nfc_dev; void __user *p = (void __user *)arg; if (cmd != IOCTL_GET_NCIDEV_IDX) return -ENOTTY; if (copy_to_user(p, &nfc_dev->idx, sizeof(nfc_dev->idx))) return -EFAULT; return 0;
On Wed, Jan 27, 2021 at 10:42 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 23 Jan 2021 18:24:24 +0900 Bongsu Jeon wrote: > > From: Bongsu Jeon <bongsu.jeon@samsung.com> > > > > NCI virtual device simulates a NCI device to the user. It can be used to > > validate the NCI module and applications. This driver supports > > communication between the virtual NCI device and NCI module. > > > > Signed-off-by: Bongsu Jeon <bongsu.jeon@samsung.com> > > > +static bool virtual_ncidev_check_enabled(void) > > +{ > > + bool ret = true; > > + > > + mutex_lock(&nci_mutex); > > + if (state != virtual_ncidev_enabled) > > + ret = false; > > + mutex_unlock(&nci_mutex); > > + > > + return ret; > > > This can be simplified like: > > bool ret; > > mutex_lock() > ret = state == virtual_ncidev_enabled; > mutex_unlock() > > return ret; > > > > +} > > + > > +static int virtual_nci_open(struct nci_dev *ndev) > > +{ > > + return 0; > > +} > > + > > +static int virtual_nci_close(struct nci_dev *ndev) > > +{ > > + mutex_lock(&nci_mutex); > > + if (send_buff) > > + kfree_skb(send_buff); > > kfree_skb() handles NULL, no need for the if, you can always call > kfree_skb() here > I see, I will remove this. > > + send_buff = NULL; > > + mutex_unlock(&nci_mutex); > > + > > + return 0; > > +} > > + > > +static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb) > > +{ > > + if (virtual_ncidev_check_enabled() == false) > > + return 0; > > Shouldn't you check this _under_ the lock below? > > Otherwise there is a small window between check and use of send_buff > In virtual_ncidev_check_enabled function, mutex is used. I think that virtual_ncidev_check_enabled function isn't necessary after refactoring. So I'll remove it. > > + mutex_lock(&nci_mutex); > > + if (send_buff) { > > + mutex_unlock(&nci_mutex); > > + return -1; > > + } > > + send_buff = skb_copy(skb, GFP_KERNEL); > > + mutex_unlock(&nci_mutex); > > + > > + return 0; > > +} > > + > > +static struct nci_ops virtual_nci_ops = { > > + .open = virtual_nci_open, > > + .close = virtual_nci_close, > > + .send = virtual_nci_send > > +}; > > + > > +static ssize_t virtual_ncidev_read(struct file *file, char __user *buf, > > + size_t count, loff_t *ppos) > > +{ > > + size_t actual_len; > > + > > + mutex_lock(&nci_mutex); > > + if (!send_buff) { > > + mutex_unlock(&nci_mutex); > > + return 0; > > + } > > + > > + actual_len = min_t(size_t, count, send_buff->len); > > + > > + if (copy_to_user(buf, send_buff->data, actual_len)) { > > + mutex_unlock(&nci_mutex); > > + return -EFAULT; > > + } > > + > > + skb_pull(send_buff, actual_len); > > + if (send_buff->len == 0) { > > + consume_skb(send_buff); > > + send_buff = NULL; > > + } > > + mutex_unlock(&nci_mutex); > > + > > + return actual_len; > > +} > > + > > +static ssize_t virtual_ncidev_write(struct file *file, > > + const char __user *buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct sk_buff *skb; > > + > > + skb = alloc_skb(count, GFP_KERNEL); > > + if (!skb) > > + return -ENOMEM; > > + > > + if (copy_from_user(skb_put(skb, count), buf, count)) { > > + kfree_skb(skb); > > + return -EFAULT; > > + } > > + > > + nci_recv_frame(ndev, skb); > > + return count; > > +} > > + > > +static int virtual_ncidev_open(struct inode *inode, struct file *file) > > +{ > > + int ret = 0; > > + > > + mutex_lock(&nci_mutex); > > + if (state != virtual_ncidev_disabled) { > > + mutex_unlock(&nci_mutex); > > + return -EBUSY; > > + } > > + > > + ndev = nci_allocate_device(&virtual_nci_ops, VIRTUAL_NFC_PROTOCOLS, > > + 0, 0); > > + if (!ndev) { > > + mutex_unlock(&nci_mutex); > > + return -ENOMEM; > > + } > > + > > + ret = nci_register_device(ndev); > > + if (ret < 0) { > > + nci_free_device(ndev); > > + mutex_unlock(&nci_mutex); > > + return ret; > > + } > > + state = virtual_ncidev_enabled; > > + mutex_unlock(&nci_mutex); > > + > > + return 0; > > +} > > + > > +static int virtual_ncidev_close(struct inode *inode, struct file *file) > > +{ > > + mutex_lock(&nci_mutex); > > + > > + if (state == virtual_ncidev_enabled) { > > + state = virtual_ncidev_disabling; > > + mutex_unlock(&nci_mutex); > > + > > + nci_unregister_device(ndev); > > + nci_free_device(ndev); > > + > > + mutex_lock(&nci_mutex); > > + } > > + > > + state = virtual_ncidev_disabled; > > + mutex_unlock(&nci_mutex); > > + > > + return 0; > > +} > > + > > +static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd, > > + unsigned long arg) > > +{ > > + if (cmd == IOCTL_GET_NCIDEV_IDX) { > > + struct nfc_dev *nfc_dev = ndev->nfc_dev; > > + void __user *p = (void __user *)arg; > > + > > + if (copy_to_user(p, &nfc_dev->idx, sizeof(nfc_dev->idx))) > > + return -EFAULT; > > + } else { > > + return -ENOTTY; > > + } > > + > > + return 0; > > Please flip the condition and return early. I think I suggested this > already: Sorry, I didn't misunderstand it. I will change it. > > { > struct nfc_dev *nfc_dev = ndev->nfc_dev; > void __user *p = (void __user *)arg; > > if (cmd != IOCTL_GET_NCIDEV_IDX) > return -ENOTTY; > > if (copy_to_user(p, &nfc_dev->idx, sizeof(nfc_dev->idx))) > return -EFAULT; > > return 0; Thank you for your review.
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig index 75c65d339018..288c6f1c6979 100644 --- a/drivers/nfc/Kconfig +++ b/drivers/nfc/Kconfig @@ -49,6 +49,17 @@ config NFC_PORT100 If unsure, say N. +config NFC_VIRTUAL_NCI + tristate "NCI device simulator driver" + depends on NFC_NCI + help + NCI virtual device simulates a NCI device to the user. + It can be used to validate the NCI module and applications. + This driver supports communication between the virtual NCI device and + module. + + If unsure, say N. + source "drivers/nfc/fdp/Kconfig" source "drivers/nfc/pn544/Kconfig" source "drivers/nfc/pn533/Kconfig" diff --git a/drivers/nfc/Makefile b/drivers/nfc/Makefile index 5393ba59b17d..7b1bfde1d971 100644 --- a/drivers/nfc/Makefile +++ b/drivers/nfc/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_NFC_ST_NCI) += st-nci/ obj-$(CONFIG_NFC_NXP_NCI) += nxp-nci/ obj-$(CONFIG_NFC_S3FWRN5) += s3fwrn5/ obj-$(CONFIG_NFC_ST95HF) += st95hf/ +obj-$(CONFIG_NFC_VIRTUAL_NCI) += virtual_ncidev.o diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c new file mode 100644 index 000000000000..3bd09dfebfe5 --- /dev/null +++ b/drivers/nfc/virtual_ncidev.c @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Virtual NCI device simulation driver + * + * Copyright (C) 2020 Samsung Electrnoics + * Bongsu Jeon <bongsu.jeon@samsung.com> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/miscdevice.h> +#include <linux/mutex.h> +#include <net/nfc/nci_core.h> + +enum virtual_ncidev_mode { + virtual_ncidev_enabled, + virtual_ncidev_disabled, + virtual_ncidev_disabling, +}; + +#define IOCTL_GET_NCIDEV_IDX 0 +#define VIRTUAL_NFC_PROTOCOLS (NFC_PROTO_JEWEL_MASK | \ + NFC_PROTO_MIFARE_MASK | \ + NFC_PROTO_FELICA_MASK | \ + NFC_PROTO_ISO14443_MASK | \ + NFC_PROTO_ISO14443_B_MASK | \ + NFC_PROTO_ISO15693_MASK) + +static enum virtual_ncidev_mode state; +static struct miscdevice miscdev; +static struct sk_buff *send_buff; +static struct nci_dev *ndev; +static DEFINE_MUTEX(nci_mutex); + +static bool virtual_ncidev_check_enabled(void) +{ + bool ret = true; + + mutex_lock(&nci_mutex); + if (state != virtual_ncidev_enabled) + ret = false; + mutex_unlock(&nci_mutex); + + return ret; +} + +static int virtual_nci_open(struct nci_dev *ndev) +{ + return 0; +} + +static int virtual_nci_close(struct nci_dev *ndev) +{ + mutex_lock(&nci_mutex); + if (send_buff) + kfree_skb(send_buff); + send_buff = NULL; + mutex_unlock(&nci_mutex); + + return 0; +} + +static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb) +{ + if (virtual_ncidev_check_enabled() == false) + return 0; + + mutex_lock(&nci_mutex); + if (send_buff) { + mutex_unlock(&nci_mutex); + return -1; + } + send_buff = skb_copy(skb, GFP_KERNEL); + mutex_unlock(&nci_mutex); + + return 0; +} + +static struct nci_ops virtual_nci_ops = { + .open = virtual_nci_open, + .close = virtual_nci_close, + .send = virtual_nci_send +}; + +static ssize_t virtual_ncidev_read(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + size_t actual_len; + + mutex_lock(&nci_mutex); + if (!send_buff) { + mutex_unlock(&nci_mutex); + return 0; + } + + actual_len = min_t(size_t, count, send_buff->len); + + if (copy_to_user(buf, send_buff->data, actual_len)) { + mutex_unlock(&nci_mutex); + return -EFAULT; + } + + skb_pull(send_buff, actual_len); + if (send_buff->len == 0) { + consume_skb(send_buff); + send_buff = NULL; + } + mutex_unlock(&nci_mutex); + + return actual_len; +} + +static ssize_t virtual_ncidev_write(struct file *file, + const char __user *buf, + size_t count, loff_t *ppos) +{ + struct sk_buff *skb; + + skb = alloc_skb(count, GFP_KERNEL); + if (!skb) + return -ENOMEM; + + if (copy_from_user(skb_put(skb, count), buf, count)) { + kfree_skb(skb); + return -EFAULT; + } + + nci_recv_frame(ndev, skb); + return count; +} + +static int virtual_ncidev_open(struct inode *inode, struct file *file) +{ + int ret = 0; + + mutex_lock(&nci_mutex); + if (state != virtual_ncidev_disabled) { + mutex_unlock(&nci_mutex); + return -EBUSY; + } + + ndev = nci_allocate_device(&virtual_nci_ops, VIRTUAL_NFC_PROTOCOLS, + 0, 0); + if (!ndev) { + mutex_unlock(&nci_mutex); + return -ENOMEM; + } + + ret = nci_register_device(ndev); + if (ret < 0) { + nci_free_device(ndev); + mutex_unlock(&nci_mutex); + return ret; + } + state = virtual_ncidev_enabled; + mutex_unlock(&nci_mutex); + + return 0; +} + +static int virtual_ncidev_close(struct inode *inode, struct file *file) +{ + mutex_lock(&nci_mutex); + + if (state == virtual_ncidev_enabled) { + state = virtual_ncidev_disabling; + mutex_unlock(&nci_mutex); + + nci_unregister_device(ndev); + nci_free_device(ndev); + + mutex_lock(&nci_mutex); + } + + state = virtual_ncidev_disabled; + mutex_unlock(&nci_mutex); + + return 0; +} + +static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd, + unsigned long arg) +{ + if (cmd == IOCTL_GET_NCIDEV_IDX) { + struct nfc_dev *nfc_dev = ndev->nfc_dev; + void __user *p = (void __user *)arg; + + if (copy_to_user(p, &nfc_dev->idx, sizeof(nfc_dev->idx))) + return -EFAULT; + } else { + return -ENOTTY; + } + + return 0; +} + +static const struct file_operations virtual_ncidev_fops = { + .owner = THIS_MODULE, + .read = virtual_ncidev_read, + .write = virtual_ncidev_write, + .open = virtual_ncidev_open, + .release = virtual_ncidev_close, + .unlocked_ioctl = virtual_ncidev_ioctl +}; + +static int __init virtual_ncidev_init(void) +{ + state = virtual_ncidev_disabled; + miscdev.minor = MISC_DYNAMIC_MINOR; + miscdev.name = "virtual_nci"; + miscdev.fops = &virtual_ncidev_fops; + miscdev.mode = S_IALLUGO; + + return misc_register(&miscdev); +} + +static void __exit virtual_ncidev_exit(void) +{ + misc_deregister(&miscdev); +} + +module_init(virtual_ncidev_init); +module_exit(virtual_ncidev_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Virtual NCI device simulation driver"); +MODULE_AUTHOR("Bongsu Jeon <bongsu.jeon@samsung.com>");