block/bsg.c

block/bsg.c

[Andrew Morton]

A belated review (I've never seen this before and there it is in mainline)

> static char bsg_version[] = "block layer sg (bsg) 0.4";

`const' would be better.  That moves it into a write-protected memory section.

> struct bsg_device {
> 	request_queue_t *queue;
> 	spinlock_t lock;
> 	struct list_head busy_list;
> 	struct list_head done_list;
> 	struct hlist_node dev_list;
> 	atomic_t ref_count;
> 	int minor;
> 	int queued_cmds;
> 	int done_cmds;
> 	wait_queue_head_t wq_done;
> 	wait_queue_head_t wq_free;
> 	char name[BUS_ID_SIZE];
> 	int max_queue;
> 	unsigned long flags;
> };

Please try to comment your data structures.  This is key to allowing others
to understand your code.

> #undef BSG_DEBUG
> 
> #ifdef BSG_DEBUG
> #define dprintk(fmt, args...) printk(KERN_ERR "%s: " fmt, __FUNCTION__, ##args)
> #else
> #define dprintk(fmt, args...)
> #endif

hm, that seems to be out 187th implementation of dprintk().

> #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)

This makes the code easier to write but harder to read.  We should optimise
for readers.  Please open-code this at callsites.

Or at least convert it into a (commented) (possibly inlined) C function.

> /*
>  * just for testing
>  */
> #define BSG_MAJOR	(240)

What's this doing in mainline?  240 is a "reserved for local use" major. 
This will cause collisions.  This code should be using dynamic major
assignment.

> static DEFINE_MUTEX(bsg_mutex);
> static int bsg_device_nr, bsg_minor_idx;
> 
> #define BSG_LIST_SIZE	(8)

afacit this isn't really the size of a list.  It has something to do with
the number of minors which are attached to that illegitimate major?  A
comment here would help.  Perhaps this name is poorly chosen.

> #define bsg_list_idx(minor)	((minor) & (BSG_LIST_SIZE - 1))

Please prefer to write code in C, not in cpp.

> static struct hlist_head bsg_device_list[BSG_LIST_SIZE];

That is an array, not a list.

> static struct class *bsg_class;
> static LIST_HEAD(bsg_class_list);
> 
> static struct kmem_cache *bsg_cmd_cachep;

How many of these items do we expect to be simultaneously allocated?  If
that number is small then a custom kmem_cache is probably not warranted.

> /*
>  * our internal command type
>  */
> struct bsg_command {
> 	struct bsg_device *bd;
> 	struct list_head list;
> 	struct request *rq;
> 	struct bio *bio;
> 	struct bio *bidi_bio;
> 	int err;
> 	struct sg_io_v4 hdr;
> 	struct sg_io_v4 __user *uhdr;
> 	char sense[SCSI_SENSE_BUFFERSIZE];
> };

Comments here, please.

> static void bsg_free_command(struct bsg_command *bc)
> {
> 	struct bsg_device *bd = bc->bd;
> 	unsigned long flags;
> 
> 	kmem_cache_free(bsg_cmd_cachep, bc);
> 
> 	spin_lock_irqsave(&bd->lock, flags);
> 	bd->queued_cmds--;
> 	spin_unlock_irqrestore(&bd->lock, flags);
> 
> 	wake_up(&bd->wq_free);
> }
> 
> static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
> {
> 	struct bsg_command *bc = ERR_PTR(-EINVAL);
> 
> 	spin_lock_irq(&bd->lock);
> 
> 	if (bd->queued_cmds >= bd->max_queue)
> 		goto out;
> 
> 	bd->queued_cmds++;
> 	spin_unlock_irq(&bd->lock);
> 
> 	bc = kmem_cache_alloc(bsg_cmd_cachep, GFP_USER);

This should be GFP_KERNEL.

> 	if (unlikely(!bc)) {
> 		spin_lock_irq(&bd->lock);
> 		bd->queued_cmds--;
> 		bc = ERR_PTR(-ENOMEM);
> 		goto out;
> 	}
> 
> 	memset(bc, 0, sizeof(*bc));

Use kmem_cache_zalloc() above, remove this.

> 	bc->bd = bd;
> 	INIT_LIST_HEAD(&bc->list);
> 	dprintk("%s: returning free cmd %p\n", bd->name, bc);
> 	return bc;
> out:
> 	spin_unlock_irq(&bd->lock);
> 	return bc;
> }
> 
> static inline void
> bsg_del_done_cmd(struct bsg_device *bd, struct bsg_command *bc)
> {
> 	bd->done_cmds--;
> 	list_del(&bc->list);
> }

This only has a single caller.  It would be clearer to move this code into
that caller.

> static inline void
> bsg_add_done_cmd(struct bsg_device *bd, struct bsg_command *bc)
> {
> 	bd->done_cmds++;
> 	list_add_tail(&bc->list, &bd->done_list);
> 	wake_up(&bd->wq_done);
> }

Ditto.  Once this has been moved into the caller, that caller can then use
the neater list_move().

> static inline int bsg_io_schedule(struct bsg_device *bd, int state)

This is too large to inline.

> {
> 	DEFINE_WAIT(wait);
> 	int ret = 0;
> 
> 	spin_lock_irq(&bd->lock);
> 
> 	BUG_ON(bd->done_cmds > bd->queued_cmds);
> 
> 	/*
> 	 * -ENOSPC or -ENODATA?  I'm going for -ENODATA, meaning "I have no
> 	 * work to do", even though we return -ENOSPC after this same test
> 	 * during bsg_write() -- there, it means our buffer can't have more
> 	 * bsg_commands added to it, thus has no space left.
> 	 */
> 	if (bd->done_cmds == bd->queued_cmds) {
> 		ret = -ENODATA;
> 		goto unlock;
> 	}
> 
> 	if (!test_bit(BSG_F_BLOCK, &bd->flags)) {
> 		ret = -EAGAIN;
> 		goto unlock;
> 	}
> 
> 	prepare_to_wait(&bd->wq_done, &wait, state);
> 	spin_unlock_irq(&bd->lock);
> 	io_schedule();
> 	finish_wait(&bd->wq_done, &wait);

No, io_schedule() _must_ be called in state TASK_UNINTERRUPTIBLE.  If it
gets called in state TASK_INTERRUPTIBLE then all the accounting which it
does becomes wrong.

Fortunately the sole caller of this function _does_ use
TASK_UNINTERRUPTIBLE.  The `state' arg to this function should be removed.

> 	if ((state == TASK_INTERRUPTIBLE) && signal_pending(current))
> 		ret = -ERESTARTSYS;

And this code should be deleted.

> 	return ret;
> unlock:
> 	spin_unlock_irq(&bd->lock);
> 	return ret;
> }
> 
> static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq,
> 				struct sg_io_v4 *hdr, int has_write_perm)
> {
> 	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
> 
> 	if (copy_from_user(rq->cmd, (void *)(unsigned long)hdr->request,
> 			   hdr->request_len))
> 		return -EFAULT;
> 
> 	if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
> 		if (blk_verify_command(rq->cmd, has_write_perm))
> 			return -EPERM;
> 	} else if (!capable(CAP_SYS_RAWIO))
> 		return -EPERM;

As a reader of this code I'm wondering "hm, why is
BSG_SUB_PROTOCOL_SCSI_CMD unprivileged, while other modes require
CAP_SYS_RAWIO"?.

This design/policy decision maybe was discussed on a mailing list
somewhere, or even perhaps in a changelog (although I can't find it).  But
it is so important, and is so unobvious from a reading of the code that I'd
suggest that it is worth some discussion right here, in a code comment.

> 	/*
> 	 * fill in request structure
> 	 */
> 	rq->cmd_len = hdr->request_len;
> 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> 
> 	rq->timeout = (hdr->timeout * HZ) / 1000;
> 	if (!rq->timeout)
> 		rq->timeout = q->sg_timeout;
> 	if (!rq->timeout)
> 		rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
> 
> 	return 0;
> }
> 
> /*
>  * Check if sg_io_v4 from user is allowed and valid
>  */
> static int
> bsg_validate_sgv4_hdr(request_queue_t *q, struct sg_io_v4 *hdr, int *rw)
> {
> 	int ret = 0;
> 
> 	if (hdr->guard != 'Q')
> 		return -EINVAL;

hm, "Q".  What is the user interface to this new stuff?

What does the code in bsg.c _do_, anyway??  Ho hum.

> 	if (hdr->request_len > BLK_MAX_CDB)
> 		return -EINVAL;
> 	if (hdr->dout_xfer_len > (q->max_sectors << 9) ||
> 	    hdr->din_xfer_len > (q->max_sectors << 9))

Are we sure that nothing here can exceed 4GB now and in the future?

> 		return -EIO;
> 
> 	switch (hdr->protocol) {
> 	case BSG_PROTOCOL_SCSI:
> 		switch (hdr->subprotocol) {
> 		case BSG_SUB_PROTOCOL_SCSI_CMD:
> 		case BSG_SUB_PROTOCOL_SCSI_TRANSPORT:
> 			break;
> 		default:
> 			ret = -EINVAL;
> 		}
> 		break;
> 	default:
> 		ret = -EINVAL;
> 	}
> 
> 	*rw = hdr->dout_xfer_len ? WRITE : READ;
> 	return ret;
> }
> 
> /*
>  * map sg_io_v4 to a request.
>  */
> static struct request *
> bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
> {
> 	request_queue_t *q = bd->queue;
> 	struct request *rq, *next_rq = NULL;
> 	int ret, rw = 0; /* shut up gcc */

The modern way of shutting up gcc is uninitialized_var().

> 	unsigned int dxfer_len;
> 	void *dxferp = NULL;
> 
> 	dprintk("map hdr %llx/%u %llx/%u\n", (unsigned long long) hdr->dout_xferp,
> 		hdr->dout_xfer_len, (unsigned long long) hdr->din_xferp,
> 		hdr->din_xfer_len);
> 
> 	ret = bsg_validate_sgv4_hdr(q, hdr, &rw);
> 	if (ret)
> 		return ERR_PTR(ret);
> 
> 	/*
> 	 * map scatter-gather elements seperately and string them to request
> 	 */
> 	rq = blk_get_request(q, rw, GFP_KERNEL);
> 	if (!rq)
> 		return ERR_PTR(-ENOMEM);
> 	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, test_bit(BSG_F_WRITE_PERM,
> 						       &bd->flags));
> 	if (ret)
> 		goto out;
> 
> 	if (rw == WRITE && hdr->din_xfer_len) {
> 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
> 			ret = -EOPNOTSUPP;
> 			goto out;
> 		}
> 
> 		next_rq = blk_get_request(q, READ, GFP_KERNEL);
> 		if (!next_rq) {
> 			ret = -ENOMEM;
> 			goto out;
> 		}
> 		rq->next_rq = next_rq;
> 
> 		dxferp = (void*)(unsigned long)hdr->din_xferp;

So... sg_io_v4.din_xferp is a user virtual address?

And `struct sg_io_v4' has just become part of the kernel ABI?  Beware that
there is a move afoot to require test code, manpages and even LTP testcases
for new ABI extensions.  Is this interface documented anywhere?

> 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
> 		if (ret)
> 			goto out;
> 	}
> 
> 	if (hdr->dout_xfer_len) {
> 		dxfer_len = hdr->dout_xfer_len;
> 		dxferp = (void*)(unsigned long)hdr->dout_xferp;
> 	} else if (hdr->din_xfer_len) {
> 		dxfer_len = hdr->din_xfer_len;
> 		dxferp = (void*)(unsigned long)hdr->din_xferp;
> 	} else
> 		dxfer_len = 0;
> 
> 	if (dxfer_len) {
> 		ret = blk_rq_map_user(q, rq, dxferp, dxfer_len);
> 		if (ret)
> 			goto out;
> 	}
> 	return rq;
> out:
> 	blk_put_request(rq);
> 	if (next_rq) {
> 		blk_rq_unmap_user(next_rq->bio);
> 		blk_put_request(next_rq);
> 	}
> 	return ERR_PTR(ret);
> }
> 
> ...
> 
> static inline struct bsg_command *bsg_next_done_cmd(struct bsg_device *bd)
> {
> 	struct bsg_command *bc = NULL;
> 
> 	spin_lock_irq(&bd->lock);
> 	if (bd->done_cmds) {
> 		bc = list_entry_bc(bd->done_list.next);
> 		bsg_del_done_cmd(bd, bc);
> 	}
> 	spin_unlock_irq(&bd->lock);
> 
> 	return bc;
> }

This is too large to inline.

> static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
> 				    struct bio *bio, struct bio *bidi_bio)
> {
> 	int ret = 0;
> 
> 	dprintk("rq %p bio %p %u\n", rq, bio, rq->errors);
> 	/*
> 	 * fill in all the output members
> 	 */
> 	hdr->device_status = status_byte(rq->errors);
> 	hdr->transport_status = host_byte(rq->errors);
> 	hdr->driver_status = driver_byte(rq->errors);
> 	hdr->info = 0;
> 	if (hdr->device_status || hdr->transport_status || hdr->driver_status)
> 		hdr->info |= SG_INFO_CHECK;
> 	hdr->din_resid = rq->data_len;
> 	hdr->response_len = 0;
> 
> 	if (rq->sense_len && hdr->response) {
> 		int len = min((unsigned int) hdr->max_response_len,
> 			      rq->sense_len);

Use min_t here

> 		ret = copy_to_user((void*)(unsigned long)hdr->response,
> 				   rq->sense, len);
> 		if (!ret)
> 			hdr->response_len = len;
> 		else
> 			ret = -EFAULT;
> 	}
> 
> 	if (rq->next_rq) {
> 		blk_rq_unmap_user(bidi_bio);
> 		blk_put_request(rq->next_rq);
> 	}
> 
> 	blk_rq_unmap_user(bio);
> 	blk_put_request(rq);
> 
> 	return ret;
> }
> 
> ...
>
> static ssize_t
> __bsg_read(char __user *buf, size_t count, struct bsg_device *bd,
> 	   const struct iovec *iov, ssize_t *bytes_read)
> {
> 	struct bsg_command *bc;
> 	int nr_commands, ret;
> 
> 	if (count % sizeof(struct sg_io_v4))
> 		return -EINVAL;
> 
> 	ret = 0;
> 	nr_commands = count / sizeof(struct sg_io_v4);
> 	while (nr_commands) {
> 		bc = bsg_get_done_cmd(bd);
> 		if (IS_ERR(bc)) {
> 			ret = PTR_ERR(bc);
> 			break;
> 		}
> 
> 		/*
> 		 * this is the only case where we need to copy data back
> 		 * after completing the request. so do that here,
> 		 * bsg_complete_work() cannot do that for us
> 		 */
> 		ret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
> 					       bc->bidi_bio);
> 
> 		if (copy_to_user(buf, (char *) &bc->hdr, sizeof(bc->hdr)))
> 			ret = -EFAULT;

Unneeded cast.

> 		bsg_free_command(bc);
> 
> 		if (ret)
> 			break;
> 
> 		buf += sizeof(struct sg_io_v4);
> 		*bytes_read += sizeof(struct sg_io_v4);
> 		nr_commands--;
> 	}
> 
> 	return ret;
> }

This function returns zero or a negative errno (as should have been
explainined in its covering comment).  Hence its return type of ssize_t is
misleading.  It should return `int'.  Which is in fact the type of the
local variable which it returns.

> static inline void bsg_set_block(struct bsg_device *bd, struct file *file)
> {
> 	if (file->f_flags & O_NONBLOCK)
> 		clear_bit(BSG_F_BLOCK, &bd->flags);
> 	else
> 		set_bit(BSG_F_BLOCK, &bd->flags);
> }
> 
> static inline void bsg_set_write_perm(struct bsg_device *bd, struct file *file)
> {
> 	if (file->f_mode & FMODE_WRITE)
> 		set_bit(BSG_F_WRITE_PERM, &bd->flags);
> 	else
> 		clear_bit(BSG_F_WRITE_PERM, &bd->flags);
> }

Still wondering what all this code does.  It _appears_ that the chosen user
interface is via some device-special file?  And that an O_NONBLOCK open of
that file has some special (undocumented?) significance?

> static inline int err_block_err(int ret)
> {
> 	if (ret && ret != -ENOSPC && ret != -ENODATA && ret != -EAGAIN)
> 		return 1;
> 
> 	return 0;
> }

What a strange function.  The name is fairly meaningless.  A little comment
would help decrease the mystery.

