Date:	Thu, 1 Oct 2015 13:33:38 -0700
From:	Bjorn Andersson <bjorn.andersson@...ymobile.com>
To:	Marcel Holtmann <marcel@...tmann.org>
CC:	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Fengwei Yin <fengwei.yin@...aro.org>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-bluetooth <linux-bluetooth@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

On Wed 30 Sep 23:54 PDT 2015, Marcel Holtmann wrote:

> Hi Bjorn,
> 
> > The Qualcomm WCNSS chip provides two SMD channels to the BT core; one
> > for command and one for event packets. This driver exposes the two
> > channels as a hci device.
> > 
[..]
> > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> > index 07c9cf381e5a..43c7dc8641ff 100644
> > --- a/drivers/bluetooth/Makefile
> > +++ b/drivers/bluetooth/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART)	+= btuart_cs.o
> > 
> > obj-$(CONFIG_BT_HCIBTUSB)	+= btusb.o
> > obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
> > +obj-$(CONFIG_BT_HCISMD)		+= hci_smd.o
> 
> I wonder if choosing a name like btqcomsmd.ko would not be better
> here. For now I am fine with keeping hci_smd.ko since there are other
> issues here to be fixed first.

I had some problems figuring out the naming scheme here, but btqcomsmd
does seem reasonable, I'll update it in the next set.

> Especially the kbuild test robot complained loudly.
> 

Sorry, I forgot to mention in the patch that the patch depends on the
qcom_smd_id patch - that's available in linux-next. I will take another
look, but I think that was the cause of all the issues.

> > obj-$(CONFIG_BT_INTEL)		+= btintel.o
> > obj-$(CONFIG_BT_ATH3K)		+= ath3k.o
> > diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c
[..]
> > +#include <net/bluetooth/hci.h>
> 
> The hci.h include is not needed. If you do, then you are doing
> something fishy.
> 

Nothing fishy going on here, so I'll drop it.

> > +
> > +static struct {
> > +	struct qcom_smd_channel *acl_channel;
> > +	struct qcom_smd_channel *cmd_channel;
> > +
> > +	struct hci_dev *hci;
> > +} smd_hci;
> 
> A driver that only supports a single instance of a device and uses a
> global variable to do so is not a good idea. There should be no reason
> for this. Why is this done this way.
> 

There's two channels coming up, each probing the associated driver that
combines these into 1 hci device.

So I have two options;

* the original idea was to implement multi-channel-per-device support in
  SMD, but as this is the only known case where that's needed I really
  don't want to add all the extra complexity to SMD.

* I can accept the fact that this is silicon inside the MSM SoC and
  should never exist in more than one instance and make this hack. The
  first version I implemented had this structure allocated on the first
  probe, but that didn't really add any value to the static struct.

I picked the second option due to the fact that the patch to SMD was way
bigger then this patch, and full of nasty logic.

> > +
> > +static int smd_hci_recv(unsigned type, const void *data, size_t count)
> > +{
> > +	struct sk_buff *skb;
> > +	void *buf;
> > +	int ret;
> > +
> > +	skb = bt_skb_alloc(count, GFP_ATOMIC);
> 
> Is it required that this is GFP_ATOMIC or can this actually be
> GFP_KERNEL?
> 

This is being called from IRQ context upon receiving data on the
channels.

> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	buf = skb_put(skb, count);
> > +	memcpy_fromio(buf, data, count);
> 
> Why is this a memcpy_fromio here?
> 

A memcpy here doesn't seem to work on ARM64, probably because the
pointer references ioremapped memory. We are discussing either marking
the data __iomem or perhaps using memremap rather then ioremap.

> > +
> > +	skb->dev = (void *)smd_hci.hci;
> 
> This is no longer needed.
> 

Thanks

> > +	bt_cb(skb)->pkt_type = type;
> > +	skb_orphan(skb);
> > +
> > +	ret = hci_recv_frame(smd_hci.hci, skb);
> > +	if (ret < 0)
> > +		kfree_skb(skb);
> 
> This is a double kfree_skb here. The hci_recv_frame will consume the
> skb no matter what.
> 

Thanks

> > +
> > +	return ret;
> > +}
> > +
[..]
> > +
> > +static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	int ret;
> > +
> > +	switch (bt_cb(skb)->pkt_type) {
> > +	case HCI_ACLDATA_PKT:
> > +	case HCI_SCODATA_PKT:
> > +		ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len);
> > +		break;
> > +	case HCI_COMMAND_PKT:
> > +		ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len);
> > +		break;
> > +	default:
> > +		ret = -ENODEV;
> > +		break;
> > +	}
> > +
> > +	kfree_skb(skb);

