On Mon, 11 Oct 2021 12:54:20 -0700
Bart Van Assche <bvanassche@xxxxxxx> 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.
