From owner-freebsd-scsi@FreeBSD.ORG Tue Oct 26 14:40:59 2010 Return-Path: Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 547AA106564A for ; Tue, 26 Oct 2010 14:40:59 +0000 (UTC) (envelope-from mjohnston@sandvine.com) Received: from mail1.sandvine.com (Mail1.sandvine.com [64.7.137.134]) by mx1.freebsd.org (Postfix) with ESMTP id E8A3B8FC0A for ; Tue, 26 Oct 2010 14:40:58 +0000 (UTC) Received: from WTL-EXCH-1.sandvine.com ([fe80::f523:8e57:71d7:5206]) by wtl-exch-2.sandvine.com ([fe80::8959:ede3:2dbe:c1b%15]) with mapi; Tue, 26 Oct 2010 10:40:58 -0400 From: Mark Johnston To: Scott Long Thread-Topic: [Patch] camcontrol - determine the defect list length Thread-Index: Act0XP7EtkzAA2RrQmqTlsb3ojUyrQAkkTSAAAnNEZA= Date: Tue, 26 Oct 2010 14:40:57 +0000 Message-ID: <649630C24D5E884F85DD13645FE903A7640B50C2@wtl-exch-1.sandvine.com> References: <649630C24D5E884F85DD13645FE903A7640B244F@wtl-exch-2.sandvine.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "freebsd-scsi@freebsd.org" Subject: RE: [Patch] camcontrol - determine the defect list length X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Oct 2010 14:40:59 -0000 >> Hi, >>=20 >> Below is a patch for camcontrol from the tree at my work. It was introdu= ced several years ago to resolve some issues that we were seeing in Adaptec= 's RAID controller firmware. The change is to the readdefects() function - = when sending the request to read the defect list, instead of using a 65000-= entry buffer, we wrote a new function to query the drive and determine the = size of the defect list before allocating any memory. Apparently, if we sen= d a maximum length longer than the number of bytes in the defect list, the = cam call returns an overflow error due to what is probably a firmware bug. >>=20 >> It's probable that this issue has been fixed in Adaptec's firmware for a= while now, and that this change is no longer necessary to us. However, it'= s conceivable that other devices could have the same problem, and as far as= I can see, this patch shouldn't introduce any regressions - it essentially= consists of sending an additional CAM request before getting the defect li= st. >>=20 >> I'd like to know whether this patch is worth trying to get committed int= o FreeBSD. If anyone has some comments or suggestions on the patch, I'd hig= hly appreciate it. >>=20 > Should the whole operation of reading the defect list abort if readdefect= len() fails, or should it just fall back to a modest length and try the ope= ration anyways? > Also, if readdefectlen() succeeds, how do the defect_lis= t and ccb get freed? > Scott Ah, thanks for pointing that out. The patch below fixes the missing frees. I would argue that there's not much point in retrying the operation if read= defectlen() fails since readdefectlen() performs essentially the same operations as readdefects() -= readdefectlen() will fail if: 1. malloc() returns NULL somewhere, 2. the defect list format isn't specified 3. cam_send_ccb() fails 4. the request defect list format isn't supported 5. there was an error when reading the defect list data Of these, I don't see any reason that readdefects() would succeed if readde= fectlen() fails. cam_send_ccb() performs a cam ioctl which in turn fails if functions like xpt_find_bus() a= nd xpt_find_target() do. It doesn't seem likely to me that a second call would succeed if the first failed. Thanks, -Mark --- camcontrol.c@@/main/RELENG_8/RELENG_8_1/svos_8_1/1 2010-10-21 15:39:04= .000000000 -0400 +++ camcontrol.c 2010-10-26 10:12:12.000000000 -0400 @@ -2168,7 +2168,7 @@ static int readdefectlen(struct cam_devi } =20 *ret_len =3D returned_length; - return 0; + defect_len_bailout: =20 if (defect_list !=3D NULL)