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

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Tue, 15 Aug, 2023 08:23:01 +0300 Leon Romanovsky <leon@kernel.org> wrote:
> 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

I do not think it's an issue for the work initialization to be after the
call to ptp_clock_register after taking a quick read.

ptp_clock_register essentially sets up the needed infrastructure for the
ptp hardware clock (PHC) and exposes the hardware clock to the
userspace. None of the PHC operations seem to depend on scheduling work
related to the ptp_tx_work and ptp_overflow_work work_struct instances
from what I can tell. Essentially, the only case this order would matter
is if any of the adapter->ptp_caps operation callbacks depends on
scheduling related work. From what I can tell, this is not the case in
the igb driver.

>
>> 
>> 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
>> > > 
>> > > 

--
Thanks,

Rahul Rameshbabu