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. _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://www.coreboot.org/mailman/listinfo/seabios