Messages in this thread Patch in this message | | | Subject | Re: [PATCH] drm/amdgpu: allocate large structures dynamically | From | Christian König <> | Date | Thu, 7 May 2020 08:42:51 +0200 |
| |
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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
| |