| Message ID | 20250328102403.2626974-4-m-malladi@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bug fixes from XDP and perout series | expand |
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; > }
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; >> } >
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; }
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(-)