lkml.org 
[lkml]   [2004]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ibmvscsi driver - sixth version
Dave Boutcher wrote:
> On Wed, 31 Mar 2004 17:02:02 -0500, Jeff Garzik <jgarzik@pobox.com> wrote:
>
>> Comments:
>>
>> 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? ;-)

Jeff



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

\
 
 \ /
  Last update: 2005-03-22 14:02    [W:0.054 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site