> static ssize_t
> bsg_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> 	struct bsg_device *bd = file->private_data;
> 	int ret;
> 	ssize_t bytes_read;
> 
> 	dprintk("%s: read %Zd bytes\n", bd->name, count);
> 
> 	bsg_set_block(bd, file);
> 	bytes_read = 0;
> 	ret = __bsg_read(buf, count, bd, NULL, &bytes_read);
> 	*ppos = bytes_read;
> 
> 	if (!bytes_read || (bytes_read && err_block_err(ret)))
> 		bytes_read = ret;
> 	return bytes_read;
> }
>
> static ssize_t __bsg_write(struct bsg_device *bd, const char __user *buf,
> 			   size_t count, ssize_t *bytes_read)
> {
> 	struct bsg_command *bc;
> 	struct request *rq;
> 	int ret, nr_commands;
> 
> 	if (count % sizeof(struct sg_io_v4))
> 		return -EINVAL;
> 
> 	nr_commands = count / sizeof(struct sg_io_v4);
> 	rq = NULL;
> 	bc = NULL;
> 	ret = 0;
> 	while (nr_commands) {
> 		request_queue_t *q = bd->queue;
> 
> 		bc = bsg_alloc_command(bd);
> 		if (IS_ERR(bc)) {
> 			ret = PTR_ERR(bc);
> 			bc = NULL;
> 			break;
> 		}
> 
> 		bc->uhdr = (struct sg_io_v4 __user *) buf;
> 		if (copy_from_user(&bc->hdr, buf, sizeof(bc->hdr))) {
> 			ret = -EFAULT;
> 			break;
> 		}
> 
> 		/*
> 		 * get a request, fill in the blanks, and add to request queue
> 		 */
> 		rq = bsg_map_hdr(bd, &bc->hdr);
> 		if (IS_ERR(rq)) {
> 			ret = PTR_ERR(rq);
> 			rq = NULL;
> 			break;
> 		}
> 
> 		bsg_add_command(bd, q, bc, rq);
> 		bc = NULL;
> 		rq = NULL;
> 		nr_commands--;
> 		buf += sizeof(struct sg_io_v4);
> 		*bytes_read += sizeof(struct sg_io_v4);
> 	}
> 
> 	if (bc)
> 		bsg_free_command(bc);
> 
> 	return ret;
> }

Return type should be `int'.

> static ssize_t
> bsg_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> {
> 	struct bsg_device *bd = file->private_data;
> 	ssize_t bytes_read;

This variable should be called bytes_written.

> 	int ret;
> 
> 	dprintk("%s: write %Zd bytes\n", bd->name, count);
> 
> 	bsg_set_block(bd, file);
> 	bsg_set_write_perm(bd, file);
> 
> 	bytes_read = 0;
> 	ret = __bsg_write(bd, buf, count, &bytes_read);
> 	*ppos = bytes_read;
> 
> 	/*
> 	 * return bytes written on non-fatal errors
> 	 */
> 	if (!bytes_read || (bytes_read && err_block_err(ret)))
> 		bytes_read = ret;
> 	dprintk("%s: returning %Zd\n", bd->name, bytes_read);
> 	return bytes_read;
> }
> 
> ...

> 
> static struct bsg_device *bsg_add_device(struct inode *inode,
> 					 struct request_queue *rq,
> 					 struct file *file)
> {
> 	struct bsg_device *bd = NULL;

Unneeded initialisation.

> #ifdef BSG_DEBUG
> 	unsigned char buf[32];
> #endif
> 
> 	bd = bsg_alloc_device();
> 	if (!bd)
> 		return ERR_PTR(-ENOMEM);
> 
> 	bd->queue = rq;
> 	kobject_get(&rq->kobj);
> 	bsg_set_block(bd, file);
> 
> 	atomic_set(&bd->ref_count, 1);
> 	bd->minor = iminor(inode);
> 	mutex_lock(&bsg_mutex);
> 	hlist_add_head(&bd->dev_list, &bsg_device_list[bsg_list_idx(bd->minor)]);
> 
> 	strncpy(bd->name, rq->bsg_dev.class_dev->class_id, sizeof(bd->name) - 1);
> 	dprintk("bound to <%s>, max queue %d\n",
> 		format_dev_t(buf, inode->i_rdev), bd->max_queue);
> 
> 	mutex_unlock(&bsg_mutex);
> 	return bd;
> }
> 
> ...
>
> static int
> bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> 	  unsigned long arg)

This is an file_operations.ioctl() method.  It should instead have been
implemented as an unlocked_ioctl handler.


> {
> 	struct bsg_device *bd = file->private_data;
> 	int __user *uarg = (int __user *) arg;
> 
> 	if (!bd)
> 		return -ENXIO;

This cannot happen, surely?

> 	switch (cmd) {
> 		/*
> 		 * our own ioctls
> 		 */
> 	case SG_GET_COMMAND_Q:
> 		return put_user(bd->max_queue, uarg);
> 	case SG_SET_COMMAND_Q: {
> 		int queue;
> 
> 		if (get_user(queue, uarg))
> 			return -EFAULT;
> 		if (queue < 1)
> 			return -EINVAL;
> 
> 		spin_lock_irq(&bd->lock);
> 		bd->max_queue = queue;
> 		spin_unlock_irq(&bd->lock);
> 		return 0;
> 	}
> 
> 	/*
> 	 * SCSI/sg ioctls
> 	 */
> 	case SG_GET_VERSION_NUM:
> 	case SCSI_IOCTL_GET_IDLUN:
> 	case SCSI_IOCTL_GET_BUS_NUMBER:
> 	case SG_SET_TIMEOUT:
> 	case SG_GET_TIMEOUT:
> 	case SG_GET_RESERVED_SIZE:
> 	case SG_SET_RESERVED_SIZE:
> 	case SG_EMULATED_HOST:
> 	case SCSI_IOCTL_SEND_COMMAND: {
> 		void __user *uarg = (void __user *) arg;
> 		return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
> 	}
> 	case SG_IO: {
> 		struct request *rq;
> 		struct bio *bio, *bidi_bio = NULL;
> 		struct sg_io_v4 hdr;
> 
> 		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
> 			return -EFAULT;
> 
> 		rq = bsg_map_hdr(bd, &hdr);
> 		if (IS_ERR(rq))
> 			return PTR_ERR(rq);
> 
> 		bio = rq->bio;
> 		if (rq->next_rq)
> 			bidi_bio = rq->next_rq->bio;
> 		blk_execute_rq(bd->queue, NULL, rq, 0);
> 		blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
> 
> 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
> 			return -EFAULT;
> 
> 		return 0;
> 	}
> 	/*
> 	 * block device ioctls
> 	 */
> 	default:
> #if 0
> 		return ioctl_by_bdev(bd->bdev, cmd, arg);
> #else
> 		return -ENOTTY;
> #endif
> 	}
> }

So we perform IO operations on this "device" by opening it and running
ioctls, the args to which point to fairly complex data structures which lie
in userspace and which contain addresses and lengths of userspace IO
buffers?

What did Christoph think of this? :)

> static struct file_operations bsg_fops = {
> 	.read		=	bsg_read,
> 	.write		=	bsg_write,
> 	.poll		=	bsg_poll,
> 	.open		=	bsg_open,
> 	.release	=	bsg_release,
> 	.ioctl		=	bsg_ioctl,

unlocked_ioctl, please.

> 	.owner		=	THIS_MODULE,
> };
> 
> void bsg_unregister_queue(struct request_queue *q)
> {
> 	struct bsg_class_device *bcd = &q->bsg_dev;
> 
> 	if (!bcd->class_dev)
> 		return;

Can this happen?

> 	mutex_lock(&bsg_mutex);
> 	sysfs_remove_link(&q->kobj, "bsg");
> 	class_device_destroy(bsg_class, MKDEV(BSG_MAJOR, bcd->minor));
> 	bcd->class_dev = NULL;
> 	list_del_init(&bcd->list);
> 	bsg_device_nr--;
> 	mutex_unlock(&bsg_mutex);
> }
> EXPORT_SYMBOL_GPL(bsg_unregister_queue);

<still wondering what all this code does>

Would I be correct in assuming that it offers services to device drivers,
which have yet to be hooked up?

> int bsg_register_queue(struct request_queue *q, const char *name)
> {
> 	struct bsg_class_device *bcd, *__bcd;
> 	dev_t dev;
> 	int ret = -EMFILE;
> 	struct class_device *class_dev = NULL;
> 
> 	/*
> 	 * we need a proper transport to send commands, not a stacked device
> 	 */
> 	if (!q->request_fn)
> 		return 0;
> 
> 	bcd = &q->bsg_dev;
> 	memset(bcd, 0, sizeof(*bcd));
> 	INIT_LIST_HEAD(&bcd->list);
> 
> 	mutex_lock(&bsg_mutex);
> 	if (bsg_device_nr == BSG_MAX_DEVS) {
> 		printk(KERN_ERR "bsg: too many bsg devices\n");

32768 is a lot of devices.  Why is there any limit at all?

> 		goto err;
> 	}
> 
> retry:
> 	list_for_each_entry(__bcd, &bsg_class_list, list) {
> 		if (__bcd->minor == bsg_minor_idx) {
> 			bsg_minor_idx++;
> 			if (bsg_minor_idx == BSG_MAX_DEVS)
> 				bsg_minor_idx = 0;
> 			goto retry;
> 		}
> 	}
> 
> 	bcd->minor = bsg_minor_idx++;
> 	if (bsg_minor_idx == BSG_MAX_DEVS)
> 		bsg_minor_idx = 0;

So what's happening here?  We're doing a linear, potentially O(n^2) search
for a unique minor number?

I expect that you'll find that lib/idr.c provides a more elegant solution. 
The tty code uses this, and there are other examples around the place.


> 	bcd->queue = q;
> 	dev = MKDEV(BSG_MAJOR, bcd->minor);
> 	class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);
> 	if (IS_ERR(class_dev)) {
> 		ret = PTR_ERR(class_dev);
> 		goto err;
> 	}
> 	bcd->class_dev = class_dev;
> 
> 	if (q->kobj.sd) {
> 		ret = sysfs_create_link(&q->kobj, &bcd->class_dev->kobj, "bsg");
> 		if (ret)
> 			goto err;
> 	}
> 
> 	list_add_tail(&bcd->list, &bsg_class_list);
> 	bsg_device_nr++;
> 
> 	mutex_unlock(&bsg_mutex);
> 	return 0;
> err:
> 	if (class_dev)
> 		class_device_destroy(bsg_class, MKDEV(BSG_MAJOR, bcd->minor));
> 	mutex_unlock(&bsg_mutex);
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(bsg_register_queue);
> 
> ...
>


In terms of presentation: this code hit the tree as base patch plus what
appear to be 20 bugfixes, none of which are really interesting or relevant
to mainline.  Personally I think it would be nicer if all that out-of-tree
development work was cleaned up and the new code goes in as a single hit.

This makes it a lot easier to find out "wtf does this code all do".  One
finds the first commit and reads the changlog.  But this algorithm yields:

    bsg: support for full generic block layer SG v3

which is not helpful.

Re: block/bsg.c

[Jeff Garzik]

Andrew Morton wrote:
> The modern way of shutting up gcc is uninitialized_var().


Should I convert my misc-2.6.git#gccbug repository over to this, and 
push upstream?

#gccbug branch is a set of places where I have verified that the 
variable is indeed initialized, even though gcc complains it may not be.

	Jeff

Re: block/bsg.c

[Satyam Sharma]

On 7/17/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>

CONFIG_BLK_DEV_BSG=y
CONFIG_SCSI=m

block/built-in.o: In function `bsg_init':
block/bsg.c:1097: undefined reference to `scsi_register_interface'
make: *** [.tmp_vmlinux1] Error 1

on latest -git.

Satyam

Re: block/bsg.c

[Andrew Morton]

On Mon, 16 Jul 2007 20:47:45 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> > The modern way of shutting up gcc is uninitialized_var().
> 
> 
> Should I convert my misc-2.6.git#gccbug repository over to this, and 
> push upstream?

Opinions differ (a bit) but personally I think the benefit of fixing the
warnings outweighs the risk that these suppressions will later hide a real
bug.

Certainly, using uninitialized_var() is better than open-coding "= 0" all
over the place.

Purists can add a config variable to centrally disable uninitialized_var()
if they want to check on the warnings.

> #gccbug branch is a set of places where I have verified that the 
> variable is indeed initialized, even though gcc complains it may not be.
> 

Do it!

Re: block/bsg.c

[FUJITA Tomonori]

From: "Satyam Sharma" <satyam.sharma@gmail.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 06:22:25 +0530

> On 7/17/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> 
> CONFIG_BLK_DEV_BSG=y
> CONFIG_SCSI=m
> 
> block/built-in.o: In function `bsg_init':
> block/bsg.c:1097: undefined reference to `scsi_register_interface'
> make: *** [.tmp_vmlinux1] Error 1

This should be fixed by the following commit (thanks !):

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=29417b899a77aaba1c060f5e123db4f50006f58a

Re: block/bsg.c

[Jeff Garzik]

Andrew Morton wrote:
> On Mon, 16 Jul 2007 20:47:45 -0400 Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Andrew Morton wrote:
>>> The modern way of shutting up gcc is uninitialized_var().
>>
>> Should I convert my misc-2.6.git#gccbug repository over to this, and 
>> push upstream?
> 
> Opinions differ (a bit) but personally I think the benefit of fixing the
> warnings outweighs the risk that these suppressions will later hide a real
> bug.

Tooting my own horn, but, anything in #gccbug I consider to be verified 
to -not- be hiding a real bug.  Human-verified not machine-verified, of 
course, so it's imperfect.  But at least it's been reviewed and 
considered carefully.

I'll look into "tarting up" #gccbug for upstream...  I had missed the 
introduction of uninitialized_var(), which was the genesis for this line 
of questioning.

	Jeff

Re: block/bsg.c

[Gabriel C]

Satyam Sharma wrote:
> On 7/17/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>   
>
> CONFIG_BLK_DEV_BSG=y
> CONFIG_SCSI=m
>
> block/built-in.o: In function `bsg_init':
> block/bsg.c:1097: undefined reference to `scsi_register_interface'
> make: *** [.tmp_vmlinux1] Error 1
>
> on latest -git.
>   

Should be fixed by this commit :

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=29417b899a77aaba1c060f5e123db4f50006f58a

> Satyam
>
>   

Gabriel

Re: block/bsg.c

[Andrew Morton]

On Mon, 16 Jul 2007 20:58:11 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> > On Mon, 16 Jul 2007 20:47:45 -0400 Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> Andrew Morton wrote:
> >>> The modern way of shutting up gcc is uninitialized_var().
> >>
> >> Should I convert my misc-2.6.git#gccbug repository over to this, and 
> >> push upstream?
> > 
> > Opinions differ (a bit) but personally I think the benefit of fixing the
> > warnings outweighs the risk that these suppressions will later hide a real
> > bug.
> 
> Tooting my own horn, but, anything in #gccbug I consider to be verified 
> to -not- be hiding a real bug.  Human-verified not machine-verified, of 
> course, so it's imperfect.  But at least it's been reviewed and 
> considered carefully.

Yup, but the concern (from Al, iirc) was that someone could change the code
later on, add a new bug and have that bug hidden by the unneeded
initialisation.

> I'll look into "tarting up" #gccbug for upstream...  I had missed the 
> introduction of uninitialized_var(), which was the genesis for this line 
> of questioning.

uninitialized_var() has the advantage that it generates no code, whereas "=
0" often adds instructions.  Plus of course it is self-documenting, greppable-for
and centrally alterable.

Re: block/bsg.c

[Jeff Garzik]

Andrew Morton wrote:
> Yup, but the concern (from Al, iirc) was that someone could change the code
> later on, add a new bug and have that bug hidden by the unneeded
> initialisation.

True.

That's why #gccbug went the safe route, took the cost of extra 
instructions, and initialized it to zero.


> uninitialized_var() has the advantage that it generates no code, whereas "=
> 0" often adds instructions.  Plus of course it is self-documenting, greppable-for
> and centrally alterable.

Agreed.

	Jeff

Re: block/bsg.c

[Jeff Garzik]

hrm.  uninitialized_var(x) does not silence the warning, on my compiler:

[jgarzik@pretzel misc-2.6]$ rpm -q gcc
gcc-4.1.2-13.fc6

@@ -1358,6 +1358,8 @@ udf_load_partition(struct super_block *sb, 
kernel_lb_addr
                         {
                                 kernel_lb_addr ino;

+ 
uninitialized_var(ino.partitionReferenceNum);
+
                                 if (!UDF_SB_LASTBLOCK(sb))

still yields

fs/udf/super.c: In function ‘udf_fill_super’:
fs/udf/super.c:1359: warning: ‘ino.partitionReferenceNum’ may be used 
uninitialized in this function


-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: block/bsg.c

[Jeremy Fitzhardinge]

Jeff Garzik wrote:
> + uninitialized_var(ino.partitionReferenceNum);
> +
>                                 if (!UDF_SB_LASTBLOCK(sb))
>
> still yields
>
> fs/udf/super.c: In function ‘udf_fill_super’:
> fs/udf/super.c:1359: warning: ‘ino.partitionReferenceNum’ may be used
> uninitialized in this function

It only works on declarations:

	int uninitialized_var(foo);


    J
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: block/bsg.c

[Andrew Morton]

On Mon, 16 Jul 2007 21:47:21 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> hrm.  uninitialized_var(x) does not silence the warning, on my compiler:
> 
> [jgarzik@pretzel misc-2.6]$ rpm -q gcc
> gcc-4.1.2-13.fc6
> 
> @@ -1358,6 +1358,8 @@ udf_load_partition(struct super_block *sb, kernel_lb_addr
>             	{
>             		kernel_lb_addr ino;
> + 			uninitialized_var(ino.partitionReferenceNum);
> +
>                	if (!UDF_SB_LASTBLOCK(sb))

[erk, wordwrapping!]

> still yields
> 
> fs/udf/super.c: In function ‘udf_fill_super’:
> fs/udf/super.c:1359: warning: ‘ino.partitionReferenceNum’ may be used 
> uninitialized in this function
> 

We use it as

-	int foo;
+	int uninitialized_var(foo);

so what you have is effectively


		kernel_lb_addr ino;

		ino.partitionReferenceNum = ino.partitionReferenceNum;


so it still warns.



Hopefully this will work:

		kernel_lb_addr uninitialized_var(ino);

	
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: block/bsg.c

[Joseph Fannin]

On Mon, Jul 16, 2007 at 04:57:06PM -0700, Andrew Morton wrote:

> What does the code in bsg.c _do_, anyway??  Ho hum.

    It reformats the hard drive on Battlestar Galactica before the
Cylon virus that has penetrated the firewalls keeps the power off long
enough for the Centurions to vent the crew into space.  (I believe it
used BSG disk slices).

--
Joseph Fannin
jfannin@gmail.com

/me suspects he has used his ration of inanity for about a year or so.

Re: block/bsg.c

[Jens Axboe]

On Mon, Jul 16 2007, Andrew Morton wrote:
> 
> A belated review (I've never seen this before and there it is in mainline)
> 
> > static char bsg_version[] = "block layer sg (bsg) 0.4";
> 
> `const' would be better.  That moves it into a write-protected memory section.

