Re: [PATCH net-next v4 15/15] ixd: add devlink support

[Larysa Zaremba <larysa.zaremba@intel.com>]

Below is the answer to Sashiko feedback for this patch.

Sashiko says:
> commit d1a5268fa124e852cecef1e844a5c981bd3df46c
> Author: Amritha Nambiar <amritha.nambiar@intel.com>
> ixd: add devlink support
> This patch adds initial devlink support to the ixd driver, allocating a
> devlink instance per PCIe function and exposing serial number, device
> type, and virtchnl version through the info_get callback. Registration
> happens from the asynchronous init_task work item after virtchnl device
> init succeeds.
> > diff --git a/drivers/net/ethernet/intel/ixd/ixd.h b/drivers/net/ethernet/intel/ixd/ixd.h
> > index 8e3cd5bc5a84..343c5053045f 100644
> > --- a/drivers/net/ethernet/intel/ixd/ixd.h
> > +++ b/drivers/net/ethernet/intel/ixd/ixd.h
> > @@ -23,6 +23,7 @@ struct ixd_adapter {
> >  		struct delayed_work init_work;
> >  		u8 reset_retries;
> >  		u8 vc_retries;
> > +		bool success;
> >  	} init_task;
> Is encoding devlink registration state via this new success flag the
> right approach?
> The flag only exists to gate ixd_devlink_unregister():
>     static inline void ixd_devlink_unregister(struct ixd_adapter *adapter)
>     {
>         if (!adapter->init_task.success)
>             return;
>         devlink_unregister(priv_to_devlink(adapter));
>     }
> Devlink core already tracks registration state (devl_is_registered(),
> ASSERT_DEVLINK_NOT_REGISTERED). Any future code path that adds another
> devlink_register()/devlink_unregister() call site (for example a reset
> recovery or renegotiation flow) would have to remember to keep this
> flag in sync, and 'init succeeded' and 'devlink registered' may
> otherwise diverge over time.
> Would relying on the devlink core's own registration tracking be
> preferable here?
> 
devl_is_registered() is not accessible to drivers.
The idea here is that init_task can stop working correctly at any stage.
Once remove() cancels it, we should establish, what parts need to be undone.
In the future, enum could be of use for the consecutive stage, but a binary flag in enough for now.
> 
> > diff --git a/drivers/net/ethernet/intel/ixd/ixd_lib.c b/drivers/net/ethernet/intel/ixd/ixd_lib.c
> > index ce870aba143d..6f2b39cd985c 100644
> > --- a/drivers/net/ethernet/intel/ixd/ixd_lib.c
> > +++ b/drivers/net/ethernet/intel/ixd/ixd_lib.c
> > @@ -154,6 +155,8 @@ void ixd_init_task(struct work_struct *work)
> >  
> >  	if (!ixd_vc_dev_init(adapter)) {
> >  		adapter->init_task.vc_retries = 0;
> > +		adapter->init_task.success = true;
> > +		ixd_devlink_register(adapter);
> >  		return;
> >  	}
> Should devlink registration really be deferred to this asynchronous work
> callback and gated on virtchnl init success?
> If the device fails initialization permanently (reset never completes
> within 10 retries, or the virtchnl handshake fails after 5 retries in
> ixd_init_task()), the PCI device remains bound to the driver but no
> devlink instance is ever exposed. Users lose devlink dev show and
> devlink dev info visibility precisely in the failure modes where such
> diagnostics would be most useful.
> Sibling Intel drivers such as i40e and ice register devlink
> synchronously from probe() so the device is always visible.
> ixd_fill_device_name() already handles the uninitialized case by
> falling back to "UNKNOWN":
>     if (adapter->caps.device_type == cpu_to_le32(VIRTCHNL2_MEV_DEVICE))
>         snprintf(buf, buf_size, "%s", "MEV");
>     else
>         snprintf(buf, buf_size, "%s", "UNKNOWN");
> Could ixd_devlink_register() be moved into ixd_probe() so failed
> devices remain visible to devlink?
> 
We have nothing to expose, if have not yet initialized basic HW communication
ixd and idpf utilize async initialization, because e.g. after power cycle,
control plane can be unresponsive for some time