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

[Siddharth Vadapalli]

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@xxxxxx>
>>>> Reviewed-by: Roger Quadros <rogerq@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxx> at:
>>>>    https://lore.kernel.org/r/202301142105.lt733Lt3-lkp@xxxxxxxxx/
>>>> 2. Collect Reviewed-by tag from Roger Quadros.
>>>>
>>>> v1:
>>>> https://lore.kernel.org/r/20230113104816.132815-1-s-vadapalli@xxxxxx/
>>>>
>>>>  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.