Agree

> > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> 
> This makes the code easier to write but harder to read.  We should optimise
> for readers.  Please open-code this at callsites.
> 
> Or at least convert it into a (commented) (possibly inlined) C function.

list_entry_to_bc(), then? The main objective is to save on typing, and
(just as important) make sure we don't bump over the 80 chars per line.

> > /*
> >  * just for testing
> >  */
> > #define BSG_MAJOR	(240)
> 
> What's this doing in mainline?  240 is a "reserved for local use" major. 
> This will cause collisions.  This code should be using dynamic major
> assignment.

Yeah, that's a big error on my part. Will get that fixed up right away.

> > static DEFINE_MUTEX(bsg_mutex);
> > static int bsg_device_nr, bsg_minor_idx;
> > 
> > #define BSG_LIST_SIZE	(8)
> 
> afacit this isn't really the size of a list.  It has something to do with
> the number of minors which are attached to that illegitimate major?  A
> comment here would help.  Perhaps this name is poorly chosen.

Yeah, it's the size of the array of lists :-)

> > #define bsg_list_idx(minor)	((minor) & (BSG_LIST_SIZE - 1))
> 
> Please prefer to write code in C, not in cpp.

Agree, that will die.

> > static struct hlist_head bsg_device_list[BSG_LIST_SIZE];
> 
> That is an array, not a list.

It's an array of lists.

> > static struct class *bsg_class;
> > static LIST_HEAD(bsg_class_list);
> > 
> > static struct kmem_cache *bsg_cmd_cachep;
> 
> How many of these items do we expect to be simultaneously allocated?  If
> that number is small then a custom kmem_cache is probably not warranted.

For your average desktop application, only a few are likely to be in
flight at the same time. For higher end stuff, it could be thousands.

> > /*
> >  * our internal command type
> >  */
> > struct bsg_command {
> > 	struct bsg_device *bd;
> > 	struct list_head list;
> > 	struct request *rq;
> > 	struct bio *bio;
> > 	struct bio *bidi_bio;
> > 	int err;
> > 	struct sg_io_v4 hdr;
> > 	struct sg_io_v4 __user *uhdr;
> > 	char sense[SCSI_SENSE_BUFFERSIZE];
> > };
> 
> Comments here, please.

Noted.

> > static void bsg_free_command(struct bsg_command *bc)
> > {
> > 	struct bsg_device *bd = bc->bd;
> > 	unsigned long flags;
> > 
> > 	kmem_cache_free(bsg_cmd_cachep, bc);
> > 
> > 	spin_lock_irqsave(&bd->lock, flags);
> > 	bd->queued_cmds--;
> > 	spin_unlock_irqrestore(&bd->lock, flags);
> > 
> > 	wake_up(&bd->wq_free);
> > }
> > 
> > static struct bsg_command *bsg_alloc_command(struct bsg_device *bd)
> > {
> > 	struct bsg_command *bc = ERR_PTR(-EINVAL);
> > 
> > 	spin_lock_irq(&bd->lock);
> > 
> > 	if (bd->queued_cmds >= bd->max_queue)
> > 		goto out;
> > 
> > 	bd->queued_cmds++;
> > 	spin_unlock_irq(&bd->lock);
> > 
> > 	bc = kmem_cache_alloc(bsg_cmd_cachep, GFP_USER);
> 
> This should be GFP_KERNEL.

Fixed

> > 	if (unlikely(!bc)) {
> > 		spin_lock_irq(&bd->lock);
> > 		bd->queued_cmds--;
> > 		bc = ERR_PTR(-ENOMEM);
> > 		goto out;
> > 	}
> > 
> > 	memset(bc, 0, sizeof(*bc));
> 
> Use kmem_cache_zalloc() above, remove this.

Fixed

> > 	bc->bd = bd;
> > 	INIT_LIST_HEAD(&bc->list);
> > 	dprintk("%s: returning free cmd %p\n", bd->name, bc);
> > 	return bc;
> > out:
> > 	spin_unlock_irq(&bd->lock);
> > 	return bc;
> > }
> > 
> > static inline void
> > bsg_del_done_cmd(struct bsg_device *bd, struct bsg_command *bc)
> > {
> > 	bd->done_cmds--;
> > 	list_del(&bc->list);
> > }
> 
> This only has a single caller.  It would be clearer to move this code into
> that caller.
>
> > static inline void
> > bsg_add_done_cmd(struct bsg_device *bd, struct bsg_command *bc)
> > {
> > 	bd->done_cmds++;
> > 	list_add_tail(&bc->list, &bd->done_list);
> > 	wake_up(&bd->wq_done);
> > }
> 
> Ditto.  Once this has been moved into the caller, that caller can then use
> the neater list_move().

Sure, why not (to both).

> > static inline int bsg_io_schedule(struct bsg_device *bd, int state)
> 
> This is too large to inline.

Fixed

> > {
> > 	DEFINE_WAIT(wait);
> > 	int ret = 0;
> > 
> > 	spin_lock_irq(&bd->lock);
> > 
> > 	BUG_ON(bd->done_cmds > bd->queued_cmds);
> > 
> > 	/*
> > 	 * -ENOSPC or -ENODATA?  I'm going for -ENODATA, meaning "I have no
> > 	 * work to do", even though we return -ENOSPC after this same test
> > 	 * during bsg_write() -- there, it means our buffer can't have more
> > 	 * bsg_commands added to it, thus has no space left.
> > 	 */
> > 	if (bd->done_cmds == bd->queued_cmds) {
> > 		ret = -ENODATA;
> > 		goto unlock;
> > 	}
> > 
> > 	if (!test_bit(BSG_F_BLOCK, &bd->flags)) {
> > 		ret = -EAGAIN;
> > 		goto unlock;
> > 	}
> > 
> > 	prepare_to_wait(&bd->wq_done, &wait, state);
> > 	spin_unlock_irq(&bd->lock);
> > 	io_schedule();
> > 	finish_wait(&bd->wq_done, &wait);
> 
> No, io_schedule() _must_ be called in state TASK_UNINTERRUPTIBLE.  If it
> gets called in state TASK_INTERRUPTIBLE then all the accounting which it
> does becomes wrong.
> 
> Fortunately the sole caller of this function _does_ use
> TASK_UNINTERRUPTIBLE.  The `state' arg to this function should be removed.

Fixed

> > 	if ((state == TASK_INTERRUPTIBLE) && signal_pending(current))
> > 		ret = -ERESTARTSYS;
> 
> And this code should be deleted.

Fixed

> > 	return ret;
> > unlock:
> > 	spin_unlock_irq(&bd->lock);
> > 	return ret;
> > }
> > 
> > static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq,
> > 				struct sg_io_v4 *hdr, int has_write_perm)
> > {
> > 	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
> > 
> > 	if (copy_from_user(rq->cmd, (void *)(unsigned long)hdr->request,
> > 			   hdr->request_len))
> > 		return -EFAULT;
> > 
> > 	if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
> > 		if (blk_verify_command(rq->cmd, has_write_perm))
> > 			return -EPERM;
> > 	} else if (!capable(CAP_SYS_RAWIO))
> > 		return -EPERM;
> 
> As a reader of this code I'm wondering "hm, why is
> BSG_SUB_PROTOCOL_SCSI_CMD unprivileged, while other modes require
> CAP_SYS_RAWIO"?.
>
> This design/policy decision maybe was discussed on a mailing list
> somewhere, or even perhaps in a changelog (although I can't find it).  But
> it is so important, and is so unobvious from a reading of the code that I'd
> suggest that it is worth some discussion right here, in a code comment.
>
It's not unprivileged, it goes through the blk_verify_command() check
list.

> > 	/*
> > 	 * fill in request structure
> > 	 */
> > 	rq->cmd_len = hdr->request_len;
> > 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> > 
> > 	rq->timeout = (hdr->timeout * HZ) / 1000;
> > 	if (!rq->timeout)
> > 		rq->timeout = q->sg_timeout;
> > 	if (!rq->timeout)
> > 		rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
> > 
> > 	return 0;
> > }
> > 
> > /*
> >  * Check if sg_io_v4 from user is allowed and valid
> >  */
> > static int
> > bsg_validate_sgv4_hdr(request_queue_t *q, struct sg_io_v4 *hdr, int *rw)
> > {
> > 	int ret = 0;
> > 
> > 	if (hdr->guard != 'Q')
> > 		return -EINVAL;
> 
> hm, "Q".  What is the user interface to this new stuff?

'Q' is just the magic identifier, like 'S' for sg v4.

> What does the code in bsg.c _do_, anyway??  Ho hum.

It's a driver for transporting sg v4 commands.

> > 	if (hdr->request_len > BLK_MAX_CDB)
> > 		return -EINVAL;
> > 	if (hdr->dout_xfer_len > (q->max_sectors << 9) ||
> > 	    hdr->din_xfer_len > (q->max_sectors << 9))
> 
> Are we sure that nothing here can exceed 4GB now and in the future?

We are far away from that in a single command currently and probably
ever.

> > 		return -EIO;
> > 
> > 	switch (hdr->protocol) {
> > 	case BSG_PROTOCOL_SCSI:
> > 		switch (hdr->subprotocol) {
> > 		case BSG_SUB_PROTOCOL_SCSI_CMD:
> > 		case BSG_SUB_PROTOCOL_SCSI_TRANSPORT:
> > 			break;
> > 		default:
> > 			ret = -EINVAL;
> > 		}
> > 		break;
> > 	default:
> > 		ret = -EINVAL;
> > 	}
> > 
> > 	*rw = hdr->dout_xfer_len ? WRITE : READ;
> > 	return ret;
> > }
> > 
> > /*
> >  * map sg_io_v4 to a request.
> >  */
> > static struct request *
> > bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
> > {
> > 	request_queue_t *q = bd->queue;
> > 	struct request *rq, *next_rq = NULL;
> > 	int ret, rw = 0; /* shut up gcc */
> 
> The modern way of shutting up gcc is uninitialized_var().

OK, I'll do that.

> > 	unsigned int dxfer_len;
> > 	void *dxferp = NULL;
> > 
> > 	dprintk("map hdr %llx/%u %llx/%u\n", (unsigned long long) hdr->dout_xferp,
> > 		hdr->dout_xfer_len, (unsigned long long) hdr->din_xferp,
> > 		hdr->din_xfer_len);
> > 
> > 	ret = bsg_validate_sgv4_hdr(q, hdr, &rw);
> > 	if (ret)
> > 		return ERR_PTR(ret);
> > 
> > 	/*
> > 	 * map scatter-gather elements seperately and string them to request
> > 	 */
> > 	rq = blk_get_request(q, rw, GFP_KERNEL);
> > 	if (!rq)
> > 		return ERR_PTR(-ENOMEM);
> > 	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, test_bit(BSG_F_WRITE_PERM,
> > 						       &bd->flags));
> > 	if (ret)
> > 		goto out;
> > 
> > 	if (rw == WRITE && hdr->din_xfer_len) {
> > 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
> > 			ret = -EOPNOTSUPP;
> > 			goto out;
> > 		}
> > 
> > 		next_rq = blk_get_request(q, READ, GFP_KERNEL);
> > 		if (!next_rq) {
> > 			ret = -ENOMEM;
> > 			goto out;
> > 		}
> > 		rq->next_rq = next_rq;
> > 
> > 		dxferp = (void*)(unsigned long)hdr->din_xferp;
> 
> So... sg_io_v4.din_xferp is a user virtual address?
> 
> And `struct sg_io_v4' has just become part of the kernel ABI?  Beware that
> there is a move afoot to require test code, manpages and even LTP testcases
> for new ABI extensions.  Is this interface documented anywhere?

The documentation is likely very scarce atm, if anything. The command
layout was discussed at the storage summit and on linux-scsi.

> > 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
> > 		if (ret)
> > 			goto out;
> > 	}
> > 
> > 	if (hdr->dout_xfer_len) {
> > 		dxfer_len = hdr->dout_xfer_len;
> > 		dxferp = (void*)(unsigned long)hdr->dout_xferp;
> > 	} else if (hdr->din_xfer_len) {
> > 		dxfer_len = hdr->din_xfer_len;
> > 		dxferp = (void*)(unsigned long)hdr->din_xferp;
> > 	} else
> > 		dxfer_len = 0;
> > 
> > 	if (dxfer_len) {
> > 		ret = blk_rq_map_user(q, rq, dxferp, dxfer_len);
> > 		if (ret)
> > 			goto out;
> > 	}
> > 	return rq;
> > out:
> > 	blk_put_request(rq);
> > 	if (next_rq) {
> > 		blk_rq_unmap_user(next_rq->bio);
> > 		blk_put_request(next_rq);
> > 	}
> > 	return ERR_PTR(ret);
> > }
> > 
> > ...
> > 
> > static inline struct bsg_command *bsg_next_done_cmd(struct bsg_device *bd)
> > {
> > 	struct bsg_command *bc = NULL;
> > 
> > 	spin_lock_irq(&bd->lock);
> > 	if (bd->done_cmds) {
> > 		bc = list_entry_bc(bd->done_list.next);
> > 		bsg_del_done_cmd(bd, bc);
> > 	}
> > 	spin_unlock_irq(&bd->lock);
> > 
> > 	return bc;
> > }
> 
> This is too large to inline.

Fixed

