Re: [PATCH net v3 3/3] net: ti: icss-iep: Fix possible NULL pointer dereference for perout request

[Malladi, Meghana]

Hi Roger,

On 4/4/2025 1:17 PM, Roger Quadros wrote:
> On 03/04/2025 14:25, Paolo Abeni wrote:
> > On 4/2/25 2:37 PM, Malladi, Meghana wrote:
> > > On 4/2/2025 5:58 PM, Roger Quadros wrote:
> > > > On 28/03/2025 12:24, Meghana Malladi wrote:
> > > > > ICSS IEP driver has flags to check if perout or pps has been enabled
> > > > > at any given point of time. Whenever there is request to enable or
> > > > > disable the signal, the driver first checks its enabled or disabled
> > > > > and acts accordingly.
> > > > >
> > > > > After bringing the interface down and up, calling PPS/perout enable
> > > > > doesn't work as the driver believes PPS is already enabled,
> > > >
> > > > How? aren't we calling icss_iep_pps_enable(iep, false)?
> > > > wouldn't this disable the IEP and clear the iep->pps_enabled flag?
> > >
> > > The whole purpose of calling icss_iep_pps_enable(iep, false) is to clear
> > > iep->pps_enabled flag, because if this flag is not cleared it hinders
> > > generating new pps signal (with the newly loaded firmware) once any of
> > > the interfaces are up (check icss_iep_perout_enable()), so instead of
> > > calling icss_iep_pps_enable(iep, false) I am just clearing the
> > > iep->pps_enabled flag.
> >
> > IDK what Roger thinks, but the above is not clear to me. I read it as
> > you are stating that icss_iep_pps_enable() indeed clears the flag, so i
> > don't see/follow the reasoning behind this change.
> >
> > Skimmir over the code, icss_iep_pps_enable() could indeed avoid clearing
> > the flag under some circumstances is that the reason?
> >
> > Possibly a more describing commit message would help.
>
> I would expect that modifying the xxx_enabled flag should be done only
> in the icss_iep_xxx_enable() function. Doing it anywhere else will be difficult
> to track/debug in the long term.

There is no problem with calling icss_iep_pps_enable() for clearing the 
pps_enable flag. Problem comes with icss_iep_perout_enable(), causing 
null pointer dereference for the NULL perout request argument we are 
passing just for clearing the perout_enable flag.

> I don't see why the flag needs to be set anywhere else. Maye better to
> improve logic inside icss_iep_pps_enable() like Paolo suggests.

Ok, one thing I can do is create a ptp_perout_request to disable perout 
instead of passing NULL to icss_iep_perout_enable(). What are your 
thoughts on this ?

> > > > And, what if you brought 2 interfaces of the same ICSS instances up
> > > > but put only 1 interface down and up?
> > >
> > > Then the flag need not be disabled if only one interface is brought down
> > > because the IEP is still alive and all the IEP configuration (including
> > > pps/perout) is still valid.
> >
> > I read the above as stating this fix is not correct in such scenario,
> > leading to the wrong final state.
> >
> > I think it would be better to either give a better reasoning about this
> > change in the commit message or refactor it to handle even such scenario,
> >
> > Thanks,
> >
> > Paolo