Message-ID: <55CD386A.5070801@codeaurora.org>
Date: Thu, 13 Aug 2015 17:38:02 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5
MIME-Version: 1.0
To: Mathieu Olivari <mathieu@codeaurora.org>
CC: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
        linux@arm.linux.org.uk, dwmw2@infradead.org,
        computersforpeace@gmail.com, agross@codeaurora.org,
        bjorn.andersson@sonymobile.com, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mtd@lists.infradead.org
Subject: Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms
References: <1439501620-24073-1-git-send-email-mathieu@codeaurora.org> <1439501620-24073-4-git-send-email-mathieu@codeaurora.org>
In-Reply-To: <1439501620-24073-4-git-send-email-mathieu@codeaurora.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5535
Lines: 205

On 08/13/2015 02:33 PM, Mathieu Olivari wrote:
> diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
> new file mode 100644
> index 0000000..6cbf610
> --- /dev/null
> +++ b/drivers/mtd/qcom_smem_part.c
>
> +#define SMEM_PARTS_MAX			32
> +
> +struct smem_partition {
> +	char name[SMEM_PART_NAME_SZ];
> +	uint32_t start;
> +	uint32_t size;
> +	uint32_t attr;

Is this little endian? __le32? I think so.

> +};
> +
> +struct smem_partition_table {
> +	uint32_t magic[2];
> +	uint32_t version;
> +	uint32_t len;

Same for these being little endian. And if so, please make magic into a 
byte array of size 8 and then do a memcmp with another static byte array.

> +	struct smem_partition parts[SMEM_PARTS_MAX];
> +};
> +
> +static int qcom_smem_get_flash_blksz(uint64_t **smem_blksz)

I don't know, but I prefer u64 because it's short and sweet. But I'm not 
the maintainer here.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> +			    (void **) smem_blksz, &size);

Is the cast necessary?

> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash blksz from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_blksz)) {
> +		pr_err("Invalid flash blksz size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_type(uint64_t **smem_flash_type)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> +			    (void **) smem_flash_type, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash type from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_flash_type)) {
> +		pr_err("Invalid flash type size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> +			    (void **) pparts, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read partition table from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static bool is_spi_device(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);

I'm pretty sure this holds a reference to the device that needs to be 
released with put_device().

> +	return dev ? true : false;
> +}
> +
> +static int parse_qcom_smem_partitions(struct mtd_info *master,
> +				      struct mtd_partition **pparts,
> +				      struct mtd_part_parser_data *data)
> +{
> +	struct smem_partition_table *smem_parts;
> +	uint64_t *smem_flash_type, *smem_blksz;
> +	struct mtd_partition *mtd_parts;
> +	struct device_node *of_node = data->of_node;
> +	int i, ret;
> +
> +	/*
> +	 * SMEM will only store the partition table of the boot device.
> +	 * If this is not the boot device, do not return any partition.
> +	 */
> +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> +		return 0;
> +
> +	/*
> +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> +	 * block size of the MTD device
> +	 */
> +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*smem_blksz != master->erasesize) {
> +		pr_err("SMEM block size differs from MTD block size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get partition pointer from SMEM */
> +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (smem_parts->magic[0] != SMEM_PTABLE_MAGIC_1 ||
> +	    smem_parts->magic[1] != SMEM_PTABLE_MAGIC_2) {
> +		pr_err("SMEM partition magic invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate and populate the mtd structures */
> +	mtd_parts = kcalloc(smem_parts->len, sizeof(struct mtd_partition),

sizeof(*mtd_parts)?

> +			    GFP_KERNEL);
> +	if (mtd_parts == NULL)

if (!mtd_parts) is more kernel style.

> +		return -ENOMEM;
> +
> +	for (i = 0; i < smem_parts->len; i++) {
> +		struct smem_partition *part = &smem_parts->parts[i];
> +
> +		mtd_parts[i].name = part->name;
> +		mtd_parts[i].size = part->size * (*smem_blksz);
> +		mtd_parts[i].offset = part->start * (*smem_blksz);
> +
> +		/*
> +		 * The last SMEM partition may have its size marked as
> +		 * something like 0xffffffff, which means "until the end of the
> +		 * flash device". In this case, truncate it.
> +		 */
> +		if (mtd_parts[i].offset + mtd_parts[i].size > master->size)
> +			mtd_parts[i].size = master->size - mtd_parts[i].offset;

That is 7 mtd_parts[i]. instances. Consider making a local variable 
pointer instead. Actually we could make two pointers for 
smem_parts->part and mtd_parts and increment both in the loop to make 
tighter code.

> +	}
> +
> +	*pparts = mtd_parts;

Oh we already have the pointer, so just do this assignment before iterating.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/