[PATCH 0/3] Qualcomm SMEM V12 Support

[Chris Lew]

SMEM V12 was devised to make better use of the global SMEM region. The
global heap region is formatted to be similar to a private partition.
This allows the maximum number of smem items to increase. The maximum
item number is written by the bootloader in a region after the table 
of contents. The number of hosts are increased for later chipsets.

This patchset depends on patch: Qualcomm SMEM cached item support

Chris Lew (3):
  soc: qcom: smem: Support global partition
  soc: qcom: smem: Support dynamic item limit
  soc: qcom: smem: Increase the number of hosts

 drivers/soc/qcom/smem.c | 177 +++-
 1 file changed, 145 insertions(+), 32 deletions(-)

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

[PATCH 3/3] soc: qcom: smem: Increase the number of hosts

[Chris Lew]

Increase the maximum number of hosts in a system to 10.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index a51f4ba42173..4385f3b4bca9 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -93,7 +93,7 @@
 #define SMEM_HOST_APPS 0
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT9
+#define SMEM_HOST_COUNT10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] soc: qcom: smem: Support global partition

[Chris Lew]

SMEM V12 creates a global partition to allocate global
smem items from instead of a global heap. The global
partition has the same structure as a private partition.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/smem.c | 134 +---
 1 file changed, 105 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..fed2934d6bda 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -65,11 +65,20 @@
 /*
  * Item 3 of the global heap contains an array of versions for the various
  * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * a valid version as a sanity check.
  */
 #define SMEM_ITEM_VERSION  3
 #define  SMEM_MASTER_SBL_VERSION_INDEX 7
-#define  SMEM_EXPECTED_VERSION 11
+#define  SMEM_GLOBAL_HEAP_VERSION  11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */
+#define SMEM_GLOBAL_PART_VERSION   12
+#define SMEM_GLOBAL_HOST   0xfffe
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -231,6 +240,8 @@ struct smem_region {
  * struct qcom_smem - device data for the smem device
  * @dev:   device pointer
  * @hwlock:reference to a hwspinlock
+ * @global_partition:  pointer to global partition when in use
+ * @global_cacheline:  cacheline size for global partition
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
@@ -242,6 +253,8 @@ struct qcom_smem {
 
struct hwspinlock *hwlock;
 
+   struct smem_partition_header *global_partition;
+   size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
 
@@ -318,16 +331,14 @@ static void *cached_entry_to_item(struct 
smem_private_entry *e)
 #define HWSPINLOCK_TIMEOUT 1000
 
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
-  unsigned host,
+  struct smem_partition_header *phdr,
   unsigned item,
   size_t size)
 {
-   struct smem_partition_header *phdr;
struct smem_private_entry *hdr, *end;
size_t alloc_size;
void *cached;
 
-   phdr = smem->partitions[host];
hdr = phdr_to_first_uncached_entry(phdr);
end = phdr_to_last_uncached_entry(phdr);
cached = phdr_to_last_cached_entry(phdr);
@@ -335,8 +346,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
while (hdr < end) {
if (hdr->canary != SMEM_PRIVATE_CANARY) {
dev_err(smem->dev,
-   "Found invalid canary in host %d partition\n",
-   host);
+   "Found invalid canary in hosts %d:%d 
partition\n",
+   phdr->host0, phdr->host1);
return -EINVAL;
}
 
@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
 {
unsigned long flags;
int ret;
+   struct smem_partition_header *phdr;
 
if (!__smem)
return -EPROBE_DEFER;
@@ -435,10 +447,15 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
if (ret)
return ret;
 
-   if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-   ret = qcom_smem_alloc_private(__smem, host, item, size);
-   else
+   if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+   phdr = __smem->partitions[host];
+   ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+   } else if (__smem->global_partition) {
+   phdr = __smem->global_partition;
+   ret = qcom_smem_alloc_private(__smem, phdr, item, size);
+   } else {
ret = qcom_smem_alloc_global(__smem, item, size);
+   }
 
hwspin_unlock_irqrestore(__smem->hwlock, &flags);
 
@@ -480,16 +497,12 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
 }
 
 static void *qcom_smem_get_private(struct qcom_smem *smem,
-  unsigned host,
+  struct smem_partition_header *phdr,
+  size_t cacheline,
   unsigned item,
   size_t *size)
 {
-   struct smem_partition_header *phdr;
struct smem_privat

[PATCH 2/3] soc: qcom: smem: Support dynamic item limit

[Chris Lew]

In V12 SMEM, SBL writes SMEM parameter information
after the TOC. Use the SBL provided item count
as the max item number.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/smem.c | 41 +++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index fed2934d6bda..a51f4ba42173 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -225,6 +225,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic: magic number, must be SMEM_INFO_MAGIC
+ * @size:  size of the smem region
+ * @base_addr: base address of the smem region
+ * @reserved: for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+   u8 magic[4];
+   __le32 size;
+   __le32 base_addr;
+   __le32 reserved;
+   __le32 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:  identifier of aux_mem base
  * @virt_base: virtual base address of memory with this aux_mem identifier
@@ -245,6 +263,7 @@ struct smem_region {
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:   list of the memory regions defining the shared memory
  */
@@ -257,6 +276,7 @@ struct qcom_smem {
size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
+   u32 item_count;
 
unsigned num_regions;
struct smem_region regions[0];
@@ -388,7 +408,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
struct smem_header *header;
struct smem_global_entry *entry;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
+   if (WARN_ON(item >= smem->item_count))
return -EINVAL;
 
header = smem->regions[0].virt_base;
@@ -473,7 +493,7 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
u32 aux_base;
unsigned i;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
+   if (WARN_ON(item >= smem->item_count))
return ERR_PTR(-EINVAL);
 
header = smem->regions[0].virt_base;
@@ -640,6 +660,19 @@ static int qcom_smem_get_sbl_version(struct qcom_smem 
*smem)
return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+   struct smem_ptable *ptable;
+   struct smem_info *info;
+
+   ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;
+   info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+   if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+   return SMEM_ITEM_COUNT;
+
+   return le32_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem,
struct smem_ptable_entry *entry)
 {
@@ -857,7 +890,11 @@ static int qcom_smem_probe(struct platform_device *pdev)
version = qcom_smem_get_sbl_version(smem);
switch (version >> 16) {
case SMEM_GLOBAL_PART_VERSION:
+   smem->item_count = qcom_smem_get_dynamic_item(smem);
+   break;
case SMEM_GLOBAL_HEAP_VERSION:
+   smem->item_count = SMEM_ITEM_COUNT;
+   break;
default:
dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
return -EINVAL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/3] GLINK intent preallocation support

[Chris Lew]

Intents are used to specify when a channel can receive data from a
remoteproc. Add support for channels to customize the size and amount
of prequeued intents.

An audio channel might expect to receive 3 packets of size 4k in rapid
succession. This change allows the channel to prepare for this data instead
of allocating on demand.

Chris Lew (3):
  dt-bindings: soc: qcom: Support GLINK intents
  rpmsg: glink: Add support to preallocate intents
  rpmsg: glink: Use best fit intent during tx

 .../devicetree/bindings/soc/qcom/qcom,glink.txt| 10 +
 drivers/rpmsg/qcom_glink_native.c  | 48 +-
 2 files changed, 47 insertions(+), 11 deletions(-)

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

[PATCH v2 1/3] dt-bindings: soc: qcom: Support GLINK intents

[Chris Lew]

Virtual GLINK channels may know what throughput to expect from a
remoteproc. An intent advertises to the remoteproc this channel is
ready to receive data. Allow a channel to define the size and amount of
intents to be prequeued.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Change intents property to qcom,intents and update example
- Change second sentence in definition to better describe usage

 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index b277eca861f7..9663cab52246 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -39,6 +39,14 @@ of these nodes are defined by the individual bindings for 
the specific function
Definition: a list of channels tied to this function, used for matching
the function to a set of virtual channels
 
+- qcom,intents:
+   Usage: optional
+   Value type: 
+   Definition: a list of size,amount pairs describing what intents should
+   be preallocated for this virtual channel. This can be used
+   to tweak the default intents available for the channel to
+   meet expectations of the remote.
+
 = EXAMPLE
 The following example represents the GLINK RPM node on a MSM8996 device, with
 the function for the "rpm_request" channel defined, which is used for
@@ -69,6 +77,8 @@ regualtors and root clocks.
compatible = "qcom,rpm-msm8996";
qcom,glink-channels = "rpm_requests";
 
+   qcom,intents = <0x400 5
+   0x800 1>;
...
};
};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/3] rpmsg: glink: Add support to preallocate intents

[Chris Lew]

The base intents prequeued during channel creation may not satisfy a
channel's throughput requirement. Add support for intents dt-binding to
allow channels to specify the size and amount of intents to prequeue
during endpoint announcement.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Change intents property string to qcom,intents
- Use np local variable to fit of_find_property call on one line

 drivers/rpmsg/qcom_glink_native.c | 37 +
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 5a5e927ea50f..7155adad6ab6 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1143,19 +1143,40 @@ static struct rpmsg_endpoint 
*qcom_glink_create_ept(struct rpmsg_device *rpdev,
 static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 {
struct glink_channel *channel = to_glink_channel(rpdev->ept);
-   struct glink_core_rx_intent *intent;
+   struct device_node *np = rpdev->dev.of_node;
struct qcom_glink *glink = channel->glink;
-   int num_intents = glink->intentless ? 0 : 5;
+   struct glink_core_rx_intent *intent;
+   const struct property *prop = NULL;
+   int num_groups = 1;
+   int num_intents;
+   int size;
+   __be32 base[2];
+   int *val = base;
+
+   if (np)
+   prop = of_find_property(np, "qcom,intents", NULL);
+
+   if (prop && !glink->intentless) {
+   val = prop->value;
+   num_groups = prop->length / sizeof(u32) / 2;
+   } else {
+   base[0] = cpu_to_be32(SZ_1K);
+   base[1] = cpu_to_be32(glink->intentless ? 0 : 5);
+   }
 
/* Channel is now open, advertise base set of intents */
-   while (num_intents--) {
-   intent = qcom_glink_alloc_intent(glink, channel, SZ_1K, true);
-   if (!intent)
-   break;
+   while (num_groups--) {
+   size = be32_to_cpup(val++);
+   num_intents = be32_to_cpup(val++);
+   while(num_intents--) {
+   intent = qcom_glink_alloc_intent(glink, channel, size,
+true);
+   if (!intent)
+   break;
 
-   qcom_glink_advertise_intent(glink, channel, intent);
+   qcom_glink_advertise_intent(glink, channel, intent);
+   }
}
-
return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/3] rpmsg: glink: Use best fit intent during tx

[Chris Lew]

Intents can vary in size, try to find the best fitting remote intent
instead of first fit when sending a message to the remote proc.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 7155adad6ab6..0ba1ffe2698f 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1252,11 +1252,16 @@ static int __qcom_glink_send(struct glink_channel 
*channel,
spin_lock_irqsave(&channel->intent_lock, flags);
idr_for_each_entry(&channel->riids, tmp, iid) {
if (tmp->size >= len && !tmp->in_use) {
-   tmp->in_use = true;
-   intent = tmp;
-   break;
+   if (!intent)
+   intent = tmp;
+   else if (intent->size > tmp->size)
+   intent = tmp;
+   if (intent->size == len)
+   break;
}
}
+   if (intent)
+   intent->in_use = true;
spin_unlock_irqrestore(&channel->intent_lock, flags);
 
/* We found an available intent */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] rpmsg: glink: Add support to preallocate intents

[Chris Lew]

The base intents prequeued during channel creation may not satisfy a
channel's throughput requirement. Add support for intents dt-binding to
allow channels to specify the size and amount of intents to prequeue
during endpoint announcement.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 36 
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 5a5e927ea50f..05e04a8943e3 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1143,19 +1143,39 @@ static struct rpmsg_endpoint 
*qcom_glink_create_ept(struct rpmsg_device *rpdev,
 static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 {
struct glink_channel *channel = to_glink_channel(rpdev->ept);
-   struct glink_core_rx_intent *intent;
struct qcom_glink *glink = channel->glink;
-   int num_intents = glink->intentless ? 0 : 5;
+   struct glink_core_rx_intent *intent;
+   const struct property *prop = NULL;
+   int num_groups = 1;
+   int num_intents;
+   int size;
+   __be32 base[2];
+   int *val = base;
+
+   if (rpdev->dev.of_node)
+   prop = of_find_property(rpdev->dev.of_node, "intents", NULL);
+
+   if (prop && !glink->intentless) {
+   val = prop->value;
+   num_groups = prop->length / sizeof(u32) / 2;
+   } else {
+   base[0] = cpu_to_be32(SZ_1K);
+   base[1] = cpu_to_be32(glink->intentless ? 0 : 5);
+   }
 
/* Channel is now open, advertise base set of intents */
-   while (num_intents--) {
-   intent = qcom_glink_alloc_intent(glink, channel, SZ_1K, true);
-   if (!intent)
-   break;
+   while (num_groups--) {
+   size = be32_to_cpup(val++);
+   num_intents = be32_to_cpup(val++);
+   while (num_intents--) {
+   intent = qcom_glink_alloc_intent(glink, channel, size,
+true);
+   if (!intent)
+   break;
 
-   qcom_glink_advertise_intent(glink, channel, intent);
+   qcom_glink_advertise_intent(glink, channel, intent);
+   }
}
-
return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 0/3] GLINK intent preallocation support

[Chris Lew]

Intents are used to specify when a channel can receive data from a
remoteproc. Add support for channels to customize the size and amount
of prequeued intents.

An audio channel might expect to receive 3 packets of size 4k in rapid
succession. This change allows the channel to prepare for this data instead
of allocating on demand.

Chris Lew (3):
  dt-bindings: soc: qcom: Support GLINK intents
  rpmsg: glink: Add support to preallocate intents
  rpmsg: glink: Use best fit intent during tx

 .../devicetree/bindings/soc/qcom/qcom,glink.txt| 10 +
 drivers/rpmsg/qcom_glink_native.c  | 47 +-
 2 files changed, 46 insertions(+), 11 deletions(-)

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

[PATCH 1/3] dt-bindings: soc: qcom: Support GLINK intents

[Chris Lew]

Virtual GLINK channels may know what throughput to expect from a
remoteproc. An intent advertises to the remoteproc this channel is
ready to receive data. Allow a channel to define the size and amount of
intents to be prequeued.

Signed-off-by: Chris Lew 
---
 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index b277eca861f7..6c21f76822ca 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -39,6 +39,14 @@ of these nodes are defined by the individual bindings for 
the specific function
Definition: a list of channels tied to this function, used for matching
the function to a set of virtual channels
 
+- intents:
+   Usage: optional
+   Value type: 
+   Definition: a list of size,amount pairs describing what intents should
+   be preallocated for this virtual channel. If a GLINK node
+   supports intents, an intent advertises this channel is ready
+   to receive data.
+
 = EXAMPLE
 The following example represents the GLINK RPM node on a MSM8996 device, with
 the function for the "rpm_request" channel defined, which is used for
@@ -69,6 +77,8 @@ regualtors and root clocks.
compatible = "qcom,rpm-msm8996";
qcom,glink-channels = "rpm_requests";
 
+   intents = <0x400 5
+  0x800 1>;
...
};
};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] rpmsg: glink: Use best fit intent during tx

[Chris Lew]

Intents can vary in size, try to find the best fitting remote intent
instead of first fit when sending a message to the remote proc.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 05e04a8943e3..650974f85a4c 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1251,11 +1251,16 @@ static int __qcom_glink_send(struct glink_channel 
*channel,
spin_lock_irqsave(&channel->intent_lock, flags);
idr_for_each_entry(&channel->riids, tmp, iid) {
if (tmp->size >= len && !tmp->in_use) {
-   tmp->in_use = true;
-   intent = tmp;
-   break;
+   if (!intent)
+   intent = tmp;
+   else if (intent->size > tmp->size)
+   intent = tmp;
+   if (intent->size == len)
+   break;
}
}
+   if (intent)
+   intent->in_use = true;
spin_unlock_irqrestore(&channel->intent_lock, flags);
 
/* We found an available intent */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] rpmsg: glink: Initialize the "intent_req_comp" completion variable

[Chris Lew]

On 10/27/2017 3:45 AM, Arun Kumar Neelakantam wrote:

The "intent_req_comp" variable is used without initialization which
results in NULL pointer derefernce in qcom_glink_request_intent().



Typo on dereference.

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

Re: [PATCH 4/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

On 12/18/2017 2:48 PM, Stephen Boyd wrote:

On 12/18/2017 02:02 PM, Chris Lew wrote:

Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.

Signed-off-by: Chris Lew 
---
  drivers/rpmsg/qcom_glink_native.c | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 786f2eca01f1..a897ccea3098 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1558,6 +1558,22 @@ static void qcom_glink_work(struct work_struct *work)
}
  }
  
+static ssize_t rpmsg_name_show(struct device *dev,

+  struct device_attribute *attr, char *buf)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct qcom_glink_device *gdev = to_glink_device(rpdev);
+
+   return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", gdev->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {


const?



Thanks Stephen, will change to "static const struct attribute *"

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

Re: [PATCH 5/6] rpmsg: Introduce rpmsg_get_rproc_name

[Chris Lew]

On 12/19/2017 9:52 AM, Bjorn Andersson wrote:

On Mon 18 Dec 14:02 PST 2017, Chris Lew wrote:


Add support for client's to query the edge name their channel is
registered for. This is useful for clients who share the same channel
identifier across different remote procs.



I presume this will result in a strcmp in some client driver?

When we're registering the rpmsg device, as part of handling of an
arriving "open request", we do look for an of_node with matching
qcom,glink-channels and if one is found we point the dev->of_node of the
new device to this node.

So I would suggest that you, in your client driver, use this to decide
which instance you're on; regardless if you're using compatible based
driver matching.

Does this work for you?

Regards,
Bjorn



Yea I think this works for us, we can drop this patch.

Just to confirm my understanding, clients can use rpdev->dev.of_node to 
get the parent of_node and from there get the label field? Also to 
ensure of_node is set, client's need to add their channel to the dt with 
qcom,glink-channels.


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

Re: [PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

On 12/21/2017 11:36 AM, Stephen Boyd wrote:

On 12/20/2017 05:35 PM, Bjorn Andersson wrote:

On Wed 20 Dec 10:30 PST 2017, Rob Herring wrote:


On Mon, Dec 18, 2017 at 02:02:09PM -0800, Chris Lew wrote:

Add a label property to identify the edge this node represents.

Why does a user need to know this?


We have multiple remoteproc instances, each one having one or more
associated SMD or GLINK links (this node), exposing logical
communication channels. Some of these logical channels are exposed to
user space and we need a way to distinguish them there.

In the current implementation of SMD this value goes straight into an
sysfs attribute that we can use when writing udev rules and for the DIAG
implementation to pair up channels related to the same remoteproc. This
adds the equivalent information for glink-backed channels.


I'm therefor in favor of picking this patch.


Please add these details to the commit log. Just writing what the patch
is doing isn't very helpful.



Ok, will do.

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

[PATCH] rpmsg: glink: Set tail pointer to 0 at end of FIFO

[Chris Lew]

When wrapping around the FIFO, the remote expects the tail pointer to
be reset to 0 on the edge case where the tail equals the FIFO length.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 2b5cf2790954..7a276be598d6 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -109,7 +109,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe 
*np,
tail = le32_to_cpu(*pipe->tail);
 
tail += count;
-   if (tail > pipe->native.length)
+   if (tail >= pipe->native.length)
tail -= pipe->native.length;
 
*pipe->tail = cpu_to_le32(tail);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1] net: qrtr: ns: Ignore ENODEV failures in ns

[Chris Lew]

On 12/23/2023 5:56 AM, Simon Horman wrote:

[Dropped bjorn.anders...@kernel.org, as the correct address seems
  to be anders...@kernel.org, which is already in the CC list.
  kernel.org rejected sending this email without that update.]

On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:

From: Chris Lew 

Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
indicate that either the local port has been closed or the remote has
gone down. Neither of these scenarios are fatal and will eventually be
handled through packets that are later queued on the control port.

Signed-off-by: Chris Lew 
Signed-off-by: Sarannya Sasikumar 
---
  net/qrtr/ns.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index abb0c70..8234339 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr *dest,
msg.msg_namelen = sizeof(*dest);
  
  	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));

