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

From: Yanling Song <songyl@ramaxel.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: <martin.petersen@oracle.com>, <linux-scsi@vger.kernel.org>,
	<songyl@ramaxel.com>
Subject: Re: [PATCH] spraid: initial commit of Ramaxel spraid driver
Date: Tue, 12 Oct 2021 14:49:06 +0000	[thread overview]
Message-ID: <20211012144906.790579d0@songyl> (raw)
In-Reply-To: <cfe5b692-6642-e317-39a7-f38c1460097c@acm.org>

On Mon, 11 Oct 2021 12:54:20 -0700
Bart Van Assche <bvanassche@acm.org> wrote:

> On 9/29/21 20:47, Yanling Song wrote:
> > +#define SPRAID_IOCTL_RESET_CMD _IOWR('N', 0x80, struct
> > spraid_passthru_common_cmd) +#define SPRAID_IOCTL_ADMIN_CMD
> > _IOWR('N', 0x41, struct spraid_passthru_common_cmd)  
> 
> Do these new ioctls provide any functionality that is not yet
> provided by SG_IO + SG_SCSI_RESET_BUS?

These new ioctls are developed to manage our raid controller by our
private tools, which has no sg device. so SG_IO cannot work for our
case.

> 
> 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? 

> 
> > +#define SPRAID_IOCTL_IOQ_CMD _IOWR('N', 0x42, struct
> > spraid_ioq_passthru_cmd)  
> 
> What functionality does this ioctl provide that is not yet provided
> by SG_IO?

See the above.

> 
> > +#define SPRAID_DRV_VERSION	"1.0.0.0"  
> 
> Although many Linux kernel drivers include a version number, a
> version number is only useful in an out-of-tree driver and not in an
> upstream driver. The Linux kernel itself already has a version number.

In practice, There are several branch/versions of the driver for
different purposes, upstream version is one of them. a version number
can easily tell us where it comes from and what's the relationship
between different versions. I think that's the reason why many Linux
kernel drivers have a version number.

> 
> > +MODULE_AUTHOR("Ramaxel Memory Technology");  
> 
> My understanding is that the MODULE_AUTHOR argument should mention
> the name of a person. From include/linux/module.h:
> 
> /*
>   * Author(s), use "Name <email>" or just "Name", for multiple
>   * authors use multiple MODULE_AUTHOR() statements/lines.
>   */
> #define MODULE_AUTHOR(_author) MODULE_INFO(author, _author)

ok. Will use the name of developer in the next version.

> 
> Thanks,
> 
> Bart.