Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Lina Iyer <ilina@codeaurora.org>]

On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
>On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index b81374bb6713..f21c5d53e721 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -97,4 +97,11 @@ config QCOM_WCNSS_CTRL
>>  	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
>>  	  firmware to a newly booted WCNSS chip.
>>
>> +config QCOM_COMMAND_DB
>> +	bool "Qualcomm Command DB"
>> +	depends on ARCH_QCOM
>> +	help
>> +	  Command DB queries shared memory by key string for shared system
>> +	  resources
>> +
>
>Please try to keep these alphabetically sorted.
>
Sure.

>> +#define RESOURCE_ID_LEN		8
>
>Only used in unused structure below?
>
Removed along with the unused structure.

>> +#define NUM_PRIORITY		2
>> +#define MAX_SLV_ID		8
>> +#define CMD_DB_MAGIC		0x0C0330DBUL
>> +#define SLAVE_ID_MASK		0x7
>> +#define SLAVE_ID_SHIFT		16
>> +
>
>It would be nice with a kernel-doc "DOC:" here that describes the
>in-memory format that we're operating on.
>
I will try to add something. Not sure how much detail I can get into.

>> +/**
>> + * entry_header: header for each entry in cmddb
>> + *
>> + * @res_id: resource's identifier
>
>"id" is shorter and just as descriptive.
>
Ok

>> + * @priority: unused
>> + * @addr: the address of the resource
>> + * @len: length of the data
>> + * @offset: offset at which dats starts
>
>"data"
>
OK

>> + */
>> +struct entry_header {
>> +	uint64_t res_id;
>> +	u32 priority[NUM_PRIORITY];
>> +	u32 addr;
>> +	u16 len;
>> +	u16 offset;
>> +};
>> +
>> +/**
>> + * rsc_hdr: resource header information
>> + *
>> + * @slv_id: id for the resource
>> + * @header_offset: Entry header offset from data
>> + * @data_offset: Entry offset for datda location
>> + * @cnt: number of enteries for HW type
>> + * @version: MSB is major, LSB is minor
>> + */
>> +struct rsc_hdr {
>> +	u16  slv_id;
>> +	u16  header_offset;
>> +	u16  data_offset;
>> +	u16  cnt;
>> +	u16  version;
>> +	u16 reserved[3];
>> +};
>> +
>> +/**
>> + * cmd_db_header: The DB header information
>> + *
>> + * @version: The cmd db version
>> + * @magic_number: constant expected in the database
>> + * @header: array of resources
>
>Missing @check_sum and @data.
>
Will add.

>> + */
>> +struct cmd_db_header {
>> +	u32 version;
>> +	u32 magic_num;
>> +	struct rsc_hdr header[MAX_SLV_ID];
>> +	u32 check_sum;
>> +	u32 reserved;
>> +	u8 data[];
>> +};
>> +
>> +/**
>> + * cmd_db_entry: Inforamtion for each line in the cmd db
>> + *
>> + * @resource_id: unique identifier for each entry
>> + * @addr: slave id + offset address
>> + * @priority: Bitmask for DRV ids
>> + * @len: aux data len
>> + * @data: data assocaited with the resource
>> + */
>> +struct cmd_db_entry {
>
>Why isn't this used anywhere? What am I missing?
>
Reminiscent of some old code. Will remove.

>> +	const char resource_id[RESOURCE_ID_LEN + 1];
>> +	const u32 addr;
>> +	const u32 priority[NUM_PRIORITY];
>> +	u32 len;
>> +	u16 version;
>> +	u8  data[];
>> +};
>> +
>> +static void __iomem *start_addr;
>> +static struct cmd_db_header *cmd_db_header;
>
>Rather than keeping a copy of this around you can verify the magic in
>probe and then just carry an object that's:
>
>struct cmd_db {
>	struct rsc_hdr *headers;
>	void *entries;
>};
>
>That way you can drop the start_addr variable as well...
>
I will try and reorganize. But I would like to keep the data structure
definition the same as it is here. It helps to be in parity with the
non-linux end that populates the data.

>> +static int cmd_db_status = -EPROBE_DEFER;
>
>You don't really need a separate variable to track that probe() isn't
>done. cmd_db_header can be used for this...
>
Will remove.

>> +
>> +static u64 cmd_db_get_u64_id(const char *id)
>> +{
>> +	uint64_t rsc_id = 0;
>> +	uint8_t *ch  = (uint8_t *)&rsc_id;
>> +	int i;
>> +
>> +	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
>> +		ch[i] = id[i];
>> +
>> +	return rsc_id;
>> +}
>
>So this "casts" a 8 byte string to a u64. Why not just use u64 constants
>to represent the resources, as we've done in the past?
>
This matches the resource specification on the remote end for this new
architecture.

>> +
>> +static int cmd_db_get_header(u64 query, struct entry_header *eh,
>> +		struct rsc_hdr *rh, bool use_addr)
>
>This function is static and there's only one caller, which has use_addr
>as false. So please omit this parameter for now.
>
Ok.
>> +{
>> +	struct rsc_hdr *rsc_hdr;
>> +	int i, j;
>> +
>> +	if (!cmd_db_header)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!eh || !rh)
>> +		return -EINVAL;
>> +
>> +	rsc_hdr = &cmd_db_header->header[0];
>
>Rather than bumping the pointer in the loop, just move this inside the
>loop as:
>	rsc_hdr = &cmd_db_header->header[i];
>
Sure.

>> +
>> +	for (i = 0; i < MAX_SLV_ID ; i++, rsc_hdr++) {
>> +		struct entry_header *ent;
>> +
>> +		if (!rsc_hdr->slv_id)
>> +			break;
>> +
>> +		ent = (struct entry_header *)(start_addr
>> +				+ sizeof(*cmd_db_header)
>> +				+ rsc_hdr->header_offset);
>
>If you have start_addr expressed as a struct cmd_db_header then this
>would be:
>
>	ent = (struct entry_header *)db->data + rsc_hdr->header_offset;
>
>> +
>> +		for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
>> +			if (use_addr) {
>> +				if (ent->addr == (u32)(query))
>> +					break;
>> +			} else if (ent->res_id == query)
>> +				break;
>> +		}
>> +
>> +		if (j < rsc_hdr->cnt) {
>> +			memcpy(eh, ent, sizeof(*ent));
>> +			memcpy(rh, &cmd_db_header->header[i], sizeof(*rh));
>> +			return 0;
>
>This function really returns reference to data, length of the data and
>version. How about just making this explicit, rather than copying two
>sets of headers to the caller and having them extract this information?
>
>I think you should turn this function into:
>
>static void *cmd_db_find(u64 id, size_t *len, u16 *version)
>
I think this is much cleaner, to return the entry that we searched for,
rather than return a void *.

>> +		}
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>> +static int cmd_db_get_header_by_rsc_id(const char *resource_id,
>> +		struct entry_header *ent_hdr,
>> +		struct rsc_hdr *rsc_hdr)
>> +{
>> +	u64 rsc_id = cmd_db_get_u64_id(resource_id);
>> +
>> +	return cmd_db_get_header(rsc_id, ent_hdr, rsc_hdr, false);
>> +}
>> +
>> +u32 cmd_db_get_addr(const char *resource_id)
>> +{
>> +	int ret;
>> +	struct entry_header ent;
>> +	struct rsc_hdr rsc_hdr;
>> +
>> +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
>> +
>> +	return ret < 0 ? 0 : ent.addr;
>> +}
>> +EXPORT_SYMBOL(cmd_db_get_addr);
>
>Please kernel-doc any publicly visible functions.
>
>I presume the returned value of this function is going to be used to
>reference the data directly, as such it should return a void *.
>
The original author chose to document the functions in the header file.
Will move to the .c file.

>> +
>> +int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)
>
>"cmd_db_read_data()" would better represent that we're copying the data
>into the buffer.
>
>
>(And the "aux" part in these function names doesn't seem to add any
>value)
>
That is how it is represented in the architecture document. It is more
helpful for the driver author to maintain relation with the database
that is being read.

