Messages in this thread |  | | Date | Sun, 16 Oct 2016 11:57:19 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] char/pcmcia: add scr24x_cs chip card interface driver |
| |
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
|  |