lkml.org 
[lkml]   [2018]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] firmware: make sure builtin firmware is page alignment
Date
在 2018-08-06一的 01:48 +0000,Zhang, Ning A写道:
> 在 2018-08-03五的 12:31 +0200,gregkh@linuxfoundation.org写道:
> > On Fri, Aug 03, 2018 at 08:42:25AM +0000, Zhang, Ning A wrote:
> > > 在 2018-08-03五的 07:39 +0200,Greg KH写道:
> > > > On Fri, Aug 03, 2018 at 09:45:21AM +0800, Zhang Ning wrote:
> > > > > when firmware is in filesystem, request_firmware will load
> > > > > it,
> > > > > and copy it to vmalloc memory, that is page align memory.
> > > > >
> > > > > but when firmware is builtin, it is 8 bytes or 4 bytes
> > > > > alignment.
> > > > >
> > > > > make sure builtin firmware is page algnment, that can
> > > > > simplify
> > > > > algorithm
> > > > > to handle firmware.
> > > >
> > > > How is it simplified?  I don't see any such change like that
> > > > here
> > > > :(
> > > >
> > >
> > > Thank you for review this patch.
> > >
> > > When driver handles its firmware based on  page, like below:
> > >
> > > struct page *p;
> > > p = vmalloc_to_page(fw->data);  // for filesystem firmware
> > > p = virt_to_page(fw->data); // for builtin firmware
> > >
> > > but if builtin firmware is not page alignment, page pointer for
> > > builtin
> > > firmware is wrong, it contains memory not belong to firmware.
> > > drivers
> > > has to use additional code to handle this. 
> > >
> > > if builtin firmware is also page alignment, no need additional
> > > code
> > > to
> > > handle builtin firmware. simplified.
> >
> > But you did not change anything like this in your code, so why
> > would
> > I
> > know this?
>
> I understand it is very difficult to review this patch without
> context.
> The driver is not opensource, I can't show the patch for driver.
>
> this patch changes kernel common code, and it has value to upstream,
> so
> I submit this patch for review and also look forward some advises
> from
> community. Once we decide to opensource, the driver changes will be
> there.
>
>

as said our driver handles firmware based on page, and if firmware is
builtin, page point from virt_to_page(fw->data) contains memory doesn't
belong to firmware. there are two way to fix it.

1, copy builtin firmware to vmalloc memory, this is additional work to
handle firmware. I create a wrap function for firmware request.

int request_XYZ_fw(const struct firmware **firmware_p, const char
*name,
 struct device *device)
{
const struct firmware *fw;
struct firmware *tmp;
int ret;

ret = request_firmware(&fw, name, device);

if (ret < 0)
return ret;

if (is_vmalloc_addr(fw->data))
*firmware_p = fw;
else {
tmp = (struct firmware *)kzalloc(sizeof(struct
firmware), GFP_KERNEL);
if (!tmp)
return -ENOMEM;
tmp->size = fw->size;
tmp->data = vmalloc(fw->size);
memcpy(tmp->data, fw->data, fw->size);
*firmware_p = tmp;
}
return ret;
}

2, make builtin firmware is also page alignment. the change in driver
code is to check memory type of fw->data, and use different XXX_to_page
function.

struct page *p;
if (is_vmalloc_addr(fw->data)) // new
p = vmalloc_to_page(fw->data);
else // new
p = virt_to_page(fw->data); // new


compare to solution, solution 2 is simple, this is what I said
simplified. and solution 2 also save memory (I don't know the affect
change memory alignment on memory usage.)

Hi, Greg
is this easier for you to review?

BR.
Ning.
>

>
Please fix this up submit this properly.

greg k-h
\
 
 \ /
  Last update: 2018-08-06 07:14    [W:0.071 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site