diff mbox series

dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM

Message ID 20250207-bam-read-fix-v1-1-027975cf1a04@oss.qualcomm.com
State New
Headers show
Series dmaengine: qcom: bam_dma: Avoid accessing BAM_REVISION on remote BAM | expand

Commit Message

Bjorn Andersson Feb. 7, 2025, 8:17 p.m. UTC 
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(-)


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

Best regards,

Comments

Sam Day Feb. 8, 2025, 11:47 a.m. UTC | #1 
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
Georgi Djakov Feb. 8, 2025, 12:42 p.m. UTC | #2 
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
Stephan Gerhold Feb. 10, 2025, 8:44 a.m. UTC | #3 
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>
>
Md Sadre Alam Feb. 10, 2025, 11:48 a.m. UTC | #4 
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>
>>
diff mbox series

Patch

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)