| Message ID | 1605086686-5140-1-git-send-email-wangqing@vivo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V4,net-bugfixs] net/ethernet: Update ret when ptp_clock is ERROR | expand |
On Wed, Nov 11, 2020 at 05:24:41PM +0800, Wang Qing wrote: > We always have to update the value of ret, otherwise the error value > may be the previous one. And ptp_clock_register() never return NULL > when PTP_1588_CLOCK enable. NAK. Your code must handle the possibility that ptp_clock_register() can return NULL. Why? 1. Because that follows the documented API. 2. Because people will copy/paste this driver. 3. Because the Kconfig for your driver can change without warning. Thanks, Richard
hi Jakub, On 11/11/2020 14:32, Richard Cochran wrote: > On Wed, Nov 11, 2020 at 05:24:41PM +0800, Wang Qing wrote: >> We always have to update the value of ret, otherwise the error value >> may be the previous one. And ptp_clock_register() never return NULL >> when PTP_1588_CLOCK enable. > > NAK. > > Your code must handle the possibility that ptp_clock_register() can > return NULL. Why? > > 1. Because that follows the documented API. > > 2. Because people will copy/paste this driver. > > 3. Because the Kconfig for your driver can change without warning. Following Richard's comments v1 of the patch has to be applied [1]. I've also added my Reviewed-by there. [1] https://lore.kernel.org/patchwork/patch/1334067/
On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: > > Following Richard's comments v1 of the patch has to be applied [1]. > I've also added my Reviewed-by there. > > [1] https://lore.kernel.org/patchwork/patch/1334067/ +1 Jakub, can you please take the original v1 of this patch? Thanks, Richard
On Wed, 11 Nov 2020 05:55:58 -0800 Richard Cochran wrote: > On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: > > > > Following Richard's comments v1 of the patch has to be applied [1]. > > I've also added my Reviewed-by there. > > > > [1] https://lore.kernel.org/patchwork/patch/1334067/ > > +1 > > Jakub, can you please take the original v1 of this patch? I don't think v1 builds cleanly folks (not 100% sure, cpts is not compiled on x86): ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV); ptp_clock is a pointer, ret is an integer, right? Grygorii, would you mind sending a correct patch in so Wang Qing can see how it's done? I've been asking for a fixes tag multiple times already :(
>> On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: >> > >> > Following Richard's comments v1 of the patch has to be applied [1]. >> > I've also added my Reviewed-by there. >> > >> > [1] https://lore.kernel.org/patchwork/patch/1334067/ >> >> +1 >> >> Jakub, can you please take the original v1 of this patch? > >I don't think v1 builds cleanly folks (not 100% sure, cpts is not >compiled on x86): > > ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV); > >ptp_clock is a pointer, ret is an integer, right? yeah, I will modify like: ret = cpts->ptp_clock ? PTR_ERR(cpts->ptp_clock) : -ENODEV; > >Grygorii, would you mind sending a correct patch in so Wang Qing can >see how it's done? I've been asking for a fixes tag multiple times >already :( I still don't quite understand what a fixes tag means， can you tell me how to do this, thanks. Wang Qing
On Thu, 12 Nov 2020 09:15:05 +0800 (GMT+08:00) 王擎 wrote: > >Grygorii, would you mind sending a correct patch in so Wang Qing can > >see how it's done? I've been asking for a fixes tag multiple times > >already :( > > I still don't quite understand what a fixes tag means， > can you tell me how to do this, thanks. Please read: Documentation/process/submitting-patches.rst You can search for "Fixes:"
On Thu, Nov 12, 2020 at 2:48 AM 王擎 <wangqing@vivo.com> wrote: > >> On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: > > > >I don't think v1 builds cleanly folks (not 100% sure, cpts is not > >compiled on x86): > > > > ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV); > > > >ptp_clock is a pointer, ret is an integer, right? > > yeah, I will modify like: ret = cpts->ptp_clock ? PTR_ERR(cpts->ptp_clock) : -ENODEV; This is not really getting any better. If Richard is worried about Kconfig getting changed here, I would suggest handling the case of PTP being disabled by returning an error early on in the function, like struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, struct device_node *node) { struct am65_cpts *cpts; int ret, i; if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK)) return -ENODEV; Then you can replace the broken IS_ERR_OR_NULL() path with a simpler IS_ERR() case and keep the rest of the function readable. > >Grygorii, would you mind sending a correct patch in so Wang Qing can > >see how it's done? I've been asking for a fixes tag multiple times > >already :( > > I still don't quite understand what a fixes tag means， > can you tell me how to do this, thanks. This identifies which patch introduced the problem you are fixing originally. If you add an alias in your ~/.gitconfig such as [alias] fixes = show --format='Fixes: %h (\"%s\")' -s then running $ git fixes f6bd59526c produces this line: Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common platform time sync driver") which you can add to the changelog, just above the Signed-off-by lines. Arnd
On 12/11/2020 10:25, Arnd Bergmann wrote: > On Thu, Nov 12, 2020 at 2:48 AM 王擎 <wangqing@vivo.com> wrote: >>>> On Wed, Nov 11, 2020 at 03:24:33PM +0200, Grygorii Strashko wrote: >>> >>> I don't think v1 builds cleanly folks (not 100% sure, cpts is not >>> compiled on x86): >>> >>> ret = cpts->ptp_clock ? cpts->ptp_clock : (-ENODEV); >>> >>> ptp_clock is a pointer, ret is an integer, right? >> >> yeah, I will modify like: ret = cpts->ptp_clock ? PTR_ERR(cpts->ptp_clock) : -ENODEV; > > This is not really getting any better. If Richard is worried about > Kconfig getting changed here, I would suggest handling the > case of PTP being disabled by returning an error early on in the > function, like > > struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, > struct device_node *node) > { > struct am65_cpts *cpts; > int ret, i; > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK)) > return -ENODEV; > > Then you can replace the broken IS_ERR_OR_NULL() path with > a simpler IS_ERR() case and keep the rest of the function readable. There is proper blocker in am65-cpts.h #if IS_ENABLED(CONFIG_TI_K3_AM65_CPTS) and in Makefile and proper dependency in Kconfig. config TI_K3_AM65_CPTS tristate "TI K3 AM65x CPTS" depends on ARCH_K3 && OF depends on PTP_1588_CLOCK But, as Richard mentioned [1], ptp_clock_register() may return NULL even if PTP_1588_CLOCK=y (which I can't confirm neither deny - from the fast look at ptp_clock_register() code it seems should not return NULL) > >>> Grygorii, would you mind sending a correct patch in so Wang Qing can >>> see how it's done? I've been asking for a fixes tag multiple times >>> already :( >> >> I still don't quite understand what a fixes tag means， >> can you tell me how to do this, thanks. > > This identifies which patch introduced the problem you are fixing > originally. If you add an alias in your ~/.gitconfig such as > > [alias] > fixes = show --format='Fixes: %h (\"%s\")' -s > > then running > > $ git fixes f6bd59526c > produces this line: > > Fixes: f6bd59526ca5 ("net: ethernet: ti: introduce am654 common > platform time sync driver") correct > > which you can add to the changelog, just above the Signed-off-by > lines. [1] https://lore.kernel.org/patchwork/patch/1334067/#1529232
On Thu, Nov 12, 2020 at 12:05:25PM +0200, Grygorii Strashko wrote: > But, as Richard mentioned [1], ptp_clock_register() may return NULL even if PTP_1588_CLOCK=y > (which I can't confirm neither deny - from the fast look at ptp_clock_register() > code it seems should not return NULL) This whole "implies" thing turned out to be a colossal PITA. I regret the fact that it got merged. It wasn't my idea. I will push back on playing games with the Kconfig settings. Even if that happens to work for your particular driver, still the call site of ptp_clock_register() must follow the correct pattern. Why? Because others will copy/paste it. Thanks, Richard
On Thu, Nov 12, 2020 at 09:25:12AM +0100, Arnd Bergmann wrote: > This is not really getting any better. If Richard is worried about > Kconfig getting changed here, I would suggest handling the > case of PTP being disabled by returning an error early on in the > function, like > > struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, > struct device_node *node) > { > struct am65_cpts *cpts; > int ret, i; > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK)) > return -ENODEV; No, please, no. That only adds confusion. The NULL return value already signals that the compile time support was missing. That was the entire point of this... * ptp_clock_register() - register a PTP hardware clock driver * * @info: Structure describing the new clock. * @parent: Pointer to the parent device of the new clock. * * Returns a valid pointer on success or PTR_ERR on failure. If PHC * support is missing at the configuration level, this function * returns NULL, and drivers are expected to gracefully handle that * case separately. Thanks, Richard
On Thu, Nov 12, 2020 at 7:19 PM Richard Cochran <richardcochran@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 09:25:12AM +0100, Arnd Bergmann wrote: > > This is not really getting any better. If Richard is worried about > > Kconfig getting changed here, I would suggest handling the > > case of PTP being disabled by returning an error early on in the > > function, like > > > > struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, > > struct device_node *node) > > { > > struct am65_cpts *cpts; > > int ret, i; > > > > if (!IS_ENABLED(CONFIG_PTP_1588_CLOCK)) > > return -ENODEV; > > No, please, no. That only adds confusion. The NULL return value > already signals that the compile time support was missing. That was > the entire point of this... > > * ptp_clock_register() - register a PTP hardware clock driver > * > * @info: Structure describing the new clock. > * @parent: Pointer to the parent device of the new clock. > * > * Returns a valid pointer on success or PTR_ERR on failure. If PHC > * support is missing at the configuration level, this function > * returns NULL, and drivers are expected to gracefully handle that > * case separately. The problem is that the caller doesn't handle that case gracefully, but it instead wants to return an error after all. I don't see a good solution either; as far as I'm concerned we should never be building code that fails if PTP_1588_CLOCK is disabled when it cannot do anything sensible in that configuration. I agree that the 'imply' keyword in Kconfig is what made this a lot worse, and it would be best to replace that with normal dependencies. It would be possible to have a ptp_clock_register_optional() interface in addition to ptp_clock_register(), which could then be changed to return an error. Some other subsystems follow this pattern, but it's not any less confusing either. Arnd
On Thu, Nov 12, 2020 at 10:21:21PM +0100, Arnd Bergmann wrote: > I agree that the 'imply' keyword in Kconfig is what made this a > lot worse, and it would be best to replace that with normal > dependencies. IIRC, this all started with tinification and wanting dynamic posix clocks to be optional at compile time. I would like to simplify this whole mess: - restore dynamic posix clocks to be always included - make PHC always included when the MAC has that feature (by saying "select" in the MAC Kconfig) -- I think this is what davem had wanted back in the day ... I'm not against tinification in principle, but I believe it is a lost cause. Thanks, Richard
On Fri, Nov 13, 2020 at 12:27 AM Richard Cochran <richardcochran@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 10:21:21PM +0100, Arnd Bergmann wrote: > > I agree that the 'imply' keyword in Kconfig is what made this a > > lot worse, and it would be best to replace that with normal > > dependencies. > > IIRC, this all started with tinification and wanting dynamic posix > clocks to be optional at compile time. > > I would like to simplify this whole mess: > > - restore dynamic posix clocks to be always included > > - make PHC always included when the MAC has that feature (by saying > "select" in the MAC Kconfig) -- I think this is what davem had > wanted back in the day ... > > I'm not against tinification in principle, but I believe it is a lost > cause. My preference would be to avoid both 'select' and 'imply' here, both of them cause their own set of problems. The main downside of 'select' is that you can't mix it with 'depends on' without risking running into circular dependencies and impossible configurations, while the main problem with 'imply' is that the behavior is close to unpredictable. The original definition still made some sense to me, but the new definition of 'imply' seems completely meaningless. I've prototyped a patch that I think makes this more sensible again: https://pastebin.com/AQ5nWS9e This needs testing, but if you think the approach makes sense, I can give it a few randconfig builds and submit for wider review. Arnd
On Fri, Nov 13, 2020 at 05:21:43PM +0100, Arnd Bergmann wrote: > I've prototyped a patch that I think makes this more sensible > again: https://pastebin.com/AQ5nWS9e I like the behavior described in the text. Instead of this ... - if a built-in driver calls PTP interface functions but fails to select HAVE_PTP_1588_CLOCK or depend on PTP_1588_CLOCK, and PTP support is a loadable module, we get a link error instead of having an unusable clock. how about simply deleting the #else clause of --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -173,7 +173,7 @@ struct ptp_clock_event { }; }; -#if IS_REACHABLE(CONFIG_PTP_1588_CLOCK) +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK) so that invalid configurations throw a compile time error instead? Thanks, Richard
On Sat, Nov 14, 2020 at 4:14 PM Richard Cochran <richardcochran@gmail.com> wrote: > > On Fri, Nov 13, 2020 at 05:21:43PM +0100, Arnd Bergmann wrote: > > I've prototyped a patch that I think makes this more sensible > > again: https://pastebin.com/AQ5nWS9e > > I like the behavior described in the text. > > Instead of this ... > > - if a built-in driver calls PTP interface functions but fails > to select HAVE_PTP_1588_CLOCK or depend on PTP_1588_CLOCK, > and PTP support is a loadable module, we get a link error > instead of having an unusable clock. > > how about simply deleting the #else clause of > > --- a/include/linux/ptp_clock_kernel.h > +++ b/include/linux/ptp_clock_kernel.h > @@ -173,7 +173,7 @@ struct ptp_clock_event { > }; > }; > > -#if IS_REACHABLE(CONFIG_PTP_1588_CLOCK) > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK) > > so that invalid configurations throw a compile time error instead? I was trying to still allow PTP clocks to be disabled, either when building a kernel that doesn't need it, or when posix timers are disabled. Leaving out the #else path would break all drivers that have PTP support in the main ethernet driver file rather than conditionally compiling it based on a Kconfig symbol that depends on CONFIG_PTP_1588_CLOCK. Arnd
diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c index 75056c1..b319d45 --- a/drivers/net/ethernet/ti/am65-cpts.c +++ b/drivers/net/ethernet/ti/am65-cpts.c @@ -1001,8 +1001,7 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs, if (IS_ERR_OR_NULL(cpts->ptp_clock)) { dev_err(dev, "Failed to register ptp clk %ld\n", PTR_ERR(cpts->ptp_clock)); - if (!cpts->ptp_clock) - ret = -ENODEV; + ret = PTR_ERR(cpts->ptp_clock); goto refclk_disable; } cpts->phc_index = ptp_clock_index(cpts->ptp_clock);
We always have to update the value of ret, otherwise the error value may be the previous one. And ptp_clock_register() never return NULL when PTP_1588_CLOCK enable. Signed-off-by: Wang Qing <wangqing@vivo.com> --- drivers/net/ethernet/ti/am65-cpts.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)