> 
> If you return an error, then the caller of hdev->send will free the
> skb.
> 

Must have looked in the wrong kernel tree :(

Thanks.

> > +
> > +	return ret;
> > +}
> > +
> > +static int smd_hci_open(struct hci_dev *hci)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int smd_hci_close(struct hci_dev *hci)
> > +{
> > +	return 0;
> > +}
> 
> These two need to at least handling the HCI_RUNNING flag setting and
> clearing like all the other drivers do.
> 

I'm puzzled to the purpose of this, is this only for indication to user
space or am I missing something?

I found a couple of others that does nothing but set and clear this bit
in their open and close. Would you be open for a framework patch that
provides this functionality to drivers that doesn't need anything else?

> > +
> > +static int smd_hci_set_bdaddr(struct hci_dev *hci,
> > +			      const bdaddr_t *bdaddr)
> > +{
> > +	u8 buf[12];
> > +
> > +	buf[0] = 0x0b;
> > +	buf[1] = 0xfc;
> > +	buf[2] = 0x9;
> > +	buf[3] = 0x1;
> > +	buf[4] = 0x2;
> 
> Use full 0x00 syntax here.
> 

Ok

> > +	buf[5] = sizeof(bdaddr_t);
> > +	memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));
> 
> We also need to be sure that this command only changes the BD_ADDR in
> RAM. It can not be persistent. When resetting / power cycle the
> controller it will fallback to its default.
> 

There seems to be no persistent storage at all in this chip, the BD_ADDR
being used comes from one of the firmware files, so this just adjust the
in-memory configuration.

> > +
> > +	return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));
> 
> Use __hci_cmd_sync since that is what is suppose to be used in this
> callback. See the other drivers on how this is done.
> 

I did see this, and I first implemented it like that. Unfortunately the
firmware does not ack the command and __hci_cmd_sync() just times out.

I didn't manage to find a way around this.

> I wonder if qca_set_bdaddr_rome is not actually the same command and
> you could just use that instead.
> 

Looks to be exactly the same thing and gives insight in what those first
5 bytes are. I'll update the magics based on this information.

If I didn't have the issue of the timeout it seems like I should be able
to just call it.

