public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
@ 2025-02-07 20:17 Bjorn Andersson
  2025-02-08 11:47 ` Sam Day
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Bjorn Andersson @ 2025-02-07 20:17 UTC (permalink / raw)
  To: Vinod Koul, Md Sadre Alam
  Cc: linux-arm-msm, dmaengine, linux-kernel, Dmitry Baryshkov,
	Georgi Djakov, Bjorn Andersson

Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
unavailable register")' made this read unconditional, in order to
identify if the instance is BAM-NDP or BAM-Lite.
But the BAM_REVISION register is not accessible on remotely managed BAM
instances and attempts to access it causes the system to crash.

Move the access back to be conditional and expand the checks that was
introduced to restore the old behavior when no revision information is
available.

Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
Reported-by: Georgi Djakov <djakov@kernel.org>
Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index c14557efd577..d42d913492a8 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
 	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
 
 	/* set descriptor threshold, start with 4 bytes */
-	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
-		     BAM_NDP_REVISION_END))
+	if (!bdev->bam_revision ||
+	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
 		writel_relaxed(DEFAULT_CNT_THRSHLD,
 			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
 
@@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
 			maxburst = bchan->slave.src_maxburst;
 		else
 			maxburst = bchan->slave.dst_maxburst;
-		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
-			     BAM_NDP_REVISION_END))
+		if (!bdev->bam_revision ||
+		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
 			writel_relaxed(maxburst,
 				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
 	}
@@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
 	u32 val;
 
 	/* read revision and configuration information */
-	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
-	if (!bdev->num_ees)
+	if (!bdev->num_ees) {
+		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
 		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
 
-	bdev->bam_revision = val & REVISION_MASK;
+		bdev->bam_revision = val & REVISION_MASK;
+	}
 
 	/* check that configured EE is within range */
 	if (bdev->ee >= bdev->num_ees)

---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250207-bam-read-fix-2b31297d3fa1

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-07 20:17 [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM Bjorn Andersson
@ 2025-02-08 11:47 ` Sam Day
  2025-02-08 12:42 ` Georgi Djakov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Sam Day @ 2025-02-08 11:47 UTC (permalink / raw)
  To: Bjorn Andersson, Vinod Koul, Md Sadre Alam
  Cc: linux-arm-msm, dmaengine, linux-kernel, Dmitry Baryshkov,
	Georgi Djakov

Hello Bjorn,

On Fri Feb 7, 2025 at 9:17 PM CET, Bjorn Andersson wrote:
> Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
> unavailable register")' made this read unconditional, in order to
> identify if the instance is BAM-NDP or BAM-Lite.
> But the BAM_REVISION register is not accessible on remotely managed BAM
> instances and attempts to access it causes the system to crash.
>
> Move the access back to be conditional and expand the checks that was
> introduced to restore the old behavior when no revision information is
> available.
>
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@kernel.org>
> Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..d42d913492a8 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
>  	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
>  
>  	/* set descriptor threshold, start with 4 bytes */
> -	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -		     BAM_NDP_REVISION_END))
> +	if (!bdev->bam_revision ||
> +	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  		writel_relaxed(DEFAULT_CNT_THRSHLD,
>  			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  
> @@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  			maxburst = bchan->slave.src_maxburst;
>  		else
>  			maxburst = bchan->slave.dst_maxburst;
> -		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -			     BAM_NDP_REVISION_END))
> +		if (!bdev->bam_revision ||
> +		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  			writel_relaxed(maxburst,
>  				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  	}
> @@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
>  	u32 val;
>  
>  	/* read revision and configuration information */
> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> -	if (!bdev->num_ees)
> +	if (!bdev->num_ees) {
> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>  
> -	bdev->bam_revision = val & REVISION_MASK;
> +		bdev->bam_revision = val & REVISION_MASK;
> +	}
>  
>  	/* check that configured EE is within range */
>  	if (bdev->ee >= bdev->num_ees)
>
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250207-bam-read-fix-2b31297d3fa1
>
> Best regards,

Thank you, this fixes the early synchronous external abort in
bam_dma_probe on my msm8916 device.

Tested-by: Sam Day <me@samcday.com>

Regards,
-Sam



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-07 20:17 [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM Bjorn Andersson
  2025-02-08 11:47 ` Sam Day
