[SeaBIOS] [PATCH 0/9] add support for generic lun enumeration

[Roman Kagan]

On Mon, Mar 27, 2017 at 12:54:03PM -0400, Kevin O'Connor wrote:
> On Mon, Mar 27, 2017 at 06:31:29PM +0300, Roman Kagan wrote:
> > On Tue, Mar 14, 2017 at 06:16:04PM +0300, Roman Kagan wrote:
> > > On Mon, Mar 13, 2017 at 12:11:49PM -0400, Kevin O'Connor wrote:
> > > > On Thu, Mar 02, 2017 at 07:37:38PM +0300, Roman Kagan wrote:
> > > > > On Thu, Mar 02, 2017 at 11:14:37AM -0500, Kevin O'Connor wrote:
> > > > I have a few high-level comments on the series.  I wonder if there is
> > > > a way to reduce the amount of control passing between the generic scsi
> > > > code and the driver code.  Specifically, I wonder if the callback
> > > > function scsi_add_lun could be simplified.  Some thoughts:
> > > > 
> > > > - instead of scsi_rep_luns_scan() being passed a callback, perhaps
> > > >   introduce a scsi_get_luns() command that returns a malloc'd struct
> > > >   containing the list of luns.  The driver code could then iterate
> > > >   over the list.
> > > 
> > > I considered this at first, but it looked like more boilerplate code in
> > > the drivers so I decided to go with the callback.
> 
> I was thinking the driver could have something like:
> 
>     struct scsi_luns *luns = scsi_get_luns(&vlun0.drive);
>     for (i = 0; luns && i < luns->count; i++)
>         xxx_add_lun(&vlun0, luns->luns[i]);
>     free(luns);
> 
> One could layout 'struct scsi_luns' to use the same memory as
> cdbres_report_luns:
> 
>     struct scsi_luns {
>         u32 count, pad;
>         u64 luns[0];
>     }
> 
> And scsi_get_luns() could fixup the big-endian conversion in-place (or
> the driver loop above could just directly call
> be32_to_cpu/be64_to_cpu).  If REPORT LUNS fails then scsi_get_luns()
> could just fill in 'struct scsi_luns' with count=1, lun[0]=0.

I'm not convinced that exposing all this boilerplate is simpler than
hiding the details under the hood of a single function.  Or is perhaps
anything wrong with passing function pointers in general?
Anyway I can do it the way you suggest if this is what you want.


>   - malloc_high shouldn't be used on temporary reservations (as it
>     can fragment the permanent memory pool) - use malloc_tmp instead.

OK

>   - scsilun2u64() looks like be64_to_cpu - or did I miss something?

No it would've been too simple :)  It's a funny mix: the luns are
encoded as four be16-s LE-ordered wrt each other.

>   - I'd prefer to avoid backwards goto's - a "for (;;)" loop would be
>     preferabale

OK

I'll rework and resubmit the patchset once I have your final word on the
interface.

Thanks!
Roman.