> > static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
> > 				    struct bio *bio, struct bio *bidi_bio)
> > {
> > 	int ret = 0;
> > 
> > 	dprintk("rq %p bio %p %u\n", rq, bio, rq->errors);
> > 	/*
> > 	 * fill in all the output members
> > 	 */
> > 	hdr->device_status = status_byte(rq->errors);
> > 	hdr->transport_status = host_byte(rq->errors);
> > 	hdr->driver_status = driver_byte(rq->errors);
> > 	hdr->info = 0;
> > 	if (hdr->device_status || hdr->transport_status || hdr->driver_status)
> > 		hdr->info |= SG_INFO_CHECK;
> > 	hdr->din_resid = rq->data_len;
> > 	hdr->response_len = 0;
> > 
> > 	if (rq->sense_len && hdr->response) {
> > 		int len = min((unsigned int) hdr->max_response_len,
> > 			      rq->sense_len);
> 
> Use min_t here

Fixed

> > 		ret = copy_to_user((void*)(unsigned long)hdr->response,
> > 				   rq->sense, len);
> > 		if (!ret)
> > 			hdr->response_len = len;
> > 		else
> > 			ret = -EFAULT;
> > 	}
> > 
> > 	if (rq->next_rq) {
> > 		blk_rq_unmap_user(bidi_bio);
> > 		blk_put_request(rq->next_rq);
> > 	}
> > 
> > 	blk_rq_unmap_user(bio);
> > 	blk_put_request(rq);
> > 
> > 	return ret;
> > }
> > 
> > ...
> >
> > static ssize_t
> > __bsg_read(char __user *buf, size_t count, struct bsg_device *bd,
> > 	   const struct iovec *iov, ssize_t *bytes_read)
> > {
> > 	struct bsg_command *bc;
> > 	int nr_commands, ret;
> > 
> > 	if (count % sizeof(struct sg_io_v4))
> > 		return -EINVAL;
> > 
> > 	ret = 0;
> > 	nr_commands = count / sizeof(struct sg_io_v4);
> > 	while (nr_commands) {
> > 		bc = bsg_get_done_cmd(bd);
> > 		if (IS_ERR(bc)) {
> > 			ret = PTR_ERR(bc);
> > 			break;
> > 		}
> > 
> > 		/*
> > 		 * this is the only case where we need to copy data back
> > 		 * after completing the request. so do that here,
> > 		 * bsg_complete_work() cannot do that for us
> > 		 */
> > 		ret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio,
> > 					       bc->bidi_bio);
> > 
> > 		if (copy_to_user(buf, (char *) &bc->hdr, sizeof(bc->hdr)))
> > 			ret = -EFAULT;
> 
> Unneeded cast.

Fixed

> > 		bsg_free_command(bc);
> > 
> > 		if (ret)
> > 			break;
> > 
> > 		buf += sizeof(struct sg_io_v4);
> > 		*bytes_read += sizeof(struct sg_io_v4);
> > 		nr_commands--;
> > 	}
> > 
> > 	return ret;
> > }
> 
> This function returns zero or a negative errno (as should have been
> explainined in its covering comment).  Hence its return type of ssize_t is
> misleading.  It should return `int'.  Which is in fact the type of the
> local variable which it returns.

Fixed

> > static inline void bsg_set_block(struct bsg_device *bd, struct file *file)
> > {
> > 	if (file->f_flags & O_NONBLOCK)
> > 		clear_bit(BSG_F_BLOCK, &bd->flags);
> > 	else
> > 		set_bit(BSG_F_BLOCK, &bd->flags);
> > }
> > 
> > static inline void bsg_set_write_perm(struct bsg_device *bd, struct file *file)
> > {
> > 	if (file->f_mode & FMODE_WRITE)
> > 		set_bit(BSG_F_WRITE_PERM, &bd->flags);
> > 	else
> > 		clear_bit(BSG_F_WRITE_PERM, &bd->flags);
> > }
> 
> Still wondering what all this code does.  It _appears_ that the chosen user
> interface is via some device-special file?  And that an O_NONBLOCK open of
> that file has some special (undocumented?) significance?

There's no special meaning. IIRC, it's to avoid passing the file around.

> > static inline int err_block_err(int ret)
> > {
> > 	if (ret && ret != -ENOSPC && ret != -ENODATA && ret != -EAGAIN)
> > 		return 1;
> > 
> > 	return 0;
> > }
> 
> What a strange function.  The name is fairly meaningless.  A little comment
> would help decrease the mystery.

It is crap, will fix that.

> > static ssize_t
> > bsg_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> > {
> > 	struct bsg_device *bd = file->private_data;
> > 	int ret;
> > 	ssize_t bytes_read;
> > 
> > 	dprintk("%s: read %Zd bytes\n", bd->name, count);
> > 
> > 	bsg_set_block(bd, file);
> > 	bytes_read = 0;
> > 	ret = __bsg_read(buf, count, bd, NULL, &bytes_read);
> > 	*ppos = bytes_read;
> > 
> > 	if (!bytes_read || (bytes_read && err_block_err(ret)))
> > 		bytes_read = ret;
> > 	return bytes_read;
> > }
> >
> > static ssize_t __bsg_write(struct bsg_device *bd, const char __user *buf,
> > 			   size_t count, ssize_t *bytes_read)
> > {
> > 	struct bsg_command *bc;
> > 	struct request *rq;
> > 	int ret, nr_commands;
> > 
> > 	if (count % sizeof(struct sg_io_v4))
> > 		return -EINVAL;
> > 
> > 	nr_commands = count / sizeof(struct sg_io_v4);
> > 	rq = NULL;
> > 	bc = NULL;
> > 	ret = 0;
> > 	while (nr_commands) {
> > 		request_queue_t *q = bd->queue;
> > 
> > 		bc = bsg_alloc_command(bd);
> > 		if (IS_ERR(bc)) {
> > 			ret = PTR_ERR(bc);
> > 			bc = NULL;
> > 			break;
> > 		}
> > 
> > 		bc->uhdr = (struct sg_io_v4 __user *) buf;
> > 		if (copy_from_user(&bc->hdr, buf, sizeof(bc->hdr))) {
> > 			ret = -EFAULT;
> > 			break;
> > 		}
> > 
> > 		/*
> > 		 * get a request, fill in the blanks, and add to request queue
> > 		 */
> > 		rq = bsg_map_hdr(bd, &bc->hdr);
> > 		if (IS_ERR(rq)) {
> > 			ret = PTR_ERR(rq);
> > 			rq = NULL;
> > 			break;
> > 		}
> > 
> > 		bsg_add_command(bd, q, bc, rq);
> > 		bc = NULL;
> > 		rq = NULL;
> > 		nr_commands--;
> > 		buf += sizeof(struct sg_io_v4);
> > 		*bytes_read += sizeof(struct sg_io_v4);
> > 	}
> > 
> > 	if (bc)
> > 		bsg_free_command(bc);
> > 
> > 	return ret;
> > }
> 
> Return type should be `int'.

Fixed

> > static ssize_t
> > bsg_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> > {
> > 	struct bsg_device *bd = file->private_data;
> > 	ssize_t bytes_read;
> 
> This variable should be called bytes_written.

Indeed, probably a copy-paste from bsg_read().

> > 	int ret;
> > 
> > 	dprintk("%s: write %Zd bytes\n", bd->name, count);
> > 
> > 	bsg_set_block(bd, file);
> > 	bsg_set_write_perm(bd, file);
> > 
> > 	bytes_read = 0;
> > 	ret = __bsg_write(bd, buf, count, &bytes_read);
> > 	*ppos = bytes_read;
> > 
> > 	/*
> > 	 * return bytes written on non-fatal errors
> > 	 */
> > 	if (!bytes_read || (bytes_read && err_block_err(ret)))
> > 		bytes_read = ret;
> > 	dprintk("%s: returning %Zd\n", bd->name, bytes_read);
> > 	return bytes_read;
> > }
> > 
> > ...
> 
> > 
> > static struct bsg_device *bsg_add_device(struct inode *inode,
> > 					 struct request_queue *rq,
> > 					 struct file *file)
> > {
> > 	struct bsg_device *bd = NULL;
> 
> Unneeded initialisation.

Fixed

> > #ifdef BSG_DEBUG
> > 	unsigned char buf[32];
> > #endif
> > 
> > 	bd = bsg_alloc_device();
> > 	if (!bd)
> > 		return ERR_PTR(-ENOMEM);
> > 
> > 	bd->queue = rq;
> > 	kobject_get(&rq->kobj);
> > 	bsg_set_block(bd, file);
> > 
> > 	atomic_set(&bd->ref_count, 1);
> > 	bd->minor = iminor(inode);
> > 	mutex_lock(&bsg_mutex);
> > 	hlist_add_head(&bd->dev_list, &bsg_device_list[bsg_list_idx(bd->minor)]);
> > 
> > 	strncpy(bd->name, rq->bsg_dev.class_dev->class_id, sizeof(bd->name) - 1);
> > 	dprintk("bound to <%s>, max queue %d\n",
> > 		format_dev_t(buf, inode->i_rdev), bd->max_queue);
> > 
> > 	mutex_unlock(&bsg_mutex);
> > 	return bd;
> > }
> > 
> > ...
> >
> > static int
> > bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> > 	  unsigned long arg)
> 
> This is an file_operations.ioctl() method.  It should instead have been
> implemented as an unlocked_ioctl handler.

Fixed.

> > {
> > 	struct bsg_device *bd = file->private_data;
> > 	int __user *uarg = (int __user *) arg;
> > 
> > 	if (!bd)
> > 		return -ENXIO;
> 
> This cannot happen, surely?

Nope, removed.

> > 	switch (cmd) {
> > 		/*
> > 		 * our own ioctls
> > 		 */
> > 	case SG_GET_COMMAND_Q:
> > 		return put_user(bd->max_queue, uarg);
> > 	case SG_SET_COMMAND_Q: {
> > 		int queue;
> > 
> > 		if (get_user(queue, uarg))
> > 			return -EFAULT;
> > 		if (queue < 1)
> > 			return -EINVAL;
> > 
> > 		spin_lock_irq(&bd->lock);
> > 		bd->max_queue = queue;
> > 		spin_unlock_irq(&bd->lock);
> > 		return 0;
> > 	}
> > 
> > 	/*
> > 	 * SCSI/sg ioctls
> > 	 */
> > 	case SG_GET_VERSION_NUM:
> > 	case SCSI_IOCTL_GET_IDLUN:
> > 	case SCSI_IOCTL_GET_BUS_NUMBER:
> > 	case SG_SET_TIMEOUT:
> > 	case SG_GET_TIMEOUT:
> > 	case SG_GET_RESERVED_SIZE:
> > 	case SG_SET_RESERVED_SIZE:
> > 	case SG_EMULATED_HOST:
> > 	case SCSI_IOCTL_SEND_COMMAND: {
> > 		void __user *uarg = (void __user *) arg;
> > 		return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
> > 	}
> > 	case SG_IO: {
> > 		struct request *rq;
> > 		struct bio *bio, *bidi_bio = NULL;
> > 		struct sg_io_v4 hdr;
> > 
> > 		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
> > 			return -EFAULT;
> > 
> > 		rq = bsg_map_hdr(bd, &hdr);
> > 		if (IS_ERR(rq))
> > 			return PTR_ERR(rq);
> > 
> > 		bio = rq->bio;
> > 		if (rq->next_rq)
> > 			bidi_bio = rq->next_rq->bio;
> > 		blk_execute_rq(bd->queue, NULL, rq, 0);
> > 		blk_complete_sgv4_hdr_rq(rq, &hdr, bio, bidi_bio);
> > 
> > 		if (copy_to_user(uarg, &hdr, sizeof(hdr)))
> > 			return -EFAULT;
> > 
> > 		return 0;
> > 	}
> > 	/*
> > 	 * block device ioctls
> > 	 */
> > 	default:
> > #if 0
> > 		return ioctl_by_bdev(bd->bdev, cmd, arg);
> > #else
> > 		return -ENOTTY;
> > #endif
> > 	}
> > }
> 
> So we perform IO operations on this "device" by opening it and running
> ioctls, the args to which point to fairly complex data structures which lie
> in userspace and which contain addresses and lengths of userspace IO
> buffers?
> 
> What did Christoph think of this? :)

The sg v3 duplicates should go, it's remnant of when bsg was a sg v3
driver.

> > static struct file_operations bsg_fops = {
> > 	.read		=	bsg_read,
> > 	.write		=	bsg_write,
> > 	.poll		=	bsg_poll,
> > 	.open		=	bsg_open,
> > 	.release	=	bsg_release,
> > 	.ioctl		=	bsg_ioctl,
> 
> unlocked_ioctl, please.

Fixed

> > 	.owner		=	THIS_MODULE,
> > };
> > 
> > void bsg_unregister_queue(struct request_queue *q)
> > {
> > 	struct bsg_class_device *bcd = &q->bsg_dev;
> > 
> > 	if (!bcd->class_dev)
> > 		return;
> 
> Can this happen?

Probably only for double unregister, which means it should probably just
be a WARN_ON() or something instead.

> > 	mutex_lock(&bsg_mutex);
> > 	sysfs_remove_link(&q->kobj, "bsg");
> > 	class_device_destroy(bsg_class, MKDEV(BSG_MAJOR, bcd->minor));
> > 	bcd->class_dev = NULL;
> > 	list_del_init(&bcd->list);
> > 	bsg_device_nr--;
> > 	mutex_unlock(&bsg_mutex);
> > }
> > EXPORT_SYMBOL_GPL(bsg_unregister_queue);
> 
> <still wondering what all this code does>
> 
> Would I be correct in assuming that it offers services to device drivers,
> which have yet to be hooked up?

Yes. As mentioned many lines up, it is a SCSI generic type driver that
uses the (now) defined version 4 command structure. So it'll get hooked
up to ny capable device.

> > int bsg_register_queue(struct request_queue *q, const char *name)
> > {
> > 	struct bsg_class_device *bcd, *__bcd;
> > 	dev_t dev;
> > 	int ret = -EMFILE;
> > 	struct class_device *class_dev = NULL;
> > 
> > 	/*
> > 	 * we need a proper transport to send commands, not a stacked device
> > 	 */
> > 	if (!q->request_fn)
> > 		return 0;
> > 
> > 	bcd = &q->bsg_dev;
> > 	memset(bcd, 0, sizeof(*bcd));
> > 	INIT_LIST_HEAD(&bcd->list);
> > 
> > 	mutex_lock(&bsg_mutex);
> > 	if (bsg_device_nr == BSG_MAX_DEVS) {
> > 		printk(KERN_ERR "bsg: too many bsg devices\n");
> 
> 32768 is a lot of devices.  Why is there any limit at all?

It is pretty pointless killed.

> > 		goto err;
> > 	}
> > 
> > retry:
> > 	list_for_each_entry(__bcd, &bsg_class_list, list) {
> > 		if (__bcd->minor == bsg_minor_idx) {
> > 			bsg_minor_idx++;
> > 			if (bsg_minor_idx == BSG_MAX_DEVS)
> > 				bsg_minor_idx = 0;
> > 			goto retry;
> > 		}
> > 	}
> > 
> > 	bcd->minor = bsg_minor_idx++;
> > 	if (bsg_minor_idx == BSG_MAX_DEVS)
> > 		bsg_minor_idx = 0;
> 
> So what's happening here?  We're doing a linear, potentially O(n^2) search
> for a unique minor number?
> 
> I expect that you'll find that lib/idr.c provides a more elegant solution. 
> The tty code uses this, and there are other examples around the place.

idr will do nicely I think. I'll punt that to Tomo, he is the bsg
maintainer.

> > 	bcd->queue = q;
> > 	dev = MKDEV(BSG_MAJOR, bcd->minor);
> > 	class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);
> > 	if (IS_ERR(class_dev)) {
> > 		ret = PTR_ERR(class_dev);
> > 		goto err;
> > 	}
> > 	bcd->class_dev = class_dev;
> > 
> > 	if (q->kobj.sd) {
> > 		ret = sysfs_create_link(&q->kobj, &bcd->class_dev->kobj, "bsg");
> > 		if (ret)
> > 			goto err;
> > 	}
> > 
> > 	list_add_tail(&bcd->list, &bsg_class_list);
> > 	bsg_device_nr++;
> > 
> > 	mutex_unlock(&bsg_mutex);
> > 	return 0;
> > err:
> > 	if (class_dev)
> > 		class_device_destroy(bsg_class, MKDEV(BSG_MAJOR, bcd->minor));
> > 	mutex_unlock(&bsg_mutex);
> > 	return ret;
> > }
> > EXPORT_SYMBOL_GPL(bsg_register_queue);
> > 
> > ...
> >
> 
> 
> In terms of presentation: this code hit the tree as base patch plus what
> appear to be 20 bugfixes, none of which are really interesting or relevant
> to mainline.  Personally I think it would be nicer if all that out-of-tree
> development work was cleaned up and the new code goes in as a single hit.
> 
> This makes it a lot easier to find out "wtf does this code all do".  One
> finds the first commit and reads the changlog.  But this algorithm yields:
> 
>     bsg: support for full generic block layer SG v3
> 
> which is not helpful.