@ 2025-02-08 12:42 ` Georgi Djakov
  2025-02-09 15:19 ` David Heidelberg
  2025-02-10  8:44 ` Stephan Gerhold
  3 siblings, 0 replies; 7+ messages in thread
From: Georgi Djakov @ 2025-02-08 12:42 UTC (permalink / raw)
  To: Bjorn Andersson, Vinod Koul, Md Sadre Alam
  Cc: linux-arm-msm, dmaengine, linux-kernel, Dmitry Baryshkov

On 7.02.25 22:17, Bjorn Andersson wrote:
> Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
> unavailable register")' made this read unconditional, in order to
> identify if the instance is BAM-NDP or BAM-Lite.
> But the BAM_REVISION register is not accessible on remotely managed BAM
> instances and attempts to access it causes the system to crash.
> 
> Move the access back to be conditional and expand the checks that was
> introduced to restore the old behavior when no revision information is
> available.
> 
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@kernel.org>
> Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Tested-by: Georgi Djakov <djakov@kernel.org> # db845c

Thanks,
Georgi

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-07 20:17 [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM Bjorn Andersson
  2025-02-08 11:47 ` Sam Day
  2025-02-08 12:42 ` Georgi Djakov
@ 2025-02-09 15:19 ` David Heidelberg
  2025-02-10  8:44 ` Stephan Gerhold
  3 siblings, 0 replies; 7+ messages in thread
From: David Heidelberg @ 2025-02-09 15:19 UTC (permalink / raw)
  To: bjorn.andersson
  Cc: djakov, dmaengine, dmitry.baryshkov, linux-arm-msm, linux-kernel,
	quic_mdalam, vkoul

On 7.02.25 22:17, Bjorn Andersson wrote:
 > Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
 > unavailable register")' made this read unconditional, in order to
 > identify if the instance is BAM-NDP or BAM-Lite.
 > But the BAM_REVISION register is not accessible on remotely managed BAM
 > instances and attempts to access it causes the system to crash.
 >
 > Move the access back to be conditional and expand the checks that was
 > introduced to restore the old behavior when no revision information is
 > available.
 >
 > Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing 
unavailable register")
 > Reported-by: Georgi Djakov <djakov@kernel.org>
 > Closes: 
https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
 > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Tested-by: David Heidelberg <david@ixit.cz> # OnePlus 6T

-- 
David Heidelberg


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-07 20:17 [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM Bjorn Andersson
                   ` (2 preceding siblings ...)
  2025-02-09 15:19 ` David Heidelberg
@ 2025-02-10  8:44 ` Stephan Gerhold
  2025-02-10 11:48   ` Md Sadre Alam
  2025-02-12  2:43   ` Bjorn Andersson
  3 siblings, 2 replies; 7+ messages in thread
From: Stephan Gerhold @ 2025-02-10  8:44 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Vinod Koul, Md Sadre Alam, linux-arm-msm, dmaengine, linux-kernel,
	Dmitry Baryshkov, Georgi Djakov

On Fri, Feb 07, 2025 at 12:17:33PM -0800, Bjorn Andersson wrote:
> Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
> unavailable register")' made this read unconditional, in order to
> identify if the instance is BAM-NDP or BAM-Lite.
> But the BAM_REVISION register is not accessible on remotely managed BAM
> instances and attempts to access it causes the system to crash.
> 
> Move the access back to be conditional and expand the checks that was
> introduced to restore the old behavior when no revision information is
> available.
> 
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@kernel.org>
> Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

This patch fixes the most critical regression (the bus hang), but the
in_range(..., BAM_NDP) checks are also wrong. They do not consider the
plain "BAM" type where the register is apparently also available. The
check should be !in_range(..., BAM_LITE) instead to fix this.

I mentioned this twice to Md Sadre Alam [1, 2], but a fix was
unfortunately never sent for that part of the regression.

I think we should take Caleb's patch and revert the entire patch for the
6.14 cycle. There are several incorrect assumptions in the original
patch, it will be easier to review a fixed version with the full diff,
rather than looking at incremental fixups.

On a somewhat related note, I'm working on a patch series for bam_dma to
clean up the handling of remotely controlled BAMs. It will make it more
clear when it's safe to access BAM registers and when not, and should
allow reading the revision also for remotely controlled BAMs. This would
avoid the need for all these if (!bdev->bam_revision) checks.

Thanks,
Stephan

[1]: https://lore.kernel.org/linux-arm-msm/Z4D2jQNNW94qGIlv@linaro.org/
[2]: https://lore.kernel.org/linux-arm-msm/Z4_U19_QyH2RJvKW@linaro.org/

