[PATCH net v3 0/3] Bug fixes from XDP and perout series

[PATCH net v3 0/3] Bug fixes from XDP and perout series

[Meghana Malladi]

This patch series consists of bug fixes from the XDP/perout series:
1. Fixes a kernel warning that occurs when bringing down the
   network interface.
2. Resolves a potential NULL pointer dereference in the
   emac_xmit_xdp_frame() function.
3. Resolves a potential NULL pointer dereference in the
   icss_iep_perout_enable() function

v2: https://lore.kernel.org/all/20250321081313.37112-1-m-malladi@ti.com/

Changes from v2 (v3-v2):
- Posting to net tree as the features have been merged to net.

Meghana Malladi (3):
  net: ti: icssg-prueth: Fix kernel warning while bringing down network
    interface
  net: ti: icssg-prueth: Fix possible NULL pointer dereference inside
    emac_xmit_xdp_frame()
  net: ti: icss-iep: Fix possible NULL pointer dereference for perout
    request

 drivers/net/ethernet/ti/icssg/icss_iep.c     | 4 ++--
 drivers/net/ethernet/ti/icssg/icssg_common.c | 9 ++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)


base-commit: 2eb6c6a34cb1c22b09b219390cdff0f02cd90258
-- 
2.43.0

[PATCH net v3 1/3] net: ti: icssg-prueth: Fix kernel warning while bringing down network interface

[Meghana Malladi]

During network interface initialization, the NIC driver needs to register
its Rx queue with the XDP, to ensure the incoming XDP buffer carries a
pointer reference to this info and is stored inside xdp_rxq_info.

While this struct isn't tied to XDP prog, if there are any changes in
Rx queue, the NIC driver needs to stop the Rx queue by unregistering
with XDP before purging and reallocating memory. Drop page_pool destroy
during Rx channel reset as this is already handled by XDP during
xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
following warning:

warning logs: https://gist.github.com/MeghanaMalladiTI/eb627e5dc8de24e42d7d46572c13e576

Fixes: 46eeb90f03e0 ("net: ti: icssg-prueth: Use page_pool API for RX buffer allocation")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index 46f500b90b17..3c0ea9044e18 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -1215,9 +1215,6 @@ void prueth_reset_rx_chan(struct prueth_rx_chn *chn,
 					  prueth_rx_cleanup, !!i);
 	if (disable)
 		k3_udma_glue_disable_rx_chn(chn->rx_chn);
-
-	page_pool_destroy(chn->pg_pool);
-	chn->pg_pool = NULL;
 }
 EXPORT_SYMBOL_GPL(prueth_reset_rx_chan);
 
-- 
2.43.0

[PATCH net v3 2/3] net: ti: icssg-prueth: Fix possible NULL pointer dereference inside emac_xmit_xdp_frame()

[Meghana Malladi]

There is an error check inside emac_xmit_xdp_frame() function which
is called when the driver wants to transmit XDP frame, to check if
the allocated tx descriptor is NULL, if true to exit and return
ICSSG_XDP_CONSUMED implying failure in transmission.

In this case trying to free a descriptor which is NULL will result
in kernel crash due to NULL pointer dereference. Fix this error handling
and increase netdev tx_dropped stats in the caller of this function
if the function returns ICSSG_XDP_CONSUMED.

Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/70d8dd76-0c76-42fc-8611-9884937c82f5@stanley.mountain/
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---

