Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init
On Mon, 2019-06-24 at 11:41 +0800, maowenan wrote: > > On 2019/6/23 2:13, Joe Perches wrote: > > On Sat, 2019-06-22 at 21:05 +0800, Mao Wenan wrote: > > > There is one warning: > > > drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’: > > > drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ > > > set but not used [-Wunused-but-set-variable] > > > int ret = 0; > > [] > > > v1->v2: change the subject for this patch; change the indenting when it > > > calls init_pmu_by_type; use the value 'ret' in > > > amdgpu_pmu_init(). > > [] > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > [] > > > @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev) > > > case CHIP_VEGA20: > > > /* init df */ > > > ret = init_pmu_by_type(adev, df_v3_6_attr_groups, > > > -"DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, > > > -DF_V3_6_MAX_COUNTERS); > > > +"DF", "amdgpu_df", > > > PERF_TYPE_AMDGPU_DF, > > > + > > > DF_V3_6_MAX_COUNTERS); > > > > trivia: > > > > The indentation change seems superfluous and > > appears to make the code harder to read. > > > > You could also cc Jonathan Kim who wrote all of this. > I think this is just display issue in mail format. It is correct that in > vi/vim. > The arguments are line up with '(' after my change. Use 8 character tabs and try again please. > @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)$ > ^Icase CHIP_VEGA20:$ > ^I^I/* init df */$ > ^I^Iret = init_pmu_by_type(adev, df_v3_6_attr_groups,$ > -^I^I^I^I "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$ > -^I^I^I^I DF_V3_6_MAX_COUNTERS);$ > +^I^I^I^I^I^I^I "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$ > +^I^I^I^I^I^I^I DF_V3_6_MAX_COUNTERS);$
Re: [PATCH -next v2] drm/amdgpu: return 'ret' in amdgpu_pmu_init
On 2019/6/23 2:13, Joe Perches wrote: > On Sat, 2019-06-22 at 21:05 +0800, Mao Wenan wrote: >> There is one warning: >> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’: >> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set >> but not used [-Wunused-but-set-variable] >> int ret = 0; > [] >> v1->v2: change the subject for this patch; change the indenting when it >> calls init_pmu_by_type; use the value 'ret' in >> amdgpu_pmu_init(). > [] >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > [] >> @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev) >> case CHIP_VEGA20: >> /* init df */ >> ret = init_pmu_by_type(adev, df_v3_6_attr_groups, >> - "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, >> - DF_V3_6_MAX_COUNTERS); >> + "DF", "amdgpu_df", >> PERF_TYPE_AMDGPU_DF, >> + >> DF_V3_6_MAX_COUNTERS); > > trivia: > > The indentation change seems superfluous and > appears to make the code harder to read. > > You could also cc Jonathan Kim who wrote all of this. I think this is just display issue in mail format. It is correct that in vi/vim. The arguments are line up with '(' after my change. @@ -252,8 +252,8 @@ int amdgpu_pmu_init(struct amdgpu_device *adev)$ ^Icase CHIP_VEGA20:$ ^I^I/* init df */$ ^I^Iret = init_pmu_by_type(adev, df_v3_6_attr_groups,$ -^I^I^I^I "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$ -^I^I^I^I DF_V3_6_MAX_COUNTERS);$ +^I^I^I^I^I^I^I "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,$ +^I^I^I^I^I^I^I DF_V3_6_MAX_COUNTERS);$ $ ^I^I/* other pmu types go here*/$ ^I^Ibreak;$ > > > > . >
[PATCH -next v3] drm/amdgpu: return 'ret' immediately if failed in amdgpu_pmu_init
There is one warning: drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’: drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] int ret = 0; ^ amdgpu_pmu_init() is called by amdgpu_device_init() in drivers/gpu/drm/amd/amdgpu/amdgpu_device.c, which will use the return value. So it should return 'ret' immediately if init_pmu_by_type() failed. amdgpu_device_init() r = amdgpu_pmu_init(adev); This patch is also to update the indenting on the arguments so they line up with the '('. Fixes: 9c7c85f7ea1f ("drm/amdgpu: add pmu counters") Signed-off-by: Mao Wenan --- v1->v2: change the subject for this patch; change the indenting when it calls init_pmu_by_type; use the value 'ret' in amdgpu_pmu_init(). v2->v3: change the subject for this patch; return 'ret' immediately if failed to call init_pmu_by_type(). drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c index 0e6dba9..b702322 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c @@ -252,8 +252,11 @@ int amdgpu_pmu_init(struct amdgpu_device *adev) case CHIP_VEGA20: /* init df */ ret = init_pmu_by_type(adev, df_v3_6_attr_groups, - "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, - DF_V3_6_MAX_COUNTERS); + "DF", "amdgpu_df", + PERF_TYPE_AMDGPU_DF, + DF_V3_6_MAX_COUNTERS); + if (ret) + return ret; /* other pmu types go here*/ break; -- 2.7.4
Re: [PATCH -next] drm/amdgpu: remove set but not used variables 'ret'
On Sun, 23 Jun 2019, Dan Carpenter wrote: > On Sat, Jun 22, 2019 at 01:43:19PM +0300, Dan Carpenter wrote: > > On Sat, Jun 22, 2019 at 11:03:14AM +0800, Mao Wenan wrote: > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > > index 0e6dba9..0bf4dd9 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > > @@ -246,12 +246,10 @@ static int init_pmu_by_type(struct amdgpu_device > > > *adev, > > > /* init amdgpu_pmu */ > > > int amdgpu_pmu_init(struct amdgpu_device *adev) > > > { > > > - int ret = 0; > > > - > > > switch (adev->asic_type) { > > > case CHIP_VEGA20: > > > /* init df */ > > > - ret = init_pmu_by_type(adev, df_v3_6_attr_groups, > > > + init_pmu_by_type(adev, df_v3_6_attr_groups, > > > "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, > > > DF_V3_6_MAX_COUNTERS); > > > > > > You're resending this for other reasons, but don't forget to update the > > indenting on the arguments so they still line up with the '('. > > > > Sorry, I was unclear. If you pull the init_pmu_by_type( back 6 > characters then you also need to pull the "DF" back 6 characters. > > init_pmu_by_type(adev, df_v3_6_attr_groups, "DF", "amdgpu_df", >PERF_TYPE_AMDGPU_DF, DF_V3_6_MAX_COUNTERS); > > You can actually fit it into two lines afterwards. My suggestion was to keep the ret = instead of discarding the indication of failure, so I think that this is not relevant. julia
Re: [PATCH -next] drm/amdgpu: remove set but not used variables 'ret'
On Sat, Jun 22, 2019 at 01:43:19PM +0300, Dan Carpenter wrote: > On Sat, Jun 22, 2019 at 11:03:14AM +0800, Mao Wenan wrote: > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > index 0e6dba9..0bf4dd9 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c > > @@ -246,12 +246,10 @@ static int init_pmu_by_type(struct amdgpu_device > > *adev, > > /* init amdgpu_pmu */ > > int amdgpu_pmu_init(struct amdgpu_device *adev) > > { > > - int ret = 0; > > - > > switch (adev->asic_type) { > > case CHIP_VEGA20: > > /* init df */ > > - ret = init_pmu_by_type(adev, df_v3_6_attr_groups, > > + init_pmu_by_type(adev, df_v3_6_attr_groups, > >"DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, > >DF_V3_6_MAX_COUNTERS); > > > You're resending this for other reasons, but don't forget to update the > indenting on the arguments so they still line up with the '('. > Sorry, I was unclear. If you pull the init_pmu_by_type( back 6 characters then you also need to pull the "DF" back 6 characters. init_pmu_by_type(adev, df_v3_6_attr_groups, "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF, DF_V3_6_MAX_COUNTERS); You can actually fit it into two lines afterwards. regards, dan carpenter ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx