From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2BC32B644 for ; Wed, 8 Jan 2025 09:59:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=45.249.212.189 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736330357; cv=none; b=rKZh6HB6rqoiEr+MZELZ9GrZgknJ9bt1tFoXX3WoHxqTlXB+JzHj2OTNNGM1ETcT6ckJEOke26CO69ehRHmXRHGoHsJ64/nefNUotaSnAmhOB5iP6mEl61MdVVsH+wyriHxbWd+o2F1w61EUwxhgIxN5M38KXzQ4/BrMraVJV60= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736330357; c=relaxed/simple; bh=OqsvJYOy33QqdUwrQgmdZNVJePXBOuA2DGV3YTiJk1M=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=OlQNgp1WNqIh59LhvPGdvqMAGstzgZmXmlQlUOk37lCu2Ie8EZKDqVe+iK7R+B7gjVDR3W7MrfGUsO/RtDbIg+HaxUjKKmlM2xukxYaNL7npwGi0eUy/iHh84qhs/QNFFslx+1jUeehcDQqMej/oaA9Le5R4HGg931zWRzMSGNU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=45.249.212.189 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.19.162.254]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4YSjxh18y6zRkxM; Wed, 8 Jan 2025 17:56:56 +0800 (CST) Received: from dggpemf100016.china.huawei.com (unknown [7.185.36.236]) by mail.maildlp.com (Postfix) with ESMTPS id 68D7D180106; Wed, 8 Jan 2025 17:59:11 +0800 (CST) Received: from [10.67.120.139] (10.67.120.139) by dggpemf100016.china.huawei.com (7.185.36.236) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Wed, 8 Jan 2025 17:59:11 +0800 Message-ID: <1fec0936-a7dd-43e4-8ad6-a18df35c6d9d@huawei.com> Date: Wed, 8 Jan 2025 17:59:10 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] soc cache: Add framework driver for HiSilicon SoC cache To: Christophe JAILLET , , , , , CC: , , References: <20250107132907.3521574-1-wangyushan12@huawei.com> <20250107132907.3521574-2-wangyushan12@huawei.com> <1a32d4fc-ff9c-45a5-82fb-1bd3b65df791@wanadoo.fr> Content-Language: en-US From: wangyushan In-Reply-To: <1a32d4fc-ff9c-45a5-82fb-1bd3b65df791@wanadoo.fr> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpemf100016.china.huawei.com (7.185.36.236) On 2025/1/8 3:05, Christophe JAILLET wrote: > Le 07/01/2025 à 14:29, Yushan Wang a écrit : >> From: Jie Wang >> >> 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 >