Re: [PATCH 1/3] soc cache: L3 cache driver for HiSilicon SoC

[wangyushan <wangyushan12@huawei.com>]

On 2/4/2026 1:19 AM, Arnd Bergmann wrote:
> On Tue, Feb 3, 2026, at 17:18, Yushan Wang wrote:
>> The driver will create a file of `/dev/hisi_l3c` on init, mmap
>> operations to it will allocate a memory region that is guaranteed to be
>> placed in L3 cache.
>>
>> The driver also provides unmap() to deallocated the locked memory.
>>
>> The driver also provides an ioctl interface for user to get cache lock
>> information, such as lock restrictions and locked sizes.
>>
>> Signed-off-by: Yushan Wang <wangyushan12@huawei.com>
> Hi Yushan,
>
> Thanks for your submission. Since we are in the last week of
> the merge window, this is not going to be linux-7.0 material,
> but I'll have a quick look for now.

Many thanks for review!
>>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>>  MAINTAINERS                                   |   6 +
>>  drivers/soc/hisilicon/Kconfig                 |  11 +
>>  drivers/soc/hisilicon/Makefile                |   2 +
>>  drivers/soc/hisilicon/hisi_soc_l3c.c          | 357 ++++++++++++++++++
>>  include/uapi/misc/hisi_l3c.h                  |  28 ++
> I don't think this should be in drivers/soc/, since I want
> to reserve that for internal drivers without a user visible
> interface other than the soc_device information. (yes, there
> are a few historic counterexamples)
>
> I also don't think this should be a hilicon specific interface,
> if possible. The functionality is not that unusual in the end.

I hesitated about the directory as well, exporting a user aware
interface directly from driver may not be a good practice and I
am hoping for advice :)
The driver itself doesn't provide much more than a mere register configuration which is platform specific, what do you think about a kernel space interface somewhere else which implemented here?
> We had similar concepts using the numactl system calls in the
> part, but I don't think we should do that here because you may
> need the numa interfaces for other purposes as well, and it
> may be confusing to existing callers.
>
> Having a generic madvise() based interface would be great,
> not sure if hardware support for that is common enough for that.
As above, cache lock could be an option for optimization of numactl or madvise(), maybe a more generic interface in kernel space which is speculatively called by other infrastructure is better, and of course extra support of the infrastructures is needed. I can try to propose an interface and maybe an example of the caller in the next version.
>> +	/* Continuous physical memory is required for L3 cache lock. */
>> +	pg = alloc_contig_pages(1 << order, GFP_KERNEL | __GFP_NOWARN | 
>> __GFP_ZERO,
>> +				cpu_to_node(smp_processor_id()), NULL);
> Since this is a user allocation, should that be GFP_USER instead
> of GFP_KERNEL?

Yes, it should be GFP_USER in this version since it is used in userspace.
>> +/* HISI_L3C_INFO: cache lock info for HiSilicon SoC */
>> +#define HISI_L3C_LOCK_INFO	_IOW(0xBB, 1, unsigned long)
> The specification here looks wrong, please see
> Documentation/driver-api/ioctl.rst
>
> I think for your implementation it should be
>
> #define HISI_L3C_LOCK_INFO	_IOR(0xBB, 1, hisi_l3c_lock_info)

Sorry, I will correct that in the next version.
>> +struct hisi_l3c_lock_info {
>> +	__u32 lock_region_num;
>> +	__u64 lock_size;
>> +	__u8 address_alignment;
>> +	__u64 max_lock_size;
>> +	__u64 min_lock_size;
>> +};
> You are leaking kernel data because of the padding in this structure,
> please rearrange the members to avoid padding.
>
> It may be better to use a different interface instead of ioctl(),
> possibly exporting global data in sysfs.

Yes, information through sysfs should do the job.
I will fix that in the next version.

>        Arnd

Thanks,
Yushan