On Mon, Feb 28, 2022 at 1:13 PM Guo Ren [off-list ref] wrote:

On Mon, Feb 28, 2022 at 8:02 PM David Laight [off-list ref] wrote:

quoted

From: Guo Ren Sent: 28 February 2022 11:52

quoted

On Mon, Feb 28, 2022 at 2:40 PM David Laight [off-list ref] wrote:

quoted

...

quoted

+struct compat_flock64 {
+     short           l_type;
+     short           l_whence;
+     compat_loff_t   l_start;
+     compat_loff_t   l_len;
+     compat_pid_t    l_pid;
+#ifdef __ARCH_COMPAT_FLOCK64_PAD
+     __ARCH_COMPAT_FLOCK64_PAD
+#endif
+} __ARCH_COMPAT_FLOCK64_PACK;
+

Provided compat_loff_t are correctly defined with __aligned__(4)

See include/asm-generic/compat.h

typedef s64 compat_loff_t;

Only:
#ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT
typedef s64 __attribute__((aligned(4))) compat_s64;

So how do you think compat_loff_t could be defined with __aligned__(4)?

compat_loff_t should be compat_s64 not s64.

The same should be done for all 64bit 'compat' types.

Changing
typedef s64 compat_loff_t;
to
typedef compat_s64 compat_loff_t;

should be another patch and it affects all architectures, I don't
think we should involve it in this series.

Agreed, your patch (originally from Christoph) looks fine, it correctly
transforms the seven copies of the structure into a shared version.

There is always more that can be improved, but for this series,
I think you have already done enough.

look at kernel/power/user.c:
struct compat_resume_swap_area {
        compat_loff_t offset;
        u32 dev;
} __packed;

I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for
COMPAT support patchset series.

The only references to compat_loff_t that we have in the kernel
could all be simplified by defining compat_loff_t as compat_s64
instead of s64, but it has no impact on correctness here.

Let's make sure you get your series into 5.18, and then David can
follow-up with any further cleanups after that.

         Arnd