Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[Bjorn Andersson]

On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote:

> Hi Bjorn,
> 
[..]
> >> 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.
> 
> which means, I can only merge this driver when this other patch has
> hit net-next tree. Unless I take the qcom_smd_id through
> bluetooth-next tree which doesn't look like a good idea.
> 

I was going to suggest that we just wait for -rc1, but the new additions
to SMD adds a bunch of new items on the dependency list.

Perhaps once we agree we can get your Ack and have Andy pull it through
the qcom-soc tree together with the SMD patches?

> >>> 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 
> >>
> >> 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.
> 
> Writing a driver that assume it is a single instance of a given device
> is never a good idea. While you might find some instances in the
> kernel if you look hard enough, but that doesn't mean we should keep
> doing this. So this needs addressing.
> 

Fair enough.

> >
> >>> +
> >>> +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.
> 
> I wonder why that is needed, but seems an issue in the SMD subsystem.
> So this is fine, might want to add a comment above the bt_skb_alloc to
> remind us.
> 

It's a design choice of mine, to deliver the incoming data directly from
the interrupt handler. Sure enough we could have had a worker thread or
something do it.

I'll add a comment.

> >
> >>> +   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.
> 
> I would add a comment here as well to remind everybody why this is
> used. And marking up the pointers is a good idea as well.
> 

Yeah, I have this on the todo.
Will put a comment here for now.

[..]

> >>> +
> >>> +   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?
> 
> This has been in the drivers for as long as the Bluetooth subsystem
> has been merged upstream. So Linux 2.4.6 to be precise. I had plans to
> move this code into the core, but that has not happened yet. So until
> then, lets just include the HCI_RUNNING part.
> 

I'll add the pieces.

> 

Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[Andy Gross]

On Thu, Oct 08, 2015 at 10:07:44AM -0700, Bjorn Andersson wrote:
> On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote:
> 
> > Hi Bjorn,
> > 
> [..]
> > >> 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.
> > 
> > which means, I can only merge this driver when this other patch has
> > hit net-next tree. Unless I take the qcom_smd_id through
> > bluetooth-next tree which doesn't look like a good idea.
> > 
> 
> I was going to suggest that we just wait for -rc1, but the new additions
> to SMD adds a bunch of new items on the dependency list.
> 
> Perhaps once we agree we can get your Ack and have Andy pull it through
> the qcom-soc tree together with the SMD patches?

This is fine by me.

> > >>> 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 
> > >>
> > >> The hci.h include is not needed. If you do, then you are doing
> > >> something fishy.
> > >>
> > >



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

--
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

Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[Marcel Holtmann]

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.

which means, I can only merge this driver when this other patch has hit 
net-next tree. Unless I take the qcom_smd_id through bluetooth-next tree which 
doesn't look like a good idea.

>>> 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 
>> 
>> 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.

Writing a driver that assume it is a single instance of a given device is never 
a good idea. While you might find some instances in the kernel if you look hard 
enough, but that doesn't mean we should keep doing this. So this needs 
addressing.

> 
>>> +
>>> +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.

I wonder why that is needed, but seems an issue in the SMD subsystem. So this 
is fine, might want to add a comment above the bt_skb_alloc to remind us.

> 
>>> +   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.

I would add a comment here as well to remind everybody why this is used. And 
marking up the pointers is a good idea as well.

>>> +
>>> +   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);

Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[Bjorn Andersson]

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 
> 
> 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 

Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[Marcel Holtmann]

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.
> 
> Signed-off-by: Bjorn Andersson 
> ---
> drivers/bluetooth/Kconfig   |  11 +++
> drivers/bluetooth/Makefile  |   1 +
> drivers/bluetooth/hci_smd.c | 222 
> include/net/bluetooth/hci.h |   1 +
> 4 files changed, 235 insertions(+)
> create mode 100644 drivers/bluetooth/hci_smd.c
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 3d480d8c6111..1a2658f373b5 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -62,6 +62,17 @@ config BT_HCIBTSDIO
> Say Y here to compile support for Bluetooth SDIO devices into the
> kernel or say M to compile it as module (btsdio).
> 
> +config BT_HCISMD
> + tristate "HCI Qualcomm SMD driver"
> + depends on QCOM_SMD
> + help
> +   Qualcomm SMD HCI driver.
> +   This driver is used to bridge HCI data onto the shared memory
> +   channels to the WCNSS core.
> +
> +   Say Y here to compile support for HCI over Qualcomm SMD into the
> +   kernelor say M to compile as a module.
> +
> config BT_HCIUART
>   tristate "HCI UART driver"
>   depends on TTY
> 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. Especially the kbuild test robot complained loudly.

