Re: ioctl cleanups: enable sg_io and serial stuff to be shared

Pavel Machek
2003-05-07T15:05:32Z

Hi!

> > +typedef struct sg_io_hdr32 {
> > + s32 interface_id; /* [i] 'S' for SCSI generic (required) */
> > + s32 dxfer_direction; /* [i] data transfer direction */
> > + u8 cmd_len; /* [i] SCSI command length ( <= 16 bytes) */
> > + u8 mx_sb_len; /* [i] max length to write to sbp */
> > + u16 iovec_count; /* [i] 0 implies no scatter gather */
> > + u32 dxfer_len; /* [i] byte count of data transfer */
> > + u32 dxferp; /* [i], [*io] points to data transfer memory
> > + or scatter gather list */
> > + u32 cmdp; /* [i], [*i] points to command to perform */
> > + u32 sbp; /* [i], [*o] points to sense_buffer memory */
> > + u32 timeout; /* [i] MAX_UINT->no timeout (unit: millisec) */
> > + u32 flags; /* [i] 0 -> default, see SG_FLAG... */
> > + s32 pack_id; /* [i->o] unused internally (normally) */
> > + u32 usr_ptr; /* [i->o] unused internally */
> > + u8 status; /* [o] scsi status */
> > + u8 masked_status; /* [o] shifted, masked scsi status */
> > + u8 msg_status; /* [o] messaging level data (optional) */
> > + u8 sb_len_wr; /* [o] byte count actually written to sbp */
> > + u16 host_status; /* [o] errors from host adapter */
> > + u16 driver_status; /* [o] errors from software driver */
> > + s32 resid; /* [o] dxfer_len - actual_transferred */
> > + u32 duration; /* [o] time taken by cmd (unit: millisec) */
> > + u32 info; /* [o] auxiliary information */
> > +} sg_io_hdr32_t; /* 64 bytes long (on sparc32) */
> > +
> > +typedef struct sg_iovec32 {
> > + u32 iov_base;
> > + u32 iov_len;
> > +} sg_iovec32_t;
>
> These should better be expressed with compat_uptr_t, compat_ulong_t etc.
>
>
> > + sg_iovec32_t *uiov = (sg_iovec32_t *) compat_ptr(uptr32);
>
> > + if (verify_area(VERIFY_WRITE, (void *)compat_ptr(iov_base32),
> > kiov->iov_len)) + return -EFAULT;
> > + kiov->iov_base = (void *)compat_ptr(iov_base32);
>
> You don't need to cast to a pointer when using compat_ptr.

I thought so. I originally wanted as clean move as possible. I'll fix it.

> > +static int __init init_compat(void)
> > +{
> > + register_ioctl32_conversion(SG_IO, sg_ioctl_trans);
> > + return 0;
> > +}
> > +
> > +__initcall(init_compat);
> > +#endif
> > +
> > #define FORMAT_UNIT_TIMEOUT (2 * 60 * 60 * HZ)
> > #define START_STOP_TIMEOUT (60 * HZ)
> > #define MOVE_MEDIUM_TIMEOUT (5 * 60 * HZ)
>
> Has anyone solved the register_ioctl32_conversion() from module problem
> yet? The patch will break if you build scsi as a module because you
> never unregister the conversion helper on unload.
> Even if you do the unregister from a module_exit() function, there
> will still be a small race against running ioctl handlers. I suppose
> we have to add an 'owner' field to struct ioctl_trans in order to
> get it right.

Its in drivers/block/scsi_ioctl.c. AFAICS, its always compiled in, so
I'm not hitting that problem *yet*.

> > +#ifdef CONFIG_COMPAT
> > +struct serial_struct32 {
> > + int type;
> > + int line;
> > + unsigned int port;
> > + int irq;
> > + int flags;
> > + int xmit_fifo_size;
> > + int custom_divisor;
> > + int baud_base;
> > + unsigned short close_delay;
> > + char io_type;
> > + char reserved_char[1];
> > + int hub6;
> > + unsigned short closing_wait; /* time to wait before closing */
> > + unsigned short closing_wait2; /* no longer used... */
> > + __u32 iomem_base;
> > + unsigned short iomem_reg_shift;
> > + unsigned int port_high;
> > + int reserved[1];
> > +};
> see above.
>
> > + ss.iomem_base = (void *)((unsigned long)ss.iomem_base & 0xffffffff);
> you need compat_ptr() for iomem_base as well

Its not pointer, AFAICS (at least it can not be dereferenced by
userspace).
Pavel
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]