Re: [PATCH v3 2/2] kexec: Use min_t to simplify logic

Andrew Morton
2013-02-27T22:31:36Z

On Tue, 26 Feb 2013 16:55:57 +0800
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:

> ___ 2013___02___26___ 16:53, Andrew Morton ______:
> > On Tue, 26 Feb 2013 16:49:02 +0800 Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:
> >
> >>>> diff --git a/kernel/kexec.c b/kernel/kexec.c
> >>> []
> >>>> @@ -822,13 +822,9 @@ static int kimage_load_normal_segment(struct kimage *image,
> >>> []
> >>>> + mchunk = min_t(size_t, mbytes,
> >>>> + (size_t)(PAGE_SIZE - (maddr & ~PAGE_MASK)));
> >>>
> >>> #define min_t(type, x, y) ({ \
> >>> type __min1 = (x); \
> >>> type __min2 = (y); \
> >>> __min1 < __min2 ? __min1: __min2; })
> >>>
> >>>
> >>>
> >>
> >> Hmm, from the definition, the cast is redundant.
> >>
> >> Maybe I misunderstood what Andrew meant in the mail related to v2:
> >>
> >> "The types of PAGE_SIZE and PAGE_MASK are vague - iirc they once had
> >> different types on different architectures, so some form of casting is
> >> unavoidable here."
> >>
> >> Andrew, could you please explain the casting you meant above?
> >
> > I mean that a cast (or min_t, which is a cast) will be needed.
> > The code you have here casts the same thing two times, which isn't
> > necessary.
> >
> >
>
> Thanks.
>
> Should I resend the patch again? After removing this redundant cast,
> it is the same with v2.

That patch was pretty busted. This should fix everything up:

--- a/kernel/kexec.c~kexec-use-min_t-to-simplify-logic-fix
+++ a/kernel/kexec.c
@@ -820,8 +820,8 @@ static int kimage_load_normal_segment(st
clear_page(ptr);
ptr += maddr & ~PAGE_MASK;
mchunk = min_t(size_t, mbytes,
- (size_t)(PAGE_SIZE - (maddr & ~PAGE_MASK)));
- uchunk = min_t(size_t, ubytes, mchunk);
+ PAGE_SIZE - (maddr & ~PAGE_MASK));
+ uchunk = min(ubytes, mchunk);

result = copy_from_user(ptr, buf, uchunk);
kunmap(page);
@@ -868,8 +868,8 @@ static int kimage_load_crash_segment(str
ptr = kmap(page);
ptr += maddr & ~PAGE_MASK;
mchunk = min_t(size_t, mbytes,
- (size_t)(PAGE_SIZE - (maddr & ~PAGE_MASK)));
- uchunk = min_t(size_t, ubytes, mchunk);
+ PAGE_SIZE - (maddr & ~PAGE_MASK));
+ uchunk = min(ubytes, mchunk);
if (mchunk > uchunk) {
/* Zero the trailing part of the page */
memset(ptr + uchunk, 0, mchunk - uchunk);
@@ -1451,7 +1451,7 @@ void vmcoreinfo_append_str(const char *f
r = vsnprintf(buf, sizeof(buf), fmt, args);
va_end(args);

- r = min_t(size_t, r, vmcoreinfo_max_size - vmcoreinfo_size);
+ r = min(r, vmcoreinfo_max_size - vmcoreinfo_size);

memcpy(&vmcoreinfo_data[vmcoreinfo_size], buf, r);

_