Reports indicates that some userspace applications try to merge more than
80k of fences into a single dma_fence_array leading to a warning from
kzalloc() that the requested size becomes to big.
While that is clearly an userspace bug we should probably handle that case
gracefully in the kernel.
So we can either reject requests to merge more than a reasonable amount of
fences (64k maybe?) or we can start to use kvzalloc() instead of kzalloc().
This patch here does the later.
Signed-off-by: Christian König <christian.koenig(a)amd.com>
CC: stable(a)vger.kernel.org
---
drivers/dma-buf/dma-fence-array.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 8a08ffde31e7..46ac42bcfac0 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -119,8 +119,8 @@ static void dma_fence_array_release(struct dma_fence *fence)
for (i = 0; i < array->num_fences; ++i)
dma_fence_put(array->fences[i]);
- kfree(array->fences);
- dma_fence_free(fence);
+ kvfree(array->fences);
+ kvfree_rcu(fence, rcu);
}
static void dma_fence_array_set_deadline(struct dma_fence *fence,
@@ -153,7 +153,7 @@ struct dma_fence_array *dma_fence_array_alloc(int num_fences)
{
struct dma_fence_array *array;
- return kzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL);
+ return kvzalloc(struct_size(array, callbacks, num_fences), GFP_KERNEL);
}
EXPORT_SYMBOL(dma_fence_array_alloc);
--
2.34.1
Am 16.10.24 um 18:43 schrieb Adrián Larumbe:
> On 16.10.2024 15:12, Christian König wrote:
>> Am 15.10.24 um 01:31 schrieb Adrián Larumbe:
>>> Doesn't make any functional difference because generic dma_fence is the
>>> first panfrost_fence structure member, but I guess it doesn't hurt either.
>> As discussed with Sima we want to push into the exactly opposite direction
>> because that requires that the panfrost module stays loaded as long as fences
>> are around.
> Does that mean in future commits the struct dma_fence_ops' .release pointer will be
> done with altogether?
Yes, exactly that's the idea.
As a first step I'm preparing patches right now to enforce using kmalloc
instead of driver brewed approaches for dma_fence handling.
Regards,
Christian.
>
>> So clearly a NAK to this one here. Rather document on the structure that the
>> dma_fence structure must be the first member.
>>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com>
>>> ---
>>> drivers/gpu/drm/panfrost/panfrost_job.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> index 5d83c6a148ec..fa219f719bdc 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> @@ -85,9 +85,15 @@ static const char *panfrost_fence_get_timeline_name(struct dma_fence *fence)
>>> }
>>> }
>>> +static void panfrost_fence_release(struct dma_fence *fence)
>>> +{
>>> + kfree(to_panfrost_fence(fence));
>>> +}
>>> +
>>> static const struct dma_fence_ops panfrost_fence_ops = {
>>> .get_driver_name = panfrost_fence_get_driver_name,
>>> .get_timeline_name = panfrost_fence_get_timeline_name,
>>> + .release = panfrost_fence_release,
>>> };
>>> static struct dma_fence *panfrost_fence_create(struct panfrost_device *pfdev, int js_num)
On 15/10/2024 14:07, Jyothi Kumar Seerapu wrote:
> When high performance with multiple i2c messages in a single transfer
> is required, employ Block Event Interrupt (BEI) to trigger interrupts
> after specific messages transfer and the last message transfer,
> thereby reducing interrupts.
> For each i2c message transfer, a series of Transfer Request Elements(TREs)
> must be programmed, including config tre for frequency configuration,
> go tre for holding i2c address and dma tre for holding dma buffer address,
> length as per the hardware programming guide. For transfer using BEI,
> multiple I2C messages may necessitate the preparation of config, go,
> and tx DMA TREs. However, a channel TRE size of 64 is often insufficient,
> potentially leading to failures due to inadequate memory space.
>
> Adjust the channel TRE size through the device tree.
> The default size is 64, but clients can modify this value based on
> their heigher channel TRE size requirements.
>
> Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu(a)quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 132 +++++++++++++--------------
> 1 file changed, 66 insertions(+), 66 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 3d8410683402..c7c0e15ff9d3 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1064,7 +1064,7 @@
> };
>
> gpi_dma0: dma-controller@900000 {
> - #dma-cells = <3>;
> + #dma-cells = <4>;
> compatible = "qcom,sc7280-gpi-dma", "qcom,sm6350-gpi-dma";
> reg = <0 0x00900000 0 0x60000>;
> interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>,
> @@ -1114,8 +1114,8 @@
> "qup-memory";
> power-domains = <&rpmhpd SC7280_CX>;
> required-opps = <&rpmhpd_opp_low_svs>;
> - dmas = <&gpi_dma0 0 0 QCOM_GPI_I2C>,
> - <&gpi_dma0 1 0 QCOM_GPI_I2C>;
> + dmas = <&gpi_dma0 0 0 QCOM_GPI_I2C 64>,
> + <&gpi_dma0 1 0 QCOM_GPI_I2C 64>;
So everywhere is 64, thus this is fixed. Deduce it from the compatible
Best regards,
Krzysztof
Am 15.10.24 um 01:31 schrieb Adrián Larumbe:
> Doesn't make any functional difference because generic dma_fence is the
> first panfrost_fence structure member, but I guess it doesn't hurt either.
As discussed with Sima we want to push into the exactly opposite
direction because that requires that the panfrost module stays loaded as
long as fences are around.
So clearly a NAK to this one here. Rather document on the structure that
the dma_fence structure must be the first member.
Regards,
Christian.
> Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com>
> ---
> drivers/gpu/drm/panfrost/panfrost_job.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 5d83c6a148ec..fa219f719bdc 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -85,9 +85,15 @@ static const char *panfrost_fence_get_timeline_name(struct dma_fence *fence)
> }
> }
>
> +static void panfrost_fence_release(struct dma_fence *fence)
> +{
> + kfree(to_panfrost_fence(fence));
> +}
> +
> static const struct dma_fence_ops panfrost_fence_ops = {
> .get_driver_name = panfrost_fence_get_driver_name,
> .get_timeline_name = panfrost_fence_get_timeline_name,
> + .release = panfrost_fence_release,
> };
>
> static struct dma_fence *panfrost_fence_create(struct panfrost_device *pfdev, int js_num)
On 15-10-24, 17:37, Jyothi Kumar Seerapu wrote:
> When high performance with multiple i2c messages in a single transfer
> is required, employ Block Event Interrupt (BEI) to trigger interrupts
> after specific messages transfer and the last message transfer,
> thereby reducing interrupts.
>
> For each i2c message transfer, a series of Transfer Request Elements(TREs)
> must be programmed, including config tre for frequency configuration,
> go tre for holding i2c address and dma tre for holding dma buffer address,
> length as per the hardware programming guide. For transfer using BEI,
> multiple I2C messages may necessitate the preparation of config, go,
> and tx DMA TREs. However, a channel TRE size of 64 is often insufficient,
> potentially leading to failures due to inadequate memory space.
>
> Add additional argument to dma-cell property for channel TRE size.
> With this, adjust the channel TRE size via the device tree.
> The default size is 64, but clients can modify this value based on
> their specific requirements.
>
> Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu(a)quicinc.com>
> ---
> Documentation/devicetree/bindings/dma/qcom,gpi.yaml | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/qcom,gpi.yaml b/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> index 4df4e61895d2..002495921643 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> @@ -54,14 +54,16 @@ properties:
> maxItems: 13
>
> "#dma-cells":
> - const: 3
> + minItems: 3
> + maxItems: 4
> description: >
> DMA clients must use the format described in dma.txt, giving a phandle
> - to the DMA controller plus the following 3 integer cells:
> + to the DMA controller plus the following 4 integer cells:
> - channel: if set to 0xffffffff, any available channel will be allocated
> for the client. Otherwise, the exact channel specified will be used.
> - seid: serial id of the client as defined in the SoC documentation.
> - client: type of the client as defined in dt-bindings/dma/qcom-gpi.h
> + - channel-tre-size: size of the channel TRE (transfer ring element)
This is a firmware /software property, why should this be in hardware
description?
--
~Vinod
On Tue, Oct 15, 2024 at 05:37:46PM +0530, Jyothi Kumar Seerapu wrote:
> When high performance with multiple i2c messages in a single transfer
> is required, employ Block Event Interrupt (BEI) to trigger interrupts
> after specific messages transfer and the last message transfer,
> thereby reducing interrupts.
>
> For each i2c message transfer, a series of Transfer Request Elements(TREs)
> must be programmed, including config tre for frequency configuration,
> go tre for holding i2c address and dma tre for holding dma buffer address,
> length as per the hardware programming guide. For transfer using BEI,
> multiple I2C messages may necessitate the preparation of config, go,
> and tx DMA TREs. However, a channel TRE size of 64 is often insufficient,
> potentially leading to failures due to inadequate memory space.
>
> Add additional argument to dma-cell property for channel TRE size.
No such property 'dma-cell'
> With this, adjust the channel TRE size via the device tree.
> The default size is 64, but clients can modify this value based on
> their specific requirements.
>
> Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu(a)quicinc.com>
> ---
> Documentation/devicetree/bindings/dma/qcom,gpi.yaml | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/qcom,gpi.yaml b/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> index 4df4e61895d2..002495921643 100644
> --- a/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> +++ b/Documentation/devicetree/bindings/dma/qcom,gpi.yaml
> @@ -54,14 +54,16 @@ properties:
> maxItems: 13
>
> "#dma-cells":
> - const: 3
> + minItems: 3
> + maxItems: 4
> description: >
> DMA clients must use the format described in dma.txt, giving a phandle
> - to the DMA controller plus the following 3 integer cells:
> + to the DMA controller plus the following 4 integer cells:
> - channel: if set to 0xffffffff, any available channel will be allocated
> for the client. Otherwise, the exact channel specified will be used.
> - seid: serial id of the client as defined in the SoC documentation.
> - client: type of the client as defined in dt-bindings/dma/qcom-gpi.h
> + - channel-tre-size: size of the channel TRE (transfer ring element)
>
> iommus:
> maxItems: 1
> --
> 2.17.1
>
On 15/10/2024 14:07, Jyothi Kumar Seerapu wrote:
> When high performance with multiple i2c messages in a single transfer
> is required, employ Block Event Interrupt (BEI) to trigger interrupts
> after specific messages transfer and the last message transfer,
> thereby reducing interrupts.
>
> For each i2c message transfer, a series of Transfer Request Elements(TREs)
> must be programmed, including config tre for frequency configuration,
> go tre for holding i2c address and dma tre for holding dma buffer address,
> length as per the hardware programming guide. For transfer using BEI,
> multiple I2C messages may necessitate the preparation of config, go,
> and tx DMA TREs. However, a channel TRE size of 64 is often insufficient,
> potentially leading to failures due to inadequate memory space.
Please kindly test the patches before you sent them. Upstream is not a
testing service.
Best regards,
Krzysztof
On Tue, 15 Oct 2024 17:37:46 +0530, Jyothi Kumar Seerapu wrote:
> When high performance with multiple i2c messages in a single transfer
> is required, employ Block Event Interrupt (BEI) to trigger interrupts
> after specific messages transfer and the last message transfer,
> thereby reducing interrupts.
>
> For each i2c message transfer, a series of Transfer Request Elements(TREs)
> must be programmed, including config tre for frequency configuration,
> go tre for holding i2c address and dma tre for holding dma buffer address,
> length as per the hardware programming guide. For transfer using BEI,
> multiple I2C messages may necessitate the preparation of config, go,
> and tx DMA TREs. However, a channel TRE size of 64 is often insufficient,
> potentially leading to failures due to inadequate memory space.
>
> Add additional argument to dma-cell property for channel TRE size.
> With this, adjust the channel TRE size via the device tree.
> The default size is 64, but clients can modify this value based on
> their specific requirements.
>
> Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu(a)quicinc.com>
> ---
> Documentation/devicetree/bindings/dma/qcom,gpi.yaml | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/dma/qcom,gpi.yaml: properties:#dma-cells: 'minItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/dma/qcom,gpi.yaml: properties:#dma-cells: 'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/202410151207…
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Sat, Oct 5, 2024 at 11:10 AM Pintu Kumar <quic_pintu(a)quicinc.com> wrote:
>
> These warnings/errors are reported by checkpatch.
> Fix them with minor changes to make it clean.
> No other functional changes.
>
> WARNING: Block comments use * on subsequent lines
> + /* only support discovering the end of the buffer,
> + but also allow SEEK_SET to maintain the idiomatic
>
> WARNING: Block comments use a trailing */ on a separate line
> + SEEK_END(0), SEEK_CUR(0) pattern */
>
> WARNING: Block comments use a trailing */ on a separate line
> + * before passing the sgt back to the exporter. */
>
> ERROR: "foo * bar" should be "foo *bar"
> +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
>
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> + d = debugfs_create_file("bufinfo", S_IRUGO, dma_buf_debugfs_dir,
>
> total: 1 errors, 4 warnings, 1746 lines checked
>
> Signed-off-by: Pintu Kumar <quic_pintu(a)quicinc.com>
Looks ok to me. Thanks for sending these cleanups!
Acked-by: John Stultz <jstultz(a)google.com>