From: Roger Quadros <rogerq@kernel.org> To: Siddharth Vadapalli <s-vadapalli@ti.com>, 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 Subject: Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action Date: Wed, 18 Jan 2023 09:25:39 +0200 [thread overview] Message-ID: <36906043-4c18-bfff-c4e4-5d16b3445906@kernel.org> (raw) In-Reply-To: <7323af1e-1f33-adcf-885e-db604f7a3788@ti.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Roger Quadros <rogerq@kernel.org> To: Siddharth Vadapalli <s-vadapalli@ti.com>, 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 Subject: Re: [PATCH net-next v2] net: ethernet: ti: am65-cpsw/cpts: Fix CPTS release action Date: Wed, 18 Jan 2023 09:25:39 +0200 [thread overview] Message-ID: <36906043-4c18-bfff-c4e4-5d16b3445906@kernel.org> (raw) In-Reply-To: <7323af1e-1f33-adcf-885e-db604f7a3788@ti.com> 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
next prev parent reply other threads:[~2023-01-18 7:26 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 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 [this message] 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=36906043-4c18-bfff-c4e4-5d16b3445906@kernel.org \ --to=rogerq@kernel.org \ --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=s-vadapalli@ti.com \ --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.