svn commit: r296589 - head/sys/dev/fdc

[Warner Losh]

On Thu, Mar 10, 2016 at 6:58 PM, Warner Losh <imp at bsdimp.com> wrote:

>
> On Mar 10, 2016 3:37 PM, "Bryan Drewery" <bdrewery at 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