| Message ID | 20240102022512.999635-1-quic_taozhan@quicinc.com |
|---|---|
| State | Accepted |
| Commit | 27825593c972abac86b9a4453a8c8c9a2c1ec60f |
| Headers | show |
| Series | qcom: smem: remove hwspinlock from item get routine | expand |
On 1/1/2024 6:25 PM, Tao Zhang wrote: > During an SSR(Sub-System Restart) process, the remoteproc driver will > try to read the crash reason from SMEM. The qcom_smem_get() backing such > operations does however take the hwspinlock (tcsr mutex), which might be > held by the dying remoteproc. > > The associated timeout on the hwspin_lock_timeout_irqsave() would take > care of the system not hanging forever, but the get operation will fail, > unnecessarily delaying the process for the 'HWSPINLOCK_TIMEOUT' duration > (currently is '1s'), and finally resulting in failure to get crash > information from SMEM. > > This timeout can be avoided by removing the hwspinlock in the > qcom_smem_get routine. SMEM ensures that the allocated item will only be > visible after the new item is safe to use by following a specific order > of updates. > > In the private partition case, qcom_smem_get_private() will use > 'offset_free_uncached' as a loop boundary when looking for existing > allocated items. The corresponding allocation will only update > offset_free_uncached once the item is fully initialized. > > hdr->canary = SMEM_PRIVATE_CANARY; > hdr->item = cpu_to_le16(item); > hdr->size = cpu_to_le32(ALIGN(size, 8)); > hdr->padding_data = cpu_to_le16(le32_to_cpu(hdr->size) - size); > hdr->padding_hdr = 0; > > wmb(); > le32_add_cpu(&phdr->offset_free_uncached, alloc_size); > > The global partition is similar but uses the "entry->allocated" variable > to ensure the item is not visible to qcom_smem_get_global(). > > Signed-off-by: Tao Zhang <quic_taozhan@quicinc.com> > --- Reviewed-by: Chris Lew <quic_clew@quicinc.com>
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c index 690afc9a12f4..7191fa0c087f 100644 --- a/drivers/soc/qcom/smem.c +++ b/drivers/soc/qcom/smem.c @@ -655,8 +655,6 @@ static void *qcom_smem_get_private(struct qcom_smem *smem, void *qcom_smem_get(unsigned host, unsigned item, size_t *size) { struct smem_partition *part; - unsigned long flags; - int ret; void *ptr = ERR_PTR(-EPROBE_DEFER); if (!__smem) @@ -665,12 +663,6 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size) if (WARN_ON(item >= __smem->item_count)) return ERR_PTR(-EINVAL); - ret = hwspin_lock_timeout_irqsave(__smem->hwlock, - HWSPINLOCK_TIMEOUT, - &flags); - if (ret) - return ERR_PTR(ret); - if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) { part = &__smem->partitions[host]; ptr = qcom_smem_get_private(__smem, part, item, size); @@ -681,10 +673,7 @@ void *qcom_smem_get(unsigned host, unsigned item, size_t *size) ptr = qcom_smem_get_global(__smem, item, size); } - hwspin_unlock_irqrestore(__smem->hwlock, &flags); - return ptr; - } EXPORT_SYMBOL_GPL(qcom_smem_get);
During an SSR(Sub-System Restart) process, the remoteproc driver will try to read the crash reason from SMEM. The qcom_smem_get() backing such operations does however take the hwspinlock (tcsr mutex), which might be held by the dying remoteproc. The associated timeout on the hwspin_lock_timeout_irqsave() would take care of the system not hanging forever, but the get operation will fail, unnecessarily delaying the process for the 'HWSPINLOCK_TIMEOUT' duration (currently is '1s'), and finally resulting in failure to get crash information from SMEM. This timeout can be avoided by removing the hwspinlock in the qcom_smem_get routine. SMEM ensures that the allocated item will only be visible after the new item is safe to use by following a specific order of updates. In the private partition case, qcom_smem_get_private() will use 'offset_free_uncached' as a loop boundary when looking for existing allocated items. The corresponding allocation will only update offset_free_uncached once the item is fully initialized. hdr->canary = SMEM_PRIVATE_CANARY; hdr->item = cpu_to_le16(item); hdr->size = cpu_to_le32(ALIGN(size, 8)); hdr->padding_data = cpu_to_le16(le32_to_cpu(hdr->size) - size); hdr->padding_hdr = 0; wmb(); le32_add_cpu(&phdr->offset_free_uncached, alloc_size); The global partition is similar but uses the "entry->allocated" variable to ensure the item is not visible to qcom_smem_get_global(). Signed-off-by: Tao Zhang <quic_taozhan@quicinc.com> --- drivers/soc/qcom/smem.c | 11 ----------- 1 file changed, 11 deletions(-) base-commit: 39676dfe52331dba909c617f213fdb21015c8d10