Re: Why shoud we cause panic in scsi_da.c?

On Mon, Jul 13, 2015 at 18:29:36 +0300, Alexander Motin wrote:
> Hi.
> 
> On 13.07.2015 11:51, Kohji Okuno wrote:
> >> On 07/13/15 10:11, Kohji Okuno wrote:
> >>> Could you comment on my quesion?
> >>>
> >>>> I found panic() in scsi_da.c. Please find the following.
> >>>> I think we should return with error without panic().
> >>>> What do you think about this?
> >>>>
> >>>> scsi_da.c:
> >>>> 3018	                } else if (bp != NULL) {
> >>>> 3019 if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0)
> >>>> 3020	                                panic("REQ_CMP with QFRZN");
> >>>>
> >>
> >> It looks to me more like an KASSERT() is appropriate here.
> 
> As I can see, this panic() call was added by ken_at_ about 15 years ago.
> I've added him to CC in case he has some idea why it was done. From my
> personal opinion I don't see much reasons to allow CAM_DEV_QFRZN to be
> returned only together with error. While is may have little sense in
> case of successful command completion, I don't think it should be
> treated as error. Simply removing this panic is probably a bad idea,
> since if it happens device will just remain frozen forever, that will be
> will be difficult to diagnose, but I would better just dropped device
> freeze in that case same as in case of completion with error.

I put it there because it indicates a software error.  The queue shouldn't
be frozen if the command is successful.  The reason for freezing the queue
is to allow error recovery to happen.  The queue will get unfrozen after
error recovery completes.

We could alternately just print a diagnostic message, unfreeze the queue
and move on, but the idea is to allow the driver writer to detect and
correct his error immediately.

As for the original poster's problem, he has uncovered a bug that needs to
be fixed.  (And I don't mean in the da(4) driver.  The bug is in the
component that left the queue frozen.  Most likely in the USB driver, but
it will take a little more investigation.)  The panic worked as intended. :)

Ken
-- 
Kenneth Merry
ken_at_FreeBSD.ORG