Re: [PATCH net 1/2] net: fec: Restart PPS after link state change

[Csókás Bence]

On 2024. 09. 25. 6:37, Wei Fang wrote:
> > -----Original Message-----
> > From: Csókás, Bence <csokas.bence@xxxxxxxxx>
> > Sent: 2024年9月24日 17:37
> > To: Frank Li <Frank.Li@xxxxxxxxxxxxx>; David S. Miller
> > <davem@xxxxxxxxxxxxx>; imx@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Csókás, Bence <csokas.bence@xxxxxxxxx>; Wei Fang <wei.fang@xxxxxxx>;
> > Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> > <xiaoning.wang@xxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub
> > Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Richard
> > Cochran <richardcochran@xxxxxxxxx>
> > Subject: [PATCH net 1/2] net: fec: Restart PPS after link state change
> The "v2" descriptor is missing in the subject, and correct the mail address
> of Frank.

My bad, sorry. get_maintainer.pl spat out the old email address, as that 
was their former committer address, and I wasn't paying attention.

> > +/* Restore PTP functionality after a reset */ void
> > +fec_ptp_restore_state(struct fec_enet_private *fep) {
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fep->tmreg_lock, flags);
> > +
> > +	/* Reset turned it off, so adjust our status flag */
> > +	fep->pps_enable = 0;
> > +
> > +	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
> > +
> > +	/* Restart PPS if needed */
> > +	if (fep->ptp_saved_state.pps_enable) {
>
> It's better to put " fep->pps_enable = 0" here so that it does
> not need to be set when PPS is disabled.

It doesn't hurt to set it to 0 when it's already 0, and it saves us 
having to unlock separately in the if {} and else blocks. Plus, after 
reset, PPS will be turned off unconditionally, since the actual HW gets 
reset.

Bence