From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pali =?utf-8?q?Roh=C3=A1r?= Subject: Possible broken MM code in dell-laptop.c? Date: Sun, 14 Jun 2015 11:05:07 +0200 Message-ID: <201506141105.07171@pali> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4151917.2Ma5RsCSLe"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Return-path: Sender: owner-linux-mm@kvack.org To: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko Cc: platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org List-Id: platform-driver-x86.vger.kernel.org --nextPart4151917.2Ma5RsCSLe Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, in drivers/platform/x86/dell-laptop.c is this part of code: static int __init dell_init(void) { =2E.. /* * Allocate buffer below 4GB for SMI data--only 32-bit physical addr * is passed to SMI handler. */ bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); if (!bufferpage) { ret =3D -ENOMEM; goto fail_buffer; } buffer =3D page_address(bufferpage); ret =3D dell_setup_rfkill(); if (ret) { pr_warn("Unable to setup rfkill\n"); goto fail_rfkill; } =2E.. fail_rfkill: free_page((unsigned long)bufferpage); fail_buffer: =2E.. } Then there is another part: static void __exit dell_exit(void) { =2E.. free_page((unsigned long)buffer); } I suspect that there is some problem with free_page() call. In dell_init=20 is called free_page() on bufferpage and in dell_exit() on buffer. Matthew and Stuart, you introduced this inconsistency in commit: =2D------------------------------------------------ commit 116ee77b2858d9c89c0327f3a47c8ba864bf4a96 Author: Stuart Hayes Committer: Matthew Garrett 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=20 the buffer address is higher than 4GB, the address is truncated to 32=20 bits and the SMI handler writes to the wrong memory address. =20 Signed-off-by: Stuart Hayes Acked-by: Matthew Garrett =2D------------------------------------------------ Can you or somebody else (CCed linux-mm) look at this page related code?=20 I think it is wrong, but somebody authoritative should provide answer. Thanks. =2D-=20 Pali Roh=C3=A1r pali.rohar@gmail.com --nextPart4151917.2Ma5RsCSLe Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEABECAAYFAlV9Q8MACgkQi/DJPQPkQ1J0UACdHR0APSrYqgL+780YeaHg5762 rN4AoKgqmDBEOE0OFNDra9d8ZG+anJ5z =cav6 -----END PGP SIGNATURE----- --nextPart4151917.2Ma5RsCSLe-- -- 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 From: Darren Hart Subject: Re: Possible broken MM code in dell-laptop.c? Date: Mon, 15 Jun 2015 13:36:45 -0700 Message-ID: <20150615203645.GD83198@vmdeb7> References: <201506141105.07171@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from bombadil.infradead.org ([198.137.202.9]:32876 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752160AbbFOUgw (ORCPT ); Mon, 15 Jun 2015 16:36:52 -0400 Content-Disposition: inline In-Reply-To: <201506141105.07171@pali> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Roh=E1r wrote: > Hello, >=20 > in drivers/platform/x86/dell-laptop.c is this part of code: >=20 > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > if (!bufferpage) { > ret =3D -ENOMEM; > goto fail_buffer; > } > buffer =3D page_address(bufferpage); >=20 > ret =3D dell_setup_rfkill(); >=20 > if (ret) { > pr_warn("Unable to setup rfkill\n"); > goto fail_rfkill; > } > ... > fail_rfkill: > free_page((unsigned long)bufferpage); > fail_buffer: > ... > } >=20 > Then there is another part: >=20 > static void __exit dell_exit(void) > { > ... > free_page((unsigned long)buffer); I believe you are correct, and this should be bufferpage. Have you obse= rved any failures? --=20 Darren Hart Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pali =?utf-8?q?Roh=C3=A1r?= Subject: Re: Possible broken MM code in dell-laptop.c? Date: Mon, 15 Jun 2015 22:42:30 +0200 Message-ID: <201506152242.30732@pali> References: <201506141105.07171@pali> <20150615203645.GD83198@vmdeb7> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2946010.OgYTOazmK5"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f172.google.com ([209.85.212.172]:35421 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751909AbbFOUme (ORCPT ); Mon, 15 Jun 2015 16:42:34 -0400 In-Reply-To: <20150615203645.GD83198@vmdeb7> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Darren Hart Cc: Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Pavel Machek --nextPart2946010.OgYTOazmK5 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Monday 15 June 2015 22:36:45 Darren Hart wrote: > On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Roh=C3=A1r wrote: > > Hello, > >=20 > > in drivers/platform/x86/dell-laptop.c is this part of code: > >=20 > > static int __init dell_init(void) > > { > > ... > >=20 > > /* > > =09 > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > addr * is passed to SMI handler. > > */ > > =09 > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > > if (!bufferpage) { > > =09 > > ret =3D -ENOMEM; > > goto fail_buffer; > > =09 > > } > > buffer =3D page_address(bufferpage); > > =09 > > ret =3D dell_setup_rfkill(); > > =09 > > if (ret) { > > =09 > > pr_warn("Unable to setup rfkill\n"); > > goto fail_rfkill; > > =09 > > } > >=20 > > ... > >=20 > > fail_rfkill: > > free_page((unsigned long)bufferpage); > >=20 > > fail_buffer: > > ... > > } > >=20 > > Then there is another part: > >=20 > > static void __exit dell_exit(void) > > { > > ... > >=20 > > free_page((unsigned long)buffer); >=20 > 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=20 that buffer (and not bufferpage) should be passed to free_page(). So in=20 my opinion problem is at fail_rfkill: label and not in dell_exit(). But somebody from linux-mm should look at it... =2D-=20 Pali Roh=C3=A1r pali.rohar@gmail.com --nextPart2946010.OgYTOazmK5 Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEABECAAYFAlV/OLYACgkQi/DJPQPkQ1Jk0ACeIGd6oijZXI+02KCAq3JSNibE TXQAn2qfELLHG8KWLj2yFeTv/L/aR46T =FGp/ -----END PGP SIGNATURE----- --nextPart2946010.OgYTOazmK5-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Date: Mon, 15 Jun 2015 23:18:16 +0200 Message-ID: <20150615211816.GC16138@dhcp22.suse.cz> References: <201506141105.07171@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <201506141105.07171@pali> Sender: owner-linux-mm@kvack.org To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org List-Id: platform-driver-x86.vger.kernel.org On Sun 14-06-15 11:05:07, Pali Roh=E1r wrote: > Hello, >=20 > in drivers/platform/x86/dell-laptop.c is this part of code: >=20 > static int __init dell_init(void) > { > ... > /* > * Allocate buffer below 4GB for SMI data--only 32-bit physical addr > * is passed to SMI handler. > */ > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); [...] > buffer =3D 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); --=20 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pali =?utf-8?q?Roh=C3=A1r?= Subject: Re: Possible broken MM code in dell-laptop.c? Date: Mon, 15 Jun 2015 23:27:59 +0200 Message-ID: <201506152327.59907@pali> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1577752.QXBNijviWD"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f182.google.com ([209.85.212.182]:34561 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751933AbbFOV2D (ORCPT ); Mon, 15 Jun 2015 17:28:03 -0400 In-Reply-To: <20150615211816.GC16138@dhcp22.suse.cz> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Michal Hocko Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org --nextPart1577752.QXBNijviWD Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > On Sun 14-06-15 11:05:07, Pali Roh=C3=A1r wrote: > > Hello, > >=20 > > in drivers/platform/x86/dell-laptop.c is this part of code: > >=20 > > static int __init dell_init(void) > > { > > ... > >=20 > > /* > > =09 > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > addr * is passed to SMI handler. > > */ > > =09 > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); >=20 > [...] >=20 > > buffer =3D page_address(bufferpage); >=20 > [...] >=20 > > fail_rfkill: > > free_page((unsigned long)bufferpage); >=20 > 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. >=20 > I have no idea why the API has been done this way and yeah, it is > really confusing. >=20 > [...] >=20 > > static void __exit dell_exit(void) > > { > > ... > >=20 > > free_page((unsigned long)buffer); So both, either: free_page((unsigned long)buffer); or __free_page(bufferpage); is correct? =2D-=20 Pali Roh=C3=A1r pali.rohar@gmail.com --nextPart1577752.QXBNijviWD Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEABECAAYFAlV/Q18ACgkQi/DJPQPkQ1KMpgCfckKUQ53inI40AC4Hf7EmetC3 w8gAoJhyPBe99vFd5nAPblRsZqtlK56y =kc+d -----END PGP SIGNATURE----- --nextPart1577752.QXBNijviWD-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Date: Tue, 16 Jun 2015 08:33:46 +0200 Message-ID: <20150616063346.GA24296@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from cantor2.suse.de ([195.135.220.15]:40504 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751486AbbFPGdw (ORCPT ); Tue, 16 Jun 2015 02:33:52 -0400 Content-Disposition: inline In-Reply-To: <201506152327.59907@pali> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Mon 15-06-15 23:27:59, Pali Roh=E1r wrote: > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > On Sun 14-06-15 11:05:07, Pali Roh=E1r wrote: > > > Hello, > > >=20 > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > >=20 > > > static int __init dell_init(void) > > > { > > > ... > > >=20 > > > /* > > > =09 > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > =09 > > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > >=20 > > [...] > >=20 > > > buffer =3D page_address(bufferpage); > >=20 > > [...] > >=20 > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > >=20 > > 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 t= o > > a struct page. > >=20 > > I have no idea why the API has been done this way and yeah, it is > > really confusing. > >=20 > > [...] > >=20 > > > static void __exit dell_exit(void) > > > { > > > ... > > >=20 > > > free_page((unsigned long)buffer); >=20 > So both, either: >=20 > free_page((unsigned long)buffer); >=20 > or >=20 > __free_page(bufferpage); >=20 > is correct? Yes. Although I would use __free_page variant as both seem to be globally visible. --=20 Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pali =?utf-8?B?Um9ow6Fy?= Subject: Re: Possible broken MM code in dell-laptop.c? Date: Tue, 16 Jun 2015 09:15:23 +0200 Message-ID: <20150616071523.GB5863@pali> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wg0-f54.google.com ([74.125.82.54]:33086 "EHLO mail-wg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751818AbbFPHP1 (ORCPT ); Tue, 16 Jun 2015 03:15:27 -0400 Content-Disposition: inline In-Reply-To: <20150616063346.GA24296@dhcp22.suse.cz> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Michal Hocko , Darren Hart Cc: Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > On Mon 15-06-15 23:27:59, Pali Roh=C3=A1r wrote: > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > On Sun 14-06-15 11:05:07, Pali Roh=C3=A1r wrote: > > > > Hello, > > > >=20 > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > >=20 > > > > static int __init dell_init(void) > > > > { > > > > ... > > > >=20 > > > > /* > > > > =09 > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physica= l > > > > addr * is passed to SMI handler. > > > > */ > > > > =09 > > > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > > >=20 > > > [...] > > >=20 > > > > buffer =3D page_address(bufferpage); > > >=20 > > > [...] > > >=20 > > > > fail_rfkill: > > > > free_page((unsigned long)bufferpage); > > >=20 > > > This one should be __free_page because it consumes struct page* a= nd > > > 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. > > >=20 > > > I have no idea why the API has been done this way and yeah, it is > > > really confusing. > > >=20 > > > [...] > > >=20 > > > > static void __exit dell_exit(void) > > > > { > > > > ... > > > >=20 > > > > free_page((unsigned long)buffer); > >=20 > > So both, either: > >=20 > > free_page((unsigned long)buffer); > >=20 > > or > >=20 > > __free_page(bufferpage); > >=20 > > is correct? >=20 > Yes. Although I would use __free_page variant as both seem to be > globally visible. >=20 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) --=20 Pali Roh=C3=A1r pali.rohar@gmail.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Date: Tue, 16 Jun 2015 09:43:09 +0200 Message-ID: <20150616074308.GB24296@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from cantor2.suse.de ([195.135.220.15]:43683 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751989AbbFPHnS (ORCPT ); Tue, 16 Jun 2015 03:43:18 -0400 Content-Disposition: inline In-Reply-To: <20150616071523.GB5863@pali> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Darren Hart , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Tue 16-06-15 09:15:23, Pali Roh=E1r wrote: [...] > Michal, thank you for explaining this situation! >=20 > Darren, I will prepare patch which will fix code and use __free_page(= ). >=20 > (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). --=20 Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Machek Subject: Re: Possible broken MM code in dell-laptop.c? Date: Tue, 16 Jun 2015 12:12:12 +0200 Message-ID: <20150616101211.GA25899@amd> References: <201506141105.07171@pali> <20150615203645.GD83198@vmdeb7> <201506152242.30732@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:50824 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756497AbbFPKMP (ORCPT ); Tue, 16 Jun 2015 06:12:15 -0400 Content-Disposition: inline In-Reply-To: <201506152242.30732@pali> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Darren Hart , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Mon 2015-06-15 22:42:30, Pali Roh=E1r wrote: > On Monday 15 June 2015 22:36:45 Darren Hart wrote: > > On Sun, Jun 14, 2015 at 11:05:07AM +0200, Pali Roh=E1r wrote: > > > Hello, > > >=20 > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > >=20 > > > static int __init dell_init(void) > > > { > > > ... > > >=20 > > > /* > > > =09 > > > * Allocate buffer below 4GB for SMI data--only 32-bit physical > > > addr * is passed to SMI handler. > > > */ > > > =09 > > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > > > if (!bufferpage) { > > > =09 > > > ret =3D -ENOMEM; > > > goto fail_buffer; > > > =09 > > > } > > > buffer =3D page_address(bufferpage); > > > =09 > > > ret =3D dell_setup_rfkill(); > > > =09 > > > if (ret) { > > > =09 > > > pr_warn("Unable to setup rfkill\n"); > > > goto fail_rfkill; > > > =09 > > > } > > >=20 > > > ... > > >=20 > > > fail_rfkill: > > > free_page((unsigned long)bufferpage); > > >=20 > > > fail_buffer: > > > ... > > > } > > >=20 > > > Then there is another part: > > >=20 > > > static void __exit dell_exit(void) > > > { > > > ... > > >=20 > > > free_page((unsigned long)buffer); > >=20 > > I believe you are correct, and this should be bufferpage. Have you > > observed any failures? >=20 > Rmmoding dell-laptop.ko works fine. There is no error in dmesg. I thi= nk=20 > that buffer (and not bufferpage) should be passed to free_page(). So = in=20 > 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 --=20 (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses= /blog.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Darren Hart Subject: Re: Possible broken MM code in dell-laptop.c? Date: Tue, 16 Jun 2015 20:43:34 -0700 Message-ID: <20150617034334.GB29788@vmdeb7> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from bombadil.infradead.org ([198.137.202.9]:57481 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751095AbbFQDnr (ORCPT ); Tue, 16 Jun 2015 23:43:47 -0400 Content-Disposition: inline In-Reply-To: <20150616071523.GB5863@pali> Sender: platform-driver-x86-owner@vger.kernel.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Michal Hocko , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On Tue, Jun 16, 2015 at 09:15:23AM +0200, Pali Roh=E1r wrote: > On Tuesday 16 June 2015 08:33:46 Michal Hocko wrote: > > On Mon 15-06-15 23:27:59, Pali Roh=E1r wrote: > > > On Monday 15 June 2015 23:18:16 Michal Hocko wrote: > > > > On Sun 14-06-15 11:05:07, Pali Roh=E1r wrote: > > > > > Hello, > > > > >=20 > > > > > in drivers/platform/x86/dell-laptop.c is this part of code: > > > > >=20 > > > > > static int __init dell_init(void) > > > > > { > > > > > ... > > > > >=20 > > > > > /* > > > > > =09 > > > > > * Allocate buffer below 4GB for SMI data--only 32-bit physi= cal > > > > > addr * is passed to SMI handler. > > > > > */ > > > > > =09 > > > > > bufferpage =3D alloc_page(GFP_KERNEL | GFP_DMA32); > > > >=20 > > > > [...] > > > >=20 > > > > > buffer =3D page_address(bufferpage); > > > >=20 > > > > [...] > > > >=20 > > > > > fail_rfkill: > > > > > free_page((unsigned long)bufferpage); > > > >=20 > > > > This one should be __free_page because it consumes struct page*= and > > > > it is the proper counter part for alloc_page. free_page, just t= o > > > > make it confusing, consumes an address which has to be translat= ed to > > > > a struct page. > > > >=20 > > > > I have no idea why the API has been done this way and yeah, it = is > > > > really confusing. > > > >=20 > > > > [...] > > > >=20 > > > > > static void __exit dell_exit(void) > > > > > { > > > > > ... > > > > >=20 > > > > > free_page((unsigned long)buffer); > > >=20 > > > So both, either: > > >=20 > > > free_page((unsigned long)buffer); > > >=20 > > > or > > >=20 > > > __free_page(bufferpage); > > >=20 > > > is correct? > >=20 > > Yes. Although I would use __free_page variant as both seem to be > > globally visible. > >=20 Michal - thanks for the context. I'm surprised by your recommendation to use __free_page() out here in p= latform driver land. I'd also prefer that the driver consistently free the same address to a= void confusion. =46or these reasons, free_page((unsigned long)buffer) seems like the be= tter option. Can you elaborate on why you feel __free_page() is a better choice? --=20 Darren Hart Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Date: Wed, 17 Jun 2015 09:19:39 +0200 Message-ID: <20150617071939.GA25056@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> <20150617034334.GB29788@vmdeb7> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20150617034334.GB29788@vmdeb7> Sender: linux-kernel-owner@vger.kernel.org To: Darren Hart Cc: Pali =?iso-8859-1?Q?Roh=E1r?= , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org List-Id: platform-driver-x86.vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Darren Hart Subject: Re: Possible broken MM code in dell-laptop.c? Date: Thu, 18 Jun 2015 14:14:46 -0700 Message-ID: <20150618211446.GB70097@vmdeb7> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> <20150617034334.GB29788@vmdeb7> <20150617071939.GA25056@dhcp22.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20150617071939.GA25056@dhcp22.suse.cz> Sender: owner-linux-mm@kvack.org To: Michal Hocko Cc: Pali =?iso-8859-1?Q?Roh=E1r?= , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org List-Id: platform-driver-x86.vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f48.google.com (mail-pa0-f48.google.com [209.85.220.48]) by kanga.kvack.org (Postfix) with ESMTP id B1AF56B0038 for ; Mon, 15 Jun 2015 16:36:53 -0400 (EDT) Received: by pacgb13 with SMTP id gb13so42171814pac.1 for ; Mon, 15 Jun 2015 13:36:53 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id kp7si19347728pbc.76.2015.06.15.13.36.52 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 15 Jun 2015 13:36:52 -0700 (PDT) Date: Mon, 15 Jun 2015 13:36:45 -0700 From: Darren Hart Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150615203645.GD83198@vmdeb7> References: <201506141105.07171@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201506141105.07171@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by kanga.kvack.org (Postfix) with ESMTP id 7AC246B0038 for ; Mon, 15 Jun 2015 17:18:21 -0400 (EDT) Received: by wifx6 with SMTP id x6so964022wif.0 for ; Mon, 15 Jun 2015 14:18:20 -0700 (PDT) Received: from mail-wg0-x22e.google.com (mail-wg0-x22e.google.com. [2a00:1450:400c:c00::22e]) by mx.google.com with ESMTPS id u7si20320619wiw.120.2015.06.15.14.18.19 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 15 Jun 2015 14:18:19 -0700 (PDT) Received: by wgv5 with SMTP id 5so78647602wgv.1 for ; Mon, 15 Jun 2015 14:18:19 -0700 (PDT) Date: Mon, 15 Jun 2015 23:18:16 +0200 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150615211816.GC16138@dhcp22.suse.cz> References: <201506141105.07171@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201506141105.07171@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f175.google.com (mail-wi0-f175.google.com [209.85.212.175]) by kanga.kvack.org (Postfix) with ESMTP id 61C4A6B0038 for ; Tue, 16 Jun 2015 02:33:53 -0400 (EDT) Received: by wigg3 with SMTP id g3so98711458wig.1 for ; Mon, 15 Jun 2015 23:33:52 -0700 (PDT) Received: from mx2.suse.de (cantor2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id o6si22324645wiy.112.2015.06.15.23.33.51 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 15 Jun 2015 23:33:51 -0700 (PDT) Date: Tue, 16 Jun 2015 08:33:46 +0200 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150616063346.GA24296@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201506152327.59907@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lb0-f176.google.com (mail-lb0-f176.google.com [209.85.217.176]) by kanga.kvack.org (Postfix) with ESMTP id 6E2E16B0038 for ; Tue, 16 Jun 2015 03:15:28 -0400 (EDT) Received: by lbbqq2 with SMTP id qq2so4818893lbb.3 for ; Tue, 16 Jun 2015 00:15:27 -0700 (PDT) Received: from mail-wi0-x230.google.com (mail-wi0-x230.google.com. [2a00:1450:400c:c05::230]) by mx.google.com with ESMTPS id bu10si247351wjc.55.2015.06.16.00.15.26 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Jun 2015 00:15:26 -0700 (PDT) Received: by wigg3 with SMTP id g3so99575784wig.1 for ; Tue, 16 Jun 2015 00:15:25 -0700 (PDT) Date: Tue, 16 Jun 2015 09:15:23 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150616071523.GB5863@pali> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150616063346.GA24296@dhcp22.suse.cz> Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko , Darren Hart Cc: Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f178.google.com (mail-wi0-f178.google.com [209.85.212.178]) by kanga.kvack.org (Postfix) with ESMTP id 4ACAE6B0038 for ; Tue, 16 Jun 2015 03:43:20 -0400 (EDT) Received: by wigg3 with SMTP id g3so100212731wig.1 for ; Tue, 16 Jun 2015 00:43:19 -0700 (PDT) Received: from mx2.suse.de (cantor2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id es2si1623674wib.12.2015.06.16.00.43.18 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 16 Jun 2015 00:43:18 -0700 (PDT) Date: Tue, 16 Jun 2015 09:43:09 +0200 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150616074308.GB24296@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150616071523.GB5863@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Darren Hart , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-la0-f54.google.com (mail-la0-f54.google.com [209.85.215.54]) by kanga.kvack.org (Postfix) with ESMTP id 32AB06B0038 for ; Tue, 16 Jun 2015 06:12:16 -0400 (EDT) Received: by laka10 with SMTP id a10so7834699lak.0 for ; Tue, 16 Jun 2015 03:12:15 -0700 (PDT) Received: from atrey.karlin.mff.cuni.cz (atrey.karlin.mff.cuni.cz. [195.113.26.193]) by mx.google.com with ESMTP id d3si933104wjr.121.2015.06.16.03.12.13 for ; Tue, 16 Jun 2015 03:12:14 -0700 (PDT) Date: Tue, 16 Jun 2015 12:12:12 +0200 From: Pavel Machek Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150616101211.GA25899@amd> References: <201506141105.07171@pali> <20150615203645.GD83198@vmdeb7> <201506152242.30732@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201506152242.30732@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Darren Hart , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , Michal Hocko , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f172.google.com (mail-pd0-f172.google.com [209.85.192.172]) by kanga.kvack.org (Postfix) with ESMTP id 9963A6B0032 for ; Tue, 16 Jun 2015 23:43:49 -0400 (EDT) Received: by pdbki1 with SMTP id ki1so28436370pdb.1 for ; Tue, 16 Jun 2015 20:43:49 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id hb1si4142523pbd.184.2015.06.16.20.43.47 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Jun 2015 20:43:47 -0700 (PDT) Date: Tue, 16 Jun 2015 20:43:34 -0700 From: Darren Hart Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150617034334.GB29788@vmdeb7> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150616071523.GB5863@pali> Sender: owner-linux-mm@kvack.org List-ID: To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Michal Hocko , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f173.google.com (mail-wi0-f173.google.com [209.85.212.173]) by kanga.kvack.org (Postfix) with ESMTP id D531E6B006E for ; Wed, 17 Jun 2015 03:19:46 -0400 (EDT) Received: by wicnd19 with SMTP id nd19so73357291wic.1 for ; Wed, 17 Jun 2015 00:19:46 -0700 (PDT) Received: from mx2.suse.de (cantor2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id gr2si6074092wjc.163.2015.06.17.00.19.44 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 17 Jun 2015 00:19:45 -0700 (PDT) Date: Wed, 17 Jun 2015 09:19:39 +0200 From: Michal Hocko Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150617071939.GA25056@dhcp22.suse.cz> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> <20150617034334.GB29788@vmdeb7> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150617034334.GB29788@vmdeb7> Sender: owner-linux-mm@kvack.org List-ID: To: Darren Hart Cc: Pali =?iso-8859-1?Q?Roh=E1r?= , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org 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: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756065AbbFOVSc (ORCPT ); Mon, 15 Jun 2015 17:18:32 -0400 Received: from mail-wi0-f177.google.com ([209.85.212.177]:34688 "EHLO mail-wi0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752714AbbFOVSU (ORCPT ); Mon, 15 Jun 2015 17:18:20 -0400 Date: Mon, 15 Jun 2015 23:18:16 +0200 From: Michal Hocko To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Hans de Goede , Darren Hart , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150615211816.GC16138@dhcp22.suse.cz> References: <201506141105.07171@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <201506141105.07171@pali> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753122AbbFRVPQ (ORCPT ); Thu, 18 Jun 2015 17:15:16 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:35882 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752264AbbFRVOx (ORCPT ); Thu, 18 Jun 2015 17:14:53 -0400 Date: Thu, 18 Jun 2015 14:14:46 -0700 From: Darren Hart To: Michal Hocko Cc: Pali =?iso-8859-1?Q?Roh=E1r?= , Hans de Goede , Ben Skeggs , Stuart Hayes , Matthew Garrett , Andrew Morton , platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: Possible broken MM code in dell-laptop.c? Message-ID: <20150618211446.GB70097@vmdeb7> References: <201506141105.07171@pali> <20150615211816.GC16138@dhcp22.suse.cz> <201506152327.59907@pali> <20150616063346.GA24296@dhcp22.suse.cz> <20150616071523.GB5863@pali> <20150617034334.GB29788@vmdeb7> <20150617071939.GA25056@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150617071939.GA25056@dhcp22.suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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