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

[Greg Kroah-Hartman]

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

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