-   if (ret < 0)
+   if (ret < 0 && ret != -ENODEV)
pr_err("failed to announce del service\n");
  
  	return ret;


Hi,

The caller of service_announce_del() ignores it's return value.
So the only action on error is the pr_err() call above, and so
with this patch -ENODEV is indeed ignored.

However, I wonder if it would make things clearer to the reader (me?)
if the return type of service_announce_del was updated void. Because
as things stand -ENODEV may be returned, which implies something might
handle that, even though it doe not.

The above notwithstanding, this change looks good to me.

Reviewed-by: Simon Horman 

...


Hi Simon, thanks for the review and suggestion. We weren't sure whether 
we should change the function prototype on these patches on the chance 
that there will be something that listens and handles this in the 
future. I think it's a good idea to change it to void and we can change 
it back if there is such a usecase in the future.

Re: [PATCH v7 1/6] soc: qcom: pdr: protect locator_addr with the main mutex

[Chris Lew]

On 4/24/2024 2:27 AM, Dmitry Baryshkov wrote:

If the service locator server is restarted fast enough, the PDR can
rewrite locator_addr fields concurrently. Protect them by placing
modification of those fields under the main pdr->lock.

Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart helpers")
Tested-by: Neil Armstrong  # on SM8550-QRD
Signed-off-by: Dmitry Baryshkov 
---
  drivers/soc/qcom/pdr_interface.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c
index a1b6a4081dea..19cfe4b41235 100644
--- a/drivers/soc/qcom/pdr_interface.c
+++ b/drivers/soc/qcom/pdr_interface.c
@@ -76,12 +76,12 @@ static int pdr_locator_new_server(struct qmi_handle *qmi,
  locator_hdl);
struct pdr_service *pds;
  
+	mutex_lock(&pdr->lock);

/* Create a local client port for QMI communication */
pdr->locator_addr.sq_family = AF_QIPCRTR;
pdr->locator_addr.sq_node = svc->node;
pdr->locator_addr.sq_port = svc->port;
  
-	mutex_lock(&pdr->lock);

pdr->locator_init_complete = true;
mutex_unlock(&pdr->lock);
  
