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

[wangyushan]

On 2025/1/8 3:05, Christophe JAILLET wrote:
> Le 07/01/2025 à 14:29, Yushan Wang a écrit :
> > From: Jie Wang <wangjie125@xxxxxxxxxx>
> >
> > 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.

Yes, will fix that in the next version.

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

Will fix here as well.

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

Yes, run checkpatch with --strict will generate some check advises.
Will fix this in the next version.

Thanks!

Regards,
Yushan

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