[net-next,v3,0/2] Add nci suit and virtual nci device driver

From: Bongsu Jeon <bongsu.jeon@samsung.com>

A NCI virtual device can be made to simulate a NCI device in user space.
Using the virtual NCI device, The NCI module and application can be
validated. This driver supports to communicate between the virtual NCI
device and NCI module. To test the basic features of NCI module, selftest
for nci is added. Test cases consist of making the virtual NCI device
on/off and controlling the device's polling for NCI1.0 and NCI2.0 version.

1/2 is the Virtual NCI device driver.
2/2 is the NCI selftest suite

v3:
 1/2
 - change the Kconfig help comment.
 - remove the mutex init code.
 - remove the unnecessary mutex(nci_send_mutex).
 - remove the full_txbuff.
 - add the code to release skb at error case.
 - refactor some code.
v2:
 1/2
 - change the permission of the Virtual NCI device.
 - add the ioctl to find the nci device index.
 2/2
 - add the NCI selftest suite.

 MAINTAINERS                           |   8 +
 drivers/nfc/Kconfig                   |  11 +
 drivers/nfc/Makefile                  |   1 +
 drivers/nfc/virtual_ncidev.c          | 227 ++++++++++
 tools/testing/selftests/Makefile      |   1 +
 tools/testing/selftests/nci/Makefile  |   6 +
 tools/testing/selftests/nci/config    |   3 +
 tools/testing/selftests/nci/nci_dev.c | 599 ++++++++++++++++++++++++++
 8 files changed, 856 insertions(+)
 create mode 100644 drivers/nfc/virtual_ncidev.c
 create mode 100644 tools/testing/selftests/nci/Makefile
 create mode 100644 tools/testing/selftests/nci/config
 create mode 100644 tools/testing/selftests/nci/nci_dev.c

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.