Re: [PATCH] firmware: make sure builtin firmware is page alignment

[Zhang, Ning A]

å 2018-08-06äç 01:48 +0000ïZhang, Ning Aåéï
> å 2018-08-03äç 12:31 +0200ïgregkh@xxxxxxxxxxxxxxxxxxxåéï
> > 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