[PATCH net v1] ptp: Make max_phase_adjustment sysfs device attribute invisible when not supported
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+imZrfj6i... --- 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;
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+imZrfj6i...
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
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+imZrfj6i...
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
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+imZrfj6i...
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=c3...
Andrew
Thanks,
Rahul Rameshbabu
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.
- We can take this patch as is, but I submit a subsequent cleanup patch for
this.
Fine by me.
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?
Humm. So c3b60ab7a4df is in net-next, and the tag net-next-6.5. So it was passed to Linus yesterday for merging. You would like this fix merged either before -rc1, or just after -rc1.
We are in the grey area where it is less clear which tree it should be against. So it is good to explain after the --- what your intention is, so the Maintainers get a heads up and understand what you are trying to achieve.
I actually thought you were trying to fix an older issue, something in 6.4 or older, which is what net is mostly used for. Under those conditions, the stable rules apply. Things like, is this fixing an issue which really effects users....
Andrew
On Wed, 28 Jun, 2023 16:35:41 +0200 Andrew Lunn andrew@lunn.ch wrote:
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?
Humm. So c3b60ab7a4df is in net-next, and the tag net-next-6.5. So it was passed to Linus yesterday for merging. You would like this fix merged either before -rc1, or just after -rc1.
It can be after -rc1. I understand your point now from this elaboration. Since the change is not heading towards a final release yet but a release candidate, it's not an "urgent" patch to be applied before -rc1.
We are in the grey area where it is less clear which tree it should be against. So it is good to explain after the --- what your intention is, so the Maintainers get a heads up and understand what you are trying to achieve.
Agreed, I could have used git notes for that in my patch generation. Noted for the future. Just to be clear, my intention is for this fix to make its way before the final 6.5 release (before the changes make their way to an end user since the NULL pointer dereference when reading that sysfs node from a PHC not supporting phase adjustment is problematic). I think the issue being present in a release candidate is a minor problem. Would I still keep the Fixes tag however if targeting net-next? I remember this email from Jakub where if a Fixes tag is used, the patch should end up in net rather than net-next. However, I fear that without a Fixes tag, targeting net-next would cause me to miss applying this fix before the 6.5 release.
Link: https://lore.kernel.org/netdev/20230607091909.321fc5d7@kernel.org/
----- BEGIN QUOTE ----- We'll obviously apply our own judgment but submitter should send all fixes against net. ------ END QUOTE ------
I personally think this fix is "worthy" of targeting net based on the discussion Jakub previously provided, given the impact of reading the sysfs node on a system without this patch on PHCs that do not support phase adjustment.
I actually thought you were trying to fix an older issue, something in 6.4 or older, which is what net is mostly used for. Under those conditions, the stable rules apply. Things like, is this fixing an issue which really effects users....
Right, this was caught before it made its way to general users. I was basing targeting net still from the previous conversation about Fixes tags.
Andrew
Thanks,
Rahul Rameshbabu
It can be after -rc1. I understand your point now from this elaboration. Since the change is not heading towards a final release yet but a release candidate, it's not an "urgent" patch to be applied before -rc1.
We are in the grey area where it is less clear which tree it should be against. So it is good to explain after the --- what your intention is, so the Maintainers get a heads up and understand what you are trying to achieve.
Agreed, I could have used git notes for that in my patch generation. Noted for the future. Just to be clear, my intention is for this fix to make its way before the final 6.5 release (before the changes make their way to an end user since the NULL pointer dereference when reading that sysfs node from a PHC not supporting phase adjustment is problematic).
A NULL pointer dereference is a valid reason for stable anyway.
I think the issue being present in a release candidate is a minor problem. Would I still keep the Fixes tag however if targeting net-next?
It is useful, even for something in net-next. It is not needed for back porting, except for making it clear back porting is not needed, But there are some statistics gathered based on Fixes tags, etc.
Andrew
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.)
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
On Wed, 28 Jun, 2023 22:46:32 +0200 Andrew Lunn andrew@lunn.ch wrote:
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.
Sorry, the only switch cases I see in the link you shared are for an integral type enum and a u32. I do not see a pointer type being used in a switch in sfp_hwmon_is_visible?
I believe Jakub is right about pointer types + switches in C. pointer types are not considered integral types. Here is a compiler explorer example to demonstrate. This slipped my mind during our discussion.
https://godbolt.org/z/nKr3x7cT8
<source>: In function 'main': <source>:6:13: error: switch quantity not an integer 6 | switch (data) { | ^~~~ Compiler returned: 1
-- Rahul Rameshbabu
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?
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
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+imZrfj6i...
Acked-by: Richard Cochran richardcochran@gmail.com
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
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+imZrfj6i...
[...]
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!
-
Andrew Lunn -
Cyril Hrubis -
Jakub Kicinski -
Nathan Chancellor -
patchwork-bot+netdevbpf＠kernel.org -
Petr Vorel -
Rahul Rameshbabu -
Richard Cochran