From owner-freebsd-hackers@FreeBSD.ORG Fri Jan 28 19:35:57 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D3E1D1065704; Fri, 28 Jan 2011 19:35:57 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id 431938FC1F; Fri, 28 Jan 2011 19:35:56 +0000 (UTC) Received: by wyf19 with SMTP id 19so3646671wyf.13 for ; Fri, 28 Jan 2011 11:35:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=JSJfQWT0GX0wX8tCDEpDCclWJ6EEkJyLnCVlGPXjkMM=; b=cJfVeCW7j8Nha8YSmIYYcXlX3dC1U3YWHcs7Jaehx0X4jMEErZpl+b4IAQ2pISeJUY wvSrgLgHeadzjVyviWjOCbQq9to6dwha9foELmFj4I4eqHF/NwEZzCt4F9F6bA/dMLdb RFrjdFPfK0wuxNCf4f8P27e0MAlJr/EalTOoU= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=nSXfud4sDizru//KS3smD3xG7fcXTgWg8/Af8ZSC5fAUrTz1oygea9MPBz17nKalSQ d2lAoS5HRrUZyvPU9XBvi41tzxULHFo+TBGo9mJTW10+I5zPRy8Ou2jSgGEx8UfIlfxh Imn2PpbsmOqp1nXne4JAIEoFS9ltFB9MqoU+Q= MIME-Version: 1.0 Received: by 10.216.153.147 with SMTP id f19mr8255709wek.40.1296243356047; Fri, 28 Jan 2011 11:35:56 -0800 (PST) Received: by 10.216.62.203 with HTTP; Fri, 28 Jan 2011 11:35:55 -0800 (PST) In-Reply-To: <201101281423.02701.jhb@freebsd.org> References: <201101281400.01840.jhb@freebsd.org> <201101281423.02701.jhb@freebsd.org> Date: Fri, 28 Jan 2011 11:35:55 -0800 Message-ID: From: Matthew Fleming To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-hackers@freebsd.org Subject: Re: Divide-by-zero in loader X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Jan 2011 19:35:57 -0000 On Fri, Jan 28, 2011 at 11:23 AM, John Baldwin wrote: > On Friday, January 28, 2011 2:14:45 pm Matthew Fleming wrote: >> On Fri, Jan 28, 2011 at 11:00 AM, John Baldwin wrote: >> > On Friday, January 28, 2011 12:41:08 pm Matthew Fleming wrote: >> >> I spent a few days chasing down a bug and I'm wondering if a loader >> >> change would be appropriate. >> >> >> >> So we have these new front-panel LCDs, and like everything these days >> >> it's a SoC. =A0Normally it presents to FreeBSD as a USB communication= s >> >> device (ucom), but when the SoC is sitting in its own boot loader, it >> >> presents as storage (umass). =A0If the box is rebooted in this state, >> >> the reboot gets into /boot/loader and then reboots itself. =A0(It too= k a >> >> few days just to figure out I was getting into /boot/loader, since th= e >> >> only prompt I could definitively stop at was boot2). >> >> >> >> Anyways, I eventually debugged it to the device somehow presenting >> >> itself to /boot/loader with a geometry of 1024/256/0, and since od_se= c >> >> is 0 that causes a divide-by-zero error in bd_io() while the loader i= s >> >> trying to figure out if this is GPT or MBR formatted. =A0We're still >> >> trying to figure out why the loader sees this incorrect geometry. >> >> >> >> But meanwhile, this patch fixes the issue, and I wonder if it would b= e >> >> a useful safety-belt for other devices where an incorrect geometry ca= n >> >> be seen? >> > >> > That's probably fine. =A0A sector count of zero is invalid for CHS. = =A0However, >> > probably we should not even be using C/H/S at all if the device claims= to >> > support EDD. =A0We already use raw LBAs if it supports EDD, and we sho= uld >> > probably just ignore C/H/S altogether if it supports EDD. >> >> This is all almost entirely outside my knowledge, but at the moment >> bd_eddprobe() requres a geometry of 1023/255/63 before it attempts to >> check if EDD can be used. =A0Is that check incorrect? > > Well, it is very conservative in that it only uses EDD if it thinks it ca= n't > use C/H/S. =A0It would be interesting to see if simply checking for a sec= tor > count of 0 there would avoid the divide-by-zero and let your device "work= ". I'll need a few more pointers to try this. As structured today, bd_io is what does the divide-by-zero, and it always looks at od_sec to determine the maximum transfer size. It's only later in bd_io that it checks the EDD mode to decide how to read. /* * Play it safe and don't cross track boundaries. * (XXX this is probably unnecessary) */ sec =3D dblk % od->od_sec; /* offset into track */ x =3D min(od->od_sec - sec, resid); if (maxfer > 0) x =3D min(x, maxfer); /* fit bounce buffer */ However, I suppose this may be safe-ish to do as just: x =3D resid; if (maxfer > 0 && maxfer < resid) x =3D maxfer; /* fit bounce buffer */ In which case the only uses of od_sec is now in bd_chs_io, and some printf'= s. Though note that currently also the bd_eddprobe is only done in the bd_open_mbr() step which is only if the GPT reads showed it isn't a GPT device. I suppose that the EDD probe can be part of bd_getgeom, though. I'll try this and see what happens on my device. Thanks, matthew > However, it might actually be useful to always use EDD if possible, esp. > EDD3 since that lets you not use bounce buffers down in 1MB. > >> In my specific case I know there's no bootable stuff on this disk; the >> earlier layers bypassed it correctly without a problem. >> >> Thanks, >> matthew >> > > -- > John Baldwin >