@@ -104,10 +104,10 @@ static void pdr_locator_del_server(struct qmi_handle *qmi,
  
  	mutex_lock(&pdr->lock);

pdr->locator_init_complete = false;
-   mutex_unlock(&pdr->lock);
  
  	pdr->locator_addr.sq_node = 0;

pdr->locator_addr.sq_port = 0;
+   mutex_unlock(&pdr->lock);
  }
  
  static const struct qmi_ops pdr_locator_ops = {




These two functions are provided as qmi_ops handlers in pdr_locator_ops. 
Aren't they serialized in the qmi handle's workqueue since it as an 
ordered_workqueue? Even in a fast pdr scenario I don't think we would 
see a race condition between these two functions.


The other access these two functions do race against is in the 
pdr_notifier_work. I think you would need to protect locator_addr in 
pdr_get_domain_list since the qmi_send_request there uses 
'pdr->locator_addr'.


Thanks!
Chris

Re: [PATCH v7 4/6] soc: qcom: qmi: add a way to remove running service

[Chris Lew]

On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

Add qmi_del_server(), a pair to qmi_add_server(), a way to remove
running server from the QMI socket. This is e.g. necessary for
pd-mapper, which needs to readd a server each time the DSP is started or


s/readd/read/


stopped.

Tested-by: Neil Armstrong  # on SM8550-QRD
Signed-off-by: Dmitry Baryshkov 
---





+/**
+ * qmi_del_server() - register a service with the name service


Update comment to describe removal of service instead of 'register'.

Re: [PATCH v7 5/6] soc: qcom: add pd-mapper implementation

[Chris Lew]

On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:


+static int qcom_pdm_start(void)
+{
+   const struct of_device_id *match;
+   const struct qcom_pdm_domain_data * const *domains;
+   struct device_node *root;
+   int ret, i;
+
+   root = of_find_node_by_path("/");
+   if (!root)
+   return -ENODEV;
+
+   match = of_match_node(qcom_pdm_domains, root);
+   of_node_put(root);
+   if (!match) {
+   pr_notice("PDM: no support for the platform, userspace daemon might 
be required.\n");
+   return -ENODEV;
+   }
+
+   domains = match->data;
+   if (!domains) {
+   pr_debug("PDM: no domains\n");
+   return -ENODEV;
+   }
+
+   mutex_lock(&qcom_pdm_mutex);
+   for (i = 0; domains[i]; i++) {
+   ret = qcom_pdm_add_domain(domains[i]);
+   if (ret)
+   goto free_domains;
+   }
+
+   ret = qmi_handle_init(&qcom_pdm_handle, 1024,
+ NULL, qcom_pdm_msg_handlers);


1024 here seems arbitrary, I think most other usage of qmi_handle_init 
has a macro defined for the max message length of the qmi service.



+   if (ret)
+   goto free_domains;
+
+   ret = qmi_add_server(&qcom_pdm_handle, SERVREG_LOCATOR_SERVICE,
+SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
+   if (ret) {
+   pr_err("PDM: error adding server %d\n", ret);
+   goto release_handle;
+   }
+   mutex_unlock(&qcom_pdm_mutex);
+
+   return 0;
+
+release_handle:
+   qmi_handle_release(&qcom_pdm_handle);
+
+free_domains:
+   qcom_pdm_free_domains();
+   mutex_unlock(&qcom_pdm_mutex);
+
+   return ret;
+}
+
+static void qcom_pdm_stop(void)
+{
+   qmi_del_server(&qcom_pdm_handle, SERVREG_LOCATOR_SERVICE,
+  SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
+
+   qmi_handle_release(&qcom_pdm_handle);
+


I don't think doing an explicit qmi_del_server() is necessary. As part 
of the qmi_handle_release(), the qrtr socket will be closed and the qrtr 
ns will broadcast a DEL_SERVER and DEL_CLIENT notification as part of 
the cleanup.



+   qcom_pdm_free_domains();
+}
+

Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

[Chris Lew]

On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1d24c9b656a8..02d0c626b03b 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)

int ret;
unsigned int val;
  
-	ret = qcom_q6v5_prepare(&adsp->q6v5);

+   ret = qcom_pdm_get();
if (ret)
return ret;


Would it make sense to try and model this as a rproc subdev? This 
section of the remoteproc code seems to be focused on making specific 
calls to setup and enable hardware resources, where as pd mapper is 
software.


sysmon and ssr are also purely software and they are modeled as subdevs 
in qcom_common. I'm not an expert on remoteproc organization but this 
was just a thought.


Thanks!
Chris

  
+	ret = qcom_q6v5_prepare(&adsp->q6v5);

+   if (ret)
+   goto put_pdm;
+
ret = adsp_map_carveout(rproc);
if (ret) {
dev_err(adsp->dev, "ADSP smmu mapping failed\n");
@@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
adsp_unmap_carveout(rproc);
  disable_irqs:
qcom_q6v5_unprepare(&adsp->q6v5);
+put_pdm:
+   qcom_pdm_release();
  
  	return ret;

  }

Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper

[Chris Lew]

On 4/26/2024 6:36 PM, Dmitry Baryshkov wrote:

On Sat, 27 Apr 2024 at 04:03, Chris Lew  wrote:




On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
b/drivers/remoteproc/qcom_q6v5_adsp.c
index 1d24c9b656a8..02d0c626b03b 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -23,6 +23,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 

@@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc)
   int ret;
   unsigned int val;

- ret = qcom_q6v5_prepare(&adsp->q6v5);
+ ret = qcom_pdm_get();
   if (ret)
   return ret;


Would it make sense to try and model this as a rproc subdev? This
section of the remoteproc code seems to be focused on making specific
calls to setup and enable hardware resources, where as pd mapper is
software.

sysmon and ssr are also purely software and they are modeled as subdevs
in qcom_common. I'm not an expert on remoteproc organization but this
was just a thought.


Well, the issue is that the pd-mapper is a global, not a per-remoteproc instance



Both sysmon and ssr have some kind of global states that they manage 
too. Each subdev functionality tends to be a mix of per-remoteproc 
instance management and global state management.


If pd-mapper was completely global, pd-mapper would be able to 
instantiate by itself. Instead, instantiation is dependent on each 
remoteproc instance properly getting and putting references.


The pdm subdev could manage the references to pd-mapper for that 
remoteproc instance.


On the other hand, I think Bjorn recommended this could be moved to 
probe time in v4. The v4 version was doing the reinitialization-dance, 
but I think the recommendation could still apply to this version.




Thanks!
Chris



+ ret = qcom_q6v5_prepare(&adsp->q6v5);
+ if (ret)
+ goto put_pdm;
+
   ret = adsp_map_carveout(rproc);
   if (ret) {
   dev_err(adsp->dev, "ADSP smmu mapping failed\n");
@@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc)
   adsp_unmap_carveout(rproc);
   disable_irqs:
   qcom_q6v5_unprepare(&adsp->q6v5);
+put_pdm:
+ qcom_pdm_release();

   return ret;
   }

Re: [PATCH V1] soc: qcom: smp2p: Introduce tracepoint support

[Chris Lew]

On 4/29/2024 12:55 AM, Sudeepgoud Patil wrote:

Introduce tracepoint support for smp2p providing useful logging
for communication between clients.



Let's add some more description as to why these tracepoint are useful. 
Do they help us track latency? debugging information for us? for clients?



Signed-off-by: Sudeepgoud Patil 
Signed-off-by: Deepak Kumar Singh 


As Elliot mentioned in the internal review, your Signed-off-by should be 
last. I would suggest removing Deepak's Signed-off-by and letting him 
reply with Reviewed-by since he was the main reviewer internally.



---
  drivers/soc/qcom/Makefile  |  1 +
  drivers/soc/qcom/smp2p.c   | 10 
  drivers/soc/qcom/trace-smp2p.h | 99 ++
  3 files changed, 110 insertions(+)
  create mode 100644 drivers/soc/qcom/trace-smp2p.h

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ca0bece0dfff..30c1bf645501 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -23,6 +23,7 @@ qcom_rpmh-y   += rpmh.o
  obj-$(CONFIG_QCOM_SMD_RPM)+= rpm-proc.o smd-rpm.o
  obj-$(CONFIG_QCOM_SMEM) +=smem.o
  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
+CFLAGS_smp2p.o := -I$(src)
  obj-$(CONFIG_QCOM_SMP2P)  += smp2p.o
  obj-$(CONFIG_QCOM_SMSM)   += smsm.o
  obj-$(CONFIG_QCOM_SOCINFO)+= socinfo.o
diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a21241cbeec7..dde8513641ae 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -20,6 +20,9 @@
  #include 
  #include 
  
+#define CREATE_TRACE_POINTS

+#include "trace-smp2p.h"
+
  /*
   * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
   * of a single 32-bit value between two processors.  Each value has a single
@@ -191,6 +194,7 @@ static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p)
struct smp2p_smem_item *out = smp2p->out;
u32 val;
  
+	trace_smp2p_ssr_ack(smp2p->remote_pid);

smp2p->ssr_ack = !smp2p->ssr_ack;
  
  	val = out->flags & ~BIT(SMP2P_FLAGS_RESTART_ACK_BIT);

@@ -213,6 +217,7 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p *smp2p)
smp2p->ssr_ack_enabled = true;
  
  		smp2p->negotiation_done = true;

+   trace_smp2p_negotiate(smp2p->remote_pid, 
smp2p->ssr_ack_enabled);


since this tracepoint is for negotiating, maybe we should capture all of 
the features (out->features) instead of just the ssr_ack feature.



}
  }
  
@@ -251,6 +256,8 @@ static void qcom_smp2p_notify_in(struct qcom_smp2p *smp2p)

status = val ^ entry->last_value;
entry->last_value = val;
  
+		trace_smp2p_notify_in(smp2p->remote_pid, entry->name, status, val);

+
/* No changes of this entry? */
if (!status)
continue;
@@ -406,6 +413,9 @@ static int smp2p_update_bits(void *data, u32 mask, u32 
value)
writel(val, entry->value);
spin_unlock_irqrestore(&entry->lock, flags);
  
+	trace_smp2p_update_bits(entry->smp2p->remote_pid,

+   entry->name, orig, val);
+
if (val != orig)
qcom_smp2p_kick(entry->smp2p);
  
diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h

new file mode 100644
index ..c61afab23f0c
--- /dev/null
+++ b/drivers/soc/qcom/trace-smp2p.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qcom_smp2p
+
+#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __QCOM_SMP2P_TRACE_H__
+
+#include 
+
+TRACE_EVENT(smp2p_ssr_ack,
+   TP_PROTO(unsigned int remote_pid),


Is there any way we can map the remote pid's to a string? I feel like 
the tracepoints would be more useful if they called out modem, adsp, etc 
instead of an integer value.



+   TP_ARGS(remote_pid),
+   TP_STRUCT__entry(
+   __field(u32, remote_pid)
+   ),
+   TP_fast_assign(
+   __entry->remote_pid = remote_pid;
+   ),
+   TP_printk("%d: SSR detected, doing SSR Handshake",
+   __entry->remote_pid
+   )
+);
+
+TRACE_EVENT(smp2p_negotiate,
+   TP_PROTO(unsigned int remote_pid, bool ssr_ack_enabled),
+   TP_ARGS(remote_pid, ssr_ack_enabled),
+   TP_STRUCT__entry(
+   __field(u32, remote_pid)
+   __field(bool, ssr_ack_enabled)
+   ),
+   TP_fast_assign(
+   __entry->remote_pid = remote_pid;
+   __entry->ssr_ack_enabled = ssr_ack_enabled;
+   ),
+   TP_printk("%d: state=open ssr_ack=%d",
+   __entry->remote_pid,
+   __entry->ssr_ack_enabled
+   )
+);
+
+TRACE_EVENT(smp2p_notify_in,
+   TP_PROTO(unsigned int remote_pid, const char *name, unsigned long 
status, u32 val),
+   TP_ARGS(remote_pid, name, status, val

Re: [PATCH V1] rpmsg: glink: Make glink smem interrupt wakeup capable

[Chris Lew]

On 6/3/2024 2:37 AM, Caleb Connolly wrote:

Hi Deepak,

On 03/06/2024 09:36, Deepak Kumar Singh wrote:

There are certain usecases which require glink interrupt to be
wakeup capable. For example if handset is in sleep state and
usb charger is plugged in, dsp wakes up and sends glink interrupt
to host for glink pmic channel communication. Glink is suppose to
wakeup host processor completely for further glink data handling.
IRQF_NO_SUSPEND does not gurantee complete wakeup, system may again
enter sleep after interrupt handling and glink data may not be
handled by pmic client driver.

To ensure data handling by client configure glink smem device as
wakeup source and attach glink interrupt as wakeup irq. Remove
IRQF_NO_SUSPEND flag as it is no longer required.


I'm not sure I agree with this approach, glink is used for lots of 
things -- like QRTR, where the sensor DSP and modem may also need to 
wake the system up (e.g. for "wake on pickup" on mobile, or for incoming 
calls/sms).


Configuring this to always wake up the system fully will result in a lot 
of spurious wakeups for arbitrary modem notifications (e.g. signal 
strength changes) if userspace hasn't properly configured these 
(something ModemManager currently lacks support for).


IRQF_NO_SUSPEND is presumably necessary to keep the DSPs happy? iirc 
downstream Qualcomm kernels have historically taken this approach to 
avoid spurious wakeups.




To give some more context, until recently the GLINK interrupt was 
managed and requested in the GLINK native layer. Any type of interrupt 
configuration would affect all of the links. The interrupt is now being 
requested at the transport layer (smem/rpm), so it has a little more 
fine grain control.


In downstream, we had switched to IRQF_NO_SUSPEND because there were a 
couple of cases where glink communication with rpm was needed during the 
suspend path. Having the interrupt configured as wake capable conflicted 
with the use case.


The general expectation from the DSPs is that if it is important enough 
to send, then it should be important enough to wake the APPS subsystem. 
We've always had to work around the fact we were using IRQF_NO_SUSPEND 
in downstream.


I proposed an alternative approach some time back that would allow the 
wakeup to be configured on a per-channel basis.


https://lore.kernel.org/linux-arm-msm/20230117142414.983946-1-caleb.conno...@linaro.org/
> Back then Bjorn proposed using some socket specific mechanism to handle
this for QRTR, but given this is now a common issue for multiple glink 
channels, maybe it's something we could revisit.


Requiring the wakeup be enabled by userspace clearly doesn't make sense 
for your proposed usecase, perhaps there's a way to configure this on a 
per-channel basis in-kernel (maybe as the rpmsg API?).




This alternative approach seems reasonable to me as well. I think the 
only drawback I see with this approach is non-data traffic may stall. 
The glink protocol traffic not tied to a TX_DATA command, such as intent 
requests, wouldn't wake the system even if the channel is configured to 
be wake capable.


Thanks,
Chris


Thanks and regards,


Signed-off-by: Deepak Kumar Singh 
---
  drivers/rpmsg/qcom_glink_smem.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_smem.c 
b/drivers/rpmsg/qcom_glink_smem.c

index 7a982c60a8dd..f1b553efab13 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -22,6 +22,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
@@ -306,8 +307,7 @@ struct qcom_glink_smem 
*qcom_glink_smem_register(struct device *parent,

  smem->irq = of_irq_get(smem->dev.of_node, 0);
  ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
-   IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
-   "glink-smem", smem);
+   IRQF_NO_AUTOEN, "glink-smem", smem);
  if (ret) {
  dev_err(&smem->dev, "failed to request IRQ\n");
  goto err_put_dev;
@@ -346,6 +346,8 @@ struct qcom_glink_smem 
*qcom_glink_smem_register(struct device *parent,

  smem->glink = glink;
+    device_init_wakeup(dev, true);
+    dev_pm_set_wake_irq(dev, smem->irq);
  enable_irq(smem->irq);
  return smem;
@@ -365,6 +367,8 @@ void qcom_glink_smem_unregister(struct 
qcom_glink_smem *smem)

  struct qcom_glink *glink = smem->glink;
  disable_irq(smem->irq);
+    dev_pm_clear_wake_irq(&smem->dev);
+    device_init_wakeup(&smem->dev, false);
  qcom_glink_native_remove(glink);

Re: [PATCH v8 1/5] soc: qcom: pdr: protect locator_addr with the main mutex

[Chris Lew]

Hi Dmitry,

On 5/11/2024 2:56 PM, Dmitry Baryshkov wrote:
...

@@ -76,12 +76,12 @@ static int pdr_locator_new_server(struct qmi_handle *qmi,
  locator_hdl);
struct pdr_service *pds;
  
+	mutex_lock(&pdr->lock);

/* Create a local client port for QMI communication */
pdr->locator_addr.sq_family = AF_QIPCRTR;
pdr->locator_addr.sq_node = svc->node;
pdr->locator_addr.sq_port = svc->port;
  
-	mutex_lock(&pdr->lock);

pdr->locator_init_complete = true;
mutex_unlock(&pdr->lock);
  
@@ -104,10 +104,10 @@ static void pdr_locator_del_server(struct qmi_handle *qmi,
  
  	mutex_lock(&pdr->lock);

pdr->locator_init_complete = false;
-   mutex_unlock(&pdr->lock);
  
  	pdr->locator_addr.sq_node = 0;

pdr->locator_addr.sq_port = 0;
+   mutex_unlock(&pdr->lock);
  }
  
  static const struct qmi_ops pdr_locator_ops = {

@@ -365,6 +365,7 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
if (ret < 0)
return ret;
  
+	mutex_lock(&pdr->lock);

ret = qmi_send_request(&pdr->locator_hdl,
   &pdr->locator_addr,
   &txn, SERVREG_GET_DOMAIN_LIST_REQ,
@@ -373,15 +374,16 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
   req);
if (ret < 0) {
qmi_txn_cancel(&txn);
-   return ret;
+   goto err_unlock;
}
  
  	ret = qmi_txn_wait(&txn, 5 * HZ);

if (ret < 0) {
pr_err("PDR: %s get domain list txn wait failed: %d\n",
   req->service_name, ret);
-   return ret;
+   goto err_unlock;
}
+   mutex_unlock(&pdr->lock);


I'm not sure it is necessary to hold the the mutex during the 
qmi_txn_wait() since the only variable we are trying to protect is 
locator_addr.


Wouldn't this delay other work like new/del server notifications if this 
qmi service is delayed or non-responsive?


Thanks,
Chris

  
  	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {

pr_err("PDR: %s get domain list failed: 0x%x\n",
@@ -390,6 +392,11 @@ static int pdr_get_domain_list(struct 
servreg_get_domain_list_req *req,
}
  
  	return 0;

+
+err_unlock:
+   mutex_unlock(&pdr->lock);
+
+   return ret;
  }
  

Re: [PATCH v8 2/5] soc: qcom: pdr: fix parsing of domains lists

[Chris Lew]

On 5/11/2024 2:56 PM, Dmitry Baryshkov wrote:

While parsing the domains list, start offsets from 0 rather than from
domains_read. The domains_read is equal to the total count of the
domains we have seen, while the domains list in the message starts from
offset 0.

Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart helpers")
Tested-by: Steev Klimaszewski 
Tested-by: Alexey Minnekhanov 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/soc/qcom/pdr_interface.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pdr_interface.c b/drivers/soc/qcom/pdr_interface.c
index e014dd2d8ab3..d495ee736519 100644
--- a/drivers/soc/qcom/pdr_interface.c
+++ b/drivers/soc/qcom/pdr_interface.c
@@ -422,7 +422,7 @@ static int pdr_locate_service(struct pdr_handle *pdr, 
struct pdr_service *pds)
if (ret < 0)
goto out;
  
-		for (i = domains_read; i < resp->domain_list_len; i++) {

+   for (i = 0; i < resp->domain_list_len; i++) {
entry = &resp->domain_list[i];
  
  			if (strnlen(entry->name, sizeof(entry->name)) == sizeof(entry->name))




Reviewed-by: Chris Lew 

Re: [PATCH v8 4/5] soc: qcom: add pd-mapper implementation

[Chris Lew]

On 5/11/2024 2:56 PM, Dmitry Baryshkov wrote:

Existing userspace protection domain mapper implementation has several
issue. It doesn't play well with CONFIG_EXTRA_FIRMWARE, it doesn't
reread JSON files if firmware location is changed (or if firmware was
not available at the time pd-mapper was started but the corresponding
directory is mounted later), etc.

Provide in-kernel service implementing protection domain mapping
required to work with several services, which are provided by the DSP
firmware.

This module is loaded automatically by the remoteproc drivers when
necessary via the symbol dependency. It uses a root node to match a
protection domains map for a particular board. It is not possible to
implement it as a 'driver' as there is no corresponding device.

Tested-by: Steev Klimaszewski 
Tested-by: Alexey Minnekhanov 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/soc/qcom/Kconfig  |  11 +
  drivers/soc/qcom/Makefile |   1 +
  drivers/soc/qcom/pdr_internal.h   |  14 +
  drivers/soc/qcom/qcom_pd_mapper.c | 676 ++
  drivers/soc/qcom/qcom_pdr_msg.c   |  34 ++
  5 files changed, 736 insertions(+)



Reviewed-by: Chris Lew 

Re: [PATCH v8 5/5] remoteproc: qcom: enable in-kernel PD mapper

[Chris Lew]

On 5/11/2024 2:56 PM, Dmitry Baryshkov wrote:

Request in-kernel protection domain mapper to be started before starting
Qualcomm DSP and release it once DSP is stopped. Once all DSPs are
stopped, the PD mapper will be stopped too.

Signed-off-by: Dmitry Baryshkov 
---
  drivers/remoteproc/qcom_common.c| 87 +
  drivers/remoteproc/qcom_common.h| 10 +
  drivers/remoteproc/qcom_q6v5_adsp.c |  3 ++
  drivers/remoteproc/qcom_q6v5_mss.c  |  3 ++
  drivers/remoteproc/qcom_q6v5_pas.c  |  3 ++
  drivers/remoteproc/qcom_q6v5_wcss.c |  3 ++
  6 files changed, 109 insertions(+)



Thanks for looking into whether this could be implemented as a 
remoteproc subdevice.


Reviewed-by: Chris Lew 

Re: [PATCH V2 1/2] soc: qcom: smp2p: Add remote name into smp2p irq devname

[Chris Lew]

On 6/11/2024 9:06 AM, Bjorn Andersson wrote:

On Tue, Jun 11, 2024 at 06:03:50PM +0530, Sudeepgoud Patil wrote:

Add smp2p irq devname which fetches remote name from respective
smp2p dtsi node, which makes the wakeup source distinguishable
in irq wakeup prints.

Signed-off-by: Sudeepgoud Patil 
---
  drivers/soc/qcom/smp2p.c | 14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index a21241cbeec7..a77fee048b38 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -122,6 +122,7 @@ struct smp2p_entry {
   * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was 
enabled
   * @ssr_ack: current cached state of the local ack bit
   * @negotiation_done: whether negotiating finished
+ * @irq_devname: poniter to the smp2p irq devname
   * @local_pid:processor id of the inbound edge
   * @remote_pid:   processor id of the outbound edge
   * @ipc_regmap:   regmap for the outbound ipc
@@ -146,6 +147,7 @@ struct qcom_smp2p {
bool ssr_ack;
bool negotiation_done;
  
+	char *irq_devname;

unsigned local_pid;
unsigned remote_pid;
  
@@ -614,10 +616,16 @@ static int qcom_smp2p_probe(struct platform_device *pdev)

/* Kick the outgoing edge after allocating entries */
qcom_smp2p_kick(smp2p);
  
+	smp2p->irq_devname = kasprintf(GFP_KERNEL, "%s", pdev->dev.of_node->name);


That's a lot of extra instructions for copying a string, which doesn't
need to be copied because of_node->name is const char and the argument
to devm_request_threaded_irq() is const char.

So, kstrdup_const() is what you're looking for.

You can then go devm_kstrdup_const() and avoid the kfree() (then
kfree_const()) below.


That said, looking at /proc/interrupts, I think it would make sense to
make this devm_kasprintf(..., "smp2p-%s", name);



Is it ok to rely on the "of_node->name"? I think device tree tends to 
always have the node name as "smp2p-%s" already, so ("smp2p-%s", name) 
would result in "smp2p-smp2p-adsp".


Also Sudeepgoud, I think this will update the irqname in 
/proc/interrupts for the ipcc irqchip entry. It would also be helpful if 
we could differentiate the instances of smp2p irqchips as well. That way 
we can see what processors the 'ready' and 'fatal' interrupts apply to 
in /proc/interrupts.


Can you refer to my internal patch that adds .irq_print_chip() and 
incorporate those changes here?



Regards,
Bjorn


+   if (!smp2p->irq_devname) {
+   ret = -ENOMEM;
+   goto unwind_interfaces;
+   }
+
ret = devm_request_threaded_irq(&pdev->dev, irq,
NULL, qcom_smp2p_intr,
IRQF_ONESHOT,
-   "smp2p", (void *)smp2p);
+   smp2p->irq_devname, (void *)smp2p);
if (ret) {
dev_err(&pdev->dev, "failed to request interrupt\n");
goto unwind_interfaces;
@@ -650,6 +658,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
list_for_each_entry(entry, &smp2p->outbound, node)
qcom_smem_state_unregister(entry->state);
  
+	kfree(smp2p->irq_devname);

+
smp2p->out->valid_entries = 0;
  
  release_mbox:

@@ -677,6 +687,8 @@ static void qcom_smp2p_remove(struct platform_device *pdev)
  
  	mbox_free_channel(smp2p->mbox_chan);
  
+	kfree(smp2p->irq_devname);

+
smp2p->out->valid_entries = 0;
  }
  
--

Re: [PATCH V2 2/2] soc: qcom: smp2p: Introduce tracepoint support

[Chris Lew]

On 6/11/2024 5:33 AM, Sudeepgoud Patil wrote:

This commit introduces tracepoint support for smp2p,
enabling logging of communication between local and remote processors.
The tracepoints include information about the remote processor ID,
remote subsystem name, negotiation details, supported features,
bit change notifications, and ssr activity.
These tracepoints are valuable for debugging issues between subsystems.

Signed-off-by: Sudeepgoud Patil 
---

...

diff --git a/drivers/soc/qcom/trace-smp2p.h b/drivers/soc/qcom/trace-smp2p.h
new file mode 100644
index ..833782460b57
--- /dev/null
+++ b/drivers/soc/qcom/trace-smp2p.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qcom_smp2p
+
+#if !defined(__QCOM_SMP2P_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __QCOM_SMP2P_TRACE_H__
+
+#include 
+
+#define SMP2P_FEATURE_SSR_ACK 0x1


Now that I see it, redefining the the feature flag here seems a bit out 
of place. I'm not sure if it's worth kicking off a header file for this 
single define though.



+
+TRACE_EVENT(smp2p_ssr_ack,
+   TP_PROTO(unsigned int remote_pid, char *irq_devname),
+   TP_ARGS(remote_pid, irq_devname),
+   TP_STRUCT__entry(
+   __field(u32, remote_pid)
+   __string(irq_devname, irq_devname)
+   ),
+   TP_fast_assign(
+   __entry->remote_pid = remote_pid;
+   __assign_str(irq_devname, irq_devname);
+   ),
+   TP_printk("%d: %s: SSR detected, doing SSR Handshake",
+   __entry->remote_pid,
+   __get_str(irq_devname)
+   )
+);
+


I don't think we need to pass remote_pid into all of the traces if we 
have a unique name "irq_devname" to identify the remote now. We could 
remove remote_pid from all the trace event arguments.


We can probably drop the "doing SSR Handshake" part of this print. I 
think it can be assumed that we're doing the handshake once we've 
detected SSR.

Re: [PATCH v9 1/5] soc: qcom: pdr: protect locator_addr with the main mutex

[Chris Lew]

On 6/21/2024 3:03 PM, Dmitry Baryshkov wrote:

If the service locator server is restarted fast enough, the PDR can
rewrite locator_addr fields concurrently. Protect them by placing
modification of those fields under the main pdr->lock.

Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart helpers")
Tested-by: Neil Armstrong  # on SM8550-QRD
Tested-by: Steev Klimaszewski 
Tested-by: Alexey Minnekhanov 
Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Chris Lew 

Re: [PATCH 1/2] soc: qcom: add missing pd-mapper dependencies

[Chris Lew]

On 6/26/2024 12:12 PM, Dmitry Baryshkov wrote:

The pd-mapper driver uses auxiliary bus and Qualcomm PDR message format
data. Add missing dependencies to the driver's Kconfig entry.

Reported-by: Mark Brown 
Fixes: 1ebcde047c54 ("soc: qcom: add pd-mapper implementation")
Signed-off-by: Dmitry Baryshkov 
---
  drivers/soc/qcom/Kconfig | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 0a2f2bfd7863..432c85bd8ad4 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -75,6 +75,8 @@ config QCOM_OCMEM
  config QCOM_PD_MAPPER
tristate "Qualcomm Protection Domain Mapper"
select QCOM_QMI_HELPERS
+   select QCOM_PDR_MSG
+   select AUXILIARY_BUS
depends on NET && QRTR
default QCOM_RPROC_COMMON
        help




Reviewed-by: Chris Lew 

Re: [PATCH 2/2] remoteproc: qcom: select AUXILIARY_BUS

[Chris Lew]

On 6/26/2024 12:12 PM, Dmitry Baryshkov wrote:

The QCOM_PD_MAPPER implementation made Qualcomm remoteproc drivers use
auxiliary bus for the pd-mapper subdevice. Add necessary dependency.

Reported-by: Mark Brown 
Fixes: 5b9f51b200dc ("remoteproc: qcom: enable in-kernel PD mapper")
Signed-off-by: Dmitry Baryshkov 
---
  drivers/remoteproc/Kconfig | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..dda2ada215b7 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -166,6 +166,7 @@ config QCOM_PIL_INFO
  
  config QCOM_RPROC_COMMON

tristate
+   select AUXILIARY_BUS
  
  config QCOM_Q6V5_COMMON

tristate




Reviewed-by: Chris Lew 

Re: [PATCH 1/3] rpmsg: glink: Tidy up RX advance handling

[Chris Lew]

On 8/5/2024 8:56 PM, Bjorn Andersson wrote:

The operation of advancing the FIFO receive pointer is sprinkled between
the interrupt handler itself, and functions being called from this.

Push all the RX advancement operations to the individual handlers, to
unify the style across the handling of the various messages.

Signed-off-by: Bjorn Andersson 
---
  drivers/rpmsg/qcom_glink_native.c | 22 +++---
  1 file changed, 15 insertions(+), 7 deletions(-)



Reviewed-by: Chris Lew 

Re: [PATCH 2/3] rpmsg: glink: Pass channel to qcom_glink_send_close_ack()

[Chris Lew]

On 8/5/2024 8:56 PM, Bjorn Andersson wrote:

Align the qcom_glink_send_close_ack() arguments with other functions to
take the struct glink_channel, so that the upcoming tracepoint patch can
access the channel attributes.

Signed-off-by: Bjorn Andersson 
---
  drivers/rpmsg/qcom_glink_native.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)



Reviewed-by: Chris Lew 

Re: [PATCH 3/3] rpmsg: glink: Introduce packet tracepoints

[Chris Lew]

On 8/5/2024 8:56 PM, Bjorn Andersson wrote:

Introduce tracepoints to allow tracing the GLINK packets being exchanged
with other subsystems. This is useful for debugging both interaction
with remote processors and client driver issues, as well as tracking
latency through the communication stack.

Channel events are traced with both local and remote channel ids, as
well as the textual representation when possible. The channel ids are
useful when matching traces with traces from the firmware side logs,
while the textual representation is necessary to identify channels when
we're starting to trace an already running system.

Signed-off-by: Bjorn Andersson 
---
  drivers/rpmsg/Makefile|   1 +
  drivers/rpmsg/qcom_glink_native.c |  96 -
  drivers/rpmsg/qcom_glink_trace.h  | 406 ++
  3 files changed, 501 insertions(+), 2 deletions(-)



Reviewed-by: Chris Lew 

Re: [PATCH v2] net: qrtr: Expose tunneling endpoint to user space

[Chris Lew]

On 4/23/2018 2:46 PM, Bjorn Andersson wrote:

This implements a misc character device named "qrtr-tun" for the purpose
of allowing user space applications to implement endpoints in the qrtr
network.

This allows more advanced (and dynamic) testing of the qrtr code as well
as opens up the ability of tunneling qrtr over a network or USB link.

Signed-off-by: Bjorn Andersson 


Acked-by: Chris Lew 


+static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+   struct file *filp = iocb->ki_filp;
+   struct qrtr_tun *tun = filp->private_data;
+   struct sk_buff *skb;
+   int count;
+
+   while (!(skb = skb_dequeue(&tun->queue))) {
+   if (filp->f_flags & O_NONBLOCK)
+   return -EAGAIN;
+
+   /* Wait until we get data or the endpoint goes away */
+   if (wait_event_interruptible(tun->readq,
+!skb_queue_empty(&tun->queue)))
+   return -ERESTARTSYS;
+   }
+
+   count = min_t(size_t, iov_iter_count(to), skb->len);
+   if (copy_to_iter(skb->data, count, to) != count)
+   count = -EFAULT;
+
+   kfree_skb(skb);


Is it better to use consume_skb() since this is the expected behavior path?


+
+   return count;
+}
+


Thanks,
Chris

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

[PATCH v3 6/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.

Signed-off-by: Chris Lew 
---

Changes since v2:
- Remove const on attribute struct

Changes since v1:
- Add const to attribute struct
- Get name from glink channel

 drivers/rpmsg/qcom_glink_native.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 1418926f6110..c2983a53058f 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1552,6 +1552,22 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static ssize_t rpmsg_name_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+   return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", channel->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {
+   &dev_attr_rpmsg_name.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
 static void qcom_glink_device_release(struct device *dev)
 {
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
@@ -1601,6 +1617,8 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
return ERR_PTR(-ENOMEM);
 
glink->dev = dev;
+   glink->dev->groups = qcom_glink_groups;
+
glink->tx_pipe = tx;
glink->rx_pipe = rx;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 5/6] rpmsg: glink: Add support for rpmsg glink chrdev

[Chris Lew]

RPMSG provides a char device interface to userspace. Probe the rpmsg
chrdev channel to enable the rpmsg_ctrl device creation on glink
transports.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Use qcom_glink_alloc_channel to create the chrdev rpmsg device

 drivers/rpmsg/qcom_glink_native.c | 40 ++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 120d6b9bb9f3..1418926f6110 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1166,7 +1166,7 @@ static int qcom_glink_announce_create(struct rpmsg_device 
*rpdev)
__be32 *val = defaults;
int size;
 
-   if (glink->intentless)
+   if (glink->intentless || !completion_done(&channel->open_ack))
return 0;
 
prop = of_find_property(np, "qcom,intents", NULL);
@@ -1552,6 +1552,40 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static void qcom_glink_device_release(struct device *dev)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+   /* Release qcom_glink_alloc_channel() reference */
+   kref_put(&channel->refcount, qcom_glink_channel_release);
+   kfree(rpdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+   struct rpmsg_device *rpdev;
+   struct glink_channel *channel;
+
+   rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+   if (!rpdev)
+   return -ENOMEM;
+
+   channel = qcom_glink_alloc_channel(glink, "rpmsg_chrdev");
+   if (IS_ERR(channel)) {
+   kfree(rpdev);
+   return PTR_ERR(channel);
+   }
+   channel->rpdev = rpdev;
+
+   rpdev->ept = &channel->ept;
+   rpdev->ops = &glink_device_ops;
+   rpdev->dev.parent = glink->dev;
+   rpdev->dev.release = qcom_glink_device_release;
+
+   return rpmsg_chrdev_register_device(rpdev);
+}
+
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
   unsigned long features,
   struct qcom_glink_pipe *rx,
@@ -1611,6 +1645,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
if (ret)
return ERR_PTR(ret);
 
+   ret = qcom_glink_create_chrdev(glink);
+   if (ret)
+   dev_err(glink->dev, "failed to register chrdev\n");
+
return glink;
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/6] rpmsg: glink: Store edge name for glink device

[Chris Lew]

Channels may need to identify the edge their channel was probed for.
Store the edge name by reading the label property from device tree or
default to the node name.

Signed-off-by: Chris Lew 
---

Changes since v1:
- None

 drivers/rpmsg/qcom_glink_native.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 768ef542a841..c3f548e2ff20 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -101,6 +101,8 @@ struct glink_core_rx_intent {
 struct qcom_glink {
struct device *dev;
 
+   const char *name;
+
struct mbox_client mbox_client;
struct mbox_chan *mbox_chan;
 
@@ -1580,6 +1582,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
idr_init(&glink->lcids);
idr_init(&glink->rcids);
 
+   ret = of_property_read_string(dev->of_node, "label", &glink->name);
+   if (ret < 0)
+   glink->name = dev->of_node->name;
+
glink->mbox_client.dev = dev;
glink->mbox_client.knows_txdone = true;
glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

There are GLINK clients who open the same channel on multiple GLINK
links. These clients need a way to distinguish which remoteproc they
are communicating to. Add a label property to identify the edge this
node represents.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Add explanation for label in commit message

 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index 9663cab52246..0b8cc533ca83 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -10,6 +10,11 @@ edge.
Value type: 
Definition: must be "qcom,glink-rpm"
 
+- label:
+   Usage: optional
+   Value type: 
+   Definition: should specify the subsystem name this edge corresponds to.
+
 - interrupts:
Usage: required
Value type: 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/6] rpmsg: glink: Use complete_all for open states

[Chris Lew]

The open_req and open_ack completion variables are the state variables
to represet a remote channel as open. Use complete_all so there are no
races with waiters and using completion_done.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

 drivers/rpmsg/qcom_glink_native.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index c3f548e2ff20..120d6b9bb9f3 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -957,7 +957,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, 
unsigned int lcid)
return -EINVAL;
}
 
-   complete(&channel->open_ack);
+   complete_all(&channel->open_ack);
 
return 0;
 }
@@ -1401,7 +1401,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, 
unsigned int rcid,
channel->rcid = ret;
spin_unlock_irqrestore(&glink->idr_lock, flags);
 
-   complete(&channel->open_req);
+   complete_all(&channel->open_req);
 
if (create_device) {
rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 0/6] Add chrdev and name query support for GLINK

[Chris Lew]

Add support for the GLINK rpmsg transport to register a rpmsg chrdev.
This will create the rpmsg_ctrl nodes for userspace clients to open 
rpmsg epts. Create a label property that will help userspace clients
distinguish between the different GLINK links. The rpmsg chrdev 
allocation is done by allocating a local channel which also allocates
an ept. We need to add some guards against edge cases for this chrdev
because it will never fully open.

Changes since v2:
- Revert change to make glink attribute table const

Changes since v1:
- Add explanation to dt-bindings commit message
- Add patch complete_all the open_req/ack variables
- Add patch to prevent null pointer dereference in chrdev channel release
- Change chrdev allocation to use glink channel allocation
- Change glink attr struct to const

Chris Lew (6):
  dt-bindings: soc: qcom: Add label for GLINK bindings
  rpmsg: glink: Store edge name for glink device
  rpmsg: glink: Use complete_all for open states
  rpmsg: Guard against null endpoint ops in destroy
  rpmsg: glink: Add support for rpmsg glink chrdev
  rpmsg: glink: Expose rpmsg name attr for glink

 .../devicetree/bindings/soc/qcom/qcom,glink.txt|  5 ++
 drivers/rpmsg/qcom_glink_native.c  | 68 +-
 drivers/rpmsg/rpmsg_core.c |  2 +-
 3 files changed, 71 insertions(+), 4 deletions(-)

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

[PATCH v3 4/6] rpmsg: Guard against null endpoint ops in destroy

[Chris Lew]

In RPMSG GLINK the chrdev device will allocate an ept as part of the
rpdev creation. This device will not register endpoint ops even though
it has an allocated ept. Protect against the case where the device is
being destroyed.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 920a02f0462c..7bfe36afccc5 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device 
*rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-   if (ept)
+   if (ept && ept->ops)
ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] soc: qcom: smem: introduce qcom_smem_virt_to_phys()

[Chris Lew]

Hi Alex,

Minor comment.

On 4/25/2018 8:18 AM, Alex Elder wrote:

Create function qcom_smem_virt_to_phys(), which returns the physical
address corresponding to a given SMEM item's virtual address.  This
feature is required for a driver that will soon be out for review.

Signed-off-by: Alex Elder 
---
  drivers/soc/qcom/smem.c   | 27 +++
  include/linux/soc/qcom/smem.h |  2 ++
  2 files changed, 29 insertions(+)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 7d9a43da5084..70b2ee80d6bd 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -655,6 +655,33 @@ int qcom_smem_get_free_space(unsigned host)
  }
  EXPORT_SYMBOL(qcom_smem_get_free_space);
  
+/**

+ * qcom_smem_virt_to_phys() - return the physical address associated
+ * with an smem item pointer (previously returned by qcom_smem_get()
+ * @p: the virtual address to convert
+ *
+ * Returns 0 if the pointer provided is not within any smem region.
+ */
+phys_addr_t qcom_smem_virt_to_phys(void *p)
+{
+   unsigned i;
+


We have a null pointer check for __smem here since it is called by 
external clients. This case should probably never happen though.



+   for (i = 0; i < __smem->num_regions; i++) {
+   struct smem_region *region = &__smem->regions[i];
+
+   if (p < region->virt_base)
+   continue;
+   if (p < region->virt_base + region->size) {
+   u64 offset = p - region->virt_base;
+
+   return (phys_addr_t)region->aux_base + offset;
+   }
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(qcom_smem_virt_to_phys);


Thanks,
Chris

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

[PATCH v2 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

There are GLINK clients who open the same channel on multiple GLINK
links. These clients need a way to distinguish which remoteproc they
are communicating to. Add a label property to identify the edge this
node represents.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Add explanation for label in commit message

 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index 9663cab52246..0b8cc533ca83 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -10,6 +10,11 @@ edge.
Value type: 
Definition: must be "qcom,glink-rpm"
 
+- label:
+   Usage: optional
+   Value type: 
+   Definition: should specify the subsystem name this edge corresponds to.
+
 - interrupts:
Usage: required
Value type: 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/6] rpmsg: glink: Store edge name for glink device

[Chris Lew]

Channels may need to identify the edge their channel was probed for.
Store the edge name by reading the label property from device tree or
default to the node name.

Signed-off-by: Chris Lew 
---

Changes since v1:
- None

 drivers/rpmsg/qcom_glink_native.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 768ef542a841..c3f548e2ff20 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -101,6 +101,8 @@ struct glink_core_rx_intent {
 struct qcom_glink {
struct device *dev;
 
+   const char *name;
+
struct mbox_client mbox_client;
struct mbox_chan *mbox_chan;
 
@@ -1580,6 +1582,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
idr_init(&glink->lcids);
idr_init(&glink->rcids);
 
+   ret = of_property_read_string(dev->of_node, "label", &glink->name);
+   if (ret < 0)
+   glink->name = dev->of_node->name;
+
glink->mbox_client.dev = dev;
glink->mbox_client.knows_txdone = true;
glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/6] rpmsg: glink: Use complete_all for open states

[Chris Lew]

The open_req and open_ack completion variables are the state variables
to represet a remote channel as open. Use complete_all so there are no
races with waiters and using completion_done.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

 drivers/rpmsg/qcom_glink_native.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index c3f548e2ff20..120d6b9bb9f3 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -957,7 +957,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, 
unsigned int lcid)
return -EINVAL;
}
 
-   complete(&channel->open_ack);
+   complete_all(&channel->open_ack);
 
return 0;
 }
@@ -1401,7 +1401,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, 
unsigned int rcid,
channel->rcid = ret;
spin_unlock_irqrestore(&glink->idr_lock, flags);
 
-   complete(&channel->open_req);
+   complete_all(&channel->open_req);
 
if (create_device) {
rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 6/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Add const to attribute struct
- Get name from glink channel

 drivers/rpmsg/qcom_glink_native.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 1418926f6110..72240614d4ca 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1552,6 +1552,22 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static ssize_t rpmsg_name_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+   return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", channel->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static const struct attribute *qcom_glink_attrs[] = {
+   &dev_attr_rpmsg_name.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
 static void qcom_glink_device_release(struct device *dev)
 {
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
@@ -1601,6 +1617,8 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
return ERR_PTR(-ENOMEM);
 
glink->dev = dev;
+   glink->dev->groups = qcom_glink_groups;
+
glink->tx_pipe = tx;
glink->rx_pipe = rx;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/6] rpmsg: Guard against null endpoint ops in destroy

[Chris Lew]

In RPMSG GLINK the chrdev device will allocate an ept as part of the
rpdev creation. This device will not register endpoint ops even though
it has an allocated ept. Protect against the case where the device is
being destroyed.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 920a02f0462c..7bfe36afccc5 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device 
*rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-   if (ept)
+   if (ept && ept->ops)
ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/6] Add chrdev and name query support for GLINK

[Chris Lew]

Add support for the GLINK rpmsg transport to register a rpmsg chrdev.
This will create the rpmsg_ctrl nodes for userspace clients to open 
rpmsg epts. Create a label property that will help userspace clients
distinguish between the different GLINK links. The rpmsg chrdev 
allocation is done by allocating a local channel which also allocates
an ept. We need to add some guards against edge cases for this chrdev
because it will never fully open.

Changes since v1:
- Add explanation to dt-bindings commit message
- Add patch complete_all the open_req/ack variables
- Add patch to prevent null pointer dereference in chrdev channel release
- Change chrdev allocation to use glink channel allocation
- Change glink attr struct to const

Chris Lew (6):
  dt-bindings: soc: qcom: Add label for GLINK bindings
  rpmsg: glink: Store edge name for glink device
  rpmsg: glink: Use complete_all for open states
  rpmsg: Guard against null endpoint ops in destroy
  rpmsg: glink: Add support for rpmsg glink chrdev
  rpmsg: glink: Expose rpmsg name attr for glink

 .../devicetree/bindings/soc/qcom/qcom,glink.txt|  5 ++
 drivers/rpmsg/qcom_glink_native.c  | 68 +-
 drivers/rpmsg/rpmsg_core.c |  2 +-
 3 files changed, 71 insertions(+), 4 deletions(-)

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

[PATCH v2 5/6] rpmsg: glink: Add support for rpmsg glink chrdev

[Chris Lew]

RPMSG provides a char device interface to userspace. Probe the rpmsg
chrdev channel to enable the rpmsg_ctrl device creation on glink
transports.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Use qcom_glink_alloc_channel to create the chrdev rpmsg device

 drivers/rpmsg/qcom_glink_native.c | 40 ++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 120d6b9bb9f3..1418926f6110 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1166,7 +1166,7 @@ static int qcom_glink_announce_create(struct rpmsg_device 
*rpdev)
__be32 *val = defaults;
int size;
 
-   if (glink->intentless)
+   if (glink->intentless || !completion_done(&channel->open_ack))
return 0;
 
prop = of_find_property(np, "qcom,intents", NULL);
@@ -1552,6 +1552,40 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static void qcom_glink_device_release(struct device *dev)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+   /* Release qcom_glink_alloc_channel() reference */
+   kref_put(&channel->refcount, qcom_glink_channel_release);
+   kfree(rpdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+   struct rpmsg_device *rpdev;
+   struct glink_channel *channel;
+
+   rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+   if (!rpdev)
+   return -ENOMEM;
+
+   channel = qcom_glink_alloc_channel(glink, "rpmsg_chrdev");
+   if (IS_ERR(channel)) {
+   kfree(rpdev);
+   return PTR_ERR(channel);
+   }
+   channel->rpdev = rpdev;
+
+   rpdev->ept = &channel->ept;
+   rpdev->ops = &glink_device_ops;
+   rpdev->dev.parent = glink->dev;
+   rpdev->dev.release = qcom_glink_device_release;
+
+   return rpmsg_chrdev_register_device(rpdev);
+}
+
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
   unsigned long features,
   struct qcom_glink_pipe *rx,
@@ -1611,6 +1645,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
if (ret)
return ERR_PTR(ret);
 
+   ret = qcom_glink_create_chrdev(glink);
+   if (ret)
+   dev_err(glink->dev, "failed to register chrdev\n");
+
return glink;
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V7 2/4] rpmsg: Guard against null endpoint ops in destroy

[Chris Lew]

Hi Greg,

On 8/7/2020 12:59 AM, Greg KH wrote:

On Wed, Jun 24, 2020 at 10:15:19PM +0530, Deepak Kumar Singh wrote:

From: Chris Lew 

In RPMSG GLINK the chrdev device will allocate an ept as part of the
rpdev creation. This device will not register endpoint ops even though
it has an allocated ept. Protect against the case where the device is
being destroyed.

Signed-off-by: Chris Lew 
Signed-off-by: Deepak Kumar Singh 
Signed-off-by: Arun Kumar Neelakantam 


Should this be marked for stable kernels?

And if so, what commit does this fix?  Any reason the Fixes: tag was not
used here?



The crash that this fixes doesn't show up unless one of the previous 
patches in the series is applied.


[PATCH V6 3/5] rpmsg: glink: Add support for rpmsg glink chrdev

I'm not sure if the fixes tag should apply to this change or one of the 
commits to the base rpmsg code.



And what happened to this series?  I don't see it in linux-next, did the
maintainer ignore it?



I believe most of the review feedback for the series has been addressed 
by Deepak. There is one remaining action item for me and Deepak to 
provide more concrete evidence that the first patch in the series is 
needed.


[PATCH V6 1/5] rpmsg: glink: Use complete_all for open states


thanks,

greg k-h



Thanks,
Chris

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

Re: [PATCH 1/2] soc: qcom: aoss: Expose send for generic usecase

[Chris Lew]

On 11/11/2020 10:33 AM, Sibi Sankar wrote:

Hey Chris,
Thanks for the patch.

On 2020-11-03 08:49, Chris Lew wrote:

Not all upcoming usecases will have an interface to allow the aoss
driver to hook onto. Expose the send api and create a get function to
enable drivers to send their own messages to aoss.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/qcom_aoss.c   | 28 +++-
 include/linux/soc/qcom/qcom_aoss.h | 33 
+

 2 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/soc/qcom/qcom_aoss.h

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index ed2c687c16b3..8f052db1880a 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -8,10 +8,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 

 #define QMP_DESC_MAGIC    0x0
 #define QMP_DESC_VERSION    0x4
@@ -222,12 +224,15 @@ static bool qmp_message_empty(struct qmp *qmp)
  *
  * Return: 0 on success, negative errno on failure
  */
-static int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
 {
 long time_left;
 size_t tlen;
 int ret;

+    if (!qmp || !data)
+    return -EINVAL;
+
 if (WARN_ON(len + sizeof(u32) > qmp->size))
 return -EINVAL;

@@ -261,6 +266,7 @@ static int qmp_send(struct qmp *qmp, const void
*data, size_t len)

 return ret;
 }
+EXPORT_SYMBOL_GPL(qmp_send);

 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {
@@ -515,6 +521,26 @@ static void qmp_cooling_devices_remove(struct qmp 
*qmp)

 thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
 }

+/**
+ * qmp_get() - get a qmp handle from device tree node
+ * @np: of node of qmp device
+ *
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp_device *qmp_get(struct device_node *np)
+{
+    struct platform_device *pdev;
+    struct qmp *qmp;


Can we use this patch series to determine
the binding the client are expected to use
to point to the qmp phandle and have it
parsed here? This would mean that qmp_get
would take in device as input instead.
Bjorn suggested that clients use "qcom,qmp"
during an offline discussion. Let me know
what you think.



Hey Sibi,

Yea I think that's a better implementation. I'll upload a second 
revision using "qcom,qmp" as the binding.



+
+    pdev = of_find_device_by_node(np);
+    if (!pdev)
+    return ERR_PTR(-EINVAL);
+
+    qmp = platform_get_drvdata(pdev);
+    return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
 struct resource *res;
diff --git a/include/linux/soc/qcom/qcom_aoss.h
b/include/linux/soc/qcom/qcom_aoss.h
new file mode 100644
index ..05fc0ed3a10d
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_AOSS_H__
+#define __QCOM_AOSS_H__
+
+#include 
+#include 
+
+struct qmp;
+
+#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len);
+struct qmp_device *qmp_get(struct device_node *np);
+
+#else
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+    return -ENODEV;
+}
+
+struct qmp *qmp_get(struct device_node *np)
+{
+    return ERR_PTR(-ENODEV);
+}
+
+#endif
+
+#endif




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

Re: [PATCH v2 1/6] rpmsg: glink: Fix reuse intents memory leak issue

[Chris Lew]

On 10/4/2019 3:26 PM, Bjorn Andersson wrote:

From: Arun Kumar Neelakantam 

Memory allocated for re-usable intents are not freed during channel
cleanup which causes memory leak in system.

Check and free all re-usable memory to avoid memory leak.

Fixes: 933b45da5d1d ("rpmsg: glink: Add support for TX intents")
Cc: sta...@vger.kernel.org
Tested-by: Srinivas Kandagatla 
Signed-off-by: Arun Kumar Neelakantam 
Reported-by: Srinivas Kandagatla 
Signed-off-by: Bjorn Andersson 
---


Acked-By: Chris Lew 

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

Re: [PATCH v2 2/6] rpmsg: glink: Fix use after free in open_ack TIMEOUT case

[Chris Lew]

On 10/4/2019 3:26 PM, Bjorn Andersson wrote:

From: Arun Kumar Neelakantam 

Extra channel reference put when remote sending OPEN_ACK after timeout
causes use-after-free while handling next remote CLOSE command.

Remove extra reference put in timeout case to avoid use-after-free.

Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver")
Cc: sta...@vger.kernel.org
Tested-by: Srinivas Kandagatla 
Signed-off-by: Arun Kumar Neelakantam 
Signed-off-by: Bjorn Andersson 
---


Acked-By: Chris Lew 

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

Re: [PATCH v2 5/6] rpmsg: glink: Don't send pending rx_done during remove

[Chris Lew]

On 10/4/2019 3:27 PM, Bjorn Andersson wrote:

Attempting to transmit rx_done messages after the GLINK instance is
being torn down will cause use after free and memory leaks. So cancel
the intent_work and free up the pending intents.

With this there are no concurrent accessors of the channel left during
qcom_glink_native_remove() and there is therefor no need to hold the
spinlock during this operation - which would prohibit the use of
cancel_work_sync() in the release function. So remove this.

Fixes: 1d2ea36eead9 ("rpmsg: glink: Add rx done command")
Cc: sta...@vger.kernel.org
Tested-by: Srinivas Kandagatla 
Signed-off-by: Bjorn Andersson 
---


Acked-By: Chris Lew 

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

Re: [PATCH v2 6/6] rpmsg: glink: Free pending deferred work on remove

[Chris Lew]

On 10/4/2019 3:27 PM, Bjorn Andersson wrote:

By just cancelling the deferred rx worker during GLINK instance teardown
any pending deferred commands are leaked, so free them.

Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver")
Cc: sta...@vger.kernel.org
Tested-by: Srinivas Kandagatla 
Signed-off-by: Bjorn Andersson 
---

Acked-By: Chris Lew 

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

[PATCH] rpmsg: glink: Remove channel decouple from rpdev release

[Chris Lew]

If a channel is being rapidly restarted and the kobj release worker is
busy, there is a chance the the rpdev_release function will run after
the channel struct itself has been released.

There should not be a need to decouple the channel from rpdev in the
rpdev release since that should only happen from the channel close
commands.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 621f1afd4d6b..836a0bd99d11 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1350,9 +1350,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops 
= {
 static void qcom_glink_rpdev_release(struct device *dev)
 {
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
-   struct glink_channel *channel = to_glink_channel(rpdev->ept);
 
-   channel->rpdev = NULL;
kfree(rpdev);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] rpmsg: glink: Remove channel decouple from rpdev release

[Chris Lew]

On 10/9/2019 10:04 PM, Stephen Boyd wrote:

Quoting Chris Lew (2019-10-08 18:33:45)

If a channel is being rapidly restarted and the kobj release worker is
busy, there is a chance the the rpdev_release function will run after
the channel struct itself has been released.

There should not be a need to decouple the channel from rpdev in the
rpdev release since that should only happen from the channel close
commands.

Signed-off-by: Chris Lew 


Fixes tag? The whole thing sounds broken and probably is still racy in
the face of SMP given that channel->rpdev is tested for "published" or
not. Can you describe the race that you're closing more?



Thanks Stephen, will add Fixes tag and try to describe the race better.

I agree that the whole thing sounds broken, the glink channel cleanup 
code has a couple bugs that need to be addressed in a more extensive 
patch. This patch is more to address the immediate issue of a 
use-after-free from one of the races.


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

[PATCH] soc: qcom: qmi: Change txn wait to non-interruptible

[Chris Lew]

Current QMI clients are not userspace facing, if their threads are
signaled, they do not do any signal checking or propagate the
ERESTARTSYS return code up. Remove the interruptible option so clients
can finish their QMI transactions even if the thread is signaled.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/qmi_interface.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
index c239a28e503f..f9e309f0acd3 100644
--- a/drivers/soc/qcom/qmi_interface.c
+++ b/drivers/soc/qcom/qmi_interface.c
@@ -345,8 +345,7 @@ int qmi_txn_wait(struct qmi_txn *txn, unsigned long timeout)
struct qmi_handle *qmi = txn->qmi;
int ret;
 
-   ret = wait_for_completion_interruptible_timeout(&txn->completion,
-   timeout);
+   ret = wait_for_completion_timeout(&txn->completion, timeout);
 
mutex_lock(&qmi->txn_lock);
mutex_lock(&txn->lock);
@@ -354,9 +353,7 @@ int qmi_txn_wait(struct qmi_txn *txn, unsigned long timeout)
mutex_unlock(&txn->lock);
mutex_unlock(&qmi->txn_lock);
 
-   if (ret < 0)
-   return ret;
-   else if (ret == 0)
+   if (ret == 0)
return -ETIMEDOUT;
else
return txn->result;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] net: qrtr: Allocate workqueue before kernel_bind

[Chris Lew]

A null pointer dereference in qrtr_ns_data_ready() is seen if a client
opens a qrtr socket before qrtr_ns_init() can bind to the control port.
When the control port is bound, the ENETRESET error will be broadcasted
and clients will close their sockets. This results in DEL_CLIENT
packets being sent to the ns and qrtr_ns_data_ready() being called
without the workqueue being allocated.

Allocate the workqueue before setting sk_data_ready and binding to the
control port. This ensures that the work and workqueue structs are
allocated and initialized before qrtr_ns_data_ready can be called.

Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Signed-off-by: Chris Lew 
---
 net/qrtr/ns.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index e7d0fe3f4330..c5b3202a14ca 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -712,6 +712,10 @@ void qrtr_ns_init(void)
goto err_sock;
}
 
+   qrtr_ns.workqueue = alloc_workqueue("qrtr_ns_handler", WQ_UNBOUND, 1);
+   if (!qrtr_ns.workqueue)
+   goto err_sock;
+
qrtr_ns.sock->sk->sk_data_ready = qrtr_ns_data_ready;
 
sq.sq_port = QRTR_PORT_CTRL;
@@ -720,17 +724,13 @@ void qrtr_ns_init(void)
ret = kernel_bind(qrtr_ns.sock, (struct sockaddr *)&sq, sizeof(sq));
if (ret < 0) {
pr_err("failed to bind to socket\n");
-   goto err_sock;
+   goto err_wq;
}
 
qrtr_ns.bcast_sq.sq_family = AF_QIPCRTR;
qrtr_ns.bcast_sq.sq_node = QRTR_NODE_BCAST;
qrtr_ns.bcast_sq.sq_port = QRTR_PORT_CTRL;
 
-   qrtr_ns.workqueue = alloc_workqueue("qrtr_ns_handler", WQ_UNBOUND, 1);
-   if (!qrtr_ns.workqueue)
-   goto err_sock;
-
ret = say_hello(&qrtr_ns.bcast_sq);
if (ret < 0)
goto err_wq;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] soc: qcom: aoss: Expose send for generic usecase

[Chris Lew]

Not all upcoming usecases will have an interface to allow the aoss
driver to hook onto. Expose the send api and create a get function to
enable drivers to send their own messages to aoss.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/qcom_aoss.c   | 28 +++-
 include/linux/soc/qcom/qcom_aoss.h | 33 +
 2 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/soc/qcom/qcom_aoss.h

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index ed2c687c16b3..8f052db1880a 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -8,10 +8,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 #define QMP_DESC_MAGIC 0x0
 #define QMP_DESC_VERSION   0x4
@@ -222,12 +224,15 @@ static bool qmp_message_empty(struct qmp *qmp)
  *
  * Return: 0 on success, negative errno on failure
  */
-static int qmp_send(struct qmp *qmp, const void *data, size_t len)
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
 {
long time_left;
size_t tlen;
int ret;
 
+   if (!qmp || !data)
+   return -EINVAL;
+
if (WARN_ON(len + sizeof(u32) > qmp->size))
return -EINVAL;
 
@@ -261,6 +266,7 @@ static int qmp_send(struct qmp *qmp, const void *data, 
size_t len)
 
return ret;
 }
+EXPORT_SYMBOL_GPL(qmp_send);
 
 static int qmp_qdss_clk_prepare(struct clk_hw *hw)
 {
@@ -515,6 +521,26 @@ static void qmp_cooling_devices_remove(struct qmp *qmp)
thermal_cooling_device_unregister(qmp->cooling_devs[i].cdev);
 }
 
+/**
+ * qmp_get() - get a qmp handle from device tree node
+ * @np: of node of qmp device
+ *
+ * Return: handle to qmp device on success, ERR_PTR() on failure
+ */
+struct qmp_device *qmp_get(struct device_node *np)
+{
+   struct platform_device *pdev;
+   struct qmp *qmp;
+
+   pdev = of_find_device_by_node(np);
+   if (!pdev)
+   return ERR_PTR(-EINVAL);
+
+   qmp = platform_get_drvdata(pdev);
+   return qmp ? qmp : ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(qmp_get);
+
 static int qmp_probe(struct platform_device *pdev)
 {
struct resource *res;
diff --git a/include/linux/soc/qcom/qcom_aoss.h 
b/include/linux/soc/qcom/qcom_aoss.h
new file mode 100644
index ..05fc0ed3a10d
--- /dev/null
+++ b/include/linux/soc/qcom/qcom_aoss.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_AOSS_H__
+#define __QCOM_AOSS_H__
+
+#include 
+#include 
+
+struct qmp;
+
+#if IS_ENABLED(CONFIG_QCOM_AOSS_QMP)
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len);
+struct qmp_device *qmp_get(struct device_node *np);
+
+#else
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+   return -ENODEV;
+}
+
+struct qmp *qmp_get(struct device_node *np)
+{
+   return ERR_PTR(-ENODEV);
+}
+
+#endif
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] soc: qcom: aoss: Add debugfs send entry

[Chris Lew]

It can be useful to control the different power states of various
parts of hardware for device testing. Add a debugfs node to send
messages through qmp to aoss for debugging and testing purposes.

Signed-off-by: Chris Lew 
---
 drivers/soc/qcom/qcom_aoss.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index 8f052db1880a..2fd755d2a92d 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -4,6 +4,7 @@
  */
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -85,6 +86,8 @@ struct qmp {
struct clk_hw qdss_clk;
struct genpd_onecell_data pd_data;
struct qmp_cooling_device *cooling_devs;
+
+   struct dentry *debugfs_fp;
 };
 
 struct qmp_pd {
@@ -541,6 +544,34 @@ struct qmp_device *qmp_get(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(qmp_get);
 
+static ssize_t aoss_dbg_write(struct file *file, const char __user *userstr,
+ size_t len, loff_t *pos)
+{
+   struct qmp *qmp = file->private_data;
+   char buf[QMP_MSG_LEN] = {};
+   int ret;
+
+   if (!len || len >= QMP_MSG_LEN)
+   return len;
+
+   ret  = copy_from_user(buf, userstr, len);
+   if (ret) {
+   dev_err(qmp->dev, "copy from user failed, ret:%d\n", ret);
+   return len;
+   }
+
+   ret = qmp_send(qmp, buf, QMP_MSG_LEN);
+   if (ret)
+   dev_err(qmp->dev, "debug send failed, ret:%d\n", ret);
+
+   return len;
+}
+
+static const struct file_operations aoss_dbg_fops = {
+   .open = simple_open,
+   .write = aoss_dbg_write,
+};
+
 static int qmp_probe(struct platform_device *pdev)
 {
struct resource *res;
@@ -595,6 +626,9 @@ static int qmp_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, qmp);
 
+   qmp->debugfs_fp = debugfs_create_file("aoss_send_message", 0220, NULL,
+ qmp, &aoss_dbg_fops);
+
return 0;
 
 err_remove_qdss_clk:
@@ -611,6 +645,8 @@ static int qmp_remove(struct platform_device *pdev)
 {
struct qmp *qmp = platform_get_drvdata(pdev);
 
+   debugfs_remove(qmp->debugfs_fp);
+
qmp_qdss_clk_remove(qmp);
qmp_pd_remove(qmp);
qmp_cooling_devices_remove(qmp);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] rpmsg: glink: Use spinlock in tx path

[Chris Lew]

Hey Bjorn,

Minor issue with the glink patch.

Thanks,
Chris

On 2/13/2018 11:04 AM, Bjorn Andersson wrote:

[..]

@@ -288,15 +288,14 @@ static int qcom_glink_tx(struct qcom_glink *glink,
 const void *data, size_t dlen, bool wait)
  {
unsigned int tlen = hlen + dlen;
+   unsigned long flags;
int ret;
  
  	/* Reject packets that are too big */

if (tlen >= glink->tx_pipe->length)
return -EINVAL;
  
-	ret = mutex_lock_interruptible(&glink->tx_lock);

-   if (ret)
-   return ret;
+   spin_lock_irqsave(&glink->tx_lock, flags);
  


qcom_glink_tx will return an uninitialized ret value with removal of 
mutex_lock_interruptible.


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

Re: [PATCH v3 1/5] soc: qcom: Introduce QMI encoder/decoder

[Chris Lew]

On 11/15/2017 9:42 PM, Bjorn Andersson wrote:

On Wed 15 Nov 12:10 PST 2017, Bjorn Andersson wrote:

diff --git a/drivers/soc/qcom/qmi_encdec.c b/drivers/soc/qcom/qmi_encdec.c

[..]

+void *qmi_encode_message(int type, unsigned int msg_id, size_t *len,
+unsigned int txn_id, struct qmi_elem_info *ei,
+const void *c_struct)
+{
+   struct qmi_header *hdr;
+   ssize_t msglen = 0;
+   void *msg;
+   int ret;
+
+   /* Check the possibility of a zero length QMI message */
+   if (!c_struct) {
+   ret = qmi_calc_min_msg_len(ei, 1);
+   if (ret) {
+   pr_err("%s: Calc. len %d != 0, but NULL c_struct\n",
+  __func__, ret);
+   return ERR_PTR(-EINVAL);
+   }
+   }
+
+   msg = kzalloc(sizeof(*hdr) + *len, GFP_KERNEL);
+   if (!msg)
+   return ERR_PTR(-ENOMEM);
+
+   msglen = qmi_encode(ei, msg + sizeof(*hdr), c_struct, *len, 1);
+   if (msglen < 0) {
+   kfree(msg);
+   return ERR_PTR(msglen);
+   }


Talked to Chris Lew about this earlier today;

The check above implies that it's valid to call this function with a
valid ei of minimum message length of 0 and c_struct being NULL. But the
call to qmi_encdec() will dereference c_struct in order to know that the
optional elements described in ei are unset.

So the call to qmi_encode() needs to only be done conditionally on
c_struct being non-NULL, logically interpreting c_struct being NULL as
all optional fields are unset.

Will post an update with this fixed.



I have tested this patch with QMI loopback servers, Qualcomm diag and 
slimbus ngd.


Tested-By: Chris Lew 

+
+   hdr = msg;
+   hdr->type = type;
+   hdr->txn_id = txn_id;
+   hdr->msg_id = msg_id;
+   hdr->msg_len = msglen;
+
+   *len = sizeof(*hdr) + msglen;
+
+   return msg;
+}
+EXPORT_SYMBOL(qmi_encode_message);


Regards,
Bjorn



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

Re: [PATCH v3 4/5] remoteproc: qcom: Introduce sysmon

[Chris Lew]

Hi Bjorn,

Question about the SSR events for sysmon.

Thanks,
Chris

On 11/15/2017 12:10 PM, Bjorn Andersson wrote:

[..]

+/**
+ * ssctl_send_event() - send notification of other remote's SSR event
+ * @sysmon:sysmon context
+ * @name:  other remote's name
+ */
+static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
+{
+   struct ssctl_subsys_event_resp resp;
+   struct ssctl_subsys_event_req req;
+   struct qmi_txn txn;
+   int ret;
+
+   memset(&resp, 0, sizeof(resp));
+   ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_subsys_event_resp_ei, 
&resp);
+   if (ret < 0) {
+   dev_err(sysmon->dev, "failed to allocate QMI txn\n");
+   return;
+   }
+
+   memset(&req, 0, sizeof(req));
+   strlcpy(req.subsys_name, name, sizeof(req.subsys_name));
+   req.subsys_name_len = strlen(req.subsys_name);
+   req.event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;


Are there plans to add the other SSR events to sysmon notifiers? I think 
the SSCTL service expects to receive events about remote procs starting 
as well.



+   req.evt_driven_valid = true;
+   req.evt_driven = SSCTL_SSR_EVENT_FORCED;
+
+   ret = qmi_send_request(&sysmon->qmi, &sysmon->ssctl, &txn,
+  SSCTL_SUBSYS_EVENT_REQ, 40,
+  ssctl_subsys_event_req_ei, &req);
+   if (ret < 0) {
+   dev_err(sysmon->dev, "failed to send shutdown request\n");
+   qmi_txn_cancel(&txn);
+   return;
+   }
+
+   ret = qmi_txn_wait(&txn, 5 * HZ);
+   if (ret < 0)
+   dev_err(sysmon->dev, "failed receiving QMI response\n");
+   else if (resp.resp.result)
+   dev_err(sysmon->dev, "ssr event send failed\n");
+   else
+   dev_dbg(sysmon->dev, "ssr event send completed\n");
+}


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

[PATCH 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

[Chris Lew]

Add a label property to identify the edge this node represents.

Signed-off-by: Chris Lew 
---
 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index 9663cab52246..0b8cc533ca83 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -10,6 +10,11 @@ edge.
Value type: 
Definition: must be "qcom,glink-rpm"
 
+- label:
+   Usage: optional
+   Value type: 
+   Definition: should specify the subsystem name this edge corresponds to.
+
 - interrupts:
Usage: required
Value type: 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 0/6] Add chrdev and name query support for GLINK

[Chris Lew]

Add support for GLINK rpmsg transport to register a rpmsg chrdev to
create the rpmsg_ctrl nodes for userspace clients to open rpmsg epts.
Create a label property to identify the glink node with. This label
will be used as a name attr for GLINK devices. Expose this name through
a new API - rpmsg_get_rproc_name() for kernel clients.

Chris Lew (6):
  dt-bindings: soc: qcom: Add label for GLINK bindings
  rpmsg: glink: Store edge name for glink device
  rpmsg: glink: Add support for rpmsg glink chrdev
  rpmsg: glink: Expose rpmsg name attr for glink
  rpmsg: Introduce rpmsg_get_rproc_name
  rpmsg: glink: Add get_rproc_name device op

 .../devicetree/bindings/soc/qcom/qcom,glink.txt|  5 ++
 drivers/rpmsg/qcom_glink_native.c  | 72 ++
 drivers/rpmsg/rpmsg_core.c | 21 +++
 drivers/rpmsg/rpmsg_internal.h |  3 +
 include/linux/rpmsg.h  | 10 +++
 5 files changed, 111 insertions(+)

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

[PATCH 2/6] rpmsg: glink: Store edge name for glink device

[Chris Lew]

Channels may need to identify the edge their channel was probed for.
Store the edge name by reading the label property from device tree or
default to the node name.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index cd9d643433d3..179132226dc2 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -101,6 +101,8 @@ struct glink_core_rx_intent {
 struct qcom_glink {
struct device *dev;
 
+   const char *name;
+
struct mbox_client mbox_client;
struct mbox_chan *mbox_chan;
 
@@ -1575,6 +1577,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
idr_init(&glink->lcids);
idr_init(&glink->rcids);
 
+   ret = of_property_read_string(dev->of_node, "label", &glink->name);
+   if (ret < 0)
+   glink->name = dev->of_node->name;
+
glink->mbox_client.dev = dev;
glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
if (IS_ERR(glink->mbox_chan)) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 5/6] rpmsg: Introduce rpmsg_get_rproc_name

[Chris Lew]

Add support for client's to query the edge name their channel is
registered for. This is useful for clients who share the same channel
identifier across different remote procs.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/rpmsg_core.c | 21 +
 drivers/rpmsg/rpmsg_internal.h |  3 +++
 include/linux/rpmsg.h  | 10 ++
 3 files changed, 34 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index dffa3aab7178..d6ebd678b089 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -78,6 +78,27 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device 
*rpdev,
 }
 EXPORT_SYMBOL(rpmsg_create_ept);
 
+
+/**
+ * rpmsg_get_rproc_name() - Return the name of the rproc for this device
+ * @rpdev: rpmsg channel device
+ *
+ * Expose the rproc name for clients who open the same channel across different
+ * rprocs and need to differentiate during their probe.
+ *
+ * Returns char string on success and NULL on failure.
+ */
+const char *rpmsg_get_rproc_name(struct rpmsg_device *rpdev)
+{
+   if (WARN_ON(!rpdev))
+   return NULL;
+
+   if (!rpdev->ops->get_rproc_name)
+   return NULL;
+
+   return rpdev->ops->get_rproc_name(rpdev);
+}
+
 /**
  * rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
  * @ept: endpoing to destroy
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 0cf9c7e2ee83..83a028b6883f 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -31,6 +31,7 @@
  * @create_ept:create backend-specific endpoint, requried
  * @announce_create:   announce presence of new channel, optional
  * @announce_destroy:  announce destruction of channel, optional
+ * @get_rproc_name:return name of the rproc for this device, optional
  *
  * Indirection table for the operations that a rpmsg backend should implement.
  * @announce_create and @announce_destroy are optional as the backend might
@@ -43,6 +44,8 @@ struct rpmsg_device_ops {
 
int (*announce_create)(struct rpmsg_device *ept);
int (*announce_destroy)(struct rpmsg_device *ept);
+
+   const char *(*get_rproc_name)(struct rpmsg_device *rpdev);
 };
 
 /**
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 10d6ae8bbb7d..167982dc5b4f 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -160,6 +160,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, 
u32 src, u32 dst,
 unsigned int rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
poll_table *wait);
 
+const char *rpmsg_get_rproc_name(struct rpmsg_device *dev);
+
 #else
 
 static inline int register_rpmsg_device(struct rpmsg_device *dev)
@@ -267,6 +269,14 @@ static inline unsigned int rpmsg_poll(struct 
rpmsg_endpoint *ept,
return 0;
 }
 
+static inline const char *rpmsg_get_rproc_name(struct rpmsg_device *rpdev)
+{
+   /* This shouldn't be possible */
+   WARN_ON(1);
+
+   return NULL;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG) */
 
 /* use a macro to avoid include chaining to get THIS_MODULE */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/6] rpmsg: glink: Expose rpmsg name attr for glink

[Chris Lew]

Expose the name field as an attr so clients listening to uevents for
rpmsg can identify the edge the events correspond to.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 786f2eca01f1..a897ccea3098 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1558,6 +1558,22 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static ssize_t rpmsg_name_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct qcom_glink_device *gdev = to_glink_device(rpdev);
+
+   return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", gdev->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {
+   &dev_attr_rpmsg_name.attr,
+   NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
 static void qcom_glink_device_release(struct device *dev)
 {
struct rpmsg_device *rpdev = to_rpmsg_device(dev);
@@ -1597,6 +1613,8 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
return ERR_PTR(-ENOMEM);
 
glink->dev = dev;
+   glink->dev->groups = qcom_glink_groups;
+
glink->tx_pipe = tx;
glink->rx_pipe = rx;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 6/6] rpmsg: glink: Add get_rproc_name device op

[Chris Lew]

Add support for clients to query the edge name for the glink device
their channel is registered for.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index a897ccea3098..4748dea0748e 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1346,6 +1346,14 @@ static struct device_node 
*qcom_glink_match_channel(struct device_node *node,
return NULL;
 }
 
+static const char *qcom_glink_get_rproc_name(struct rpmsg_device *rpdev)
+{
+   struct glink_channel *channel = to_glink_channel(rpdev->ept);
+   struct qcom_glink *glink = channel->glink;
+
+   return glink->name;
+}
+
 static const struct rpmsg_device_ops glink_chrdev_ops = {
.create_ept = qcom_glink_create_ept,
 };
@@ -1353,6 +1361,7 @@ static struct device_node 
*qcom_glink_match_channel(struct device_node *node,
 static const struct rpmsg_device_ops glink_device_ops = {
.create_ept = qcom_glink_create_ept,
.announce_create = qcom_glink_announce_create,
+   .get_rproc_name = qcom_glink_get_rproc_name,
 };
 
 static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/6] rpmsg: glink: Add support for rpmsg glink chrdev

[Chris Lew]

RPMSG provides a char device interface to userspace. Probe the rpmsg
chrdev channel to enable the rpmsg_ctrl device creation on glink
transports.

Signed-off-by: Chris Lew 
---
 drivers/rpmsg/qcom_glink_native.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c 
b/drivers/rpmsg/qcom_glink_native.c
index 179132226dc2..786f2eca01f1 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -132,6 +132,13 @@ enum {
GLINK_STATE_CLOSING,
 };
 
+struct qcom_glink_device {
+   struct rpmsg_device rpdev;
+   struct qcom_glink *glink;
+};
+
+#define to_glink_device(_x) container_of(_x, struct qcom_glink_device, rpdev)
+
 /**
  * struct glink_channel - internal representation of a channel
  * @rpdev: rpdev reference, only used for primary endpoints
@@ -1339,6 +1346,10 @@ static struct device_node 
*qcom_glink_match_channel(struct device_node *node,
return NULL;
 }
 
+static const struct rpmsg_device_ops glink_chrdev_ops = {
+   .create_ept = qcom_glink_create_ept,
+};
+
 static const struct rpmsg_device_ops glink_device_ops = {
.create_ept = qcom_glink_create_ept,
.announce_create = qcom_glink_announce_create,
@@ -1547,6 +1558,30 @@ static void qcom_glink_work(struct work_struct *work)
}
 }
 
+static void qcom_glink_device_release(struct device *dev)
+{
+   struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+   struct qcom_glink_device *gdev = to_glink_device(rpdev);
+
+   kfree(gdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+   struct qcom_glink_device *gdev;
+
+   gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
+   if (!gdev)
+   return -ENOMEM;
+
+   gdev->glink = glink;
+   gdev->rpdev.ops = &glink_chrdev_ops;
+   gdev->rpdev.dev.parent = glink->dev;
+   gdev->rpdev.dev.release = qcom_glink_device_release;
+
+   return rpmsg_chrdev_register_device(&gdev->rpdev);
+}
+
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
   unsigned long features,
   struct qcom_glink_pipe *rx,
@@ -1605,6 +1640,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device 
*dev,
if (ret)
return ERR_PTR(ret);
 
+   ret = qcom_glink_create_chrdev(glink);
+   if (ret)
+   dev_err(glink->dev, "failed to register chrdev\n");
+
return glink;
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] rpmsg: glink: smem: Ensure ordering during tx

[Chris Lew]

On 12/14/2017 12:15 PM, Bjorn Andersson wrote:

Ensure the ordering of the fifo write and the update of the write index,
so that the index is not updated before the data has landed in the fifo.

Reported-by: Arun Kumar Neelakantam 
Signed-off-by: Bjorn Andersson 
---


Acked-By: Chris Lew 

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

[PATCH v2 4/5] soc: qcom: smem: Support dynamic item limit

[Chris Lew]

In V12 SMEM, SBL writes SMEM parameter information
after the TOC. Use the SBL provided item count
as the max item number.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Change num_items to __le16 from __le32
- Move max smem item warning to generic get/alloc functions
- Use get ptable helper function

 drivers/soc/qcom/smem.c | 51 +++--
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 88d5ec663304..2f3b1e1a8904 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -223,6 +223,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic: magic number, must be SMEM_INFO_MAGIC
+ * @size:  size of the smem region
+ * @base_addr: base address of the smem region
+ * @reserved:  for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+   u8 magic[4];
+   __le32 size;
+   __le32 base_addr;
+   __le32 reserved;
+   __le16 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:  identifier of aux_mem base
  * @virt_base: virtual base address of memory with this aux_mem identifier
@@ -243,6 +261,7 @@ struct smem_region {
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:   list of the memory regions defining the shared memory
  */
@@ -255,6 +274,7 @@ struct qcom_smem {
size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
+   u32 item_count;
 
unsigned num_regions;
struct smem_region regions[0];
@@ -386,9 +406,6 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
struct smem_global_entry *entry;
struct smem_header *header;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
-   return -EINVAL;
-
header = smem->regions[0].virt_base;
entry = &header->toc[item];
if (entry->allocated)
@@ -439,6 +456,9 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
return -EINVAL;
}
 
+   if (WARN_ON(item >= __smem->item_count))
+   return -EINVAL;
+
ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
  HWSPINLOCK_TIMEOUT,
  &flags);
