Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver

[Lubomir Rintel]

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@xxxxx>
> > ---
> > Â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@xxxxx>
> > +S:	Supported
> > +F:	drivers/char/pcmcia/scr24x_cs.c
> > +
> > ÂSYSTEM CONTROL & POWER INTERFACE (SCPI) Message Protocol drivers
> > ÂM:	Sudeep Holla <sudeep.holla@xxxxxxx>
> > ÂL:	linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > 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