On Fri, 23 Mar 2018 19:03:47 +0100, Yaodong Li <yaodong.li@xxxxxxxxx>
wrote:
On 03/23/2018 05:27 AM, Michal Wajdeczko wrote:
On Fri, 23 Mar 2018 13:07:15 +0100, Sagar Arun Kamble
<sagar.a.kamble@xxxxxxxxx> wrote:
On 3/23/2018 4:53 PM, Piotr Piórkowski wrote:
If GuC firmware is not available on the system and we load i915 with
enable
GuC, then we hit this null pointer dereference issue:
Patch looks good but I have query on usage of enable_guc modparam.
enable_guc modparam will enable GuC/HuC only if firmware is available.
During modparam sanitization phase, code will only check for firmware
name, code will attempt to check if firmware file exists.
Is it better if we won't even call into upload FW once we found the FW
fetching was failed?
If user sets to forcefully enable GuC/HuC on systems that don't have
firmware then it is user's fault.
Sure its user's fault, but event in such case we should just gracefully
abort driver load, without any NULL pointer BUG ;)
And note, that we will hit this bug not only when user force GuC with:
enable_guc=1 guc_firmware_path=/does/not/exist
but also when user just specify auto mode:
enable_guc=-1
when predefined firmwares are not present (not installed or removed)
it feels like it's still user's fault even with the auto mode. why the
user even set
the enable_guc to -1 after known the fact that the FWs are not present?
I mean should users be expected to know the fact that the auto mode
enable_guc = -1 will enable the GuC and HuC, if the guc_firmware_path
was left
to be none?
GuC fw path will not be none, as we have some hardcoded paths in the code.
And we don't tell the user that after turning on 'auto' mode, he has to
do something special. Only later in case of fw fetch errors, we are just
printing error message with extra help:
"%s: Failed to fetch firmware %s (error %d)\n"
"%s: Firmware can be downloaded from %s\n"
I agree we should make sure correct pointer access. it's a non-excusable
fault:)
The thing actually frustrated me was that we failed to screen such an
error
with the CI, but suddenly it broke things and tests with incorrect
configurations
(enable_guc set to 1 or -1 + No FW available). so maybe we should add a
case to
cover such a case in CI as well if we did have such test configurations.
I guess you can write such test on your own and submit patch to igt-dev ;)
/m
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