@@ -471,9 +491,6 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
u32 aux_base;
unsigned i;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
-   return ERR_PTR(-EINVAL);
-
header = smem->regions[0].virt_base;
entry = &header->toc[item];
if (!entry->allocated)
@@ -569,6 +586,9 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t 
*size)
if (!__smem)
return ptr;
 
+   if (WARN_ON(item >= __smem->item_count))
+   return ERR_PTR(-EINVAL);
+
ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
  HWSPINLOCK_TIMEOUT,
  &flags);
@@ -656,6 +676,22 @@ static struct smem_ptable *qcom_smem_get_ptable(struct 
qcom_smem *smem)
return ptable;
 }
 
+static u32 qcom_smem_get_dynamic_item(struct qcom_smem *smem)
+{
+   struct smem_ptable *ptable;
+   struct smem_info *info;
+
+   ptable = qcom_smem_get_ptable(smem);
+   if (IS_ERR_OR_NULL(ptable))
+   return SMEM_ITEM_COUNT;
+
+   info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+   if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+   return SMEM_ITEM_COUNT;
+
+   return le16_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem)
 {
struct smem_partition_header *header;
@@ -883,7 +919,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
ret = qcom_smem_set_global_partition(smem);
if (ret < 0)
return ret;
+   smem->item_count = qcom_smem_get_dynamic_item(smem);
+   break;
case SMEM_GLOBAL_HEAP_VERSION:
+   smem->item_count = SMEM_ITEM_COUNT;
break;
default:
dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n", version);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Chris Lew]

Endianness can vary in the system, add le32_to_cpu when comparing
size values from smem.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..db04c45d4132 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem 
*smem,
return -EINVAL;
}
 
