Octavian Purdila wrote:
> On Wednesday 09 July 2008, Hinko Kocevar wrote:
>
>> This is second stab at the ussp driver.
>> I'm not sure about the TTY_NORMAL that was in port->tty->flip.flag_buf_ptr
>> under the USSP_READ - should this flag character be inserted into the flip
>> buffer after the data bytes, as seen in the patch?
>>
>
> From what I know, TTY_NORMAL refers to a regular, non-control character. But
> look at my comments for that section.
>
>> case TIOCSSOFTCAR:
>> - if ((rc = verify_area(VERIFY_READ, (void *) arg,
>> - sizeof(int))) == 0) {
>> - get_user(ival, (unsigned int *) arg);
>> - tty->termios->c_cflag =
>> - (tty->termios->c_cflag & ~CLOCAL) |
>> - (ival ? CLOCAL : 0);
>> - }
>> + get_user(ival, (unsigned int *) arg);
>
> You need to verify the return value. If not zero, then return -EFAULT.
>
Will do.
>> + case TCSETS:
>> + ussp_dprintk (DEBUG_IOCTL, "TCSETS -- what to do
>> here!?\n"); + break;
>> +
>
What does TCSETS do? Do I have to handle it? I've seen some errors logged in system log about this ioctl..
>> int rc;
>> struct ussp_operation op;
>> struct ussp_port *port;
>> + char *buff;
>> func_enter();
>>
>> port = filp->private_data;
>> @@ -1011,13 +1009,20 @@
>> break;
>> case USSP_READ:
>> ussp_dprintk (DEBUG_IOCTL, "USSP_READ %d\n", (int)op.len);
>> - rc = copy_from_user (port->tty->flip.char_buf_ptr, buf +
>> sizeof(struct ussp_operation), op.len);
>> - memset(port->tty->flip.flag_buf_ptr, TTY_NORMAL, op.len);
>
> Hmm, this original code does not make sense to me. First copying from
> userspace into the tty buffer, then zeroying it out?
Me sees copying the data from userspace to flip.char_buf_ptr DATA buffer and then zeroing the flip.flag_buf_ptr FLAG buffer!?
>
> Also, it looks like a proprietary ioctl, so maybe you could just remove it...
I'm not sure if I can do that. USSP_READ and USSP_WRITE are two mandatory ioctls that do most of the work when talking with the device.
>
>> - port->tty->flip.count += op.len;
>> - port->tty->flip.char_buf_ptr += op.len;
>> - port->tty->flip.flag_buf_ptr += op.len;
>> + buff = kmalloc((int)op.len, GFP_KERNEL);
>> + rc = copy_from_user (buff, buf + sizeof(struct
>> ussp_operation), op.len); + if (rc)
>> + {
>> + kfree(buff);
>> + return -ENOMEM;
>
> Usually its -EFAULT.
Fixed.
Thank you for the comments.
--
ČETRTA POT, d.o.o., Kranj
Planina 3
4000 Kranj
Slovenia, Europe
Tel. +386 (0) 4 280 66 03
E-mail: hinko.kocevar@xxxxxxxxxxxx
Http: www.cetrtapot.si
--
To unsubscribe from this list: send an email with
"unsubscribe kernelnewbies" to ecartis@xxxxxxxxxxxx
Please read the FAQ at http://kernelnewbies.org/FAQ
