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

[Lubomir Rintel]

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@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.
> 
> 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