-   if (header->size != entry->size) {
+   if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
dev_err(smem->dev,
"Partition %d has invalid size\n", i);
return -EINVAL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/5] Qualcomm SMEM V12 Support

[Chris Lew]

SMEM V12 was devised to make better use of the global SMEM region. The
global heap region is formatted to be similar to a private partition.
This allows the maximum number of smem items to increase. The maximum
item number is written by the bootloader in a region after the table 
of contents. The number of hosts are increased for later chipsets.

This patchset depends on patch: Qualcomm SMEM cached item support

Chris Lew (5):
  soc: qcom: smem: Use le32_to_cpu for partition size comparison
  soc: qcom: smem: Read version by using the smem header
  soc: qcom: smem: Support global partition
  soc: qcom: smem: Support dynamic item limit
  soc: qcom: smem: Increase the number of hosts

 drivers/soc/qcom/smem.c | 248 +---
 1 file changed, 195 insertions(+), 53 deletions(-)

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

[PATCH v2 5/5] soc: qcom: smem: Increase the number of hosts

[Chris Lew]

Increase the maximum number of hosts in a system to 10.

Signed-off-by: Chris Lew 
---

Changes since v1:
- None

 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 2f3b1e1a8904..086f31b6c584 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -91,7 +91,7 @@
 #define SMEM_GLOBAL_HOST   0xfffe
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT9
+#define SMEM_HOST_COUNT10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] soc: qcom: smem: Support global partition

