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

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

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