All of lore.kernel.org
 help / color / mirror / Atom feed
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
	alsa-devel@alsa-project.org, bgoswami@codeaurora.org,
	rohkumar@qti.qualcomm.com, linux-arm-msm@vger.kernel.org,
	plai@codeaurora.org, spatakok@qti.qualcomm.com,
	lgirdwood@gmail.com, robh+dt@kernel.org, tiwai@suse.com,
	david.brown@linaro.org, broonie@kernel.org,
	linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
	andy.gross@linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 02/24] soc: qcom: Add APR bus driver
Date: Sat, 28 Apr 2018 12:12:51 +0100	[thread overview]
Message-ID: <a5304418-1ce3-32b2-577d-0aab9160b7b0@linaro.org> (raw)
In-Reply-To: <20180428045155.GA491@tuxbook-pro>

Thanks Bjorn for the review comments.

On 28/04/18 05:51, Bjorn Andersson wrote:
> On Thu 26 Apr 02:45 PDT 2018, Srinivas Kandagatla wrote:
>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> [..]
>> +int apr_send_pkt(struct apr_device *adev, void *buf)
> 
> Sorry, but I think we have discussed this before?
> 
Yes, I did mention that I would give it a try and see, This change was 
pretty intrusive when I last looked at this.

I agree with you on asymmetry! I will change this and add struc apr_pkt 
which would apr_hdr followed by payload. This should also work for 
callback as well.

