[PATCH 1/2] cifs: remove unused status severity defines

[PATCH 1/2] cifs: remove unused status severity defines

[Sergey Senozhatsky]

STATUS_SEVERITY_* do not appear to be used by anyone, so
drop them. Besides, the name of __constant_cpu_to_le32()
is misspelled there: __constanst_cpu_to_le32().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 fs/cifs/smb2status.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h
index 3d5f62150de4..e9da9c6da277 100644
--- a/fs/cifs/smb2status.h
+++ b/fs/cifs/smb2status.h
@@ -29,11 +29,6 @@
  *  C is set if "customer defined" error, N bit is reserved and MBZ
  */
 
-#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)
-#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
-#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
-#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
-
 struct ntstatus {
 	/* Facility is the high 12 bits of the following field */
 	__le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */
-- 
2.21.0

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Steve French]

Since this file (smb2status.h) is intended to track the official
protocol documentation (albeit smb2status.h probably needs to be
updated), in this case the protocol document MS-ERREF.  I would prefer
to keep it closer to MS-ERREF and leave definitions in even if unused
(if nothing else it helps some of us when debugging to recognize what
these errors on the wire mean).  There is a real danger that we have
run into in the past that in removing some protocol definitions
(flags, etc.) from the code or forgetting to update our headers to
match newer versions of the protocol specifications, that  with future
code changes we can forget to handle flags (for example) or misparse
responses due to not realizing that there are additional flags that
need to be parsed.

On Thu, Mar 14, 2019 at 1:17 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> STATUS_SEVERITY_* do not appear to be used by anyone, so
> drop them. Besides, the name of __constant_cpu_to_le32()
> is misspelled there: __constanst_cpu_to_le32().
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  fs/cifs/smb2status.h | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h
> index 3d5f62150de4..e9da9c6da277 100644
> --- a/fs/cifs/smb2status.h
> +++ b/fs/cifs/smb2status.h
> @@ -29,11 +29,6 @@
>   *  C is set if "customer defined" error, N bit is reserved and MBZ
>   */
>
> -#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)
> -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
> -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
> -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
> -
>  struct ntstatus {
>         /* Facility is the high 12 bits of the following field */
>         __le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */
> --
> 2.21.0
>


-- 
Thanks,

Steve

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

updated it to fix the spelling mistake


On Thu, Mar 14, 2019 at 1:54 AM Steve French <smfrench@gmail.com> wrote:
>
> Since this file (smb2status.h) is intended to track the official
> protocol documentation (albeit smb2status.h probably needs to be
> updated), in this case the protocol document MS-ERREF.  I would prefer
> to keep it closer to MS-ERREF and leave definitions in even if unused
> (if nothing else it helps some of us when debugging to recognize what
> these errors on the wire mean).  There is a real danger that we have
> run into in the past that in removing some protocol definitions
> (flags, etc.) from the code or forgetting to update our headers to
> match newer versions of the protocol specifications, that  with future
> code changes we can forget to handle flags (for example) or misparse
> responses due to not realizing that there are additional flags that
> need to be parsed.
>
> On Thu, Mar 14, 2019 at 1:17 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > STATUS_SEVERITY_* do not appear to be used by anyone, so
> > drop them. Besides, the name of __constant_cpu_to_le32()
> > is misspelled there: __constanst_cpu_to_le32().
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > ---
> >  fs/cifs/smb2status.h | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h
> > index 3d5f62150de4..e9da9c6da277 100644
> > --- a/fs/cifs/smb2status.h
> > +++ b/fs/cifs/smb2status.h
> > @@ -29,11 +29,6 @@
> >   *  C is set if "customer defined" error, N bit is reserved and MBZ
> >   */
> >
> > -#define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)
> > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
> > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
> > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
> > -
> >  struct ntstatus {
> >         /* Facility is the high 12 bits of the following field */
> >         __le32 Facility; /* low 2 bits Severity, next is Customer, then rsrvd */
> > --
> > 2.21.0
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb2-fix-typo-in-definition-of-a-few-error-flags.patch --]
[-- Type: text/x-patch, Size: 1275 bytes --]

From c1d061d2d47d536d7674a162937b51d8dd7ac7dc Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 14 Mar 2019 01:56:34 -0500
Subject: [PATCH] smb2: fix typo in definition of a few error flags

As Sergey Senozhatsky pointed out __constant_cpu_to_le32()
is misspelled in a few definitions in the list of status
codes smb2status.h as __constanst_cpu_to_le32()

Signed-off-by: Steve French <stfrench@microsoft.com>
CC: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 fs/cifs/smb2status.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/smb2status.h b/fs/cifs/smb2status.h
index 3d5f62150de4..f5429c8bfb0d 100644
--- a/fs/cifs/smb2status.h
+++ b/fs/cifs/smb2status.h
@@ -30,9 +30,9 @@
  */
 
 #define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)
-#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
-#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
-#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
+#define STATUS_SEVERITY_INFORMATIONAL cpu_to_le32(0x0001)
+#define STATUS_SEVERITY_WARNING cpu_to_le32(0x0002)
+#define STATUS_SEVERITY_ERROR cpu_to_le32(0x0003)
 
 struct ntstatus {
 	/* Facility is the high 12 bits of the following field */
-- 
2.17.1

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Sergey Senozhatsky]

On (03/14/19 01:54), Steve French wrote:
> Since this file (smb2status.h) is intended to track the official
> protocol documentation (albeit smb2status.h probably needs to be
> updated), in this case the protocol document MS-ERREF.  I would prefer
> to keep it closer to MS-ERREF and leave definitions in even if unused
> (if nothing else it helps some of us when debugging to recognize what
> these errors on the wire mean).  There is a real danger that we have
> run into in the past that in removing some protocol definitions
> (flags, etc.) from the code or forgetting to update our headers to
> match newer versions of the protocol specifications, that  with future
> code changes we can forget to handle flags (for example) or misparse
> responses due to not realizing that there are additional flags that
> need to be parsed.

OK, works for me.

	-ss

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Sergey Senozhatsky]

