[PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling

[i.kononenko]

On 27.06.2021 17:23, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote:
>> Implements a universal way to define SCSI commands and configure
>> precheck handlers.
> 
> What is the reason for doing this?

I have started implementing a way to specify a backend-file of 
mass-storage images greater than 2.1Gb for cdrom-like mediums. 
I notice the implementation of each scsi-command handler uses too 
many magic-constant, hardcoded indexes and shifts. I decided to 
define structures that contained appropriate SCSI-defined fields 
and constant-values to clarify the code.

Additionally, I noticed, many kernel subsystems use the 'separate
data and logic' approach, making a code more explicit and readable.
This looks reasonable to me, and a code looks more clearly, at 
least - we don't need to examine each magic constant and its purpose. 

> 
> At first glance, it appears you have added a great deal of complexity
> to the driver.  The patch replaces a large amount of easily understood
> (albeit rather repetitious) code with an approximately equal amount
> of rather complicated code.  This does not seem like an improvement.

The SCSI-commands table is defined as unifying a way to specify the 
SCSI-command handler, with corresponding required data instead pass 
it to each repeatedly switch-case block, which makes code more readable
to me. If there isn't, I can keep the definition of SCSI-handlers as is,
but the SCSI-data structures with their constant-values are still 
required, in my opinion.

> 
> Furthermore, the code you removed is flexible; it easily allows for
> small variations as neede by some command handlers.  But the code you
> added is all table-driven, which does not easily permit arbitrary
> variations.
> 

I don't think that the SCSI-command handlers table is an obstacle to 
define variation into a specific handler because the current patch has 
helper macros, which can specify a behavior for each requirement of 
handler.

Anyway, the definition of the scsi-command handlers table may be discarded,
because this work done to helping developers who will work the 
'usb:gadget:mass-storage' subsystem in the future.

>> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
>> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
>> USBGadget MassStorage debug print showed all sent commands works
>> properly.
>>
>> Signed-off-by: Igor Kononenko <i.kononenko at yadro.com>
>> ---
>>  drivers/usb/gadget/function/f_mass_storage.c | 540 +++++++++++--------
>>  drivers/usb/gadget/function/storage_common.h |   5 +
>>  2 files changed, 310 insertions(+), 235 deletions(-)
> 
> I don't see the point of adding 75 lines to the kernel source if they
> don't accomplish anything new.
> 
> Alan Stern
> 

-- 
Best regards,

Igor Kononenko