Date: Thu, 16 Aug 2018 08:28:39 -0600 From: Warner Losh <imp@bsdimp.com> To: Toomas Soome <tsoome@me.com> Cc: John Baldwin <jhb@freebsd.org>, Ian Lepore <ian@freebsd.org>, Toomas Soome <tsoome@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r337878 - head/stand/i386/libi386 Message-ID: <CANCZdfoizLmOj_rPQi5M5_SS=JMnjnMpD2evx1oZeEjfukTOvQ@mail.gmail.com> In-Reply-To: <4A015D53-1336-4E7F-B53E-28ACEBA3318A@me.com> References: <201808152225.w7FMP5J2018006@repo.freebsd.org> <1534372134.1466.21.camel@freebsd.org> <CANCZdfpR_v5_ES=XhpKK6GD6Hd7zneObwKQwf40P0aDLxWh=WA@mail.gmail.com> <05b36083-9f4d-658e-ab39-e8c317d01db7@FreeBSD.org> <18E7897D-9985-44CE-B648-2A839417B1BC@me.com> <CANCZdfpr%2B5rmEwkzZckOZG5sydMLmZ5AdGoQFuSoPFkVNUHK7w@mail.gmail.com> <4A015D53-1336-4E7F-B53E-28ACEBA3318A@me.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Aug 16, 2018 at 8:00 AM, Toomas Soome <tsoome@me.com> wrote: > > > On 16 Aug 2018, at 16:03, Warner Losh <imp@bsdimp.com> wrote: > > > > On Thu, Aug 16, 2018 at 1:10 AM, Toomas Soome <tsoome@me.com> wrote: > >> >> >> > On 16 Aug 2018, at 09:59, John Baldwin <jhb@FreeBSD.org> wrote: >> > >> > On 8/15/18 11:59 PM, Warner Losh wrote: >> >> >> >> >> >> On Wed, Aug 15, 2018 at 4:28 PM, Ian Lepore <ian@freebsd.org <mailto: >> ian@freebsd.org>> wrote: >> >> >> >> On Wed, 2018-08-15 at 22:25 +0000, Toomas Soome wrote: >> >>> Author: tsoome >> >>> Date: Wed Aug 15 22:25:05 2018 >> >>> New Revision: 337878 >> >>> URL: https://svnweb.freebsd.org/changeset/base/337878 <htt >> ps://svnweb.freebsd.org/changeset/base/337878> >> >>> >> >>> Log: >> >>> libi386: remove bd_read() and bd_write() wrappers >> >>> >> >>> Those wroappers are nice, but do not really add much value. >> >>> >> >>> Modified: >> >>> head/stand/i386/libi386/biosdisk.c >> >>> >> >>> Modified: head/stand/i386/libi386/biosdisk.c >> >>> ===================================================================== >> >>> ========= >> >>> --- head/stand/i386/libi386/biosdisk.c Wed Aug 15 21:47:03 >> >>> 2018 (r337877) >> >>> +++ head/stand/i386/libi386/biosdisk.c Wed Aug 15 22:25:05 >> >>> 2018 (r337878) >> >>> @@ -94,10 +94,7 @@ static int nbdinfo = 0; >> >>> >> >>> static void bd_io_workaround(struct disk_devdesc *dev); >> >>> >> >>> -static int bd_read(struct disk_devdesc *dev, daddr_t dblk, int blks, >> >>> - caddr_t dest); >> >>> -static int bd_write(struct disk_devdesc *dev, daddr_t dblk, int >> >>> blks, >> >>> - caddr_t dest); >> >>> +static int bd_io(struct disk_devdesc *, daddr_t, int, caddr_t, int); >> >>> static int bd_int13probe(struct bdinfo *bd); >> >>> >> >>> static int bd_init(void); >> >>> @@ -506,7 +503,7 @@ bd_realstrategy(void *devdata, int rw, daddr_t >> >>> dblk, s >> >>> case F_READ: >> >>> DEBUG("read %d from %lld to %p", blks, dblk, buf); >> >>> >> >>> - if (blks && (rc = bd_read(dev, dblk, blks, buf))) { >> >>> + if (blks && (rc = bd_io(dev, dblk, blks, buf, 0))) { >> >>> /* Filter out floppy controller errors */ >> >>> if (BD(dev).bd_flags != BD_FLOPPY || rc != >> >>> 0x20) { >> >>> printf("read %d from %lld to %p, >> >>> error: 0x%x\n", >> >>> @@ -518,7 +515,7 @@ bd_realstrategy(void *devdata, int rw, daddr_t >> >>> dblk, s >> >>> case F_WRITE : >> >>> DEBUG("write %d from %lld to %p", blks, dblk, buf); >> >>> >> >>> - if (blks && bd_write(dev, dblk, blks, buf)) { >> >>> + if (blks && bd_io(dev, dblk, blks, buf, 1)) { >> >>> DEBUG("write error"); >> >>> return (EIO); >> >>> } >> >>> @@ -713,20 +710,6 @@ bd_io(struct disk_devdesc *dev, daddr_t dblk, >> >>> int blks >> >>> } >> >>> >> >>> return (0); >> >>> -} >> >>> - >> >>> -static int >> >>> -bd_read(struct disk_devdesc *dev, daddr_t dblk, int blks, caddr_t >> >>> dest) >> >>> -{ >> >>> - >> >>> - return (bd_io(dev, dblk, blks, dest, 0)); >> >>> -} >> >>> - >> >>> -static int >> >>> -bd_write(struct disk_devdesc *dev, daddr_t dblk, int blks, caddr_t >> >>> dest) >> >>> -{ >> >>> - >> >>> - return (bd_io(dev, dblk, blks, dest, 1)); >> >>> } >> >>> >> >>> /* >> >>> >> >> >> >> This would be a more satisfying change if there were something like >> >> >> >> #define BD_RD 0 >> >> #define BD_WR 1 >> >> >> >> so that it was clear at a glance what a bd_io() call is doing. >> >> >> >> >> >> I think that's a good idea... >> > >> > Arguably the bd_read/write wrappers were even clearer (and there purpose >> > was readability in that case). >> >> Yes thats true, but also will leave us in mercy of inlining etc.. anyhow, >> *my* purpose is to get the line of changes done (to be able to perform IO >> with >512 sector size, merge with bioscd.c and split up floppy, cd and hdd >> cases, so the user can distinguish the devices. >> > > OK, Now I'm confused. All this sounds like regression, apart from the >512 > sector thing. > > Warner > > > Oh sorry, it was bad wording I think - the split up in sense that > currently the BIOS loader can not distinguish floppy from disk, so the user > has to guess if diskX is floppy or disk - so I want to make the device > list for user very clear (fdX:, cdX:, diskX: as its already done in EFI > version). The code should be consolidated (biosdisk + bioscd). > OK. I have concerns, but be sure to put me on the code review so we can talk about it there when there's a concrete example to talk about. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoizLmOj_rPQi5M5_SS=JMnjnMpD2evx1oZeEjfukTOvQ>