Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] New: Omnikey CardMan 4040 PCMCIA Driver

32 views
Skip to first unread message

Harald Welte

unread,
Sep 3, 2005, 5:10:17 PM9/3/05
to Linux Kernel Mailinglist
Hi!

Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
Smartcard Reader.

It's based on some source code originally made available by the vendor
(as BSD/GPL dual licensed code), but has undergone significant changes
to make it more compliant with the general kernel community coding
practise.

As this is the first PCMCIA driver that I'm involved in, please let me
know if I missed something.

If there are no objections, I'd like to see it included in mainline.
Thanks!

--
- Harald Welte <laf...@gnumonks.org> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)

cm4040-driver.patch

Chase Venters

unread,
Sep 3, 2005, 5:27:27 PM9/3/05
to Harald Welte, linux-...@vger.kernel.org
> Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> Smartcard Reader.

Someone correct me if I'm wrong, but wouldn't these #defines be a problem with
the new HZ flexibility:

#define CCID_DRIVER_BULK_DEFAULT_TIMEOUT (150*HZ)
#define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
#define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
#define READ_WRITE_BUFFER_SIZE 512
#define POLL_LOOP_COUNT 1000

/* how often to poll for fifo status change */
#define POLL_PERIOD (HZ/100)

In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0.
Your later calls to mod_timer would be setting cmx_poll_timer to the current
value of jiffies.

Also, you've got a typo in the comments:

* - adhere to linux kenrel coding style and policies

Forgive me if I'm way off - I'm just now getting my feet wet in kernel
development. Just making comments based on what I (think) I know at this
point.

Best Regards,
Chase Venters
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Harald Welte

unread,
Sep 3, 2005, 5:34:30 PM9/3/05
to Linux Kernel Mailinglist
On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> Hi!
>
> Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> Smartcard Reader.

Sorry, the patch was missing a "cg-add" of the header file. Please use
the patch below.

cm4040-driver.patch

Alexey Dobriyan

unread,
Sep 3, 2005, 5:50:56 PM9/3/05
to Harald Welte, linux-...@vger.kernel.org
On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> Smartcard Reader.

> --- /dev/null
> +++ b/drivers/char/pcmcia/cm4040_cs.c

> +#include <linux/config.h>

Not needed.

> +static volatile char *version =

Can we lose all volatile and register keywords?

> +typedef struct reader_dev_t {
> + dev_link_t link;
> + dev_node_t node;
> + wait_queue_head_t devq;
> +
> + wait_queue_head_t poll_wait;
> + wait_queue_head_t read_wait;
> + wait_queue_head_t write_wait;
> +
> + unsigned int buffer_status;
> +
> + unsigned int fTimerExpired;
> + struct timer_list timer;
> + unsigned long timeout;
> + unsigned char sBuf[READ_WRITE_BUFFER_SIZE];
> + unsigned char rBuf[READ_WRITE_BUFFER_SIZE];
> + struct task_struct *owner;
> +} reader_dev_t;

And typedefs too.

struct reader_dev {

};

> +static ssize_t cmx_read(struct file *filp,char *buf,size_t count,loff_t *ppos)

char __user *buf

> + ulBytesToRead = 5 +
> + (0x000000FF&((char)dev->rBuf[1])) +
> + (0x0000FF00&((char)dev->rBuf[2] << 8)) +
> + (0x00FF0000&((char)dev->rBuf[3] << 16)) +
> + (0xFF000000&((char)dev->rBuf[4] << 24));

ulBytesToRead = 5 + le32_to_cpu(*(__le32 *)&dev->rBuf[1]);

> + ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5);

ulMin = min(count, ulBytesToRead + 5);

> + copy_to_user(buf, dev->rBuf, ulMin);

Can fail.

> +static ssize_t cmx_write(struct file *filp,const char *buf,size_t count,

const char __user *buf

> + loff_t *ppos)

> + copy_from_user(dev->sBuf, buf, uiBytesToWrite);

Can fail.

> +static int cmx_ioctl(struct inode *inode,struct file *filp,unsigned int cmd,
> + unsigned long arg)
> +{
> + dev_link_t *link;
> + int rc, size;
> +
> + link=dev_table[MINOR(inode->i_rdev)];
> + if (!(DEV_OK(link))) {
> + DEBUG(4, "DEV_OK false\n");
> + return -ENODEV;
> + }
> + if (_IOC_TYPE(cmd)!=CM_IOC_MAGIC) {
> + DEBUG(4,"ioctype mismatch\n");
> + return -EINVAL;
> + }
> + if (_IOC_NR(cmd)>CM_IOC_MAXNR) {
> + DEBUG(4,"iocnr mismatch\n");
> + return -EINVAL;
> + }
> + size = _IOC_SIZE(cmd);
> + rc = 0;
> + DEBUG(4,"iocdir=%.4x iocr=%.4x iocw=%.4x iocsize=%d cmd=%.4x\n",
> + _IOC_DIR(cmd),_IOC_READ,_IOC_WRITE,size,cmd);
> +
> + if (_IOC_DIR(cmd)&_IOC_READ) {
> + if (!access_ok(VERIFY_WRITE, (void *)arg, size))
> + return -EFAULT;
> + }
> + if (_IOC_DIR(cmd)&_IOC_WRITE) {
> + if (!access_ok(VERIFY_READ, (void *)arg, size))
> + return -EFAULT;
> + }
> +
> + return rc;
> +}

Whoo, empty ioctl handler.

> +static void reader_release(u_long arg)

> + link = (dev_link_t *)arg;

You do

reader_release((unsigned long)link);

somewhere above and below.

> +static void reader_detach_by_devno(int devno,dev_link_t *link)

> + reader_release((u_long)link);

Like this.

Nish Aravamudan

unread,
Sep 3, 2005, 6:15:14 PM9/3/05
to Chase Venters, Harald Welte, linux-...@vger.kernel.org
On 9/3/05, Chase Venters <chase....@clientec.com> wrote:
> > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > Smartcard Reader.
>
> Someone correct me if I'm wrong, but wouldn't these #defines be a problem
> with the new HZ flexibility:
>
> #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT (150*HZ)
> #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
> #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
> #define READ_WRITE_BUFFER_SIZE 512
> #define POLL_LOOP_COUNT 1000

These are all fine. Although I am a bit suspicious of 150 second
timeouts; but if that is the hardware...

> /* how often to poll for fifo status change */
> #define POLL_PERIOD (HZ/100)

This needs to be msecs_to_jiffies(10), please.

> In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0.

Um, 100/100 = 1, not 0?

> Your later calls to mod_timer would be setting cmx_poll_timer to the current
> value of jiffies.

Which is technically ok, because HZ=100, a
jiffies + 0
or
jiffies + 1
timeout request will both result in the soft-timer being expired at
the *next* timer interrupt. Regardless, you're right, and
msecs_to_jiffies() will cover it.

> Also, you've got a typo in the comments:
>
> * - adhere to linux kenrel coding style and policies
>
> Forgive me if I'm way off - I'm just now getting my feet wet in kernel
> development. Just making comments based on what I (think) I know at this
> point.

Of bigger concern to me is the use of the sleep_on() family of
functions, all of which are deprecated.

Thanks,
Nish

Chase Venters

unread,
Sep 3, 2005, 6:24:00 PM9/3/05
to nish.ar...@gmail.com, linux-...@vger.kernel.org
> Um, 100/100 = 1, not 0?

Oh my... it's been a long day.

Regards,
Chase Venters

Jesper Juhl

unread,
Sep 3, 2005, 6:27:45 PM9/3/05
to Harald Welte, Linux Kernel Mailinglist
On 9/4/05, Harald Welte <laf...@gnumonks.org> wrote:
> On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> > Hi!
> >
> > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > Smartcard Reader.
>
> Sorry, the patch was missing a "cg-add" of the header file. Please use
> the patch below.

It would be so much nicer if the patch actually was "below" - that is
"inline in the email as opposed to as an attachment". Having to first
save an attachment and then cut'n'paste from it is a pain.

Anyway, a few comments below :

+#define DEBUG(n, x, args...) do { if (pc_debug >= (n)) \


line longer than 80 chars. Please adhere to CodingStyle and keep lines
<80 chars.
There's more than one occourance of this.

+static inline int cmx_waitForBulkOutReady(reader_dev_t *dev)


Why TheStudlyCaps ? Please keep function names lowercase. There are
more instances of this, only pointing out one.

+ register int i;

+ register int iobase = dev->link.io.BasePort1;


Please use only tabs for indentation (line 1 of the above is indented
with spaces).