> "buf" isn't some random buffer to be sent, it is a apr_hdr followed by
> some data. As such I think you should make this type struct apr_hdr *,
> or if you think that doesn't imply there's payload make a type apr_pkt
> that has a payload[] at the end.
> 
> It will make it obvious for both future readers and the compiler what
> kind of data we're passing here.
> 
> 
> This comment also applies to functions calling functions that calls
> apr_send_pkt() as they too lug around a void *.
> 
>> +{
>> +	struct apr *apr = dev_get_drvdata(adev->dev.parent);
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&adev->lock, flags);
>> +
>> +	hdr = (struct apr_hdr *)buf;
>> +	hdr->src_domain = APR_DOMAIN_APPS;
>> +	hdr->src_svc = adev->svc_id;
>> +	hdr->dest_domain = adev->domain_id;
>> +	hdr->dest_svc = adev->svc_id;
>> +
>> +	ret = rpmsg_trysend(apr->ch, buf, hdr->pkt_size);
>> +	if (ret) {
>> +		dev_err(&adev->dev, "Unable to send APR pkt %d\n",
>> +			hdr->pkt_size);
> 
> Afaict all callers of this function will print an error message,
> sometimes on more than one level in the stack. And if some code path
> does retry sending you will get a printout for each attempt, even though
> the caller is fine with it.
> 
> I would recommend unlocking the spinlock and then do:

I can do that !!

> 
> 	return ret ? : hdr->pkt_size;
> 
>> +	} else {
>> +		ret = hdr->pkt_size;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&adev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(apr_send_pkt);
>> +

>> +
>> +static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> +				  int len, void *priv, u32 addr)
>> +{
>> +	struct apr *apr = dev_get_drvdata(&rpdev->dev);
>> +	struct apr_client_message data;
>> +	struct apr_device *p, *c_svc = NULL;
>> +	struct apr_driver *adrv = NULL;
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	uint16_t hdr_size;
>> +	uint16_t msg_type;
>> +	uint16_t ver;
>> +	uint16_t svc;
>> +
>> +	if (len <= APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
>> +			buf, len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	hdr = buf;
>> +	ver = APR_HDR_FIELD_VER(hdr->hdr_field);
>> +	if (ver > APR_PKT_VER + 1)
>> +		return -EINVAL;
>> +
>> +	hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
>> +	if (hdr_size < APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->pkt_size < APR_HDR_SIZE) {
> 
> I think it would be nice to make sure that hdr->pkt_size is < len as
> well, to reject messages that larger than the incoming buffer.
> 
> The pkt_size should be in the ballpark of len, could this check be
> changed to hdr->pkt_size != len?

Yep, It makes sense, I can add that check here.

> 
>> +		dev_err(apr->dev, "APR: Wrong paket size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
>> +	if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {
>> +		dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->src_domain >= APR_DOMAIN_MAX ||
>> +			hdr->dest_domain >= APR_DOMAIN_MAX ||
>> +			hdr->src_svc >= APR_SVC_MAX ||
>> +			hdr->dest_svc >= APR_SVC_MAX) {
>> +		dev_err(apr->dev, "APR: Wrong APR header\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	svc = hdr->dest_svc;
>> +	spin_lock_irqsave(&apr->svcs_lock, flags);
>> +	list_for_each_entry(p, &apr->svcs, node) {
> 
> Rather than doing a O(n) search for the svc with svc_id you could use a
> idr or a radix_tree to keep the id -> svc mapping.

Am not 100% sure idr is correct thing here, as the svc_ids are static 
and the list we are talking here in worst case would be max of 13 
entires, in audio case it is just 4 services.
I think using radix_tree would be over do.

> 
>> +		if (svc == p->svc_id) {
>> +			c_svc = p;
>> +			if (c_svc->dev.driver)
>> +				adrv = to_apr_driver(c_svc->dev.driver);
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&apr->svcs_lock, flags);
>> +
>> +	if (!adrv) {
>> +		dev_err(apr->dev, "APR: service is not registered\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	data.payload_size = hdr->pkt_size - hdr_size;
>> +	data.opcode = hdr->opcode;
>> +	data.src_port = hdr->src_port;
>> +	data.dest_port = hdr->dest_port;
>> +	data.token = hdr->token;
>> +	data.msg_type = msg_type;
>> +
>> +	if (data.payload_size > 0)
>> +		data.payload = buf + hdr_size;
>> +
> 
> Making a verbatim copy of parts of the hdr and then passing that to the
> APR devices creates an asymmetry in the send/callback API, without a
> whole lot of benefits. I would prefer that you introduce the apr_pkt, as
> described above, and once you have validated the size et al and found
> the service you just pass it along.
Okay, this would be a big rework, I will do it in next version.

> 
>> +	adrv->callback(c_svc, &data);
>> +
>> +	return 0;
>> +}
> [..]
>> +static int apr_uevent(struct device *dev, struct kobj_uevent_env *env)
>> +{
>> +	struct apr_device *adev = to_apr_device(dev);
>> +	int ret;
>> +
>> +	ret = of_device_uevent_modalias(dev, env);
>> +	if (ret != -ENODEV)
>> +		return ret;
>> +
>> +	return add_uevent_var(env, "MODALIAS= apr:%s", adev->name);
> 
> No space between '=' and "apr".
>
Yep.

>> +}
>> +
>> +struct bus_type aprbus = {
>> +	.name		= "aprbus",
> 
> Most busses doesn't have "bus" in the name.
> 
Yep, just "apr" make sense.

>> +	.match		= apr_device_match,
>> +	.probe		= apr_device_probe,
>> +	.uevent		= apr_uevent,
>> +	.remove		= apr_device_remove,
>> +};
>> +EXPORT_SYMBOL_GPL(aprbus);
>> +
>> +static int apr_add_device(struct device *dev, struct device_node *np,
>> +			  const struct apr_device_id *id)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct apr_device *adev = NULL;
>> +
>> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>> +	if (!adev)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&adev->lock);
>> +
>> +	adev->svc_id = id->svc_id;
>> +	adev->domain_id = id->domain_id;
>> +	adev->version = id->svc_version;
>> +	if (np)
>> +		strncpy(adev->name, np->name, APR_NAME_SIZE);
>> +	else
>> +		strncpy(adev->name, id->name, APR_NAME_SIZE);
>> +
>> +	dev_set_name(&adev->dev, "aprsvc:%s:%x:%x", adev->name,
>> +		     id->domain_id, id->svc_id);
>> +
>> +	adev->dev.bus = &aprbus;
>> +	adev->dev.parent = dev;
>> +	adev->dev.of_node = np;
>> +	adev->dev.release = apr_dev_release;
>> +	adev->dev.driver = NULL;
>> +
>> +	spin_lock(&apr->svcs_lock);
>> +	list_add_tail(&adev->node, &apr->svcs);
>> +	spin_unlock(&apr->svcs_lock);
>> +
>> +	dev_info(dev, "Adding APR dev: %s\n", dev_name(&adev->dev));
>> +
>> +	return device_register(&adev->dev);
> 
> If this fails you must call put_device(&adev->dev);
> 
Agree!!
>> +}
>> +
>> +static void of_register_apr_devices(struct device *dev)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct device_node *node;
>> +
>> +	for_each_child_of_node(dev->of_node, node) {
>> +		struct apr_device_id id = { {0} };
>> +
>> +		if (of_property_read_u32(node, "reg", &id.svc_id))
>> +			continue;
>> +
>> +		id.domain_id = apr->dest_domain_id;
>> +
>> +		if (apr_add_device(dev, node, &id))
>> +			dev_err(dev, "Failed to add arp %d svc\n", id.svc_id);
>> +	}
>> +}
> [..]
>> +
>> +static int __init apr_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = bus_register(&aprbus);
>> +	if (!ret)
>> +		ret = register_rpmsg_driver(&apr_driver);
> 
> bus_unregister() if ret here.
> 
Yep.

Thanks,
srini
>> +
>> +	return ret;
>> +}
>> +
> 
> Regards,
> Bjorn
> 

WARNING: multiple messages have this Message-ID
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: andy.gross@linaro.org, broonie@kernel.org,
	linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org,
	robh+dt@kernel.org, bgoswami@codeaurora.org,
	gregkh@linuxfoundation.org, david.brown@linaro.org,
	mark.rutland@arm.com, lgirdwood@gmail.com, plai@codeaurora.org,
	tiwai@suse.com, perex@perex.cz, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, rohkumar@qti.qualcomm.com,
	spatakok@qti.qualcomm.com
Subject: Re: [PATCH v6 02/24] soc: qcom: Add APR bus driver
Date: Sat, 28 Apr 2018 12:12:51 +0100	[thread overview]
Message-ID: <a5304418-1ce3-32b2-577d-0aab9160b7b0@linaro.org> (raw)
In-Reply-To: <20180428045155.GA491@tuxbook-pro>

Thanks Bjorn for the review comments.

On 28/04/18 05:51, Bjorn Andersson wrote:
> On Thu 26 Apr 02:45 PDT 2018, Srinivas Kandagatla wrote:
>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> [..]
>> +int apr_send_pkt(struct apr_device *adev, void *buf)
> 
> Sorry, but I think we have discussed this before?
> 
Yes, I did mention that I would give it a try and see, This change was 
pretty intrusive when I last looked at this.

I agree with you on asymmetry! I will change this and add struc apr_pkt 
which would apr_hdr followed by payload. This should also work for 
callback as well.

> "buf" isn't some random buffer to be sent, it is a apr_hdr followed by
> some data. As such I think you should make this type struct apr_hdr *,
> or if you think that doesn't imply there's payload make a type apr_pkt
> that has a payload[] at the end.
> 
> It will make it obvious for both future readers and the compiler what
> kind of data we're passing here.
> 
> 
> This comment also applies to functions calling functions that calls
> apr_send_pkt() as they too lug around a void *.
> 
>> +{
>> +	struct apr *apr = dev_get_drvdata(adev->dev.parent);
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&adev->lock, flags);
>> +
>> +	hdr = (struct apr_hdr *)buf;
>> +	hdr->src_domain = APR_DOMAIN_APPS;
>> +	hdr->src_svc = adev->svc_id;
>> +	hdr->dest_domain = adev->domain_id;
>> +	hdr->dest_svc = adev->svc_id;
>> +
>> +	ret = rpmsg_trysend(apr->ch, buf, hdr->pkt_size);
>> +	if (ret) {
>> +		dev_err(&adev->dev, "Unable to send APR pkt %d\n",
>> +			hdr->pkt_size);
> 
> Afaict all callers of this function will print an error message,
> sometimes on more than one level in the stack. And if some code path
> does retry sending you will get a printout for each attempt, even though
> the caller is fine with it.
> 
> I would recommend unlocking the spinlock and then do:

I can do that !!

> 
> 	return ret ? : hdr->pkt_size;
> 
>> +	} else {
>> +		ret = hdr->pkt_size;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&adev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(apr_send_pkt);
>> +

>> +
>> +static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> +				  int len, void *priv, u32 addr)
>> +{
>> +	struct apr *apr = dev_get_drvdata(&rpdev->dev);
>> +	struct apr_client_message data;
>> +	struct apr_device *p, *c_svc = NULL;
>> +	struct apr_driver *adrv = NULL;
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	uint16_t hdr_size;
>> +	uint16_t msg_type;
>> +	uint16_t ver;
>> +	uint16_t svc;
>> +
>> +	if (len <= APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
>> +			buf, len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	hdr = buf;
>> +	ver = APR_HDR_FIELD_VER(hdr->hdr_field);
>> +	if (ver > APR_PKT_VER + 1)
>> +		return -EINVAL;
>> +
>> +	hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
>> +	if (hdr_size < APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->pkt_size < APR_HDR_SIZE) {
> 
> I think it would be nice to make sure that hdr->pkt_size is < len as
> well, to reject messages that larger than the incoming buffer.
> 
> The pkt_size should be in the ballpark of len, could this check be
> changed to hdr->pkt_size != len?

Yep, It makes sense, I can add that check here.

> 
>> +		dev_err(apr->dev, "APR: Wrong paket size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
>> +	if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {
>> +		dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->src_domain >= APR_DOMAIN_MAX ||
>> +			hdr->dest_domain >= APR_DOMAIN_MAX ||
>> +			hdr->src_svc >= APR_SVC_MAX ||
>> +			hdr->dest_svc >= APR_SVC_MAX) {
>> +		dev_err(apr->dev, "APR: Wrong APR header\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	svc = hdr->dest_svc;
>> +	spin_lock_irqsave(&apr->svcs_lock, flags);
>> +	list_for_each_entry(p, &apr->svcs, node) {
> 
> Rather than doing a O(n) search for the svc with svc_id you could use a
> idr or a radix_tree to keep the id -> svc mapping.

Am not 100% sure idr is correct thing here, as the svc_ids are static 
and the list we are talking here in worst case would be max of 13 
entires, in audio case it is just 4 services.
I think using radix_tree would be over do.

> 
>> +		if (svc == p->svc_id) {
>> +			c_svc = p;
>> +			if (c_svc->dev.driver)
>> +				adrv = to_apr_driver(c_svc->dev.driver);
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&apr->svcs_lock, flags);
>> +
>> +	if (!adrv) {
>> +		dev_err(apr->dev, "APR: service is not registered\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	data.payload_size = hdr->pkt_size - hdr_size;
>> +	data.opcode = hdr->opcode;
>> +	data.src_port = hdr->src_port;
>> +	data.dest_port = hdr->dest_port;
>> +	data.token = hdr->token;
>> +	data.msg_type = msg_type;
>> +
>> +	if (data.payload_size > 0)
>> +		data.payload = buf + hdr_size;
>> +
> 
> Making a verbatim copy of parts of the hdr and then passing that to the
> APR devices creates an asymmetry in the send/callback API, without a
> whole lot of benefits. I would prefer that you introduce the apr_pkt, as
> described above, and once you have validated the size et al and found
> the service you just pass it along.
Okay, this would be a big rework, I will do it in next version.

> 
>> +	adrv->callback(c_svc, &data);
>> +
>> +	return 0;
>> +}
> [..]
>> +static int apr_uevent(struct device *dev, struct kobj_uevent_env *env)
>> +{
>> +	struct apr_device *adev = to_apr_device(dev);
>> +	int ret;
>> +
>> +	ret = of_device_uevent_modalias(dev, env);
>> +	if (ret != -ENODEV)
>> +		return ret;
>> +
>> +	return add_uevent_var(env, "MODALIAS= apr:%s", adev->name);
> 
> No space between '=' and "apr".
>
Yep.

>> +}
>> +
>> +struct bus_type aprbus = {
>> +	.name		= "aprbus",
> 
> Most busses doesn't have "bus" in the name.
> 
Yep, just "apr" make sense.

>> +	.match		= apr_device_match,
>> +	.probe		= apr_device_probe,
>> +	.uevent		= apr_uevent,
>> +	.remove		= apr_device_remove,
>> +};
>> +EXPORT_SYMBOL_GPL(aprbus);
>> +
>> +static int apr_add_device(struct device *dev, struct device_node *np,
>> +			  const struct apr_device_id *id)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct apr_device *adev = NULL;
>> +
>> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>> +	if (!adev)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&adev->lock);
>> +
>> +	adev->svc_id = id->svc_id;
>> +	adev->domain_id = id->domain_id;
>> +	adev->version = id->svc_version;
>> +	if (np)
>> +		strncpy(adev->name, np->name, APR_NAME_SIZE);
>> +	else
>> +		strncpy(adev->name, id->name, APR_NAME_SIZE);
>> +
>> +	dev_set_name(&adev->dev, "aprsvc:%s:%x:%x", adev->name,
>> +		     id->domain_id, id->svc_id);
>> +
>> +	adev->dev.bus = &aprbus;
>> +	adev->dev.parent = dev;
>> +	adev->dev.of_node = np;
>> +	adev->dev.release = apr_dev_release;
>> +	adev->dev.driver = NULL;
>> +
>> +	spin_lock(&apr->svcs_lock);
>> +	list_add_tail(&adev->node, &apr->svcs);
>> +	spin_unlock(&apr->svcs_lock);
>> +
>> +	dev_info(dev, "Adding APR dev: %s\n", dev_name(&adev->dev));
>> +
>> +	return device_register(&adev->dev);
> 
> If this fails you must call put_device(&adev->dev);
> 
Agree!!
>> +}
>> +
>> +static void of_register_apr_devices(struct device *dev)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct device_node *node;
>> +
>> +	for_each_child_of_node(dev->of_node, node) {
>> +		struct apr_device_id id = { {0} };
>> +
>> +		if (of_property_read_u32(node, "reg", &id.svc_id))
>> +			continue;
>> +
>> +		id.domain_id = apr->dest_domain_id;
>> +
>> +		if (apr_add_device(dev, node, &id))
>> +			dev_err(dev, "Failed to add arp %d svc\n", id.svc_id);
>> +	}
>> +}
> [..]
>> +
>> +static int __init apr_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = bus_register(&aprbus);
>> +	if (!ret)
>> +		ret = register_rpmsg_driver(&apr_driver);
> 
> bus_unregister() if ret here.
> 
Yep.

Thanks,
srini
>> +
>> +	return ret;
>> +}
>> +
> 
> Regards,
> Bjorn
> 

WARNING: multiple messages have this Message-ID
From: srinivas.kandagatla@linaro.org (Srinivas Kandagatla)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v6 02/24] soc: qcom: Add APR bus driver
Date: Sat, 28 Apr 2018 12:12:51 +0100	[thread overview]
Message-ID: <a5304418-1ce3-32b2-577d-0aab9160b7b0@linaro.org> (raw)
In-Reply-To: <20180428045155.GA491@tuxbook-pro>

Thanks Bjorn for the review comments.

On 28/04/18 05:51, Bjorn Andersson wrote:
> On Thu 26 Apr 02:45 PDT 2018, Srinivas Kandagatla wrote:
>> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> [..]
>> +int apr_send_pkt(struct apr_device *adev, void *buf)
> 
> Sorry, but I think we have discussed this before?
> 
Yes, I did mention that I would give it a try and see, This change was 
pretty intrusive when I last looked at this.

I agree with you on asymmetry! I will change this and add struc apr_pkt 
which would apr_hdr followed by payload. This should also work for 
callback as well.

> "buf" isn't some random buffer to be sent, it is a apr_hdr followed by
> some data. As such I think you should make this type struct apr_hdr *,
> or if you think that doesn't imply there's payload make a type apr_pkt
> that has a payload[] at the end.
> 
> It will make it obvious for both future readers and the compiler what
> kind of data we're passing here.
> 
> 
> This comment also applies to functions calling functions that calls
> apr_send_pkt() as they too lug around a void *.
> 
>> +{
>> +	struct apr *apr = dev_get_drvdata(adev->dev.parent);
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&adev->lock, flags);
>> +
>> +	hdr = (struct apr_hdr *)buf;
>> +	hdr->src_domain = APR_DOMAIN_APPS;
>> +	hdr->src_svc = adev->svc_id;
>> +	hdr->dest_domain = adev->domain_id;
>> +	hdr->dest_svc = adev->svc_id;
>> +
>> +	ret = rpmsg_trysend(apr->ch, buf, hdr->pkt_size);
>> +	if (ret) {
>> +		dev_err(&adev->dev, "Unable to send APR pkt %d\n",
>> +			hdr->pkt_size);
> 
> Afaict all callers of this function will print an error message,
> sometimes on more than one level in the stack. And if some code path
> does retry sending you will get a printout for each attempt, even though
> the caller is fine with it.
> 
> I would recommend unlocking the spinlock and then do:

I can do that !!

> 
> 	return ret ? : hdr->pkt_size;
> 
>> +	} else {
>> +		ret = hdr->pkt_size;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&adev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(apr_send_pkt);
>> +

>> +
>> +static int apr_callback(struct rpmsg_device *rpdev, void *buf,
>> +				  int len, void *priv, u32 addr)
>> +{
>> +	struct apr *apr = dev_get_drvdata(&rpdev->dev);
>> +	struct apr_client_message data;
>> +	struct apr_device *p, *c_svc = NULL;
>> +	struct apr_driver *adrv = NULL;
>> +	struct apr_hdr *hdr;
>> +	unsigned long flags;
>> +	uint16_t hdr_size;
>> +	uint16_t msg_type;
>> +	uint16_t ver;
>> +	uint16_t svc;
>> +
>> +	if (len <= APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
>> +			buf, len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	hdr = buf;
>> +	ver = APR_HDR_FIELD_VER(hdr->hdr_field);
>> +	if (ver > APR_PKT_VER + 1)
>> +		return -EINVAL;
>> +
>> +	hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
>> +	if (hdr_size < APR_HDR_SIZE) {
>> +		dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->pkt_size < APR_HDR_SIZE) {
> 
> I think it would be nice to make sure that hdr->pkt_size is < len as
> well, to reject messages that larger than the incoming buffer.
> 
> The pkt_size should be in the ballpark of len, could this check be
> changed to hdr->pkt_size != len?

Yep, It makes sense, I can add that check here.

> 
>> +		dev_err(apr->dev, "APR: Wrong paket size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
>> +	if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {
>> +		dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (hdr->src_domain >= APR_DOMAIN_MAX ||
>> +			hdr->dest_domain >= APR_DOMAIN_MAX ||
>> +			hdr->src_svc >= APR_SVC_MAX ||
>> +			hdr->dest_svc >= APR_SVC_MAX) {
>> +		dev_err(apr->dev, "APR: Wrong APR header\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	svc = hdr->dest_svc;
>> +	spin_lock_irqsave(&apr->svcs_lock, flags);
>> +	list_for_each_entry(p, &apr->svcs, node) {
> 
> Rather than doing a O(n) search for the svc with svc_id you could use a
> idr or a radix_tree to keep the id -> svc mapping.

Am not 100% sure idr is correct thing here, as the svc_ids are static 
and the list we are talking here in worst case would be max of 13 
entires, in audio case it is just 4 services.
I think using radix_tree would be over do.

> 
>> +		if (svc == p->svc_id) {
>> +			c_svc = p;
>> +			if (c_svc->dev.driver)
>> +				adrv = to_apr_driver(c_svc->dev.driver);
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&apr->svcs_lock, flags);
>> +
>> +	if (!adrv) {
>> +		dev_err(apr->dev, "APR: service is not registered\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	data.payload_size = hdr->pkt_size - hdr_size;
>> +	data.opcode = hdr->opcode;
>> +	data.src_port = hdr->src_port;
>> +	data.dest_port = hdr->dest_port;
>> +	data.token = hdr->token;
>> +	data.msg_type = msg_type;
>> +
>> +	if (data.payload_size > 0)
>> +		data.payload = buf + hdr_size;
>> +
> 
> Making a verbatim copy of parts of the hdr and then passing that to the
> APR devices creates an asymmetry in the send/callback API, without a
> whole lot of benefits. I would prefer that you introduce the apr_pkt, as
> described above, and once you have validated the size et al and found
> the service you just pass it along.
Okay, this would be a big rework, I will do it in next version.

> 
>> +	adrv->callback(c_svc, &data);
>> +
>> +	return 0;
>> +}
> [..]
>> +static int apr_uevent(struct device *dev, struct kobj_uevent_env *env)
>> +{
>> +	struct apr_device *adev = to_apr_device(dev);
>> +	int ret;
>> +
>> +	ret = of_device_uevent_modalias(dev, env);
>> +	if (ret != -ENODEV)
>> +		return ret;
>> +
>> +	return add_uevent_var(env, "MODALIAS= apr:%s", adev->name);
> 
> No space between '=' and "apr".
>
Yep.

>> +}
>> +
>> +struct bus_type aprbus = {
>> +	.name		= "aprbus",
> 
> Most busses doesn't have "bus" in the name.
> 
Yep, just "apr" make sense.

>> +	.match		= apr_device_match,
>> +	.probe		= apr_device_probe,
>> +	.uevent		= apr_uevent,
>> +	.remove		= apr_device_remove,
>> +};
>> +EXPORT_SYMBOL_GPL(aprbus);
>> +
>> +static int apr_add_device(struct device *dev, struct device_node *np,
>> +			  const struct apr_device_id *id)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct apr_device *adev = NULL;
>> +
>> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>> +	if (!adev)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_init(&adev->lock);
>> +
>> +	adev->svc_id = id->svc_id;
>> +	adev->domain_id = id->domain_id;
>> +	adev->version = id->svc_version;
>> +	if (np)
>> +		strncpy(adev->name, np->name, APR_NAME_SIZE);
>> +	else
>> +		strncpy(adev->name, id->name, APR_NAME_SIZE);
>> +
>> +	dev_set_name(&adev->dev, "aprsvc:%s:%x:%x", adev->name,
>> +		     id->domain_id, id->svc_id);
>> +
>> +	adev->dev.bus = &aprbus;
>> +	adev->dev.parent = dev;
>> +	adev->dev.of_node = np;
>> +	adev->dev.release = apr_dev_release;
>> +	adev->dev.driver = NULL;
>> +
>> +	spin_lock(&apr->svcs_lock);
>> +	list_add_tail(&adev->node, &apr->svcs);
>> +	spin_unlock(&apr->svcs_lock);
>> +
>> +	dev_info(dev, "Adding APR dev: %s\n", dev_name(&adev->dev));
>> +
>> +	return device_register(&adev->dev);
> 
> If this fails you must call put_device(&adev->dev);
> 
Agree!!
>> +}
>> +
>> +static void of_register_apr_devices(struct device *dev)
>> +{
>> +	struct apr *apr = dev_get_drvdata(dev);
>> +	struct device_node *node;
>> +
>> +	for_each_child_of_node(dev->of_node, node) {
>> +		struct apr_device_id id = { {0} };
>> +
>> +		if (of_property_read_u32(node, "reg", &id.svc_id))
>> +			continue;
>> +
>> +		id.domain_id = apr->dest_domain_id;
>> +
>> +		if (apr_add_device(dev, node, &id))
>> +			dev_err(dev, "Failed to add arp %d svc\n", id.svc_id);
>> +	}
>> +}
> [..]
>> +
>> +static int __init apr_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = bus_register(&aprbus);
>> +	if (!ret)
>> +		ret = register_rpmsg_driver(&apr_driver);
> 
> bus_unregister() if ret here.
> 
Yep.

Thanks,
srini
>> +
>> +	return ret;
>> +}
>> +
> 
> Regards,
> Bjorn
> 

  reply	other threads:[~2018-04-28 11:12 UTC|newest]

Thread overview: 115+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26  9:45 [PATCH v6 00/24] ASoC: qcom: Add support to QDSP based Audio srinivas.kandagatla
2018-04-26  9:45 ` srinivas.kandagatla at linaro.org
2018-04-26  9:45 ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 01/24] soc: qcom dt-bindings: Add APR bus bindings srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-27 14:07   ` Rob Herring
2018-04-27 14:07     ` Rob Herring
2018-04-27 14:07     ` Rob Herring
2018-05-11  3:19   ` Applied "soc: qcom dt-bindings: Add APR bus bindings" to the asoc tree Mark Brown
2018-05-11  3:19     ` Mark Brown
2018-05-11  3:19     ` Mark Brown
2018-04-26  9:45 ` [PATCH v6 02/24] soc: qcom: Add APR bus driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26 11:39   ` Mark Brown
2018-04-26 11:39     ` Mark Brown
2018-04-26 11:39     ` Mark Brown
2018-04-26 12:05     ` Srinivas Kandagatla
2018-04-26 12:05       ` Srinivas Kandagatla
2018-04-26 12:05       ` Srinivas Kandagatla
2018-04-26 21:17     ` Andy Gross
2018-04-26 21:17       ` Andy Gross
2018-04-27 11:06       ` Mark Brown
2018-04-27 11:06         ` Mark Brown
2018-04-27 11:06         ` Mark Brown
2018-04-26 21:16   ` Andy Gross
2018-04-26 21:16     ` Andy Gross
2018-04-28  4:51   ` Bjorn Andersson
2018-04-28  4:51     ` Bjorn Andersson
2018-04-28 11:12     ` Srinivas Kandagatla [this message]
2018-04-28 11:12       ` Srinivas Kandagatla
2018-04-28 11:12       ` Srinivas Kandagatla
2018-04-26  9:45 ` [PATCH v6 03/24] ASoC: qdsp6: dt-bindings: Add q6core dt bindings srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-27 14:10   ` Rob Herring
2018-04-27 14:10     ` Rob Herring
2018-04-27 14:10     ` Rob Herring
2018-04-26  9:45 ` [PATCH v6 04/24] ASoC: qdsp6: dt-bindings: Add q6afe " srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-27 14:13   ` Rob Herring
2018-04-27 14:13     ` Rob Herring
2018-04-27 14:13     ` Rob Herring
2018-04-27 14:58     ` Srinivas Kandagatla
2018-04-27 14:58       ` Srinivas Kandagatla
2018-04-27 18:32       ` Rob Herring
2018-04-27 18:32         ` Rob Herring
2018-04-27 19:16         ` Srinivas Kandagatla
2018-04-27 19:16           ` Srinivas Kandagatla
2018-04-26  9:45 ` [PATCH v6 05/24] ASoC: qdsp6: dt-bindings: Add q6adm " srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-27 14:14   ` Rob Herring
2018-04-27 14:14     ` Rob Herring
2018-05-11  3:17   ` Applied "ASoC: qdsp6: dt-bindings: Add q6adm dt bindings" to the asoc tree Mark Brown
2018-05-11  3:17     ` Mark Brown
2018-05-11  3:17     ` Mark Brown
2018-04-26  9:45 ` [PATCH v6 06/24] ASoC: qdsp6: dt-bindings: Add q6asm dt bindings srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-27 14:17   ` Rob Herring
2018-04-27 14:17     ` Rob Herring
2018-04-27 14:17     ` Rob Herring
2018-05-11  3:16   ` Applied "ASoC: qdsp6: dt-bindings: Add q6asm dt bindings" to the asoc tree Mark Brown
2018-05-11  3:16     ` Mark Brown
2018-05-11  3:16     ` Mark Brown
2018-04-26  9:45 ` [PATCH v6 07/24] ASoC: qdsp6: q6common: Add qdsp6 helper functions srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 08/24] ASoC: qdsp6: q6core: Add q6core driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 09/24] ASoC: qdsp6: q6afe: Add q6afe driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45 ` [PATCH v6 10/24] ASoC: qdsp6: qdafe: Add SLIMBus port Support srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45 ` [PATCH v6 11/24] ASoC: qdsp6: q6afe: Add support to MI2S ports srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45 ` [PATCH v6 12/24] ASoC: qdsp6: q6afe: Add support to MI2S sysclks srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 13/24] ASoC: qdsp6: q6adm: Add q6adm driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 14/24] ASoC: qdsp6: q6asm: Add q6asm driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 15/24] ASoC: qdsp6: q6asm: Add support to memory map and unmap srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 16/24] ASoC: qdsp6: q6asm: Add support to audio stream apis srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:45   ` srinivas.kandagatla
2018-04-26  9:45 ` [PATCH v6 17/24] ASoC: qdsp6: q6routing: Add q6routing driver srinivas.kandagatla
2018-04-26  9:45   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 18/24] ASoC: qdsp6: q6routing: Add support to all SLIMBus Mixers srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 19/24] ASoC: qdsp6: q6routing: Add support to MI2S Mixers srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 20/24] ASoC: qdsp6: q6afe: Add q6afe dai driver srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 21/24] ASoC: qdsp6: q6asm: Add q6asm " srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 22/24] ASoC: qdsp6: dt-bindings: Add apq8096 machine bindings srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-27 14:18   ` Rob Herring
2018-04-27 14:18     ` Rob Herring
2018-04-27 14:18     ` Rob Herring
2018-05-21 15:47   ` Applied "ASoC: qdsp6: dt-bindings: Add apq8096 machine bindings" to the asoc tree Mark Brown
2018-05-21 15:47     ` Mark Brown
2018-05-21 15:47     ` Mark Brown
2018-04-26  9:46 ` [PATCH v6 23/24] ASoC: qcom: apq8096: Add db820c machine driver srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org
2018-04-26  9:46 ` [PATCH v6 24/24] MAINTAINERS: Add myself as co-maintainer of qcom audio srinivas.kandagatla
2018-04-26  9:46   ` srinivas.kandagatla at linaro.org

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a5304418-1ce3-32b2-577d-0aab9160b7b0@linaro.org \
    --to=srinivas.kandagatla@linaro.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=andy.gross@linaro.org \
    --cc=bgoswami@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=broonie@kernel.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=plai@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=rohkumar@qti.qualcomm.com \
    --cc=spatakok@qti.qualcomm.com \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v6 02/24] soc: qcom: Add APR bus driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.