[Chris Lew]

SMEM V12 creates a global partition to allocate global
smem items from instead of a global heap. The global
partition has the same structure as a private partition.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Move V12 descriptions to top comment
- Add cacheline support to global partition
- Add ptable get helper function
- Move global partition init to version check

 drivers/soc/qcom/smem.c | 170 +++-
 1 file changed, 141 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 540322ae409e..88d5ec663304 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -55,6 +55,10 @@
  * is hence the region between the cached and non-cached offsets. The header of
  * cached items comes after the data.
  *
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
  *
  * To synchronize allocations in the shared memory heaps a remote spinlock must
  * be held - currently lock number 3 of the sfpb or tcsr is used for this on 
all
@@ -68,7 +72,8 @@
  * version is a valid version as a sanity check.
  */
 #define SMEM_MASTER_SBL_VERSION_INDEX  7
-#define SMEM_EXPECTED_VERSION  11
+#define SMEM_GLOBAL_HEAP_VERSION   11
+#define SMEM_GLOBAL_PART_VERSION   12
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -82,6 +87,9 @@
 /* Processor/host identifier for the application processor */
 #define SMEM_HOST_APPS 0
 
+/* Processor/host identifier for the global partition */
+#define SMEM_GLOBAL_HOST   0xfffe
+
 /* Max number of processors/hosts in a system */
 #define SMEM_HOST_COUNT9
 
