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

From: Rahul Rameshbabu <[email-masked]>
Date: Tue, 27 Jun 2023 19:22:38 -0700

On Wed, 28 Jun, 2023 03:16:43 +0200 Andrew Lunn <[email-masked]> 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 <[email-masked]>
>> Reported-by: Nathan Chancellor <[email-masked]>
>> Reported-by: Naresh Kamboju <[email-masked]>
>> Reported-by: Linux Kernel Functional Testing <[email-masked]>
>> Link: https://lore.kernel.org/netdev/[email-masked]/
>> Link: https://lore.kernel.org/all/[email-masked]/
>
> 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