Re: [PATCH v2 01/13] trinity: Add base driver

[Jiho Chu]

On Sat, 17 Sep 2022 09:41:13 +0200
"Arnd Bergmann" <arnd@xxxxxxxx> wrote:

> On Sat, Sep 17, 2022, at 9:23 AM, Jiho Chu wrote:
> > It contains the base codes for trinity driver. Minimal codes to load and
> > probe device is provided. The Trinity Family is controlled by the
> > Memory-Mapped Registers, the register addresses and offsets are
> > described. And user api interfaces are presented to control device under
> > ioctl manner.
> 
> I'm not doing a full review of the driver at the moment, but
> here are some comments on the usage of chardev ioctl based on
> Documentation/driver-api/ioctl.rst
> 

Hi, Arnd
Thanks for your review.
I'll read the document more precisely.

> > +int trinity_probe(struct platform_device *pdev, const struct 
> > trinity_desc *desc)
> > +{
> > +	struct device_node *np;
> > +	struct device *dev;
> > +	struct trinity_driver *drv;
> > +	int i, err;
> > +
> > +	dev = &pdev->dev;
> > +	dev->id = ((desc->ver & TRINITY_MASK_DEV) >> TRINITY_SHIFT_DEV);
> > +
> > +	/* set private data */
> > +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->dev_id = ida_alloc(&dev_nrs, GFP_KERNEL);
> > +	if (drv->dev_id < 0) {
> > +		devm_kfree(dev, drv);
> > +		return drv->dev_id;
> > +	}
> > +	snprintf(drv->name, DEV_NAME_LEN, "%s-%u", desc->type, drv->dev_id);
> > +
> > +	platform_set_drvdata(pdev, drv);
> > +	dev_set_drvdata(dev, drv);
> > +
> 
> If you have the need to manage multiple devices here, maybe use
> a dynamic major number and have the chardev code allocate the
> minor numbers, instead of using multiple misc devices and
> doing that yourself.
> 

I'm little confusing. It means that managing own char devices is proper,
not using misc device? But, it's still under misc dir.

> > +
> > +#ifndef TASK_COMM_LEN
> > +#define TASK_COMM_LEN 16
> > +#endif
> > +
> > +#define TRINITY_APP_NAME_MAX TASK_COMM_LEN
> > +#define TRINITY_APP_STAT_MAX 10
> > +#define TRINITY_REQ_STAT_MAX 10
> 
> The structure layout should not depend on whether an application
> has included a header that defines TASK_COMM_LEN.
> 
> What is the purpose of including an application name here?
> 
> > +/**
> > + * struct trinity_ioctl_stat_app - Describes stat of the target app
> > + * @app_id: Trinity app id (currently, equal to pid)
> > + * @name: Trinity app name
> > + * @status: Trinity app status
> > + * @num_total_reqs: Number of total requests in app (including 
> > finished ones)
> > + * @num_active_reqs: Number of active (running or pending) requests in 
> > app
> > + * @total_alloc_mem: Total size of allocated memory in the device
> > + * @total_freed_mem: Total size of freed memory in the device
> > + */
> > +struct trinity_ioctl_stat_app {
> > +	__s32 app_id;
> > +
> > +	char name[TRINITY_APP_NAME_MAX];
> > +	enum trinity_app_status status;
> > +
> > +	__u32 num_total_reqs;
> > +	__u32 num_active_reqs;
> > +
> > +	__u64 total_alloc_mem;
> > +	__u64 total_freed_mem;
> > +} __packed;
> 
> 'enum' in a uapi structure is not well-defined across
> architectures, so better use a fixed-size type there.
> 
> Instead of packing the structure, you should keep all
> members naturally aligned and add explicit padding
> or change some members for 32-bit to 64-bit size
> to keep everything naturally aligned.
> 

I checked, the members will be aligned.


> > +/**
> > + * struct trinity_ioctl_profile_buff - Describes profiling buff info.
> > + * @req_id: The target req id for profiling
> > + * @profile_pos: The start position to extract profiling data
> > + * @profile_size: The size of user-allocated profiling buffer
> > + * @profile_buf: The profiling buffer which user allocated
> > + */
> > +struct trinity_ioctl_profile_buff {
> > +	__s32 req_id;
> > +	__u32 profile_pos;
> > +	__u32 profile_size;
> > +	void __user *profile_buf;
> > +} __packed;
> 
> Don't put pointers into ioctl structures, they just make compat
> mode unnecessarily hard. You can use a __u64 member.
> 

OK. thanks.

> > +/**
> > + * Major number can not be dynamic as ioctls need it,
> > + */
> > +#define TRINITY_DRIVER_MAGIC 0x88
> > +
> > +#define TRINITY_IO(no) _IO(TRINITY_DRIVER_MAGIC, no)
> > +#define TRINITY_IOR(no, data_type) _IOR(TRINITY_DRIVER_MAGIC, no, 
> > data_type)
> > +#define TRINITY_IOW(no, data_type) _IOW(TRINITY_DRIVER_MAGIC, no, 
> > data_type)
> > +#define TRINITY_IOWR(no, data_type) _IOWR(TRINITY_DRIVER_MAGIC, no, 
> > data_type)
> 
> These macros just hurt tools that want to parse the headers.
> Please just open-code the usage.
> 
> > +#ifdef __KERNEL__
> > +__s32 trinity_run_internal_req(dev_t);
> > +#endif
> 
> This doesn't seem to belong into the uapi header.
> 
>       Arnd
> 

macros and useless codes will be cleared.

Thanks.
Jiho Chu