Date: Thu, 10 Mar 2016 22:14:58 -0700 From: Warner Losh <imp@bsdimp.com> To: Bryan Drewery <bdrewery@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, Warner Losh <imp@freebsd.org> Subject: Re: svn commit: r296589 - head/sys/dev/fdc Message-ID: <CANCZdfqQQwtC=30WFc_hcabnR06Vnk=esHACDCBo0-WYdVs6KA@mail.gmail.com> In-Reply-To: <CANCZdfq5zHhaR5NvasJRTz%2BVUk1kq-cMb6SxNrYeEr2-nV6wdw@mail.gmail.com> References: <201603100033.u2A0X6uN027771@repo.freebsd.org> <56E1F72D.7000002@FreeBSD.org> <CANCZdfq5zHhaR5NvasJRTz%2BVUk1kq-cMb6SxNrYeEr2-nV6wdw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 10, 2016 at 6:58 PM, Warner Losh <imp@bsdimp.com> wrote: > > On Mar 10, 2016 3:37 PM, "Bryan Drewery" <bdrewery@freebsd.org> wrote: > > > > On 3/9/16 4:33 PM, Warner Losh wrote: > > > Author: imp > > > Date: Thu Mar 10 00:33:06 2016 > > > New Revision: 296589 > > > URL: https://svnweb.freebsd.org/changeset/base/296589 > > > > > > Log: > > > Stop assuming that bio_cmd is a bit field. > > > > > > Differential Revision: https://reviews.freebsd.org/D5587 > > > > > > Modified: > > > head/sys/dev/fdc/fdc.c > > > > > > Modified: head/sys/dev/fdc/fdc.c > > > > ============================================================================== > > > --- head/sys/dev/fdc/fdc.c Thu Mar 10 00:27:10 2016 (r296588) > > > +++ head/sys/dev/fdc/fdc.c Thu Mar 10 00:33:06 2016 (r296589) > > > @@ -941,7 +941,7 @@ fdc_worker(struct fdc_data *fdc) > > > /* Disable ISADMA if we bailed while it was active */ > > > if (fd != NULL && (fd->flags & FD_ISADMA)) { > > > isa_dmadone( > > > - bp->bio_cmd & BIO_READ ? ISADMA_READ : ISADMA_WRITE, > > > + bp->bio_cmd == BIO_READ ? ISADMA_READ : ISADMA_WRITE, > > > > I think we should have some kind of file (like ports CHANGES) that lists > > subtle KPI changes. This and the bio bzero change were easily missed > > and could lead to who-knows-what downstream for vendors or even > > out-of-tree modules. > > True. However, these have never been documented one way or another.... > > And this change isn't a change yet... > > I'd love a kpi change file. This is but one of many. We'd need someone > clueful to watch the tree and remind people to add things to it. > > I'm also working on documenting our storage api so that people know better > what is defined, vs what's there and subject to change. > Re-reading this, I wasn't very clear: I think we need this file. I think we need someone else (not me) to spearhead it and police changes I think that the sooner we start the better. Can I get a volunteer? > > Btw there are some missed still: > > > > ./dev/mfi/mfi.c: switch (bio->bio_cmd & 0x03) { > > ./dev/mfi/mfi.c: switch (bio->bio_cmd & 0x03) { > > That makes me sad. Code like that has never been guaranteed. Bare > constants.... shudder. > Now that I've had a closer look at the code, it doesn't actually assume that BIO_READ and BIO_WRITE are bits. It only assumes that their values are <= 3. I'll fix that. That's also a bad assumption, but it's a different kind of bad. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqQQwtC=30WFc_hcabnR06Vnk=esHACDCBo0-WYdVs6KA>