On Tue, 08 Feb 2022, Namhyung Kim <[email protected]> wrote:
> With upcoming lock tracepoints config, it'd define some of lockdep
> functions without enabling CONFIG_LOCKDEP actually. The existing code
> assumes those functions will be removed by the preprocessor but it's
> not the case anymore. Let's protect the code with #ifdef's explicitly.
I don't understand why you can't keep the no-op stubs for
CONFIG_LOCKDEP=n.
BR,
Jani.
>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_wakeref.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
> index dfd87d082218..6e4b8d036283 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -106,8 +106,11 @@ void __intel_wakeref_init(struct intel_wakeref *wf,
> wf->wakeref = 0;
>
> INIT_DELAYED_WORK(&wf->work, __intel_wakeref_put_work);
> +
> +#ifdef CONFIG_LOCKDEP
> lockdep_init_map(&wf->work.work.lockdep_map,
> "wakeref.work", &key->work, 0);
> +#endif
> }
>
> int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
--
Jani Nikula, Intel Open Source Graphics Center
Hello,
On Tue, Feb 8, 2022 at 10:51 AM Jani Nikula <[email protected]> wrote:
>
> On Tue, 08 Feb 2022, Namhyung Kim <[email protected]> wrote:
> > With upcoming lock tracepoints config, it'd define some of lockdep
> > functions without enabling CONFIG_LOCKDEP actually. The existing code
> > assumes those functions will be removed by the preprocessor but it's
> > not the case anymore. Let's protect the code with #ifdef's explicitly.
>
> I don't understand why you can't keep the no-op stubs for
> CONFIG_LOCKDEP=n.
Because I want to use the lockdep annotation for other purposes.
But the workqueue lockdep_map was defined under LOCKDEP
only. Please see the description in the cover letter.
https://lore.kernel.org/all/[email protected]/
Thanks,
Namhyung
On Tue, 08 Feb 2022, Namhyung Kim <[email protected]> wrote:
> Hello,
>
> On Tue, Feb 8, 2022 at 10:51 AM Jani Nikula <[email protected]> wrote:
>>
>> On Tue, 08 Feb 2022, Namhyung Kim <[email protected]> wrote:
>> > With upcoming lock tracepoints config, it'd define some of lockdep
>> > functions without enabling CONFIG_LOCKDEP actually. The existing code
>> > assumes those functions will be removed by the preprocessor but it's
>> > not the case anymore. Let's protect the code with #ifdef's explicitly.
>>
>> I don't understand why you can't keep the no-op stubs for
>> CONFIG_LOCKDEP=n.
>
> Because I want to use the lockdep annotation for other purposes.
> But the workqueue lockdep_map was defined under LOCKDEP
> only. Please see the description in the cover letter.
>
> https://lore.kernel.org/all/[email protected]/
So lockdep_init_map() might still be there and build just fine for
CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites
in #ifdefs depending on the purpose? I'm not convinced yet.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Wed, 09 Feb 2022 15:49:01 +0200
Jani Nikula <[email protected]> wrote:
> > Because I want to use the lockdep annotation for other purposes.
> > But the workqueue lockdep_map was defined under LOCKDEP
> > only. Please see the description in the cover letter.
> >
> > https://lore.kernel.org/all/[email protected]/
>
> So lockdep_init_map() might still be there and build just fine for
> CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites
> in #ifdefs depending on the purpose? I'm not convinced yet.
I addressed this already. I suggested to add a "raw" variant that turns to
a nop when LOCKDEP is not enabled. That is, for those locations that are
only for working with LOCKDEP, the call will be:
lockdep_init_map_raw()
This will differentiate the locations that are for just lockdep and those
that are for both lockdep and tracing.
-- Steve
Hello,
On Wed, Feb 9, 2022 at 8:27 AM Steven Rostedt <[email protected]> wrote:
>
> On Wed, 09 Feb 2022 15:49:01 +0200
> Jani Nikula <[email protected]> wrote:
>
> > > Because I want to use the lockdep annotation for other purposes.
> > > But the workqueue lockdep_map was defined under LOCKDEP
> > > only. Please see the description in the cover letter.
> > >
> > > https://lore.kernel.org/all/[email protected]/
> >
> > So lockdep_init_map() might still be there and build just fine for
> > CONFIG_LOCKDEP=n, but now we're actually required to wrap all call sites
> > in #ifdefs depending on the purpose? I'm not convinced yet.
Because work_struct.lockdep_map is there only if CONFIG_LOCKDEP=y.
>
> I addressed this already. I suggested to add a "raw" variant that turns to
> a nop when LOCKDEP is not enabled. That is, for those locations that are
> only for working with LOCKDEP, the call will be:
>
> lockdep_init_map_raw()
>
> This will differentiate the locations that are for just lockdep and those
> that are for both lockdep and tracing.
Yep, this should be fine if it's actually defined on CONFIG_LOCKDEP=y.
Thanks,
Namhyung