linux-kernel - Re: [PATCH 1/2] soc cache: Add framework driver for HiSilicon SoC cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a32d4fc-ff9c-45a5-82fb-1bd3b65df791@wanadoo.fr>
Date: Tue, 7 Jan 2025 20:05:28 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Yushan Wang <wangyushan12@...wei.com>, xuwei5@...ilicon.com,
 yangyicong@...ilicon.com, Jonathan.Cameron@...wei.com,
 wangjie125@...wei.com, linux-kernel@...r.kernel.org
Cc: prime.zeng@...ilicon.com, fanghao11@...wei.com, linuxarm@...wei.com
Subject: Re: [PATCH 1/2] soc cache: Add framework driver for HiSilicon SoC
 cache

Le 07/01/2025 à 14:29, Yushan Wang a écrit :
> From: Jie Wang <wangjie125@...wei.com>
> 
> HiSilicon SoC cache is comprised of multiple hardware devices, a driver
> in this patch is used to provide common utilities for other drivers to
> avoid redundancy.

...

> +static int hisi_soc_cache_lock(int cpu, phys_addr_t addr, size_t size)
> +{
> +	struct hisi_soc_comp_inst *inst;
> +	struct list_head *head;
> +	int ret = -ENOMEM;
> +
> +	guard(spinlock)(&soc_cache_devs[HISI_SOC_L3C].lock);
> +
> +	/* Iterate L3C instances to perform operation, break loop once found. */
> +	head = &soc_cache_devs[HISI_SOC_L3C].node;
> +	list_for_each_entry(inst, head, node) {
> +		if (!cpumask_test_cpu(cpu, &inst->comp->affinity_mask))
> +			continue;
> +		ret = inst->comp->ops->do_lock(inst->comp, addr, size);
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +
> +	list_for_each_entry(inst, head, node) {

Do we need to iterate another time.
Isn't "inst" already correct?

If so, I guess that:
	ret = inst->comp->ops->poll_lock_done(inst->comp, addr, size)
	if (ret)
		return ret;

could be moved at the end the previous loop to both simplify the code, 
and save a few cycles.

> +		if (!cpumask_test_cpu(cpu, &inst->comp->affinity_mask))
> +			continue;
> +		ret = inst->comp->ops->poll_lock_done(inst->comp, addr, size);
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int hisi_soc_cache_unlock(int cpu, phys_addr_t addr)
> +{
> +	struct hisi_soc_comp_inst *inst;
> +	struct list_head *head;
> +	int ret = 0;
> +
> +	guard(spinlock)(&soc_cache_devs[HISI_SOC_L3C].lock);
> +
> +	/* Iterate L3C instances to perform operation, break loop once found. */
> +	head = &soc_cache_devs[HISI_SOC_L3C].node;
> +	list_for_each_entry(inst, head, node) {
> +		if (!cpumask_test_cpu(cpu, &inst->comp->affinity_mask))
> +			continue;
> +		ret = inst->comp->ops->do_unlock(inst->comp, addr);
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +
> +	list_for_each_entry(inst, head, node) {

Same as above.

> +		if (!cpumask_test_cpu(cpu, &inst->comp->affinity_mask))
> +			continue;
> +		ret = inst->comp->ops->poll_unlock_done(inst->comp, addr);
> +		if (ret)
> +			return ret;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int hisi_soc_cache_inst_check(const struct hisi_soc_comp *comp,
> +				     enum hisi_soc_comp_type comp_type)
> +{
> +	struct hisi_soc_comp_ops *ops = comp->ops;
> +
> +	/* Different types of component could have different ops. */
> +	switch (comp_type) {
> +	case HISI_SOC_L3C:
> +		if (!ops->do_lock || !ops->poll_lock_done
> +		    || !ops->do_unlock || !ops->poll_unlock_done)

I think that || should be at the end of the previous line.
If I remember correctly checkpatch (maybe with --strict) complains about it.

> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...

CJ