Re: [PATCH] ibmvscsi driver - sixth version

From: Jeff Garzik
Date: Wed Mar 31 2004 - 19:19:43 EST

Dave Boutcher wrote:
On Wed, 31 Mar 2004 17:02:02 -0500, Jeff Garzik <jgarzik@xxxxxxxxx> wrote:


1) Would be nice to eliminate module options for commands-per-lun, max-requests etc. and actually have the driver figure out the real needs. One or two options could fall into the "performance tuning" category, and stay, but the others are really dependent on the underlying configuration and underlying limits.

Hmmm...well, I could collapse the two together, since commands_per_lun
is not limited by anything specific for this adapter. I would prefer
to leave them broken out to handle users who have extreme requirements.

"Do what you need to do, and no more."

Don't engineer code because users _might_ have some outlandish requirement.

2) why is one-descriptor a special case in map_sg_data()? You proceed to do the same thing in a loop, right below that... AFAICS you can just use the loop, and clamp the number of scatterlist elements to one.

The SRP spec has two different buffer formats: SRP_DIRECT_BUFFER for a
single buffer, and SRP_INDIRECT_BUFFER for lists of buffers, and the
layout of the buffers in the request is different for those two cases.

My mistake here -- I was misreading the code as testing both DIRECT_BUFFER and INDIRECT_BUFFER in both the loop and non-sg case.

10) what the heck is this??? do some people not know they are running Linux???
+static ssize_t show_host_os_type(struct class_device *class_dev, char *buf)

This whole driver has to do with adapter sharing....and this answers
the question with whom you are sharing it. Which may in fact NOT
be Linux.

ewwww ;-)

12) in ibmvscsi_probe(), you want to use TASK_UNINTERRUPTIBLE here:

+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(5);

13) in the code pasted in #12, you should pass a value calculated using the 'HZ' constant.

Hmmm...above code copied from the qlogic driver...and it looked reasonable
to me, but I'll tweak it.

Well, qlogic is wrong too. Do you want to submit a patch fixing qlogic while you're at it? ;-)


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at