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

[Warner Losh]

On Thu, Aug 16, 2018 at 8:00 AM, Toomas Soome <tsoome at me.com> wrote:

>
>
> 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> 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>> 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