Re: [PATCH] [v2] aha152x cmnd->device oops

["Juergen E. Fischer" <fischer@linux-buechse.de>]

Hi Randy,

On Tue, Oct 28, 2003 at 16:26:10 -0800, Randy.Dunlap wrote:
> On Tue, 28 Oct 2003 14:50:28 -0800 Mike Christie <mikenc@us.ibm.com> wrote:
> 
> | Hi Randy,
> | 
> | >  			if(!(DONE_SC->SCp.Status & not_issued)) {
> | > -				Scsi_Cmnd *cmnd = kmalloc(sizeof(Scsi_Cmnd), GFP_ATOMIC);
> | > +				Scsi_Cmnd *cmnd = scsi_get_command(DONE_SC->device, GFP_ATOMIC);
> | 
> | Do you need to add a scsi_put_command to balance this get when the 
> | command completes?
> | 
> | > +					cmnd->device          = ptr->device;
> | 
> | Don't even need this now. scsi_get_command will set the device for you.
> | 
> | Mike
> 
> Hers's v3 of the aha152x patch.  However, I'm not satisfied with it,
> so I'm not asking James to apply it.  I don't know the state machine
> or the hardware well enough to understand and patch it.

> Things that I don't like about it:
> 
> a.  calling aha152x_internal_queue() with cmnd pointer in 2 places.
>     Probably the second one should be NULL.
> b.  Easy to confuse the state machine by changing just one variable,
>     like DONE_SC (i.e., it's risky and I can't test it, although we
>     do have a bug report and can ask that reporter to test it).
> c.  int result = SCpnt->SCp.Status; /*FIXME*/
>     SCp.Status is not valid here AFAIK, and I don't know how to get
>     the current command status at this point.

There are 2 specially "commands" which are used internally in the driver
only (so I don't see a problem in using kmalloc instead of
scsi_get_command; but nevertheless ->device needs to be intialized
correctly, which is what scsi_get_command() does and the current code
fails to do). 

The first is when a command returns with status CHECK CONDITION and the
driver needs to do a REQUEST SENSE to fetch sense data and add that to
the Ssci_Cmnd which resulted in the CHECK CONDITION.  The internally
queued command fills the sense_buffer of the offending command and then
calls its ->scsi_done().

The second it when the controller needs to be resetted.  Both are purely
internal to the driver.

I'll test this in the evening.


Jürgen

-- 
Phase 1: Where do you want to go today?
Phase 2: This is where you want to go today.
Phase 3: You're not going anywhere today.
  -- seen on /.
-
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