Re: [PATCH V2] scsi:spraid: initial commit of Ramaxel spraid driver

[Yanling song <yanling.song@linux.dev>]

On 11/30/21 11:55 AM, Hannes Reinecke wrote:
> On 11/30/21 12:38 PM, Yanling Song wrote:
>> On Mon, 29 Nov 2021 14:04:12 +0100
>> Hannes Reinecke <hare@suse.de> wrote:
>>
>>> On 11/26/21 8:33 AM, Yanling Song wrote:
>>>> This initial commit contains Ramaxel's spraid module.
>>>>
>>>> Signed-off-by: Yanling Song <songyl@ramaxel.com>
>>>>
>>>> Changes from V1:
>>>> 1. Use BSG module to replace with ioctrl
>>>> 2. Use author's email as MODULE_AUTHOR
>>>> 3. Remove "default=m" in drivers/scsi/spraid/Kconfig
>>>> 4. To be changed in the next version:
>>>>     a. Use get_unaligned_be*() in spraid_setup_rw_cmd();
>>>>     b. Use pr_debug() instead of introducing dev_log_dbg().
>>>>
>>>> ---
>>>>   Documentation/scsi/spraid.rst     |   16 +
>>>>   MAINTAINERS                       |    7 +
>>>>   drivers/scsi/Kconfig              |    1 +
>>>>   drivers/scsi/Makefile             |    1 +
>>>>   drivers/scsi/spraid/Kconfig       |   10 +
>>>>   drivers/scsi/spraid/Makefile      |    7 +
>>>>   drivers/scsi/spraid/spraid.h      |  693 ++++++
>>>>   drivers/scsi/spraid/spraid_main.c | 3328
>>>> +++++++++++++++++++++++++++++ 8 files changed, 4063 insertions(+)
>>>>   create mode 100644 Documentation/scsi/spraid.rst
>>>>   create mode 100644 drivers/scsi/spraid/Kconfig
>>>>   create mode 100644 drivers/scsi/spraid/Makefile
>>>>   create mode 100644 drivers/scsi/spraid/spraid.h
>>>>   create mode 100644 drivers/scsi/spraid/spraid_main.c
>>>>   
>>> Hmm.
>>> This entire thing looks like an NVMe controller which is made to look 
>>> like a SCSI controller.
>>> It even uses most of the NVMe structures.
>>> And from what I've seen there is not much SCSI specific in here; I/O
>>> and queue setup is pretty much what every NVMe controller does.
>>> So why not make it a true NVMe controller?
>>> Yes, you would need to discuss with the NVMe folks on how a RAID 
>>> controller should look like in NVMe terms.
>>> But overall I guess the driver would be far smaller and possibly
>>> easier to maintain.
>>>
>>> So where's the benefit having it as a SCSI driver (apart from the
>>> fact that is allows you to side-step having to discuss the interface
>>> with NVMexpress.org ...)?
>>> Or, to put it the other way round: Is there anything SCSI specific
>>> which would prevent such an approach?
>>>
>>
>> Actually it is a SCSI driver, and it does register a scsi_host_template
>> and host does send SCSI commands to our raid controller just like other
>> raid controllers. You are right "it looks a lot like NVMe" since we
>> believe the communication mechanism of NVME between host and the end
>> device is good and it was leveraged when we designed the raid
>> controller. That's why it looks like there are some code from NVME
>> because the mechanism is the same.
>>
> Thank you, but that was precisely my question.
> 
> Seeing that the driver is using the NVMe mechanism to communicate
> commands between the driver and the hardware, doesn't it make it a NVMe
> driver?
> Especially as you are sending NVMe commands and not SCSI commands, so
> you always will have to re-write the incoming SCSI commands into NVMe
> commands, and knowing from experience this is not a good fit.
> 
> Hence my question: what exactly is SCSI specific on the hardware side?
> Wouldn't an implementation as a NVMe driver be a better fit, as then you
> could leverage all the existing code like setup prps, completion
> handling etc?
> 
Our controller supports both SCSI and NVMe. In current raid mode, it only supports SAS/SATA disks.
and reports SCSI disk to host after creating raid group. In this use case, SCSI is the protocol and the spraid driver is needed.
In another mode, controller can support NVMe disks and works with the NVMe driver in linux kernel.
To reduce the complexity, NVMe mechanism is used as the communication method between driver and controller so that we can use the same interface for both SCSI and NVMe.

> Cheers,
> 
> Hannes
>