From: Siddharth Vadapalli <s-vadapalli@ti.com> To: Roger Quadros <rogerq@kernel.org>, Leon Romanovsky <leon@kernel.org> Cc: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <linux@armlinux.org.uk>, <pabeni@redhat.com>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <vigneshr@ti.com>, <srk@ti.com>, <s-vadapalli@ti.com> Subject: Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action Date: Tue, 17 Jan 2023 10:30:26 +0530 [thread overview] Message-ID: <aebaa171-bf4e-c143-a186-a37cd34b724e@ti.com> (raw) In-Reply-To: <b33c25c5-c93f-6860-b0a5-58279022a91c@kernel.org> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Siddharth Vadapalli <s-vadapalli@ti.com> To: Roger Quadros <rogerq@kernel.org>, Leon Romanovsky <leon@kernel.org> Cc: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <linux@armlinux.org.uk>, <pabeni@redhat.com>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <vigneshr@ti.com>, <srk@ti.com>, <s-vadapalli@ti.com> Subject: Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action Date: Tue, 17 Jan 2023 10:30:26 +0530 [thread overview] Message-ID: <aebaa171-bf4e-c143-a186-a37cd34b724e@ti.com> (raw) In-Reply-To: <b33c25c5-c93f-6860-b0a5-58279022a91c@kernel.org> 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-17 5:01 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-16 4:45 [PATCH net-next v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action Siddharth Vadapalli 2023-01-16 4:45 ` Siddharth Vadapalli 2023-01-16 7:30 ` Leon Romanovsky 2023-01-16 7:30 ` Leon Romanovsky 2023-01-16 7:43 ` Siddharth Vadapalli 2023-01-16 7:43 ` Siddharth Vadapalli 2023-01-16 10:04 ` Leon Romanovsky 2023-01-16 10:04 ` Leon Romanovsky 2023-01-16 10:37 ` Siddharth Vadapalli 2023-01-16 10:37 ` Siddharth Vadapalli 2023-01-16 11:26 ` Leon Romanovsky 2023-01-16 11:26 ` Leon Romanovsky 2023-01-16 16:01 ` Roger Quadros 2023-01-16 16:01 ` Roger Quadros 2023-01-17 5:00 ` Siddharth Vadapalli [this message] 2023-01-17 5:00 ` Siddharth Vadapalli 2023-01-17 9:27 ` Roger Quadros 2023-01-17 9:27 ` Roger Quadros 2023-01-17 9:48 ` Siddharth Vadapalli 2023-01-17 9:48 ` Siddharth Vadapalli 2023-01-17 11:34 ` Leon Romanovsky 2023-01-17 11:34 ` Leon Romanovsky 2023-01-18 4:58 ` Siddharth Vadapalli 2023-01-18 4:58 ` Siddharth Vadapalli 2023-01-18 7:25 ` Roger Quadros 2023-01-18 7:25 ` Roger Quadros
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aebaa171-bf4e-c143-a186-a37cd34b724e@ti.com \ --to=s-vadapalli@ti.com \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=kuba@kernel.org \ --cc=leon@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=rogerq@kernel.org \ --cc=srk@ti.com \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.