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 netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