@@ -230,6 +238,8 @@ struct smem_region {
  * struct qcom_smem - device data for the smem device
  * @dev:   device pointer
  * @hwlock:reference to a hwspinlock
+ * @global_partition:  pointer to global partition when in use
+ * @global_cacheline:  cacheline size for global partition
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
@@ -241,6 +251,8 @@ struct qcom_smem {
 
struct hwspinlock *hwlock;
 
+   struct smem_partition_header *global_partition;
+   size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
 
@@ -317,16 +329,14 @@ static void *cached_entry_to_item(struct 
smem_private_entry *e)
 #define HWSPINLOCK_TIMEOUT 1000
 
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
-  unsigned host,
+  struct smem_partition_header *phdr,
   unsigned item,
   size_t size)
 {
-   struct smem_partition_header *phdr;
struct smem_private_entry *hdr, *end;
size_t alloc_size;
void *cached;
 
-   phdr = smem->partitions[host];
hdr = phdr_to_first_uncached_entry(phdr);
end = phdr_to_last_uncached_entry(phdr);
cached = phdr_to_last_cached_entry(phdr);
@@ -334,8 +344,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
while (hdr < end) {
if (hdr->canary != SMEM_PRIVATE_CANARY) {
dev_err(smem->dev,
-   "Found invalid canary in host %d partition\n",
-   host);
+   "Found invalid canary in hosts %d:%d 
partition\n",
+   phdr->host0, phdr->host1);
return -EINVAL;
}
 
@@ -373,8 +383,8 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  unsigned item,
  size_t size)
 {
-   struct smem_header *header;
struct smem_global_entry *entry;
+   struct smem_header *header;
 
if (WARN_ON(item >= SMEM_ITEM_COUNT))
return -EINVAL;
@@ -416,6 +426,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  */
 int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
+   struct smem_partition_header *phdr;
unsigned long flags;
int ret;
 
@@ -434,10 +445,15 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
if (ret)
return ret;
 
-   if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-   ret = qcom_smem_alloc_private(__smem, host, item, size);
-   else
+   if (host < SMEM_HOST_COUNT && __smem->partitions[host]) {
+   phdr = __smem->partitions[host];
+   ret = qcom

[PATCH v2 2/5] soc: qcom: smem: Read version by using the smem header

[Chris Lew]

The SMEM header structure includes the version information.
Read the version directly from the header instead of getting
an item from the global heap.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Remove unused smem item version macro
- Move smem get version change to separate commit

 drivers/soc/qcom/smem.c | 25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index db04c45d4132..540322ae409e 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -63,13 +63,12 @@
  */
 
 /*
- * Item 3 of the global heap contains an array of versions for the various
- * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * The version member of the smem header contains an array of versions for the
+ * various software components in the SoC. We verify that the boot loader
+ * version is a valid version as a sanity check.
  */
-#define SMEM_ITEM_VERSION  3
-#define  SMEM_MASTER_SBL_VERSION_INDEX 7
-#define  SMEM_EXPECTED_VERSION 11
+#define SMEM_MASTER_SBL_VERSION_INDEX  7
+#define SMEM_EXPECTED_VERSION  11
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -604,19 +603,11 @@ int qcom_smem_get_free_space(unsigned host)
 
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
+   struct smem_header *header;
__le32 *versions;
-   size_t size;
-
-   versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-   if (IS_ERR(versions)) {
-   dev_err(smem->dev, "Unable to read the version item\n");
-   return -ENOENT;
-   }
 
-   if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-   dev_err(smem->dev, "Version item is too small\n");
-   return -EINVAL;
-   }
+   header = smem->regions[0].virt_base;
+   versions = header->version;
 
return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/5] soc: qcom: smem: Use le32_to_cpu for partition size comparison

[Chris Lew]

On 9/15/2017 11:39 AM, Stephen Boyd wrote:

On 09/14, Chris Lew wrote:

Endianness can vary in the system, add le32_to_cpu when comparing
size values from smem.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

  drivers/soc/qcom/smem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..db04c45d4132 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem 
*smem,
return -EINVAL;
}
  