I agree, I did consider rebasing the merging all patches into a single
commit prior to submission. In retrospect that would have been better,
the bug fixes commits prior to inclusion is not that interesting.

-- 
Jens Axboe

Re: block/bsg.c

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 08:38:11 +0200

> On Mon, Jul 16 2007, Andrew Morton wrote:
> > 
> > A belated review (I've never seen this before and there it is in mainline)
> > 
> > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > 
> > `const' would be better.  That moves it into a write-protected memory section.
> 
> Agree
> 
> > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > 
> > This makes the code easier to write but harder to read.  We should optimise
> > for readers.  Please open-code this at callsites.
> > 
> > Or at least convert it into a (commented) (possibly inlined) C function.
> 
> list_entry_to_bc(), then? The main objective is to save on typing, and
> (just as important) make sure we don't bump over the 80 chars per line.
> 
> > > /*
> > >  * just for testing
> > >  */
> > > #define BSG_MAJOR	(240)
> > 
> > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > This will cause collisions.  This code should be using dynamic major
> > assignment.
> 
> Yeah, that's a big error on my part. Will get that fixed up right away.

I've been testing the patchset to fix the issues that Andrew pointed
out. I can send it soon.

Jens, have you already fixed some?

Re: block/bsg.c

[Jens Axboe]

On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: block/bsg.c
> Date: Tue, 17 Jul 2007 08:38:11 +0200
> 
> > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > 
> > > A belated review (I've never seen this before and there it is in mainline)
> > > 
> > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > 
> > > `const' would be better.  That moves it into a write-protected memory section.
> > 
> > Agree
> > 
> > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > 
> > > This makes the code easier to write but harder to read.  We should optimise
> > > for readers.  Please open-code this at callsites.
> > > 
> > > Or at least convert it into a (commented) (possibly inlined) C function.
> > 
> > list_entry_to_bc(), then? The main objective is to save on typing, and
> > (just as important) make sure we don't bump over the 80 chars per line.
> > 
> > > > /*
> > > >  * just for testing
> > > >  */
> > > > #define BSG_MAJOR	(240)
> > > 
> > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > This will cause collisions.  This code should be using dynamic major
> > > assignment.
> > 
> > Yeah, that's a big error on my part. Will get that fixed up right away.
> 
> I've been testing the patchset to fix the issues that Andrew pointed
> out. I can send it soon.
> 
> Jens, have you already fixed some?

I already pushed some of the changes locally, just the idr change is
missing. See the #bsg branch:

http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg

I'll ask for that to get pulled, and can I then assume that you will
look after bsg from now on?

-- 
Jens Axboe

Re: block/bsg.c

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 08:59:40 +0200

> On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: block/bsg.c
> > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > 
> > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > 
> > > > A belated review (I've never seen this before and there it is in mainline)
> > > > 
> > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > 
> > > > `const' would be better.  That moves it into a write-protected memory section.
> > > 
> > > Agree
> > > 
> > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > 
> > > > This makes the code easier to write but harder to read.  We should optimise
> > > > for readers.  Please open-code this at callsites.
> > > > 
> > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > 
> > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > (just as important) make sure we don't bump over the 80 chars per line.
> > > 
> > > > > /*
> > > > >  * just for testing
> > > > >  */
> > > > > #define BSG_MAJOR	(240)
> > > > 
> > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > This will cause collisions.  This code should be using dynamic major
> > > > assignment.
> > > 
> > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > 
> > I've been testing the patchset to fix the issues that Andrew pointed
> > out. I can send it soon.
> > 
> > Jens, have you already fixed some?
> 
> I already pushed some of the changes locally, just the idr change is
> missing. See the #bsg branch:
> 
> http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg

I see.


> I'll ask for that to get pulled, and can I then assume that you will
> look after bsg from now on?

I'm happy to do that.

Re: block/bsg.c

[Jens Axboe]

On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: block/bsg.c
> Date: Tue, 17 Jul 2007 08:59:40 +0200
> 
> > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > From: Jens Axboe <jens.axboe@oracle.com>
> > > Subject: Re: block/bsg.c
> > > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > > 
> > > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > > 
> > > > > A belated review (I've never seen this before and there it is in mainline)
> > > > > 
> > > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > > 
> > > > > `const' would be better.  That moves it into a write-protected memory section.
> > > > 
> > > > Agree
> > > > 
> > > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > > 
> > > > > This makes the code easier to write but harder to read.  We should optimise
> > > > > for readers.  Please open-code this at callsites.
> > > > > 
> > > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > > 
> > > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > > (just as important) make sure we don't bump over the 80 chars per line.
> > > > 
> > > > > > /*
> > > > > >  * just for testing
> > > > > >  */
> > > > > > #define BSG_MAJOR	(240)
> > > > > 
> > > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > > This will cause collisions.  This code should be using dynamic major
> > > > > assignment.
> > > > 
> > > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > > 
> > > I've been testing the patchset to fix the issues that Andrew pointed
> > > out. I can send it soon.
> > > 
> > > Jens, have you already fixed some?
> > 
> > I already pushed some of the changes locally, just the idr change is
> > missing. See the #bsg branch:
> > 
> > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg
> 
> I see.

Since Linus is happily snoring by now, could you test and see if the
tree works for you?

> > I'll ask for that to get pulled, and can I then assume that you will
> > look after bsg from now on?
> 
> I'm happy to do that.

Great!

-- 
Jens Axboe

Re: block/bsg.c

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 09:10:45 +0200

> On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: block/bsg.c
> > Date: Tue, 17 Jul 2007 08:59:40 +0200
> > 
> > > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > > From: Jens Axboe <jens.axboe@oracle.com>
> > > > Subject: Re: block/bsg.c
> > > > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > > > 
> > > > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > > > 
> > > > > > A belated review (I've never seen this before and there it is in mainline)
> > > > > > 
> > > > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > > > 
> > > > > > `const' would be better.  That moves it into a write-protected memory section.
> > > > > 
> > > > > Agree
> > > > > 
> > > > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > > > 
> > > > > > This makes the code easier to write but harder to read.  We should optimise
> > > > > > for readers.  Please open-code this at callsites.
> > > > > > 
> > > > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > > > 
> > > > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > > > (just as important) make sure we don't bump over the 80 chars per line.
> > > > > 
> > > > > > > /*
> > > > > > >  * just for testing
> > > > > > >  */
> > > > > > > #define BSG_MAJOR	(240)
> > > > > > 
> > > > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > > > This will cause collisions.  This code should be using dynamic major
> > > > > > assignment.
> > > > > 
> > > > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > > > 
> > > > I've been testing the patchset to fix the issues that Andrew pointed
> > > > out. I can send it soon.
> > > > 
> > > > Jens, have you already fixed some?
> > > 
> > > I already pushed some of the changes locally, just the idr change is
> > > missing. See the #bsg branch:
> > > 
> > > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg
> > 
> > I see.
> 
> Since Linus is happily snoring by now, could you test and see if the
> tree works for you?

Yeah, I'll do and send patches later if necessary.

Re: block/bsg.c

[Jens Axboe]

On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: block/bsg.c
> Date: Tue, 17 Jul 2007 09:10:45 +0200
> 
> > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > From: Jens Axboe <jens.axboe@oracle.com>
> > > Subject: Re: block/bsg.c
> > > Date: Tue, 17 Jul 2007 08:59:40 +0200
> > > 
> > > > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > > > From: Jens Axboe <jens.axboe@oracle.com>
> > > > > Subject: Re: block/bsg.c
> > > > > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > > > > 
> > > > > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > > > > 
> > > > > > > A belated review (I've never seen this before and there it is in mainline)
> > > > > > > 
> > > > > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > > > > 
> > > > > > > `const' would be better.  That moves it into a write-protected memory section.
> > > > > > 
> > > > > > Agree
> > > > > > 
> > > > > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > > > > 
> > > > > > > This makes the code easier to write but harder to read.  We should optimise
> > > > > > > for readers.  Please open-code this at callsites.
> > > > > > > 
> > > > > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > > > > 
> > > > > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > > > > (just as important) make sure we don't bump over the 80 chars per line.
> > > > > > 
> > > > > > > > /*
> > > > > > > >  * just for testing
> > > > > > > >  */
> > > > > > > > #define BSG_MAJOR	(240)
> > > > > > > 
> > > > > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > > > > This will cause collisions.  This code should be using dynamic major
> > > > > > > assignment.
> > > > > > 
> > > > > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > > > > 
> > > > > I've been testing the patchset to fix the issues that Andrew pointed
> > > > > out. I can send it soon.
> > > > > 
> > > > > Jens, have you already fixed some?
> > > > 
> > > > I already pushed some of the changes locally, just the idr change is
> > > > missing. See the #bsg branch:
> > > > 
> > > > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg
> > > 
> > > I see.
> > 
> > Since Linus is happily snoring by now, could you test and see if the
> > tree works for you?
> 
> Yeah, I'll do and send patches later if necessary.

Great, thanks.

-- 
Jens Axboe

Re: block/bsg.c

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 08:38:11 +0200

> > As a reader of this code I'm wondering "hm, why is
> > BSG_SUB_PROTOCOL_SCSI_CMD unprivileged, while other modes require
> > CAP_SYS_RAWIO"?.
> >
> > This design/policy decision maybe was discussed on a mailing list
> > somewhere, or even perhaps in a changelog (although I can't find it).  But
> > it is so important, and is so unobvious from a reading of the code that I'd
> > suggest that it is worth some discussion right here, in a code comment.
> >
> It's not unprivileged, it goes through the blk_verify_command() check
> list.

SCSI commands goes to blk_verify_command() like SG v3
(block/scsi_ioctl.c and drivers/scsi/sg.c).

Except for SCSI commands, only SAS Management Protocol (SMP) is
supported now. Sending management requests needs CAP_SYS_RAWIO, that's
a reasonable policy, I guess.


> > And `struct sg_io_v4' has just become part of the kernel ABI?  Beware that
> > there is a move afoot to require test code, manpages and even LTP testcases
> > for new ABI extensions.  Is this interface documented anywhere?
> 
> The documentation is likely very scarce atm, if anything. The command
> layout was discussed at the storage summit and on linux-scsi.

Not the latest, but Doug's RFC:

http://lwn.net/Articles/208082/

gives pretty good information.



> > Would I be correct in assuming that it offers services to device drivers,
> > which have yet to be hooked up?
> 
> Yes. As mentioned many lines up, it is a SCSI generic type driver that
> uses the (now) defined version 4 command structure. So it'll get hooked
> up to ny capable device.

The SAS transport class use bsg to handle SAS Management
Protocol. Every SAS object (host, device, expander, etc) has the own
bsg device. They call bsg_register_queue() for the own request
queue. Users can send SMP requests to any object via its bsg device.

http://marc.info/?l=linux-scsi&m=118395317724148&w=2

I guess that James plan to put SMP patches in the second SCSI merge
for 2.6.22.

Re: block/bsg.c

[Jan Engelhardt]

On Jul 16 2007 16:57, Andrew Morton wrote:
>A belated review (I've never seen this before and there it is in mainline)
>
>> static char bsg_version[] = "block layer sg (bsg) 0.4";
>
>`const' would be better.  That moves it into a write-protected memory section.

Or perhaps MODULE_DESCRIPTION() and MODULE_VERSION()?


	Jan
-- 

Re: block/bsg.c

[FUJITA Tomonori]

From: Jens Axboe <jens.axboe@oracle.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 09:10:45 +0200

> On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > From: Jens Axboe <jens.axboe@oracle.com>
> > Subject: Re: block/bsg.c
> > Date: Tue, 17 Jul 2007 08:59:40 +0200
> > 
> > > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > > From: Jens Axboe <jens.axboe@oracle.com>
> > > > Subject: Re: block/bsg.c
> > > > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > > > 
> > > > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > > > 
> > > > > > A belated review (I've never seen this before and there it is in mainline)
> > > > > > 
> > > > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > > > 
> > > > > > `const' would be better.  That moves it into a write-protected memory section.
> > > > > 
> > > > > Agree
> > > > > 
> > > > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > > > 
> > > > > > This makes the code easier to write but harder to read.  We should optimise
> > > > > > for readers.  Please open-code this at callsites.
> > > > > > 
> > > > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > > > 
> > > > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > > > (just as important) make sure we don't bump over the 80 chars per line.
> > > > > 
> > > > > > > /*
> > > > > > >  * just for testing
> > > > > > >  */
> > > > > > > #define BSG_MAJOR	(240)
> > > > > > 
> > > > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > > > This will cause collisions.  This code should be using dynamic major
> > > > > > assignment.
> > > > > 
> > > > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > > > 
> > > > I've been testing the patchset to fix the issues that Andrew pointed
> > > > out. I can send it soon.
> > > > 
> > > > Jens, have you already fixed some?
> > > 
> > > I already pushed some of the changes locally, just the idr change is
> > > missing. See the #bsg branch:
> > > 
> > > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg
> > 
> > I see.
> 
> Since Linus is happily snoring by now, could you test and see if the
> tree works for you?

It works for me. I'll submit some minor patches against your bsg
branch to scsi-ml. Can you push them together?

Thanks,

Re: block/bsg.c

[Jens Axboe]

On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> From: Jens Axboe <jens.axboe@oracle.com>
> Subject: Re: block/bsg.c
> Date: Tue, 17 Jul 2007 09:10:45 +0200
> 
> > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > From: Jens Axboe <jens.axboe@oracle.com>
> > > Subject: Re: block/bsg.c
> > > Date: Tue, 17 Jul 2007 08:59:40 +0200
> > > 
> > > > On Tue, Jul 17 2007, FUJITA Tomonori wrote:
> > > > > From: Jens Axboe <jens.axboe@oracle.com>
> > > > > Subject: Re: block/bsg.c
> > > > > Date: Tue, 17 Jul 2007 08:38:11 +0200
> > > > > 
> > > > > > On Mon, Jul 16 2007, Andrew Morton wrote:
> > > > > > > 
> > > > > > > A belated review (I've never seen this before and there it is in mainline)
> > > > > > > 
> > > > > > > > static char bsg_version[] = "block layer sg (bsg) 0.4";
> > > > > > > 
> > > > > > > `const' would be better.  That moves it into a write-protected memory section.
> > > > > > 
> > > > > > Agree
> > > > > > 
> > > > > > > > #define list_entry_bc(entry)	list_entry((entry), struct bsg_command, list)
> > > > > > > 
> > > > > > > This makes the code easier to write but harder to read.  We should optimise
> > > > > > > for readers.  Please open-code this at callsites.
> > > > > > > 
> > > > > > > Or at least convert it into a (commented) (possibly inlined) C function.
> > > > > > 
> > > > > > list_entry_to_bc(), then? The main objective is to save on typing, and
> > > > > > (just as important) make sure we don't bump over the 80 chars per line.
> > > > > > 
> > > > > > > > /*
> > > > > > > >  * just for testing
> > > > > > > >  */
> > > > > > > > #define BSG_MAJOR	(240)
> > > > > > > 
> > > > > > > What's this doing in mainline?  240 is a "reserved for local use" major. 
> > > > > > > This will cause collisions.  This code should be using dynamic major
> > > > > > > assignment.
> > > > > > 
> > > > > > Yeah, that's a big error on my part. Will get that fixed up right away.
> > > > > 
> > > > > I've been testing the patchset to fix the issues that Andrew pointed
> > > > > out. I can send it soon.
> > > > > 
> > > > > Jens, have you already fixed some?
> > > > 
> > > > I already pushed some of the changes locally, just the idr change is
> > > > missing. See the #bsg branch:
> > > > 
> > > > http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=bsg
> > > 
> > > I see.
> > 
> > Since Linus is happily snoring by now, could you test and see if the
> > tree works for you?
> 
> It works for me. I'll submit some minor patches against your bsg
> branch to scsi-ml. Can you push them together?

Certainly, I'll pull them into the bsg branch.

-- 
Jens Axboe

[PATCH] Don't define empty struct bsg_class_device if !CONFIG_BLK_DEV_BSG (was: Re: block/bsg.c)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1436 bytes --]

Don't define an empty struct bsg_class_device if !CONFIG_BLK_DEV_BSG.

It's embedded in struct request_queue, but there we have

#if defined(CONFIG_BLK_DEV_BSG)
	struct bsg_class_device bsg_dev;
#endif

anyway.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
At first I considered dropping the #ifdef around request_queue.bsg_dev, but
that would make the code more error-prone.

 include/linux/bsg.h |    1 -
 1 files changed, 1 deletion(-)

--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -60,7 +60,6 @@ struct bsg_class_device {
 extern int bsg_register_queue(struct request_queue *, const char *);
 extern void bsg_unregister_queue(struct request_queue *);
 #else
-struct bsg_class_device { };
 #define bsg_register_queue(disk, name)		(0)
 #define bsg_unregister_queue(disk)	do { } while (0)
 #endif

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH] Don't define empty struct bsg_class_device if !CONFIG_BLK_DEV_BSG (was: Re: block/bsg.c)

[Jens Axboe]

On Tue, Jul 17 2007, Geert Uytterhoeven wrote:
> Don't define an empty struct bsg_class_device if !CONFIG_BLK_DEV_BSG.
> 
> It's embedded in struct request_queue, but there we have
> 
> #if defined(CONFIG_BLK_DEV_BSG)
> 	struct bsg_class_device bsg_dev;
> #endif
> 
> anyway.

Thanks, applied.

-- 
Jens Axboe

Re: block/bsg.c

[James Bottomley]

On Tue, 2007-07-17 at 12:19 +0200, Jens Axboe wrote:
> > > Since Linus is happily snoring by now, could you test and see if the
> > > tree works for you?
> > 
> > It works for me. I'll submit some minor patches against your bsg
> > branch to scsi-ml. Can you push them together?
> 
> Certainly, I'll pull them into the bsg branch.

While you're at it, here's a patch to separate BSG and SCSI again (so
SCSI can be built modular).  The way I did it was simply to move the
SCSI specific logic into SCSI.  When you come up with a generic way to
register the bsg requiring drivers, then we can move it out again.

James

diff --git a/block/Kconfig b/block/Kconfig
index 6597b60..3468d75 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -53,7 +53,7 @@ endif # BLOCK
 
 config BLK_DEV_BSG
 	bool "Block layer SG support"
-	depends on (SCSI=y) && EXPERIMENTAL
+	depends on EXPERIMENTAL
 	default y
 	---help---
 	Saying Y here will enable generic SG (SCSI generic) v4
diff --git a/block/bsg.c b/block/bsg.c
index 576933f..1d9e6f7 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -1030,29 +1030,6 @@ err:
 }
 EXPORT_SYMBOL_GPL(bsg_register_queue);
 
-static int bsg_add(struct class_device *cl_dev, struct class_interface *cl_intf)
-{
-	int ret;
-	struct scsi_device *sdp = to_scsi_device(cl_dev->dev);
-	struct request_queue *rq = sdp->request_queue;
-
-	if (rq->kobj.parent)
-		ret = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
-	else
-		ret = bsg_register_queue(rq, kobject_name(&sdp->sdev_gendev.kobj));
-	return ret;
-}
-
-static void bsg_remove(struct class_device *cl_dev, struct class_interface *cl_intf)
-{
-	bsg_unregister_queue(to_scsi_device(cl_dev->dev)->request_queue);
-}
-
-static struct class_interface bsg_intf = {
-	.add	= bsg_add,
-	.remove	= bsg_remove,
-};
-
 static struct cdev bsg_cdev = {
 	.kobj   = {.name = "bsg", },
 	.owner  = THIS_MODULE,
@@ -1094,15 +1071,6 @@ static int __init bsg_init(void)
 		return ret;
 	}
 
-	ret = scsi_register_interface(&bsg_intf);
-	if (ret) {
-		printk(KERN_ERR "bsg: failed register scsi interface %d\n", ret);
-		kmem_cache_destroy(bsg_cmd_cachep);
-		class_destroy(bsg_class);
-		unregister_chrdev(BSG_MAJOR, "bsg");
-		return ret;
-	}
-
 	printk(KERN_INFO "%s loaded\n", bsg_version);
 	return 0;
 }
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ed72086..fcbe94c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -714,6 +714,7 @@ static int attr_add(struct device *dev, struct device_attribute *attr)
 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 {
 	int error, i;
+	struct request_queue *rq = sdev->request_queue;
 
 	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
 		return error;
@@ -733,6 +734,16 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	/* take a reference for the sdev_classdev; this is
 	 * released by the sdev_class .release */
 	get_device(&sdev->sdev_gendev);
+
+	if (rq->kobj.parent)
+		error = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
+	else
+		error = bsg_register_queue(rq, kobject_name(&sdev->sdev_gendev.kobj));
+	if (error) {
+		sdev_printk(KERN_INFO, sdev, "Failed to register bsg queue\n");
+		goto out;
+	}
+
 	if (sdev->host->hostt->sdev_attrs) {
 		for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
 			error = attr_add(&sdev->sdev_gendev,
@@ -779,6 +790,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
 		return;
 
+	bsg_unregister_queue(sdev->request_queue);
 	class_device_unregister(&sdev->sdev_classdev);
 	transport_remove_device(dev);
 	device_del(dev);

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 08:38:11 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:

> > In terms of presentation: this code hit the tree as base patch plus what
> > appear to be 20 bugfixes, none of which are really interesting or relevant
> > to mainline.  Personally I think it would be nicer if all that out-of-tree
> > development work was cleaned up and the new code goes in as a single hit.
> > 
> > This makes it a lot easier to find out "wtf does this code all do".  One
> > finds the first commit and reads the changlog.  But this algorithm yields:
> > 
> >     bsg: support for full generic block layer SG v3
> > 
> > which is not helpful.
> 
> I agree, I did consider rebasing the merging all patches into a single
> commit prior to submission. In retrospect that would have been better,
> the bug fixes commits prior to inclusion is not that interesting.

I'm doing a git-bisect and....

block/bsg.c: In function 'bsg_register_queue':
block/bsg.c:1014: error: 'struct kobject' has no member named 'dentry'

That's easily fixable in config, but it's another reason for doing
that consolidation prior to merging.

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 13:53:54 -0500 James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Tue, 2007-07-17 at 12:19 +0200, Jens Axboe wrote:
> > > > Since Linus is happily snoring by now, could you test and see if the
> > > > tree works for you?
> > > 
> > > It works for me. I'll submit some minor patches against your bsg
> > > branch to scsi-ml. Can you push them together?
> > 
> > Certainly, I'll pull them into the bsg branch.
> 
> While you're at it, here's a patch to separate BSG and SCSI again (so
> SCSI can be built modular).  The way I did it was simply to move the
> SCSI specific logic into SCSI.  When you come up with a generic way to
> register the bsg requiring drivers, then we can move it out again.

I note that block/scsi_ioctl.c is geting compiled with CONFIG_SCSI=n. 
Seems odd.

(Actually, it's failing to compile (in the middle of the bsg series) so I need
to fix it by hand somehow to continue this bisect)

Re: block/bsg.c

[James Bottomley]

On Tue, 2007-07-17 at 12:48 -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2007 13:53:54 -0500 James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > On Tue, 2007-07-17 at 12:19 +0200, Jens Axboe wrote:
> > > > > Since Linus is happily snoring by now, could you test and see if the
> > > > > tree works for you?
> > > > 
> > > > It works for me. I'll submit some minor patches against your bsg
> > > > branch to scsi-ml. Can you push them together?
> > > 
> > > Certainly, I'll pull them into the bsg branch.
> > 
> > While you're at it, here's a patch to separate BSG and SCSI again (so
> > SCSI can be built modular).  The way I did it was simply to move the
> > SCSI specific logic into SCSI.  When you come up with a generic way to
> > register the bsg requiring drivers, then we can move it out again.
> 
> I note that block/scsi_ioctl.c is geting compiled with CONFIG_SCSI=n. 
> Seems odd.

No, that's fine ... the reason scsi_ioctl.c moved to block was precisely
so that non SCSI devices could use the ioctls, thus it should be there
even if SCSI is not.

> (Actually, it's failing to compile (in the middle of the bsg series) so I need
> to fix it by hand somehow to continue this bisect)

I generally use quilt to help with this (just quilt up the fix and apply
and remove it around the bisects).  I seem to get tons of trees with
unrelated breakage right around where the voyager failures are.

James

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 12:18:21 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 17 Jul 2007 08:38:11 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > > In terms of presentation: this code hit the tree as base patch plus what
> > > appear to be 20 bugfixes, none of which are really interesting or relevant
> > > to mainline.  Personally I think it would be nicer if all that out-of-tree
> > > development work was cleaned up and the new code goes in as a single hit.
> > > 
> > > This makes it a lot easier to find out "wtf does this code all do".  One
> > > finds the first commit and reads the changlog.  But this algorithm yields:
> > > 
> > >     bsg: support for full generic block layer SG v3
> > > 
> > > which is not helpful.
> > 
> > I agree, I did consider rebasing the merging all patches into a single
> > commit prior to submission. In retrospect that would have been better,
> > the bug fixes commits prior to inclusion is not that interesting.
> 
> I'm doing a git-bisect and....
> 
> block/bsg.c: In function 'bsg_register_queue':
> block/bsg.c:1014: error: 'struct kobject' has no member named 'dentry'
> 
> That's easily fixable in config, but it's another reason for doing
> that consolidation prior to merging.

So this rather painful and compiler-errorful bisection session ended up at:

commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
Author: Jens Axboe <jens.axboe@oracle.com>
Date:   Mon Jul 9 12:38:05 2007 +0200

    bsg: support for full generic block layer SG v3                                 


What is happening is that my old dual-PIII IDE-PIIX box running
hacked-about FC1 is locking up early in initscripts, just after "Finding
module dependencies".

Config is http://userweb.kernel.org/~akpm/config-vmm.txt with CONFIG_SCSI=n.

Occasionally I'll get an NMI watchdog timeout, but it's a rather
uninteresting one: the stack trace just points up into the idle task.

This:

--- a/drivers/ide/ide.c~a
+++ a/drivers/ide/ide.c
@@ -1052,9 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
 	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
-	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
-	if (err != -ENOTTY)
-		return err;
+//	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
+//	if (err != -ENOTTY)
+//		return err;
 
 	switch (cmd) {
 	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
_

fixes it.


I added this:

--- a/drivers/ide/ide.c~a
+++ a/drivers/ide/ide.c
@@ -1052,7 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
 	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
+	printk("%s: cmd=%d\n", __FUNCTION__, cmd);
 	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
+	printk("%s: err=%d\n", __FUNCTION__, err);
 	if (err != -ENOTTY)
 		return err;
 
_


and got:

default.hotplug used greatest stack depth: 6448 bytes left
hotplug used greatest stack depth: 6396 bytes left
hotplug used greatest stack depth: 5540 bytes left
EXT3 FS on hdc2, internal journal
Adding 1020116k swap on /dev/hdc3.  Priority:-1 extents:1 across:1020116k
generic_ide_ioctl: cmd=21382
generic_ide_ioctl: err=0
generic_ide_ioctl: cmd=1
program scsi_unique_id is using a deprecated SCSI ioctl, please convert it to SG_IO
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

<20000 lines removed>

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 
ide_do_rw_disk - bad command: dev hdc: type=2, flags=104c8

sector 1515870810, nr/cnr 0/0
bio f73714f8, biotail f73714f8, buffer 00000000, data 00000000, len 96
cdb: 12 01 80 00 60 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 


and there it all stopped.

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

Hi,

Some more bsg findings...


block/Kconfig:

endif # BLOCK

Shouldn't BLK_DEV_BSG depend on BLOCK?

config BLK_DEV_BSG
        bool "Block layer SG support"
        depends on (SCSI=y) && EXPERIMENTAL
        default y
        ---help---
        Saying Y here will enable generic SG (SCSI generic) v4
        support for any block device.


block/bsg.c:

...

/*
 * TODO
 *      - Should this get merged, block/scsi_ioctl.c will be migrated into
 *        this file. To keep maintenance down, it's easier to have them
 *        seperated right now.
 *
 */

This TODO should be fixed/removed.

Firstly bsg got merged. ;)  Moreover bsg depends on SCSI and scsi_ioctl doesn't.
Even if SCSI dependency is fixed bsg requires block driver to have struct
class devices which is not a case for scsi_ioctl.

...

static struct bsg_device *__bsg_get_device(int minor)
{
        struct hlist_head *list = &bsg_device_list[bsg_list_idx(minor)];

bsg_device_list[] access should be done under bsg_mutex lock.

May not be a problem currently because of lock_kernel but worth fixing anyway.

        struct bsg_device *bd = NULL;
        struct hlist_node *entry;

        mutex_lock(&bsg_mutex);

...

static int
bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
          unsigned long arg)
{
...
        case SCSI_IOCTL_SEND_COMMAND: {

Do we really want to add support for this *deprecated* ioctl to
the *new* and shiny bsg driver?

                void __user *uarg = (void __user *) arg;
                return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
        }

...

int bsg_register_queue(struct request_queue *q, const char *name)
{
...
        memset(bcd, 0, sizeof(*bcd));
...
        dev = MKDEV(BSG_MAJOR, bcd->minor);
        class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);

bcd->dev is always 0 (NULL).

Is it OK to pass NULL struct device *dev argument to class_device_create()?

It should be fixed by either removing bcd->dev or by setting it to something
other than zero.

...

MODULE_AUTHOR("Jens Axboe");
MODULE_DESCRIPTION("Block layer SGSI generic (sg) driver");

SGSI? :)

MODULE_LICENSE("GPL");


Now back to the first bsg commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3:

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index f429be8..a21f585 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -1258,19 +1258,25 @@ static void idefloppy_create_rw_cmd (idefloppy_floppy_t *floppy, idefloppy_pc_t
 	set_bit(PC_DMA_RECOMMENDED, &pc->flags);
 }
 
-static int
+static void
 idefloppy_blockpc_cmd(idefloppy_floppy_t *floppy, idefloppy_pc_t *pc, struct request *rq)
 {
-	/*
-	 * just support eject for now, it would not be hard to make the
-	 * REQ_BLOCK_PC support fully-featured
-	 */
-	if (rq->cmd[0] != IDEFLOPPY_START_STOP_CMD)
-		return 1;
-
 	idefloppy_init_pc(pc);
+	pc->callback = &idefloppy_rw_callback;
 	memcpy(pc->c, rq->cmd, sizeof(pc->c));
-	return 0;
+	pc->rq = rq;
+	pc->b_count = rq->data_len;
+	if (rq->data_len && rq_data_dir(rq) == WRITE)
+		set_bit(PC_WRITING, &pc->flags);
+	pc->buffer = rq->data;
+	if (rq->bio)
+		set_bit(PC_DMA_RECOMMENDED, &pc->flags);

Great to see SG_IO support being added to ide-floppy but this change has
nothing to do with the addition of bsg driver so WTF they have been mixed
within the same commit?

I also can't recall seeing this patch on linux-ide mailing list...

+	/*
+	 * possibly problematic, doesn't look like ide-floppy correctly
+	 * handled scattered requests if dma fails...
+	 */

Could you give some details on this?

+	pc->request_transfer = pc->buffer_size = rq->data_len;
 }
 
 /*
@@ -1317,10 +1323,7 @@ static ide_startstop_t idefloppy_do_request (ide_drive_t *drive, struct request
 		pc = (idefloppy_pc_t *) rq->buffer;
 	} else if (blk_pc_request(rq)) {
 		pc = idefloppy_next_pc_storage(drive);
-		if (idefloppy_blockpc_cmd(floppy, pc, rq)) {
-			idefloppy_do_end_request(drive, 0, 0);
-			return ide_stopped;
-		}
+		idefloppy_blockpc_cmd(floppy, pc, rq);
 	} else {
 		blk_dump_rq_flags(rq,
 			"ide-floppy: unsupported command in queue");
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index c948a5c..9ae60a7 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c

ide.c changes also have nothing to do with addition of bsg driver.

@@ -1049,9 +1049,13 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 	unsigned long flags;
 	ide_driver_t *drv;
 	void __user *p = (void __user *)arg;
-	int err = 0, (*setfunc)(ide_drive_t *, int);
+	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
+	err = scsi_cmd_ioctl(file, bdev->bd_disk, cmd, p);
+	if (err != -ENOTTY)
+		return err;
+

Probably the goal was to add SG_IO IOCTL support for ide-floppy but instead
it adds support for *all* scsi_ioctl.c IOCTLs to *all* IDE device drivers.

ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
(so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
will result in requests being failed and error messages in the kernel logs).
Without REQ_TYPE_BLOCK_PC support there is no sense in supporting any other
scsi_ioctl.c IOCTLs (while at it: SG_{GET,SET}_RESERVED seems to have no
real effect in the current tree).

ide-cd already supports all scsi_ioctl.c IOCTLs through cdrom_ioctl() call.

This leaves us with ide-floppy where the above scsi_cmd_ioctl() call should
belong (we may also want to filter out deprecated SCSI_IOCTL_SEND_COMMAND
and legacy CDROM_SEND_PACKET IOCTLs).

Please fix it.

 	switch (cmd) {
 	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
 	case HDIO_GET_KEEPSETTINGS: val = &drive->keep_settings; goto read_val;
@@ -1171,10 +1175,6 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 			return 0;
 		}
 
-		case CDROMEJECT:
-		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(file, bdev->bd_disk, cmd, p);
-

This chunk is OK, handling of these IOCTLs should go to ide-floppy
(ide-cd already handles them fine).

Thanks,
Bart

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 22:52:25 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
> (so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
> will result in requests being failed and error messages in the kernel logs).

In my case it results in a completely tits-up machine.  CPU stuck somewhere
spinning with local interrutps disabled.

If you see the log output I sent, I'm suspecting this might be because
this BSG brainfart has exposed underlying bugs in IDE or SCSI.

Note that I saw literally 20,000 lines of output in the bit which I snipped,
so something has gone pretty wild in the handling of these bogus commands.

Re: block/bsg.c

[James Bottomley]

On Tue, 2007-07-17 at 13:22 -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2007 12:18:21 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Tue, 17 Jul 2007 08:38:11 +0200 Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > > In terms of presentation: this code hit the tree as base patch plus what
> > > > appear to be 20 bugfixes, none of which are really interesting or relevant
> > > > to mainline.  Personally I think it would be nicer if all that out-of-tree
> > > > development work was cleaned up and the new code goes in as a single hit.
> > > > 
> > > > This makes it a lot easier to find out "wtf does this code all do".  One
> > > > finds the first commit and reads the changlog.  But this algorithm yields:
> > > > 
> > > >     bsg: support for full generic block layer SG v3
> > > > 
> > > > which is not helpful.
> > > 
> > > I agree, I did consider rebasing the merging all patches into a single
> > > commit prior to submission. In retrospect that would have been better,
> > > the bug fixes commits prior to inclusion is not that interesting.
> > 
> > I'm doing a git-bisect and....
> > 
> > block/bsg.c: In function 'bsg_register_queue':
> > block/bsg.c:1014: error: 'struct kobject' has no member named 'dentry'
> > 
> > That's easily fixable in config, but it's another reason for doing
> > that consolidation prior to merging.
> 
> So this rather painful and compiler-errorful bisection session ended up at:
> 
> commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3
> Author: Jens Axboe <jens.axboe@oracle.com>
> Date:   Mon Jul 9 12:38:05 2007 +0200
> 
>     bsg: support for full generic block layer SG v3                                 
> 
> 
> What is happening is that my old dual-PIII IDE-PIIX box running
> hacked-about FC1 is locking up early in initscripts, just after "Finding
> module dependencies".
> 
> Config is http://userweb.kernel.org/~akpm/config-vmm.txt with CONFIG_SCSI=n.
> 
> Occasionally I'll get an NMI watchdog timeout, but it's a rather
> uninteresting one: the stack trace just points up into the idle task.
> 
> This:
> 
> --- a/drivers/ide/ide.c~a
> +++ a/drivers/ide/ide.c
> @@ -1052,9 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
>  	int err, (*setfunc)(ide_drive_t *, int);
>  	u8 *val;
>  
> -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> -	if (err != -ENOTTY)
> -		return err;
> +//	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> +//	if (err != -ENOTTY)
> +//		return err;
>  
>  	switch (cmd) {
>  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> _
> 
> fixes it.
> 
> 
> I added this:
> 
> --- a/drivers/ide/ide.c~a
> +++ a/drivers/ide/ide.c
> @@ -1052,7 +1052,9 @@ int generic_ide_ioctl(ide_drive_t *drive
>  	int err, (*setfunc)(ide_drive_t *, int);
>  	u8 *val;
>  
> +	printk("%s: cmd=%d\n", __FUNCTION__, cmd);
>  	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> +	printk("%s: err=%d\n", __FUNCTION__, err);
>  	if (err != -ENOTTY)
>  		return err;
>  
> _
> 
> 
> and got:
> 
> default.hotplug used greatest stack depth: 6448 bytes left
> hotplug used greatest stack depth: 6396 bytes left
> hotplug used greatest stack depth: 5540 bytes left
> EXT3 FS on hdc2, internal journal
> Adding 1020116k swap on /dev/hdc3.  Priority:-1 extents:1 across:1020116k
> generic_ide_ioctl: cmd=21382
> generic_ide_ioctl: err=0
> generic_ide_ioctl: cmd=1
> program scsi_unique_id is using a deprecated SCSI ioctl, please convert it to SG_IO

I can tell you what went wrong:

This cmd=1 is SCSI_IOCTL_SEND_COMMAND, but that doesn't seem to be
what's intended ... I'm guessing it's a legacy ide ioctl value which
suddenly has become interpreted as a scsi_ioctl ... and certainly a non
CD IDE device cannot handle a SCSI command, so all hell breaks loose.

I suspect all we really want from this addition is to be able to get
SG_IO on the ide device, in which case this should be the fix (I put a
case statement instead of an if so we can add other ioctl values to it
in case I missed any).

James

diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 8cd7694..9e96662 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -156,6 +156,8 @@
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <scsi/sg.h>
+
 
 /* default maximum number of failures */
 #define IDE_DEFAULT_MAX_FAILURES 	1
@@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 	int err, (*setfunc)(ide_drive_t *, int);
 	u8 *val;
 
-	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
-	if (err != -ENOTTY)
-		return err;
+	switch (cmd) {
+	case SG_IO:
+		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
+	}
 
 	switch (cmd) {
 	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;

Re: block/bsg.c

[FUJITA Tomonori]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 22:52:25 +0200

> /*
>  * TODO
>  *      - Should this get merged, block/scsi_ioctl.c will be migrated into
>  *        this file. To keep maintenance down, it's easier to have them
>  *        seperated right now.
>  *
>  */
> 
> This TODO should be fixed/removed.

Yeah, this should be removed now. I'll do.


> Firstly bsg got merged. ;)  Moreover bsg depends on SCSI and scsi_ioctl doesn't.
> Even if SCSI dependency is fixed bsg requires block driver to have struct
> class devices which is not a case for scsi_ioctl.
> 
> ...
> 
> static struct bsg_device *__bsg_get_device(int minor)
> {
>         struct hlist_head *list = &bsg_device_list[bsg_list_idx(minor)];
> 
> bsg_device_list[] access should be done under bsg_mutex lock.
> 
> May not be a problem currently because of lock_kernel but worth fixing anyway.
> 
>         struct bsg_device *bd = NULL;
>         struct hlist_node *entry;
> 
>         mutex_lock(&bsg_mutex);
> 

They were fixed. Please check the latest code:

git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg

We wait for Linus to pull from it.


> static int
> bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
>           unsigned long arg)
> {
> ...
>         case SCSI_IOCTL_SEND_COMMAND: {
> 
> Do we really want to add support for this *deprecated* ioctl to
> the *new* and shiny bsg driver?
> 
>                 void __user *uarg = (void __user *) arg;
>                 return scsi_cmd_ioctl(file, bd->queue, NULL, cmd, uarg);
>         }

We might remove it.


> int bsg_register_queue(struct request_queue *q, const char *name)
> {
> ...
>         memset(bcd, 0, sizeof(*bcd));
> ...
>         dev = MKDEV(BSG_MAJOR, bcd->minor);
>         class_dev = class_device_create(bsg_class, NULL, dev, bcd->dev, "%s", name);
> 
> bcd->dev is always 0 (NULL).
> 
> Is it OK to pass NULL struct device *dev argument to class_device_create()?

It's ok, I guess.


> It should be fixed by either removing bcd->dev or by setting it to something
> other than zero.
> 
> ...
> 
> MODULE_AUTHOR("Jens Axboe");
> MODULE_DESCRIPTION("Block layer SGSI generic (sg) driver");
> 
> SGSI? :)

It was fixed in the latest code.


Thanks,

Re: block/bsg.c

[Andrew Morton]

On Tue, 17 Jul 2007 17:19:15 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> > Adding 1020116k swap on /dev/hdc3.  Priority:-1 extents:1 across:1020116k
> > generic_ide_ioctl: cmd=21382
> > generic_ide_ioctl: err=0
> > generic_ide_ioctl: cmd=1
> > program scsi_unique_id is using a deprecated SCSI ioctl, please convert it to SG_IO
> 
> I can tell you what went wrong:
> 
> This cmd=1 is SCSI_IOCTL_SEND_COMMAND, but that doesn't seem to be
> what's intended ... I'm guessing it's a legacy ide ioctl value which
> suddenly has become interpreted as a scsi_ioctl ... and certainly a non
> CD IDE device cannot handle a SCSI command, so all hell breaks loose.

An interrupt-off lockup is a pretty bad reaction to an unexpected ioctl
command.   It makes one wonder if tht lockup can be triggered by other means...

Re: block/bsg.c

[James Bottomley]

On Tue, 2007-07-17 at 15:54 -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2007 17:19:15 -0500
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > > Adding 1020116k swap on /dev/hdc3.  Priority:-1 extents:1 across:1020116k
> > > generic_ide_ioctl: cmd=21382
> > > generic_ide_ioctl: err=0
> > > generic_ide_ioctl: cmd=1
> > > program scsi_unique_id is using a deprecated SCSI ioctl, please convert it to SG_IO
> > 
> > I can tell you what went wrong:
> > 
> > This cmd=1 is SCSI_IOCTL_SEND_COMMAND, but that doesn't seem to be
> > what's intended ... I'm guessing it's a legacy ide ioctl value which
> > suddenly has become interpreted as a scsi_ioctl ... and certainly a non
> > CD IDE device cannot handle a SCSI command, so all hell breaks loose.
> 
> An interrupt-off lockup is a pretty bad reaction to an unexpected ioctl
> command.   It makes one wonder if tht lockup can be triggered by other means...

I think this is pretty much par for the course for IDE drivers ... send
it a bogus taskfile (which root is allowed to do) and you'll see it
exhibit the same behaviour.  I suspect what happened is that the driver
expects to treat all REQ_BLOCK_PC requests as taskfiles, so the
wrappered SCSI command got treated as a task file ... the rest you know.

James

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

On Tuesday 17 July 2007, Andrew Morton wrote:
> On Tue, 17 Jul 2007 22:52:25 +0200
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
> > (so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
> > will result in requests being failed and error messages in the kernel logs).
> 
> In my case it results in a completely tits-up machine.  CPU stuck somewhere
> spinning with local interrutps disabled.
> 
> If you see the log output I sent, I'm suspecting this might be because

I don't. :(

> this BSG brainfart has exposed underlying bugs in IDE or SCSI.

Possible but ATM I have an (overdue) IDE update to push, people waiting for
(overdue) patches to try and some (overdue) patches to finish and post...

Would be great if somebody beats me to investigate this issue.

Hint: this would be a great way to redempt pushing buggy commit behind
my back. ;-)

Thanks,
Bart

Re: block/bsg.c

[Jeff Garzik]

James Bottomley wrote:
> @@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
>  	int err, (*setfunc)(ide_drive_t *, int);
>  	u8 *val;
>  
> -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> -	if (err != -ENOTTY)
> -		return err;
> +	switch (cmd) {
> +	case SG_IO:
> +		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> +	}
>  
>  	switch (cmd) {
>  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;


At that point you might as well use an 'if'.

But overall -- agreed.  ACK.

	Jeff

Re: block/bsg.c

[FUJITA Tomonori]

From: James Bottomley <James.Bottomley@SteelEye.com>
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 13:53:54 -0500

> On Tue, 2007-07-17 at 12:19 +0200, Jens Axboe wrote:
> > > > Since Linus is happily snoring by now, could you test and see if the
> > > > tree works for you?
> > > 
> > > It works for me. I'll submit some minor patches against your bsg
> > > branch to scsi-ml. Can you push them together?
> > 
> > Certainly, I'll pull them into the bsg branch.
> 
> While you're at it, here's a patch to separate BSG and SCSI again (so
> SCSI can be built modular).  The way I did it was simply to move the
> SCSI specific logic into SCSI.  When you come up with a generic way to
> register the bsg requiring drivers, then we can move it out again.

Thanks, looks nice.


> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -714,6 +714,7 @@ static int attr_add(struct device *dev, struct device_attribute *attr)
>  int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  {
>  	int error, i;
> +	struct request_queue *rq = sdev->request_queue;
>  
>  	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
>  		return error;
> @@ -733,6 +734,16 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  	/* take a reference for the sdev_classdev; this is
>  	 * released by the sdev_class .release */
>  	get_device(&sdev->sdev_gendev);
> +
> +	if (rq->kobj.parent)
> +		error = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
> +	else
> +		error = bsg_register_queue(rq, kobject_name(&sdev->sdev_gendev.kobj));
> +	if (error) {
> +		sdev_printk(KERN_INFO, sdev, "Failed to register bsg queue\n");
> +		goto out;

Needs more cleanup here?

We might just ignore the error here since it's not fatal not to create
a bsg device, I guess.

I updated the patch against the latest code (which has just be merged
to Linus's tree).


diff --git a/block/Kconfig b/block/Kconfig
index 0768741..93adf61 100644
--- a/block/Kconfig
+++ b/block/Kconfig
@@ -53,7 +53,7 @@ endif # BLOCK
 
 config BLK_DEV_BSG
 	bool "Block layer SG support v4 (EXPERIMENTAL)"
-	depends on (SCSI=y) && EXPERIMENTAL
+ 	depends on EXPERIMENTAL
 	---help---
 	Saying Y here will enable generic SG (SCSI generic) v4 support
 	for any block device.
diff --git a/block/bsg.c b/block/bsg.c
index baa04e7..4e0be1b 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -1009,29 +1009,6 @@ err:
 }
 EXPORT_SYMBOL_GPL(bsg_register_queue);
 
-static int bsg_add(struct class_device *cl_dev, struct class_interface *cl_intf)
-{
-	int ret;
-	struct scsi_device *sdp = to_scsi_device(cl_dev->dev);
-	struct request_queue *rq = sdp->request_queue;
-
-	if (rq->kobj.parent)
-		ret = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
-	else
-		ret = bsg_register_queue(rq, kobject_name(&sdp->sdev_gendev.kobj));
-	return ret;
-}
-
-static void bsg_remove(struct class_device *cl_dev, struct class_interface *cl_intf)
-{
-	bsg_unregister_queue(to_scsi_device(cl_dev->dev)->request_queue);
-}
-
-static struct class_interface bsg_intf = {
-	.add	= bsg_add,
-	.remove	= bsg_remove,
-};
-
 static struct cdev bsg_cdev = {
 	.kobj   = {.name = "bsg", },
 	.owner  = THIS_MODULE,
@@ -1069,16 +1046,9 @@ static int __init bsg_init(void)
 	if (ret)
 		goto unregister_chrdev;
 
-	ret = scsi_register_interface(&bsg_intf);
-	if (ret)
-		goto remove_cdev;
-
 	printk(KERN_INFO BSG_DESCRIPTION " version " BSG_VERSION
 	       " loaded (major %d)\n", bsg_major);
 	return 0;
-remove_cdev:
-	printk(KERN_ERR "bsg: failed register scsi interface %d\n", ret);
-	cdev_del(&bsg_cdev);
 unregister_chrdev:
 	unregister_chrdev_region(MKDEV(bsg_major, 0), BSG_MAX_DEVS);
 destroy_bsg_class:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ed72086..9ebd215 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -714,6 +714,7 @@ static int attr_add(struct device *dev,
 int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 {
 	int error, i;
+	struct request_queue *rq = sdev->request_queue;
 
 	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
 		return error;
@@ -733,6 +734,15 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	/* take a reference for the sdev_classdev; this is
 	 * released by the sdev_class .release */
 	get_device(&sdev->sdev_gendev);
+
+	if (rq->kobj.parent)
+		error = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
+	else
+		error = bsg_register_queue(rq, kobject_name(&sdev->sdev_gendev.kobj));
+
+	if (error)
+		sdev_printk(KERN_INFO, sdev, "Failed to register bsg queue\n");
+
 	if (sdev->host->hostt->sdev_attrs) {
 		for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
 			error = attr_add(&sdev->sdev_gendev,
@@ -779,6 +789,7 @@ void __scsi_remove_device(struct scsi_de
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
 		return;
 
+	bsg_unregister_queue(sdev->request_queue);
 	class_device_unregister(&sdev->sdev_classdev);
 	transport_remove_device(dev);
 	device_del(dev);

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

[ James, please remeber to cc: linux-ide on IDE patches, thanks. ]

On Wednesday 18 July 2007, Jeff Garzik wrote:
> James Bottomley wrote:
> > @@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
> >  	int err, (*setfunc)(ide_drive_t *, int);
> >  	u8 *val;
> >  
> > -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > -	if (err != -ENOTTY)
> > -		return err;
> > +	switch (cmd) {
> > +	case SG_IO:
> > +		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > +	}
> >  
> >  	switch (cmd) {
> >  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> 
> 
> At that point you might as well use an 'if'.
> 
> But overall -- agreed.  ACK.

James/Jeff thanks for following the issue but NAK. ;)

Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.

Luckily Linus has already fixed the issue properly.

BTW cmd == 1 IOCTL is not defined/used by IDE driver.

Thanks,
Bart

Re: block/bsg.c

[James Bottomley]

On Wed, 2007-07-18 at 09:20 +0900, FUJITA Tomonori wrote:
> From: James Bottomley <James.Bottomley@SteelEye.com>
> Subject: Re: block/bsg.c
> Date: Tue, 17 Jul 2007 13:53:54 -0500
> 
> > On Tue, 2007-07-17 at 12:19 +0200, Jens Axboe wrote:
> > > > > Since Linus is happily snoring by now, could you test and see if the
> > > > > tree works for you?
> > > > 
> > > > It works for me. I'll submit some minor patches against your bsg
> > > > branch to scsi-ml. Can you push them together?
> > > 
> > > Certainly, I'll pull them into the bsg branch.
> > 
> > While you're at it, here's a patch to separate BSG and SCSI again (so
> > SCSI can be built modular).  The way I did it was simply to move the
> > SCSI specific logic into SCSI.  When you come up with a generic way to
> > register the bsg requiring drivers, then we can move it out again.
> 
> Thanks, looks nice.

You're welcome ... although there's still a problem for modular builds.
This is what my /sys/class/bsg looks like:

So you see the if (rq->kobj.parent) is causing confusing naming.  The
reason the first one shows up as 0:0:0:0 is because in an initrd
scsi_mod is loaded first (which is when bsg binds) followed by sd_mod
(which is what gives the device the ULD binding and hence the name).  I
don't see any way around this, so I'd advocate simply using the sdev
name rather than the block device name and dumping the if.

> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -714,6 +714,7 @@ static int attr_add(struct device *dev, struct device_attribute *attr)
> >  int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> >  {
> >  	int error, i;
> > +	struct request_queue *rq = sdev->request_queue;
> >  
> >  	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
> >  		return error;
> > @@ -733,6 +734,16 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> >  	/* take a reference for the sdev_classdev; this is
> >  	 * released by the sdev_class .release */
> >  	get_device(&sdev->sdev_gendev);
> > +
> > +	if (rq->kobj.parent)
> > +		error = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
> > +	else
> > +		error = bsg_register_queue(rq, kobject_name(&sdev->sdev_gendev.kobj));
> > +	if (error) {
> > +		sdev_printk(KERN_INFO, sdev, "Failed to register bsg queue\n");
> > +		goto out;
> 
> Needs more cleanup here?

No ... this bit's magic and clever.  Once you've set up the devices and
done a get_device, cleanup is simply doing a put_device because it's all
done in the release routine.

> We might just ignore the error here since it's not fatal not to create
> a bsg device, I guess.
> 
> I updated the patch against the latest code (which has just be merged
> to Linus's tree).

Thanks,

James

Re: block/bsg.c

[James Bottomley]

On Wed, 2007-07-18 at 02:43 +0200, Bartlomiej Zolnierkiewicz wrote:
> [ James, please remeber to cc: linux-ide on IDE patches, thanks. ]

Blame Andrew ... I assumed he'd be reporting the problem to the relevant
lists, so I just did a reply all ...

> On Wednesday 18 July 2007, Jeff Garzik wrote:
> > James Bottomley wrote:
> > > @@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
> > >  	int err, (*setfunc)(ide_drive_t *, int);
> > >  	u8 *val;
> > >  
> > > -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > -	if (err != -ENOTTY)
> > > -		return err;
> > > +	switch (cmd) {
> > > +	case SG_IO:
> > > +		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > +	}
> > >  
> > >  	switch (cmd) {
> > >  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> > 
> > 
> > At that point you might as well use an 'if'.
> > 
> > But overall -- agreed.  ACK.
> 
> James/Jeff thanks for following the issue but NAK. ;)
> 
> Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
> compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.

Well ... that was why I put the case statement in ... I was sure there
would be other ioctls I missed.

> Luckily Linus has already fixed the issue properly.

Actually, no, that's just a reversion.  I think we need the attached to
complete all of this.

> BTW cmd == 1 IOCTL is not defined/used by IDE driver.

Andrew's trace clearly shows that something is sending cmd == 1 down, so
I'm nearly positive at one time this was a legitimate ioctl for IDE.
Anyway it's probably time to kill this deprecated ioctl in SCSI.

James

diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 077fb67..fe65473 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -156,6 +156,7 @@
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <scsi/sg.h>
 
 /* default maximum number of failures */
 #define IDE_DEFAULT_MAX_FAILURES 	1
@@ -1173,6 +1174,7 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
 
 		case CDROMEJECT:
 		case CDROMCLOSETRAY:
+		case SG_IO:
 			return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
 
 		case HDIO_GET_BUSSTATE:

Re: block/bsg.c

[James Bottomley]

On Wed, 2007-07-18 at 08:54 -0500, James Bottomley wrote:
> You're welcome ... although there's still a problem for modular builds.
> This is what my /sys/class/bsg looks like:

Sorry, lets see if I can get the paste to work this time:

jejb@hobholes> ls /sys/class/bsg/
0:0:1:0/  sdb/

> So you see the if (rq->kobj.parent) is causing confusing naming.  The
> reason the first one shows up as 0:0:0:0 is because in an initrd
> scsi_mod is loaded first (which is when bsg binds) followed by sd_mod
> (which is what gives the device the ULD binding and hence the name).  I
> don't see any way around this, so I'd advocate simply using the sdev
> name rather than the block device name and dumping the if.
> 
> > > --- a/drivers/scsi/scsi_sysfs.c
> > > +++ b/drivers/scsi/scsi_sysfs.c
> > > @@ -714,6 +714,7 @@ static int attr_add(struct device *dev, struct device_attribute *attr)
> > >  int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> > >  {
> > >  	int error, i;
> > > +	struct request_queue *rq = sdev->request_queue;
> > >  
> > >  	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
> > >  		return error;
> > > @@ -733,6 +734,16 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> > >  	/* take a reference for the sdev_classdev; this is
> > >  	 * released by the sdev_class .release */
> > >  	get_device(&sdev->sdev_gendev);
> > > +
> > > +	if (rq->kobj.parent)
> > > +		error = bsg_register_queue(rq, kobject_name(rq->kobj.parent));
> > > +	else
> > > +		error = bsg_register_queue(rq, kobject_name(&sdev->sdev_gendev.kobj));
> > > +	if (error) {
> > > +		sdev_printk(KERN_INFO, sdev, "Failed to register bsg queue\n");
> > > +		goto out;
> > 
> > Needs more cleanup here?
> 
> No ... this bit's magic and clever.  Once you've set up the devices and
> done a get_device, cleanup is simply doing a put_device because it's all
> done in the release routine.
> 
> > We might just ignore the error here since it's not fatal not to create
> > a bsg device, I guess.
> > 
> > I updated the patch against the latest code (which has just be merged
> > to Linus's tree).
> 
> Thanks,
> 
> James
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 18 July 2007, James Bottomley wrote:
> On Wed, 2007-07-18 at 02:43 +0200, Bartlomiej Zolnierkiewicz wrote:
> > [ James, please remeber to cc: linux-ide on IDE patches, thanks. ]
> 
> Blame Andrew ... I assumed he'd be reporting the problem to the relevant
> lists, so I just did a reply all ...

No need to blame anybody, especially since it seems that one more person
forgot about adding linux-ide ML.  ;)

> > On Wednesday 18 July 2007, Jeff Garzik wrote:
> > > James Bottomley wrote:
> > > > @@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
> > > >  	int err, (*setfunc)(ide_drive_t *, int);
> > > >  	u8 *val;
> > > >  
> > > > -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > > -	if (err != -ENOTTY)
> > > > -		return err;
> > > > +	switch (cmd) {
> > > > +	case SG_IO:
> > > > +		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > > +	}
> > > >  
> > > >  	switch (cmd) {
> > > >  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> > > 
> > > 
> > > At that point you might as well use an 'if'.
> > > 
> > > But overall -- agreed.  ACK.
> > 
> > James/Jeff thanks for following the issue but NAK. ;)
> > 
> > Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
> > compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.
> 
> Well ... that was why I put the case statement in ... I was sure there
> would be other ioctls I missed.

The thing is that ide-{disk,scsi,tape} really don't support
REQ_TYPE_BLOCK_PC type requests so ide.c::generic_ide_ioctl()
is not the best place to add handling of SG_IO.

Patch attached.

> > Luckily Linus has already fixed the issue properly.
> 
> Actually, no, that's just a reversion.  I think we need the attached to
> complete all of this.
> 
> > BTW cmd == 1 IOCTL is not defined/used by IDE driver.
> 
> Andrew's trace clearly shows that something is sending cmd == 1 down, so

I still haven't got the trace.  Andrew, please (re)send it in PM, thanks.

> I'm nearly positive at one time this was a legitimate ioctl for IDE.

Could be...

> Anyway it's probably time to kill this deprecated ioctl in SCSI.

Fully agreed.

Thanks,
Bart


[PATCH] ide: add support for SCSI ioctls to ide-floppy

Now that ide-floppy supports SG_IO we can add support for SCSI ioctls
(except deprecated SCSI_IOCTL_SEND_COMMAND and legacy CDROM_SEND_PACKET
ones - we can add them later iff really needed).

While at it remove handling of CDROMEJECT and CDROMCLOSETRAY ioctls from
generic_ide_ioctl():

- This prevents ide-{disk,tape,scsi} device drivers from obtaining
  REQ_TYPE_BLOCK_PC type requests which are currently unsupported by
  these drivers and which are potentially harmful (as reported by Andrew).

- There is no functionality loss since aforementioned ioctls will now be
  handled by idefloppy_ioctl()->scsi_cmd_ioctl() (for devices using
  ide-floppy driver) and by idecd_ioctl->cdrom_ioctl()->scsi_cmd_ioctl()
  (for devices using ide-cd driver).

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@steeleye.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/ide/ide-floppy.c |   18 +++++++++++++++++-
 drivers/ide/ide.c        |    4 ----
 2 files changed, 17 insertions(+), 5 deletions(-)

Index: b/drivers/ide/ide-floppy.c
===================================================================
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -99,6 +99,8 @@
 #include <linux/bitops.h>
 #include <linux/mutex.h>
 
+#include <scsi/scsi_ioctl.h>
+
 #include <asm/byteorder.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
@@ -2099,7 +2101,21 @@ static int idefloppy_ioctl(struct inode 
 	case IDEFLOPPY_IOCTL_FORMAT_GET_PROGRESS:
 		return idefloppy_get_format_progress(drive, argp);
 	}
-	return generic_ide_ioctl(drive, file, bdev, cmd, arg);
+
+	/*
+	 * skip SCSI_IOCTL_SEND_COMMAND (deprecated)
+	 * and CDROM_SEND_PACKET (legacy) ioctls
+	 */
+	if (cmd != CDROM_SEND_PACKET && cmd != SCSI_IOCTL_SEND_COMMAND)
+		err = scsi_cmd_ioctl(file, bdev->bd_disk->queue,
+					bdev->bd_disk, cmd, argp);
+	else
+		err = -ENOTTY;
+
+	if (err == -ENOTTY)
+		err = generic_ide_ioctl(drive, file, bdev, cmd, arg);
+
+	return err;
 }
 
 static int idefloppy_media_changed(struct gendisk *disk)
Index: b/drivers/ide/ide.c
===================================================================
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1171,10 +1171,6 @@ int generic_ide_ioctl(ide_drive_t *drive
 			return 0;
 		}
 
-		case CDROMEJECT:
-		case CDROMCLOSETRAY:
-			return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
-
 		case HDIO_GET_BUSSTATE:
 			if (!capable(CAP_SYS_ADMIN))
 				return -EACCES;

Re: block/bsg.c

[Bartlomiej Zolnierkiewicz]

On Wednesday 18 July 2007, FUJITA Tomonori wrote:

> They were fixed. Please check the latest code:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg
> 
> We wait for Linus to pull from it.

It has been merged.  Thanks for fixing the mentioned issues.

Thanks,
Bart

Re: block/bsg.c

[James Bottomley]

On Wed, 2007-07-18 at 22:32 +0200, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On Wednesday 18 July 2007, James Bottomley wrote:
> > On Wed, 2007-07-18 at 02:43 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > [ James, please remeber to cc: linux-ide on IDE patches, thanks. ]
> > 
> > Blame Andrew ... I assumed he'd be reporting the problem to the relevant
> > lists, so I just did a reply all ...
> 
> No need to blame anybody, especially since it seems that one more person
> forgot about adding linux-ide ML.  ;)
> 
> > > On Wednesday 18 July 2007, Jeff Garzik wrote:
> > > > James Bottomley wrote:
> > > > > @@ -1052,9 +1054,10 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
> > > > >  	int err, (*setfunc)(ide_drive_t *, int);
> > > > >  	u8 *val;
> > > > >  
> > > > > -	err = scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > > > -	if (err != -ENOTTY)
> > > > > -		return err;
> > > > > +	switch (cmd) {
> > > > > +	case SG_IO:
> > > > > +		return scsi_cmd_ioctl(file, bdev->bd_disk->queue, bdev->bd_disk, cmd, p);
> > > > > +	}
> > > > >  
> > > > >  	switch (cmd) {
> > > > >  	case HDIO_GET_32BIT:	    val = &drive->io_32bit;	 goto read_val;
> > > > 
> > > > 
> > > > At that point you might as well use an 'if'.
> > > > 
> > > > But overall -- agreed.  ACK.
> > > 
> > > James/Jeff thanks for following the issue but NAK. ;)
> > > 
> > > Causes regression wrt ide-floppy CDROMEJECT/CDROMCLOSETRAY support when
> > > compared to 2.6.22 and SG_IO is not supported by ide-{disk,scsi,tape}.
> > 
> > Well ... that was why I put the case statement in ... I was sure there
> > would be other ioctls I missed.
> 
> The thing is that ide-{disk,scsi,tape} really don't support
> REQ_TYPE_BLOCK_PC type requests so ide.c::generic_ide_ioctl()
> is not the best place to add handling of SG_IO.

SG_IO has become a general packet request ioctl, so I think the idea is
to allow it to send taskfiles down ... I'll let Jens speak to where the
actual pieces to do this currently stand.

> Patch attached.
> 
> > > Luckily Linus has already fixed the issue properly.
> > 
> > Actually, no, that's just a reversion.  I think we need the attached to
> > complete all of this.
> > 
> > > BTW cmd == 1 IOCTL is not defined/used by IDE driver.
> > 
> > Andrew's trace clearly shows that something is sending cmd == 1 down, so
> 
> I still haven't got the trace.  Andrew, please (re)send it in PM, thanks.

http://marc.info/?l=linux-scsi&m=118470384305607

James

Re: block/bsg.c

[FUJITA Tomonori]

From: James Bottomley <James.Bottomley@SteelEye.com>
Subject: Re: block/bsg.c
Date: Wed, 18 Jul 2007 09:23:44 -0500

> On Wed, 2007-07-18 at 08:54 -0500, James Bottomley wrote:
> > You're welcome ... although there's still a problem for modular builds.
> > This is what my /sys/class/bsg looks like:
> 
> Sorry, lets see if I can get the paste to work this time:
> 
> jejb@hobholes> ls /sys/class/bsg/
> 0:0:1:0/  sdb/

Surely, confusing. Let's dump the if and use the sdev name for now.

Thanks,

Re: block/bsg.c

[FUJITA Tomonori]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: Re: block/bsg.c
Date: Wed, 18 Jul 2007 22:39:38 +0200

> 
> On Wednesday 18 July 2007, FUJITA Tomonori wrote:
> 
> > They were fixed. Please check the latest code:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg
> > 
> > We wait for Linus to pull from it.
> 
> It has been merged.  Thanks for fixing the mentioned issues.

You're welcome. Thanks for the ide-floppy fix.