RE: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero to PTR_ERR

[Divya.Koppera]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, December 6, 2022 7:39 PM
> To: Divya Koppera - I30481 <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> richardcochran@xxxxxxxxx; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v5 net-next 2/2] net: phy: micrel: Fix warn: passing zero
> to PTR_ERR
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > > > -     if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > > > -         !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > > > -             return 0;
> > > > -
> > >
> > > Why are you removing this ?
> > >
> >
> > I got review comment from Richard in v2 as below, making it as consistent
> by checking ptp_clock. So removed it in next revision.
> >
> > " > static int lan8814_ptp_probe_once(struct phy_device *phydev)
> > > {
> > >         struct lan8814_shared_priv *shared = phydev->shared->priv;
> > >
> > >         if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) ||
> > >             !IS_ENABLED(CONFIG_NETWORK_PHY_TIMESTAMPING))
> > >                 return 0;
> >
> > It is weird to use macros here, but not before calling ptp_clock_register.
> > Make it consistent by checking shared->ptp_clock instead.
> > That is also better form."
> 
> O.K. If Richard said this fine.
> 
> Just out of interest, could you disassemble lan8814_ptp_probe_once() when
> CONFIG_PTP_1588_CLOCK is disabled, with and without this check?
> 

If I understand correctly,

With (!IS_ENABLED(CONFIG_PTP_1588_CLOCK) check, initialization of mutex and members of shared->ptp_clock_info need to be done in first function.
Without above check ptp_clock_register should be done in second function. Correct me if I'm wrong.

In this case, if first function is bypassed because of clock disable config, no need to go to second function. Could you please check below solution, if that works fine?

> My guess is, when PTP is disabled, the mutex still gets initialised, all the
> member of shared->ptp_clock_info are set. The optimise cannot remove it.
> With the macro check, the function is empty. So you end up with a slightly
> bigger text size.
> 

If that is the case, I'll keep the CONFIG_PTP_1588_CLOCK disabled check before calling lan8814_ptp_probe_once.

Next thing is if CONFIG_PTP_1588_CLOCK disabled check pass then ptp_clock_register will never return null because we are bypassing hardware clock check before calling function itself.
So, I can remove ptp_clock null check too. Let me know if this works, I'll do changes in next revision.

>        Andrew