> 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
> new file mode 100644
> index ..e5748da2f902
> --- /dev/null
> +++ b/drivers/bluetooth/hci_smd.c
> @@ -0,0 +1,222 @@
> +/*
> + * Copyright (c) 2015, Sony Mobile Communications Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

The hci.h include is not needed. If you do, then you are doing something fishy.

> +
> +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.

> +
> +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?

> + if (!skb)
> + return -ENOMEM;
> +
> + buf = skb_put(skb, count);
> + memcpy_fromio(buf, data, count);

Why is this a memcpy_fromio here?

> +
> + skb->dev = (void *)smd_hci.hci;

This is no longer needed.

> + 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.

> +
> + return ret;
> +}
> +
> +static int smd_hci_acl_callback(struct qcom_smd_device *qsdev,
> + const void *data,
> + size_t count)
> +{
> + return smd_hci_recv(HCI_ACLDATA_PKT, data, count);
> +}
> +
> +static int smd_hci_cmd_callback(struct qcom_smd_device *qsdev,
> + const void *data,
> + size_t count)
> +{
> + return smd_hci_recv(HCI_EVENT_PKT, data, count);
> +}
> +
> +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 = 

[PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[bjorn.andersson]

From: Bjorn Andersson 

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.

Signed-off-by: Bjorn Andersson 
---
 drivers/bluetooth/Kconfig   |  11 +++
 drivers/bluetooth/Makefile  |   1 +
 drivers/bluetooth/hci_smd.c | 222 
 include/net/bluetooth/hci.h |   1 +
 4 files changed, 235 insertions(+)
 create mode 100644 drivers/bluetooth/hci_smd.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 3d480d8c6111..1a2658f373b5 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -62,6 +62,17 @@ config BT_HCIBTSDIO
  Say Y here to compile support for Bluetooth SDIO devices into the
  kernel or say M to compile it as module (btsdio).
 
+config BT_HCISMD
+   tristate "HCI Qualcomm SMD driver"
+   depends on QCOM_SMD
+   help
+ Qualcomm SMD HCI driver.
+ This driver is used to bridge HCI data onto the shared memory
+ channels to the WCNSS core.
+
+ Say Y here to compile support for HCI over Qualcomm SMD into the
+ kernelor say M to compile as a module.
+
 config BT_HCIUART
tristate "HCI UART driver"
depends on TTY
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
 
 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
new file mode 100644
index ..e5748da2f902
--- /dev/null
+++ b/drivers/bluetooth/hci_smd.c
@@ -0,0 +1,222 @@
+/*
+ * Copyright (c) 2015, Sony Mobile Communications Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static struct {
+   struct qcom_smd_channel *acl_channel;
+   struct qcom_smd_channel *cmd_channel;
+
+   struct hci_dev *hci;
+} smd_hci;
+
+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);
+   if (!skb)
+   return -ENOMEM;
+
+   buf = skb_put(skb, count);
+   memcpy_fromio(buf, data, count);
+
+   skb->dev = (void *)smd_hci.hci;
+   bt_cb(skb)->pkt_type = type;
+   skb_orphan(skb);
+
+   ret = hci_recv_frame(smd_hci.hci, skb);
+   if (ret < 0)
+   kfree_skb(skb);
+
+   return ret;
+}
+
+static int smd_hci_acl_callback(struct qcom_smd_device *qsdev,
+   const void *data,
+   size_t count)
+{
+   return smd_hci_recv(HCI_ACLDATA_PKT, data, count);
+}
+
+static int smd_hci_cmd_callback(struct qcom_smd_device *qsdev,
+   const void *data,
+   size_t count)
+{
+   return smd_hci_recv(HCI_EVENT_PKT, data, count);
+}
+
+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);
+
+   return ret;
+}
+
+static int smd_hci_open(struct hci_dev *hci)
+{
+   return 0;
+}
+
+static int smd_hci_close(struct hci_dev *hci)
+{
+   return 0;
+}
+
+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;
+   buf[5] = sizeof(bdaddr_t);
+   memcpy(buf + 6, bdaddr, sizeof(bdaddr_t));
+
+   return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf));
+}
+
+static int smd_hci_register(void)
+{
+   struct hci_dev *hci;
+   int ret;
+
+   if 

Re: [PATCH] Bluetooth: hci_smd: Qualcomm WCNSS HCI driver

[kbuild test robot]

Hi Bjorn,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please 
ignore]

config: arm-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout a380238e176a2641aa74bac37f4e735fcec4d854
# save the attached .config to linux build tree
make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

>> drivers/bluetooth/hci_smd.c:186:33: error: array type has incomplete element 
>> type
static const struct qcom_smd_id smd_hci_acl_match[] = {
^
>> drivers/bluetooth/hci_smd.c:187:2: error: field name not in record or union 
>> initializer
 { .name = "APPS_RIVA_BT_ACL" },
 ^
   drivers/bluetooth/hci_smd.c:187:2: error: (near initialization for 
'smd_hci_acl_match')
   drivers/bluetooth/hci_smd.c:191:33: error: array type has incomplete element 
type
static const struct qcom_smd_id smd_hci_cmd_match[] = {
^
   drivers/bluetooth/hci_smd.c:192:2: error: field name not in record or union 
initializer
 { .name = "APPS_RIVA_BT_CMD" },
 ^
   drivers/bluetooth/hci_smd.c:192:2: error: (near initialization for 
'smd_hci_cmd_match')
>> drivers/bluetooth/hci_smd.c:200:2: error: unknown field 'smd_match_table' 
>> specified in initializer
 .smd_match_table = smd_hci_acl_match,
 ^
>> drivers/bluetooth/hci_smd.c:200:2: warning: excess elements in struct 
>> initializer
>> drivers/bluetooth/hci_smd.c:200:2: warning: (near initialization for 
>> 'smd_hci_acl_driver')
   drivers/bluetooth/hci_smd.c:211:2: error: unknown field 'smd_match_table' 
specified in initializer
 .smd_match_table = smd_hci_cmd_match,
 ^
   drivers/bluetooth/hci_smd.c:211:2: warning: excess elements in struct 
initializer
>> drivers/bluetooth/hci_smd.c:211:2: warning: (near initialization for 
>> 'smd_hci_cmd_driver')
   In file included from drivers/bluetooth/hci_smd.c:14:0:
>> include/linux/module.h:128:27: error: redefinition of '__inittest'
 static inline initcall_t __inittest(void)  \
  ^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 
>> 'module_driver'
 module_driver(__smd_driver, qcom_smd_driver_register, \
 ^
>> drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 
>> 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_cmd_driver);
^
   include/linux/module.h:128:27: note: previous definition of '__inittest' was 
here
 static inline initcall_t __inittest(void)  \
  ^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 
>> 'module_driver'
 module_driver(__smd_driver, qcom_smd_driver_register, \
 ^
   drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 
'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_acl_driver);
^
>> include/linux/module.h:130:6: error: redefinition of 'init_module'
 int init_module(void) __attribute__((alias(#initfn)));
 ^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 
>> 'module_driver'
 module_driver(__smd_driver, qcom_smd_driver_register, \
 ^
>> drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 
>> 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_cmd_driver);
^
   include/linux/module.h:130:6: note: previous definition of 'init_module' was 
here
 int init_module(void) __attribute__((alias(#initfn)));
 ^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 
>> 'module_driver'
 module_driver(__smd_driver, qcom_smd_driver_register, \
 ^
   drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 
'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_acl_driver);
^
>> include/linux/module.h:134:27: error: redefinition of '__exittest'
 static inline exitcall_t __exittest(void)  \
  ^
>> include/linux/device.h:1326:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^

vim +186 drivers/bluetooth/hci_smd.c

   180  static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
   181  {
   182  smd_hci.cmd_channel = NULL;
   183  smd_hci_unregister();
   184  }
   185  
 > 186  static const struct qcom_smd_id smd_hci_acl_match[] = {
 > 187  { .name = "APPS_RIVA_BT_ACL" },
   188  {}
   189  };