Date:   Fri, 22 Jan 2021 18:49:29 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Bongsu Jeon <bongsu.jeon2@...il.com>
Cc:     shuah@...nel.org, netdev@...r.kernel.org, linux-nfc@...ts.01.org,
        linux-kselftest@...r.kernel.org,
        Bongsu Jeon <bongsu.jeon@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH net-next v2 1/2] nfc: Add a virtual nci device driver

On Wed, 20 Jan 2021 20:56:44 +0900 Bongsu Jeon wrote:
> From: Bongsu Jeon <bongsu.jeon@...sung.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.
> 
> Signed-off-by: Bongsu Jeon <bongsu.jeon@...sung.com>

Please CC Krzysztof on next version, maybe we'll be lucky and he finds
time to look at this :)

> diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
> index 75c65d339018..d32c3a8937ed 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
> +	  A NCI virtual device can be made to simulate a NCI device in user
> +	  level. 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.

How about:

  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.

Just trying to improve the grammar.


> +#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 mutex nci_send_mutex;
> +static struct miscdevice miscdev;
> +static struct sk_buff *send_buff;
> +static struct mutex nci_mutex;

I think if you use:

static DEFINE_MUTEX(...);

you won't have to init them in the code

> +static struct nci_dev *ndev;
> +static bool full_txbuff;
> +
> +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_send_mutex);
> +	if (full_txbuff)
> +		kfree_skb(send_buff);
> +	full_txbuff = false;
> +	mutex_unlock(&nci_send_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_send_mutex);
> +	if (full_txbuff) {
> +		mutex_unlock(&nci_send_mutex);
> +		return -1;
> +	}
> +	send_buff = skb_copy(skb, GFP_KERNEL);
> +	full_txbuff = true;

Do you need this variable? looks like you can just set send_buff to NULL

> +	mutex_unlock(&nci_send_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_send_mutex);
> +	if (!full_txbuff) {
> +		mutex_unlock(&nci_send_mutex);
> +		return 0;
> +	}
> +
> +	actual_len = count > send_buff->len ? send_buff->len : count;

min_t()

> +	if (copy_to_user(buf, send_buff->data, actual_len)) {
> +		mutex_unlock(&nci_send_mutex);
> +		return -EFAULT;
> +	}
> +
> +	skb_pull(send_buff, actual_len);
> +	if (send_buff->len == 0) {
> +		kfree_skb(send_buff);

consume_skb()

> +		full_txbuff = false;
> +	}
> +	mutex_unlock(&nci_send_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))

leaks skb

> +		return -EFAULT;
> +
> +	nci_recv_frame(ndev, skb);
> +	return count;
> +}

> +static long virtual_ncidev_ioctl(struct file *flip, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	long res = -ENOTTY;
> +
> +	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;
> +		res = 0;
> +	}
> +
> +	return res;

The condition can be flipped to save the indentation and local variable:

if (cmd != ...)
	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)
> +{
> +	int ret;
> +
> +	mutex_init(&nci_mutex);
> +	state = virtual_ncidev_disabled;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.name = "virtual_nci";
> +	miscdev.fops = &virtual_ncidev_fops;
> +	miscdev.mode = S_IALLUGO;
> +	ret = misc_register(&miscdev);
> +
> +	return ret;

no need for the local variable here, just 

	return misc_register()

> +}
> +
> +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@...sung.com>");

Powered by blists - more mailing lists