> ---
>  drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..d42d913492a8 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
>  	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
>  
>  	/* set descriptor threshold, start with 4 bytes */
> -	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -		     BAM_NDP_REVISION_END))
> +	if (!bdev->bam_revision ||
> +	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  		writel_relaxed(DEFAULT_CNT_THRSHLD,
>  			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  
> @@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  			maxburst = bchan->slave.src_maxburst;
>  		else
>  			maxburst = bchan->slave.dst_maxburst;
> -		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> -			     BAM_NDP_REVISION_END))
> +		if (!bdev->bam_revision ||
> +		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>  			writel_relaxed(maxburst,
>  				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>  	}
> @@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
>  	u32 val;
>  
>  	/* read revision and configuration information */
> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> -	if (!bdev->num_ees)
> +	if (!bdev->num_ees) {
> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>  
> -	bdev->bam_revision = val & REVISION_MASK;
> +		bdev->bam_revision = val & REVISION_MASK;
> +	}
>  
>  	/* check that configured EE is within range */
>  	if (bdev->ee >= bdev->num_ees)
> 
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250207-bam-read-fix-2b31297d3fa1
> 
> Best regards,
> -- 
> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-10  8:44 ` Stephan Gerhold
@ 2025-02-10 11:48   ` Md Sadre Alam
  2025-02-12  2:43   ` Bjorn Andersson
  1 sibling, 0 replies; 7+ messages in thread
From: Md Sadre Alam @ 2025-02-10 11:48 UTC (permalink / raw)
  To: Stephan Gerhold, Bjorn Andersson
  Cc: Vinod Koul, linux-arm-msm, dmaengine, linux-kernel,
	Dmitry Baryshkov, Georgi Djakov



On 2/10/2025 2:14 PM, Stephan Gerhold wrote:
> On Fri, Feb 07, 2025 at 12:17:33PM -0800, Bjorn Andersson wrote:
>> Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
>> unavailable register")' made this read unconditional, in order to
>> identify if the instance is BAM-NDP or BAM-Lite.
>> But the BAM_REVISION register is not accessible on remotely managed BAM
>> instances and attempts to access it causes the system to crash.
>>
>> Move the access back to be conditional and expand the checks that was
>> introduced to restore the old behavior when no revision information is
>> available.
>>
>> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
>> Reported-by: Georgi Djakov <djakov@kernel.org>
>> Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> This patch fixes the most critical regression (the bus hang), but the
> in_range(..., BAM_NDP) checks are also wrong. They do not consider the
> plain "BAM" type where the register is apparently also available. The
> check should be !in_range(..., BAM_LITE) instead to fix this.
> 
> I mentioned this twice to Md Sadre Alam [1, 2], but a fix was
> unfortunately never sent for that part of the regression.

I apologize for the delay. I was attending to a family member's medical 
emergency and couldn't address this sooner. I will test and post a new 
revision as soon as possible.

Thanks
Alam

> 
> I think we should take Caleb's patch and revert the entire patch for the
> 6.14 cycle. There are several incorrect assumptions in the original
> patch, it will be easier to review a fixed version with the full diff,
> rather than looking at incremental fixups.
> 
> On a somewhat related note, I'm working on a patch series for bam_dma to
> clean up the handling of remotely controlled BAMs. It will make it more
> clear when it's safe to access BAM registers and when not, and should
> allow reading the revision also for remotely controlled BAMs. This would
> avoid the need for all these if (!bdev->bam_revision) checks.
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-arm-msm/Z4D2jQNNW94qGIlv@linaro.org/
> [2]: https://lore.kernel.org/linux-arm-msm/Z4_U19_QyH2RJvKW@linaro.org/
> 
>> ---
>>   drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index c14557efd577..d42d913492a8 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
>>   	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
>>   
>>   	/* set descriptor threshold, start with 4 bytes */
>> -	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
>> -		     BAM_NDP_REVISION_END))
>> +	if (!bdev->bam_revision ||
>> +	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>>   		writel_relaxed(DEFAULT_CNT_THRSHLD,
>>   			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>>   
>> @@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>>   			maxburst = bchan->slave.src_maxburst;
>>   		else
>>   			maxburst = bchan->slave.dst_maxburst;
>> -		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
>> -			     BAM_NDP_REVISION_END))
>> +		if (!bdev->bam_revision ||
>> +		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
>>   			writel_relaxed(maxburst,
>>   				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
>>   	}
>> @@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
>>   	u32 val;
>>   
>>   	/* read revision and configuration information */
>> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>> -	if (!bdev->num_ees)
>> +	if (!bdev->num_ees) {
>> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>>   		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>>   
>> -	bdev->bam_revision = val & REVISION_MASK;
>> +		bdev->bam_revision = val & REVISION_MASK;
>> +	}
>>   
>>   	/* check that configured EE is within range */
>>   	if (bdev->ee >= bdev->num_ees)
>>
>> ---
>> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
>> change-id: 20250207-bam-read-fix-2b31297d3fa1
>>
>> Best regards,
>> -- 
>> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM
  2025-02-10  8:44 ` Stephan Gerhold
  2025-02-10 11:48   ` Md Sadre Alam
@ 2025-02-12  2:43   ` Bjorn Andersson
  1 sibling, 0 replies; 7+ messages in thread
From: Bjorn Andersson @ 2025-02-12  2:43 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Bjorn Andersson, Vinod Koul, Md Sadre Alam, linux-arm-msm,
	dmaengine, linux-kernel, Dmitry Baryshkov, Georgi Djakov

On Mon, Feb 10, 2025 at 09:44:30AM +0100, Stephan Gerhold wrote:
> On Fri, Feb 07, 2025 at 12:17:33PM -0800, Bjorn Andersson wrote:
> > Commit '57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing
> > unavailable register")' made this read unconditional, in order to
> > identify if the instance is BAM-NDP or BAM-Lite.
> > But the BAM_REVISION register is not accessible on remotely managed BAM
> > instances and attempts to access it causes the system to crash.
> > 
> > Move the access back to be conditional and expand the checks that was
> > introduced to restore the old behavior when no revision information is
> > available.
> > 
> > Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> > Reported-by: Georgi Djakov <djakov@kernel.org>
> > Closes: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> This patch fixes the most critical regression (the bus hang), but the
> in_range(..., BAM_NDP) checks are also wrong. They do not consider the
> plain "BAM" type where the register is apparently also available. The
> check should be !in_range(..., BAM_LITE) instead to fix this.
> 

Sorry, I must have not paid sufficient attention while browsing the
replies; and just tried to restore the one case I thought the author
didn't consider...

Thanks for staying on top of it. Revert makes total sense.

Regards,
Bjorn

> I mentioned this twice to Md Sadre Alam [1, 2], but a fix was
> unfortunately never sent for that part of the regression.
> 
> I think we should take Caleb's patch and revert the entire patch for the
> 6.14 cycle. There are several incorrect assumptions in the original
> patch, it will be easier to review a fixed version with the full diff,
> rather than looking at incremental fixups.
> 
> On a somewhat related note, I'm working on a patch series for bam_dma to
> clean up the handling of remotely controlled BAMs. It will make it more
> clear when it's safe to access BAM registers and when not, and should
> allow reading the revision also for remotely controlled BAMs. This would
> avoid the need for all these if (!bdev->bam_revision) checks.
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/linux-arm-msm/Z4D2jQNNW94qGIlv@linaro.org/
> [2]: https://lore.kernel.org/linux-arm-msm/Z4_U19_QyH2RJvKW@linaro.org/
> 
> > ---
> >  drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > index c14557efd577..d42d913492a8 100644
> > --- a/drivers/dma/qcom/bam_dma.c
> > +++ b/drivers/dma/qcom/bam_dma.c
> > @@ -445,8 +445,8 @@ static void bam_reset(struct bam_device *bdev)
> >  	writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL));
> >  
> >  	/* set descriptor threshold, start with 4 bytes */
> > -	if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> > -		     BAM_NDP_REVISION_END))
> > +	if (!bdev->bam_revision ||
> > +	    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
> >  		writel_relaxed(DEFAULT_CNT_THRSHLD,
> >  			       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
> >  
> > @@ -1006,8 +1006,8 @@ static void bam_apply_new_config(struct bam_chan *bchan,
> >  			maxburst = bchan->slave.src_maxburst;
> >  		else
> >  			maxburst = bchan->slave.dst_maxburst;
> > -		if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START,
> > -			     BAM_NDP_REVISION_END))
> > +		if (!bdev->bam_revision ||
> > +		    in_range(bdev->bam_revision, BAM_NDP_REVISION_START, BAM_NDP_REVISION_END))
> >  			writel_relaxed(maxburst,
> >  				       bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD));
> >  	}
> > @@ -1199,11 +1199,12 @@ static int bam_init(struct bam_device *bdev)
> >  	u32 val;
> >  
> >  	/* read revision and configuration information */
> > -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> > -	if (!bdev->num_ees)
> > +	if (!bdev->num_ees) {
> > +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> >  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
> >  
> > -	bdev->bam_revision = val & REVISION_MASK;
> > +		bdev->bam_revision = val & REVISION_MASK;
> > +	}
> >  
> >  	/* check that configured EE is within range */
> >  	if (bdev->ee >= bdev->num_ees)
> > 
> > ---
> > base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> > change-id: 20250207-bam-read-fix-2b31297d3fa1
> > 
> > Best regards,
> > -- 
> > Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-02-12  2:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-07 20:17 [PATCH] dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM Bjorn Andersson
2025-02-08 11:47 ` Sam Day
2025-02-08 12:42 ` Georgi Djakov
2025-02-09 15:19 ` David Heidelberg
2025-02-10  8:44 ` Stephan Gerhold
2025-02-10 11:48   ` Md Sadre Alam
2025-02-12  2:43   ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox