| Message ID | 20230116044517.310461-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next,v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action | expand |
On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: > The am65_cpts_release() function is registered as a devm_action in the > am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver > invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm > actions associated with the am65-cpsw driver's device. > > In the event of probe failure or probe deferral, the platform_drv_probe() > function invokes dev_pm_domain_detach() which powers off the CPSW and the > CPSW's CPTS hardware, both of which share the same power domain. Since the > am65_cpts_disable() function invoked by the am65_cpts_release() function > attempts to reset the CPTS hardware by writing to its registers, the CPTS > hardware is assumed to be powered on at this point. However, the hardware > is powered off before the devm actions are executed. > > Fix this by getting rid of the devm action for am65_cpts_release() and > invoking it directly on the cleanup and exit paths. > > Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > Reviewed-by: Roger Quadros <rogerq@kernel.org> > --- > Changes from v1: > 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This > error was reported by kernel test robot <lkp@intel.com> at: > https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ > 2. Collect Reviewed-by tag from Roger Quadros. > > v1: > https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ > drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- > drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ > 3 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > index 5cac98284184..00f25d8a026b 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, > return 0; > } > > +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) > +{ > + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? How is it possible to have common->cpts == NULL? And why do you need special am65_cpsw_cpts_cleanup() which does nothing except call to am65_cpts_release()? It will be more intuitive change the latter to be exported function. Thanks
On 16/01/23 13:00, Leon Romanovsky wrote: > On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >> The am65_cpts_release() function is registered as a devm_action in the >> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >> actions associated with the am65-cpsw driver's device. >> >> In the event of probe failure or probe deferral, the platform_drv_probe() >> function invokes dev_pm_domain_detach() which powers off the CPSW and the >> CPSW's CPTS hardware, both of which share the same power domain. Since the >> am65_cpts_disable() function invoked by the am65_cpts_release() function >> attempts to reset the CPTS hardware by writing to its registers, the CPTS >> hardware is assumed to be powered on at this point. However, the hardware >> is powered off before the devm actions are executed. >> >> Fix this by getting rid of the devm action for am65_cpts_release() and >> invoking it directly on the cleanup and exit paths. >> >> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> Reviewed-by: Roger Quadros <rogerq@kernel.org> >> --- >> Changes from v1: >> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >> error was reported by kernel test robot <lkp@intel.com> at: >> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >> 2. Collect Reviewed-by tag from Roger Quadros. >> >> v1: >> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >> >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >> 3 files changed, 18 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index 5cac98284184..00f25d8a026b 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >> return 0; >> } >> >> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >> +{ >> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) > > Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if > am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? > > How is it possible to have common->cpts == NULL? Thank you for reviewing the patch. I realize now that checking CONFIG_TI_K3_AM65_CPTS is unnecessary. common->cpts remains NULL in the following cases: 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() function with a return value of 0 when cpts is disabled. 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function fails with an error. Of the above cases, the am65_cpsw_cpts_cleanup() function would have to handle cases 1 and 3, since the probe might fail at a later point, following which the probe cleanup path will invoke the am65_cpts_cpts_cleanup() function. This function then checks for common->cpts not being NULL, so that it can invoke the am65_cpts_release() function with this pointer. > > And why do you need special am65_cpsw_cpts_cleanup() which does nothing > except call to am65_cpts_release()? It will be more intuitive change > the latter to be exported function. The am65_cpts_release() function expects the cpts pointer to be valid. Thus, I had added the am65_cpsw_cpts_cleanup() function to conditionally invoke the am65_cpts_release() function whenever the cpts pointer is valid. Based on your suggestion, I believe that you want me to check for the cpts pointer being valid within the am65_cpts_release() function instead, so that the am65_cpsw_cpts_cleanup() function doesn't have to be added. Please let me know if this is what you meant. Regards, Siddharth.
On Mon, Jan 16, 2023 at 01:13:36PM +0530, Siddharth Vadapalli wrote: > > > On 16/01/23 13:00, Leon Romanovsky wrote: > > On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: > >> The am65_cpts_release() function is registered as a devm_action in the > >> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver > >> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm > >> actions associated with the am65-cpsw driver's device. > >> > >> In the event of probe failure or probe deferral, the platform_drv_probe() > >> function invokes dev_pm_domain_detach() which powers off the CPSW and the > >> CPSW's CPTS hardware, both of which share the same power domain. Since the > >> am65_cpts_disable() function invoked by the am65_cpts_release() function > >> attempts to reset the CPTS hardware by writing to its registers, the CPTS > >> hardware is assumed to be powered on at this point. However, the hardware > >> is powered off before the devm actions are executed. > >> > >> Fix this by getting rid of the devm action for am65_cpts_release() and > >> invoking it directly on the cleanup and exit paths. > >> > >> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") > >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >> Reviewed-by: Roger Quadros <rogerq@kernel.org> > >> --- > >> Changes from v1: > >> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This > >> error was reported by kernel test robot <lkp@intel.com> at: > >> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ > >> 2. Collect Reviewed-by tag from Roger Quadros. > >> > >> v1: > >> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ > >> > >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ > >> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- > >> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ > >> 3 files changed, 18 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> index 5cac98284184..00f25d8a026b 100644 > >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, > >> return 0; > >> } > >> > >> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) > >> +{ > >> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) > > > > Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if > > am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? > > > > How is it possible to have common->cpts == NULL? > > Thank you for reviewing the patch. I realize now that checking > CONFIG_TI_K3_AM65_CPTS is unnecessary. > > common->cpts remains NULL in the following cases: > 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. In this case am65_cpsw_cpts_cleanup() will NOP as well. > 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. It is an error and all callers unwind properly. > 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() > function with a return value of 0 when cpts is disabled. It is disabled by CONFIG_TI_K3_AM65_CPTS, which in turn will make am65_cpsw_cpts_cleanup() NOP. > 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function > fails with an error. > > Of the above cases, the am65_cpsw_cpts_cleanup() function would have to handle > cases 1 and 3, since the probe might fail at a later point, following which the > probe cleanup path will invoke the am65_cpts_cpts_cleanup() function. This > function then checks for common->cpts not being NULL, so that it can invoke the > am65_cpts_release() function with this pointer. I still don't see how it is possible. Thanks
On 16/01/23 15:34, Leon Romanovsky wrote: > On Mon, Jan 16, 2023 at 01:13:36PM +0530, Siddharth Vadapalli wrote: >> >> >> On 16/01/23 13:00, Leon Romanovsky wrote: >>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>> The am65_cpts_release() function is registered as a devm_action in the >>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>> actions associated with the am65-cpsw driver's device. >>>> >>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>> hardware is assumed to be powered on at this point. However, the hardware >>>> is powered off before the devm actions are executed. >>>> >>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>> invoking it directly on the cleanup and exit paths. >>>> >>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>> --- >>>> Changes from v1: >>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>> error was reported by kernel test robot <lkp@intel.com> at: >>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>> >>>> v1: >>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>> >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> index 5cac98284184..00f25d8a026b 100644 >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>> return 0; >>>> } >>>> >>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>> +{ >>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>> >>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>> >>> How is it possible to have common->cpts == NULL? >> >> Thank you for reviewing the patch. I realize now that checking >> CONFIG_TI_K3_AM65_CPTS is unnecessary. >> >> common->cpts remains NULL in the following cases: >> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. > > In this case am65_cpsw_cpts_cleanup() will NOP as well. > >> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. > > It is an error and all callers unwind properly. > >> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >> function with a return value of 0 when cpts is disabled. > > It is disabled by CONFIG_TI_K3_AM65_CPTS, which in turn will make > am65_cpsw_cpts_cleanup() NOP. > >> 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function >> fails with an error. >> >> Of the above cases, the am65_cpsw_cpts_cleanup() function would have to handle >> cases 1 and 3, since the probe might fail at a later point, following which the >> probe cleanup path will invoke the am65_cpts_cpts_cleanup() function. This >> function then checks for common->cpts not being NULL, so that it can invoke the >> am65_cpts_release() function with this pointer. > > I still don't see how it is possible. You are right! I apologize for not analyzing the cases well enough. The only case where common->cpts will remain NULL and the am65_cpsw_cpts_cleanup() function is invoked, is the case where the CONFIG_TI_K3_AM65_CPTS config is disabled. As you had pointed it out, in this case, the am65_cpts_release() is NOP, so passing the NULL pointer common->cpts will have no effect. With this, I understand that the am65_cpsw_cpts_cleanup() function is unnecessary like you had mentioned, and am65_cpts_release() can be directly invoked for common->cpts. Please let me know if my understanding is correct. If so, I will implement this in the v3 patch. Regards, Siddharth.
On Mon, Jan 16, 2023 at 04:07:16PM +0530, Siddharth Vadapalli wrote: > > > On 16/01/23 15:34, Leon Romanovsky wrote: > > On Mon, Jan 16, 2023 at 01:13:36PM +0530, Siddharth Vadapalli wrote: > >> > >> > >> On 16/01/23 13:00, Leon Romanovsky wrote: > >>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: > >>>> The am65_cpts_release() function is registered as a devm_action in the > >>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver > >>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm > >>>> actions associated with the am65-cpsw driver's device. > >>>> > >>>> In the event of probe failure or probe deferral, the platform_drv_probe() > >>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the > >>>> CPSW's CPTS hardware, both of which share the same power domain. Since the > >>>> am65_cpts_disable() function invoked by the am65_cpts_release() function > >>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS > >>>> hardware is assumed to be powered on at this point. However, the hardware > >>>> is powered off before the devm actions are executed. > >>>> > >>>> Fix this by getting rid of the devm action for am65_cpts_release() and > >>>> invoking it directly on the cleanup and exit paths. > >>>> > >>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") > >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> > >>>> --- > >>>> Changes from v1: > >>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This > >>>> error was reported by kernel test robot <lkp@intel.com> at: > >>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ > >>>> 2. Collect Reviewed-by tag from Roger Quadros. > >>>> > >>>> v1: > >>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ > >>>> > >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ > >>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- > >>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ > >>>> 3 files changed, 18 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> index 5cac98284184..00f25d8a026b 100644 > >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, > >>>> return 0; > >>>> } > >>>> > >>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) > >>>> +{ > >>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) > >>> > >>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if > >>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? > >>> > >>> How is it possible to have common->cpts == NULL? > >> > >> Thank you for reviewing the patch. I realize now that checking > >> CONFIG_TI_K3_AM65_CPTS is unnecessary. > >> > >> common->cpts remains NULL in the following cases: > >> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. > > > > In this case am65_cpsw_cpts_cleanup() will NOP as well. > > > >> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. > > > > It is an error and all callers unwind properly. > > > >> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() > >> function with a return value of 0 when cpts is disabled. > > > > It is disabled by CONFIG_TI_K3_AM65_CPTS, which in turn will make > > am65_cpsw_cpts_cleanup() NOP. > > > >> 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function > >> fails with an error. > >> > >> Of the above cases, the am65_cpsw_cpts_cleanup() function would have to handle > >> cases 1 and 3, since the probe might fail at a later point, following which the > >> probe cleanup path will invoke the am65_cpts_cpts_cleanup() function. This > >> function then checks for common->cpts not being NULL, so that it can invoke the > >> am65_cpts_release() function with this pointer. > > > > I still don't see how it is possible. > > You are right! I apologize for not analyzing the cases well enough. The only > case where common->cpts will remain NULL and the am65_cpsw_cpts_cleanup() > function is invoked, is the case where the CONFIG_TI_K3_AM65_CPTS config is > disabled. As you had pointed it out, in this case, the am65_cpts_release() is > NOP, so passing the NULL pointer common->cpts will have no effect. > > With this, I understand that the am65_cpsw_cpts_cleanup() function is > unnecessary like you had mentioned, and am65_cpts_release() can be directly > invoked for common->cpts. Please let me know if my understanding is correct. If > so, I will implement this in the v3 patch. Yes, you understood me right. Thanks > > Regards, > Siddharth.
Hi Siddharth, On 16/01/2023 09:43, Siddharth Vadapalli wrote: > > > On 16/01/23 13:00, Leon Romanovsky wrote: >> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>> The am65_cpts_release() function is registered as a devm_action in the >>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>> actions associated with the am65-cpsw driver's device. >>> >>> In the event of probe failure or probe deferral, the platform_drv_probe() >>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>> hardware is assumed to be powered on at this point. However, the hardware >>> is powered off before the devm actions are executed. >>> >>> Fix this by getting rid of the devm action for am65_cpts_release() and >>> invoking it directly on the cleanup and exit paths. >>> >>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>> --- >>> Changes from v1: >>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>> error was reported by kernel test robot <lkp@intel.com> at: >>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>> 2. Collect Reviewed-by tag from Roger Quadros. >>> >>> v1: >>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>> >>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>> 3 files changed, 18 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>> index 5cac98284184..00f25d8a026b 100644 >>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>> return 0; >>> } >>> >>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>> +{ >>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >> >> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >> >> How is it possible to have common->cpts == NULL? > > Thank you for reviewing the patch. I realize now that checking > CONFIG_TI_K3_AM65_CPTS is unnecessary. > > common->cpts remains NULL in the following cases: > 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. > 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. > 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() > function with a return value of 0 when cpts is disabled. In this case common->cpts is not NULL and is set to error pointer. Probe will continue normally. Is it OK to call any of the cpts APIs with invalid handle? Also am65_cpts_release() will be called with invalid handle. > 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function > fails with an error. In this case common->cpts is not NULL and will invoke am65_cpts_release() with invalid handle. > > Of the above cases, the am65_cpsw_cpts_cleanup() function would have to handle > cases 1 and 3, since the probe might fail at a later point, following which the > probe cleanup path will invoke the am65_cpts_cpts_cleanup() function. This > function then checks for common->cpts not being NULL, so that it can invoke the > am65_cpts_release() function with this pointer. > >> >> And why do you need special am65_cpsw_cpts_cleanup() which does nothing >> except call to am65_cpts_release()? It will be more intuitive change >> the latter to be exported function. > > The am65_cpts_release() function expects the cpts pointer to be valid. Thus, I > had added the am65_cpsw_cpts_cleanup() function to conditionally invoke the > am65_cpts_release() function whenever the cpts pointer is valid. Based on your > suggestion, I believe that you want me to check for the cpts pointer being valid > within the am65_cpts_release() function instead, so that the > am65_cpsw_cpts_cleanup() function doesn't have to be added. Please let me know > if this is what you meant. > > Regards, > Siddharth. cheers, -roger
Roger, Leon, On 16/01/23 21:31, Roger Quadros wrote: > Hi Siddharth, > > On 16/01/2023 09:43, Siddharth Vadapalli wrote: >> >> >> On 16/01/23 13:00, Leon Romanovsky wrote: >>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>> The am65_cpts_release() function is registered as a devm_action in the >>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>> actions associated with the am65-cpsw driver's device. >>>> >>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>> hardware is assumed to be powered on at this point. However, the hardware >>>> is powered off before the devm actions are executed. >>>> >>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>> invoking it directly on the cleanup and exit paths. >>>> >>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>> --- >>>> Changes from v1: >>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>> error was reported by kernel test robot <lkp@intel.com> at: >>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>> >>>> v1: >>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>> >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> index 5cac98284184..00f25d8a026b 100644 >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>> return 0; >>>> } >>>> >>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>> +{ >>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>> >>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>> >>> How is it possible to have common->cpts == NULL? >> >> Thank you for reviewing the patch. I realize now that checking >> CONFIG_TI_K3_AM65_CPTS is unnecessary. >> >> common->cpts remains NULL in the following cases: I realized that the cases I mentioned are not explained clearly. Therefore, I will mention the cases again, along with the section of code they correspond to, in order to make it clear. Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. This corresponds to the following section within am65_cpsw_init_cpts(): if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) return 0; In this case, common->cpts remains NULL, but it is not a problem even if the am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is NOP. Thus, this case is not an issue. Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present in the device tree. This corresponds to the following section within am65_cpsw_init_cpts(): node = of_get_child_by_name(dev->of_node, "cpts"); if (!node) { dev_err(dev, "%s cpts not found\n", __func__); return -ENOENT; } In this case as well, common->cpts remains NULL, but it is not a problem because the probe fails and the execution jumps to "err_of_clear", which doesn't invoke am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. Case-3 and Case-4 are described later in this mail. >> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. >> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. >> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >> function with a return value of 0 when cpts is disabled. > > In this case common->cpts is not NULL and is set to error pointer. > Probe will continue normally. > Is it OK to call any of the cpts APIs with invalid handle? > Also am65_cpts_release() will be called with invalid handle. Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had meant that the following section is executed within the am65_cpsw_init_cpts() function: Case-3: cpts = am65_cpts_create(dev, reg_base, node); if (IS_ERR(cpts)) { int ret = PTR_ERR(cpts); of_node_put(node); if (ret == -EOPNOTSUPP) { dev_info(dev, "cpts disabled\n"); return 0; } ...... } Leon, In the above code, when the section corresponding to: dev_info(dev, "cpts disabled\n"); is executed, CONFIG_TI_K3_AM65_CPTS is enabled. Therefore, the am65_cpts_release() is not NOP. If the probe fails after the call to am65_cpsw_init_cpts(), then the am65_cpsw_cpts_cleanup() function will be called in the cleanup path of probe, which needs to check for common->cpts not being NULL. This is because common->cpts is NULL after returning 0 from the am65_cpsw_init_cpts() function at the dev_info(dev, "cpts disabled\n"); section. Thus, I believe that in this case, am65_cpts_release() shouldn't be invoked from the am65_cpsw_cpts_cleanup() function, since it would have already been invoked from am65_cpts_create()'s cleanup path. This can be ensured by checking whether common->cpts is NULL or not, before invoking am65_cpts_release() within am65_cpsw_cpts_cleanup(). > >> 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function >> fails with an error. > > In this case common->cpts is not NULL and will invoke am65_cpts_release() with > invalid handle. Case-4: The call to am65_cpts_create() within the am65_cpsw_init_cpts() function fails with an error. This corresponds to the following section within am65_cpsw_init_cpts(): cpts = am65_cpts_create(dev, reg_base, node); if (IS_ERR(cpts)) { ...... dev_err(dev, "cpts create err %d\n", ret); return ret; } Roger, If the call to am65_cpts_create() fails with an error other than -EOPNOTSUPP, which corresponds to Case-4, the call to am65_cpts_release() would have been invoked within the am65_cpts_create()'s cleanup path itself if necessary. Also, when the error is not -EOPNOTSUPP, the am65_cpsw_init_cpts() function returns an error, due to which the execution jumps to "err_of_clear" in am65_cpsw_nuss_probe(). Therefore, am65_cpsw_cpts_cleanup() is not invoked in this case, due to which common->cpts being NULL is not a problem. Roger, Leon, please review my comments and let me know. I think that Case-3 demands checking whether common->cpts is NULL or not, within the am65_cpsw_cpts_cleanup() function. Regards, Siddharth.
Siddharth, On 17/01/2023 07:00, Siddharth Vadapalli wrote: > Roger, Leon, > > On 16/01/23 21:31, Roger Quadros wrote: >> Hi Siddharth, >> >> On 16/01/2023 09:43, Siddharth Vadapalli wrote: >>> >>> >>> On 16/01/23 13:00, Leon Romanovsky wrote: >>>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>>> The am65_cpts_release() function is registered as a devm_action in the >>>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>>> actions associated with the am65-cpsw driver's device. >>>>> >>>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>>> hardware is assumed to be powered on at this point. However, the hardware >>>>> is powered off before the devm actions are executed. >>>>> >>>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>>> invoking it directly on the cleanup and exit paths. >>>>> >>>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>> --- >>>>> Changes from v1: >>>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>>> error was reported by kernel test robot <lkp@intel.com> at: >>>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>>> >>>>> v1: >>>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>>> >>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>> index 5cac98284184..00f25d8a026b 100644 >>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>>> return 0; >>>>> } >>>>> >>>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>>> +{ >>>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>>> >>>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>>> >>>> How is it possible to have common->cpts == NULL? >>> >>> Thank you for reviewing the patch. I realize now that checking >>> CONFIG_TI_K3_AM65_CPTS is unnecessary. >>> >>> common->cpts remains NULL in the following cases: > > I realized that the cases I mentioned are not explained clearly. Therefore, I > will mention the cases again, along with the section of code they correspond to, > in order to make it clear. > > Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not > enabled. This corresponds to the following section within am65_cpsw_init_cpts(): > > if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) > return 0; > > In this case, common->cpts remains NULL, but it is not a problem even if the > am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is > NOP. Thus, this case is not an issue. > > Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present > in the device tree. This corresponds to the following section within > am65_cpsw_init_cpts(): > > node = of_get_child_by_name(dev->of_node, "cpts"); > if (!node) { > dev_err(dev, "%s cpts not found\n", __func__); > return -ENOENT; > } > > In this case as well, common->cpts remains NULL, but it is not a problem because > the probe fails and the execution jumps to "err_of_clear", which doesn't invoke > am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. > > Case-3 and Case-4 are described later in this mail. > >>> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. >>> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. >>> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >>> function with a return value of 0 when cpts is disabled. >> >> In this case common->cpts is not NULL and is set to error pointer. >> Probe will continue normally. >> Is it OK to call any of the cpts APIs with invalid handle? >> Also am65_cpts_release() will be called with invalid handle. > > Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had > meant that the following section is executed within the am65_cpsw_init_cpts() > function: > > Case-3: > > cpts = am65_cpts_create(dev, reg_base, node); > if (IS_ERR(cpts)) { > int ret = PTR_ERR(cpts); > > of_node_put(node); > if (ret == -EOPNOTSUPP) { > dev_info(dev, "cpts disabled\n"); > return 0; > } > > ...... > } > > Leon, > > In the above code, when the section corresponding to: > dev_info(dev, "cpts disabled\n"); > > is executed, CONFIG_TI_K3_AM65_CPTS is enabled. Therefore, the > am65_cpts_release() is not NOP. If the probe fails after the call to > am65_cpsw_init_cpts(), then the am65_cpsw_cpts_cleanup() function will be called > in the cleanup path of probe, which needs to check for common->cpts not being > NULL. This is because common->cpts is NULL after returning 0 from the > am65_cpsw_init_cpts() function at the > dev_info(dev, "cpts disabled\n"); > > section. Thus, I believe that in this case, am65_cpts_release() shouldn't be > invoked from the am65_cpsw_cpts_cleanup() function, since it would have already > been invoked from am65_cpts_create()'s cleanup path. This can be ensured by > checking whether common->cpts is NULL or not, before invoking > am65_cpts_release() within am65_cpsw_cpts_cleanup(). > Yes, I agree. >> >>> 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function >>> fails with an error. >> >> In this case common->cpts is not NULL and will invoke am65_cpts_release() with >> invalid handle. > > Case-4: The call to am65_cpts_create() within the am65_cpsw_init_cpts() function > fails with an error. This corresponds to the following section within > am65_cpsw_init_cpts(): > > cpts = am65_cpts_create(dev, reg_base, node); > if (IS_ERR(cpts)) { > ...... > dev_err(dev, "cpts create err %d\n", ret); > return ret; > } > > > Roger, > > If the call to am65_cpts_create() fails with an error other than -EOPNOTSUPP, > which corresponds to Case-4, the call to am65_cpts_release() would have been > invoked within the am65_cpts_create()'s cleanup path itself if necessary. Also, > when the error is not -EOPNOTSUPP, the am65_cpsw_init_cpts() function returns an > error, due to which the execution jumps to "err_of_clear" in > am65_cpsw_nuss_probe(). Therefore, am65_cpsw_cpts_cleanup() is not invoked in > this case, due to which common->cpts being NULL is not a problem. Correct. > > > Roger, Leon, please review my comments and let me know. I think that Case-3 > demands checking whether common->cpts is NULL or not, within the > am65_cpsw_cpts_cleanup() function. Do you really need a separate am65_cpsw_cpts_cleanup() or can just add the NULL check in am65_cpts_release()? cheers, -roger
On 17/01/23 14:57, Roger Quadros wrote: > Siddharth, > > On 17/01/2023 07:00, Siddharth Vadapalli wrote: >> Roger, Leon, >> >> On 16/01/23 21:31, Roger Quadros wrote: >>> Hi Siddharth, >>> >>> On 16/01/2023 09:43, Siddharth Vadapalli wrote: >>>> >>>> >>>> On 16/01/23 13:00, Leon Romanovsky wrote: >>>>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>>>> The am65_cpts_release() function is registered as a devm_action in the >>>>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>>>> actions associated with the am65-cpsw driver's device. >>>>>> >>>>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>>>> hardware is assumed to be powered on at this point. However, the hardware >>>>>> is powered off before the devm actions are executed. >>>>>> >>>>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>>>> invoking it directly on the cleanup and exit paths. >>>>>> >>>>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>> --- >>>>>> Changes from v1: >>>>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>>>> error was reported by kernel test robot <lkp@intel.com> at: >>>>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>>>> >>>>>> v1: >>>>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>>>> >>>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> index 5cac98284184..00f25d8a026b 100644 >>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>>>> +{ >>>>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>>>> >>>>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>>>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>>>> >>>>> How is it possible to have common->cpts == NULL? >>>> >>>> Thank you for reviewing the patch. I realize now that checking >>>> CONFIG_TI_K3_AM65_CPTS is unnecessary. >>>> >>>> common->cpts remains NULL in the following cases: >> >> I realized that the cases I mentioned are not explained clearly. Therefore, I >> will mention the cases again, along with the section of code they correspond to, >> in order to make it clear. >> >> Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not >> enabled. This corresponds to the following section within am65_cpsw_init_cpts(): >> >> if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) >> return 0; >> >> In this case, common->cpts remains NULL, but it is not a problem even if the >> am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is >> NOP. Thus, this case is not an issue. >> >> Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present >> in the device tree. This corresponds to the following section within >> am65_cpsw_init_cpts(): >> >> node = of_get_child_by_name(dev->of_node, "cpts"); >> if (!node) { >> dev_err(dev, "%s cpts not found\n", __func__); >> return -ENOENT; >> } >> >> In this case as well, common->cpts remains NULL, but it is not a problem because >> the probe fails and the execution jumps to "err_of_clear", which doesn't invoke >> am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. >> >> Case-3 and Case-4 are described later in this mail. >> >>>> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. >>>> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. >>>> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >>>> function with a return value of 0 when cpts is disabled. >>> >>> In this case common->cpts is not NULL and is set to error pointer. >>> Probe will continue normally. >>> Is it OK to call any of the cpts APIs with invalid handle? >>> Also am65_cpts_release() will be called with invalid handle. >> >> Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had >> meant that the following section is executed within the am65_cpsw_init_cpts() >> function: >> >> Case-3: >> >> cpts = am65_cpts_create(dev, reg_base, node); >> if (IS_ERR(cpts)) { >> int ret = PTR_ERR(cpts); >> >> of_node_put(node); >> if (ret == -EOPNOTSUPP) { >> dev_info(dev, "cpts disabled\n"); >> return 0; >> } >> >> ...... >> } >> >> Leon, >> >> In the above code, when the section corresponding to: >> dev_info(dev, "cpts disabled\n"); >> >> is executed, CONFIG_TI_K3_AM65_CPTS is enabled. Therefore, the >> am65_cpts_release() is not NOP. If the probe fails after the call to >> am65_cpsw_init_cpts(), then the am65_cpsw_cpts_cleanup() function will be called >> in the cleanup path of probe, which needs to check for common->cpts not being >> NULL. This is because common->cpts is NULL after returning 0 from the >> am65_cpsw_init_cpts() function at the >> dev_info(dev, "cpts disabled\n"); >> >> section. Thus, I believe that in this case, am65_cpts_release() shouldn't be >> invoked from the am65_cpsw_cpts_cleanup() function, since it would have already >> been invoked from am65_cpts_create()'s cleanup path. This can be ensured by >> checking whether common->cpts is NULL or not, before invoking >> am65_cpts_release() within am65_cpsw_cpts_cleanup(). >> > > Yes, I agree. > >>> >>>> 4. The call to am65_cpts_create() within the am65_cpsw_init_cpts() function >>>> fails with an error. >>> >>> In this case common->cpts is not NULL and will invoke am65_cpts_release() with >>> invalid handle. >> >> Case-4: The call to am65_cpts_create() within the am65_cpsw_init_cpts() function >> fails with an error. This corresponds to the following section within >> am65_cpsw_init_cpts(): >> >> cpts = am65_cpts_create(dev, reg_base, node); >> if (IS_ERR(cpts)) { >> ...... >> dev_err(dev, "cpts create err %d\n", ret); >> return ret; >> } >> >> >> Roger, >> >> If the call to am65_cpts_create() fails with an error other than -EOPNOTSUPP, >> which corresponds to Case-4, the call to am65_cpts_release() would have been >> invoked within the am65_cpts_create()'s cleanup path itself if necessary. Also, >> when the error is not -EOPNOTSUPP, the am65_cpsw_init_cpts() function returns an >> error, due to which the execution jumps to "err_of_clear" in >> am65_cpsw_nuss_probe(). Therefore, am65_cpsw_cpts_cleanup() is not invoked in >> this case, due to which common->cpts being NULL is not a problem. > > Correct. > >> >> >> Roger, Leon, please review my comments and let me know. I think that Case-3 >> demands checking whether common->cpts is NULL or not, within the >> am65_cpsw_cpts_cleanup() function. > > Do you really need a separate am65_cpsw_cpts_cleanup() or can just add > the NULL check in am65_cpts_release()? Adding a NULL check in am65_cpts_release() works too. I will implement this in the v3 patch, if there are no objections to this approach. Leon, please let me know if this is acceptable. Regards, Siddharth.
On Tue, Jan 17, 2023 at 10:30:26AM +0530, Siddharth Vadapalli wrote: > Roger, Leon, > > On 16/01/23 21:31, Roger Quadros wrote: > > Hi Siddharth, > > > > On 16/01/2023 09:43, Siddharth Vadapalli wrote: > >> > >> > >> On 16/01/23 13:00, Leon Romanovsky wrote: > >>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: > >>>> The am65_cpts_release() function is registered as a devm_action in the > >>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver > >>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm > >>>> actions associated with the am65-cpsw driver's device. > >>>> > >>>> In the event of probe failure or probe deferral, the platform_drv_probe() > >>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the > >>>> CPSW's CPTS hardware, both of which share the same power domain. Since the > >>>> am65_cpts_disable() function invoked by the am65_cpts_release() function > >>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS > >>>> hardware is assumed to be powered on at this point. However, the hardware > >>>> is powered off before the devm actions are executed. > >>>> > >>>> Fix this by getting rid of the devm action for am65_cpts_release() and > >>>> invoking it directly on the cleanup and exit paths. > >>>> > >>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") > >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> > >>>> --- > >>>> Changes from v1: > >>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This > >>>> error was reported by kernel test robot <lkp@intel.com> at: > >>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ > >>>> 2. Collect Reviewed-by tag from Roger Quadros. > >>>> > >>>> v1: > >>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ > >>>> > >>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ > >>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- > >>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ > >>>> 3 files changed, 18 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> index 5cac98284184..00f25d8a026b 100644 > >>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > >>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, > >>>> return 0; > >>>> } > >>>> > >>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) > >>>> +{ > >>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) > >>> > >>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if > >>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? > >>> > >>> How is it possible to have common->cpts == NULL? > >> > >> Thank you for reviewing the patch. I realize now that checking > >> CONFIG_TI_K3_AM65_CPTS is unnecessary. > >> > >> common->cpts remains NULL in the following cases: > > I realized that the cases I mentioned are not explained clearly. Therefore, I > will mention the cases again, along with the section of code they correspond to, > in order to make it clear. > > Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not > enabled. This corresponds to the following section within am65_cpsw_init_cpts(): > > if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) > return 0; > > In this case, common->cpts remains NULL, but it is not a problem even if the > am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is > NOP. Thus, this case is not an issue. > > Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present > in the device tree. This corresponds to the following section within > am65_cpsw_init_cpts(): > > node = of_get_child_by_name(dev->of_node, "cpts"); > if (!node) { > dev_err(dev, "%s cpts not found\n", __func__); > return -ENOENT; > } > > In this case as well, common->cpts remains NULL, but it is not a problem because > the probe fails and the execution jumps to "err_of_clear", which doesn't invoke > am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. > > Case-3 and Case-4 are described later in this mail. > > >> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. > >> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. > >> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() > >> function with a return value of 0 when cpts is disabled. > > > > In this case common->cpts is not NULL and is set to error pointer. > > Probe will continue normally. > > Is it OK to call any of the cpts APIs with invalid handle? > > Also am65_cpts_release() will be called with invalid handle. > > Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had > meant that the following section is executed within the am65_cpsw_init_cpts() > function: > > Case-3: > > cpts = am65_cpts_create(dev, reg_base, node); > if (IS_ERR(cpts)) { > int ret = PTR_ERR(cpts); > > of_node_put(node); > if (ret == -EOPNOTSUPP) { > dev_info(dev, "cpts disabled\n"); > return 0; > } This code block is unreachable, because of config earlier. 1916 static int am65_cpsw_init_cpts(struct am65_cpsw_common *common) 1917 { ... 1923 if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) 1924 return 0; ... 1933 cpts = am65_cpts_create(dev, reg_base, node); 1934 if (IS_ERR(cpts)) { 1935 int ret = PTR_ERR(cpts); 1936 1937 of_node_put(node); 1938 if (ret == -EOPNOTSUPP) { 1939 dev_info(dev, "cpts disabled\n"); 1940 return 0; 1941 } You should delete all the logic above. Thanks
On 17/01/23 17:04, Leon Romanovsky wrote: > On Tue, Jan 17, 2023 at 10:30:26AM +0530, Siddharth Vadapalli wrote: >> Roger, Leon, >> >> On 16/01/23 21:31, Roger Quadros wrote: >>> Hi Siddharth, >>> >>> On 16/01/2023 09:43, Siddharth Vadapalli wrote: >>>> >>>> >>>> On 16/01/23 13:00, Leon Romanovsky wrote: >>>>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>>>> The am65_cpts_release() function is registered as a devm_action in the >>>>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>>>> actions associated with the am65-cpsw driver's device. >>>>>> >>>>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>>>> hardware is assumed to be powered on at this point. However, the hardware >>>>>> is powered off before the devm actions are executed. >>>>>> >>>>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>>>> invoking it directly on the cleanup and exit paths. >>>>>> >>>>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>> --- >>>>>> Changes from v1: >>>>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>>>> error was reported by kernel test robot <lkp@intel.com> at: >>>>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>>>> >>>>>> v1: >>>>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>>>> >>>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> index 5cac98284184..00f25d8a026b 100644 >>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>>>> +{ >>>>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>>>> >>>>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>>>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>>>> >>>>> How is it possible to have common->cpts == NULL? >>>> >>>> Thank you for reviewing the patch. I realize now that checking >>>> CONFIG_TI_K3_AM65_CPTS is unnecessary. >>>> >>>> common->cpts remains NULL in the following cases: >> >> I realized that the cases I mentioned are not explained clearly. Therefore, I >> will mention the cases again, along with the section of code they correspond to, >> in order to make it clear. >> >> Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not >> enabled. This corresponds to the following section within am65_cpsw_init_cpts(): >> >> if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) >> return 0; >> >> In this case, common->cpts remains NULL, but it is not a problem even if the >> am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is >> NOP. Thus, this case is not an issue. >> >> Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present >> in the device tree. This corresponds to the following section within >> am65_cpsw_init_cpts(): >> >> node = of_get_child_by_name(dev->of_node, "cpts"); >> if (!node) { >> dev_err(dev, "%s cpts not found\n", __func__); >> return -ENOENT; >> } >> >> In this case as well, common->cpts remains NULL, but it is not a problem because >> the probe fails and the execution jumps to "err_of_clear", which doesn't invoke >> am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. >> >> Case-3 and Case-4 are described later in this mail. >> >>>> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. >>>> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. >>>> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >>>> function with a return value of 0 when cpts is disabled. >>> >>> In this case common->cpts is not NULL and is set to error pointer. >>> Probe will continue normally. >>> Is it OK to call any of the cpts APIs with invalid handle? >>> Also am65_cpts_release() will be called with invalid handle. >> >> Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had >> meant that the following section is executed within the am65_cpsw_init_cpts() >> function: >> >> Case-3: >> >> cpts = am65_cpts_create(dev, reg_base, node); >> if (IS_ERR(cpts)) { >> int ret = PTR_ERR(cpts); >> >> of_node_put(node); >> if (ret == -EOPNOTSUPP) { >> dev_info(dev, "cpts disabled\n"); >> return 0; >> } > > This code block is unreachable, because of config earlier. > 1916 static int am65_cpsw_init_cpts(struct am65_cpsw_common *common) > 1917 { > ... > 1923 if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) > 1924 return 0; > ... > 1933 cpts = am65_cpts_create(dev, reg_base, node); > 1934 if (IS_ERR(cpts)) { > 1935 int ret = PTR_ERR(cpts); > 1936 > 1937 of_node_put(node); > 1938 if (ret == -EOPNOTSUPP) { > 1939 dev_info(dev, "cpts disabled\n"); > 1940 return 0; > 1941 } > > You should delete all the logic above. Leon, I did not realize that the code block is unreachable. I had assumed it was valid and handled the case where the CONFIG_TI_K3_AM65_CPTS config is enabled and one of the functions within am65_cpts_create() return -EOPNOTSUPP, since this section of code was already present. I analyzed the possible return values of all the functions within am65_cpts_create() and like you pointed out, none of them seem to return -EOPNOTSUPP. Roger, Please let me know if you can identify a case where CONFIG_TI_K3_AM65_CPTS is enabled and one of the functions within the am65_cpts_create() function return -EOPNOTSUPP. I was unable to find one after analyzing the return values. Therefore, I shall proceed with adding a cleanup patch which deletes the unreachable code block, followed by updating this patch with Leon's first suggestion of dropping am65_cpsw_cpts_cleanup() entirely, since common->cpts being NULL won't have any problem and am65_cpts_release() can be invoked directly. I will post these two patches as the v3 series if there are no issues. Regards, Siddharth.
Hi, On 18/01/2023 06:58, Siddharth Vadapalli wrote: > On 17/01/23 17:04, Leon Romanovsky wrote: >> On Tue, Jan 17, 2023 at 10:30:26AM +0530, Siddharth Vadapalli wrote: >>> Roger, Leon, >>> >>> On 16/01/23 21:31, Roger Quadros wrote: >>>> Hi Siddharth, >>>> >>>> On 16/01/2023 09:43, Siddharth Vadapalli wrote: >>>>> >>>>> >>>>> On 16/01/23 13:00, Leon Romanovsky wrote: >>>>>> On Mon, Jan 16, 2023 at 10:15:17AM +0530, Siddharth Vadapalli wrote: >>>>>>> The am65_cpts_release() function is registered as a devm_action in the >>>>>>> am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver >>>>>>> invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm >>>>>>> actions associated with the am65-cpsw driver's device. >>>>>>> >>>>>>> In the event of probe failure or probe deferral, the platform_drv_probe() >>>>>>> function invokes dev_pm_domain_detach() which powers off the CPSW and the >>>>>>> CPSW's CPTS hardware, both of which share the same power domain. Since the >>>>>>> am65_cpts_disable() function invoked by the am65_cpts_release() function >>>>>>> attempts to reset the CPTS hardware by writing to its registers, the CPTS >>>>>>> hardware is assumed to be powered on at this point. However, the hardware >>>>>>> is powered off before the devm actions are executed. >>>>>>> >>>>>>> Fix this by getting rid of the devm action for am65_cpts_release() and >>>>>>> invoking it directly on the cleanup and exit paths. >>>>>>> >>>>>>> Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") >>>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>>> --- >>>>>>> Changes from v1: >>>>>>> 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This >>>>>>> error was reported by kernel test robot <lkp@intel.com> at: >>>>>>> https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ >>>>>>> 2. Collect Reviewed-by tag from Roger Quadros. >>>>>>> >>>>>>> v1: >>>>>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ >>>>>>> >>>>>>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ >>>>>>> drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- >>>>>>> drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ >>>>>>> 3 files changed, 18 insertions(+), 10 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>>> index 5cac98284184..00f25d8a026b 100644 >>>>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >>>>>>> @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) >>>>>>> +{ >>>>>>> + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) >>>>>> >>>>>> Why do you have IS_ENABLED(CONFIG_TI_K3_AM65_CPTS), if >>>>>> am65_cpts_release() defined as empty when CONFIG_TI_K3_AM65_CPTS not set? >>>>>> >>>>>> How is it possible to have common->cpts == NULL? >>>>> >>>>> Thank you for reviewing the patch. I realize now that checking >>>>> CONFIG_TI_K3_AM65_CPTS is unnecessary. >>>>> >>>>> common->cpts remains NULL in the following cases: >>> >>> I realized that the cases I mentioned are not explained clearly. Therefore, I >>> will mention the cases again, along with the section of code they correspond to, >>> in order to make it clear. >>> >>> Case-1: am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not >>> enabled. This corresponds to the following section within am65_cpsw_init_cpts(): >>> >>> if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) >>> return 0; >>> >>> In this case, common->cpts remains NULL, but it is not a problem even if the >>> am65_cpsw_nuss_probe() fails later, since the am65_cpts_release() function is >>> NOP. Thus, this case is not an issue. >>> >>> Case-2: am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not present >>> in the device tree. This corresponds to the following section within >>> am65_cpsw_init_cpts(): >>> >>> node = of_get_child_by_name(dev->of_node, "cpts"); >>> if (!node) { >>> dev_err(dev, "%s cpts not found\n", __func__); >>> return -ENOENT; >>> } >>> >>> In this case as well, common->cpts remains NULL, but it is not a problem because >>> the probe fails and the execution jumps to "err_of_clear", which doesn't invoke >>> am65_cpsw_cpts_cleanup(). Therefore, common->cpts being NULL is not a problem. >>> >>> Case-3 and Case-4 are described later in this mail. >>> >>>>> 1. am65_cpsw_init_cpts() returns 0 since CONFIG_TI_K3_AM65_CPTS is not enabled. >>>>> 2. am65_cpsw_init_cpts() returns -ENOENT since the cpts node is not defined. >>>>> 3. The call to am65_cpts_create() fails within the am65_cpsw_init_cpts() >>>>> function with a return value of 0 when cpts is disabled. >>>> >>>> In this case common->cpts is not NULL and is set to error pointer. >>>> Probe will continue normally. >>>> Is it OK to call any of the cpts APIs with invalid handle? >>>> Also am65_cpts_release() will be called with invalid handle. >>> >>> Yes Roger, thank you for pointing it out. When I wrote "cpts is disabled", I had >>> meant that the following section is executed within the am65_cpsw_init_cpts() >>> function: >>> >>> Case-3: >>> >>> cpts = am65_cpts_create(dev, reg_base, node); >>> if (IS_ERR(cpts)) { >>> int ret = PTR_ERR(cpts); >>> >>> of_node_put(node); >>> if (ret == -EOPNOTSUPP) { >>> dev_info(dev, "cpts disabled\n"); >>> return 0; >>> } >> >> This code block is unreachable, because of config earlier. >> 1916 static int am65_cpsw_init_cpts(struct am65_cpsw_common *common) >> 1917 { >> ... >> 1923 if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS)) >> 1924 return 0; >> ... >> 1933 cpts = am65_cpts_create(dev, reg_base, node); >> 1934 if (IS_ERR(cpts)) { >> 1935 int ret = PTR_ERR(cpts); >> 1936 >> 1937 of_node_put(node); >> 1938 if (ret == -EOPNOTSUPP) { >> 1939 dev_info(dev, "cpts disabled\n"); >> 1940 return 0; >> 1941 } >> >> You should delete all the logic above. > > Leon, > > I did not realize that the code block is unreachable. I had assumed it was valid > and handled the case where the CONFIG_TI_K3_AM65_CPTS config is enabled and one > of the functions within am65_cpts_create() return -EOPNOTSUPP, since this > section of code was already present. I analyzed the possible return values of > all the functions within am65_cpts_create() and like you pointed out, none of > them seem to return -EOPNOTSUPP. > > > Roger, > > Please let me know if you can identify a case where CONFIG_TI_K3_AM65_CPTS is > enabled and one of the functions within the am65_cpts_create() function return > -EOPNOTSUPP. I was unable to find one after analyzing the return values. I couldn't find either. > Therefore, I shall proceed with adding a cleanup patch which deletes the > unreachable code block, followed by updating this patch with Leon's first > suggestion of dropping am65_cpsw_cpts_cleanup() entirely, since common->cpts > being NULL won't have any problem and am65_cpts_release() can be invoked > directly. I will post these two patches as the v3 series if there are no issues. cheers, -roger
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index 5cac98284184..00f25d8a026b 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -1913,6 +1913,12 @@ static int am65_cpsw_am654_get_efuse_macid(struct device_node *of_node, return 0; } +static void am65_cpsw_cpts_cleanup(struct am65_cpsw_common *common) +{ + if (IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) && common->cpts) + am65_cpts_release(common->cpts); +} + static int am65_cpsw_init_cpts(struct am65_cpsw_common *common) { struct device *dev = common->dev; @@ -2917,6 +2923,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev) err_free_phylink: am65_cpsw_nuss_phylink_cleanup(common); + am65_cpsw_cpts_cleanup(common); err_of_clear: of_platform_device_destroy(common->mdio_dev, NULL); err_pm_clear: @@ -2945,6 +2952,7 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev) */ am65_cpsw_nuss_cleanup_ndev(common); am65_cpsw_nuss_phylink_cleanup(common); + am65_cpsw_cpts_cleanup(common); am65_cpsw_disable_serdes_phy(common); of_platform_device_destroy(common->mdio_dev, NULL); diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c index 9535396b28cd..a297890152d9 100644 --- a/drivers/net/ethernet/ti/am65-cpts.c +++ b/drivers/net/ethernet/ti/am65-cpts.c @@ -929,14 +929,13 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node) return cpts_of_mux_clk_setup(cpts, node); } -static void am65_cpts_release(void *data) +void am65_cpts_release(struct am65_cpts *cpts) { - struct am65_cpts *cpts = data; - ptp_clock_unregister(cpts->ptp_clock); am65_cpts_disable(cpts); clk_disable_unprepare(cpts->refclk); } +EXPORT_SYMBOL_GPL(am65_cpts_release); struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, struct device_node *node) @@ -1014,18 +1013,12 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, } cpts->phc_index = ptp_clock_index(cpts->ptp_clock); - ret = devm_add_action_or_reset(dev, am65_cpts_release, cpts); - if (ret) { - dev_err(dev, "failed to add ptpclk reset action %d", ret); - return ERR_PTR(ret); - } - ret = devm_request_threaded_irq(dev, cpts->irq, NULL, am65_cpts_interrupt, IRQF_ONESHOT, dev_name(dev), cpts); if (ret < 0) { dev_err(cpts->dev, "error attaching irq %d\n", ret); - return ERR_PTR(ret); + goto reset_ptpclk; } dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u\n", @@ -1034,6 +1027,8 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, return cpts; +reset_ptpclk: + am65_cpts_release(cpts); refclk_disable: clk_disable_unprepare(cpts->refclk); return ERR_PTR(ret); diff --git a/drivers/net/ethernet/ti/am65-cpts.h b/drivers/net/ethernet/ti/am65-cpts.h index bd08f4b2edd2..6e14df0be113 100644 --- a/drivers/net/ethernet/ti/am65-cpts.h +++ b/drivers/net/ethernet/ti/am65-cpts.h @@ -18,6 +18,7 @@ struct am65_cpts_estf_cfg { }; #if IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) +void am65_cpts_release(struct am65_cpts *cpts); struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, struct device_node *node); int am65_cpts_phc_index(struct am65_cpts *cpts); @@ -31,6 +32,10 @@ void am65_cpts_estf_disable(struct am65_cpts *cpts, int idx); void am65_cpts_suspend(struct am65_cpts *cpts); void am65_cpts_resume(struct am65_cpts *cpts); #else +static inline void am65_cpts_release(struct am65_cpts *cpts) +{ +} + static inline struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, struct device_node *node)
The am65_cpts_release() function is registered as a devm_action in the am65_cpts_create() function in am65-cpts driver. When the am65-cpsw driver invokes am65_cpts_create(), am65_cpts_release() is added in the set of devm actions associated with the am65-cpsw driver's device. In the event of probe failure or probe deferral, the platform_drv_probe() function invokes dev_pm_domain_detach() which powers off the CPSW and the CPSW's CPTS hardware, both of which share the same power domain. Since the am65_cpts_disable() function invoked by the am65_cpts_release() function attempts to reset the CPTS hardware by writing to its registers, the CPTS hardware is assumed to be powered on at this point. However, the hardware is powered off before the devm actions are executed. Fix this by getting rid of the devm action for am65_cpts_release() and invoking it directly on the cleanup and exit paths. Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> Reviewed-by: Roger Quadros <rogerq@kernel.org> --- Changes from v1: 1. Fix the build issue when "CONFIG_TI_K3_AM65_CPTS" is not set. This error was reported by kernel test robot <lkp@intel.com> at: https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@intel.com/ 2. Collect Reviewed-by tag from Roger Quadros. v1: https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@ti.com/ drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++ drivers/net/ethernet/ti/am65-cpts.c | 15 +++++---------- drivers/net/ethernet/ti/am65-cpts.h | 5 +++++ 3 files changed, 18 insertions(+), 10 deletions(-)