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

[Pavel Machek]

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?]