Re: [PATCH] drm/amdgpu: allocate large structures dynamically

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 06.05.20 um 21:01 schrieb Joe Perches:
On Tue, 2020-05-05 at 16:01 +0200, Arnd Bergmann wrote:
After the structure was padded to 1024 bytes, it is no longer
suitable for being a local variable, as the function surpasses
the warning limit for 32-bit architectures:

drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:587:5: error: stack frame size of 1072 bytes in function 'amdgpu_ras_feature_enable' [-Werror,-Wframe-larger-than=]
int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
     ^

Use kzalloc() instead to get it from the heap.
[]
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
[]
@@ -588,19 +588,23 @@ int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
  		struct ras_common_if *head, bool enable)
  {
  	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
-	union ta_ras_cmd_input info;
+	union ta_ras_cmd_input *info;
  	int ret;
if (!con)
  		return -EINVAL;
+ info = kzalloc(sizeof(union ta_ras_cmd_input), GFP_KERNEL);
Spaces were used for indentation here not a tab.
It might be useful to run your proposed patches through checkpatch

Is this an actual bug fix as the previous use didn't
zero unused info members?

+	if (!info)
+		return -ENOMEM;
+
  	if (!enable) {
-		info.disable_features = (struct ta_ras_disable_features_input) {
+		info->disable_features = (struct ta_ras_disable_features_input) {
  			.block_id =  amdgpu_ras_block_to_ta(head->block),
  			.error_type = amdgpu_ras_error_to_ta(head->type),
  		};
  	} else {
-		info.enable_features = (struct ta_ras_enable_features_input) {
+		info->enable_features = (struct ta_ras_enable_features_input) {
  			.block_id =  amdgpu_ras_block_to_ta(head->block),
  			.error_type = amdgpu_ras_error_to_ta(head->type),
  		};
@@ -609,26 +613,33 @@ int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
  	/* Do not enable if it is not allowed. */
  	WARN_ON(enable && !amdgpu_ras_is_feature_allowed(adev, head));
  	/* Are we alerady in that state we are going to set? */
-	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
-		return 0;
+	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head))) {
And trivia:

The !! uses with bool seem unnecessary and it's probably better
to make amdgpu_ras_is_feature_enabled to return bool.

Maybe something like:
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 538895cfd862..05c4eaf0ddfa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -526,16 +526,16 @@ void amdgpu_ras_parse_status_code(struct amdgpu_device* adev,
  }
/* feature ctl begin */
-static int amdgpu_ras_is_feature_allowed(struct amdgpu_device *adev,
-		struct ras_common_if *head)
+static bool amdgpu_ras_is_feature_allowed(struct amdgpu_device *adev,
+					  struct ras_common_if *head)
  {
  	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
return con->hw_supported & BIT(head->block);
  }
-static int amdgpu_ras_is_feature_enabled(struct amdgpu_device *adev,
-		struct ras_common_if *head)
+static bool amdgpu_ras_is_feature_enabled(struct amdgpu_device *adev,
+					  struct ras_common_if *head)
  {
  	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
@@ -560,7 +560,7 @@ static int __amdgpu_ras_feature_enable(struct amdgpu_device *adev,
  	 */
  	if (!amdgpu_ras_is_feature_allowed(adev, head))
  		return 0;
-	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
+	if (!(enable ^ amdgpu_ras_is_feature_enabled(adev, head)))

And while we are at improving coding style I think that writing this as "if (enabled == amdgpu_ras_is_feature_enabled(adev, head))" would be much more readable.

Christian.

  		return 0;
if (enable) {
@@ -609,7 +609,7 @@ int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
  	/* Do not enable if it is not allowed. */
  	WARN_ON(enable && !amdgpu_ras_is_feature_allowed(adev, head));
  	/* Are we alerady in that state we are going to set? */
-	if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
+	if (!(enable ^ amdgpu_ras_is_feature_enabled(adev, head)))
  		return 0;
if (!amdgpu_ras_intr_triggered()) {



_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux