Re: [PATCH] drm/radeon/r100: enhance error handling in r100_cp_init_microcode
Fix some error in my previous email
On Tue, May 28, 2024 at 9:36 AM Zhouyi Zhou wrote:
>
> Thanks for reviewing the patch
>
> On Mon, May 27, 2024 at 3:58 PM Christian König
> 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
> >
> > > ---
> > > 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;
> > > }
> > >
> >
Re: [PATCH] drm/radeon/r100: enhance error handling in r100_cp_init_microcode
Thanks for reviewing the patch
On Mon, May 27, 2024 at 3:58 PM Christian König
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
>
> > ---
> > 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;
> > }
> >
>
[PATCH] drm/radeon/r100: enhance error handling in r100_cp_init_microcode
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
---
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:
return err;
}
--
2.34.1
Re: [PATCH] drm/radeon/r100: enhance error handling in r100_cp_init_microcode
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
---
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;
}
