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

From: Bart Van Assche <bvanassche@acm.org>
To: Yanling Song <songyl@ramaxel.com>
Cc: martin.petersen@oracle.com, linux-scsi@vger.kernel.org
Subject: Re: [PATCH] spraid: initial commit of Ramaxel spraid driver
Date: Wed, 13 Oct 2021 15:00:07 -0700	[thread overview]
Message-ID: <9d9d2f95-7782-85a7-b79a-ce481292c451@acm.org> (raw)
In-Reply-To: <20211013065012.02b76336@songyl>

On 10/12/21 11:50 PM, Yanling Song wrote:
> On Tue, 12 Oct 2021 09:59:30 -0700
> Bart Van Assche <bvanassche@acm.org> wrote:
>> Why is it that SG_IO is not sufficient? This is something that should
>> have been explained in the patch description.
> 
> There are two cases that there are no SG devices and SG_IO cannot work.
> 1. To access raid controller:
> a. Raid controller is a scsi host, not a scsi device, so there
> is no SG device associated with it.
> b. Even there is a scsi device for raid controller, SG_IO
> cannot work when something wrong with IO queue and only admin queue can
> work;
> 2. To access the physical disks behinds raid controller:
> raid controller only reports VDs to OS and only VDs have SG devices. OS
> has no idea about physical disks behinds raid controller and there is no
> SG devices associated with physical disks.

Please take a look at the bsg_setup_queue() call in ufs_bsg_probe(). 
That call associates a BSG queue with the UFS host. That queue supports 
requests of type struct ufs_bsg_request. The Fibre Channel transport 
driver does something similar. I believe that this is a better solution 
than introducing entirely new ioctls.

>>>> Additionally, mixing driver-internal and user space definitions in
>>>> a single header file is not OK. Definitions of data structures and
>>>> ioctls that are needed by user space software should occur in a
>>>> header file in the directory include/uapi/scsi/.
>>>
>>> Sounds reasonable. But after checking the directory
>>> include/uapi/scsi/, there are only several files in it. It is
>>> expected that there should be many files if developers follow the
>>> rule. Do you know why?
>>
>> If this rule is not followed, that will be a red flag for the SCSI
>> maintainer and something that will probably delay upstream acceptance
>> of this patch.
> 
> Since there are not much examples in include/uapi/scsi/, what' your
> suggestion on how to put the definitions into the folder? for example,
> what's the file name? spraid_ioctrl.h?

How about include/uapi/scsi/spraid.h, since I assume that that header 
file will cover all user space interfaces for interaction with the 
spraid driver?

Thanks,

Bart.