Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-935-ge4ccd4c47b-fm-20220914.001-ge4ccd4c4
Mime-Version: 1.0
Message-Id: <e035ac54-35af-4e86-a74a-9a4c7f936a19@www.fastmail.com>
In-Reply-To: <20220917072356.2255620-2-jiho.chu@samsung.com>
References: <20220917072356.2255620-1-jiho.chu@samsung.com>
 <CGME20220917072357epcas1p17b277154f0d020435417450fa0337906@epcas1p1.samsung.com>
 <20220917072356.2255620-2-jiho.chu@samsung.com>
Date:   Sat, 17 Sep 2022 09:41:13 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Jiho Chu" <jiho.chu@samsung.com>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        ogabbay@kernel.org,
        "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>,
        "Mark Brown" <broonie@kernel.org>
Cc:     linux-kernel@vger.kernel.org, yelini.jeong@samsung.com,
        myungjoo.ham@samsung.com
Subject: Re: [PATCH v2 01/13] trinity: Add base driver
Content-Type: text/plain
Precedence: bulk

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

> +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.

> +
> +#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.

> +/**
> + * 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.

> +/**
> + * 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