diff mbox series

[net-next,v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action

Message ID 20230116044517.310461-1-s-vadapalli@ti.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: grygorii.strashko@ti.com; 3 maintainers not CCed: jacob.e.keller@intel.com yangyingliang@huawei.com grygorii.strashko@ti.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 86 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Siddharth Vadapalli Jan. 16, 2023, 4:45 a.m. UTC
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(-)

Comments

Leon Romanovsky Jan. 16, 2023, 7:30 a.m. UTC | #1
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
Siddharth Vadapalli Jan. 16, 2023, 7:43 a.m. UTC | #2
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.
Leon Romanovsky Jan. 16, 2023, 10:04 a.m. UTC | #3
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
Siddharth Vadapalli Jan. 16, 2023, 10:37 a.m. UTC | #4
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.
Leon Romanovsky Jan. 16, 2023, 11:26 a.m. UTC | #5
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.
Roger Quadros Jan. 16, 2023, 4:01 p.m. UTC | #6
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
Siddharth Vadapalli Jan. 17, 2023, 5 a.m. UTC | #7
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.
Roger Quadros Jan. 17, 2023, 9:27 a.m. UTC | #8
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
Siddharth Vadapalli Jan. 17, 2023, 9:48 a.m. UTC | #9
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.
Leon Romanovsky Jan. 17, 2023, 11:34 a.m. UTC | #10
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
Siddharth Vadapalli Jan. 18, 2023, 4:58 a.m. UTC | #11
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.
Roger Quadros Jan. 18, 2023, 7:25 a.m. UTC | #12
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 mbox series

Patch

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)