Re: [PATCH] Cleanup: replace prefered with preferred

Re: [PATCH] Cleanup: replace prefered with preferred

[Laurent Pinchart]

Hi Mark,

Thank you for the patch.

On Tue, Oct 22, 2019 at 02:41:45PM -0700, Mark Salyzyn wrote:
> Replace all occurrences of prefered with preferred to make future
> checkpatch.pl's happy.  A few places the incorrect spelling is
> matched with the correct spelling to preserve existing user space API.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> ---
>  Documentation/networking/ip-sysctl.txt        |   2 +-
>  .../firmware/efi/libstub/efi-stub-helper.c    |   2 +-
>  .../gpu/drm/amd/display/dc/inc/compressor.h   |   4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |   2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.h           |   2 +-
>  drivers/media/usb/uvc/uvc_video.c             |   6 +-
>  fs/nfs/nfs4xdr.c                              |   2 +-
>  include/linux/ipv6.h                          |   2 +-
>  include/net/addrconf.h                        |   4 +-
>  include/net/if_inet6.h                        |   2 +-
>  include/net/ndisc.h                           |   8 +-
>  include/uapi/linux/if_addr.h                  |   5 +-
>  include/uapi/linux/ipv6.h                     |   4 +-
>  include/uapi/linux/sysctl.h                   |   4 +-
>  include/uapi/linux/usb/video.h                |   5 +-
>  kernel/sysctl_binary.c                        |   3 +-
>  net/6lowpan/ndisc.c                           |   4 +-
>  net/ipv4/devinet.c                            |  20 ++--
>  net/ipv6/addrconf.c                           | 113 ++++++++++--------
>  19 files changed, 112 insertions(+), 82 deletions(-)

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 8fa77a81dd7f..0096e6aacdb4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -276,13 +276,13 @@ static int uvc_get_video_ctrl(struct uvc_streaming *stream,
>  	if (size >= 34) {
>  		ctrl->dwClockFrequency = get_unaligned_le32(&data[26]);
>  		ctrl->bmFramingInfo = data[30];
> -		ctrl->bPreferedVersion = data[31];
> +		ctrl->bPreferredVersion = data[31];
>  		ctrl->bMinVersion = data[32];
>  		ctrl->bMaxVersion = data[33];
>  	} else {
>  		ctrl->dwClockFrequency = stream->dev->clock_frequency;
>  		ctrl->bmFramingInfo = 0;
> -		ctrl->bPreferedVersion = 0;
> +		ctrl->bPreferredVersion = 0;
>  		ctrl->bMinVersion = 0;
>  		ctrl->bMaxVersion = 0;
>  	}
> @@ -325,7 +325,7 @@ static int uvc_set_video_ctrl(struct uvc_streaming *stream,
>  	if (size >= 34) {
>  		put_unaligned_le32(ctrl->dwClockFrequency, &data[26]);
>  		data[30] = ctrl->bmFramingInfo;
> -		data[31] = ctrl->bPreferedVersion;
> +		data[31] = ctrl->bPreferredVersion;
>  		data[32] = ctrl->bMinVersion;
>  		data[33] = ctrl->bMaxVersion;
>  	}

[snip]

> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index d854cb19c42c..59167f0ed5c1 100644
> --- a/include/uapi/linux/usb/video.h
> +++ b/include/uapi/linux/usb/video.h
> @@ -448,7 +448,10 @@ struct uvc_streaming_control {
>  	__u32 dwMaxPayloadTransferSize;
>  	__u32 dwClockFrequency;
>  	__u8  bmFramingInfo;
> -	__u8  bPreferedVersion;
> +	union {
> +		__u8 bPreferredVersion;
> +		__u8 bPreferedVersion __attribute__((deprecated)); /* NOTYPO */
> +	} __attribute__((__packed__));

Quite interestingly this typo is part of the USB device class definition
for video devices (UVC) specification. I thus think we should keep using
the field name bPreferedVersion through the code, otherwise it wouldn't
match the spec.

>  	__u8  bMinVersion;
>  	__u8  bMaxVersion;
>  } __attribute__((__packed__));

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH] Cleanup: replace prefered with preferred

[Jarkko Sakkinen]

On Tue, Oct 22, 2019 at 02:41:45PM -0700, Mark Salyzyn wrote:
> Replace all occurrences of prefered with preferred to make future
> checkpatch.pl's happy.  A few places the incorrect spelling is
> matched with the correct spelling to preserve existing user space API.
> 
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>

I'd fix such things when the code is otherwise change and scope this
patch only to Documentation/. There is no pragmatic benefit of doing
this for the code.

/Jarkko

Re: [PATCH] Cleanup: replace prefered with preferred

[Mark Salyzyn]

On 10/23/19 4:56 AM, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 02:41:45PM -0700, Mark Salyzyn wrote:
>> Replace all occurrences of prefered with preferred to make future
>> checkpatch.pl's happy.  A few places the incorrect spelling is
>> matched with the correct spelling to preserve existing user space API.
>>
>> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> I'd fix such things when the code is otherwise change and scope this
> patch only to Documentation/. There is no pragmatic benefit of doing
> this for the code.
>
> /Jarkko

The pragmatic benefit comes with the use of an ABI/API checker (which is 
a 'distro' thing, not a top of tree kernel thing) produces its map which 
is typically required to be co-located in the same tree as the kernel 
repository. Quite a few ABI/API update checkins result in a 
checkpatch.pl complaint about the misspelled elements being 
(re-)recorded due to proximity. We have a separate task to improve how 
it is tracked in Android to reduce milepost marker changes that result 
in sweeping changes to the database which would reduce the occurrences.

I will split this between pure and inert documentation/comments for now, 
with a followup later for the code portion which understandably is more 
controversial.

Cleanup is the least appreciated part of kernel maintenance ;-}.