> > +}
> > +
> > +static int smd_hci_register(void)
> > +{
> > +	struct hci_dev *hci;
> > +	int ret;
> > +
> > +	if (smd_hci.hci)
> > +		return 0;
> > +
> > +	/* Wait for both channels to probe before registering */
> > +	if (!smd_hci.acl_channel || !smd_hci.cmd_channel)
> > +		return 0;
> > +
> > +	hci = hci_alloc_dev();
> > +	if (!hci)
> > +		return -ENOMEM;
> > +
> > +	hci->bus = HCI_SMD;
> > +	hci->open = smd_hci_open;
> > +	hci->close = smd_hci_close;
> > +	hci->send = smd_hci_send;
> > +	hci->set_bdaddr = smd_hci_set_bdaddr;
> > +
> > +	ret = hci_register_dev(hci);
> > +	if (ret < 0) {
> > +		hci_free_dev(hci);
> > +		return ret;
> > +	}
> > +
> > +	smd_hci.hci = hci;
> > +
> > +	return 0;
> > +}
> > +
> > +static void smd_hci_unregister(void)
> > +{
> > +	/* Only unregister on the first remove call */
> > +	if (!smd_hci.hci)
> > +		return;
> > +
> > +	hci_unregister_dev(smd_hci.hci);
> > +	hci_free_dev(smd_hci.hci);
> > +	smd_hci.hci = NULL;
> > +}
> > +
> > +static int smd_hci_acl_probe(struct qcom_smd_device *sdev)
> > +{
> > +	smd_hci.acl_channel = sdev->channel;
> > +	smd_hci_register();
> > +
> > +	return 0;
> > +}
> > +
> > +static int smd_hci_cmd_probe(struct qcom_smd_device *sdev)
> > +{
> > +	smd_hci.cmd_channel = sdev->channel;
> > +	smd_hci_register();
> > +
> > +	return 0;
> > +}
> > +
> > +static void smd_hci_acl_remove(struct qcom_smd_device *sdev)
> > +{
> > +	smd_hci.acl_channel = NULL;
> > +	smd_hci_unregister();
> > +}
> > +
> > +static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
> > +{
> > +	smd_hci.cmd_channel = NULL;
> > +	smd_hci_unregister();
> > +}
> > +
> > +static const struct qcom_smd_id smd_hci_acl_match[] = {
> > +	{ .name = "APPS_RIVA_BT_ACL" },
> > +	{}
> > +};
> > +
> > +static const struct qcom_smd_id smd_hci_cmd_match[] = {
> > +	{ .name = "APPS_RIVA_BT_CMD" },
> > +	{}
> > +};
> > +
> > +static struct qcom_smd_driver smd_hci_acl_driver = {
> > +	.probe = smd_hci_acl_probe,
> > +	.remove = smd_hci_acl_remove,
> > +	.callback = smd_hci_acl_callback,
> > +	.smd_match_table = smd_hci_acl_match,
> > +	.driver  = {
> > +		.name  = "qcom_smd_hci_acl",
> > +		.owner = THIS_MODULE,
> > +	},
> > +};
> > +
> > +static struct qcom_smd_driver smd_hci_cmd_driver = {
> > +	.probe = smd_hci_cmd_probe,
> > +	.remove = smd_hci_cmd_remove,
> > +	.callback = smd_hci_cmd_callback,
> > +	.smd_match_table = smd_hci_cmd_match,
> > +	.driver  = {
> > +		.name  = "qcom_smd_hci_cmd",
> > +		.owner = THIS_MODULE,
> > +	},
> > +};
> > +
> > +module_qcom_smd_driver(smd_hci_acl_driver);
> > +module_qcom_smd_driver(smd_hci_cmd_driver);

> 
> This is not going to fly. This turns it into two module_{init,exit}
> calls. That really does not work this way.
> 

Right, I would need to at least fill in my own init and exit functions
with the two registers/unregisters calls...

> I mean why do you need two different drivers in the first place. I
> should be matching via a single qcom_smd_id in the first place. And
> you handle everything else in the probe() callback.
> 

The first iteration had 1 probe, 1 remove and 1 callback. I pulled the
match-data in probe and made the rest use this information by drvdata.
It was the same blocks of code as now, but accessed via the wrapper
drvdata object - so although one shouldn't do like this, this is much
cleaner.

> For me this is similar to the USB case where you actually have to
> claim to interfaces. We just lookup the other interface and claim it.
> It looks like SMD core functionality needs the same feature.
> 

Indeed, as I said above the plan was to implement something where I
either only probe when all channels where up (turned out to be a mess to
implement) or have some accessor function that can be called from probe
to open a second channel.

The latter seems reasonable to do, but it will be way more code than
there's in this driver - which seems to be the only case where that
feature would be needed.

> > +
> > +MODULE_DESCRIPTION("Qualcomm SMD HCI driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 7ca6690355ea..ee5b2dd922f6 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -58,6 +58,7 @@
> > #define HCI_RS232	4
> > #define HCI_PCI		5
> > #define HCI_SDIO	6
> > +#define HCI_SMD		7
> 
> I would prefer if this come in a separate patch from the driver.
> 

Sure thing.


Thanks for your review Marcel.

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

Powered by blists - more mailing lists