All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
@ 2016-10-10 15:58 Lubomir Rintel
  2016-10-10 17:33 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 7+ messages in thread
From: Lubomir Rintel @ 2016-10-10 15:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Dominik Brodowski, Lubomir Rintel

This implements only the very basic protocol "Mode A", just to make the
device functional. Patches to implement "Mode C" that uses better bulking
and is interrupt-driver may follow.

The device essentially speaks the same protocol as USB CCID devices do over
the bulk endpoints. The driver exchanges the command submissions and
responses over a plain read()/write() interface, compatible with legacy
OpenCT's pcmcia_block driver.

Patches for the newer CCID driver are available:
https://github.com/lkundrak/CCID/tree/lr/pcmcia_block

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 MAINTAINERS                     |   5 +
 drivers/char/pcmcia/Kconfig     |  11 ++
 drivers/char/pcmcia/Makefile    |   1 +
 drivers/char/pcmcia/scr24x_cs.c | 357 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 374 insertions(+)
 create mode 100644 drivers/char/pcmcia/scr24x_cs.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5327bbe..8dc6a9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10608,6 +10608,11 @@ W:	http://www.sunplus.com
 S:	Supported
 F:	arch/score/
 
+SCR24X CHIP CARD INTERFACE DRIVER
+M:	Lubomir Rintel <lkundrak@v3.sk>
+S:	Supported
+F:	drivers/char/pcmcia/scr24x_cs.c
+
 SYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
 M:	Sudeep Holla <sudeep.holla@arm.com>
 L:	linux-arm-kernel@lists.infradead.org
diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
index 8d3dfb0..1d1e7da 100644
--- a/drivers/char/pcmcia/Kconfig
+++ b/drivers/char/pcmcia/Kconfig
@@ -43,6 +43,17 @@ config CARDMAN_4040
 	  (http://www.omnikey.com/), or a current development version of OpenCT
 	  (http://www.opensc-project.org/opensc).
 
+config SCR24X
+	tristate "SCR24x Chip Card Interface support"
+	depends on PCMCIA
+	help
+	  Enable support for the SCR24x PCMCIA Chip Card Interface.
+
+	  To compile this driver as a module, choose M here.
+	  The module will be called scr24x_cs..
+
+	  If unsure say N.
+
 config IPWIRELESS
 	tristate "IPWireless 3G UMTS PCMCIA card support"
 	depends on PCMCIA && NETDEVICES && TTY
diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
index 0aae209..5b836bc 100644
--- a/drivers/char/pcmcia/Makefile
+++ b/drivers/char/pcmcia/Makefile
@@ -7,3 +7,4 @@
 obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
 obj-$(CONFIG_CARDMAN_4000) += cm4000_cs.o
 obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
+obj-$(CONFIG_SCR24X) += scr24x_cs.o
diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
new file mode 100644
index 0000000..24379ac
--- /dev/null
+++ b/drivers/char/pcmcia/scr24x_cs.c
@@ -0,0 +1,357 @@
+/*
+ * SCR24x PCMCIA Smart Card Reader Driver
+ *
+ * Copyright (C) 2005-2006 TL Sudheendran
+ * Copyright (C) 2016 Lubomir Rintel
+ *
+ * Derived from "scr24x_v4.2.6_Release.tar.gz" driver by TL Sudheendran.
+ *
+ * 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, 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; see the file COPYING.  If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/cdev.h>
+
+#include <pcmcia/cistpl.h>
+#include <pcmcia/ds.h>
+
+#define CCID_HEADER_SIZE	10
+#define CCID_LENGTH_OFFSET	1
+#define CCID_MAX_LEN		271
+
+#define SCR24X_DATA(n)		(1 + n)
+#define SCR24X_CMD_STATUS	7
+#define CMD_START		0x40
+#define CMD_WRITE_BYTE		0x41
+#define CMD_READ_BYTE		0x42
+#define STATUS_BUSY		0x80
+
+struct scr24x_dev {
+	struct device *dev;
+	struct cdev c_dev;
+	unsigned char buf[CCID_MAX_LEN];
+	int devno;
+	struct mutex lock;
+	struct kref refcnt;
+	u8 __iomem *regs;
+};
+
+#define SCR24X_DEVS 8
+static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
+
+static struct class *scr24x_class;
+dev_t scr24x_devt;
+
+static void scr24x_delete(struct kref *kref)
+{
+	struct scr24x_dev *dev = container_of(kref, struct scr24x_dev,
+								refcnt);
+
+	kfree(dev);
+}
+
+static int scr24x_wait_ready(struct scr24x_dev *dev)
+{
+	u_char status;
+	int timeout = 100;
+
+	do {
+		status = ioread8(dev->regs + SCR24X_CMD_STATUS);
+		if (!(status & STATUS_BUSY))
+			return 0;
+
+		msleep(20);
+	} while (--timeout);
+
+	return -EIO;
+}
+
+static int scr24x_open(struct inode *inode, struct file *filp)
+{
+	struct scr24x_dev *dev = container_of(inode->i_cdev,
+				struct scr24x_dev, c_dev);
+
+	kref_get(&dev->refcnt);
+	filp->private_data = dev;
+
+	return nonseekable_open(inode, filp);
+}
+
+static int scr24x_release(struct inode *inode, struct file *filp)
+{
+	struct scr24x_dev *dev = filp->private_data;
+
+	kref_put(&dev->refcnt, scr24x_delete);
+	return 0;
+}
+
+static int read_chunk(struct scr24x_dev *dev, size_t offset, size_t limit)
+{
+	size_t i, y;
+	int ret;
+
+	for (i = offset; i < limit; i += 5) {
+		iowrite8(CMD_READ_BYTE, dev->regs + SCR24X_CMD_STATUS);
+		ret = scr24x_wait_ready(dev);
+		if (ret < 0)
+			return ret;
+
+		for (y = 0; y < 5 && i + y < limit; y++)
+			dev->buf[i + y] = ioread8(dev->regs + SCR24X_DATA(y));
+	}
+
+	return 0;
+}
+
+static ssize_t scr24x_read(struct file *filp, char __user *buf, size_t count,
+								loff_t *ppos)
+{
+	struct scr24x_dev *dev = filp->private_data;
+	int ret;
+	int len;
+
+	if (count < CCID_HEADER_SIZE)
+		return -EINVAL;
+
+	if (mutex_lock_interruptible(&dev->lock))
+		return -ERESTARTSYS;
+
+	if (!dev->dev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = scr24x_wait_ready(dev);
+	if (ret < 0)
+		goto out;
+	len = CCID_HEADER_SIZE;
+	ret = read_chunk(dev, 0, len);
+	if (ret < 0)
+		goto out;
+
+	len += le32_to_cpu(*(__le32 *)(&dev->buf[CCID_LENGTH_OFFSET]));
+	if (len > sizeof(dev->buf)) {
+		ret = -EIO;
+		goto out;
+	}
+	read_chunk(dev, CCID_HEADER_SIZE, len);
+	if (ret < 0)
+		goto out;
+
+	if (len < count)
+		count = len;
+
+	if (copy_to_user(buf, dev->buf, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = count;
+out:
+	mutex_unlock(&dev->lock);
+	return ret;
+}
+
+static ssize_t scr24x_write(struct file *filp, const char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	struct scr24x_dev *dev = filp->private_data;
+	size_t i, y;
+	int ret;
+
+	if (mutex_lock_interruptible(&dev->lock))
+		return -ERESTARTSYS;
+
+	if (!dev->dev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (count > sizeof(dev->buf)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (copy_from_user(dev->buf, buf, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = scr24x_wait_ready(dev);
+	if (ret < 0)
+		goto out;
+
+	iowrite8(CMD_START, dev->regs + SCR24X_CMD_STATUS);
+	ret = scr24x_wait_ready(dev);
+	if (ret < 0)
+		goto out;
+
+	for (i = 0; i < count; i += 5) {
+		for (y = 0; y < 5 && i + y < count; y++)
+			iowrite8(dev->buf[i + y], dev->regs + SCR24X_DATA(y));
+
+		iowrite8(CMD_WRITE_BYTE, dev->regs + SCR24X_CMD_STATUS);
+		ret = scr24x_wait_ready(dev);
+		if (ret < 0)
+			goto out;
+	}
+
+	ret = count;
+out:
+	mutex_unlock(&dev->lock);
+	return ret;
+}
+
+static const struct file_operations scr24x_fops = {
+	.owner		= THIS_MODULE,
+	.read		= scr24x_read,
+	.write		= scr24x_write,
+	.open		= scr24x_open,
+	.release	= scr24x_release,
+	.llseek		= no_llseek,
+};
+
+static int scr24x_config_check(struct pcmcia_device *link, void *priv_data)
+{
+	if (resource_size(link->resource[PCMCIA_IOPORT_0]) != 0x11)
+		return -ENODEV;
+	return pcmcia_request_io(link);
+}
+
+static int scr24x_probe(struct pcmcia_device *link)
+{
+	struct scr24x_dev *dev;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+
+	dev->devno = find_first_zero_bit(scr24x_minors, SCR24X_DEVS);
+	if (dev->devno >= SCR24X_DEVS)
+		return -EBUSY;
+
+	mutex_init(&dev->lock);
+	kref_init(&dev->refcnt);
+
+	link->priv = dev;
+	link->config_flags |= CONF_ENABLE_IRQ | CONF_AUTO_SET_IO;
+
+	ret = pcmcia_loop_config(link, scr24x_config_check, NULL);
+	if (ret < 0)
+		goto err;
+
+	dev->dev = &link->dev;
+	dev->regs = devm_ioport_map(&link->dev,
+				link->resource[PCMCIA_IOPORT_0]->start,
+				resource_size(link->resource[PCMCIA_IOPORT_0]));
+	if (!dev->regs) {
+		ret = -EIO;
+		goto err;
+	}
+
+	cdev_init(&dev->c_dev, &scr24x_fops);
+	dev->c_dev.owner = THIS_MODULE;
+	dev->c_dev.ops = &scr24x_fops;
+	ret = cdev_add(&dev->c_dev, MKDEV(MAJOR(scr24x_devt), dev->devno), 1);
+	if (ret < 0)
+		goto err;
+
+	ret = pcmcia_enable_device(link);
+	if (ret < 0) {
+		pcmcia_disable_device(link);
+		goto err;
+	}
+
+	device_create(scr24x_class, NULL, MKDEV(MAJOR(scr24x_devt), dev->devno),
+		      NULL, "scr24x%d", dev->devno);
+
+	dev_info(&link->dev, "SCR24x Chip Card Interface\n");
+	return 0;
+
+err:
+	clear_bit(dev->devno, scr24x_minors);
+	return ret;
+}
+
+static void scr24x_remove(struct pcmcia_device *link)
+{
+	struct scr24x_dev *dev = (struct scr24x_dev *)link->priv;
+
+	device_destroy(scr24x_class, MKDEV(MAJOR(scr24x_devt), dev->devno));
+	mutex_lock(&dev->lock);
+	pcmcia_disable_device(link);
+	cdev_del(&dev->c_dev);
+	clear_bit(dev->devno, scr24x_minors);
+	dev->dev = NULL;
+	mutex_unlock(&dev->lock);
+
+	kref_put(&dev->refcnt, scr24x_delete);
+}
+
+static const struct pcmcia_device_id scr24x_ids[] = {
+	PCMCIA_DEVICE_PROD_ID12("HP", "PC Card Smart Card Reader",
+					0x53cb94f9, 0xbfdf89a5),
+	PCMCIA_DEVICE_PROD_ID1("SCR241 PCMCIA", 0x6271efa3),
+	PCMCIA_DEVICE_PROD_ID1("SCR243 PCMCIA", 0x2054e8de),
+	PCMCIA_DEVICE_PROD_ID1("SCR24x PCMCIA", 0x54a33665),
+	PCMCIA_DEVICE_NULL
+};
+MODULE_DEVICE_TABLE(pcmcia, scr24x_ids);
+
+static struct pcmcia_driver scr24x_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "scr24x_cs",
+	.probe		= scr24x_probe,
+	.remove		= scr24x_remove,
+	.id_table	= scr24x_ids,
+};
+
+static int __init scr24x_init(void)
+{
+	int ret;
+
+	scr24x_class = class_create(THIS_MODULE, "scr24x");
+	if (IS_ERR(scr24x_class))
+		return PTR_ERR(scr24x_class);
+
+	ret = alloc_chrdev_region(&scr24x_devt, 0, SCR24X_DEVS, "scr24x");
+	if (ret < 0)  {
+		class_destroy(scr24x_class);
+		return ret;
+	}
+
+	ret = pcmcia_register_driver(&scr24x_driver);
+	if (ret < 0) {
+		unregister_chrdev_region(scr24x_devt, SCR24X_DEVS);
+		class_destroy(scr24x_class);
+	}
+
+	return ret;
+}
+
+static void __exit scr24x_exit(void)
+{
+	pcmcia_unregister_driver(&scr24x_driver);
+	unregister_chrdev_region(scr24x_devt, SCR24X_DEVS);
+	class_destroy(scr24x_class);
+}
+
+module_init(scr24x_init);
+module_exit(scr24x_exit);
+
+MODULE_AUTHOR("Lubomir Rintel");
+MODULE_DESCRIPTION("SCR24x PCMCIA Smart Card Reader Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-10 15:58 [PATCH] char/pcmcia: add scr24x_cs chip card interface driver Lubomir Rintel
@ 2016-10-10 17:33 ` Greg Kroah-Hartman
  2016-10-10 19:24   ` Arnd Bergmann
  2016-10-16  9:23   ` Lubomir Rintel
  0 siblings, 2 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2016-10-10 17:33 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: linux-kernel, Arnd Bergmann, Dominik Brodowski

On Mon, Oct 10, 2016 at 05:58:15PM +0200, Lubomir Rintel wrote:
> This implements only the very basic protocol "Mode A", just to make the
> device functional. Patches to implement "Mode C" that uses better bulking
> and is interrupt-driver may follow.
> 
> The device essentially speaks the same protocol as USB CCID devices do over
> the bulk endpoints. The driver exchanges the command submissions and
> responses over a plain read()/write() interface, compatible with legacy
> OpenCT's pcmcia_block driver.
> 
> Patches for the newer CCID driver are available:
> https://github.com/lkundrak/CCID/tree/lr/pcmcia_block
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  MAINTAINERS                     |   5 +
>  drivers/char/pcmcia/Kconfig     |  11 ++
>  drivers/char/pcmcia/Makefile    |   1 +
>  drivers/char/pcmcia/scr24x_cs.c | 357 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 374 insertions(+)
>  create mode 100644 drivers/char/pcmcia/scr24x_cs.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5327bbe..8dc6a9f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10608,6 +10608,11 @@ W:	http://www.sunplus.com
>  S:	Supported
>  F:	arch/score/
>  
> +SCR24X CHIP CARD INTERFACE DRIVER
> +M:	Lubomir Rintel <lkundrak@v3.sk>
> +S:	Supported
> +F:	drivers/char/pcmcia/scr24x_cs.c
> +
>  SYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
>  M:	Sudeep Holla <sudeep.holla@arm.com>
>  L:	linux-arm-kernel@lists.infradead.org
> diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
> index 8d3dfb0..1d1e7da 100644
> --- a/drivers/char/pcmcia/Kconfig
> +++ b/drivers/char/pcmcia/Kconfig
> @@ -43,6 +43,17 @@ config CARDMAN_4040
>  	  (http://www.omnikey.com/), or a current development version of OpenCT
>  	  (http://www.opensc-project.org/opensc).
>  
> +config SCR24X
> +	tristate "SCR24x Chip Card Interface support"
> +	depends on PCMCIA
> +	help
> +	  Enable support for the SCR24x PCMCIA Chip Card Interface.
> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called scr24x_cs..
> +
> +	  If unsure say N.

A new PCMCIA driver?  What decade is this?  :)

> +
>  config IPWIRELESS
>  	tristate "IPWireless 3G UMTS PCMCIA card support"
>  	depends on PCMCIA && NETDEVICES && TTY
> diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
> index 0aae209..5b836bc 100644
> --- a/drivers/char/pcmcia/Makefile
> +++ b/drivers/char/pcmcia/Makefile
> @@ -7,3 +7,4 @@
>  obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
>  obj-$(CONFIG_CARDMAN_4000) += cm4000_cs.o
>  obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
> +obj-$(CONFIG_SCR24X) += scr24x_cs.o
> diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
> new file mode 100644
> index 0000000..24379ac
> --- /dev/null
> +++ b/drivers/char/pcmcia/scr24x_cs.c
> @@ -0,0 +1,357 @@
> +/*
> + * SCR24x PCMCIA Smart Card Reader Driver
> + *
> + * Copyright (C) 2005-2006 TL Sudheendran
> + * Copyright (C) 2016 Lubomir Rintel
> + *
> + * Derived from "scr24x_v4.2.6_Release.tar.gz" driver by TL Sudheendran.
> + *
> + * 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, 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; see the file COPYING.  If not, write to
> + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/cdev.h>
> +
> +#include <pcmcia/cistpl.h>
> +#include <pcmcia/ds.h>
> +
> +#define CCID_HEADER_SIZE	10
> +#define CCID_LENGTH_OFFSET	1
> +#define CCID_MAX_LEN		271
> +
> +#define SCR24X_DATA(n)		(1 + n)
> +#define SCR24X_CMD_STATUS	7
> +#define CMD_START		0x40
> +#define CMD_WRITE_BYTE		0x41
> +#define CMD_READ_BYTE		0x42
> +#define STATUS_BUSY		0x80
> +
> +struct scr24x_dev {
> +	struct device *dev;
> +	struct cdev c_dev;
> +	unsigned char buf[CCID_MAX_LEN];
> +	int devno;
> +	struct mutex lock;
> +	struct kref refcnt;
> +	u8 __iomem *regs;
> +};
> +
> +#define SCR24X_DEVS 8
> +static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
> +
> +static struct class *scr24x_class;
> +dev_t scr24x_devt;

global variable?  And why do you need a char device for this type of
hardware?  Isn't there already an existing interface for this device
class?


> +
> +static void scr24x_delete(struct kref *kref)
> +{
> +	struct scr24x_dev *dev = container_of(kref, struct scr24x_dev,
> +								refcnt);
> +
> +	kfree(dev);
> +}
> +
> +static int scr24x_wait_ready(struct scr24x_dev *dev)
> +{
> +	u_char status;
> +	int timeout = 100;
> +
> +	do {
> +		status = ioread8(dev->regs + SCR24X_CMD_STATUS);
> +		if (!(status & STATUS_BUSY))
> +			return 0;
> +
> +		msleep(20);
> +	} while (--timeout);
> +
> +	return -EIO;
> +}
> +
> +static int scr24x_open(struct inode *inode, struct file *filp)
> +{
> +	struct scr24x_dev *dev = container_of(inode->i_cdev,
> +				struct scr24x_dev, c_dev);
> +
> +	kref_get(&dev->refcnt);
> +	filp->private_data = dev;
> +
> +	return nonseekable_open(inode, filp);
> +}
> +
> +static int scr24x_release(struct inode *inode, struct file *filp)
> +{
> +	struct scr24x_dev *dev = filp->private_data;
> +
> +	kref_put(&dev->refcnt, scr24x_delete);

No locking?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-10 17:33 ` Greg Kroah-Hartman
@ 2016-10-10 19:24   ` Arnd Bergmann
  2016-10-16  9:23   ` Lubomir Rintel
  1 sibling, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2016-10-10 19:24 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Lubomir Rintel, linux-kernel, Dominik Brodowski

On Monday, October 10, 2016 7:33:45 PM CEST Greg Kroah-Hartman wrote:
> > +
> > +#define SCR24X_DEVS 8
> > +static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
> > +
> > +static struct class *scr24x_class;
> > +dev_t scr24x_devt;
> 
> global variable?  And why do you need a char device for this type of
> hardware?  Isn't there already an existing interface for this device
> class?

It looks like this is modeled after the other two chip card drivers
in drivers/char/pcmcia/, which are also char devices. Looking at
https://pcsclite.alioth.debian.org/ccid.html, it seems that other
drivers for this protocol are typically in user space when they are
for USB devices, so apparently no existing subsystem abstraction
in the kernel.

This reminds me that there is just one other driver in drivers/char/pcmcia
(synclink_cs), and that should be moved to drivers/tty/ with the other
synclink variants. If we do this, we could rename drivers/char/pcmcia
to something that implies "smartcard".

	Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-10 17:33 ` Greg Kroah-Hartman
  2016-10-10 19:24   ` Arnd Bergmann
@ 2016-10-16  9:23   ` Lubomir Rintel
  2016-10-16  9:57     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 7+ messages in thread
From: Lubomir Rintel @ 2016-10-16  9:23 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, Arnd Bergmann, Dominik Brodowski

Hi Greg,

(responses inline)

On Mon, 2016-10-10 at 19:33 +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 10, 2016 at 05:58:15PM +0200, Lubomir Rintel wrote:
> > 
> > This implements only the very basic protocol "Mode A", just to make
> > the
> > device functional. Patches to implement "Mode C" that uses better
> > bulking
> > and is interrupt-driver may follow.
> > 
> > The device essentially speaks the same protocol as USB CCID devices
> > do over
> > the bulk endpoints. The driver exchanges the command submissions
> > and
> > responses over a plain read()/write() interface, compatible with
> > legacy
> > OpenCT's pcmcia_block driver.
> > 
> > Patches for the newer CCID driver are available:
> > https://github.com/lkundrak/CCID/tree/lr/pcmcia_block
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  MAINTAINERS                     |   5 +
> >  drivers/char/pcmcia/Kconfig     |  11 ++
> >  drivers/char/pcmcia/Makefile    |   1 +
> >  drivers/char/pcmcia/scr24x_cs.c | 357
> > ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 374 insertions(+)
> >  create mode 100644 drivers/char/pcmcia/scr24x_cs.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5327bbe..8dc6a9f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10608,6 +10608,11 @@ W:	http://www.sunplus.com
> >  S:	Supported
> >  F:	arch/score/
> >  
> > +SCR24X CHIP CARD INTERFACE DRIVER
> > +M:	Lubomir Rintel <lkundrak@v3.sk>
> > +S:	Supported
> > +F:	drivers/char/pcmcia/scr24x_cs.c
> > +
> >  SYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
> >  M:	Sudeep Holla <sudeep.holla@arm.com>
> >  L:	linux-arm-kernel@lists.infradead.org
> > diff --git a/drivers/char/pcmcia/Kconfig
> > b/drivers/char/pcmcia/Kconfig
> > index 8d3dfb0..1d1e7da 100644
> > --- a/drivers/char/pcmcia/Kconfig
> > +++ b/drivers/char/pcmcia/Kconfig
> > @@ -43,6 +43,17 @@ config CARDMAN_4040
> >  	  (http://www.omnikey.com/), or a current development
> > version of OpenCT
> >  	  (http://www.opensc-project.org/opensc).
> >  
> > +config SCR24X
> > +	tristate "SCR24x Chip Card Interface support"
> > +	depends on PCMCIA
> > +	help
> > +	  Enable support for the SCR24x PCMCIA Chip Card
> > Interface.
> > +
> > +	  To compile this driver as a module, choose M here.
> > +	  The module will be called scr24x_cs..
> > +
> > +	  If unsure say N.
> 
> A new PCMCIA driver?  What decade is this?  :)

Not sure if you actually expect me to answer this :)

I guess it's still the decade we dislike running our hardware with
crappy out-of-tree drivers on a horribly outdated kernel.

> > +
> >  config IPWIRELESS
> >  	tristate "IPWireless 3G UMTS PCMCIA card support"
> >  	depends on PCMCIA && NETDEVICES && TTY
> > diff --git a/drivers/char/pcmcia/Makefile
> > b/drivers/char/pcmcia/Makefile
> > index 0aae209..5b836bc 100644
> > --- a/drivers/char/pcmcia/Makefile
> > +++ b/drivers/char/pcmcia/Makefile
> > @@ -7,3 +7,4 @@
> >  obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
> >  obj-$(CONFIG_CARDMAN_4000) += cm4000_cs.o
> >  obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
> > +obj-$(CONFIG_SCR24X) += scr24x_cs.o
> > diff --git a/drivers/char/pcmcia/scr24x_cs.c
> > b/drivers/char/pcmcia/scr24x_cs.c
> > new file mode 100644
> > index 0000000..24379ac
> > --- /dev/null
> > +++ b/drivers/char/pcmcia/scr24x_cs.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * SCR24x PCMCIA Smart Card Reader Driver
> > + *
> > + * Copyright (C) 2005-2006 TL Sudheendran
> > + * Copyright (C) 2016 Lubomir Rintel
> > + *
> > + * Derived from "scr24x_v4.2.6_Release.tar.gz" driver by TL
> > Sudheendran.
> > + *
> > + * 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, 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; see the file COPYING.  If not, write
> > to
> > + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA
> > 02139, USA.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/cdev.h>
> > +
> > +#include <pcmcia/cistpl.h>
> > +#include <pcmcia/ds.h>
> > +
> > +#define CCID_HEADER_SIZE	10
> > +#define CCID_LENGTH_OFFSET	1
> > +#define CCID_MAX_LEN		271
> > +
> > +#define SCR24X_DATA(n)		(1 + n)
> > +#define SCR24X_CMD_STATUS	7
> > +#define CMD_START		0x40
> > +#define CMD_WRITE_BYTE		0x41
> > +#define CMD_READ_BYTE		0x42
> > +#define STATUS_BUSY		0x80
> > +
> > +struct scr24x_dev {
> > +	struct device *dev;
> > +	struct cdev c_dev;
> > +	unsigned char buf[CCID_MAX_LEN];
> > +	int devno;
> > +	struct mutex lock;
> > +	struct kref refcnt;
> > +	u8 __iomem *regs;
> > +};
> > +
> > +#define SCR24X_DEVS 8
> > +static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
> > +
> > +static struct class *scr24x_class;
> > +dev_t scr24x_devt;
> 
> global variable?

Well, that's used to track the major number assigned on device init so
that we can release it on exit. Is there a better place to store it?

A quick look at the other drivers suggests that they either do the
same, or just store the major part. Would that be a better idea?

> And why do you need a char device for this type of
> hardware?  Isn't there already an existing interface for this device
> class?

(Arnd answered this better than I would do in his response to this
message).

> > +static void scr24x_delete(struct kref *kref)
> > +{
> > +	struct scr24x_dev *dev = container_of(kref, struct
> > scr24x_dev,
> > +								re
> > fcnt);
> > +
> > +	kfree(dev);
> > +}
> > +
> > +static int scr24x_wait_ready(struct scr24x_dev *dev)
> > +{
> > +	u_char status;
> > +	int timeout = 100;
> > +
> > +	do {
> > +		status = ioread8(dev->regs + SCR24X_CMD_STATUS);
> > +		if (!(status & STATUS_BUSY))
> > +			return 0;
> > +
> > +		msleep(20);
> > +	} while (--timeout);
> > +
> > +	return -EIO;
> > +}
> > +
> > +static int scr24x_open(struct inode *inode, struct file *filp)
> > +{
> > +	struct scr24x_dev *dev = container_of(inode->i_cdev,
> > +				struct scr24x_dev, c_dev);
> > +
> > +	kref_get(&dev->refcnt);
> > +	filp->private_data = dev;
> > +
> > +	return nonseekable_open(inode, filp);
> > +}
> > +
> > +static int scr24x_release(struct inode *inode, struct file *filp)
> > +{
> > +	struct scr24x_dev *dev = filp->private_data;
> > +
> > +	kref_put(&dev->refcnt, scr24x_delete);
> 
> No locking?

The device instance keeps a reference as well. Thus when it drops to
zero here, it means that the device itself was removed and
unregistered. At that point there's no possibility of acquiring another
reference to the device instance (via open()) and thus noone to lock
against.

Unless I'm, of course, mistaken.

> thanks,
> 
> greg k-h

Thank you,
Lubo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-16  9:23   ` Lubomir Rintel
@ 2016-10-16  9:57     ` Greg Kroah-Hartman
  2016-10-17 11:22       ` Lubomir Rintel
  0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2016-10-16  9:57 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: linux-kernel, Arnd Bergmann, Dominik Brodowski

On Sun, Oct 16, 2016 at 11:23:36AM +0200, Lubomir Rintel wrote:
> Hi Greg,
> 
> (responses inline)
> 
> On Mon, 2016-10-10 at 19:33 +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 10, 2016 at 05:58:15PM +0200, Lubomir Rintel wrote:
> > > 
> > > This implements only the very basic protocol "Mode A", just to make
> > > the
> > > device functional. Patches to implement "Mode C" that uses better
> > > bulking
> > > and is interrupt-driver may follow.
> > > 
> > > The device essentially speaks the same protocol as USB CCID devices
> > > do over
> > > the bulk endpoints. The driver exchanges the command submissions
> > > and
> > > responses over a plain read()/write() interface, compatible with
> > > legacy
> > > OpenCT's pcmcia_block driver.
> > > 
> > > Patches for the newer CCID driver are available:
> > > https://github.com/lkundrak/CCID/tree/lr/pcmcia_block
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > >  MAINTAINERS                     |   5 +
> > >  drivers/char/pcmcia/Kconfig     |  11 ++
> > >  drivers/char/pcmcia/Makefile    |   1 +
> > >  drivers/char/pcmcia/scr24x_cs.c | 357
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 374 insertions(+)
> > >  create mode 100644 drivers/char/pcmcia/scr24x_cs.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 5327bbe..8dc6a9f 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -10608,6 +10608,11 @@ W:	http://www.sunplus.com
> > >  S:	Supported
> > >  F:	arch/score/
> > >  
> > > +SCR24X CHIP CARD INTERFACE DRIVER
> > > +M:	Lubomir Rintel <lkundrak@v3.sk>
> > > +S:	Supported
> > > +F:	drivers/char/pcmcia/scr24x_cs.c
> > > +
> > >  SYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
> > >  M:	Sudeep Holla <sudeep.holla@arm.com>
> > >  L:	linux-arm-kernel@lists.infradead.org
> > > diff --git a/drivers/char/pcmcia/Kconfig
> > > b/drivers/char/pcmcia/Kconfig
> > > index 8d3dfb0..1d1e7da 100644
> > > --- a/drivers/char/pcmcia/Kconfig
> > > +++ b/drivers/char/pcmcia/Kconfig
> > > @@ -43,6 +43,17 @@ config CARDMAN_4040
> > >  	  (http://www.omnikey.com/), or a current development
> > > version of OpenCT
> > >  	  (http://www.opensc-project.org/opensc).
> > >  
> > > +config SCR24X
> > > +	tristate "SCR24x Chip Card Interface support"
> > > +	depends on PCMCIA
> > > +	help
> > > +	  Enable support for the SCR24x PCMCIA Chip Card
> > > Interface.
> > > +
> > > +	  To compile this driver as a module, choose M here.
> > > +	  The module will be called scr24x_cs..
> > > +
> > > +	  If unsure say N.
> > 
> > A new PCMCIA driver?  What decade is this?  :)
> 
> Not sure if you actually expect me to answer this :)
> 
> I guess it's still the decade we dislike running our hardware with
> crappy out-of-tree drivers on a horribly outdated kernel.

Yes, fair enough, sorry I was just joking :)

> > >  config IPWIRELESS
> > >  	tristate "IPWireless 3G UMTS PCMCIA card support"
> > >  	depends on PCMCIA && NETDEVICES && TTY
> > > diff --git a/drivers/char/pcmcia/Makefile
> > > b/drivers/char/pcmcia/Makefile
> > > index 0aae209..5b836bc 100644
> > > --- a/drivers/char/pcmcia/Makefile
> > > +++ b/drivers/char/pcmcia/Makefile
> > > @@ -7,3 +7,4 @@
> > >  obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
> > >  obj-$(CONFIG_CARDMAN_4000) += cm4000_cs.o
> > >  obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
> > > +obj-$(CONFIG_SCR24X) += scr24x_cs.o
> > > diff --git a/drivers/char/pcmcia/scr24x_cs.c
> > > b/drivers/char/pcmcia/scr24x_cs.c
> > > new file mode 100644
> > > index 0000000..24379ac
> > > --- /dev/null
> > > +++ b/drivers/char/pcmcia/scr24x_cs.c
> > > @@ -0,0 +1,357 @@
> > > +/*
> > > + * SCR24x PCMCIA Smart Card Reader Driver
> > > + *
> > > + * Copyright (C) 2005-2006 TL Sudheendran
> > > + * Copyright (C) 2016 Lubomir Rintel
> > > + *
> > > + * Derived from "scr24x_v4.2.6_Release.tar.gz" driver by TL
> > > Sudheendran.
> > > + *
> > > + * 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, 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; see the file COPYING.  If not, write
> > > to
> > > + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA
> > > 02139, USA.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/cdev.h>
> > > +
> > > +#include <pcmcia/cistpl.h>
> > > +#include <pcmcia/ds.h>
> > > +
> > > +#define CCID_HEADER_SIZE	10
> > > +#define CCID_LENGTH_OFFSET	1
> > > +#define CCID_MAX_LEN		271
> > > +
> > > +#define SCR24X_DATA(n)		(1 + n)
> > > +#define SCR24X_CMD_STATUS	7
> > > +#define CMD_START		0x40
> > > +#define CMD_WRITE_BYTE		0x41
> > > +#define CMD_READ_BYTE		0x42
> > > +#define STATUS_BUSY		0x80
> > > +
> > > +struct scr24x_dev {
> > > +	struct device *dev;
> > > +	struct cdev c_dev;
> > > +	unsigned char buf[CCID_MAX_LEN];
> > > +	int devno;
> > > +	struct mutex lock;
> > > +	struct kref refcnt;
> > > +	u8 __iomem *regs;
> > > +};
> > > +
> > > +#define SCR24X_DEVS 8
> > > +static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
> > > +
> > > +static struct class *scr24x_class;
> > > +dev_t scr24x_devt;
> > 
> > global variable?
> 
> Well, that's used to track the major number assigned on device init so
> that we can release it on exit. Is there a better place to store it?
> 
> A quick look at the other drivers suggests that they either do the
> same, or just store the major part. Would that be a better idea?

It's fine to have this variable, I was objecting to the fact that it is
now in the global namespace.  Use 'static' please...

> > And why do you need a char device for this type of
> > hardware?  Isn't there already an existing interface for this device
> > class?
> 
> (Arnd answered this better than I would do in his response to this
> message).

Yeah, that makes sense.  A bit messy though.

> > > +static void scr24x_delete(struct kref *kref)
> > > +{
> > > +	struct scr24x_dev *dev = container_of(kref, struct
> > > scr24x_dev,
> > > +								re
> > > fcnt);
> > > +
> > > +	kfree(dev);
> > > +}
> > > +
> > > +static int scr24x_wait_ready(struct scr24x_dev *dev)
> > > +{
> > > +	u_char status;
> > > +	int timeout = 100;
> > > +
> > > +	do {
> > > +		status = ioread8(dev->regs + SCR24X_CMD_STATUS);
> > > +		if (!(status & STATUS_BUSY))
> > > +			return 0;
> > > +
> > > +		msleep(20);
> > > +	} while (--timeout);
> > > +
> > > +	return -EIO;
> > > +}
> > > +
> > > +static int scr24x_open(struct inode *inode, struct file *filp)
> > > +{
> > > +	struct scr24x_dev *dev = container_of(inode->i_cdev,
> > > +				struct scr24x_dev, c_dev);
> > > +
> > > +	kref_get(&dev->refcnt);
> > > +	filp->private_data = dev;
> > > +
> > > +	return nonseekable_open(inode, filp);
> > > +}
> > > +
> > > +static int scr24x_release(struct inode *inode, struct file *filp)
> > > +{
> > > +	struct scr24x_dev *dev = filp->private_data;
> > > +
> > > +	kref_put(&dev->refcnt, scr24x_delete);
> > 
> > No locking?
> 
> The device instance keeps a reference as well. Thus when it drops to
> zero here, it means that the device itself was removed and
> unregistered. At that point there's no possibility of acquiring another
> reference to the device instance (via open()) and thus noone to lock
> against.
> 
> Unless I'm, of course, mistaken.

What is keeping someone else from calling open at this same point in
time to increment the count?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-16  9:57     ` Greg Kroah-Hartman
@ 2016-10-17 11:22       ` Lubomir Rintel
  2016-10-17 11:34         ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Lubomir Rintel @ 2016-10-17 11:22 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: linux-kernel, Arnd Bergmann, Dominik Brodowski

On Sun, 2016-10-16 at 11:57 +0200, Greg Kroah-Hartman wrote:
> On Sun, Oct 16, 2016 at 11:23:36AM +0200, Lubomir Rintel wrote:
> > 
> > Hi Greg,
> > 
> > (responses inline)
> > 
> > On Mon, 2016-10-10 at 19:33 +0200, Greg Kroah-Hartman wrote:
> > > 
> > > On Mon, Oct 10, 2016 at 05:58:15PM +0200, Lubomir Rintel wrote:
> > > > 
> > > > 
> > > > This implements only the very basic protocol "Mode A", just to make
> > > > the
> > > > device functional. Patches to implement "Mode C" that uses better
> > > > bulking
> > > > and is interrupt-driver may follow.
> > > > 
> > > > The device essentially speaks the same protocol as USB CCID devices
> > > > do over
> > > > the bulk endpoints. The driver exchanges the command submissions
> > > > and
> > > > responses over a plain read()/write() interface, compatible with
> > > > legacy
> > > > OpenCT's pcmcia_block driver.
> > > > 
> > > > Patches for the newer CCID driver are available:
> > > > https://github.com/lkundrak/CCID/tree/lr/pcmcia_block
> > > > 
> > > > > > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > > ---
> > > >  MAINTAINERS                     |   5 +
> > > >  drivers/char/pcmcia/Kconfig     |  11 ++
> > > >  drivers/char/pcmcia/Makefile    |   1 +
> > > >  drivers/char/pcmcia/scr24x_cs.c | 357
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 374 insertions(+)
> > > >  create mode 100644 drivers/char/pcmcia/scr24x_cs.c
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 5327bbe..8dc6a9f 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -10608,6 +10608,11 @@ W:	http://www.sunplus.com
> > > > > > > >  S:	Supported
> > > > > > > >  F:	arch/score/
> > > >  
> > > > +SCR24X CHIP CARD INTERFACE DRIVER
> > > > > > > > > > > > +M:	Lubomir Rintel <lkundrak@v3.sk>
> > > > > > > > +S:	Supported
> > > > > > > > +F:	drivers/char/pcmcia/scr24x_cs.c
> > > > +
> > > >  SYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
> > > > > > > > > > > >  M:	Sudeep Holla <sudeep.holla@arm.com>
> > > >  L:	linux-arm-kernel@lists.infradead.org
> > > > diff --git a/drivers/char/pcmcia/Kconfig
> > > > b/drivers/char/pcmcia/Kconfig
> > > > index 8d3dfb0..1d1e7da 100644
> > > > --- a/drivers/char/pcmcia/Kconfig
> > > > +++ b/drivers/char/pcmcia/Kconfig
> > > > @@ -43,6 +43,17 @@ config CARDMAN_4040
> > > > > > > > > > > >  	  (http://www.omnikey.com/), or a current development
> > > > version of OpenCT
> > > > > > > > > > > >  	  (http://www.opensc-project.org/opensc).
> > > >  
> > > > +config SCR24X
> > > > > > > > +	tristate "SCR24x Chip Card Interface support"
> > > > > > > > +	depends on PCMCIA
> > > > > > > > +	help
> > > > > > > > +	  Enable support for the SCR24x PCMCIA Chip Card
> > > > Interface.
> > > > +
> > > > > > > > +	  To compile this driver as a module, choose M here.
> > > > > > > > +	  The module will be called scr24x_cs..
> > > > +
> > > > > > > > +	  If unsure say N.
> > > 
> > > A new PCMCIA driver?  What decade is this?  :)
> > 
> > Not sure if you actually expect me to answer this :)
> > 
> > I guess it's still the decade we dislike running our hardware with
> > crappy out-of-tree drivers on a horribly outdated kernel.
> 
> Yes, fair enough, sorry I was just joking :)
> 
> > 
> > > 
> > > > 
> > > >  config IPWIRELESS
> > > > > > > >  	tristate "IPWireless 3G UMTS PCMCIA card support"
> > > > > > > >  	depends on PCMCIA && NETDEVICES && TTY
> > > > diff --git a/drivers/char/pcmcia/Makefile
> > > > b/drivers/char/pcmcia/Makefile
> > > > index 0aae209..5b836bc 100644
> > > > --- a/drivers/char/pcmcia/Makefile
> > > > +++ b/drivers/char/pcmcia/Makefile
> > > > @@ -7,3 +7,4 @@
> > > >  obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
> > > >  obj-$(CONFIG_CARDMAN_4000) += cm4000_cs.o
> > > >  obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
> > > > +obj-$(CONFIG_SCR24X) += scr24x_cs.o
> > > > diff --git a/drivers/char/pcmcia/scr24x_cs.c
> > > > b/drivers/char/pcmcia/scr24x_cs.c
> > > > new file mode 100644
> > > > index 0000000..24379ac
> > > > --- /dev/null
> > > > +++ b/drivers/char/pcmcia/scr24x_cs.c
> > > > @@ -0,0 +1,357 @@
> > > > +/*
> > > > + * SCR24x PCMCIA Smart Card Reader Driver
> > > > + *
> > > > + * Copyright (C) 2005-2006 TL Sudheendran
> > > > + * Copyright (C) 2016 Lubomir Rintel
> > > > + *
> > > > + * Derived from "scr24x_v4.2.6_Release.tar.gz" driver by TL
> > > > Sudheendran.
> > > > + *
> > > > + * 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, 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; see the file COPYING.  If not, write
> > > > to
> > > > + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA
> > > > 02139, USA.
> > > > + */
> > > > +
> > > > +#include <linux/device.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/cdev.h>
> > > > +
> > > > +#include <pcmcia/cistpl.h>
> > > > +#include <pcmcia/ds.h>
> > > > +
> > > > > > > > +#define CCID_HEADER_SIZE	10
> > > > > > > > +#define CCID_LENGTH_OFFSET	1
> > > > > > > > +#define CCID_MAX_LEN		271
> > > > +
> > > > > > > > +#define SCR24X_DATA(n)		(1 + n)
> > > > > > > > +#define SCR24X_CMD_STATUS	7
> > > > > > > > +#define CMD_START		0x40
> > > > > > > > +#define CMD_WRITE_BYTE		0x41
> > > > > > > > +#define CMD_READ_BYTE		0x42
> > > > > > > > +#define STATUS_BUSY		0x80
> > > > +
> > > > +struct scr24x_dev {
> > > > > > > > +	struct device *dev;
> > > > > > > > +	struct cdev c_dev;
> > > > > > > > +	unsigned char buf[CCID_MAX_LEN];
> > > > > > > > +	int devno;
> > > > > > > > +	struct mutex lock;
> > > > > > > > +	struct kref refcnt;
> > > > > > > > +	u8 __iomem *regs;
> > > > +};
> > > > +
> > > > +#define SCR24X_DEVS 8
> > > > +static DECLARE_BITMAP(scr24x_minors, SCR24X_DEVS);
> > > > +
> > > > +static struct class *scr24x_class;
> > > > +dev_t scr24x_devt;
> > > 
> > > global variable?
> > 
> > Well, that's used to track the major number assigned on device init so
> > that we can release it on exit. Is there a better place to store it?
> > 
> > A quick look at the other drivers suggests that they either do the
> > same, or just store the major part. Would that be a better idea?
> 
> It's fine to have this variable, I was objecting to the fact that it is
> now in the global namespace.  Use 'static' please...

Ah, okay. Will fix in v2.

> > > And why do you need a char device for this type of
> > > hardware?  Isn't there already an existing interface for this device
> > > class?
> > 
> > (Arnd answered this better than I would do in his response to this
> > message).
> 
> Yeah, that makes sense.  A bit messy though.

I guess that makes sense; the chip card readers tend to be rather
complex and the API (IFD Handler API) user space typically uses is
rather generic -- it would require quite some logic to interface with a
particular hardware.

Moreover, most of the present-day smart card readers are standard CCID
class USB devices, with the CCID to IFD done in userspace. There's some
serial readers too, attached via UARTs too that use the tty ioctls to
detect the card presence and set the baud rate, etc.

The rest (PCMCIA) seem to emulate either of the two. It still makes
sense to implement the IFD in userspace and I guess there's not too
much common ground for a device class here.

> > > > +static void scr24x_delete(struct kref *kref)
> > > > +{
> > > > > > > > +	struct scr24x_dev *dev = container_of(kref, struct
> > > > scr24x_dev,
> > > > > > > > +								re
> > > > fcnt);
> > > > +
> > > > > > > > +	kfree(dev);
> > > > +}
> > > > +
> > > > +static int scr24x_wait_ready(struct scr24x_dev *dev)
> > > > +{
> > > > > > > > +	u_char status;
> > > > > > > > +	int timeout = 100;
> > > > +
> > > > > > > > +	do {
> > > > > > > > +		status = ioread8(dev->regs + SCR24X_CMD_STATUS);
> > > > > > > > +		if (!(status & STATUS_BUSY))
> > > > > > > > +			return 0;
> > > > +
> > > > > > > > +		msleep(20);
> > > > > > > > +	} while (--timeout);
> > > > +
> > > > > > > > +	return -EIO;
> > > > +}
> > > > +
> > > > +static int scr24x_open(struct inode *inode, struct file *filp)
> > > > +{
> > > > > > > > +	struct scr24x_dev *dev = container_of(inode->i_cdev,
> > > > > > > > +				struct scr24x_dev, c_dev);
> > > > +
> > > > > > > > +	kref_get(&dev->refcnt);
> > > > > > > > +	filp->private_data = dev;
> > > > +
> > > > > > > > +	return nonseekable_open(inode, filp);
> > > > +}
> > > > +
> > > > +static int scr24x_release(struct inode *inode, struct file *filp)
> > > > +{
> > > > > > > > +	struct scr24x_dev *dev = filp->private_data;
> > > > +
> > > > > > > > +	kref_put(&dev->refcnt, scr24x_delete);
> > > 
> > > No locking?
> > 
> > The device instance keeps a reference as well. Thus when it drops to
> > zero here, it means that the device itself was removed and
> > unregistered. At that point there's no possibility of acquiring another
> > reference to the device instance (via open()) and thus noone to lock
> > against.
> > 
> > Unless I'm, of course, mistaken.
> 
> What is keeping someone else from calling open at this same point in
> time to increment the count?

Not sure if I understand you correctly. I guess you're not asking about
kref's counter itself, as that one is incremented/decremented
atomically?

The _remove() routine only releases the reference acquired at _probe()
time after the chrdev is unregiestered, thus no more open()s can occur.

> thanks,
> 
> greg k-h

PS: The build bot noticed that I fail to error-check the kmalloc() in
probe() and am missing a <slab.h> include. Will fix in v2 too.

Thank you,
Lubo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver
  2016-10-17 11:22       ` Lubomir Rintel
@ 2016-10-17 11:34         ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2016-10-17 11:34 UTC (permalink / raw)
  To: Lubomir Rintel; +Cc: Greg Kroah-Hartman, linux-kernel, Dominik Brodowski

On Monday, October 17, 2016 1:22:01 PM CEST Lubomir Rintel wrote:
> > > > And why do you need a char device for this type of
> > > > hardware?  Isn't there already an existing interface for this device
> > > > class?
> > > 
> > > (Arnd answered this better than I would do in his response to this
> > > message).
> > 
> > Yeah, that makes sense.  A bit messy though.
> 
> I guess that makes sense; the chip card readers tend to be rather
> complex and the API (IFD Handler API) user space typically uses is
> rather generic -- it would require quite some logic to interface with a
> particular hardware.
> 
> Moreover, most of the present-day smart card readers are standard CCID
> class USB devices, with the CCID to IFD done in userspace. There's some
> serial readers too, attached via UARTs too that use the tty ioctls to
> detect the card presence and set the baud rate, etc.
> 
> The rest (PCMCIA) seem to emulate either of the two. It still makes
> sense to implement the IFD in userspace and I guess there's not too
> much common ground for a device class here.

Well, you could in theory implement this as a tty driver
and be compatible with the serial port devices, see
drivers/tty/goldfish.c for an example driver.

Not sure if that actually improves things for you or not,
but the advantage is that there is an existing kernel
subsystem for it.

	Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-10-17 11:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-10 15:58 [PATCH] char/pcmcia: add scr24x_cs chip card interface driver Lubomir Rintel
2016-10-10 17:33 ` Greg Kroah-Hartman
2016-10-10 19:24   ` Arnd Bergmann
2016-10-16  9:23   ` Lubomir Rintel
2016-10-16  9:57     ` Greg Kroah-Hartman
2016-10-17 11:22       ` Lubomir Rintel
2016-10-17 11:34         ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.