On Thu, 2002-11-07 at 16:09, Thomas Wahrenbruch wrote:
> Because this is my first Linux driver, I don't think that it's perfect...
> (especially the kobil_write method...) But it worked very well on my machine.
Its a lot better than most "my first driver"'s
> +
> + // allocate memory for transfer buffer
> + transfer_buffer = (unsigned char *) kmalloc(transfer_buffer_length *
>sizeof(char), GFP_KERNEL);
> + if (! transfer_buffer) {
> + return (-1);
> + } else {
> + memset(transfer_buffer, 0, transfer_buffer_length);
> + }
> +
> + // allocate write_urb
> + if (!port->write_urb) {
> + dbg( __FUNCTION__ " Allocating port->write_urb");
> + port->write_urb = usb_alloc_urb(0);
> + if (!port->write_urb) {
> + dbg( __FUNCTION__ " usb_alloc_urb failed");
But you dont free the transfer buffer..
> + return (-1);
> + }
> + }
> +
> + // allocate memory for write_urb transfer buffer
> + port->write_urb->transfer_buffer = (unsigned char *)
>kmalloc(write_urb_transfer_buffer_length * sizeof(char), GFP_KERNEL);
> + if (! port->write_urb->transfer_buffer) {
And here you don't free the write urb or main transfer buffer
> +static int kobil_write (struct usb_serial_port *port, int from_user,
> + const unsigned char *buf, int count)
> +{
> + int length = 0;
> + int result = 0;
> + int todo = 0;
> + struct kobil_private * priv;
> +
> + if (count == 0) {
> + dbg( __FUNCTION__ " write request of 0 bytes");
> + return (0);
> + }
> +
> + if (port->write_urb->status == -EINPROGRESS) {
> + dbg( __FUNCTION__ " Already writing");
> + return (0);
0 is end of file rather than writing. I'd expect the code here to be a
while loop with a sleep that exited when the urb became free or if you
said O_NDELAY
> + }
> +
> + if (count > KOBIL_BUF_LENGTH) {
> + dbg( __FUNCTION__ " Error: write request bigger than buffer size");
> + return (0);
> + }
Much better to do
count = min(count, KOBIL_BUF_LENGTH);
then the write will be done piece by piece. If you error it 0 is EOF not
an error
> +
> + down (&port->sem);
But by now your check for write_urb->status is not valid on an SMP box
or with pre-emption. Don't you need to take the semaphore first. Or
would it not be simpler to take the semaphore to use the URB and drop it
when the URB ceases to be in progress (down in an irq path is naughty up
is just fine)
That would remove the write status race, the delay handling and turn it
all into a single if(down_interruptible())
[Aside - there is an old saying in SMP OS design "lock data not code" -
what that really is trying to say is that locking is about resources and
control of the resource not about which bits of code require locks]
> +
> + case TIOCMGET: // 0x5415
> + // allocate memory for transfer buffer
> + transfer_buffer = (unsigned char *) kmalloc(transfer_buffer_length *
>sizeof(char), GFP_KERNEL);
> + if (! transfer_buffer) {
> + return (-1);
You want to return a proper error code (I guess -ENOBUFS)
> + transfer_buffer = (unsigned char *) kmalloc(transfer_buffer_length *
>sizeof(char), GFP_KERNEL);
sizeof(char) is one.
Hope these comments help
Alan
-------------------------------------------------------
This sf.net email is sponsored by: See the NEW Palm
Tungsten T handheld. Power & Color in a compact size!
http://ads.sourceforge.net/cgi-bin/redirect.pl?palm0001en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
