Date: Fri, 17 Jul 2015 13:46:44 +0900 (JST) From: Kohji Okuno <okuno.kohji@jp.panasonic.com> To: ken@FreeBSD.ORG Cc: mav@FreeBSD.org, freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com Subject: Re: Why shoud we cause panic in scsi_da.c? Message-ID: <20150717.134644.1446845623556333449.okuno.kohji@jp.panasonic.com> In-Reply-To: <20150714194929.GA51157@doriath.kdm.org> References: <20150713.175143.290106286605820529.okuno.kohji@jp.panasonic.com> <55A3D960.5000704@FreeBSD.org> <20150714194929.GA51157@doriath.kdm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Subject: Re: Why shoud we cause panic in scsi_da.c?
Date: Tue, 14 Jul 2015 15:49:29 -0400
> 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@ 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. :)
I don't know the reaseon. When I accessed the specified sector on the
specified HDD, I encounter the panic. But, I can access other sectors
on the same HDD. And, I can access all sectors on the other HDD (same
model).
Even if Ken's logic is correct, I think that we should do panic in
da(4) driver.
Best regards,
Kohji Okuno
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150717.134644.1446845623556333449.okuno.kohji>