| 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
| ||
|
Message-ID: <14438cf9-ec78-afb5-107a-4ed954ac0eb7@nvidia.com>
Date: Tue, 9 May 2023 18:47:47 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<treding@...dia.com>, <dmitry.osipenko@...labora.com>,
<viresh.kumar@...aro.org>, <rafael@...nel.org>,
<jonathanh@...dia.com>, <robh+dt@...nel.org>,
<lpieralisi@...nel.org>, <helgaas@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <mmaddireddy@...dia.com>,
<kw@...ux.com>, <bhelgaas@...gle.com>, <vidyas@...dia.com>,
<sanjayc@...dia.com>, <ksitaraman@...dia.com>, <ishah@...dia.com>,
<bbasu@...dia.com>, Sumit Gupta <sumitg@...dia.com>
Subject: Re: [Patch v7 1/8] memory: tegra: add interconnect support for DRAM
scaling in Tegra234
On 07/05/23 14:41, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 24/04/2023 17:21, Sumit Gupta wrote:
>>
>>
>> On 24/04/23 20:30, Sumit Gupta wrote:
>>>
>>>
>>> On 24/04/23 19:18, Krzysztof Kozlowski wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 24/04/2023 15:13, Sumit Gupta wrote:
>>>>> Add Interconnect framework support to dynamically set the DRAM
>>>>> bandwidth from different clients. Both the MC and EMC drivers are
>>>>> added as ICC providers. The path for any request is:
>>>>> MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM
>>>>>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>
>>>>> +static int tegra_emc_interconnect_init(struct tegra186_emc *emc)
>>>>> +{
>>>>> + struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);
>>>>> + const struct tegra_mc_soc *soc = mc->soc;
>>>>> + struct icc_node *node;
>>>>> + int err;
>>>>> +
>>>>> + emc->provider.dev = emc->dev;
>>>>> + emc->provider.set = tegra_emc_icc_set_bw;
>>>>> + emc->provider.data = &emc->provider;
>>>>> + emc->provider.aggregate = soc->icc_ops->aggregate;
>>>>> + emc->provider.xlate = tegra_emc_of_icc_xlate;
>>>>> + emc->provider.get_bw = tegra_emc_icc_get_init_bw;
>>>>> +
>>>>> + icc_provider_init(&emc->provider);
>>>>> +
>>>>> + /* create External Memory Controller node */
>>>>> + node = icc_node_create(TEGRA_ICC_EMC);
>>>>> + if (IS_ERR(node)) {
>>>>> + err = PTR_ERR(node);
>>>>> + goto err_msg;
>>>>> + }
>>>>> +
>>>>> + node->name = "External Memory Controller";
>>>>> + icc_node_add(node, &emc->provider);
>>>>> +
>>>>> + /* link External Memory Controller to External Memory (DRAM) */
>>>>> + err = icc_link_create(node, TEGRA_ICC_EMEM);
>>>>> + if (err)
>>>>> + goto remove_nodes;
>>>>> +
>>>>> + /* create External Memory node */
>>>>> + node = icc_node_create(TEGRA_ICC_EMEM);
>>>>> + if (IS_ERR(node)) {
>>>>> + err = PTR_ERR(node);
>>>>> + goto remove_nodes;
>>>>> + }
>>>>> +
>>>>> + node->name = "External Memory (DRAM)";
>>>>> + icc_node_add(node, &emc->provider);
>>>>> +
>>>>> + err = icc_provider_register(&emc->provider);
>>>>> + if (err)
>>>>> + goto remove_nodes;
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +remove_nodes:
>>>>> + icc_nodes_remove(&emc->provider);
>>>>> +err_msg:
>>>>> + dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
>>>>> +
>>>>> + return err;
>>>>> +}
>>>>> +
>>>>> static int tegra186_emc_probe(struct platform_device *pdev)
>>>>> {
>>>>> + struct tegra_mc *mc = dev_get_drvdata(pdev->dev.parent);
>>>>> struct mrq_emc_dvfs_latency_response response;
>>>>> struct tegra_bpmp_message msg;
>>>>> struct tegra186_emc *emc;
>>>>> @@ -236,6 +339,29 @@ static int tegra186_emc_probe(struct
>>>>> platform_device *pdev)
>>>>> debugfs_create_file("max_rate", S_IRUGO | S_IWUSR,
>>>>> emc->debugfs.root,
>>>>> emc, &tegra186_emc_debug_max_rate_fops);
>>>>>
>>>>> + if (mc && mc->soc->icc_ops) {
>>>>> + /*
>>>>> + * Initialize the ICC even if BPMP-FW doesn't support
>>>>> 'MRQ_BWMGR_INT'.
>>>>> + * Use the flag 'mc->bwmgr_mrq_supported' within MC
>>>>> driver and return
>>>>> + * EINVAL instead of passing the request to BPMP-FW
>>>>> later when the BW
>>>>> + * request is made by client with 'icc_set_bw()' call.
>>>>> + */
>>>>> + err = tegra_emc_interconnect_init(emc);
>>>>> + if (err)
>>>>> + goto put_bpmp;
>>>>> +
>>>>> + if (tegra_bpmp_mrq_is_supported(emc->bpmp,
>>>>> MRQ_BWMGR_INT)) {
>>>>> + mc->bwmgr_mrq_supported = true;
>>>>> +
>>>>> + /*
>>>>> + * MC driver probe can't get BPMP reference as
>>>>> it gets probed
>>>>> + * earlier than BPMP. So, save the BPMP ref got
>>>>> from the EMC
>>>>> + * DT node in the mc->bpmp and use it in MC's
>>>>> icc_set hook.
>>>>> + */
>>>>> + mc->bpmp = emc->bpmp;
>>>>
>>>> This (and ()) are called without any locking. You register first the
>>>> interconnect, so set() callback can be used, right? Then set() could be
>>>> called anytime between tegra_emc_interconnect_init() and assignment
>>>> above. How do you synchronize these?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Currently, the tegra234_mc_icc_set() has NULL check. So, it will give
>>> this error.
>>> if (!mc->bpmp) {
>
> How does it solve concurrent accesses and re-ordering of instructions by
> compiler or CPU?
>
Now, the "mc->bpmp" is set before tegra_emc_interconnect_init().
So, until the EMC interconnect initializes, set() won't be
called as the devm_of_icc_get() call will fail.
Added the change in v8.
Thank you,
Sumit Gupta
>>> dev_err(mc->dev, "BPMP reference NULL\n");
>>> return -ENOENT;
>>> }
>>>
>>> To fix, we can do the below change and swap the order. Please let me
>>> know if this sounds fine ?
>>> if (mc && mc->soc->icc_ops) {
>>> if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
>>> mc->bwmgr_mrq_supported = true;
>>> mc->bpmp = emc->bpmp;
>>> }
>>>
>>> err = tegra_emc_interconnect_init(emc);
>>> if (err)
>>> goto put_bpmp;
>>>
>>> }
>> Sorry, replied too soon. Missed setting "mc->bpmp" to NULL. Sharing new
>> proposed change. Please let me know if this looks OK.
>>
>> if (mc && mc->soc->icc_ops) {
>> if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
>> mc->bwmgr_mrq_supported = true;
>> mc->bpmp = emc->bpmp;
>> }
>>
>> err = tegra_emc_interconnect_init(emc);
>> if (err) {
>> mc->bpmp = NULL;
>> goto put_bpmp;
>
> Sorry, I didn't check the code. I assume you should do it...
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists