On Fri, 28 Aug 2026, Andi Shyti <[email protected]> wrote:
> Hi Krzysztof,
>
> ...
>
>> static int intel_fw_table_check(const struct intel_forcewake_range *ranges,
>> unsigned int num_ranges,
>> - bool is_watertight)
>> + bool is_watertight,
>> + const struct drm_device *drm)
First, we generally don't pass around struct drm_device *, it's struct
drm_i915_private * in i915.
Second, the highest level context parameters should be first. This one
looks like an afterthought.
>> {
>> unsigned int i;
>> s32 prev;
>> @@ -36,22 +39,22 @@ static int intel_fw_table_check(const struct
>> intel_forcewake_range *ranges,
>> for (i = 0, prev = -1; i < num_ranges; i++, ranges++) {
>> /* Check that the table is watertight */
>> if (is_watertight && (prev + 1) != (s32)ranges->start) {
>> - pr_err("%s: entry[%d]:(%x, %x) is not watertight to
>> previous (%x)\n",
>> - __func__, i, ranges->start, ranges->end, prev);
>> + drm_err(drm, "%s: entry[%d]:(%x, %x) is not watertight
>> to previous (%x)\n",
>> + __func__, i, ranges->start, ranges->end, prev);
>> return -EINVAL;
>> }
>>
>> /* Check that the table never goes backwards */
>> if (prev >= (s32)ranges->start) {
>> - pr_err("%s: entry[%d]:(%x, %x) is less than the
>> previous (%x)\n",
>> - __func__, i, ranges->start, ranges->end, prev);
>> + drm_err(drm, "%s: entry[%d]:(%x, %x) is less than the
>> previous (%x)\n",
>> + __func__, i, ranges->start, ranges->end, prev);
>> return -EINVAL;
>> }
>>
>> /* Check that the entry is valid */
>> if (ranges->start >= ranges->end) {
>> - pr_err("%s: entry[%d]:(%x, %x) has negative length\n",
>> - __func__, i, ranges->start, ranges->end);
>> + drm_err(drm, "%s: entry[%d]:(%x, %x) has negative
>> length\n",
>> + __func__, i, ranges->start, ranges->end);
>> return -EINVAL;
>> }
>>
>> @@ -126,7 +129,8 @@ int intel_uncore_mock_selftests(void)
>> for (i = 0; i < ARRAY_SIZE(fw); i++) {
>> err = intel_fw_table_check(fw[i].ranges,
>> fw[i].num_ranges,
>> - fw[i].is_watertight);
>> + fw[i].is_watertight,
>> + NULL);
>
> arrghh! Here we are again at the drm_err(NULL,...) case.
>
> You could eventually do in intel_fw_table_check()
>
> if (drm)
> drm_err(drm, ...);
> else
> pr_err(...);
drm_err() handles NULL drm_device just fine.
>
> but I think it's not worth the effort, just leave pr_err().
> Besides, passing the drm device is quite out of the scope of the
> function itself, it's just odd!
But agreed.
BR,
Jani.
>
> Andi
>
>> if (err)
>> return err;
>> }
--
Jani Nikula, Intel