+ for (i=0; i < POLL_LOOP_COUNT; i++) {


for (i = 0; i < POLL_LOOP_COUNT; i++) {

+ if (rc != 1)


Again spaces used for indentation, please fix all that up to use tabs.

+ unsigned long ulBytesToRead;


lowercase prefered also for variables.

+ for (i=0; i<5; i++) {


for (i = 0; i < 5; i++) {

+ DEBUG(5,"cmx_waitForBulkInReady rc=%.2x\n",rc);


Space after ","s please : DEBUG(5, "cmx_waitForBulkInReady rc=%.2x\n", rc);

+ ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5);


needs spaces :
ulMin = (count < (ulBytesToRead + 5)) ? count : (ulBytesToRead + 5);

+ reader_dev_t *dev=(reader_dev_t *)filp->private_data;


reader_dev_t *dev = (reader_dev_t *)filp->private_data;


+static int cmx_open (struct inode *inode, struct file *filp)


get rid of the space before the opening paren :
static int cmx_open(struct inode *inode, struct file *filp)


+ for (rc = pcmcia_get_first_tuple(handle, &tuple);

+ rc == CS_SUCCESS;

+ rc = pcmcia_get_next_tuple(handle, &tuple)) {

..
+ if (parse.cftable_entry.io.nwin) {

+ link->io.BasePort1 = parse.cftable_entry.io.win[0].base;

+ link->io.NumPorts1 = parse.cftable_entry.io.win[0].len;

+ link->io.Attributes1 = IO_DATA_PATH_WIDTH_AUTO;

+ if(!(parse.cftable_entry.io.flags & CISTPL_IO_8BIT))

+ link->io.Attributes1 = IO_DATA_PATH_WIDTH_16;

..
+ }
+ }

How about not having to indent so deep by rewriting that as

for (rc = pcmcia_get_first_tuple(handle, &tuple);
rc == CS_SUCCESS;
rc = pcmcia_get_next_tuple(handle, &tuple)) {
..
if (!parse.cftable_entry.io.nwin)
continue;

link->io.BasePort1 = parse.cftable_entry.io.win[0].base;
link->io.NumPorts1 = parse.cftable_entry.io.win[0].len;
link->io.Attributes1 = IO_DATA_PATH_WIDTH_AUTO;
if(!(parse.cftable_entry.io.flags & CISTPL_IO_8BIT))
link->io.Attributes1 = IO_DATA_PATH_WIDTH_16;
..
}


+ link->conf.IntType = 00000002;


more spaces used for indentation. Not going to point out any more of these.

+ cmx_poll_timer.function = &cmx_do_poll;


shouldn't this be
cmx_poll_timer.function = cmx_do_poll;
???

+ int i;

+ DEBUG(3, "-> reader_detach(link=%p\n", link);


please have a blank line between variable declarations and other statements.

--
Jesper Juhl <jespe...@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

Harald Welte

unread,
Sep 4, 2005, 3:14:04 AM9/4/05
to Alexey Dobriyan, linux-...@vger.kernel.org
Thanks for your comments, Alexey.

I've now incorprorated all of the requested changes and am testing the
driver. If everything is still fine, I'll repost later today.

Harald Welte

unread,
Sep 4, 2005, 3:34:29 AM9/4/05
to Nish Aravamudan, Chase Venters, linux-...@vger.kernel.org
Hi Nish,

thanks for your comments.

On Sat, Sep 03, 2005 at 03:13:43PM -0700, Nish Aravamudan wrote:
> On 9/3/05, Chase Venters <chase....@clientec.com> wrote:
> > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > > Smartcard Reader.
> >

> > #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT (150*HZ)


>
> These are all fine. Although I am a bit suspicious of 150 second
> timeouts; but if that is the hardware...

That's a definition from the original vendor-supplied driver.
Unfortunately there's no hardware documentation, so I can't verify it.
But generally speaking, serial smart cards can really be slow, so I
think it could make sense.

> > /* how often to poll for fifo status change */
> > #define POLL_PERIOD (HZ/100)
>
> This needs to be msecs_to_jiffies(10), please.

thanks, changed in my local tree now.

> Of bigger concern to me is the use of the sleep_on() family of
> functions, all of which are deprecated.

Ok, I'm working on replacing the respective code with
wait_event_interruptible_timeout().

Ingo Oeser

unread,
Sep 4, 2005, 9:02:26 AM9/4/05
to Harald Welte, Linux Kernel Mailinglist
On Sunday 04 September 2005 12:12, Harald Welte wrote:
> cmx_llseek

just use

return nonseekable_open(inode, filp);

as your last statement in cmx_open() instead of

return 0;

to really disable any file pointer positioning (e.g. pwrite/pread too).

Addtionally cmx_llseek() is implement already as "no_llseek()"
by the VFS, so you delete it from the driver an use no_llseek() from
the VFS instead.


Regards

Ingo Oeser

Horst von Brand

unread,
Sep 4, 2005, 6:01:58 PM9/4/05
to Jesper Juhl, Harald Welte, Linux Kernel Mailinglist
Jesper Juhl <jespe...@gmail.com> wrote:
> On 9/4/05, Harald Welte <laf...@gnumonks.org> wrote:
> > On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> > > Hi!
> > >
> > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > > Smartcard Reader.
> >
> > Sorry, the patch was missing a "cg-add" of the header file. Please use
> > the patch below.
>
> It would be so much nicer if the patch actually was "below" - that is
> "inline in the email as opposed to as an attachment". Having to first
> save an attachment and then cut'n'paste from it is a pain.
>
> Anyway, a few comments below :

[...]

> + unsigned long ulBytesToRead;
>
>
> lowercase prefered also for variables.

Also, "encoding" the type (ul) into the variable name is nonsense.

[...]

> + ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5);

Again.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513

Jesper Juhl

unread,
Sep 4, 2005, 6:10:55 PM9/4/05
to Horst von Brand, Harald Welte, Linux Kernel Mailinglist
On 9/4/05, Horst von Brand <vonb...@inf.utfsm.cl> wrote:
> Jesper Juhl <jespe...@gmail.com> wrote:
> > On 9/4/05, Harald Welte <laf...@gnumonks.org> wrote:
> > > On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> > > > Hi!
> > > >
> > > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > > > Smartcard Reader.
> > >
> > > Sorry, the patch was missing a "cg-add" of the header file. Please use
> > > the patch below.
> >
> > It would be so much nicer if the patch actually was "below" - that is
> > "inline in the email as opposed to as an attachment". Having to first
> > save an attachment and then cut'n'paste from it is a pain.
> >
> > Anyway, a few comments below :
>
> [...]
>
> > + unsigned long ulBytesToRead;
> >
> >
> > lowercase prefered also for variables.
>
> Also, "encoding" the type (ul) into the variable name is nonsense.
>
Agreed, and it's even mentioned in CodingStyle (ok, it talks about
functions, but the same goes for variables):

..
Encoding the type of a function into the name (so-called Hungarian
notation) is brain damaged - the compiler knows the types anyway and can
check those, and it only confuses the programmer. No wonder MicroSoft
makes buggy programs.

LOCAL variable names should be short, and to the point. If you have
some random integer loop counter, it should probably be called "i".
Calling it "loop_counter" is non-productive, if there is no chance of it
being mis-understood. Similarly, "tmp" can be just about any type of
variable that is used to hold a temporary value.
..

Harald Welte

unread,
Sep 5, 2005, 5:14:36 AM9/5/05
to Ingo Oeser, Linux Kernel Mailinglist
On Sun, Sep 04, 2005 at 02:58:27PM +0200, Ingo Oeser wrote:

> just use
> return nonseekable_open(inode, filp);
>

> to really disable any file pointer positioning (e.g. pwrite/pread too).
>
> Addtionally cmx_llseek() is implement already as "no_llseek()"
> by the VFS, so you delete it from the driver an use no_llseek() from
> the VFS instead.

great, thanks, I've merged your suggested changes into my local tree.
Stay tuned for a re-submit later today.

Harald Welte

unread,
Sep 5, 2005, 6:32:15 AM9/5/05
to Jesper Juhl, Linux Kernel Mailinglist
On Sun, Sep 04, 2005 at 12:27:20AM +0200, Jesper Juhl wrote:
> On 9/4/05, Harald Welte <laf...@gnumonks.org> wrote:
> > On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> > > Hi!
> > >
> > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > > Smartcard Reader.
> >
> > Sorry, the patch was missing a "cg-add" of the header file. Please use
> > the patch below.
>
> It would be so much nicer if the patch actually was "below" - that is
> "inline in the email as opposed to as an attachment". Having to first
> save an attachment and then cut'n'paste from it is a pain.

This is a neverending discussion. A number of kernel develpoers I know
prefer attachements these days. It's a matter of your email client, and
why it doesn't just append a "plaintext" attachment at the bottom of
your mail and rather display you the "save as" icon/button/whatever.

> Anyway, a few comments below :
>
> +#define DEBUG(n, x, args...) do { if (pc_debug >= (n)) \
>
>
> line longer than 80 chars. Please adhere to CodingStyle and keep lines
> <80 chars.

The line is _not_ longer than 80 chars, at least not if you remove the
"+" from the beginning of the patch and you hve 8 chars wide

> There's more than one occourance of this.

there was one occurrence in the file which I have missed, thanks.

> +static inline int cmx_waitForBulkOutReady(reader_dev_t *dev)
>
>
> Why TheStudlyCaps ? Please keep function names lowercase. There are
> more instances of this, only pointing out one.

Yes, there are. My initial idea was to diverge as little as possible
from the original vendor driver, making it easy to pull in changes from
their tree in the future, should it be neccessarry.

However, it has diverged that much now, it doesn't really matter whether
I also rename the functions, too. Please stay tuned for the next
revision of the patch.

> Please use only tabs for indentation (line 1 of the above is indented
> with spaces).

thanks, should have catched all of them now.

> lowercase prefered also for variables.

done

> Space after ","s please : DEBUG(5, "cmx_waitForBulkInReady rc=%.2x\n", rc);

done

> get rid of the space before the opening paren :
> static int cmx_open(struct inode *inode, struct file *filp)

done

> How about not having to indent so deep by rewriting that as

done

> + cmx_poll_timer.function = &cmx_do_poll;
>
> shouldn't this be
> cmx_poll_timer.function = cmx_do_poll;
> ???

I don't really know what difference it would make. My understanding of
'c' is that both cases would take the address of the function.

My personal taste is rather to explicitly indicate this with an '&' than
let the compiler implicitly take the address.

Harald Welte

unread,
Sep 5, 2005, 12:39:40 PM9/5/05
to Chase Venters, linux-...@vger.kernel.org
On Sat, Sep 03, 2005 at 04:27:00PM -0500, Chase Venters wrote:
> > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > Smartcard Reader.
>
> Someone correct me if I'm wrong, but wouldn't these #defines be a problem with
> the new HZ flexibility:
>
> #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT (150*HZ)
> #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
> #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)

The defines above certainly have no problems. They want to wait for
multiples of seconds.

> /* how often to poll for fifo status change */
> #define POLL_PERIOD (HZ/100)
>
> In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0.
> Your later calls to mod_timer would be setting cmx_poll_timer to the current
> value of jiffies.

100/100 == 1. As far as my limited math skills go, only 0 divided by
something can give a result of zero ;)