Sincerely -- Mark Salyzyn

Re: [PATCH] Cleanup: replace prefered with preferred

[Jani Nikula]

On Wed, 23 Oct 2019, Mark Salyzyn <salyzyn@android.com> wrote:
> I will split this between pure and inert documentation/comments for now, 
> with a followup later for the code portion which understandably is more 
> controversial.

Please split by driver/subsystem too, and it'll be all around much
easier for everyone.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] Cleanup: replace prefered with preferred

[Jarkko Sakkinen]

On Wed, Oct 23, 2019 at 08:40:59AM -0700, Mark Salyzyn wrote:
> On 10/23/19 4:56 AM, Jarkko Sakkinen wrote:
> > On Tue, Oct 22, 2019 at 02:41:45PM -0700, Mark Salyzyn wrote:
> > > Replace all occurrences of prefered with preferred to make future
> > > checkpatch.pl's happy.  A few places the incorrect spelling is
> > > matched with the correct spelling to preserve existing user space API.
> > > 
> > > Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> > I'd fix such things when the code is otherwise change and scope this
> > patch only to Documentation/. There is no pragmatic benefit of doing
> > this for the code.
> > 
> > /Jarkko
> 
> The pragmatic benefit comes with the use of an ABI/API checker (which is a
> 'distro' thing, not a top of tree kernel thing) produces its map which is
> typically required to be co-located in the same tree as the kernel
> repository. Quite a few ABI/API update checkins result in a checkpatch.pl
> complaint about the misspelled elements being (re-)recorded due to
> proximity. We have a separate task to improve how it is tracked in Android
> to reduce milepost marker changes that result in sweeping changes to the
> database which would reduce the occurrences.
> 
> I will split this between pure and inert documentation/comments for now,
> with a followup later for the code portion which understandably is more
> controversial.
> 
> Cleanup is the least appreciated part of kernel maintenance ;-}.
> 
> Sincerely -- Mark Salyzyn

I'm a strong believer of "evolutionary" approach. Patch sets for the
most part (everything in the end has to be considered case by case, not
a strict rule) should have some functional changes involved.

What I do require for the parts that I maintain is that any new change
will result cleaner code base than the one that existed before that
change was applied. Again, there are some exceptions to this e.g.
circulating a firmware bug but this is my driving guideline as a
maintainer.

Doing cleanups just for cleanups can sometimes add unnecessary merge
conflicts when backporting patches to stable kernels. Thus, if you are
doing just a cleanup you should have extremely good reasons to do so.

/Jarkko

Re: [PATCH] Cleanup: replace prefered with preferred

[Greg Kroah-Hartman]

On Wed, Oct 23, 2019 at 08:40:59AM -0700, Mark Salyzyn wrote:
> On 10/23/19 4:56 AM, Jarkko Sakkinen wrote:
> > On Tue, Oct 22, 2019 at 02:41:45PM -0700, Mark Salyzyn wrote:
> > > Replace all occurrences of prefered with preferred to make future
> > > checkpatch.pl's happy.  A few places the incorrect spelling is
> > > matched with the correct spelling to preserve existing user space API.
> > > 
> > > Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> > I'd fix such things when the code is otherwise change and scope this
> > patch only to Documentation/. There is no pragmatic benefit of doing
> > this for the code.
> > 
> > /Jarkko
> 
> The pragmatic benefit comes with the use of an ABI/API checker (which is a
> 'distro' thing, not a top of tree kernel thing) produces its map which is
> typically required to be co-located in the same tree as the kernel
> repository. Quite a few ABI/API update checkins result in a checkpatch.pl
> complaint about the misspelled elements being (re-)recorded due to
> proximity. We have a separate task to improve how it is tracked in Android
> to reduce milepost marker changes that result in sweeping changes to the
> database which would reduce the occurrences.

Requiring checkpatch spelling warnings to be correct based on function
names is crazy, you should fix your tools if you are requiring something
as looney as that :)

> I will split this between pure and inert documentation/comments for now,
> with a followup later for the code portion which understandably is more
> controversial.

Please break up per subsystem, like all trivial patches, as this
isn't anything special.

thanks,

greg k-h