Re: wd33c93 cleanup patch set

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

Hi,

> I have four patches I'd like to be reviewed and considered for
> inclusion. There's one bug and some clean-ups based on what I'm doing
> in my local branch.

I'm not a SCSI guru, but most your patches seem reasonable.

> * patch-1 fixes a panic if the timed out command isn't currently
> active. This can happen if the device disconnects mid-transfer and the
> controller is then idle (or active on another transfer to another
> device.)

This seems correct fix, acb passed via callout should be used.

> * patch-2 adds a string parameter to wd33c93_error() so the caller can
> specify what the SCSI error was.

Looks reasonable debug info.

> * patch-3 adds a new debug flag to log submitted SCSI requests to the
> driver with some details about what the underlying request is (which i
> found useful for debugging transfer vs sync/async setup stuff.)

I wonder if how ADAPTER_REQ_GROW_RESOURCES should be handled
if the indivitual driver doesn't support it.

Many (most?) major SCSI drivers (ncr53x9c, ncr5390sbc, mb89352 etc.)
have been left with a comment "XXX: Not supported." since
thorpej_scsipi branch merge back in 2001:
 https://github.com/NetBSD/src/commit/937a7a3ed9796b6b06da5c5ae500672154ba4a0e

> * patch-4 splits wd33c93_timeout() into wd33c93_timeout() and
> wd33c93_timeout_callback(). It's being used both as a timeout and as a
> callback, and whilst tinkering with error handling locally I found it
> was useful to have them split so I could do stuff like abort/complete
> the transfer if it's a timeout as there's no subsequent code in that
> path that would actually complete the transfer.
 :
> I still have an outstanding question on the kern list about SCSI
> timeouts and where they're supposed to be handled - for patch-4, I
> don't think the scsipi layer is running its own timeouts on transfers
> - it's asking the driver to do it - and if the driver hits the timeout
> but never completes the transfer it will just be hung/lost for good.

>>  wd33c93: split wd33c93_timeout() into timeout and callback
>>  
>>  The code actually directly calls the same function for both
>>  processing a timeout (and thus sending an abort, bus reset, etc)
>>  and the callout running.
>>  
>>  However, the timeout code doesn't actually complete an acb.
>>  So if an acb times out, it seems to just .. be timed out.
>>  Forever.
>>  
>>  So in preparation for adding support for completing an acb with
>>  timeout, split wd33c93_timeout() into timeout and callout.
>>  
>>  This should be a no-op.

Maybe we should ask SCSI gurus (bouyer? thorpej?) about this, but
AFAICT there are several SCSI drivers that don't handle timeout
errors properly.

As you mentioned, calling a timeout function via callout is just
preparation for future support so it looks reasonable for me.

Thanks,
---
Izumi Tsutsui