Changes from v2(v3-v2):
- Add Reported-by tag and link to the bug reported by Dan Carpenter <dan.carpenter@linaro.org>

 drivers/net/ethernet/ti/icssg/icssg_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index 3c0ea9044e18..15c092a923d7 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -583,7 +583,7 @@ u32 emac_xmit_xdp_frame(struct prueth_emac *emac,
 	first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool);
 	if (!first_desc) {
 		netdev_dbg(ndev, "xdp tx: failed to allocate descriptor\n");
-		goto drop_free_descs;	/* drop */
+		return ICSSG_XDP_CONSUMED;	/* drop */
 	}
 
 	if (page) { /* already DMA mapped by page_pool */
@@ -671,8 +671,10 @@ static u32 emac_run_xdp(struct prueth_emac *emac, struct xdp_buff *xdp,
 
 		q_idx = smp_processor_id() % emac->tx_ch_num;
 		result = emac_xmit_xdp_frame(emac, xdpf, page, q_idx);
-		if (result == ICSSG_XDP_CONSUMED)
+		if (result == ICSSG_XDP_CONSUMED) {
+			ndev->stats.tx_dropped++;
 			goto drop;
+		}
 
 		dev_sw_netstats_rx_add(ndev, xdpf->len);
 		return result;
-- 
2.43.0

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

[Meghana Malladi]

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,
(iep->pps_enabled is not cleared during interface bring down)
and driver will just return true even though there is no signal.
Fix this by setting pps and perout flags to false instead of
disabling perout to avoid possible null pointer dereference.

Fixes: 9b115361248d ("net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---

Changes from v2(v3-v2):
- Add Reported-by tag and link to the bug reported by Dan Carpenter <dan.carpenter@linaro.org>
- drop calling icss_iep_perout_enable() for disabling perout and set perout to false instead

 drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index b4a34c57b7b4..b70e4c482d74 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -820,9 +820,9 @@ int icss_iep_exit(struct icss_iep *iep)
 	icss_iep_disable(iep);
 
 	if (iep->pps_enabled)
-		icss_iep_pps_enable(iep, false);
+		iep->pps_enabled = false;
 	else if (iep->perout_enabled)
-		icss_iep_perout_enable(iep, NULL, false);
+		iep->perout_enabled = false;
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH net v3 1/3] net: ti: icssg-prueth: Fix kernel warning while bringing down network interface

[Roger Quadros]

On 28/03/2025 12:24, Meghana Malladi wrote:
> During network interface initialization, the NIC driver needs to register
> its Rx queue with the XDP, to ensure the incoming XDP buffer carries a
> pointer reference to this info and is stored inside xdp_rxq_info.
> 
> While this struct isn't tied to XDP prog, if there are any changes in
> Rx queue, the NIC driver needs to stop the Rx queue by unregistering
> with XDP before purging and reallocating memory. Drop page_pool destroy
> during Rx channel reset as this is already handled by XDP during
> xdp_rxq_info_unreg (Rx queue unregister), failing to do will cause the
> following warning:
> 
> warning logs: https://gist.github.com/MeghanaMalladiTI/eb627e5dc8de24e42d7d46572c13e576
> 
> Fixes: 46eeb90f03e0 ("net: ti: icssg-prueth: Use page_pool API for RX buffer allocation")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> Reviewed-by: Simon Horman <horms@kernel.org>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH net v3 2/3] net: ti: icssg-prueth: Fix possible NULL pointer dereference inside emac_xmit_xdp_frame()

[Roger Quadros]

On 28/03/2025 12:24, Meghana Malladi wrote:
> There is an error check inside emac_xmit_xdp_frame() function which
> is called when the driver wants to transmit XDP frame, to check if
> the allocated tx descriptor is NULL, if true to exit and return
> ICSSG_XDP_CONSUMED implying failure in transmission.
> 
> In this case trying to free a descriptor which is NULL will result
> in kernel crash due to NULL pointer dereference. Fix this error handling
> and increase netdev tx_dropped stats in the caller of this function
> if the function returns ICSSG_XDP_CONSUMED.
> 
> Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/70d8dd76-0c76-42fc-8611-9884937c82f5@stanley.mountain/
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> Reviewed-by: Simon Horman <horms@kernel.org>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

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

[Roger Quadros]

Meghana,

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?

And, what if you brought 2 interfaces of the same ICSS instances up
but put only 1 interface down and up?

> (iep->pps_enabled is not cleared during interface bring down)
> and driver will just return true even though there is no signal.
> Fix this by setting pps and perout flags to false instead of
> disabling perout to avoid possible null pointer dereference.
> 
> Fixes: 9b115361248d ("net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
> 
> Changes from v2(v3-v2):
> - Add Reported-by tag and link to the bug reported by Dan Carpenter <dan.carpenter@linaro.org>
> - drop calling icss_iep_perout_enable() for disabling perout and set perout to false instead
> 
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index b4a34c57b7b4..b70e4c482d74 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -820,9 +820,9 @@ int icss_iep_exit(struct icss_iep *iep)
>  	icss_iep_disable(iep);
>  
>  	if (iep->pps_enabled)
> -		icss_iep_pps_enable(iep, false);
> +		iep->pps_enabled = false;
>  	else if (iep->perout_enabled)
> -		icss_iep_perout_enable(iep, NULL, false);
> +		iep->perout_enabled = false;
>  
>  	return 0;
>  }

-- 
cheers,
-roger

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

[Malladi, Meghana]

On 4/2/2025 5:58 PM, Roger Quadros wrote:
> Meghana,
> 
> 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.

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

please refer for more details: 
https://lore.kernel.org/all/20241211135941.1800240-1-m-malladi@ti.com/

>> (iep->pps_enabled is not cleared during interface bring down)
>> and driver will just return true even though there is no signal.
>> Fix this by setting pps and perout flags to false instead of
>> disabling perout to avoid possible null pointer dereference.
>>
>> Fixes: 9b115361248d ("net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init")
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>
>> Changes from v2(v3-v2):
>> - Add Reported-by tag and link to the bug reported by Dan Carpenter <dan.carpenter@linaro.org>
>> - drop calling icss_iep_perout_enable() for disabling perout and set perout to false instead
>>
>>   drivers/net/ethernet/ti/icssg/icss_iep.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> index b4a34c57b7b4..b70e4c482d74 100644
>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> @@ -820,9 +820,9 @@ int icss_iep_exit(struct icss_iep *iep)
>>   	icss_iep_disable(iep);
>>   
>>   	if (iep->pps_enabled)
>> -		icss_iep_pps_enable(iep, false);
>> +		iep->pps_enabled = false;
>>   	else if (iep->perout_enabled)
>> -		icss_iep_perout_enable(iep, NULL, false);
>> +		iep->perout_enabled = false;
>>   
>>   	return 0;
>>   }
> 

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