On (03/14/19 02:04), Steve French wrote:
[..]
>  #define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)

Does STATUS_SEVERITY_SUCCESS still use __constant_cpu_to_le32?

> -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
> -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
> -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
> +#define STATUS_SEVERITY_INFORMATIONAL cpu_to_le32(0x0001)
> +#define STATUS_SEVERITY_WARNING cpu_to_le32(0x0002)
> +#define STATUS_SEVERITY_ERROR cpu_to_le32(0x0003)

Otherwise looks good.

	-ss

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Steve French]

All of those uses of __constant_cpu_to_le32 apparently (at least
according to checkpatch) should be changed (someday) to cpu_to_le32
but I didn't research why the change from __constant_cpu_to_le32
--->   cpu_to_le32

If it has benefit - and checkpatch is right (it warned about
__constant_cpu_to_le32 being no longer preferred) ... perhaps would be
worth a followup patch to clean the rest of them up?   If you have any
context on why kernel code has moved away from using the older format
of __constant_cpu_to_.... would be useful to know if any benefit to
the change

On Thu, Mar 14, 2019 at 2:12 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (03/14/19 02:04), Steve French wrote:
> [..]
> >  #define STATUS_SEVERITY_SUCCESS __constant_cpu_to_le32(0x0000)
>
> Does STATUS_SEVERITY_SUCCESS still use __constant_cpu_to_le32?
>
> > -#define STATUS_SEVERITY_INFORMATIONAL __constanst_cpu_to_le32(0x0001)
> > -#define STATUS_SEVERITY_WARNING __constanst_cpu_to_le32(0x0002)
> > -#define STATUS_SEVERITY_ERROR __constanst_cpu_to_le32(0x0003)
> > +#define STATUS_SEVERITY_INFORMATIONAL cpu_to_le32(0x0001)
> > +#define STATUS_SEVERITY_WARNING cpu_to_le32(0x0002)
> > +#define STATUS_SEVERITY_ERROR cpu_to_le32(0x0003)
>
> Otherwise looks good.
>
>         -ss



-- 
Thanks,

Steve

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Sergey Senozhatsky]

On (03/14/19 02:19), Steve French wrote:
> All of those uses of __constant_cpu_to_le32 apparently (at least
> according to checkpatch) should be changed (someday) to cpu_to_le32
> but I didn't research why the change from __constant_cpu_to_le32
> --->   cpu_to_le32

Probably historic reasons.

Looking at linux 2.4.21

/*
 * Allow constant folding
 */
#if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__)
#  define __swahw32(x) \
(__builtin_constant_p((__u32)(x)) ? \
 ___swahw32((x)) : \
 __fswahw32((x)))


My assumption would be that __GNUC__ < 2 did no support
__builtin_constant_p?


> If it has benefit - and checkpatch is right (it warned about
> __constant_cpu_to_le32 being no longer preferred) ... perhaps would be
> worth a followup patch to clean the rest of them up?   If you have any
> context on why kernel code has moved away from using the older format
> of __constant_cpu_to_.... would be useful to know if any benefit to
> the change

Right, that's what I'm going to do - send out patches and update the rest
of __constant_cpu_to_XX users; so, eventually, __constant_cpu_to_XX
can be removed.

	-ss

Re: [PATCH 1/2] cifs: remove unused status severity defines

[Steve French]

I am fine with taking a patch to get rid of __constant_cpu_to_XXX
(and converting to the same cpu_to_XXX with the "__constant")  in
fs/cifs  (assuming that that is still recommended).

