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 <[email protected]>
---
drivers/net/ethernet/ti/am65-cpts.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
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);
--
2.7.4
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/
--
Best regards,
grygorii
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:"
I see, but this bug is not caused by a specific patch, it exists at the beginning, so
there is no need to add a fixes tag. Please point out if I understand it incorrectly,thanks!
Wang Qing
On Thu, 12 Nov 2020 10:48:37 +0800 (GMT+08:00) 王擎 wrote:
> >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:"
>
> I see, but this bug is not caused by a specific patch, it exists at the beginning, so
> there is no need to add a fixes tag. Please point out if I understand it incorrectly,thanks!
Please put whatever constitutes the beginning here (first commit of the
driver or first commit of git history).
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 王擎 <[email protected]> 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 王擎 <[email protected]> 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
--
Best regards,
grygorii
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
<[email protected]> 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
<[email protected]> 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
<[email protected]> 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