From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [PATCH] ibmvscsi driver - sixth version Date: Wed, 31 Mar 2004 19:16:12 -0500 Sender: linux-scsi-owner@vger.kernel.org Message-ID: <406B5F4C.10000@pobox.com> References: <20040225134518.A4238@infradead.org> <1079027038.2820.57.camel@mulgrave> <406B3FDA.9010507@pobox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from parcelfarce.linux.theplanet.co.uk ([195.92.249.252]:18386 "EHLO www.linux.org.uk") by vger.kernel.org with ESMTP id S262143AbUDAAQ0 (ORCPT ); Wed, 31 Mar 2004 19:16:26 -0500 In-Reply-To: List-Id: linux-scsi@vger.kernel.org To: Dave Boutcher Cc: SCSI Mailing List , linux-kernel@vger.kernel.org Dave Boutcher wrote: > On Wed, 31 Mar 2004 17:02:02 -0500, Jeff Garzik 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