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

["Malladi, Meghana" <m-malladi@ti.com>]

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