linux-kernel - Re: [PATCH 1/2] Staging: comedi: add dyna

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4DF0A8B3.8090900@mev.co.uk>
Date:	Thu, 9 Jun 2011 12:04:19 +0100
From:	Ian Abbott <abbotti@....co.uk>
To:	"Prashant P. Shah" <pshah.mumbai@...il.com>
CC:	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Frank Mori Hess <fmhess@...rs.sourceforge.net>,
	Ian Abbott <ian.abbott@....co.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] Staging: comedi: add dyna_pci10xx driver

On 09/06/11 10:24, Prashant P. Shah wrote:
> For Dynalog PCI DAQ cards:
>   PCI-1050
> 
> Signed-off-by: Prashant P. Shah <pshah.mumbai@...il.com>

It looks mostly okay to me as long as you got the PCI device ID
allocated by PLX (which they'll happily do for their customers if you
tell them what product the chip is is part of), and didn't just pull it
out of a hat!

A small criticism is a slight overuse of udelay(10) calls in the driver.
 Are they all necessary?  (I can understand its use in the code that
waits for AI conversions to complete.)  In the more general cases, you
might want to look at configuring wait states in the PLX chip instead of
relying on software delays.  It doesn't matter too much though.

I have a few more specific comments on the code (not many!):

> +/* digital output bit interface */
> +static int dyna_pci10xx_do_insn_bits(struct comedi_device *dev,
> +                             struct comedi_subdevice *s,
> +                             struct comedi_insn *insn, unsigned int *data)
> +{
> +       if (insn->n != 2)
> +               return -EINVAL;
> +
> +       /* The insn data is a mask in data[0] and the new data
> +        * in data[1], each channel cooresponding to a bit.
> +        * s->state contains the previous write data
> +        */
> +
> +       if (data[0]) {
> +               down(&devpriv->sem);
> +               s->state &= ~data[0];
> +               s->state |= (data[0] & data[1]);
> +               smp_mb();
> +               outw_p(s->state, devpriv->BADR3);
> +               udelay(10);
> +               up(&devpriv->sem);
> +       }
> +
> +       /*
> +        * On return, data[1] contains the value of the digital
> +        * input and output lines. We just return the software copy of the
> +        * output values if it was a purely digital output subdevice.
> +        */
> +       data[1] = s->state;
> +
> +       return 2;
> +}

Since you've gone to the bother of using a semaphore here, it may be
better to grab it before the 'if (data[0])' and release it after the
'data[1] = s->state;'.  Then data[1] will definitely be the value that
was written to the outputs by this ioctl.

It would be better to use a mutex than a semaphore for the reasons given
in Documentation/mutex-design.txt.

> +static int dyna_pci10xx_detach(struct comedi_device *dev)
> +{
> +       if (devpriv->pci_dev)
> +               comedi_pci_disable(devpriv->pci_dev);
> +
> +       return 0;
> +}

devpriv may be NULL when _detach is called, so you need to replace the
above test with 'if (devpriv && devpriv->pci_dev)' to avoid a possible
kernel oops.

Best regards,
Ian.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/