[PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Rahul Rameshbabu]

The .adjphase operation is an operation that is implemented only by certain
PHCs. The sysfs device attribute node for querying the maximum phase
adjustment supported should not be exposed on devices that do not support
.adjphase.

Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/
---
 drivers/ptp/ptp_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 77219cdcd683..6e4d5456a885 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -358,6 +358,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj,
 		   attr == &dev_attr_max_vclocks.attr) {
 		if (ptp->is_virtual_clock)
 			mode = 0;
+	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
+		if (!info->adjphase || !info->getmaxphase)
+			mode = 0;
 	}
 
 	return mode;
-- 
2.40.1

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Nathan Chancellor]

On Tue, Jun 27, 2023 at 04:21:39PM -0700, Rahul Rameshbabu wrote:
> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.
> 
> Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
> Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/

Thanks a lot for the fix!

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/ptp/ptp_sysfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index 77219cdcd683..6e4d5456a885 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -358,6 +358,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj,
>  		   attr == &dev_attr_max_vclocks.attr) {
>  		if (ptp->is_virtual_clock)
>  			mode = 0;
> +	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
> +		if (!info->adjphase || !info->getmaxphase)
> +			mode = 0;
>  	}
>  
>  	return mode;
> -- 
> 2.40.1
> 

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Andrew Lunn]

On Tue, Jun 27, 2023 at 04:21:39PM -0700, Rahul Rameshbabu wrote:
> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.
> 
> Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
> Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/

I think Signed-off-by should be last.

> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index 77219cdcd683..6e4d5456a885 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -358,6 +358,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj,
>  		   attr == &dev_attr_max_vclocks.attr) {
>  		if (ptp->is_virtual_clock)
>  			mode = 0;
> +	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
> +		if (!info->adjphase || !info->getmaxphase)
> +			mode = 0;

Maybe it is time to turn this into a switch statement?

I also wounder if this really is something for net. How do you think
this patch matches against the stable rules?

    Andrew

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Rahul Rameshbabu]

On Wed, 28 Jun, 2023 03:16:43 +0200 Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Jun 27, 2023 at 04:21:39PM -0700, Rahul Rameshbabu wrote:
>> The .adjphase operation is an operation that is implemented only by certain
>> PHCs. The sysfs device attribute node for querying the maximum phase
>> adjustment supported should not be exposed on devices that do not support
>> .adjphase.
>> 
>> Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
>> Reported-by: Nathan Chancellor <nathan@kernel.org>
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
>> Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
>> Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/
>
> I think Signed-off-by should be last.

I can agree with this. Follows the "real route" of the patch, reports
came in and then the patch was implemented and signed-off.

>
>> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
>> index 77219cdcd683..6e4d5456a885 100644
>> --- a/drivers/ptp/ptp_sysfs.c
>> +++ b/drivers/ptp/ptp_sysfs.c
>> @@ -358,6 +358,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj,
>>  		   attr == &dev_attr_max_vclocks.attr) {
>>  		if (ptp->is_virtual_clock)
>>  			mode = 0;
>> +	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
>> +		if (!info->adjphase || !info->getmaxphase)
>> +			mode = 0;
>
> Maybe it is time to turn this into a switch statement?

I agree. However, I do not want to conflate two separate things being a
bugfix and a cleanup. I think we can do one of two options.

  1. We can take this patch as is, but I submit a subsequent cleanup patch for
  this.
  2. I can turn this into a two patch submission where the first patch
  does the conversion.

I am a fan of the first option personally.

>
> I also wounder if this really is something for net. How do you think
> this patch matches against the stable rules?

Apologize in advance but not sure I am following along. The commit for
the patch the introduces the problematic logic has made its way to net
and this patch is a fix. Therefore, isn't net the right tree to target?

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c3b60ab7a4dff6e6e608e685b70ddc3d6b2aca81

>
>     Andrew

Thanks,

Rahul Rameshbabu

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Jakub Kicinski]

On Wed, 28 Jun 2023 03:16:43 +0200 Andrew Lunn wrote:
> > +	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
> > +		if (!info->adjphase || !info->getmaxphase)
> > +			mode = 0;  
> 
> Maybe it is time to turn this into a switch statement?

I don't think we can switch on pointers in C.
The patch is good as is, right?
(The tree we'll pick appropriately when applying.)

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Andrew Lunn]

On Wed, Jun 28, 2023 at 01:38:50PM -0700, Jakub Kicinski wrote:
> On Wed, 28 Jun 2023 03:16:43 +0200 Andrew Lunn wrote:
> > > +	} else if (attr == &dev_attr_max_phase_adjustment.attr) {
> > > +		if (!info->adjphase || !info->getmaxphase)
> > > +			mode = 0;  
> > 
> > Maybe it is time to turn this into a switch statement?
> 
> I don't think we can switch on pointers in C.

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/sfp.c#L749

Works for temperature sensors, voltage sensors, current sensors, and
power sensors. Maybe hwmon is different to what is going on here, but
both a sysfs files.

> The patch is good as is, right?

Yes.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Jakub Kicinski]

On Tue, 27 Jun 2023 16:21:39 -0700 Rahul Rameshbabu wrote:
> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.

Richard, ack?

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Richard Cochran]

On Thu, Jun 29, 2023 at 11:06:48AM -0700, Jakub Kicinski wrote:
> On Tue, 27 Jun 2023 16:21:39 -0700 Rahul Rameshbabu wrote:
> > The .adjphase operation is an operation that is implemented only by certain
> > PHCs. The sysfs device attribute node for querying the maximum phase
> > adjustment supported should not be exposed on devices that do not support
> > .adjphase.
> 
> Richard, ack?

yes

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Richard Cochran]

On Tue, Jun 27, 2023 at 04:21:39PM -0700, Rahul Rameshbabu wrote:
> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.
> 
> Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
> Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [LTP] [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Petr Vorel]

Hi all,

> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

I wonder why LTP was Cc'ed.

Kind regards,
Petr

Re: [LTP] [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[Cyril Hrubis]

Hi!
> I wonder why LTP was Cc'ed.

Since the NULL dereference was found by the read_all LTP test.

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 27 Jun 2023 16:21:39 -0700 you wrote:
> The .adjphase operation is an operation that is implemented only by certain
> PHCs. The sysfs device attribute node for querying the maximum phase
> adjustment supported should not be exposed on devices that do not support
> .adjphase.
> 
> Fixes: c3b60ab7a4df ("ptp: Add .getmaxphase callback to ptp_clock_info")
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Link: https://lore.kernel.org/netdev/20230627162146.GA114473@dev-arch.thelio-3990X/
> Link: https://lore.kernel.org/all/CA+G9fYtKCZeAUTtwe69iK8Xcz1mOKQzwcy49wd+imZrfj6ifXA@mail.gmail.com/
> 
> [...]

Here is the summary with links:
  - [net,v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported
    https://git.kernel.org/netdev/net/c/2c5d234d7f55

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html