Re: [PATCH v9 net-next 13/14] pds_core: publish events to the clients

[Leon Romanovsky <leon@kernel.org>]

On Mon, Apr 10, 2023 at 02:01:13PM -0700, Shannon Nelson wrote:
> On 4/9/23 10:11 AM, Leon Romanovsky wrote:
> > 
> > On Thu, Apr 06, 2023 at 04:41:42PM -0700, Shannon Nelson wrote:
> > > When the Core device gets an event from the device, or notices
> > > the device FW to be up or down, it needs to send those events
> > > on to the clients that have an event handler.  Add the code to
> > > pass along the events to the clients.
> > > 
> > > The entry points pdsc_register_notify() and pdsc_unregister_notify()
> > > are EXPORTed for other drivers that want to listen for these events.
> > > 
> > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> > > ---
> > >   drivers/net/ethernet/amd/pds_core/adminq.c |  2 ++
> > >   drivers/net/ethernet/amd/pds_core/core.c   | 32 ++++++++++++++++++++++
> > >   drivers/net/ethernet/amd/pds_core/core.h   |  3 ++
> > >   include/linux/pds/pds_common.h             |  2 ++
> > >   4 files changed, 39 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
> > > index 25c7dd0d37e5..bb18ac1aabab 100644
> > > --- a/drivers/net/ethernet/amd/pds_core/adminq.c
> > > +++ b/drivers/net/ethernet/amd/pds_core/adminq.c
> > > @@ -27,11 +27,13 @@ static int pdsc_process_notifyq(struct pdsc_qcq *qcq)
> > >                case PDS_EVENT_LINK_CHANGE:
> > >                        dev_info(pdsc->dev, "NotifyQ LINK_CHANGE ecode %d eid %lld\n",
> > >                                 ecode, eid);
> > > +                     pdsc_notify(PDS_EVENT_LINK_CHANGE, comp);
> > 
> > Aren't you "resending" standard netdev event?
> > It will be better to send only custom, specific to pds_core events,
> > while leaving general ones to netdev.
> 
> We have no netdev in pds_core, so we have to publish this to clients that
> might have a netdev or some other need to know.

I don't know netdev well enough if it is ok or not and maybe netdev will
sent this LINK_CHANGE by itself anyway.

Jakub???

> 
> > 
> > >                        break;
> > > 
> > >                case PDS_EVENT_RESET:
> > >                        dev_info(pdsc->dev, "NotifyQ RESET ecode %d eid %lld\n",
> > >                                 ecode, eid);
> > > +                     pdsc_notify(PDS_EVENT_RESET, comp);
> > 
> > We can argue if clients should get this event. Once reset is detected,
> > the pds_core should close devices by deleting aux drivers.
> 
> We can get a reset signal from the device when it has done a crash recovery
> or when it is preparing to do an update, and this allows clients to quiesce
> their operations when reset.state==0 and restart when they see
> reset.state==1

I don't think that it is safe behaviour from user POV. If FW resets
itself under the hood, how can client be sure that nothing changes
in its operation? Once FW reset occurs, it is much safer for the clients
to reconfigure everything.

Thanks