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

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

[Zhang Ning]

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.

Signed-off-by: Zhang Ning <ning.a.zhang@intel.com>
Signed-off-by: Li, Ting <ting.li@intel.com>
---
 firmware/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/firmware/Makefile b/firmware/Makefile
index 29641383e136..d7bfce56378f 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -16,10 +16,11 @@ quiet_cmd_fwbin = MK_FW   $@
 				firmware/%.gen.S,%,$@))))";		     \
 		  ASM_WORD=$(if $(CONFIG_64BIT),.quad,.long);		     \
 		  ASM_ALIGN=$(if $(CONFIG_64BIT),3,2);			     \
+		  ASM_FW_ALIGN=12;			  	   	     \
 		  PROGBITS=$(if $(CONFIG_ARM),%,@)progbits;		     \
 		  echo "/* Generated by firmware/Makefile */"		> $@;\
 		  echo "    .section .rodata"				>>$@;\
-		  echo "    .p2align $${ASM_ALIGN}"			>>$@;\
+		  echo "    .p2align $${ASM_FW_ALIGN}"			>>$@;\
 		  echo "_fw_$${FWSTR}_bin:"				>>$@;\
 		  echo "    .incbin \"$(2)\""				>>$@;\
 		  echo "_fw_end:"					>>$@;\
-- 
2.18.0

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

[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 :(

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

[Zhang, Ning A]

在 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.

BR.
Ning.

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

[gregkh]

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?

Please fix this up submit this properly.

greg k-h

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

[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.




> Please fix this up submit this properly.
> 
> greg k-h

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@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

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

[gregkh]

On Mon, Aug 06, 2018 at 01:48:44AM +0000, Zhang, Ning A wrote:
> 在 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.

Then I can not accept your patch.  Go talk to your corporate lawyers
about changing core kernel code for a closed source driver and what that
implies about that closed driver (hint, your driver can not be
closed...) :)

Then come back with a proper open sourced driver, that's the only way
Linux drivers can be written sorry.

best of luck,

greg k-h

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

[Zhang, Ning A]

在 2018-08-06一的 16:05 +0200，gregkh@linuxfoundation.org写道：
> On Mon, Aug 06, 2018 at 01:48:44AM +0000, Zhang, Ning A wrote:
> > 在 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.
> 
> Then I can not accept your patch.  Go talk to your corporate lawyers
> about changing core kernel code for a closed source driver and what
> that
> implies about that closed driver (hint, your driver can not be
> closed...) :)

It's very sad, but anyway, thank you for your review.
your review will definitely help us improve our work.

BR.
Ning.

> 
> Then come back with a proper open sourced driver, that's the only way
> Linux drivers can be written sorry.
> 
> best of luck,
> 
> greg k-h
> 

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

[gregkh]

On Tue, Aug 07, 2018 at 02:27:31AM +0000, Zhang, Ning A wrote:
> 在 2018-08-06一的 16:05 +0200，gregkh@linuxfoundation.org写道：
> > On Mon, Aug 06, 2018 at 01:48:44AM +0000, Zhang, Ning A wrote:
> > > 在 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.
> > 
> > Then I can not accept your patch.  Go talk to your corporate lawyers
> > about changing core kernel code for a closed source driver and what
> > that
> > implies about that closed driver (hint, your driver can not be
> > closed...) :)
> 
> It's very sad,

"sad"?  Again, please go discuss this with your corporate lawyers before
thinking that this is even something that is possible to do.  Hint, if
I _were_ to accept this, they would be _VERY_ upset at both me, and you.

I am trying to _save_ you problems, please realize this.

greg k-h