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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ilb8ba1d.fsf@nvidia.com>
Date: Tue, 27 Jun 2023 19:22:38 -0700
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org,  Richard Cochran <richardcochran@...il.com>,
  Paolo Abeni <pabeni@...hat.com>,  Jakub Kicinski <kuba@...nel.org>,
  Saeed Mahameed <saeed@...nel.org>,  Gal Pressman <gal@...dia.com>,
  "David S. Miller" <davem@...emloft.net>,  lkft-triage@...ts.linaro.org,
  LTP List <ltp@...ts.linux.it>,  Nathan Chancellor <nathan@...nel.org>,
  Naresh Kamboju <naresh.kamboju@...aro.org>,  Linux Kernel Functional
 Testing <lkft@...aro.org>
Subject: Re: [PATCH net v1] ptp: Make max_phase_adjustment sysfs device
 attribute invisible when not supported

On Wed, 28 Jun, 2023 03:16:43 +0200 Andrew Lunn <andrew@...n.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@...dia.com>
>> Reported-by: Nathan Chancellor <nathan@...nel.org>
>> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
>> Reported-by: Linux Kernel Functional Testing <lkft@...aro.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