>> +{
>> +	int ret;
>> +	struct entry_header ent;
>> +	struct rsc_hdr rsc_hdr;
>> +
>> +	if (!data)
>> +		return -EINVAL;
>> +
>> +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (ent.len < len)
>> +		return -EINVAL;
>
>The purpose of this check is very likely "will the data fit in @data".
>In which case the comparison should be len < ent.len
>
Yes. Will fix.

>> +
>> +	len = (ent.len < len) ? ent.len : len;
>
>In particular as we here will make len = MIN(ent.len, len);
>
yes.
>> +
>> +	memcpy_fromio(data,
>> +			start_addr + sizeof(*cmd_db_header)
>> +			+ rsc_hdr.data_offset + ent.offset,
>> +			len);
>
>Why is this fromio when there is a vanilla memcpy() below?
>
memcpy should do.

>> +	return len;
>> +}
>> +EXPORT_SYMBOL(cmd_db_get_aux_data);
>> +
>> +int cmd_db_get_aux_data_len(const char *resource_id)
>
>data_len is a typical size_t, in particular as you're clamping the
>value to a positive number.
>
Makes sense. Will fix.
>> +{
>> +	int ret;
>> +	struct entry_header ent;
>> +	struct rsc_hdr rsc_hdr;
>> +
>> +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
>> +
>> +	return ret < 0 ? 0 : ent.len;
>> +}
>> +EXPORT_SYMBOL(cmd_db_get_aux_data_len);
>> +
>> +u16 cmd_db_get_version(const char *resource_id)
>> +{
>> +	int ret;
>> +	struct entry_header ent;
>> +	struct rsc_hdr rsc_hdr;
>> +
>> +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
>> +	return ret < 0 ? 0 : rsc_hdr.version;
>> +}
>> +EXPORT_SYMBOL(cmd_db_get_version);
>> +
>> +int cmd_db_ready(void)
>> +{
>> +	return cmd_db_status;
>> +}
>> +EXPORT_SYMBOL(cmd_db_ready);
>
>Move this function one step down, to keep the getters together.
>
>Based on the function name this function should return a bool.
>
Moved it up as I plan to reuse this function in
cmd_db_get_header_by_rsc_id().

>> +
>> +int cmd_db_get_slave_id(const char *resource_id)
>> +{
>> +	int ret;
>> +	struct entry_header ent;
>> +	struct rsc_hdr rsc_hdr;
>> +
>> +	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
>> +	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
>
>Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this
>rsc_hdr.slv_id?
>
BITS 16:18 indicate what type of slave it is - ARC, BCM, VRM. This
information is part of each entry.

>> +}
>> +EXPORT_SYMBOL(cmd_db_get_slave_id);
>> +
>> +static int cmd_db_dev_probe(struct platform_device *pdev)
>> +{
>> +	struct resource res;
>> +	void __iomem *dict;
>> +
>> +	dict = of_iomap(pdev->dev.of_node, 0);
>> +	if (!dict) {
>> +		cmd_db_status = -ENOMEM;
>> +		goto failed;
>> +	}
>
>Please use the idiomatic way of remapping memory in a platform driver:
>
>res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>base = ioremap_resource(&pdev->dev, res);
>if (IS_ERR(base))
>	...
>
>And I think you should just return the error code directly, rather than
>updating cmd_db_status. There isn't much benefit of letting the client
>know that command db is in "ENOMEM" state vs "EPROBE_DEFER"; neither
>case will provide a working device and it invites the client
>implementor to complicate their drivers by trying to do clever things
>depending on the result.
>
Okay
>> +
>> +	/*
>> +	 * Read start address and size of the command DB address from
>> +	 * shared dictionary location
>> +	 */
>> +	res.start = readl_relaxed(dict);
>> +	res.end = res.start + readl_relaxed(dict + 0x4);
>> +	res.flags = IORESOURCE_MEM;
>> +	iounmap(dict);
>
>Why can't we just describe the memory directly?
>
We could. Just that it is how the memory is defined in the remote end.

>> +
>> +	start_addr = devm_ioremap_resource(&pdev->dev, &res);
>
>No error handling?
>
Will fix.

>> +
>> +	cmd_db_header = devm_kzalloc(&pdev->dev,
>> +			sizeof(*cmd_db_header), GFP_KERNEL);
>> +
>> +	if (!cmd_db_header) {
>> +		cmd_db_status = -ENOMEM;
>> +		goto failed;
>> +	}
>> +
>> +	memcpy(cmd_db_header, start_addr, sizeof(*cmd_db_header));
>> +
>> +	if (cmd_db_header->magic_num != CMD_DB_MAGIC) {
>> +		pr_err("%s(): Invalid Magic\n", __func__);
>
>dev_err() and drop the __func__
>
OK.
>> +		cmd_db_status = -EINVAL;
>> +		goto failed;
>> +	}
>> +	cmd_db_status = 0;
>> +	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>
>Why? This isn't described in the binding document.
>
Not needed. Will remove.

>> +
>> +failed:
>
>This is not a good name for a label that is used for successful returns
>as well.
>
:)

