Re: [PATCH net-next 1/2] igb: Stop PTP related workqueues if aren't necessary

[Leon Romanovsky <leon@kernel.org>]

On Mon, Aug 14, 2023 at 03:16:05PM -0700, Tony Nguyen wrote:
> 
> 
> On 8/13/2023 2:01 AM, Leon Romanovsky wrote:
> > On Thu, Aug 10, 2023 at 10:54:09AM -0700, Tony Nguyen wrote:
> > > From: Alessio Igor Bogani <alessio.bogani@elettra.eu>
> > > 
> > > The workqueues ptp_tx_work and ptp_overflow_work are unconditionally allocated
> > > by igb_ptp_init(). Stop them if aren't necessary (ptp_clock_register() fails
> > > and CONFIG_PTP is disabled).
> > > 
> > > Signed-off-by: Alessio Igor Bogani <alessio.bogani@elettra.eu>
> > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/igb/igb_ptp.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > > index 405886ee5261..02276c922ac0 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > > @@ -1406,7 +1406,13 @@ void igb_ptp_init(struct igb_adapter *adapter)
> > >   		dev_info(&adapter->pdev->dev, "added PHC on %s\n",
> > >   			 adapter->netdev->name);
> > >   		adapter->ptp_flags |= IGB_PTP_ENABLED;
> > > +		return;
> > >   	}
> > > +
> > > +	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
> > > +		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
> > > +
> > > +	cancel_work_sync(&adapter->ptp_tx_work);
> > 
> > Is it possible to move work initialization to be after call to ptp_clock_register()?
> 
> I'm under the impression that everything should be ready to go before
> calling ptp_clock_register() so we shouldn't register until the workqueues
> are set up.

I honestly don't know.

Thanks

> 
> Thanks,
> Tony
> 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > index 405886ee5261..56fd2b0fe70c 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> > @@ -1386,11 +1386,6 @@ void igb_ptp_init(struct igb_adapter *adapter)
> >          }
> >          spin_lock_init(&adapter->tmreg_lock);
> > -       INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
> > -
> > -       if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
> > -               INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
> > -                                 igb_ptp_overflow_check);
> >          adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
> >          adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
> > @@ -1407,6 +1402,15 @@ void igb_ptp_init(struct igb_adapter *adapter)
> >                           adapter->netdev->name);
> >                  adapter->ptp_flags |= IGB_PTP_ENABLED;
> >          }
> > +
> > +       if (!adapter->ptp_clock)
> > +               return;
> > +
> > +       INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
> > +
> > +       if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
> > +               INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
> > +                                 igb_ptp_overflow_check);
> >   }
> >   /**
> > 
> > 
> > 
> > 
> > >   }
> > >   /**
> > > -- 
> > > 2.38.1
> > > 
> > >