-		if (header->size != entry->size) {

+   if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {


Also, it doesn't really matter. We're comparing two numbers with
the same endianness, so comparing them for equality before or
after swapping makes no difference. Sparse also (correctly)
doesn't complain here, because adding the conversion is not
necessary. Drop this patch?



Hey Bjorn, should we remove this patch? You had flagged this comparison 
in the first version of the global partition changes.


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

Re: [PATCH v2 3/5] soc: qcom: smem: Support global partition

[Chris Lew]

On 9/15/2017 11:33 AM, Bjorn Andersson wrote:

On Thu 14 Sep 14:25 PDT 2017, Chris Lew wrote:

[..]

+static struct smem_ptable *qcom_smem_get_ptable(struct qcom_smem *smem)
  {
-   struct smem_partition_header *header;
-   struct smem_ptable_entry *entry;
struct smem_ptable *ptable;
-   unsigned remote_host;
-   u32 version, host0, host1;
-   int i;
+   u32 version;
  
  	ptable = smem->regions[0].virt_base + smem->regions[0].size - SZ_4K;

if (memcmp(ptable->magic, SMEM_PTABLE_MAGIC, sizeof(ptable->magic)))
-   return 0;
+   return NULL;
  
  	version = le32_to_cpu(ptable->version);

if (version != 1) {
dev_err(smem->dev,
"Unsupported partition header version %d\n", version);
+   return ERR_PTR(-EINVAL);


In the calling places NULL and -EINVAL are both treated as -EINVAL, so I
think it's better to just return NULL here as well as check for !ptable
in callers.

Regards,
Bjorn



qcom_smem_enumerate_partitions allowed the partition table to be 
optional before. I want to keep that behavior for V11 where smem might 
only have the global heap. The probe will continue with a NULL/0 return 
from qcom_get_ptable/qcom_smem_enumerate_partitions.


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

Re: [PATCH v2 11/20] rpmsg: glink: Fix idr_lock from mutex to spinlock

[Chris Lew]

Hi Sricharan,

Minor bug in this patch.

On 8/24/2017 12:21 AM, Sricharan R wrote:
[..]

@@ -829,11 +839,14 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, 
unsigned int rcid,
struct device_node *node;
int lcid;
int ret;
+   unsigned long flags;
  
+	spin_lock_irqsave(&glink->idr_lock, flags);

idr_for_each_entry(&glink->lcids, channel, lcid) {
if (!strcmp(channel->name, name))
break;
}
+   spin_unlock_irqrestore(&glink->idr_lock, flags);
  
  	if (!channel) {

channel = qcom_glink_alloc_channel(glink, name);
@@ -844,15 +857,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, 
unsigned int rcid,
create_device = true;
}
  
-	mutex_lock(&glink->idr_lock);

+   spin_lock_irqsave(&glink->idr_lock, flags);
ret = idr_alloc(&glink->rcids, channel, rcid, rcid + 1, GFP_KERNEL);


I think GFP_KERNEL should be changed to GFP_ATOMIC if the mutex is 
changed to spinlock.


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

Re: [PATCH 1/3] soc: qcom: smem: Support global partition

[Chris Lew]

Hi Bjorn,

Thanks for the review.

On 8/21/2017 10:17 AM, Bjorn Andersson wrote:

  #define  SMEM_MASTER_SBL_VERSION_INDEX7
-#define  SMEM_EXPECTED_VERSION 11
+#define  SMEM_GLOBAL_HEAP_VERSION  11
+
+/*
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
+ */


Please incorporate this paragraph in the larger comment at the top of
the file, so we keep that up to date.

[..]



Will do.


@@ -419,6 +430,7 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
  {
unsigned long flags;
int ret;
+   struct smem_partition_header *phdr;


Sorry for my OCD, but please maintain my reverse XMAS tree (put this
declaration first, as it's the longest).



Ok, will change.

  
  	if (!__smem)

return -EPROBE_DEFER;

[..]

@@ -604,21 +631,61 @@ int qcom_smem_get_free_space(unsigned host)
  
  static int qcom_smem_get_sbl_version(struct qcom_smem *smem)

  {
+   struct smem_header *header;
__le32 *versions;
-   size_t size;
  
-	versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);

-   if (IS_ERR(versions)) {
-   dev_err(smem->dev, "Unable to read the version item\n");
-   return -ENOENT;
+   header = smem->regions[0].virt_base;
+   versions = header->version;
+
+   return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
+}


I would prefer if you split the change of this function into its own
patch, just to clarify that it's unrelated to the new version support.




Ok, will separate the change and adjust the descriptions accordingly.


+
+static int qcom_smem_set_global_partition(struct qcom_smem *smem,
+   struct smem_ptable_entry *entry)
+{
+   struct smem_partition_header *header;
+   u32  host0, host1;
+
+   if (!le32_to_cpu(entry->offset) || !le32_to_cpu(entry->size)) {
+   dev_err(smem->dev, "Invalid entry for gloabl partition\n");
+   return -EINVAL;
}
  
-	if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {

-   dev_err(smem->dev, "Version item is too small\n");
+   if (smem->global_partition) {
+   dev_err(smem->dev, "Already found the global partition\n");
return -EINVAL;
}
+   header = smem->regions[0].virt_base + le32_to_cpu(entry->offset);
+   host0 = le16_to_cpu(header->host0);
+   host1 = le16_to_cpu(header->host1);
  
-	return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);

+   if (memcmp(header->magic, SMEM_PART_MAGIC,
+   sizeof(header->magic))) {
+   dev_err(smem->dev, "Gloal partition has invalid magic\n");
+   return -EINVAL;
+   }
+
+   if (host0 != SMEM_GLOBAL_HOST && host1 != SMEM_GLOBAL_HOST) {
+   dev_err(smem->dev, "Global partition hosts are invalid\n");
+   return -EINVAL;
+   }
+
+   if (header->size != entry->size) {


This happens to work, because they are both in the same endian. But
please sprinkle some le32_to_cpu() here as well.




These checks mimic the sanity checks being done in enumerate_partitions. 
Should I create a patch to increase le32_to_cpu usage in 
qcom_smem_enumerate_partitions?



+   dev_err(smem->dev, "Global partition has invalid size\n");
+   return -EINVAL;
+   }
+
+   if (le32_to_cpu(header->offset_free_uncached) >
+   le32_to_cpu(header->size)) {


Consider using local variables in favor of wrapping lines.




Ok, will do.


+   dev_err(smem->dev,
+   "Global partition has invalid free pointer\n");
+   return -EINVAL;
+   }
+
+   smem->global_partition = header;
+   smem->global_cacheline = le32_to_cpu(entry->cacheline);
+
+   return 0;
  }
  
  static int qcom_smem_enumerate_partitions(struct qcom_smem *smem,

@@ -647,6 +714,12 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem 
*smem,
host0 = le16_to_cpu(entry->host0);
host1 = le16_to_cpu(entry->host1);
  
+		if (host0 == SMEM_GLOBAL_HOST && host0 == host1) {

+   if (qcom_smem_set_global_partition(smem, entry))
+   return -EINVAL;
+   continue;
+   }
+


As you're not able to leverage any of the checks from this loop I think
it's cleaner to duplicate the traversal of the partition table in both
functions and call the "search for global partition" directly from
probe - if the version indicates there should be one.




Ok, will set the global partition in the version case statement and 
error out of the probe if finding the global partition fails since it is 
not optional in the new version.



 

[PATCH v3 2/5] soc: qcom: smem: Read version from the smem header

[Chris Lew]

From: Chris Lew 

The SMEM header structure includes the version information. Read the
version directly from the header instead of getting an item from the
global heap.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Remove unused smem item version macro
- Move smem get version change to separate commit

Changes since v2:
- Reduce subject to 50 chars and wrap summary to 72 chars

 drivers/soc/qcom/smem.c | 25 -
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index db04c45d4132..540322ae409e 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -63,13 +63,12 @@
  */
 
 /*
- * Item 3 of the global heap contains an array of versions for the various
- * software components in the SoC. We verify that the boot loader version is
- * what the expected version (SMEM_EXPECTED_VERSION) as a sanity check.
+ * The version member of the smem header contains an array of versions for the
+ * various software components in the SoC. We verify that the boot loader
+ * version is a valid version as a sanity check.
  */
-#define SMEM_ITEM_VERSION  3
-#define  SMEM_MASTER_SBL_VERSION_INDEX 7
-#define  SMEM_EXPECTED_VERSION 11
+#define SMEM_MASTER_SBL_VERSION_INDEX  7
+#define SMEM_EXPECTED_VERSION  11
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -604,19 +603,11 @@ int qcom_smem_get_free_space(unsigned host)
 
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
+   struct smem_header *header;
__le32 *versions;
-   size_t size;
-
-   versions = qcom_smem_get_global(smem, SMEM_ITEM_VERSION, &size);
-   if (IS_ERR(versions)) {
-   dev_err(smem->dev, "Unable to read the version item\n");
-   return -ENOENT;
-   }
 
-   if (size < sizeof(unsigned) * SMEM_MASTER_SBL_VERSION_INDEX) {
-   dev_err(smem->dev, "Version item is too small\n");
-   return -EINVAL;
-   }
+   header = smem->regions[0].virt_base;
+   versions = header->version;
 
return le32_to_cpu(versions[SMEM_MASTER_SBL_VERSION_INDEX]);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 0/5] Qualcomm SMEM V12 Support

[Chris Lew]

SMEM V12 was devised to make better use of the global SMEM region. The
global heap region is formatted to be similar to a private partition.
This allows the maximum number of smem items to increase. The maximum
item number is written by the bootloader in a region after the table 
of contents. The number of hosts are increased for later chipsets.

This patchset depends on patch: Qualcomm SMEM cached item support.

Chris Lew (5):
  soc: qcom: smem: Use le32_to_cpu for comparison
  soc: qcom: smem: Read version from the smem header
  soc: qcom: smem: Support global partition
  soc: qcom: smem: Support dynamic item limit
  soc: qcom: smem: Increase the number of hosts

 drivers/soc/qcom/smem.c | 250 +---
 1 file changed, 196 insertions(+), 54 deletions(-)

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

[PATCH v3 5/5] soc: qcom: smem: Increase the number of hosts

[Chris Lew]

From: Chris Lew 

Increase the maximum number of hosts in a system to 10.

Signed-off-by: Chris Lew 
---

Changes since v1:
- None

Changes since v2:
- None

 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 19704baa65f4..0b94d62fad2b 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -91,7 +91,7 @@
 #define SMEM_GLOBAL_HOST   0xfffe
 
 /* Max number of processors/hosts in a system */
-#define SMEM_HOST_COUNT9
+#define SMEM_HOST_COUNT10
 
 /**
   * struct smem_proc_comm - proc_comm communication struct (legacy)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 1/5] soc: qcom: smem: Use le32_to_cpu for comparison

[Chris Lew]

From: Chris Lew 

Endianness can vary in the system, add le32_to_cpu when comparing
partition sizes from smem.

Signed-off-by: Chris Lew 
---

Changes since v1:
- New change

Changes since v2:
- Reduce subject to 50 chars and wrap summary to 72 chars

 drivers/soc/qcom/smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c28275be0038..db04c45d4132 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -698,7 +698,7 @@ static int qcom_smem_enumerate_partitions(struct qcom_smem 
*smem,
return -EINVAL;
}
 
-   if (header->size != entry->size) {
+   if (le32_to_cpu(header->size) != le32_to_cpu(entry->size)) {
dev_err(smem->dev,
"Partition %d has invalid size\n", i);
return -EINVAL;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 3/5] soc: qcom: smem: Support global partition

[Chris Lew]

From: Chris Lew 

SMEM V12 creates a global partition to allocate global smem items from
instead of a global heap. The global partition has the same structure as
a private partition.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Move V12 descriptions to top comment
- Add cacheline support to global partition
- Add ptable get helper function
- Move global partition init to version check

Changes since v2:
- Return -ENOENT if partition table does not exist
- Exclude -ENOENT error propagation from enumerate_partitions()
- Reduce subject to 50 chars and wrap summary to 72 chars

 drivers/soc/qcom/smem.c | 172 +++-
 1 file changed, 142 insertions(+), 30 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 540322ae409e..6a3134e9c591 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -55,6 +55,10 @@
  * is hence the region between the cached and non-cached offsets. The header of
  * cached items comes after the data.
  *
+ * Version 12 (SMEM_GLOBAL_PART_VERSION) changes the item alloc/get procedure
+ * for the global heap. A new global partition is created from the global heap
+ * region with partition type (SMEM_GLOBAL_HOST) and the max smem item count is
+ * set by the bootloader.
  *
  * To synchronize allocations in the shared memory heaps a remote spinlock must
  * be held - currently lock number 3 of the sfpb or tcsr is used for this on 
all
@@ -68,7 +72,8 @@
  * version is a valid version as a sanity check.
  */
 #define SMEM_MASTER_SBL_VERSION_INDEX  7
-#define SMEM_EXPECTED_VERSION  11
+#define SMEM_GLOBAL_HEAP_VERSION   11
+#define SMEM_GLOBAL_PART_VERSION   12
 
 /*
  * The first 8 items are only to be allocated by the boot loader while
@@ -82,6 +87,9 @@
 /* Processor/host identifier for the application processor */
 #define SMEM_HOST_APPS 0
 
+/* Processor/host identifier for the global partition */
+#define SMEM_GLOBAL_HOST   0xfffe
+
 /* Max number of processors/hosts in a system */
 #define SMEM_HOST_COUNT9
 
@@ -230,6 +238,8 @@ struct smem_region {
  * struct qcom_smem - device data for the smem device
  * @dev:   device pointer
  * @hwlock:reference to a hwspinlock
+ * @global_partition:  pointer to global partition when in use
+ * @global_cacheline:  cacheline size for global partition
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
@@ -241,6 +251,8 @@ struct qcom_smem {
 
struct hwspinlock *hwlock;
 
+   struct smem_partition_header *global_partition;
+   size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
 
@@ -317,16 +329,14 @@ static void *cached_entry_to_item(struct 
smem_private_entry *e)
 #define HWSPINLOCK_TIMEOUT 1000
 
 static int qcom_smem_alloc_private(struct qcom_smem *smem,
-  unsigned host,
+  struct smem_partition_header *phdr,
   unsigned item,
   size_t size)
 {
-   struct smem_partition_header *phdr;
struct smem_private_entry *hdr, *end;
size_t alloc_size;
void *cached;
 
-   phdr = smem->partitions[host];
hdr = phdr_to_first_uncached_entry(phdr);
end = phdr_to_last_uncached_entry(phdr);
cached = phdr_to_last_cached_entry(phdr);
@@ -334,8 +344,8 @@ static int qcom_smem_alloc_private(struct qcom_smem *smem,
while (hdr < end) {
if (hdr->canary != SMEM_PRIVATE_CANARY) {
dev_err(smem->dev,
-   "Found invalid canary in host %d partition\n",
-   host);
+   "Found invalid canary in hosts %d:%d 
partition\n",
+   phdr->host0, phdr->host1);
return -EINVAL;
}
 
@@ -373,8 +383,8 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  unsigned item,
  size_t size)
 {
-   struct smem_header *header;
struct smem_global_entry *entry;
+   struct smem_header *header;
 
if (WARN_ON(item >= SMEM_ITEM_COUNT))
return -EINVAL;
@@ -416,6 +426,7 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
  */
 int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 {
+   struct smem_partition_header *phdr;
unsigned long flags;
int ret;
 
@@ -434,10 +445,15 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
if (ret)
return ret;
 
-   if (host < SMEM_HOST_COUNT && __smem->partitions[host])
-   ret = qcom_sme

[PATCH v3 4/5] soc: qcom: smem: Support dynamic item limit

[Chris Lew]

From: Chris Lew 

In V12 SMEM, SBL writes SMEM parameter information after the TOC. Use
the SBL provided item count as the max item number.

Signed-off-by: Chris Lew 
---

Changes since v1:
- Change num_items to __le16 from __le32
- Move max smem item warning to generic get/alloc functions
- Use get ptable helper function

Changes since v2:
- Rename qcom_smem_get_dynamic_item to qcom_smem_get_item_count
- Reduce subject to 50 chars and wrap summary to 72 chars

 drivers/soc/qcom/smem.c | 51 +++--
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 6a3134e9c591..19704baa65f4 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -223,6 +223,24 @@ struct smem_private_entry {
 #define SMEM_PRIVATE_CANARY0xa5a5
 
 /**
+ * struct smem_info - smem region info located after the table of contents
+ * @magic: magic number, must be SMEM_INFO_MAGIC
+ * @size:  size of the smem region
+ * @base_addr: base address of the smem region
+ * @reserved:  for now reserved entry
+ * @num_items: highest accepted item number
+ */
+struct smem_info {
+   u8 magic[4];
+   __le32 size;
+   __le32 base_addr;
+   __le32 reserved;
+   __le16 num_items;
+};
+
+static const u8 SMEM_INFO_MAGIC[] = { 0x53, 0x49, 0x49, 0x49 }; /* SIII */
+
+/**
  * struct smem_region - representation of a chunk of memory used for smem
  * @aux_base:  identifier of aux_mem base
  * @virt_base: virtual base address of memory with this aux_mem identifier
@@ -243,6 +261,7 @@ struct smem_region {
  * @partitions:list of pointers to partitions affecting the current
  * processor/host
  * @cacheline: list of cacheline sizes for each host
+ * @item_count: max accepted item number
  * @num_regions: number of @regions
  * @regions:   list of the memory regions defining the shared memory
  */
@@ -255,6 +274,7 @@ struct qcom_smem {
size_t global_cacheline;
struct smem_partition_header *partitions[SMEM_HOST_COUNT];
size_t cacheline[SMEM_HOST_COUNT];
+   u32 item_count;
 
unsigned num_regions;
struct smem_region regions[0];
@@ -386,9 +406,6 @@ static int qcom_smem_alloc_global(struct qcom_smem *smem,
struct smem_global_entry *entry;
struct smem_header *header;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
-   return -EINVAL;
-
header = smem->regions[0].virt_base;
entry = &header->toc[item];
if (entry->allocated)
@@ -439,6 +456,9 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t 
size)
return -EINVAL;
}
 
+   if (WARN_ON(item >= __smem->item_count))
+   return -EINVAL;
+
ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
  HWSPINLOCK_TIMEOUT,
  &flags);
@@ -471,9 +491,6 @@ static void *qcom_smem_get_global(struct qcom_smem *smem,
u32 aux_base;
unsigned i;
 
-   if (WARN_ON(item >= SMEM_ITEM_COUNT))
-   return ERR_PTR(-EINVAL);
-
header = smem->regions[0].virt_base;
entry = &header->toc[item];
if (!entry->allocated)
@@ -569,6 +586,9 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t 
*size)
if (!__smem)
return ptr;
 
+   if (WARN_ON(item >= __smem->item_count))
+   return ERR_PTR(-EINVAL);
+
ret = hwspin_lock_timeout_irqsave(__smem->hwlock,
  HWSPINLOCK_TIMEOUT,
  &flags);
@@ -656,6 +676,22 @@ static struct smem_ptable *qcom_smem_get_ptable(struct 
qcom_smem *smem)
return ptable;
 }
 
+static u32 qcom_smem_get_item_count(struct qcom_smem *smem)
+{
+   struct smem_ptable *ptable;
+   struct smem_info *info;
+
+   ptable = qcom_smem_get_ptable(smem);
+   if (IS_ERR_OR_NULL(ptable))
+   return SMEM_ITEM_COUNT;
+
+   info = (struct smem_info *)&ptable->entry[ptable->num_entries];
+   if (memcmp(info->magic, SMEM_INFO_MAGIC, sizeof(info->magic)))
+   return SMEM_ITEM_COUNT;
+
+   return le16_to_cpu(info->num_items);
+}
+
 static int qcom_smem_set_global_partition(struct qcom_smem *smem)
 {
struct smem_partition_header *header;
@@ -883,7 +919,10 @@ static int qcom_smem_probe(struct platform_device *pdev)
ret = qcom_smem_set_global_partition(smem);
if (ret < 0)
return ret;
+   smem->item_count = qcom_smem_get_item_count(smem);
+   break;
case SMEM_GLOBAL_HEAP_VERSION:
+   smem->item_count = SMEM_ITEM_COUNT;
break;
default:
dev_err(&pdev->dev, "Unsupported SMEM version 0x%x\n&

Re: [PATCH 1/7] net: qrtr: Invoke sk_error_report() after setting sk_err

[Chris Lew]

On 9/6/2017 11:03 PM, Bjorn Andersson wrote:

Rather than manually waking up any context sleeping on the sock to
signal an error we should call sk_error_report(). This has the added
benefit that in-kernel consumers can override this notificatino with
its own callback.



Typo with notification.

Thanks,
Chris

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

Re: [PATCH v4 4/5] remoteproc: qcom: Introduce sysmon

[Chris Lew]

On 11/29/2017 5:16 PM, Bjorn Andersson wrote:

The sysmon client communicates either via a dedicated SMD/GLINK channel
or via QMI encoded messages over IPCROUTER with remote processors in
order to perform graceful shutdown and inform about other remote
processors shutting down.

Signed-off-by: Bjorn Andersson 
---

Acked-By: Chris Lew 

Looked into who is listening to the other notifications besides 
BEFORE_SHUTDOWN and I think subsequent patches will need to add support 
for at least AFTER_POWERUP.


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

Re: [PATCH 1/3] dt-bindings: soc: qcom: Support GLINK intents

[Chris Lew]

On 10/19/2017 10:00 PM, Bjorn Andersson wrote:

On Wed 18 Oct 18:10 PDT 2017, Chris Lew wrote:


Virtual GLINK channels may know what throughput to expect from a
remoteproc. An intent advertises to the remoteproc this channel is
ready to receive data. Allow a channel to define the size and amount of
intents to be prequeued.

Signed-off-by: Chris Lew 
---
  Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index b277eca861f7..6c21f76822ca 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -39,6 +39,14 @@ of these nodes are defined by the individual bindings for 
the specific function
   Definition: a list of channels tied to this function, used for matching
  the function to a set of virtual channels

+- intents:


qcom,intents or qcom,default-intents



Will use "qcom,intents".


+ Usage: optional
+ Value type: 
+ Definition: a list of size,amount pairs describing what intents should
+be preallocated for this virtual channel. If a GLINK node
+supports intents, an intent advertises this channel is ready
+to receive data.


Rather than describing what a GLINK intent is I would suggest that you
replace this second sentence with something like "This can be used to
tweak the default intents available for the channel to meet expectations
of the remote."



Sounds good, will change.


+
  = EXAMPLE
  The following example represents the GLINK RPM node on a MSM8996 device, with
  the function for the "rpm_request" channel defined, which is used for
@@ -69,6 +77,8 @@ regualtors and root clocks.
   compatible = "qcom,rpm-msm8996";
   qcom,glink-channels = "rpm_requests";

+ intents = <0x400 5
+   0x800 1>;
   ...
   };
   };


Regards,
Bjorn



Thanks,
Chris

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

Re: [PATCH v2 2/5] soc: qcom: Introduce QMI helpers

[Chris Lew]

Hi Bjorn,

Found a minor typo.

On 11/6/2017 9:20 PM, Bjorn Andersson wrote:

[..]

+ssize_t qmi_send_indication(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+   int msg_id, size_t len, struct qmi_elem_info *ei,
+   const void *c_struct)
+{
+   struct qmi_txn txn;
+   ssize_t rval;
+   int ret;
+
+   ret = qmi_txn_init(qmi, &txn, NULL, NULL);
+   if (ret < 0)
+   return ret;;


Double semi colon on ret.

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

Re: [PATCH v3 4/5] remoteproc: qcom: Introduce sysmon

[Chris Lew]

On 11/16/2017 9:58 PM, Bjorn Andersson wrote:

On Thu 16 Nov 12:05 PST 2017, Chris Lew wrote:

+   req.event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;


Are there plans to add the other SSR events to sysmon notifiers? I think the
SSCTL service expects to receive events about remote procs starting as well.



We could easily add support here to send out the AFTER_BOOTUP
notification, beyond that we would need to extend the subdev notifiers
as well.

But the downstream code paths does confuse me, can you confirm that
these messages are actually sent to the remote ssctl services?

Regards,
Bjorn



Yea the other three events are definitely sent to the remote. The remote 
side posts the events for other modules to query. I'm not sure if all of 
the events are used by other other modules though.


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

Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers

[Chris Lew]

On 11/15/2017 12:10 PM, Bjorn Andersson wrote:
[..]> +static void qmi_handle_message(struct qmi_handle *qmi,

+  struct sockaddr_qrtr *sq,
+  const void *buf, size_t len)
+{
+   const struct qmi_header *hdr;
+   struct qmi_txn tmp_txn;
+   struct qmi_txn *txn = NULL;
+   int ret;
+
+   if (len < sizeof(*hdr)) {
+   pr_err("ignoring short QMI packet\n");
+   return;
+   }
+
+   hdr = buf;
+
+   /* If this is a response, find the matching transaction handle */
+   if (hdr->type == QMI_RESPONSE) {
+   mutex_lock(&qmi->txn_lock);
+   txn = idr_find(&qmi->txns, hdr->txn_id);
+   if (txn)
+   mutex_lock(&txn->lock);
+   mutex_unlock(&qmi->txn_lock);
+   }
+
+   if (txn && txn->dest && txn->ei) {
+   ret = qmi_decode_message(buf, len, txn->ei, txn->dest);
+   if (ret < 0)
+   pr_err("failed to decode incoming message\n");
+
+   txn->result = ret;
+   complete(&txn->completion);
+
+   mutex_unlock(&txn->lock);
+   } else if (txn) {
+   qmi_invoke_handler(qmi, sq, txn, buf, len);
+
+   mutex_unlock(&txn->lock);
+   } else {
+   /* Create a txn based on the txn_id of the incoming message */
+   memset(&tmp_txn, 0, sizeof(tmp_txn));
+   tmp_txn.id = hdr->txn_id;
+
+   qmi_invoke_handler(qmi, sq, &tmp_txn, buf, len);


I'm seeing an opportunity for user error with timed out transactions. If 
txn_wait gets timed out the txn is removed from the txns list. Later if 
we get the response, it comes down to this else with the tmp_txn. Some 
handlers may try to do a complete(&txn->completion) like the 
qmi_sample_client ping_pong_cb. This leads to a null pointer dereference 
since the temp txn was never initialized.


Should clients not call complete and we can call the complete in the 
else if(txn) condition?


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

Re: [PATCH v4 1/5] soc: qcom: Introduce QMI encoder/decoder

[Chris Lew]

On 11/29/2017 5:16 PM, Bjorn Andersson wrote:

Add the helper library for encoding and decoding QMI encoded messages.
The implementation is taken from lib/qmi_encdec.c of the Qualcomm kernel
(msm-3.18).

Modifications has been made to the public API, source buffers has been
made const and the debug-logging part was omitted, for now.

Tested-By: Chris Lew 
Tested-By: Srinivas Kandagatla 
Signed-off-by: Bjorn Andersson 
---


Acked-By: Chris Lew 

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

Re: [PATCH v4 2/5] soc: qcom: Introduce QMI helpers

[Chris Lew]

On 11/29/2017 5:16 PM, Bjorn Andersson wrote:

Drivers that needs to communicate with a remote QMI service all has to
perform the operations of discovering the service, encoding and decoding
the messages and operate the socket. This introduces an abstraction for
these common operations, reducing most of the duplication in such cases.

Tested-By: Srinivas Kandagatla 
Signed-off-by: Bjorn Andersson 
---

Acked-By: Chris Lew 

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