>> +	return cmd_db_status;
>> +}
>> +
>> +static const struct of_device_id cmd_db_match_table[] = {
>> +	{ .compatible = "qcom,cmd-db" },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver cmd_db_dev_driver = {
>> +	.probe = cmd_db_dev_probe,
>> +	.driver = {
>> +		.name = "cmd-db",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = cmd_db_match_table,
>> +	},
>> +};
>> +
>> +int __init cmd_db_device_init(void)
>> +{
>> +	return platform_driver_register(&cmd_db_dev_driver);
>> +}
>> +arch_initcall(cmd_db_device_init);
>> diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
>[..]
>> +#ifdef CONFIG_QCOM_COMMAND_DB
>
>#if IS_ENABLED(CONFIG_QCOM_COMMAND_DB)
>
>> +/**
>> + * cmd_db_get_addr() - Query command db for resource id address.
>
>Move the kerneldoc to the implementation.
>
>> + *
>> + *  This is used to retrieve resource address based on resource
>> + *  id.
>
>The description should come after the list of parameters.
>
OK
>> + *
>> + *  @resource_id : resource id to query for address
>
>Drop the extra spaces before and after @resource_id.
>
OK
>> + *
>> + *  returns resource address on success or 0 on error otherwise
>
>Return:
>
>
>In what address space does the return value live?
>
The address is a unique identifier for the resource. Its a 32 bit value.

>> + */
>> +u32 cmd_db_get_addr(const char *resource_id);
>> +
>> +/**
>> + * cmd_db_get_aux_data() - Query command db for aux data. This is used to
>> + * retrieve a command DB entry based resource address.
>> + *
>> + *  @resource_id : Resource to retrieve AUX Data on.
>> + *  @data : Data buffer to copy returned aux data to. Returns size on NULL
>> + *  @len : Caller provides size of data buffer passed in.
>> + *
>> + *  returns size of data on success, errno on error
>> + */
>> +int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len);
>> +
>> +/**
>> + * cmd_db_get_aux_data_len - Get the length of the auxllary data stored in DB.
>> + *
>> + * @resource_id: Resource to retrieve AUX Data.
>> + *
>> + * returns size on success, errno on error
>
>No, this returns 0 on error.
>
Thanks, good catch.
>> + */
>> +int cmd_db_get_aux_data_len(const char *resource_id);
>> +
>> +/**
>> + * cmd_db_get_version - Get the version of the command DB data
>> + *
>> + * @resource_id: Resource id to query the DB for version
>> + *
>> + * returns version on success, 0 on error.
>> + *	Major number in MSB, minor number in LSB
>> + */
>> +u16 cmd_db_get_version(const char *resource_id);
>
>If the two pieces is to be used separately then pass u8 *msg, u8 *lsb to
>the function and fill these out.
>
This function doesnt seem to be used downstream. Will remove.
>> +
>> +/**
>> + * cmd_db_ready - Indicates if command DB is probed
>> + *
>> + * returns  0 on success , errno otherwise
>> + */
>> +int cmd_db_ready(void);
>> +
>> +/**
>> + * cmd_db_get_slave_id - Get the slave ID for a given resource address
>> + *
>> + * @resource_id: Resource id to query the DB for version
>> + *
>> + * return  cmd_db_hw_type enum  on success, errno on error
>
>This returns 0 on error, so you could make the return type enum
>cmd_db_hw_type to clarify the interface.
>
OK
>> + */
>> +int cmd_db_get_slave_id(const char *resource_id);
>
>Regards,
>Bjorn
Thanks for the review Bjorn.

--Lina