[Paolo Abeni]

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.

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

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

[Roger Quadros]

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.

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.

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

-- 
cheers,
-roger

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

[Malladi, Meghana]

On 4/3/2025 4:55 PM, 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
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> updqndalvOwRqgXOXDPJf9wy4vKojW68gavBCIsz5DlBLvSeawwT53qgFGcvIm0ULRBQkJv028AcR194Ei9ZDPp5ily-uAw$>
> ZjQcmQRYFpfptBannerEnd
> 
> 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.
> 

The reason behind this change is to fix the possible null pointer 
dereference which will occur when icss_iep_perout_enable(iep, NULL, 
false) is called during icss_iep_exit(), my bad for not mentioning it 
clearly in the commit message.

> Skimmir over the code, icss_iep_pps_enable() could indeed avoid clearing
> the flag under some circumstances is that the reason?
> 

icss_iep_pps_enable() does indeed clear the flag, but 
icss_iep_perout_enable() doesn't due to the null pointer dereference. So 
instead of calling these functions for clearing the flag, we can simply 
just clear the flag directly.

> Possibly a more describing commit message would help.
> 

Yes agreed. I will update it for v4.

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

No it is the correct scenario. When you bring down all the existing 
interfaces (be it one or two, when whatever is up goes down) you unload 
the existing firmware and clear the all the driver configuration (this 
flag also needs to be cleared) to ensure everything starts with a clean 
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
> 

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