Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753059AbdHWA2U (ORCPT ); Tue, 22 Aug 2017 20:28:20 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:54164 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752985AbdHWA2S (ORCPT ); Tue, 22 Aug 2017 20:28:18 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 917816047C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=clew@codeaurora.org Subject: Re: [PATCH 1/3] soc: qcom: smem: Support global partition To: Bjorn Andersson Cc: andy.gross@linaro.org, david.brown@linaro.org, aneela@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, linux-kernel@vger.kernel.org References: <1503018948-26629-1-git-send-email-clew@codeaurora.org> <1503018948-26629-2-git-send-email-clew@codeaurora.org> <20170821171733.GU29306@minitux> From: Chris Lew Message-ID: Date: Tue, 22 Aug 2017 17:28:14 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170821171733.GU29306@minitux> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5293 Lines: 178 Hi Bjorn, Thanks for the review. On 8/21/2017 10:17 AM, Bjorn Andersson wrote: >> #define SMEM_MASTER_SBL_VERSION_INDEX 7 >> -#define SMEM_EXPECTED_VERSION 11 >> +#define SMEM_GLOBAL_HEAP_VERSION 11 >> + >> +/* >> + * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure >> + * for the global heap. A new global partition is created from the global heap >> + * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is >> + * set by the bootloader. >> + */ > > Please incorporate this paragraph in the larger comment at the top of > the file, so we keep that up to date. > > [..] Will do. >> @@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size) >> { >> unsigned long flags; >> int ret; >> + struct smem_partition_header *phdr; > > Sorry for my OCD, but please maintain my reverse XMAS tree (put this > declaration first, as it's the longest). > Ok, will change. >> >> if (!__smem) >> return -EPROBE_DEFER; > [..] >> @@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host) >> >> static int qcom_smem_get_sbl_version(struct qcom_smem *smem) >> { >> + struct smem_header *header; >> __le32 *versions; >> - size_t size; >> >> - versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size); >> - if (IS_ERR(versions)) { >> - dev_err(smem->dev, "Unable to read the version item\n"); >> - return -ENOENT; >> + header = smem->regions[0].virt_base; >> + versions = header->version; >> + >> + return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]); >> +} > > I would prefer if you split the change of this function into its own > patch, just to clarify that it's unrelated to the new version support. > Ok, will separate the change and adjust the descriptions accordingly. >> + >> +static int qcom_smem_set_global_partition(struct qcom_smem *smem, >> + struct smem_ptable_entry *entry) >> +{ >> + struct smem_partition_header *header; >> + u32 host0, host1; >> + >> + if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) { >> + dev_err(smem->dev, "Invalid entry for gloabl partition\n"); >> + return -EINVAL; >> } >> >> - if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) { >> - dev_err(smem->dev, "Version item is too small\n"); >> + if (smem->global_partition) { >> + dev_err(smem->dev, "Already found the global partition\n"); >> return -EINVAL; >> } >> + header = smem->regions[0].virt_base + le32_to_cpu(entry->offset); >> + host0 = le16_to_cpu(header->host0); >> + host1 = le16_to_cpu(header->host1); >> >> - return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]); >> + if (memcmp(header->magic, SMEM_PART_MAGIC, >> + sizeof(header->magic))) { >> + dev_err(smem->dev, "Gloal partition has invalid magic\n"); >> + return -EINVAL; >> + } >> + >> + if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) { >> + dev_err(smem->dev, "Global partition hosts are invalid\n"); >> + return -EINVAL; >> + } >> + >> + if (header->size != entry->size) { > > This happens to work, because they are both in the same endian. But > please sprinkle some le32_to_cpu() here as well. > These checks mimic the sanity checks being done in enumerate_partitions. Should I create a patch to increase le32_to_cpu usage in qcom_smem_enumerate_partitions? >> + dev_err(smem->dev, "Global partition has invalid size\n"); >> + return -EINVAL; >> + } >> + >> + if (le32_to_cpu(header->offset_free_uncached) > >> + le32_to_cpu(header->size)) { > > Consider using local variables in favor of wrapping lines. > Ok, will do. >> + dev_err(smem->dev, >> + "Global partition has invalid free pointer\n"); >> + return -EINVAL; >> + } >> + >> + smem->global_partition = header; >> + smem->global_cacheline = le32_to_cpu(entry->cacheline); >> + >> + return 0; >> } >> >> static int qcom_smem_enumerate_partitions(struct qcom_smem *smem, >> @@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem *smem, >> host0 = le16_to_cpu(entry->host0); >> host1 = le16_to_cpu(entry->host1); >> >> + if (host0 == SMEM_GLOBAL_HOST && host0 == host1) { >> + if (qcom_smem_set_global_partition(smem, entry)) >> + return -EINVAL; >> + continue; >> + } >> + > > As you're not able to leverage any of the checks from this loop I think > it's cleaner to duplicate the traversal of the partition table in both > functions and call the "search for global partition" directly from > probe - if the version indicates there should be one. > Ok, will set the global partition in the version case statement and error out of the probe if finding the global partition fails since it is not optional in the new version. >> if (host0 != local_host && host1 != local_host) >> continue; >> >> @@ -782,7 +855,10 @@ static int qcom_smem_probe(struct platform_device *pdev) >> } >> >> version = qcom_smem_get_sbl_version(smem); >> - if (version >> 16 != SMEM_EXPECTED_VERSION) { >> + switch (version >> 16) { >> + case SMEM_GLOBAL_PART_VERSION: >> + case SMEM_GLOBAL_HEAP_VERSION: > > As Arun pointed out, there should be a "break" here. > Will change. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project