| Message ID | 20240527012018.351223-1-zhouzhouyi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/radeon/r100: enhance error handling in r100_cp_init_microcode | expand |
Am 27.05.24 um 03:20 schrieb Zhouyi Zhou: > In r100_cp_init_microcode, if rdev->family don't match any of > if statement, fw_name will be NULL, which will cause > gcc (11.4.0 powerpc64le-linux-gnu) complain: > > In function ‘r100_cp_init_microcode’, > inlined from ‘r100_cp_init’ at drivers/gpu/drm/radeon/r100.c:1136:7: > ./include/linux/printk.h:457:44: warning: ‘%s’ directive argument is null [-Wformat-overflow=] > 457 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) > > Above warning is emitted during the rcutorture test in > in PPC VM of Opensource Lab of Oregon State Univerisity. > > Enhance error handling in r100_cp_init_microcode, let r100_cp_init_microcode > return with -EINVAL when none of chip families is matched. > > Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com> > --- > drivers/gpu/drm/radeon/r100.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > index 0b1e19345f43..4f8a1bdd9365 100644 > --- a/drivers/gpu/drm/radeon/r100.c > +++ b/drivers/gpu/drm/radeon/r100.c > @@ -1055,6 +1055,11 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > (rdev->family == CHIP_RV570)) { > DRM_INFO("Loading R500 Microcode\n"); > fw_name = FIRMWARE_R520; > + } else { > + pr_err("radeon_cp: Failed to load firmware \"%d\"\n", > + rdev->family); > + err = -EINVAL; > + goto out; > } > > err = request_firmware(&rdev->me_fw, fw_name, rdev->dev); > @@ -1067,6 +1072,8 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > release_firmware(rdev->me_fw); > rdev->me_fw = NULL; > } > + > +out: That looks superfluous, just return -EINVAL directly in the else case above. Apart from that this is for ~15year old hardware. I'm a bit reluctant adding code for something that old even when this change here looks harmless. Is there a plan to complain about that in an automated checker? If yes then the change is probably justified, if no then I would rather not do it. Regards, Christian. > return err; > } >
Thanks for reviewing the patch On Mon, May 27, 2024 at 3:58 PM Christian König <christian.koenig@amd.com> wrote: > > Am 27.05.24 um 03:20 schrieb Zhouyi Zhou: > > In r100_cp_init_microcode, if rdev->family don't match any of > > if statement, fw_name will be NULL, which will cause > > gcc (11.4.0 powerpc64le-linux-gnu) complain: > > > > In function ‘r100_cp_init_microcode’, > > inlined from ‘r100_cp_init’ at drivers/gpu/drm/radeon/r100.c:1136:7: > > ./include/linux/printk.h:457:44: warning: ‘%s’ directive argument is null [-Wformat-overflow=] > > 457 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) > > > > Above warning is emitted during the rcutorture test in > > in PPC VM of Opensource Lab of Oregon State Univerisity. > > > > Enhance error handling in r100_cp_init_microcode, let r100_cp_init_microcode > > return with -EINVAL when none of chip families is matched. > > > > Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com> > > > --- > > drivers/gpu/drm/radeon/r100.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > > index 0b1e19345f43..4f8a1bdd9365 100644 > > --- a/drivers/gpu/drm/radeon/r100.c > > +++ b/drivers/gpu/drm/radeon/r100.c > > @@ -1055,6 +1055,11 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > > (rdev->family == CHIP_RV570)) { > > DRM_INFO("Loading R500 Microcode\n"); > > fw_name = FIRMWARE_R520; > > + } else { > > + pr_err("radeon_cp: Failed to load firmware \"%d\"\n", > > + rdev->family); > > + err = -EINVAL; > > + goto out; > > } > > > > err = request_firmware(&rdev->me_fw, fw_name, rdev->dev); > > @@ -1067,6 +1072,8 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > > release_firmware(rdev->me_fw); > > rdev->me_fw = NULL; > > } > > + > > +out: > > That looks superfluous, just return -EINVAL directly in the else case above. > > Apart from that this is for ~15year old hardware. I'm a bit reluctant > adding code for something that old even when this change here looks > harmless. > > Is there a plan to complain about that in an automated checker? If yes > then the change is probably justified, if no then I would rather not do it. The warning is emitted when I invoke following commands in ubuntu 22.04 (ppc64le) linux$make allmodconfig linux$make drivers/gpu/drm/radeon/r100.o (a quick alternative to 'make -j$nproc) But everything is OK when I invoke following commands in ubuntu 22.04 (ppc64le) linux$make allmodconfig LLVM=1 linux$make drivers/gpu/drm/radeon/r100.o LLVM=1 And I can't reproduce the warning in the x86 environment, so I guess this phenomenon is toolchain related ;-) Thanks again Regards, Zhouyi > > Regards, > Christian. > > > return err; > > } > > >
Fix some error in my previous email On Tue, May 28, 2024 at 9:36 AM Zhouyi Zhou <zhouzhouyi@gmail.com> wrote: > > Thanks for reviewing the patch > > On Mon, May 27, 2024 at 3:58 PM Christian König > <christian.koenig@amd.com> wrote: > > > > Am 27.05.24 um 03:20 schrieb Zhouyi Zhou: > > > In r100_cp_init_microcode, if rdev->family don't match any of > > > if statement, fw_name will be NULL, which will cause > > > gcc (11.4.0 powerpc64le-linux-gnu) complain: > > > > > > In function ‘r100_cp_init_microcode’, > > > inlined from ‘r100_cp_init’ at drivers/gpu/drm/radeon/r100.c:1136:7: > > > ./include/linux/printk.h:457:44: warning: ‘%s’ directive argument is null [-Wformat-overflow=] > > > 457 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) > > > > > > Above warning is emitted during the rcutorture test in > > > in PPC VM of Opensource Lab of Oregon State Univerisity. > > > > > > Enhance error handling in r100_cp_init_microcode, let r100_cp_init_microcode > > > return with -EINVAL when none of chip families is matched. > > > > > > Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com> > > > > > --- > > > drivers/gpu/drm/radeon/r100.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > > > index 0b1e19345f43..4f8a1bdd9365 100644 > > > --- a/drivers/gpu/drm/radeon/r100.c > > > +++ b/drivers/gpu/drm/radeon/r100.c > > > @@ -1055,6 +1055,11 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > > > (rdev->family == CHIP_RV570)) { > > > DRM_INFO("Loading R500 Microcode\n"); > > > fw_name = FIRMWARE_R520; > > > + } else { > > > + pr_err("radeon_cp: Failed to load firmware \"%d\"\n", > > > + rdev->family); > > > + err = -EINVAL; > > > + goto out; > > > } > > > > > > err = request_firmware(&rdev->me_fw, fw_name, rdev->dev); > > > @@ -1067,6 +1072,8 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) > > > release_firmware(rdev->me_fw); > > > rdev->me_fw = NULL; > > > } > > > + > > > +out: > > > > That looks superfluous, just return -EINVAL directly in the else case above. > > > > Apart from that this is for ~15year old hardware. I'm a bit reluctant > > adding code for something that old even when this change here looks > > harmless. > > > > Is there a plan to complain about that in an automated checker? If yes > > then the change is probably justified, if no then I would rather not do it. > The warning is emitted when I invoke following commands in ubuntu > 22.04 (ppc64le) > linux$make allmodconfig > linux$make drivers/gpu/drm/radeon/r100.o (a quick alternative to 'make -j$nproc) the command should be make -j(number of CPUs in my machine), please don't invoke 'make -j$nproc' which is my mistake, this command is very dangerous, and make my machine hung ;-( Sorry for the trouble Thanks again Zhouyi > But everything is OK when I invoke following commands in ubuntu 22.04 (ppc64le) > linux$make allmodconfig LLVM=1 > linux$make drivers/gpu/drm/radeon/r100.o LLVM=1 > > And I can't reproduce the warning in the x86 environment, so I guess > this phenomenon > is toolchain related ;-) > > Thanks again > Regards, > Zhouyi > > > > Regards, > > Christian. > > > > > return err; > > > } > > > > >
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index 0b1e19345f43..4f8a1bdd9365 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -1055,6 +1055,11 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) (rdev->family == CHIP_RV570)) { DRM_INFO("Loading R500 Microcode\n"); fw_name = FIRMWARE_R520; + } else { + pr_err("radeon_cp: Failed to load firmware \"%d\"\n", + rdev->family); + err = -EINVAL; + goto out; } err = request_firmware(&rdev->me_fw, fw_name, rdev->dev); @@ -1067,6 +1072,8 @@ static int r100_cp_init_microcode(struct radeon_device *rdev) release_firmware(rdev->me_fw); rdev->me_fw = NULL; } + +out: return err; }
In r100_cp_init_microcode, if rdev->family don't match any of if statement, fw_name will be NULL, which will cause gcc (11.4.0 powerpc64le-linux-gnu) complain: In function ‘r100_cp_init_microcode’, inlined from ‘r100_cp_init’ at drivers/gpu/drm/radeon/r100.c:1136:7: ./include/linux/printk.h:457:44: warning: ‘%s’ directive argument is null [-Wformat-overflow=] 457 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) Above warning is emitted during the rcutorture test in in PPC VM of Opensource Lab of Oregon State Univerisity. Enhance error handling in r100_cp_init_microcode, let r100_cp_init_microcode return with -EINVAL when none of chip families is matched. Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com> --- drivers/gpu/drm/radeon/r100.c | 7 +++++++ 1 file changed, 7 insertions(+)