[SeaBIOS] Re: [RFC v2 1/1] Add LBA 64bit support for reads beyond 2TB.

[Paul Menzel]

Dear Max,

Thank you for your instant reply.

Am 25.01.24 um 15:36 schrieb Max Tottenham:
> On 01/25, Paul Menzel wrote:
> > Am 25.01.24 um 12:35 schrieb Max Tottenham via SeaBIOS:

[…]
> > > src/hw/blockcmd.c | 61 +++++++++++++++++++++++++++++++++++------------
> > >    src/hw/blockcmd.h | 27 +++++++++++++++++++++
> > >    2 files changed, 73 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c
> > > index 6b6fea97..9b6a32bf 100644
> > > --- a/src/hw/blockcmd.c
> > > +++ b/src/hw/blockcmd.c
> > > @@ -66,6 +66,23 @@ cdb_test_unit_ready(struct disk_op_s *op)
> > >        return process_op(op);
> > >    }
> > >
> > > +static int
> > > +cdb_read_capacity16(struct disk_op_s *op, struct cdbres_read_capacity_16 *data)
> > > +{
> > > struct cdb_sai_read_capacity_16 cmd;
> > > memset(&cmd, 0, sizeof(cmd));
> > > cmd.command = CDB_CMD_SERVICE_ACTION_IN;
> > > cmd.flags = CDB_CMD_SAI_READ_CAPACITY_16;
> > > cmd.len = cpu_to_be32(sizeof(struct cdbres_read_capacity_16));
> > > op->command = CMD_SCSI;
> > > op->count = 1;
> > > op->buf_fl = data;
> > > op->cdbcmd = &cmd;
> > > op->blocksize = sizeof(*data);
> > > return process_op(op);
> > >
> > > +}
> > > // Request capacity
> > >  static int
> > >  cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data)
> > >
> > > @@ -111,13 +128,23 @@ scsi_fill_cmd(struct disk_op_s *op, void *cdbcmd, int maxcdb)
> > >        switch (op->command) {
> > >        case CMD_READ:
> > >        case CMD_WRITE: ;
> > >    struct cdb_rwdata_10 *cmd = cdbcmd;
> > >    memset(cmd, 0, maxcdb);
> > >    cmd->command = (op->command == CMD_READ ? CDB_CMD_READ_10
> > >                    : CDB_CMD_WRITE_10);
> > >    cmd->lba = cpu_to_be32(op->lba);
> > >    cmd->count = cpu_to_be16(op->count);
> > >    return GET_FLATPTR(op->drive_fl->blksize);
> > >    if (op->lba < 0xFFFFFFFFULL) {
> > >        struct cdb_rwdata_10 *cmd = cdbcmd;
> > >        memset(cmd, 0, maxcdb);
> > >        cmd->command = (op->command == CMD_READ ? CDB_CMD_READ_10
> > >                        : CDB_CMD_WRITE_10);
> > >        cmd->lba = cpu_to_be32(op->lba);
> > >        cmd->count = cpu_to_be16(op->count);
> > >        return GET_FLATPTR(op->drive_fl->blksize);
> > >    } else {
> > >        struct cdb_rwdata_16 *cmd = cdbcmd;
> > >        memset(cmd, 0, maxcdb);
> > >        cmd->command = (op->command == CMD_READ ? CDB_CMD_READ_16
> > >                        : CDB_CMD_WRITE_16);
> > >        cmd->lba = cpu_to_be64(op->lba);
> > >        cmd->count = cpu_to_be32(op->count);
> > >        return GET_FLATPTR(op->drive_fl->blksize);
> > >    }
> >
> > It’s bikeshedding, but maybe the code in the branches could be factored
> > into separate functions, and then the return also wouldn’t need to be
> > duplicated.
>
> I missed this comment earlier - Perhaps rather than functions we could
> just extract the return statement out of each of the branches e.g.
> do the following?:
>  if (op->lba < 0xFFFFFFFFULL) {
>      struct cdb_rwdata_10 *cmd = cdbcmd;
>      memset(cmd, 0, maxcdb);
>      cmd->command = (op->command == CMD_READ ? CDB_CMD_READ_10
>                      : CDB_CMD_WRITE_10);
>      cmd->lba = cpu_to_be32(op->lba);
>      cmd->count = cpu_to_be16(op->count);
>  } else {
>      struct cdb_rwdata_16 *cmd = cdbcmd;
>      memset(cmd, 0, maxcdb);
>      cmd->command = (op->command == CMD_READ ? CDB_CMD_READ_16
>                      : CDB_CMD_WRITE_16);
>      cmd->lba = cpu_to_be64(op->lba);
>      cmd->count = cpu_to_be32(op->count);
>  }
>  return GET_FLATPTR(op->drive_fl->blksize);
>
> Seems odd to me to make two separate functions for such a small 'fill
> this struct with data' ?

Yes, agreed.

[…]

Kind regards,

Paul