Re: [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function

[i.kononenko <i.kononenko@yadro.com>]

Good morning, Alan!

First of all, thank you for your time to review my first patchset for 
the Linux Kernel and valuable advice on the right way of patchwriting!

On 27.06.2021 17:18, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
>> SCSI command handlers currently have an ambiguous return value. This
> 
> (I dislike very much this way of writing patch descriptions.  Unless
> the reader has already looked at the email subject line and remembers
> that this patch affects the mass-storage gadget, he will think the
> sentence above is talking about command handlers in the SCSI core -- a
> completely different part of the kernel.  When writing patch
> descriptions, please do not assume that the reader already knows what
> the patch is about.)
> 
>> return value may indicate the length of the data written to the response
>> buffer and the command's processing status. Thus, the understanding of
>> command handling may be implicit.

First of all, thank you for your time to review my first patchset for the
Linux Kernel and valuable advice on the right way of patchwriting!

I noticed that the status/datasize return value pattern is pervasive for 
Linux and used through many subsystems. But for the f_mass_storage.c,
such approach use case is not documented anywhere, and implementation has 
too many magic-constant, e.g.
```
static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
{
   ....
   return 36;
}
```
IMHO, this way is not giving the developer an explicit understanding of 
'what is the 36' and its origin.
If moving to the suggested way is unwanted, I'd keep the implementation 
as is with additional documentation for each function where uses this 
approach.
Additionally, I guess, define clarify macros of return value instead of 
magic numbers is required.

> 
> The return value is _not_ ambiguous.  If the value is >= 0 then it is
> a data length, otherwise it is a status.  Yes, this is implicit, but it
> is a very common pattern used throughout the kernel and everyone
> understands it.
> 
>> After this patch, the output buffer's size will be set in the
>> 'data_size_to_handle' field of 'struct fsg_common', and the command
>> handler's return value indicates only the processing status.
> 
> What is the reason for making this change?  Does it fix any problems
> or prepare the way for any future patches?  It seems like this is
> completely unnecessary.

Yes, the patch uses as part of the incoming implementation of refactoring
'usb:gadget:mass-storage:scsi' command handling.
I believed the suggested improvement would be useful for the community as 
an improvement of code.

> 
> Alan Stern
> 
>> 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@yadro.com>


-- 
Best regards,

Igor Kononenko