Re: [PATCH 2/5] drm/i915: Notify user about outdated dmc firmware

[Marc Herbert <Marc.Herbert@intel.com>]

> On machines that had 1.19 symlinked, in filesystem, execlist submission
> sometimes broke due to interrupt delivery problem. To reach a conclusion
> that it was csr firmware, before 1.21 was out, took quite amount of work.
>
> I bet there are still machines with 1.19 only, and we get to
> wade through error states trying to connect the dots.
>
> [...]
> So we are left with triaging. Which is true detective work as there are
> no traces of firmware versions nor loading success/fails on
> logs/error states.
>

I think this "Finished loading" log statement below should not just include the version
numbers, its level should also be increased to something like "INFO" so 
the DMC version always makes it to the logs. Collecting the error state is
less obvious and less usual than collecting logs; some users don't even know
about it.

>>>>> @@ -387,6 +390,12 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>>>>
>>>>>    	DRM_DEBUG_KMS("Finished loading %s v%u.%u\n", dev_priv->csr.fw_path,
>>>>>    		      csr->dmc_ver_major, csr->dmc_ver_minor);


    ---------

To be honest (and likely off-topic), I even think request_firmware() should log the
resolved symlink by default. Quick & dirty proof of concept below to illustrate.

The kernel is normally quick enough to squeal "TAINTED!". On the other hand
request_firmware() loads random binaries without even saying anywhere it did.
Rationale?

Marc

--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -344,17 +345,24 @@ static int fw_get_filesystem_firmware(struct device *device,
 		else
 			break;
 	}
-	__putname(path);
 
 	if (!rc) {
-		dev_dbg(device, "firmware: direct-loading firmware %s\n",
-			buf->fw_id);
+		// fallback on symlink in case lookup goes wrong
+		const char *resolved_sym = path;
+
+		struct path dp;
+		if (!kern_path(path, LOOKUP_FOLLOW, &dp))
+			resolved_sym = dp.dentry->d_name.name;
+
+		dev_info(device, "firmware: direct-loading firmware %s\n",
+			 resolved_sym);
 		mutex_lock(&fw_lock);
 		set_bit(FW_STATUS_DONE, &buf->status);
 		complete_all(&buf->completion);
 		mutex_unlock(&fw_lock);
 	}
 
+	__putname(path);
 	return rc;
 }
 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx