svn commit: r337878 - head/stand/i386/libi386

[Toomas Soome]

> On 16 Aug 2018, at 16:03, Warner Losh <imp at bsdimp.com> wrote:
> 
> 
> 
> On Thu, Aug 16, 2018 at 1:10 AM, Toomas Soome <tsoome at me.com <mailto:tsoome at me.com>> wrote:
> 
> 
> > On 16 Aug 2018, at 09:59, John Baldwin <jhb at 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 at freebsd.org <mailto:ian at freebsd.org> <mailto:ian at freebsd.org <mailto:ian at 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 <https://svnweb.freebsd.org/changeset/base/337878> <https://svnweb.freebsd.org/changeset/base/337878 <https://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).

rgds,
toomas