On Mon, May 21, 2001 at 08:39:20AM -0700, Matthew Jacob wrote:
> On Mon, 21 May 2001, Manuel Bouyer wrote:
> > > I think you're missing the point. The only mechanism with which
> > > scsipi_run_queue can be called now after a thaw is via the indirect softcall
> > > mechanism in scsipi_channel_thaw (which is exposed). I don't care whether
> > > scsipi_run_queue itself is private. I *do* care that you can't restart except
> > > via a softcall. That's just plain wrong when you've already got a per-HBA
> > > thread.
> > 
> > This is why I propose to make scsipi_channel_thaw public, this once can be
> > called from your thread. But I don't think we should allow HBA drivers to
> > bypass the freeze/thaw mechanism; this will break if we want to change the
> > internals of scsipi.
> 
> Oops- sorry! It's scspi_channel_timed_thaw that does the softcall...
> 
> But scsipi_channel_thaw (which is public- I'm assuming that if it's in
> scsipiconf.h it's public) doesn't call scsipi_run_queue- all it does is
> decrement the qfreeze count.
> 
> The freeze/thaw mechanism may not be entirely complete yet anyway-
> 
> This is all coming up because I'm finally implementing a kthread to manage
> loop events. What I'm forced to do now because I do not know how many channel
> 'freeze' events I get but I do know when they should all thaw, is to do the
> following:
> 
>         case ISPASYNC_LIP:
>                 if (isp->isp_osinfo.blocked == 0) {
>                         isp->isp_osinfo.blocked = 1;
>                         scsipi_channel_freeze(&isp->isp_chanA, 1);
>                 }
>                 isp_prt(isp, ISP_LOGINFO, "LIP Received");
>                 break; 
>         case ISPASYNC_LOOP_RESET:
>                 if (isp->isp_osinfo.blocked == 0) {
>                         isp->isp_osinfo.blocked = 1;
>                         scsipi_channel_freeze(&isp->isp_chanA, 1);
>                 }
>                 isp_prt(isp, ISP_LOGINFO, "Loop Reset Received");
>                 break;
>         case ISPASYNC_LOOP_DOWN:
>                 if (isp->isp_osinfo.blocked == 0) {
>                         isp->isp_osinfo.blocked = 1;
>                         scsipi_channel_freeze(&isp->isp_chanA, 1);
>                 }
>                 isp_prt(isp, ISP_LOGINFO, "Loop DOWN");
>                 break;  
> ...
>         case ISPASYNC_CHANGE_NOTIFY:
>                 if (isp->isp_osinfo.blocked == 0) {
>                         isp->isp_osinfo.blocked = 1;
>                         scsipi_channel_freeze(&isp->isp_chanA, 1);
>                 }
>                 if (arg == ISPASYNC_CHANGE_PDB) {
>                         isp_prt(isp, ISP_LOGINFO, "Port Database Changed");  
>                 } else if (arg == ISPASYNC_CHANGE_SNS) {
>                         isp_prt(isp, ISP_LOGINFO,
>                             "Name Server Database Changed");
>                 }       
>                 /*      
>                  * Note that we have work for the thread to do, and
>                  * if the thread is here already, wake it up.
>                  */
>                 isp->isp_osinfo.threadwork++;
>                 if (isp->isp_osinfo.thread) {
>                         wakeup(&isp->isp_osinfo.thread);
>                 }       
>                 break;
> 
> and then the last event is the one which allows me to fire up the loop cleanup
> thread, which when it gets done rebuilding loop state and (re)finding all
> devices, wants to thaw everything and call scsipi_run_queue directly so that
> things run now, darn it, not off of a softcall later:
> 
>                 if (isp->isp_osinfo.blocked) {
>                         isp->isp_osinfo.blocked = 0;
>                         isp_prt(isp, ISP_LOGINFO, "restarting queues");
>                         isp->isp_chanA.chan_qfreeze = 0;
>                         scsipi_run_queue(&isp->isp_chanA);
>                 }
> 
> The two latter line there are both layering violations. What I'd *really* like
> to see is:
> 
> 	scsipi_channel_thaw(&isp->isp_chanA, -1);
> 
> to be taken as:
> 
> 	zero freeze count (-1 count argument)

I don't think this is good. The channel may have been frozen by something
else inside scsipi too; we don't want to drop it.
You should keep track on how many times you call scsipi_channel_freeze()
in the driver, and use the count for scsipi_channel_thaw().

I'll look at calling scsipi_run_queue() from scsipi_channel_thaw() when the
count drops to 0.

--
Manuel Bouyer <bouyer@antioche.eu.org>
--