Re: [PATCH v2 01/23] drm/xe: Error handling in xe_force_wake_get()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 19 Sep 2024, "Nilawar, Badal" <badal.nilawar@intel.com> wrote:
> On 18-09-2024 12:49, Jani Nikula wrote:
>> On Wed, 18 Sep 2024, "Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com> wrote:
>>> On 18-09-2024 00:20, Matthew Brost wrote:
>>>> On Tue, Sep 17, 2024 at 11:18:47AM +0530, Nilawar, Badal wrote:
>>>>> On 13-09-2024 18:47, Ghimiray, Himal Prasad wrote:
>>>>>> Agreed implementation/usage will be same, will use explicit type for
>>>>>> clarity.
>>>>>> IMO typedef unsigned int xe_wakeref_t is sufficient instead of
>>>>>> typedef unsigned long xe_wakeref_t;
>>>>>
>>>>> I agree with this.
>>>>>
>>>>
>>>> What? Really? I thought it was pretty clear rule in kernel programing
>>>> not use typedefs [1]. Reading through conditions acceptable and I don't
>>>> use anything applies to this series, maybe a) applies but not really
>>>> convinced. The example in a) is a pte_t which can likely change based on
>>>> platform target whereas here we only have one target and see no reason
>>>> this needs to be opaque.
>>>>
>>>> Matt
>>>>
>>>> [1] https://www.kernel.org/doc/html/v4.14/process/coding-style.html#typedefs
>>>
>>>
>>> While running checkpatch on my changes, patchwork had also issued a
>>> WARNING: NEW_TYPEDEFS: do not add new typedefs. I reviewed the usage in
>>> the Linux kernel tree and found it used in many places, which led me to
>>> assume it was safe. I now realize that I should have been more careful
>>> in understanding the context of its usage and referred to the kernel
>>> coding guidelines. This was an oversight on my part.
>>>
>>> Since this doesn’t impact the CI or runtime, I will avoid reverting to
>>> unsigned int immediately and will hold off until I receive the other
>>> review comments. I will incorporate the changes to revert it in
>>> subsequent versions while also addressing the other review comments.
>>> Thank you for bringing this to the attention.
>> 
>> If you end up replicating intel_wakeref_t from i915, and go as deep as
>> the rabbit hole goes, you'll realize intel_wakeref_t is a pointer
>> disguised as an unsigned long. It's a struct ref_tracker * when you have
>> certain configs enabled.
>> 
>> You could just use struct ref_tracker * everywhere. It's an opaque type
>> to start with.
>
> The original idea of using typedef for the fw return mask was for the 
> sake of clarity. However, Matt B pointed that the use of typedef in this 
> instance is not in accordance with the Linux kernel coding standards. 
> Additionally, I agree with Matt B that there is no need for the fw 
> return mask to be opaque; therefore, it is preferable to maintain the 
> use of unsigned int.

I'm not sure it's a hot idea to explicitly state that the return value
is a domain mask. The callers shouldn't need to care, should they?

For example:

 +	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
 +	if (fw_ref != XE_FORCEWAKE_ALL) {

Under what conditions do you expect this to happen? Shouldn't
xe_force_wake_get() flag cases where it couldn't deliver what you asked?

BR,
Jani.


-- 
Jani Nikula, Intel