On Thu, Mar 14, 2019 at 2:39 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (03/14/19 02:19), Steve French wrote:
> > All of those uses of __constant_cpu_to_le32 apparently (at least
> > according to checkpatch) should be changed (someday) to cpu_to_le32
> > but I didn't research why the change from __constant_cpu_to_le32
> > --->   cpu_to_le32
>
> Probably historic reasons.
>
> Looking at linux 2.4.21
>
> /*
>  * Allow constant folding
>  */
> #if defined(__GNUC__) && (__GNUC__ >= 2) && defined(__OPTIMIZE__)
> #  define __swahw32(x) \
> (__builtin_constant_p((__u32)(x)) ? \
>  ___swahw32((x)) : \
>  __fswahw32((x)))
>
>
> My assumption would be that __GNUC__ < 2 did no support
> __builtin_constant_p?
>
>
> > If it has benefit - and checkpatch is right (it warned about
> > __constant_cpu_to_le32 being no longer preferred) ... perhaps would be
> > worth a followup patch to clean the rest of them up?   If you have any
> > context on why kernel code has moved away from using the older format
> > of __constant_cpu_to_.... would be useful to know if any benefit to
> > the change
>
> Right, that's what I'm going to do - send out patches and update the rest
> of __constant_cpu_to_XX users; so, eventually, __constant_cpu_to_XX
> can be removed.
>
>         -ss



-- 
Thanks,

Steve

RE: [PATCH 2/2] cifs: don't use __constant_cpu_to_le32()

[David Laight]

From: Sergey Senozhatsky
> 
> cpu_to_le32() is capable enough to detect __builtin_constant_p()
> and to use an appropriate compile time ___constant_swahb32()
> function.
> 
> So we can use cpu_to_le32() instead of __constant_cpu_to_le32().

Unless any code tries to use them as case statement labels.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/2] cifs: don't use __constant_cpu_to_le32()

[Sergey Senozhatsky]

On (03/15/19 12:31), David Laight wrote:
> From: Sergey Senozhatsky
> > 
> > cpu_to_le32() is capable enough to detect __builtin_constant_p()
> > and to use an appropriate compile time ___constant_swahb32()
> > function.
> > 
> > So we can use cpu_to_le32() instead of __constant_cpu_to_le32().
> 
> Unless any code tries to use them as case statement labels.

What is the problem?

For compile time constants cpu_to_le32() should do the same thing
as __constant_cpu_to_le32().

There seems a whole bunch of cpu_to_XX compile time constants which
are used in case statement labels

include/linux/sunrpc/xdr.h:#define      rpc_msg_accepted        cpu_to_be32(RPC_MSG_ACCEPTED)
include/linux/sunrpc/xdr.h:#define      rpc_success             cpu_to_be32(RPC_SUCCESS)
include/linux/sunrpc/xdr.h:#define      rpc_prog_unavail        cpu_to_be32(RPC_PROG_UNAVAIL)
include/linux/sunrpc/xdr.h:#define      rpc_prog_mismatch       cpu_to_be32(RPC_PROG_MISMATCH)
include/linux/sunrpc/xdr.h:#define      rpc_proc_unavail        cpu_to_be32(RPC_PROC_UNAVAIL)
include/linux/sunrpc/xdr.h:#define      rpc_garbage_args        cpu_to_be32(RPC_GARBAGE_ARGS)
include/linux/sunrpc/xdr.h:#define      rpc_system_err          cpu_to_be32(RPC_SYSTEM_ERR)
include/linux/sunrpc/xdr.h:#define      rpc_drop_reply          cpu_to_be32(RPC_DROP_REPLY)
include/linux/sunrpc/xdr.h:#define      rpc_mismatch            cpu_to_be32(RPC_MISMATCH)
include/linux/sunrpc/xdr.h:#define      rpc_auth_error          cpu_to_be32(RPC_AUTH_ERROR)
include/linux/sunrpc/xdr.h:#define      rpc_auth_ok             cpu_to_be32(RPC_AUTH_OK)
include/linux/sunrpc/xdr.h:#define      rpc_autherr_badcred     cpu_to_be32(RPC_AUTH_BADCRED)
include/linux/sunrpc/xdr.h:#define      rpc_autherr_rejectedcred cpu_to_be32(RPC_AUTH_REJECTEDCRED)
include/linux/sunrpc/xdr.h:#define      rpc_autherr_badverf     cpu_to_be32(RPC_AUTH_BADVERF)
include/linux/sunrpc/xdr.h:#define      rpc_autherr_rejectedverf cpu_to_be32(RPC_AUTH_REJECTEDVERF)
include/linux/sunrpc/xdr.h:#define      rpc_autherr_tooweak     cpu_to_be32(RPC_AUTH_TOOWEAK)


net/sunrpc/clnt.c

2479         switch (*p) {
2480         case rpc_success:
2481                 return 0;
2482         case rpc_prog_unavail:
2483                 trace_rpc__prog_unavail(task);
2484                 error = -EPFNOSUPPORT;
2485                 goto out_err;
2486         case rpc_prog_mismatch:
2487                 trace_rpc__prog_mismatch(task);
2488                 error = -EPROTONOSUPPORT;
2489                 goto out_err;
2490         case rpc_proc_unavail:
2491                 trace_rpc__proc_unavail(task);
2492                 error = -EOPNOTSUPP;
2493                 goto out_err;
2494         case rpc_garbage_args:
2495                 trace_rpc__garbage_args(task);
2496                 break;
2497         default:
2498                 trace_rpc__unparsable(task);
2499         }

	-ss