So yes, the code would poll every 1/100th of a second, even with HZ=100.

Obviously, if HZ would ever go below 100, the code above would provide
some problems. I'm not sure what the future plans with HZ are, but I'll
add an #error statement in case HZ goes smaller than that.

> Also, you've got a typo in the comments:

thanks.

Roland Dreier

unread,
Sep 6, 2005, 12:15:57 PM9/6/05
to Harald Welte, Chase Venters, linux-...@vger.kernel.org
Harald> Obviously, if HZ would ever go below 100, the code above
Harald> would provide some problems. I'm not sure what the future
Harald> plans with HZ are, but I'll add an #error statement in
Harald> case HZ goes smaller than that.

It might be simpler just to define it to msecs_to_jiffies(10).

- R.

Harald Welte

unread,
Sep 6, 2005, 1:11:48 PM9/6/05
to Roland Dreier, Chase Venters, linux-...@vger.kernel.org
On Tue, Sep 06, 2005 at 09:15:10AM -0700, Roland Dreier wrote:
> Harald> Obviously, if HZ would ever go below 100, the code above
> Harald> would provide some problems. I'm not sure what the future
> Harald> plans with HZ are, but I'll add an #error statement in
> Harald> case HZ goes smaller than that.
>
> It might be simpler just to define it to msecs_to_jiffies(10).

That's what I did in the last version that was posted to lkml ;)

Rahul Mohan G

unread,
Nov 29, 2022, 1:56:24 AM11/29/22
to
Very old thread, sorry. But any idea about the version of the BSD license here (BSD/GPL dual licensed code)?
0 new messages