* Possible broken MM code in dell-laptop.c?
@ 2015-06-14 9:05 Pali Rohár
2015-06-15 20:36 ` Darren Hart
2015-06-15 21:18 ` Michal Hocko
0 siblings, 2 replies; 22+ messages in thread
From: Pali Rohár @ 2015-06-14 9:05 UTC (permalink / raw)
To: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes,
Matthew Garrett, Andrew Morton, Michal Hocko
Cc: platform-driver-x86, linux-mm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]
Hello,
in drivers/platform/x86/dell-laptop.c is this part of code:
static int __init dell_init(void)
{
...
/*
* Allocate buffer below 4GB for SMI data--only 32-bit physical addr
* is passed to SMI handler.
*/
bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32);
if (!bufferpage) {
ret = -ENOMEM;
goto fail_buffer;
}
buffer = page_address(bufferpage);
ret = dell_setup_rfkill();
if (ret) {
pr_warn("Unable to setup rfkill\n");
goto fail_rfkill;
}
...
fail_rfkill:
free_page((unsigned long)bufferpage);
fail_buffer:
...
}
Then there is another part:
static void __exit dell_exit(void)
{
...
free_page((unsigned long)buffer);
}
I suspect that there is some problem with free_page() call. In dell_init
is called free_page() on bufferpage and in dell_exit() on buffer.
Matthew and Stuart, you introduced this inconsistency in commit:
-------------------------------------------------
commit 116ee77b2858d9c89c0327f3a47c8ba864bf4a96
Author: Stuart Hayes <stuart_hayes@dell.com>
Committer: Matthew Garrett <mjg@redhat.com>
Date: Wed Feb 10 14:12:13 2010 -0500
dell-laptop: Use buffer with 32-bit physical address
Calls to communicate with system firmware via a SMI (using dcdbas)
need to use a buffer that has a physical address of 4GB or less.
Currently the dell-laptop driver does not guarantee this, and when
the
buffer address is higher than 4GB, the address is truncated to 32
bits
and the SMI handler writes to the wrong memory address.
Signed-off-by: Stuart Hayes <stuart_hayes@dell.com>
Acked-by: Matthew Garrett <mjg@redhat.com>
-------------------------------------------------
Can you or somebody else (CCed linux-mm) look at this page related code?
I think it is wrong, but somebody authoritative should provide answer.
Thanks.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Possible broken MM code in dell-laptop.c? 2015-06-14 9:05 Possible broken MM code in dell-laptop.c? Pali Rohár @ 2015-06-15 20:36 ` Darren Hart 2015-06-15 21:18 ` Michal Hocko 1 sibling, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-15 20:36 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, Michal Hocko, platform-driver-x86, linux-mm, linux-kernel On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Rohár wrote: > Hello, > > in drivers/platform/x86/dell-laptop.c is this part of code: > > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > if (!bufferpage) { > ret = -ENOMEM; > goto fail_buffer; > } > buffer = page_address(bufferpage); > > ret = dell_setup_rfkill(); > > if (ret) { > pr_warn("Unable to setup rfkill\n"); > goto fail_rfkill; > } > ... > fail_rfkill: > free_page((unsigned long)bufferpage); > fail_buffer: > ... > } > > Then there is another part: > > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); I believe you are correct, and this should be bufferpage. Have you observed any failures? -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-15 20:36 ` Darren Hart 0 siblings, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-15 20:36 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, Michal Hocko, platform-driver-x86, linux-mm, linux-kernel On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Rohar wrote: > Hello, > > in drivers/platform/x86/dell-laptop.c is this part of code: > > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > if (!bufferpage) { > ret = -ENOMEM; > goto fail_buffer; > } > buffer = page_address(bufferpage); > > ret = dell_setup_rfkill(); > > if (ret) { > pr_warn("Unable to setup rfkill\n"); > goto fail_rfkill; > } > ... > fail_rfkill: > free_page((unsigned long)bufferpage); > fail_buffer: > ... > } > > Then there is another part: > > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); I believe you are correct, and this should be bufferpage. Have you observed any failures? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-15 20:36 ` Darren Hart (?) @ 2015-06-15 20:42 ` Pali Rohár 2015-06-16 10:12 ` Pavel Machek -1 siblings, 1 reply; 22+ messages in thread From: Pali Rohár @ 2015-06-15 20:42 UTC (permalink / raw) To: Darren Hart Cc: Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, Michal Hocko, platform-driver-x86, linux-mm, linux-kernel, Pavel Machek [-- Attachment #1: Type: Text/Plain, Size: 1436 bytes --] On Monday 15 June 2015 22:36:45 Darren Hart wrote: > On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Rohár wrote: > > Hello, > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > static int __init dell_init(void) > > { > > ... > > > > /* > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > addr * is passed to SMI handler. > > */ > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > if (!bufferpage) { > > > > ret = -ENOMEM; > > goto fail_buffer; > > > > } > > buffer = page_address(bufferpage); > > > > ret = dell_setup_rfkill(); > > > > if (ret) { > > > > pr_warn("Unable to setup rfkill\n"); > > goto fail_rfkill; > > > > } > > > > ... > > > > fail_rfkill: > > free_page((unsigned long)bufferpage); > > > > fail_buffer: > > ... > > } > > > > Then there is another part: > > > > static void __exit dell_exit(void) > > { > > ... > > > > free_page((unsigned long)buffer); > > I believe you are correct, and this should be bufferpage. Have you > observed any failures? Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I think that buffer (and not bufferpage) should be passed to free_page(). So in my opinion problem is at fail_rfkill: label and not in dell_exit(). But somebody from linux-mm should look at it... -- Pali Rohár pali.rohar@gmail.com [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-15 20:42 ` Pali Rohár @ 2015-06-16 10:12 ` Pavel Machek 0 siblings, 0 replies; 22+ messages in thread From: Pavel Machek @ 2015-06-16 10:12 UTC (permalink / raw) To: Pali Rohár Cc: Darren Hart, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, Michal Hocko, platform-driver-x86, linux-mm, linux-kernel On Mon 2015-06-15 22:42:30, Pali Rohár wrote: > On Monday 15 June 2015 22:36:45 Darren Hart wrote: > > On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Rohár wrote: > > > Hello, > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > static int __init dell_init(void) > > > { > > > ... > > > > > > /* > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > if (!bufferpage) { > > > > > > ret = -ENOMEM; > > > goto fail_buffer; > > > > > > } > > > buffer = page_address(bufferpage); > > > > > > ret = dell_setup_rfkill(); > > > > > > if (ret) { > > > > > > pr_warn("Unable to setup rfkill\n"); > > > goto fail_rfkill; > > > > > > } > > > > > > ... > > > > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > > > > > > fail_buffer: > > > ... > > > } > > > > > > Then there is another part: > > > > > > static void __exit dell_exit(void) > > > { > > > ... > > > > > > free_page((unsigned long)buffer); > > > > I believe you are correct, and this should be bufferpage. Have you > > observed any failures? > > Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I think > that buffer (and not bufferpage) should be passed to free_page(). So in > my opinion problem is at fail_rfkill: label and not in dell_exit(). You seem to be right. Interface is strange... alloc_pages() returns struct page *, __free_pages() takes struct page *, free_pages() takes unsinged long. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-16 10:12 ` Pavel Machek 0 siblings, 0 replies; 22+ messages in thread From: Pavel Machek @ 2015-06-16 10:12 UTC (permalink / raw) To: Pali Rohár Cc: Darren Hart, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, Michal Hocko, platform-driver-x86, linux-mm, linux-kernel On Mon 2015-06-15 22:42:30, Pali Rohar wrote: > On Monday 15 June 2015 22:36:45 Darren Hart wrote: > > On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Rohar wrote: > > > Hello, > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > static int __init dell_init(void) > > > { > > > ... > > > > > > /* > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > if (!bufferpage) { > > > > > > ret = -ENOMEM; > > > goto fail_buffer; > > > > > > } > > > buffer = page_address(bufferpage); > > > > > > ret = dell_setup_rfkill(); > > > > > > if (ret) { > > > > > > pr_warn("Unable to setup rfkill\n"); > > > goto fail_rfkill; > > > > > > } > > > > > > ... > > > > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > > > > > > fail_buffer: > > > ... > > > } > > > > > > Then there is another part: > > > > > > static void __exit dell_exit(void) > > > { > > > ... > > > > > > free_page((unsigned long)buffer); > > > > I believe you are correct, and this should be bufferpage. Have you > > observed any failures? > > Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I think > that buffer (and not bufferpage) should be passed to free_page(). So in > my opinion problem is at fail_rfkill: label and not in dell_exit(). You seem to be right. Interface is strange... alloc_pages() returns struct page *, __free_pages() takes struct page *, free_pages() takes unsinged long. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-14 9:05 Possible broken MM code in dell-laptop.c? Pali Rohár 2015-06-15 20:36 ` Darren Hart @ 2015-06-15 21:18 ` Michal Hocko 1 sibling, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-15 21:18 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Sun 14-06-15 11:05:07, Pali Rohár wrote: > Hello, > > in drivers/platform/x86/dell-laptop.c is this part of code: > > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); [...] > buffer = page_address(bufferpage); [...] > fail_rfkill: > free_page((unsigned long)bufferpage); This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page. I have no idea why the API has been done this way and yeah, it is really confusing. [...] > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-15 21:18 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-15 21:18 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Sun 14-06-15 11:05:07, Pali Rohar wrote: > Hello, > > in drivers/platform/x86/dell-laptop.c is this part of code: > > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); [...] > buffer = page_address(bufferpage); [...] > fail_rfkill: > free_page((unsigned long)bufferpage); This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page. I have no idea why the API has been done this way and yeah, it is really confusing. [...] > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-15 21:18 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-15 21:18 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Sun 14-06-15 11:05:07, Pali Rohár wrote: > Hello, > > in drivers/platform/x86/dell-laptop.c is this part of code: > > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); [...] > buffer = page_address(bufferpage); [...] > fail_rfkill: > free_page((unsigned long)bufferpage); This one should be __free_page because it consumes struct page* and it is the proper counter part for alloc_page. free_page, just to make it confusing, consumes an address which has to be translated to a struct page. I have no idea why the API has been done this way and yeah, it is really confusing. [...] > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-15 21:18 ` Michal Hocko (?) (?) @ 2015-06-15 21:27 ` Pali Rohár 2015-06-16 6:33 ` Michal Hocko -1 siblings, 1 reply; 22+ messages in thread From: Pali Rohár @ 2015-06-15 21:27 UTC (permalink / raw) To: Michal Hocko Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel [-- Attachment #1: Type: Text/Plain, Size: 1182 bytes --] On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > On Sun 14-06-15 11:05:07, Pali Rohár wrote: > > Hello, > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > static int __init dell_init(void) > > { > > ... > > > > /* > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > addr * is passed to SMI handler. > > */ > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > [...] > > > buffer = page_address(bufferpage); > > [...] > > > fail_rfkill: > > free_page((unsigned long)bufferpage); > > This one should be __free_page because it consumes struct page* and > it is the proper counter part for alloc_page. free_page, just to > make it confusing, consumes an address which has to be translated to > a struct page. > > I have no idea why the API has been done this way and yeah, it is > really confusing. > > [...] > > > static void __exit dell_exit(void) > > { > > ... > > > > free_page((unsigned long)buffer); So both, either: free_page((unsigned long)buffer); or __free_page(bufferpage); is correct? -- Pali Rohár pali.rohar@gmail.com [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-15 21:27 ` Pali Rohár @ 2015-06-16 6:33 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-16 6:33 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Mon 15-06-15 23:27:59, Pali Rohár wrote: > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > On Sun 14-06-15 11:05:07, Pali Rohár wrote: > > > Hello, > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > static int __init dell_init(void) > > > { > > > ... > > > > > > /* > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > [...] > > > > > buffer = page_address(bufferpage); > > > > [...] > > > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > > > > This one should be __free_page because it consumes struct page* and > > it is the proper counter part for alloc_page. free_page, just to > > make it confusing, consumes an address which has to be translated to > > a struct page. > > > > I have no idea why the API has been done this way and yeah, it is > > really confusing. > > > > [...] > > > > > static void __exit dell_exit(void) > > > { > > > ... > > > > > > free_page((unsigned long)buffer); > > So both, either: > > free_page((unsigned long)buffer); > > or > > __free_page(bufferpage); > > is correct? Yes. Although I would use __free_page variant as both seem to be globally visible. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-16 6:33 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-16 6:33 UTC (permalink / raw) To: Pali Rohár Cc: Hans de Goede, Darren Hart, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Mon 15-06-15 23:27:59, Pali Rohar wrote: > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > On Sun 14-06-15 11:05:07, Pali Rohar wrote: > > > Hello, > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > static int __init dell_init(void) > > > { > > > ... > > > > > > /* > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > [...] > > > > > buffer = page_address(bufferpage); > > > > [...] > > > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > > > > This one should be __free_page because it consumes struct page* and > > it is the proper counter part for alloc_page. free_page, just to > > make it confusing, consumes an address which has to be translated to > > a struct page. > > > > I have no idea why the API has been done this way and yeah, it is > > really confusing. > > > > [...] > > > > > static void __exit dell_exit(void) > > > { > > > ... > > > > > > free_page((unsigned long)buffer); > > So both, either: > > free_page((unsigned long)buffer); > > or > > __free_page(bufferpage); > > is correct? Yes. Although I would use __free_page variant as both seem to be globally visible. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-16 6:33 ` Michal Hocko @ 2015-06-16 7:15 ` Pali Rohár -1 siblings, 0 replies; 22+ messages in thread From: Pali Rohár @ 2015-06-16 7:15 UTC (permalink / raw) To: Michal Hocko, Darren Hart Cc: Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > On Mon 15-06-15 23:27:59, Pali Rohár wrote: > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > On Sun 14-06-15 11:05:07, Pali Rohár wrote: > > > > Hello, > > > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > > > static int __init dell_init(void) > > > > { > > > > ... > > > > > > > > /* > > > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > > addr * is passed to SMI handler. > > > > */ > > > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > > > [...] > > > > > > > buffer = page_address(bufferpage); > > > > > > [...] > > > > > > > fail_rfkill: > > > > free_page((unsigned long)bufferpage); > > > > > > This one should be __free_page because it consumes struct page* and > > > it is the proper counter part for alloc_page. free_page, just to > > > make it confusing, consumes an address which has to be translated to > > > a struct page. > > > > > > I have no idea why the API has been done this way and yeah, it is > > > really confusing. > > > > > > [...] > > > > > > > static void __exit dell_exit(void) > > > > { > > > > ... > > > > > > > > free_page((unsigned long)buffer); > > > > So both, either: > > > > free_page((unsigned long)buffer); > > > > or > > > > __free_page(bufferpage); > > > > is correct? > > Yes. Although I would use __free_page variant as both seem to be > globally visible. > Michal, thank you for explaining this situation! Darren, I will prepare patch which will fix code and use __free_page(). (Btw, execution on fail_rfkill label caused kernel panic) -- Pali Rohár pali.rohar@gmail.com ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-16 7:15 ` Pali Rohár 0 siblings, 0 replies; 22+ messages in thread From: Pali Rohár @ 2015-06-16 7:15 UTC (permalink / raw) To: Michal Hocko, Darren Hart Cc: Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > On Mon 15-06-15 23:27:59, Pali RohA!r wrote: > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > On Sun 14-06-15 11:05:07, Pali RohA!r wrote: > > > > Hello, > > > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > > > static int __init dell_init(void) > > > > { > > > > ... > > > > > > > > /* > > > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > > addr * is passed to SMI handler. > > > > */ > > > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > > > [...] > > > > > > > buffer = page_address(bufferpage); > > > > > > [...] > > > > > > > fail_rfkill: > > > > free_page((unsigned long)bufferpage); > > > > > > This one should be __free_page because it consumes struct page* and > > > it is the proper counter part for alloc_page. free_page, just to > > > make it confusing, consumes an address which has to be translated to > > > a struct page. > > > > > > I have no idea why the API has been done this way and yeah, it is > > > really confusing. > > > > > > [...] > > > > > > > static void __exit dell_exit(void) > > > > { > > > > ... > > > > > > > > free_page((unsigned long)buffer); > > > > So both, either: > > > > free_page((unsigned long)buffer); > > > > or > > > > __free_page(bufferpage); > > > > is correct? > > Yes. Although I would use __free_page variant as both seem to be > globally visible. > Michal, thank you for explaining this situation! Darren, I will prepare patch which will fix code and use __free_page(). (Btw, execution on fail_rfkill label caused kernel panic) -- Pali RohA!r pali.rohar@gmail.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-16 7:15 ` Pali Rohár @ 2015-06-16 7:43 ` Michal Hocko -1 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-16 7:43 UTC (permalink / raw) To: Pali Rohár Cc: Darren Hart, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue 16-06-15 09:15:23, Pali Rohár wrote: [...] > Michal, thank you for explaining this situation! > > Darren, I will prepare patch which will fix code and use __free_page(). > > (Btw, execution on fail_rfkill label caused kernel panic) I am sorry, I could have made it more clear in the very first email. A panic is to be expected because free_page will translate the given address to a struct page* but this is what the code gave it. So an unrelated struct page would be freed (or maybe an invalid one). -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-16 7:43 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-16 7:43 UTC (permalink / raw) To: Pali Rohár Cc: Darren Hart, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue 16-06-15 09:15:23, Pali Rohar wrote: [...] > Michal, thank you for explaining this situation! > > Darren, I will prepare patch which will fix code and use __free_page(). > > (Btw, execution on fail_rfkill label caused kernel panic) I am sorry, I could have made it more clear in the very first email. A panic is to be expected because free_page will translate the given address to a struct page* but this is what the code gave it. So an unrelated struct page would be freed (or maybe an invalid one). -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-16 7:15 ` Pali Rohár @ 2015-06-17 3:43 ` Darren Hart -1 siblings, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-17 3:43 UTC (permalink / raw) To: Pali Rohár Cc: Michal Hocko, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue, Jun 16, 2015 at 09:15:23AM +0200, Pali Rohár wrote: > On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > > On Mon 15-06-15 23:27:59, Pali Rohár wrote: > > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > > On Sun 14-06-15 11:05:07, Pali Rohár wrote: > > > > > Hello, > > > > > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > > > > > static int __init dell_init(void) > > > > > { > > > > > ... > > > > > > > > > > /* > > > > > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > > > addr * is passed to SMI handler. > > > > > */ > > > > > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > > > > > [...] > > > > > > > > > buffer = page_address(bufferpage); > > > > > > > > [...] > > > > > > > > > fail_rfkill: > > > > > free_page((unsigned long)bufferpage); > > > > > > > > This one should be __free_page because it consumes struct page* and > > > > it is the proper counter part for alloc_page. free_page, just to > > > > make it confusing, consumes an address which has to be translated to > > > > a struct page. > > > > > > > > I have no idea why the API has been done this way and yeah, it is > > > > really confusing. > > > > > > > > [...] > > > > > > > > > static void __exit dell_exit(void) > > > > > { > > > > > ... > > > > > > > > > > free_page((unsigned long)buffer); > > > > > > So both, either: > > > > > > free_page((unsigned long)buffer); > > > > > > or > > > > > > __free_page(bufferpage); > > > > > > is correct? > > > > Yes. Although I would use __free_page variant as both seem to be > > globally visible. > > Michal - thanks for the context. I'm surprised by your recommendation to use __free_page() out here in platform driver land. I'd also prefer that the driver consistently free the same address to avoid confusion. For these reasons, free_page((unsigned long)buffer) seems like the better option. Can you elaborate on why you feel __free_page() is a better choice? -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-17 3:43 ` Darren Hart 0 siblings, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-17 3:43 UTC (permalink / raw) To: Pali Rohár Cc: Michal Hocko, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue, Jun 16, 2015 at 09:15:23AM +0200, Pali Rohar wrote: > On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > > On Mon 15-06-15 23:27:59, Pali Rohar wrote: > > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > > On Sun 14-06-15 11:05:07, Pali Rohar wrote: > > > > > Hello, > > > > > > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > > > > > > > static int __init dell_init(void) > > > > > { > > > > > ... > > > > > > > > > > /* > > > > > > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > > > addr * is passed to SMI handler. > > > > > */ > > > > > > > > > > bufferpage = alloc_page(GFP_KERNEL | GFP_DMA32); > > > > > > > > [...] > > > > > > > > > buffer = page_address(bufferpage); > > > > > > > > [...] > > > > > > > > > fail_rfkill: > > > > > free_page((unsigned long)bufferpage); > > > > > > > > This one should be __free_page because it consumes struct page* and > > > > it is the proper counter part for alloc_page. free_page, just to > > > > make it confusing, consumes an address which has to be translated to > > > > a struct page. > > > > > > > > I have no idea why the API has been done this way and yeah, it is > > > > really confusing. > > > > > > > > [...] > > > > > > > > > static void __exit dell_exit(void) > > > > > { > > > > > ... > > > > > > > > > > free_page((unsigned long)buffer); > > > > > > So both, either: > > > > > > free_page((unsigned long)buffer); > > > > > > or > > > > > > __free_page(bufferpage); > > > > > > is correct? > > > > Yes. Although I would use __free_page variant as both seem to be > > globally visible. > > Michal - thanks for the context. I'm surprised by your recommendation to use __free_page() out here in platform driver land. I'd also prefer that the driver consistently free the same address to avoid confusion. For these reasons, free_page((unsigned long)buffer) seems like the better option. Can you elaborate on why you feel __free_page() is a better choice? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-17 3:43 ` Darren Hart @ 2015-06-17 7:19 ` Michal Hocko -1 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-17 7:19 UTC (permalink / raw) To: Darren Hart Cc: Pali Rohár, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue 16-06-15 20:43:34, Darren Hart wrote: [...] > Michal - thanks for the context. > > I'm surprised by your recommendation to use __free_page() out here in platform > driver land. > > I'd also prefer that the driver consistently free the same address to avoid > confusion. > > For these reasons, free_page((unsigned long)buffer) seems like the better > option. > > Can you elaborate on why you feel __free_page() is a better choice? Well the allocation uses alloc_page and __free_page is the freeing counterpart so it is natural to use it if the allocated page is available. Which is the case here. Anyway the code can be cleaned up by using __get_free_page for the allocation, then you do not have to care about the struct page and get the address right away without an additional code. free_page would be a natural freeing path. __get_free_page would be even a better API because it enforces that the allocation is not from the highmem - which the driver already does by not using __GFP_HIGHMEM. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-17 7:19 ` Michal Hocko 0 siblings, 0 replies; 22+ messages in thread From: Michal Hocko @ 2015-06-17 7:19 UTC (permalink / raw) To: Darren Hart Cc: Pali Rohár, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Tue 16-06-15 20:43:34, Darren Hart wrote: [...] > Michal - thanks for the context. > > I'm surprised by your recommendation to use __free_page() out here in platform > driver land. > > I'd also prefer that the driver consistently free the same address to avoid > confusion. > > For these reasons, free_page((unsigned long)buffer) seems like the better > option. > > Can you elaborate on why you feel __free_page() is a better choice? Well the allocation uses alloc_page and __free_page is the freeing counterpart so it is natural to use it if the allocated page is available. Which is the case here. Anyway the code can be cleaned up by using __get_free_page for the allocation, then you do not have to care about the struct page and get the address right away without an additional code. free_page would be a natural freeing path. __get_free_page would be even a better API because it enforces that the allocation is not from the highmem - which the driver already does by not using __GFP_HIGHMEM. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? 2015-06-17 7:19 ` Michal Hocko @ 2015-06-18 21:14 ` Darren Hart -1 siblings, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-18 21:14 UTC (permalink / raw) To: Michal Hocko Cc: Pali Rohár, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Wed, Jun 17, 2015 at 09:19:39AM +0200, Michal Hocko wrote: > On Tue 16-06-15 20:43:34, Darren Hart wrote: > [...] > > Michal - thanks for the context. > > > > I'm surprised by your recommendation to use __free_page() out here in platform > > driver land. > > > > I'd also prefer that the driver consistently free the same address to avoid > > confusion. > > > > For these reasons, free_page((unsigned long)buffer) seems like the better > > option. > > > > Can you elaborate on why you feel __free_page() is a better choice? > > Well the allocation uses alloc_page and __free_page is the freeing > counterpart so it is natural to use it if the allocated page is > available. Which is the case here. > > Anyway the code can be cleaned up by using __get_free_page for the > allocation, then you do not have to care about the struct page and get > the address right away without an additional code. free_page would be a > natural freeing path. > __get_free_page would be even a better API because it enforces that > the allocation is not from the highmem - which the driver already does > by not using __GFP_HIGHMEM. > Thank you Michal, I guess I'm just tripping over an API with mismatched __ and no __ prefix paired calls. Thanks for the clarification. Pali, I'm fine with any of these options - it sounds as though __get_free_page() may be a general improvement. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Possible broken MM code in dell-laptop.c? @ 2015-06-18 21:14 ` Darren Hart 0 siblings, 0 replies; 22+ messages in thread From: Darren Hart @ 2015-06-18 21:14 UTC (permalink / raw) To: Michal Hocko Cc: Pali Rohár, Hans de Goede, Ben Skeggs, Stuart Hayes, Matthew Garrett, Andrew Morton, platform-driver-x86, linux-mm, linux-kernel On Wed, Jun 17, 2015 at 09:19:39AM +0200, Michal Hocko wrote: > On Tue 16-06-15 20:43:34, Darren Hart wrote: > [...] > > Michal - thanks for the context. > > > > I'm surprised by your recommendation to use __free_page() out here in platform > > driver land. > > > > I'd also prefer that the driver consistently free the same address to avoid > > confusion. > > > > For these reasons, free_page((unsigned long)buffer) seems like the better > > option. > > > > Can you elaborate on why you feel __free_page() is a better choice? > > Well the allocation uses alloc_page and __free_page is the freeing > counterpart so it is natural to use it if the allocated page is > available. Which is the case here. > > Anyway the code can be cleaned up by using __get_free_page for the > allocation, then you do not have to care about the struct page and get > the address right away without an additional code. free_page would be a > natural freeing path. > __get_free_page would be even a better API because it enforces that > the allocation is not from the highmem - which the driver already does > by not using __GFP_HIGHMEM. > Thank you Michal, I guess I'm just tripping over an API with mismatched __ and no __ prefix paired calls. Thanks for the clarification. Pali, I'm fine with any of these options - it sounds as though __get_free_page() may be a general improvement. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-06-18 21:15 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-14 9:05 Possible broken MM code in dell-laptop.c? Pali Rohár 2015-06-15 20:36 ` Darren Hart 2015-06-15 20:36 ` Darren Hart 2015-06-15 20:42 ` Pali Rohár 2015-06-16 10:12 ` Pavel Machek 2015-06-16 10:12 ` Pavel Machek 2015-06-15 21:18 ` Michal Hocko 2015-06-15 21:18 ` Michal Hocko 2015-06-15 21:18 ` Michal Hocko 2015-06-15 21:27 ` Pali Rohár 2015-06-16 6:33 ` Michal Hocko 2015-06-16 6:33 ` Michal Hocko 2015-06-16 7:15 ` Pali Rohár 2015-06-16 7:15 ` Pali Rohár 2015-06-16 7:43 ` Michal Hocko 2015-06-16 7:43 ` Michal Hocko 2015-06-17 3:43 ` Darren Hart 2015-06-17 3:43 ` Darren Hart 2015-06-17 7:19 ` Michal Hocko 2015-06-17 7:19 ` Michal Hocko 2015-06-18 21:14 ` Darren Hart 2015-06-18 21:14 ` Darren Hart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.