Re: [PATCH net v2] devlink: Delay health recover notification until devlink registered

[Jiri Pirko <jiri@resnulli.us>]

Thu, Aug 10, 2023 at 07:33:00PM CEST, kuba@kernel.org wrote:
>On Wed, 9 Aug 2023 23:35:21 +0300 Shay Drory wrote:
>> From one side, devl_register() is done last in device initialization
>> phase, in order to expose devlink to the user only when device is
>> ready. From second side, it is valid to create health reporters
>> during device initialization, in order to recover and/or notify the
>> user.
>> As a result, a health recover can be invoked before devl_register().
>> However, invoking health recover before devl_register() triggers a
>> WARN_ON.
>
>My comment on v1 wasn't clear enough, I guess.
>
>What I was trying to get across is that because drivers can take
>devl_lock(), devl_register() does not have to be last.
>
>AFAIU your driver does:
>
>  devlink_port_health_reporter_create()
>  ...
>  devlink_register()
>
>why not change it to do:
>
>  devl_lock()
>  devl_register()
>  devl_port_health_reporter_create()
>  ...
>  devl_unlock() # until unlock user space can't access the instance

This patch is not about user accessing it, this is about
notification that would be tried to send before the instance is
registered. So the lock scheme you suggest is not necessary. What helps
is to move devl_port_health_reporter_create() call after register.

We have the same issue with mlxsw where this notification may be called
before register from mlxsw_core_health_event_work()

Limiting the creation of health reporter only when instance is
registered does not seem like a good solution to me.

As with any other object, we postpone the notifications only after
register is done, that sounds fine, doesn't it?

>
>?
>