Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

> On 11/26/2012 09:35 AM, Andrzej Pietrasiewicz wrote:
>> In some parts of the kernel (e.g. planned configfs integration into usb
>> gadget) there is a need to programmatically create config groups
>> (directories) but it would be preferable to disallow creating them by
>> the user. This is more or less what default_groups used to be for.
>> But e.g. in the mass storage gadget, after storing the number of
>> luns (logical units) into some configfs attribute, the corresponding lun#
>> directories should be created, their number is not known up front so
>> default_groups are no good for this.
>>
>> Example:
>>
>> $ echo 3>  /cfg//mass_storage/luns
>>
>> causes
>>
>> /cfg/../mass_storage/lun0
>> /cfg/../mass_storage/lun1
>> /cfg/../mass_storage/lun2

On Mon, Nov 26 2012, Sebastian Andrzej Siewior wrote:
> I though we did not want the luns file but instead use
>
> mkdir /cfg/../mass_storage/lun0
> mkdir /cfg/../mass_storage/lun1
>
> directly.

I think that's suboptimal, and we can do better.  There are too many
corner cases (ie. what if user does “mkdir lun7” without the previous
luns?), it adds complexity to the kernel (ie. parsing of the name), and
the thing is overly complicated for user (“echo 5 > nluns” is much nicer
than having to create 5 directories).

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpbwO2ESPE55.pgp
Description: PGP signature

Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

On Mon, Nov 26 2012, Sebastian Andrzej Siewior wrote:
> Wouldn't say that. It may adds complexity on another level. The target
> subsystem has the same problem with adding luns and there seems nothing
> wrong with having lun3 and 4 and leaving 0 and 1 unsued.

That's not what Wikipedia claims though (from
):

LUN 0: There is one LUN which is required to exist in every
target: zero. The logical unit with LUN zero is special in that
it must implement a few specific commands, most notably Report
LUNs, which is how an initiator can find out all the other LUNs
in the target. But LUN zero need not provide any other services,
such as a storage volume.

That's why I proposed solution where one needs to have continuous
numbering of LUNs.  I'm not an expert on SCSI though.

> With the tcm gadget I get:
>
> |scsi 0:0:0:2: Direct-Access LIO-ORG  RAMDISK-MCP  4.0  PQ: 0 
> ANSI: 5
> |scsi 0:0:0:3: Direct-Access LIO-ORG  FILEIO   4.0  PQ: 0 
> ANSI: 5
>
> You notice :2 and :3 instead :0 and :1. While should be there something
> wrong with this?

It may be that it works on Linux but fails on some other systems (or
even older Linux kernels).  Like I've said, I'm not SCSI expert, so my
knowledge of it is (embarrassingly) minimal.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpGFqOFT9Awy.pgp
Description: PGP signature

RE: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

On Tue, Nov 27 2012, Andrzej Pietrasiewicz  wrote:
> I think we _still_ need a way to programmatically create/remove configfs
> directories. Without it, this: "After name is written it will request
> the module and special configuration related files pop up."
> (http://www.spinics.net/lists/linux-usb/msg75118.html)

I agree, and this is orthogonal to exact API used by configfs gadget.

I may be missing something about configfs, but the way I see it, lack of
such functions is a mistake in the configfs API.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpi644TjVx56.pgp
Description: PGP signature

Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

On Tue, Nov 27 2012, Sebastian Andrzej Siewior wrote:
> I don't want to push python on anyone but the removal magic is simply
> straight forward: unlink the disk ports, rmdir luns, tpgt,…

How should a generic tool know what kind of actions are needed for given
function to be removed?  If you ask me, there should be a way to unbind
gadget and unload all modules without any specific knowledge of the
functions.  If there is no such mechanism, then it's a bad user
interface.

> I understand the need for things that pop later like interfaceXX but
> couldn't the user manually create them if he needs them?

I think the question is of information flow direction.  If user gives
some information to the kernel, she should be the one creating any
necessary directories.  But if the information comes from kernel to the
user, the kernel should create the structure.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpDHfGOEwJxW.pgp
Description: PGP signature

Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

> On 11/27/2012 05:23 PM, Michal Nazarewicz wrote:
>> How should a generic tool know what kind of actions are needed for given
>> function to be removed?  If you ask me, there should be a way to unbind
>> gadget and unload all modules without any specific knowledge of the
>> functions.  If there is no such mechanism, then it's a bad user
>> interface.

On Wed, Nov 28 2012, Sebastian Andrzej Siewior wrote:
> Well. You need only to remove the directories you created.

My point is that there should be a way to write a script that is unaware
of the way function is configured, ie. which directories were created
and which were not.

Besides, if you rmdir lun0, is the function still supposed to work with
all LUNs present?  In my opinion, while gadget is bound, it should not
be possible to modify such things.

> An unbind would be  simply an unlink of the gadget  which is linked to
> the udc.   All configurations  remain so  you can link  it at  a later
> point without touching the configuration because it is as it was.

Yes, but that's not my concern.  My concern is that I should be able to
put a relatively simple code in my shutdown script (or whatever) which
unbinds all gadgets, without knowing what kind of functions are used.

And I'm proposing that this could be done by allowing user to just do:

cd /cfs/...
rmdir gadgets/* # unbind and remove all gadgets
rmdir functions/*/* # unbind and remove all function instances
rmdir functions/*   # unload all functions
rmdir udcs/*# unload all UDCs

>> I think the question is of information flow direction.  If user gives
>> some information to the kernel, she should be the one creating any
>> necessary directories.  But if the information comes from kernel to the
>> user, the kernel should create the structure.

> Yes that is a point. But the "name" can go away if we use it in the
> directory name. That is what other configfs user do. The same is true
> for luns for instance. I just want to avoid adding features because we
> do something different compared to every other configfs user.

You've lost me here.  What are we talking about again?  What “name” are
you referring to?

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpd4Yhe2VOdP.pgp
Description: PGP signature

Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

On Wed, Nov 28 2012, Sebastian Andrzej Siewior wrote:
> -
> /functions/acm-function/
>
> instead of
> 
> /functions/function1/
>   +name
> with attribute file named "name" which contains the name of the
> function (i.e. acm). My point is to keep "name" in the directory name
> instead having an extra attribute.

Right.  But as I've suggested:

mkdir functions/
mkdir functions//

which IMO is easier to handle in kernel (no parsing) and looks nicer in
user space.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpF2kwL3u0F9.pgp
Description: PGP signature

Re: [RFC PATCHv2 1/1] usb: f_rndis: Avoid to use ERROR macro if cdev can be null

[Michal Nazarewicz]

On Tue, Mar 19 2013, oskar.and...@sonymobile.com wrote:
> The udc_irq service runs the isr_tr_complete_handler which in turn
> "nukes" the endpoints, including a call to rndis_response_complete,
> if appropriate. If the rndis_msg_parser fails here, an error will
> be printed using a dev_err call (through the ERROR() macro).
>
> However, if the usb cable was just disconnected the device (cdev)
> might not be available and will be null. Since the dev_err macro will
> dereference the cdev pointer we get a null pointer exception.
>
> Reviewed-by: Radovan Lekanovic 
> Signed-off-by: Truls Bengtsson 
> Signed-off-by: Oskar Andero 

Acked-by: Michal Nazarewicz 

I think this is the best solution.  Adding if statements around it would
just add noise.

> ---
>  drivers/usb/gadget/f_rndis.c |3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_rndis.c b/drivers/usb/gadget/f_rndis.c
> index 71beeb8..cc9c49c 100644
> --- a/drivers/usb/gadget/f_rndis.c
> +++ b/drivers/usb/gadget/f_rndis.c
> @@ -447,14 +447,13 @@ static void rndis_response_complete(struct usb_ep *ep, 
> struct usb_request *req)
>  static void rndis_command_complete(struct usb_ep *ep, struct usb_request 
> *req)
>  {
>   struct f_rndis  *rndis = req->context;
> - struct usb_composite_dev*cdev = rndis->port.func.config->cdev;
>   int status;
>  
>   /* received RNDIS command from USB_CDC_SEND_ENCAPSULATED_COMMAND */
>  //   spin_lock(&dev->lock);
>   status = rndis_msg_parser(rndis->config, (u8 *) req->buf);
>   if (status < 0)
> - ERROR(cdev, "RNDIS command error %d, %d/%d\n",
> + pr_err("RNDIS command error %d, %d/%d\n",
>   status, req->actual, req->length);
>  //   spin_unlock(&dev->lock);
>  }

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpWVgo4rxuTQ.pgp
Description: PGP signature

Re: [PATCH] tools: usb: ffs-test: Fix build failure

[Michal Nazarewicz]

On Thu, Feb 21 2013, maxin.j...@gmail.com wrote:
> From: "Maxin B. John" 
>
> Fixes this build failure:
> gcc -Wall -Wextra -g -lpthread -I../include -o testusb testusb.c
> gcc -Wall -Wextra -g -lpthread -I../include -o ffs-test ffs-test.c
> In file included from ffs-test.c:41:0:
> ../../include/linux/usb/functionfs.h:4:39: fatal error:
> uapi/linux/usb/functionfs.h: No such file or directory
> compilation terminated.
> make: *** [ffs-test] Error 1
>
> Signed-off-by: Maxin B. John 

Acked-by: Michal Nazarewicz 

> ---
>  tools/usb/ffs-test.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
> index 8674b9e..fe1e66b 100644
> --- a/tools/usb/ffs-test.c
> +++ b/tools/usb/ffs-test.c
> @@ -38,7 +38,7 @@
>  #include 
>  #include 
>  
> -#include "../../include/linux/usb/functionfs.h"
> +#include "../../include/uapi/linux/usb/functionfs.h"
>  
>  
>  / Little Endian Handling 
> /
> -- 
> 1.7.7
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpbBCf_yr1UH.pgp
Description: PGP signature

Re: [PATCH] tools: usb: ffs-test: Fix build failure

[Michal Nazarewicz]

On Thu, Feb 21 2013, Maxin B. John wrote:
> Hi,
>
> On Thu, Feb 21, 2013 at 2:06 AM, Greg KH  wrote:
>> On Thu, Feb 21, 2013 at 01:57:51AM +0200, maxin.j...@gmail.com wrote:
>>> From: "Maxin B. John" 
>>>
>>> Fixes this build failure:
>>> gcc -Wall -Wextra -g -lpthread -I../include -o testusb testusb.c
>>> gcc -Wall -Wextra -g -lpthread -I../include -o ffs-test ffs-test.c
>>> In file included from ffs-test.c:41:0:
>>> ../../include/linux/usb/functionfs.h:4:39: fatal error:
>>> uapi/linux/usb/functionfs.h: No such file or directory
>>> compilation terminated.
>>> make: *** [ffs-test] Error 1
>>
>> This is a build failure where, 3.8, or linux-next, or somewhere else?
>
> It is in 3.8

This also happens in 3.7.  [commit
5e1ddb481776a487b15b40579a000b279ce527c9: UAPI: (Scripted) Disintegrate
include/linux/usb] is the culprit.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpbFJmia2nRt.pgp
Description: PGP signature

Re: [PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[Michal Nazarewicz]

On Thu, Feb 21 2013, John Stultz wrote:
> In many cases, documentation around composite drivers suggest
> setting the idVendor and other module params as follows:
>
> $ insmod g_ffs.ko idVendor= iSerialNumber=
>
> However, this won't work if the driver is not compiled in as a
> module, as the module_param permissions are S_IRUGO.
>
> Thus this patch changes the composite module_param permissions
> to S_IRUGO|S_IWUSR to allow the module_params to be set at
> runtime via /sys/modules//parameters/

If the driver is not compiled as a module, setting those variables won't
work anyway.  Or am I missing something?

You can, however, pass them on Linux command line (with some prefix
which I can never remember).

If you want to configure things at run-time without having to compile
stuff as modules, you need to wait for the configfs based gadgets.

> Cc: Sebastian Andrzej Siewior 
> Cc: Andrzej Pietrasiewicz 
> Cc: Michal Nazarewicz 
> Cc: Felipe Balbi 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: John Stultz 
> ---
>  include/linux/usb/composite.h |   16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index b09c37e..22b1b02 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -399,24 +399,28 @@ struct usb_composite_overwrite {
>  #define USB_GADGET_COMPOSITE_OPTIONS()   
> \
>   static struct usb_composite_overwrite coverwrite;   \
>   \
> - module_param_named(idVendor, coverwrite.idVendor, ushort, S_IRUGO); \
> + module_param_named(idVendor, coverwrite.idVendor, ushort,   \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(idVendor, "USB Vendor ID");\
>   \
> - module_param_named(idProduct, coverwrite.idProduct, ushort, S_IRUGO); \
> + module_param_named(idProduct, coverwrite.idProduct, ushort, \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(idProduct, "USB Product ID");  \
>   \
> - module_param_named(bcdDevice, coverwrite.bcdDevice, ushort, S_IRUGO); \
> + module_param_named(bcdDevice, coverwrite.bcdDevice, ushort, \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(bcdDevice, "USB Device version (BCD)");\
>   \
>   module_param_named(iSerialNumber, coverwrite.serial_number, charp, \
> - S_IRUGO); \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(iSerialNumber, "SerialNumber string"); \
>   \
>   module_param_named(iManufacturer, coverwrite.manufacturer, charp, \
> - S_IRUGO); \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(iManufacturer, "USB Manufacturer string"); \
>   \
> - module_param_named(iProduct, coverwrite.product, charp, S_IRUGO); \
> + module_param_named(iProduct, coverwrite.product, charp, \
> + S_IRUGO|S_IWUSR);   \
>   MODULE_PARM_DESC(iProduct, "USB Product string")
>  
>  void usb_composite_overwrite_options(struct usb_composite_dev *cdev,
> -- 
> 1.7.10.4
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgptX2XspqmJ0.pgp
Description: PGP signature

Re: [PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[Michal Nazarewicz]

> On 02/21/2013 02:52 PM, Michal Nazarewicz wrote:
>> If the driver is not compiled as a module, setting those variables
>> won't work anyway.  Or am I missing something?

On Fri, Feb 22 2013, John Stultz wrote:
> Huh. It worked in my testing. But maybe that's only the first time its 
> set? I'll play around with it some more, but yea, on further thought, 
> without unloading the module those values probably shouldn't change. 
> Sorry for the confusion on my part here.

If you are using g_ffs than yes, it's a bit different, since the module
is initialised only after the user space daemon is active, but for all
other gadgets, setting those won't work.

>> If you want to configure things at run-time without having to compile
>> stuff as modules, you need to wait for the configfs based gadgets.
>
> Heh. I thought I was just sorting things out between the out-of-tree 
> android composite, ccg in staging, and functionfs. And now there's 
> *another*?
>
> Any details on configfs based gadget? Is there a git tree somewhere?

There are patches from Andrzej Pietrasiewicz and Sebastian Andrzej
Siewior on the list.  I'm a bit lost in the fate of them and all the
namings.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpE1xGpxYing.pgp
Description: PGP signature

Re: [PATCH] mm: cma: Discard clean pages during contiguous allocation instead of migration

[Michal Nazarewicz]

On Tue, Sep 11 2012, Minchan Kim wrote:
> This patch drops clean cache pages instead of migration during
> alloc_contig_range() to minimise allocation latency by reducing the amount
> of migration is necessary. It's useful for CMA because latency of migration
> is more important than evicting the background processes working set.
> In addition, as pages are reclaimed then fewer free pages for migration
> targets are required so it avoids memory reclaiming to get free pages,
> which is a contributory factor to increased latency.
>
> * from v1
>   * drop migrate_mode_t
>   * add reclaim_clean_pages_from_list instad of MIGRATE_DISCARD support - Mel
>
> I measured elapsed time of __alloc_contig_migrate_range which migrates
> 10M in 40M movable zone in QEMU machine.
>
> Before - 146ms, After - 7ms
>
> Cc: Marek Szyprowski 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

Thanks!

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpyd0UvM47ra.pgp
Description: PGP signature

Re: [RFC] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

On Tue, Sep 11 2012, Minchan Kim wrote:
> I saw two bug report about MIGRATE_ISOALTE type and NR_FREE_PAGES accounting
> mistmatch problem until now and I think we can meet more problems
> in the near future without solving it.
>
> Maybe, [1] would be a solution but I really don't like to add new branch
> in hotpath, even MIGRATE_ISOLATE used very rarely.
> so, my patch is inpired. If there is another good idea that avoid
> new branch in hotpath and solve the fundamental problem, I will drop this
> patch.
>
> [1] http://permalink.gmane.org/gmane.linux.kernel.mm/85199.

Well...  I am still not convinced.

Than again you clearly have put some thought into it and, more
importantly, I'm not the one deciding whether this gets merged or
not, anyway. ;)

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpbShNQf7YBq.pgp
Description: PGP signature

Re: [PATCH 1/2] mm: refactor out __alloc_contig_migrate_alloc

[Michal Nazarewicz]

On Wed, Sep 12 2012, Minchan Kim  wrote:
> __alloc_contig_migrate_alloc can be used by memory-hotplug so
> refactor out(move + rename as a common name) it into
> page_isolation.c.
>
> Cc: Kamezawa Hiroyuki 
> Cc: Yasuaki Ishimatsu 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> Cc: Marek Szyprowski 
> Cc: Wen Congyang 
> Signed-off-by: Minchan Kim 

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpM7I3MD34Nl.pgp
Description: PGP signature

Re: [PATCH 2/2] memory-hotplug: don't replace lowmem pages with highmem

[Michal Nazarewicz]

On Wed, Sep 12 2012, Minchan Kim wrote:
> [1] reporeted that lowmem pages could be replaced by
> highmem pages during migration of CMA and fixed.
>
> Quote from [1]'s description
> "
> The filesystem layer expects pages in the block device's mapping to not
> be in highmem (the mapping's gfp mask is set in bdget()), but CMA can
> currently replace lowmem pages with highmem pages, leading to crashes in
> filesystem code such as the one below:
>
>   Unable to handle kernel NULL pointer dereference at virtual address 
> 0400
>   pgd = c0c98000
>   [0400] *pgd=00c91831, *pte=, *ppte=
>   Internal error: Oops: 817 [#1] PREEMPT SMP ARM
>   CPU: 0Not tainted  (3.5.0-rc5+ #80)
>   PC is at __memzero+0x24/0x80
>   ...
>   Process fsstress (pid: 323, stack limit = 0xc0cbc2f0)
>   Backtrace:
>   [] (ext4_getblk+0x0/0x180) from [] 
> (ext4_bread+0x1c/0x98)
>   [] (ext4_bread+0x0/0x98) from [] 
> (ext4_mkdir+0x160/0x3bc)
>r4:c15337f0
>   [] (ext4_mkdir+0x0/0x3bc) from [] 
> (vfs_mkdir+0x8c/0x98)
>   [] (vfs_mkdir+0x0/0x98) from [] 
> (sys_mkdirat+0x74/0xac)
>r6: r5:c152eb40 r4:01ff r3:c14b43f0
>   [] (sys_mkdirat+0x0/0xac) from [] 
> (sys_mkdir+0x20/0x24)
>r6:beccdcf0 r5:00074000 r4:beccdbbc
>   [] (sys_mkdir+0x0/0x24) from [] 
> (ret_fast_syscall+0x0/0x30)
> "
>
> Memory-hotplug has same problem with CMA so [1]'s fix could be applied
> with memory-hotplug, too.
>
> Fix it by reusing.
>
> [1] 6a6dccba2, mm: cma: don't replace lowmem pages with highmem
>
> Cc: Kamezawa Hiroyuki 
> Cc: Yasuaki Ishimatsu 
> Cc: Michal Nazarewicz 

Looks legit:

Acked-by: Michal Nazarewicz 

I also like how both of the patches cumulatively have negative delta. :]

> Cc: Marek Szyprowski 
> Cc: Wen Congyang 
> Signed-off-by: Minchan Kim 

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpFHM25psJe2.pgp
Description: PGP signature

Re: [PATCH] usb/gadget: let file storage gadget select libcomposite

[Michal Nazarewicz]

On Wed, Oct 24 2012, Sebastian Andrzej Siewior wrote:
> From: Sebastian Andrzej Siewior 
> Date: Wed, 24 Oct 2012 16:40:10 +0200
>
> found by randconfig, Randy Dunlap and Stephen Rothwell:
>
> |drivers/built-in.o: In function `fsg_setup':
> |file_storage.c:(.text+0x24db7c): undefined reference to 
> `usb_gadget_config_buf'
> |file_storage.c:(.text+0x24db98): undefined reference to 
> `usb_gadget_get_string'
> |drivers/built-in.o: In function `fsg_bind':
> |file_storage.c:(.init.text+0xee42): undefined reference to 
> `usb_ep_autoconfig_reset'
> |file_storage.c:(.init.text+0xee51): undefined reference to 
> `usb_ep_autoconfig'
> |file_storage.c:(.init.text+0xee73): undefined reference to 
> `usb_ep_autoconfig'
> |file_storage.c:(.init.text+0xee9e): undefined reference to 
> `usb_ep_autoconfig'
>
> Signed-off-by: Sebastian Andrzej Siewior 

At first it looks strange as FSG does not use composite, but yeah:

Acked-by: Michal Nazarewicz 

> ---
> for v3.7 please.
>
>  drivers/usb/gadget/Kconfig |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index bbcec83..66bc3ab 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -737,6 +737,7 @@ config USB_FUNCTIONFS_GENERIC
>  
>  config USB_FILE_STORAGE
>   tristate "File-backed Storage Gadget (DEPRECATED)"
> + select USB_LIBCOMPOSITE
>   depends on BLOCK
>   help
> The File-backed Storage Gadget acts as a USB Mass Storage

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpm3ktKU205V.pgp
Description: PGP signature

Re: [PATCH] usb/gadget: let file storage gadget select libcomposite

[Michal Nazarewicz]

> On Wed, Oct 24, 2012 at 07:30:42PM +0200, Michal Nazarewicz wrote:
>> At first it looks strange as FSG does not use composite, but yeah:

On Wed, Oct 24 2012, Sebastian Andrzej Siewior wrote:
> Yeah. However, it should be removed in v3.8 anyway :)

Yep, that's what feature-removal-schedule.txt says. :]

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpFjWBXHyEmB.pgp
Description: PGP signature

Re: [PATCH] usb/gadget: let file storage gadget select libcomposite

[Michal Nazarewicz]

On Wed, Oct 31 2012, Felipe Balbi  wrote:
> nevertheless, I should be receiving a patch right about now removing
> that file. Alan, care to send a patch for that ?

I have some other stuff to remove as well, so I can take care of it, so
Alan can spend his time on something more important.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpXEcn53TGWi.pgp
Description: PGP signature

Re: [PATCH 1/4] lib: vsprintf: Optimize division by 10 for small integers.

[Michal Nazarewicz]

On Fri, Aug 03 2012, George Spelvin  wrote:
> Shrink the reciprocal approximations used in put_dec_full4
> based on the comments in put_dec_full9.
>
> Signed-off-by: George Spelvin 
> Cc: Denys Vlasenko 
> Cc: Michal Nazarewicz 

Have you verified that the comment is correct?

> ---
>  lib/vsprintf.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> I was looking over the code and noticed that the constants could be smaller.
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index c3f36d41..2f32fe8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -243,13 +243,14 @@ char *put_dec(char *buf, unsigned long long n)
>  
>  /* Second algorithm: valid only for 64-bit long longs */
>  
> +/* See comment in put_dec_full9 for choice of constants */
>  static noinline_for_stack
>  char *put_dec_full4(char *buf, unsigned q)
>  {
>   unsigned r;
> - r  = (q * 0xcccd) >> 19;
> + r  = (q * 0xccd) >> 15;
>   *buf++ = (q - 10 * r) + '0';
> - q  = (r * 0x199a) >> 16;
> + q  = (r * 0xcd) >> 11;
>   *buf++ = (r - 10 * q)  + '0';
>   r  = (q * 0xcd) >> 11;

If you are changing everything, this could also be changed to:

r  = (q * 0x67) >> 10;

no?

>   *buf++ = (q - 10 * r)  + '0';

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgphxPh3rOuGX.pgp
Description: PGP signature

Re: [PATCH 2/4] lib: vsprintf: Optimize division by 10000

[Michal Nazarewicz]

On Fri, Aug 03 2012, George Spelvin wrote:
> The same multiply-by-inverse technique can be used to
> convert division by 1 to a 32x32->64-bit multiply.
>
> Signed-off-by: George Spelvin 

You are using a 64-bit multiply in a path that is designed for 32-bit
processors, which makes me feel that it will be slower.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpopqkAMIww1.pgp
Description: PGP signature

Re: [PATCH 3/4] lib: vsprintf: Optimize put_dec_trunc8

[Michal Nazarewicz]

On Fri, Aug 03 2012, George Spelvin wrote:
> If you're going to have a conditional branch after
> each 32x32->64-bit multiply, might as well shrink the code
> and make it a loop.
>
> This also avoids using the long multiply for small integers.
>
> (This leaves the comments in a confusing state, but that's a separate
> patch to make review easier.)
>
> Signed-off-by: George Spelvin 

NAK.

> ---
>  lib/vsprintf.c |   20 ++--
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a8e7392..3ca77b8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -174,20 +174,12 @@ char *put_dec_trunc8(char *buf, unsigned r)
>   unsigned q;
>  
>   /* Copy of previous function's body with added early returns */
> - q  = (r * (uint64_t)0x199a) >> 32;
> - *buf++ = (r - 10 * q) + '0'; /* 2 */
> - if (q == 0)
> - return buf;
> - r  = (q * (uint64_t)0x199a) >> 32;
> - *buf++ = (q - 10 * r) + '0'; /* 3 */
> - if (r == 0)
> - return buf;
> - q  = (r * (uint64_t)0x199a) >> 32;
> - *buf++ = (r - 10 * q) + '0'; /* 4 */
> - if (q == 0)
> - return buf;
> - r  = (q * (uint64_t)0x199a) >> 32;
> - *buf++ = (q - 10 * r) + '0'; /* 5 */
> + while (r >= 1) {
> + q = r + '0';
> + r  = (r * (uint64_t)0x199a) >> 32;
> + *buf++ = q - 10*r;
> + }

This loop looks nothing like the original code.  Why are you adding '0'
at the beginning?  Also, the original code switches the role of q and r,
the loop does not.

>   if (r == 0)
>   return buf;
>   q  = (r * 0x199a) >> 16;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpSw0MhJNqOH.pgp
Description: PGP signature

Re: [PATCH 3/4] lib: vsprintf: Optimize put_dec_trunc8

[Michal Nazarewicz]

>>> @@ -174,20 +174,12 @@ char *put_dec_trunc8(char *buf, unsigned r)
>>> unsigned q;
>>> /* Copy of previous function's body with added early returns */
>>> -   q  = (r * (uint64_t)0x199a) >> 32;
>>> -   *buf++ = (r - 10 * q) + '0'; /* 2 */
>>> -   if (q == 0)
>>> -   return buf;
>>> -   r  = (q * (uint64_t)0x199a) >> 32;
>>> -   *buf++ = (q - 10 * r) + '0'; /* 3 */
>>> -   if (r == 0)
>>> -   return buf;
>>> -   q  = (r * (uint64_t)0x199a) >> 32;
>>> -   *buf++ = (r - 10 * q) + '0'; /* 4 */
>>> -   if (q == 0)
>>> -   return buf;
>>> -   r  = (q * (uint64_t)0x199a) >> 32;
>>> -   *buf++ = (q - 10 * r) + '0'; /* 5 */
>>> +   while (r >= 1) {
>>> +   q = r + '0';
>>> +   r  = (r * (uint64_t)0x199a) >> 32;
>>> +   *buf++ = q - 10*r;
>>> +   }

All right, I now see what the loop is doing (I couldn't grasp it
yesterday) and expect for r=0 it looks legit.

On Mon, Sep 24 2012, George Spelvin wrote:
> Truthfully, it would have made *more* sense to swap q and r globally,
> so the loop had a more sensible q=quotient/r=remainder assignment,
> but I wanted to show that the unmodified tail was in fact unmodified.

The original has it a bit awkwardly because it just copies code from
put_dec_full9() with the first iteration skipped.

> The big saving from using a loop is that it avoids unnecessary
> 32x32->64-bit multiplies, falling through to the 16x16->32-bit
> code as early as possible.  Given that most numbers are small,
> this seemed like a significant win.

Ah, makes sense.

I guess the following should work, even though it's not so pretty:

static noinline_for_stack
char *put_dec_trunc8(char *buf, unsigned r) {
unsigned q;

if (r > 1) {
do {
q = r + '0';
r = (r * (uint64_t)0x199a) >> 32;
*buf++ = q - 10 * r;
} while (r >= 1);
if (r == 0)
return buf;
}

q  = (r * 0x199a) >> 16;
*buf++ = (r - 10 * q)  + '0'; /* 6 */
if (q == 0)
return buf;
r  = (q * 0xcd) >> 11;
*buf++ = (q - 10 * r)  + '0'; /* 7 */
if (r == 0)
return buf;
q  = (r * 0xcd) >> 11;
*buf++ = (r - 10 * q) + '0'; /* 8 */
if (q == 0)
return buf;
*buf++ = q + '0'; /* 9 */
return buf;
}

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpNHMfZB0BEV.pgp
Description: PGP signature

Re: [PATCH 2/4] lib: vsprintf: Optimize division by 10000

[Michal Nazarewicz]

On Mon, Sep 24 2012, George Spelvin wrote:
>> You are using a 64-bit multiply in a path that is designed for 32-bit
>> processors, which makes me feel that it will be slower.
>
> Slower than the divide it's replacing?

OK, granted, it might be faster after all. ;)  Still, I'd love to see
some benchmark.

> The following 32-bit processors have 32x32->64-bit multiply:
>
> x86
> ARM (as of ARMv4 = ARM7TDMI, the lowest version in common use)
> SPARCv7, SPARCv8

Didn't some SPARCs have 32x32->32 multiply?  I remember reading some
rant from a GMP developer about how SPARC is broken that way.

> MIPS32
> MC68020
> PA-RISC 1.1 (XMPYU)
> avr32
> PowerPC (MULHWU)
> VAX (EMUL)

> I could do some Kconfig hacking and make the code path
> architecture-dependent.  Do you think it's worth it?

Definitely not.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp06tsPHyVRz.pgp
Description: PGP signature

Re: [PATCH 3/4] lib: vsprintf: Optimize put_dec_trunc8

[Michal Nazarewicz]

On Mon, Sep 24 2012, George Spelvin wrote:
> The fix is straightforward:
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index e755083..9872855 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -180,8 +180,6 @@ char *put_dec_trunc8(char *buf, unsigned r)
>   *buf++ = q - 10*r;
>   }
>  
> - if (r == 0)
> - return buf;
>   q  = (r * 0x199a) >> 16;/* r <=  */
>   *buf++ = (r - 10 * q)  + '0';
>   if (q == 0)
>
> Inspired by Michal Nazarewicz, I have some ideas for more tweaking to
> that code.

Ah, right.  I also thought about that first but than started worrying
that it could produce unnecessary zeros if the loop iterates at least
once and exits with r being zero, but now I see that this cannot happen
since if the loop condition was true, r >= 1 and it has been divided
by 1 in the loop so now it's at least 1.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpcdKhuu7frg.pgp
Description: PGP signature

Re: [PATCH 3/4] lib: vsprintf: Optimize put_dec_trunc8

[Michal Nazarewicz]

On Mon, Sep 24 2012, Michal Nazarewicz wrote:
> Ah, right.  I also thought about that first but than started worrying
> that it could produce unnecessary zeros if the loop iterates at least
> once and exits with r being zero, but now I see that this cannot happen
> since if the loop condition was true, r >= 1 and it has been divided
> by 1 in the loop so now it's at least 1.
 ^

I of course meant “10” and “at least 1000”.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQnXFH3FWAc.pgp
Description: PGP signature

Re: [PATCH 3/4] lib: vsprintf: Optimize put_dec_trunc8

[Michal Nazarewicz]

On Mon, Sep 24 2012, George Spelvin wrote:
> Michal Nazarewicz  wrote:
>> static noinline_for_stack
>> char *put_dec_trunc8(char *buf, unsigned r) {
>>  unsigned q;
>> 
>>  if (r > 1) {
>>  do {
>>  q = r + '0';
>>  r = (r * (uint64_t)0x199a) >> 32;
>>  *buf++ = q - 10 * r;
>>  } while (r >= 1);
>>  if (r == 0)
>>  return buf;
>>  }
>> 
>>  q  = (r * 0x199a) >> 16;
>>  *buf++ = (r - 10 * q)  + '0'; /* 6 */
[...]
>>  return buf;
>> }
>
> Two bugs:
>
> 1) The initial "(r > 1)" should be >=.
>If you let r == 1 through to the remaining code, you'll get
>":000".

Obviously... ;)

>
> 2) The "r == 0" test isn't necessary.
>Given that the loop divides r by 10 each time, r >= 1 at the
>beginning implies r >= 1000 at the end, so 1000 <= r < 1
>when the loop exits.

Yeah, I've just figured that out. :]
-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpdvHOqXzWqD.pgp
Description: PGP signature

Re: [PATCH] USB: gadget: f_fs: fix error handling

[Michal Nazarewicz]

On Fri, Sep 27 2013, Robert Baldyga wrote:
> This patch add missing error check in ffs_func_bind() function, after
> ffs_do_descs() funcion call for hs descriptors. Without this check it's
> possible that the module will try dereference incorrect pointer.
>
> Signed-off-by: Robert Baldyga 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/f_fs.c |3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 1a66c5b..fe7d532 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -2264,7 +2264,10 @@ static int ffs_func_bind(struct usb_configuration *c,
>  data->raw_descs + ret,
>  (sizeof data->raw_descs) - ret,
>  __ffs_func_bind_do_descs, func);
> + if (unlikely(ret < 0))
> + goto error;
>   }
> + 

This new line with only a tab in it is not needed here though.

>  
>   /*
>* Now handle interface numbers allocation and interface and

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--


signature.asc
Description: PGP signature

Re: [PATCH v2] USB: gadget: f_fs: fix error handling

[Michal Nazarewicz]

On Mon, Sep 30 2013, Robert Baldyga wrote:
> Hello,
>
> This is update for my patch fixing error handling in functionfs module.
> I have fixed typos from previous version, and changed description for greater
> clearity as Sergei Shtylyov suggested.
>
> This patch add missing error check in ffs_func_bind() function, after
> ffs_do_descs() function call for high speed descriptors. Without this check
> it's possible that the module will try to dereference incorrect pointer.
>
> Signed-off-by: Robert Baldyga 

Acked-by: Michal Nazarewicz 

> Changelog:
>
> v2:
> - fix typos
> - expand patch desctiprion
>
> v1: https://lkml.org/lkml/2013/9/27/128
> ---

Note that it's helpful for maintainers to include everything that is not
meant to go in the commit message after those three minus signs.  This
way, “git am” will ignore the text.

>  drivers/usb/gadget/f_fs.c |2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 1a66c5b..0da66ba 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -2264,6 +2264,8 @@ static int ffs_func_bind(struct usb_configuration *c,
>  data->raw_descs + ret,
>  (sizeof data->raw_descs) - ret,
>  __ffs_func_bind_do_descs, func);
> + if (unlikely(ret < 0))
> + goto error;
>   }
>  
>   /*
> -- 
> 1.7.9.5
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--


signature.asc
Description: PGP signature

Re: [PATCH][RFC] Fix breakage in ffs_fs_mount()

[Michal Nazarewicz]

On Fri, Sep 20 2013, Al Viro wrote:
>   There's a bunch of failure exits in ffs_fs_mount() with
> seriously broken recovery logics.  Most of that appears to stem
> from misunderstanding of the ->kill_sb() semantics;

That sounds likely.

[…]

> Signed-off-by: Al Viro 

Acked-by: Michal Nazarewicz 

> -- 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 1a66c5b..0658908 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -1034,37 +1034,19 @@ struct ffs_sb_fill_data {
>   struct ffs_file_perms perms;
>   umode_t root_mode;
>   const char *dev_name;
> - union {
> - /* set by ffs_fs_mount(), read by ffs_sb_fill() */
> - void *private_data;
> - /* set by ffs_sb_fill(), read by ffs_fs_mount */
> - struct ffs_data *ffs_data;
> - };
> + struct ffs_data *ffs_data;
>  };
>  
>  static int ffs_sb_fill(struct super_block *sb, void *_data, int silent)
>  {
>   struct ffs_sb_fill_data *data = _data;
>   struct inode*inode;
> - struct ffs_data *ffs;
> + struct ffs_data *ffs = data->ffs_data;
>  
>   ENTER();
>  
> - /* Initialise data */
> - ffs = ffs_data_new();
> - if (unlikely(!ffs))
> - goto Enomem;
> -
>   ffs->sb  = sb;
> - ffs->dev_name= kstrdup(data->dev_name, GFP_KERNEL);
> - if (unlikely(!ffs->dev_name))
> - goto Enomem;
> - ffs->file_perms  = data->perms;
> - ffs->private_data= data->private_data;
> -
> - /* used by the caller of this function */
> - data->ffs_data   = ffs;
> -
> + data->ffs_data   = NULL;
>   sb->s_fs_info= ffs;
>   sb->s_blocksize  = PAGE_CACHE_SIZE;
>   sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> @@ -1080,17 +1062,14 @@ static int ffs_sb_fill(struct super_block *sb, void 
> *_data, int silent)
> &data->perms);
>   sb->s_root = d_make_root(inode);
>   if (unlikely(!sb->s_root))
> - goto Enomem;
> + return -ENOMEM;
>  
>   /* EP0 file */
>   if (unlikely(!ffs_sb_create_file(sb, "ep0", ffs,
>&ffs_ep0_operations, NULL)))
> - goto Enomem;
> + return -ENOMEM;
>  
>   return 0;
> -
> -Enomem:
> - return -ENOMEM;
>  }
>  
>  static int ffs_fs_parse_opts(struct ffs_sb_fill_data *data, char *opts)
> @@ -1193,6 +1172,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
>   struct dentry *rv;
>   int ret;
>   void *ffs_dev;
> + struct ffs_data *ffs;
>  
>   ENTER();
>  
> @@ -1200,18 +1180,30 @@ ffs_fs_mount(struct file_system_type *t, int flags,
>   if (unlikely(ret < 0))
>   return ERR_PTR(ret);
>  
> + ffs = ffs_data_new();
> + if (unlikely(!ffs))
> + return ERR_PTR(-ENOMEM);
> + ffs->file_perms = data.perms;
> +
> + ffs->dev_name = kstrdup(dev_name, GFP_KERNEL);
> + if (unlikely(!ffs->dev_name)) {
> + ffs_data_put(ffs);
> + return ERR_PTR(-ENOMEM);
> + }
> +
>   ffs_dev = functionfs_acquire_dev_callback(dev_name);
> - if (IS_ERR(ffs_dev))
> - return ffs_dev;
> + if (IS_ERR(ffs_dev)) {
> + ffs_data_put(ffs);
> + return ERR_CAST(ffs_dev);
> + }
> + ffs->private_data = ffs_dev;
> + data.ffs_data = ffs;
>  
> - data.dev_name = dev_name;
> - data.private_data = ffs_dev;
>   rv = mount_nodev(t, flags, &data, ffs_sb_fill);
> -
> - /* data.ffs_data is set by ffs_sb_fill */
> - if (IS_ERR(rv))
> + if (IS_ERR(rv) && data.ffs_data) {
>   functionfs_release_dev_callback(data.ffs_data);
> -
> + ffs_data_put(data.ffs_data);
> + }
>   return rv;
>  }
>  

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--


signature.asc
Description: PGP signature

Re: [PATCH 1/2] mm: change enum migrate_mode with bitwise type

[Michal Nazarewicz]

On Wed, Sep 05 2012, Minchan Kim wrote:
> This patch changes migrate_mode type to bitwise type because
> next patch will add MIGRATE_DISCARD and it could be ORed with other
> attributes so it would be better to change it with bitwise type.
>
> Cc: Rik van Riel 
> Cc: Mel Gorman 
> Suggested-by: Michal Nazarewicz 
> Signed-off-by: Minchan Kim 

Acked-by: Michal Nazarewicz 

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQAJsCxLEFA.pgp
Description: PGP signature

Re: [PATCH 2/2] mm: support MIGRATE_DISCARD

[Michal Nazarewicz]

On Wed, Sep 05 2012, Minchan Kim wrote:
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
>
> Cc: Marek Szyprowski 
> Cc: Michal Nazarewicz 
> Cc: Rik van Riel 
> Cc: Mel Gorman 
> Signed-off-by: Minchan Kim 

The principle is very good (I always intended for CMA to discard clean
pages but simply did not have time to implement it), and the code looks
reasonably good to me.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp5ySj9CeL89.pgp
Description: PGP signature

Re: [RFC] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

On Wed, Sep 05 2012, Minchan Kim wrote:
> Normally, MIGRATE_ISOLATE type is used for memory-hotplug.
> But it's irony type because the pages isolated would exist
> as free page in free_area->free_list[MIGRATE_ISOLATE] so people
> can think of it as allocatable pages but it is *never* allocatable.
> It ends up confusing NR_FREE_PAGES vmstat so it would be
> totally not accurate so some of place which depend on such vmstat
> could reach wrong decision by the context.
>
> There were already report about it.[1]
> [1] 702d1a6e, memory-hotplug: fix kswapd looping forever problem
>
> Then, there was other report which is other problem.[2]
> [2] http://www.spinics.net/lists/linux-mm/msg41251.html
>
> I believe it can make problems in future, too.
> So I hope removing such irony type by another design.
>
> I hope this patch solves it and let's revert [1] and doesn't need [2].
>
> Cc: Mel Gorman 
> Cc: Kamezawa Hiroyuki 
> Cc: Yasuaki Ishimatsu 
> Cc: Konrad Rzeszutek Wilk 
> Signed-off-by: Minchan Kim 

If you ask me, I'm not convinced that this improves anything.

> ---
>
> It's very early version which show the concept and just tested it with simple
> test and works. This patch is needed indepth review from memory-hotplug
> guys from fujitsu because I saw there are lots of patches recenlty they sent 
> to
> about memory-hotplug change. Please take a look at this patch.
>
>  drivers/xen/balloon.c  |3 +-
>  include/linux/mmzone.h |2 +-
>  include/linux/page-isolation.h |   11 ++-
>  mm/internal.h  |4 +
>  mm/memory_hotplug.c|   38 +
>  mm/page_alloc.c|   35 
>  mm/page_isolation.c|  184 
> +++-
>  7 files changed, 218 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 31ab82f..617d7a3 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -50,6 +50,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -66,7 +67,6 @@
>  #include 
>  #include 
>  #include 
> -

Unrelated and in fact should not be here at all.

>  /*
>   * balloon_process() state:
>   *
> @@ -268,6 +268,7 @@ static void xen_online_page(struct page *page)
>   else
>   --balloon_stats.balloon_hotplug;
>  
> + delete_from_isolated_list(page);
>   mutex_unlock(&balloon_mutex);
>  }
>  
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 2daa54f..977dceb 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -57,7 +57,7 @@ enum {
>*/
>   MIGRATE_CMA,
>  #endif
> - MIGRATE_ISOLATE,/* can't allocate from here */
> + MIGRATE_ISOLATE,
>   MIGRATE_TYPES
>  };

Why remove that comment?

> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index 105077a..a26eb8a 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -1,11 +1,16 @@
>  #ifndef __LINUX_PAGEISOLATION_H
>  #define __LINUX_PAGEISOLATION_H
>  
> +extern struct list_head isolated_pages;
>  
>  bool has_unmovable_pages(struct zone *zone, struct page *page, int count);
>  void set_pageblock_migratetype(struct page *page, int migratetype);
>  int move_freepages_block(struct zone *zone, struct page *page,
>   int migratetype);
> +
> +void isolate_free_page(struct page *page, unsigned int order);
> +void delete_from_isolated_list(struct page *page);
> +
>  /*
>   * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
>   * If specified range includes migrate types other than MOVABLE or CMA,
> @@ -20,9 +25,13 @@ start_isolate_page_range(unsigned long start_pfn, unsigned 
> long end_pfn,
>unsigned migratetype);
>  
>  /*
> - * Changes MIGRATE_ISOLATE to MIGRATE_MOVABLE.
> + * Changes MIGRATE_ISOLATE to @migratetype.
>   * target range is [start_pfn, end_pfn)
>   */
> +void
> +undo_isolate_pageblock(unsigned long start_pfn, unsigned long end_pfn,
> + unsigned migratetype);
> +
>  int
>  undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>   unsigned migratetype);
> diff --git a/mm/internal.h b/mm/internal.h
> index 3314f79..4551179 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -96,6 +96,7 @@ extern void putback_lru_page(struct page *page);
>   */
>  extern void __free_pages_bootmem(struct page *page, unsigned int order);
>  extern void prep_compound_page(struct page *page, unsigned long order);
> +extern int destroy_compound_page(struct page *page, unsigned long order);
>  #ifdef CONFIG_MEMORY_FAILURE
>  extern bool is_free_buddy_page(struct page *page);
>  #endif
> @@ -144,6 +145,9 @@ isolate_migratepages_range(struct zone *zone, struct 
> compact_control *cc,
>   * function for dealing with page's order in buddy system.
>   * zone->lock is already acquired when we use these.
>   * So

Re: [RFC v2] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

On Thu, Sep 06 2012, Minchan Kim wrote:
> Normally, MIGRATE_ISOLATE type is used for memory-hotplug.
> But it's irony type because the pages isolated would exist
> as free page in free_area->free_list[MIGRATE_ISOLATE] so people
> can think of it as allocatable pages but it is *never* allocatable.
> It ends up confusing NR_FREE_PAGES vmstat so it would be
> totally not accurate so some of place which depend on such vmstat
> could reach wrong decision by the context.
>
> There were already report about it.[1]
> [1] 702d1a6e, memory-hotplug: fix kswapd looping forever problem
>
> Then, there was other report which is other problem.[2]
> [2] http://www.spinics.net/lists/linux-mm/msg41251.html
>
> I believe it can make problems in future, too.
> So I hope removing such irony type by another design.
>
> I hope this patch solves it and let's revert [1] and doesn't need [2].
>
> * Changelog v1
>  * Fix from Michal's many suggestion

> ---
> It's very early version which show the concept so I still marked it with RFC.
> I just tested it with simple test and works.
> This patch is needed indepth review from memory-hotplug guys from fujitsu
> because I saw there are lots of patches recenlty they sent to about
> memory-hotplug change. Please take a look at this patch.

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 2daa54f..438bab8 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -57,8 +57,8 @@ enum {
>*/
>   MIGRATE_CMA,
>  #endif
> - MIGRATE_ISOLATE,/* can't allocate from here */
> - MIGRATE_TYPES
> + MIGRATE_TYPES,
> + MIGRATE_ISOLATE
>  };

So now you're saying that MIGRATE_ISOLATE is not a migrate type at all,
since its not < MIGRATE_TYPES.  And still,  I don't see any reason to
remove the comment.

>  
>  #ifdef CONFIG_CMA
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index 105077a..1ae2cd6 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -1,11 +1,16 @@
>  #ifndef __LINUX_PAGEISOLATION_H
>  #define __LINUX_PAGEISOLATION_H
>  
> +extern struct list_head isolated_pages;

I don't think this is needed.

>  
>  bool has_unmovable_pages(struct zone *zone, struct page *page, int count);
>  void set_pageblock_migratetype(struct page *page, int migratetype);
>  int move_freepages_block(struct zone *zone, struct page *page,
>   int migratetype);
> +
> +void isolate_free_page(struct page *page, unsigned int order);
> +void delete_from_isolated_list(struct page *page);
> +
>  /*
>   * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE.
>   * If specified range includes migrate types other than MOVABLE or CMA,
> @@ -20,9 +25,13 @@ start_isolate_page_range(unsigned long start_pfn, unsigned 
> long end_pfn,
>unsigned migratetype);
>  
>  /*
> - * Changes MIGRATE_ISOLATE to MIGRATE_MOVABLE.
> + * Changes MIGRATE_ISOLATE to @migratetype.
>   * target range is [start_pfn, end_pfn)
>   */

You could submit this as a separate patch because this documentation fix
is obviously correct.  Not sure how maintainers respond to one-line
patches though. ;)

> +void
> +undo_isolate_pageblocks(unsigned long start_pfn, unsigned long end_pfn,
> + unsigned migratetype);
> +
>  int
>  undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>   unsigned migratetype);
> diff --git a/mm/internal.h b/mm/internal.h
> index 3314f79..393197e 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -144,6 +144,9 @@ isolate_migratepages_range(struct zone *zone, struct 
> compact_control *cc,
>   * function for dealing with page's order in buddy system.
>   * zone->lock is already acquired when we use these.
>   * So, we don't need atomic page->flags operations here.
> + *
> + * Page order should be put on page->private because
> + * memory-hotplug depends on it. Look mm/page_isolation.c.
>   */
>  static inline unsigned long page_order(struct page *page)
>  {
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 3ad25f9..30c36d5 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -410,26 +410,29 @@ void __online_page_set_limits(struct page *page)
>   unsigned long pfn = page_to_pfn(page);
>  
>   if (pfn >= num_physpages)
> - num_physpages = pfn + 1;
> + num_physpages = pfn + (1 << page_order(page));
>  }
>  EXPORT_SYMBOL_GPL(__online_page_set_limits);
>  
>  void __online_page_increment_counters(struct page *page)
>  {
> - totalram_pages++;
> + totalram_pages += (1 << page_order(page));
>  
>  #ifdef CONFIG_HIGHMEM
>   if (PageHighMem(page))
> - totalhigh_pages++;
> + totalhigh_pages += (1 << page_order(page));
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(__online_page_increment_counters);
>  
>  void __online_page_free(struct page *page)
>  {
> - ClearPageReserved(page);
> - init_pa

Re: [RFC v2] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

On Thu, Sep 06 2012, Lai Jiangshan wrote:
> +found:
> + next_pfn = page_to_pfn(page);
> + list_for_each_entry_from(page, &isolated_pages, lru) {
> + if (page_to_pfn(page) != next_pfn)
> + return false;
> + pfn = page_to_pfn(page);

+   pfn = page_to_pfn(page);
+   if (pfn != next_pfn)
+   return false;

> + next_pfn = pfn + (1UL << page_order(page));
> + if (next_pfn >= end_pfn)
> + return true;
>   }

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQJJ8QTzkJO.pgp
Description: PGP signature

Re: [RFC v2] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

>> +pfn = page_to_pfn(page);
>> +if (pfn >= end_pfn)
>> +return false;
>> +if (pfn >= start_pfn)
>> +goto found;

On Thu, Sep 06 2012, Lai Jiangshan wrote:
> this test is wrong.
>
> use this:
>
> if ((pfn <= start_pfn) && (start_pfn < pfn + (1UL << page_order(page
>   goto found;
>
> if (pfn > start_pfn)
>   return false;

if (pfn > start_pfn)
return false;
if (pfn + (1UL << page_order(page)) > start_pfn)
goto found;

>> +}
>> +return false;
>> +
>> +list_for_each_entry_continue(page, &isolated_pages, lru) {
>> +if (page_to_pfn(page) != next_pfn)
>> +return false;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpIVXD5T4ym5.pgp
Description: PGP signature

Re: [RFC] memory-hotplug: remove MIGRATE_ISOLATE from free_area->free_list

[Michal Nazarewicz]

> On Wed, Sep 05, 2012 at 07:28:23PM +0200, Michal Nazarewicz wrote:
>> If you ask me, I'm not convinced that this improves anything.

On Thu, Sep 06 2012, Minchan Kim wrote:
> At least, it removes MIGRATE_ISOLATE type in free_area->free_list
> which is very irony type as I mentioned. I really don't like such
> type in free_area. What's the benefit if we remain code as it is?
> It could make more problem in future.

I don't really see current situation as making more problems in the
future compared to this code.

You are introducing a new state for a page (ie. it's not in buddy, but
in some new limbo state) and add a bunch of new code and thus bunch of
new  bugs.  I don't see how this improves things over having generic
code that handles moving pages between free lists.

PS.  free_list does exactly what it says on the tin -> the pages are
free, ie. unallocated.  It does not say that they can be allocated. ;)

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpE6gAiVX3dQ.pgp
Description: PGP signature

Re: [PATCH] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

On Fri, Nov 30 2012, Vitaly Andrianov  wrote:
> This patch fixes a couple of bugs that otherwise impair CMA functionality on
> PAE machines:
>
>   - alignment must be a 64-bit type when running on systems with 64-bit
> physical addresses.  If this is not the case, the limit calculation thunks
> allocations down to an address range < 4G.
>
>   - The allocated range check is removed. On 32bit ARM kernel with LPAE
> enabled the base may be allocated outside the fist 4GB of physical
> memory (keystone SoC for example).
>
> Signed-off-by: Vitaly Andrianov 
> Signed-off-by: Cyril Chemparathy 

To be consistent, you should also change the type in:

static long size_cmdline = -1;
static const unsigned long size_bytes = CMA_SIZE_MBYTES * SZ_1M;
static unsigned long __init __maybe_unused cma_early_percent_memory(void)

and in

void __init dma_contiguous_reserve(phys_addr_t limit)

> ---
>  drivers/base/dma-contiguous.c |6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index 9a14694..7936b2e 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -234,7 +234,7 @@ int __init dma_declare_contiguous(struct device *dev, 
> unsigned long size,
> phys_addr_t base, phys_addr_t limit)
>  {
>   struct cma_reserved *r = &cma_reserved[cma_reserved_count];
> - unsigned long alignment;
> + phys_addr_t alignment;
>  
>   pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
>(unsigned long)size, (unsigned long)base,
> @@ -271,10 +271,6 @@ int __init dma_declare_contiguous(struct device *dev, 
> unsigned long size,
>   if (!addr) {
>   base = -ENOMEM;
>   goto err;
> - } else if (addr + size > ~(unsigned long)0) {
> - memblock_free(addr, size);
> - base = -EINVAL;
> - goto err;
>   } else {
>   base = addr;
>   }
> -- 
> 1.7.9.5
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgppNEkaRR05E.pgp
Description: PGP signature

RE: [PATCH] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

>> On Fri, Nov 30 2012, Vitaly Andrianov  wrote:
>> > This patch fixes a couple of bugs that otherwise impair CMA
>> > functionality on PAE machines:
>> >
>> >   - alignment must be a 64-bit type when running on systems with 64-
>> bit
>> > physical addresses.  If this is not the case, the limit
>> calculation thunks
>> > allocations down to an address range < 4G.
>> >
>> >   - The allocated range check is removed. On 32bit ARM kernel with
>> LPAE
>> > enabled the base may be allocated outside the fist 4GB of
>> physical
>> > memory (keystone SoC for example).
>> >
>> > Signed-off-by: Vitaly Andrianov 
>> > Signed-off-by: Cyril Chemparathy 
>> 
>> To be consistent, you should also change the type in:
>> 
>> static long size_cmdline = -1;
>> static const unsigned long size_bytes = CMA_SIZE_MBYTES * SZ_1M; static
>> unsigned long __init __maybe_unused cma_early_percent_memory(void)
>> 
>> and in
>> 
>> void __init dma_contiguous_reserve(phys_addr_t limit)

On Fri, Nov 30 2012, Andrianov, Vitaly wrote:
> Can you elaborate why we need to change type for size? 

Like I've wrote, for consistency.  The size should use the same type as
alignment.  This most likely won't affect the way you use the code, but
it's the right thing to do.  It should be done that way from the start,
so it's really my mistake, but since you are touching the code, it'd
best to have the changes contained in a single commit.  Alternatively
I can just send my own patch with all those fixes (including yours).

>> > diff --git a/drivers/base/dma-contiguous.c
>> > b/drivers/base/dma-contiguous.c index 9a14694..7936b2e 100644
>> > --- a/drivers/base/dma-contiguous.c
>> > +++ b/drivers/base/dma-contiguous.c
>> > @@ -234,7 +234,7 @@ int __init dma_declare_contiguous(struct device
>> *dev, unsigned long size,
>> >  phys_addr_t base, phys_addr_t limit)  {
>> >struct cma_reserved *r = &cma_reserved[cma_reserved_count];
>> > -  unsigned long alignment;
>> > +  phys_addr_t alignment;
>> >
>> >pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
>> > (unsigned long)size, (unsigned long)base, @@ -271,10 +271,6
>> @@ int
>> > __init dma_declare_contiguous(struct device *dev, unsigned long size,
>> >if (!addr) {
>> >base = -ENOMEM;
>> >goto err;
>> > -  } else if (addr + size > ~(unsigned long)0) {
>> > -  memblock_free(addr, size);
>> > -  base = -EINVAL;
>> > -  goto err;
>> >} else {
>> >base = addr;
>> >}

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpBLpWvQLnZ0.pgp
Description: PGP signature

Re: [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

On Mon, Dec 03 2012, Vitaly Andrianov  wrote:
> This patch fixes a couple of bugs that otherwise impair CMA functionality on
> PAE machines:
>
>   - alignment must be a 64-bit type when running on systems with 64-bit
> physical addresses.  If this is not the case, the limit calculation thunks
> allocations down to an address range < 4G.
>
>   - The allocated range check is removed. On 32bit ARM kernel with LPAE
> enabled the base may be allocated outside the fist 4GB of physical
> memory (keystone SoC for example).
>
> Signed-off-by: Vitaly Andrianov 
> Signed-off-by: Cyril Chemparathy 

Acked-by: Michal Nazarewicz 

> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index 9a14694..097dd44 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -60,8 +60,8 @@ struct cma *dma_contiguous_default_area;
>   * Users, who want to set the size of global CMA area for their system
>   * should use cma= kernel parameter.
>   */
> -static const unsigned long size_bytes = CMA_SIZE_MBYTES * SZ_1M;
> -static long size_cmdline = -1;
> +static const phys_addr_t size_bytes = CMA_SIZE_MBYTES * SZ_1M;
> +static phys_addr_t size_cmdline = -1;
>  
>  static int __init early_cma(char *p)
>  {
> @@ -73,7 +73,7 @@ early_param("cma", early_cma);
>  
>  #ifdef CONFIG_CMA_SIZE_PERCENTAGE
>  
> -static unsigned long __init __maybe_unused cma_early_percent_memory(void)
> +static phys_addr_t __init __maybe_unused cma_early_percent_memory(void)
>  {
>   struct memblock_region *reg;
>   unsigned long total_pages = 0;
> @@ -91,7 +91,7 @@ static unsigned long __init __maybe_unused 
> cma_early_percent_memory(void)
>  
>  #else
>  
> -static inline __maybe_unused unsigned long cma_early_percent_memory(void)
> +static inline __maybe_unused phys_addr_t cma_early_percent_memory(void)
>  {
>   return 0;
>  }
> @@ -109,7 +109,7 @@ static inline __maybe_unused unsigned long 
> cma_early_percent_memory(void)
>   */
>  void __init dma_contiguous_reserve(phys_addr_t limit)
>  {
> - unsigned long selected_size = 0;
> + phys_addr_t selected_size = 0;
>  
>   pr_debug("%s(limit %08lx)\n", __func__, (unsigned long)limit);
>  
> @@ -129,7 +129,7 @@ void __init dma_contiguous_reserve(phys_addr_t limit)
>  
>   if (selected_size) {
>   pr_debug("%s: reserving %ld MiB for global area\n", __func__,
> -  selected_size / SZ_1M);
> +  (unsigned long)selected_size / SZ_1M);
>  
>   dma_declare_contiguous(NULL, selected_size, 0, limit);
>   }
> @@ -230,11 +230,11 @@ core_initcall(cma_init_reserved_areas);
>   * called by board specific code when early allocator (memblock or bootmem)
>   * is still activate.
>   */
> -int __init dma_declare_contiguous(struct device *dev, unsigned long size,
> +int __init dma_declare_contiguous(struct device *dev, phys_addr_t size,
> phys_addr_t base, phys_addr_t limit)
>  {
>   struct cma_reserved *r = &cma_reserved[cma_reserved_count];
> - unsigned long alignment;
> + phys_addr_t alignment;
>  
>   pr_debug("%s(size %lx, base %08lx, limit %08lx)\n", __func__,
>(unsigned long)size, (unsigned long)base,
> @@ -271,10 +271,6 @@ int __init dma_declare_contiguous(struct device *dev, 
> unsigned long size,
>   if (!addr) {
>   base = -ENOMEM;
>   goto err;
> - } else if (addr + size > ~(unsigned long)0) {
> - memblock_free(addr, size);
> - base = -EINVAL;
> - goto err;
>   } else {
>   base = addr;
>   }
> diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
> index 2f303e4..01b5c84 100644
> --- a/include/linux/dma-contiguous.h
> +++ b/include/linux/dma-contiguous.h
> @@ -68,7 +68,7 @@ struct device;
>  extern struct cma *dma_contiguous_default_area;
>  
>  void dma_contiguous_reserve(phys_addr_t addr_limit);
> -int dma_declare_contiguous(struct device *dev, unsigned long size,
> +int dma_declare_contiguous(struct device *dev, phys_addr_t size,
>  phys_addr_t base, phys_addr_t limit);
>  
>  struct page *dma_alloc_from_contiguous(struct device *dev, int count,
> @@ -83,7 +83,7 @@ bool dma_release_from_contiguous(struct device *dev, struct 
> page *pages,
>  static inline void dma_contiguous_reserve(phys_addr_t limit) { }
>  
>  static inline
> -int dma_declare_contiguous(struct device *dev, unsigned long size,
> +

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

> On Monday 03 December 2012 09:16 PM, Vitaly Andrianov wrote:
>> This patch fixes a couple of bugs that otherwise impair CMA functionality on
>> PAE machines:
>>
>>- alignment must be a 64-bit type when running on systems with 64-bit
>>  physical addresses.  If this is not the case, the limit calculation 
>> thunks
>>  allocations down to an address range < 4G.
>>
>>- The allocated range check is removed. On 32bit ARM kernel with LPAE
>>  enabled the base may be allocated outside the fist 4GB of physical
>>  memory (keystone SoC for example).

On Tue, Dec 04 2012, Santosh Shilimkar wrote:
> Any reason you have clubbed two fixes in one patch. Its better to keep
> the two fixes separate patches.

They are all related to the very same issue, and what the whole patch
does is change the type used to store physical addresses from unsigned
long to phys_addr_t.  This is really a single change.

>> Signed-off-by: Vitaly Andrianov 
>> Signed-off-by: Cyril Chemparathy 

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpqKTyhgv5Fx.pgp
Description: PGP signature

Re: [linux-keystone] [PATCH v2] drivers: cma: fix addressing on PAE machines

[Michal Nazarewicz]

> On Tuesday 04 December 2012 06:37 PM, Michal Nazarewicz wrote:
>> They are all related to the very same issue, and what the whole patch
>> does is change the type used to store physical addresses from unsigned
>> long to phys_addr_t.  This is really a single change.

On Tue, Dec 04 2012, Santosh Shilimkar wrote:
> Thanks for clarification. 64 bit alignment fix and the allocation
> range checks can be two separate fixes and that is exactly what change
> log describes.  You have a last say though :-) No problem if you want
> to commit the patch as is.

I don't have strong feelings on this one, but I feel like it's really
a single change which manifests itself in a few ways.  If this is
confusing, maybe commit message could be improved, to something like:

- >8 ---
drivers: cma: represent physicall addresses as phys_addr_t

This commit changes the CMA early initialisation code to use phys_addr_t
for representing physical addresses instead of unsigned long.

Without this change, among other things, dma_declare_contiguous() simply
discards any memory regions whose address is not represtible as unsigned
long.

This is a problem on 32-bit PAE machines where unsigned long is 32-bit
but physical address space is larger.
- 8< ---

Vitaly, if you could resend with that description, it would be awesome,
and sorry for so much trouble in what appears to be a trivial patch. :P

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQvO2bFgD7E.pgp
Description: PGP signature

Re: [PATCH] mm: cma: WARN if freed memory is still in use

[Michal Nazarewicz]

On Mon, Nov 12 2012, Marek Szyprowski wrote:
> Memory return to free_contig_range() must have no other references. Let
> kernel to complain loudly if page reference count is not equal to 1.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 022e4ed..290c2eb 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5888,8 +5888,13 @@ done:
>  
>  void free_contig_range(unsigned long pfn, unsigned nr_pages)
>  {
> - for (; nr_pages--; ++pfn)
> - __free_page(pfn_to_page(pfn));
> + struct page *page = pfn_to_page(pfn);
> + int refcount = nr_pages;
> + for (; nr_pages--; page++) {
> + refcount -= page_count(page) == 1;
> + __free_page(page);
> + }
> + WARN(refcount != 0, "some pages are still in use!\n");

This decrementing logic seem backward to me.  Why not:

struct page *page = pfn_to_page(pfn);
unsigned int refcount = 0;
for (; nr_pages--; page++) {
refcount += page_count(page) != 1;
__free_page(page);
}
WARN(refcount != 0, "some pages are still in use!\n");

>  }
>  #endif

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpOv6U2Dnor8.pgp
Description: PGP signature

Re: [PATCH] mm: use migrate_prep() instead of migrate_prep_local()

[Michal Nazarewicz]

On Mon, Nov 12 2012, Marek Szyprowski wrote:
> __alloc_contig_migrate_range() should use all possible ways to get all the
> pages migrated from the given memory range, so pruning per-cpu lru lists
> for all CPUs is required, regadless the cost of such operation. Otherwise
> some pages which got stuck at per-cpu lru list might get missed by
> migration procedure causing the contiguous allocation to fail.
>
> Reported-by: SeongHwan Yoon 
> Signed-off-by: Marek Szyprowski 
> Signed-off-by: Kyungmin Park 

Acked-by: Michal Nazarewicz 

> ---
>  mm/page_alloc.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1bfe2b0..fcb9719 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5677,7 +5677,7 @@ static int __alloc_contig_migrate_range(struct 
> compact_control *cc,
>   unsigned int tries = 0;
>   int ret = 0;
>  
> - migrate_prep_local();
> + migrate_prep();
>  
>   while (pfn < end || !list_empty(&cc->migratepages)) {
>   if (fatal_signal_pending(current)) {

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpdhwifs4U4p.pgp
Description: PGP signature

Re: [PATCH v2] mm: cma: WARN if freed memory is still in use

[Michal Nazarewicz]

On Tue, Nov 13 2012, Marek Szyprowski  wrote:
> Memory returned to free_contig_range() must have no other references. Let
> kernel to complain loudly if page reference count is not equal to 1.
>
> Signed-off-by: Marek Szyprowski 
> Reviewed-by: Kyungmin Park 
> CC: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 022e4ed..290c2eb 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5888,8 +5888,13 @@ done:
>  
>  void free_contig_range(unsigned long pfn, unsigned nr_pages)
>  {
> - for (; nr_pages--; ++pfn)
> - __free_page(pfn_to_page(pfn));
> + struct page *page = pfn_to_page(pfn);
> + int count = 0;
> + for (; nr_pages--; page++) {
> + count += page_count(page) != 1;
> + __free_page(page);
> + }
> + WARN(count != 0, "%d pages are still in use!\n", count);
>  }
>  #endif

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpDmxjcmoyhB.pgp
Description: PGP signature

Re: [PATCH] mm: Remove unused variable in alloc_contig_range()

[Michal Nazarewicz]

On Mon, Nov 12 2012, Thierry Reding wrote:
> Commit 872ca38f7afd9566bf2f88b95616f7ab71b50064 removed the last
> reference to this variable but not the variable itself.
>
> Signed-off-by: Thierry Reding 

Acked-by: Michal Nazarewicz 

I could have sworn that someone (Marek?) sent that patch already.

> ---
>  mm/page_alloc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6b990cb..71933dd 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5822,7 +5822,6 @@ static int __alloc_contig_migrate_range(struct 
> compact_control *cc,
>  int alloc_contig_range(unsigned long start, unsigned long end,
>  unsigned migratetype)
>  {
> - struct zone *zone = page_zone(pfn_to_page(start));
>   unsigned long outer_start, outer_end;
>   int ret = 0, order;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp1yP5RDhMbM.pgp
Description: PGP signature

Re: [PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

> On Sat, 22 Dec 2012, Michal Nazarewicz wrote:
>> So I think just adding the following, should be sufficient to make
>> everyone happy:
>> 
>> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
>> index e34e3e0..e91743b 100644
>> --- a/drivers/base/dma-contiguous.c
>> +++ b/drivers/base/dma-contiguous.c
>> @@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device 
>> *dev, unsigned int count,
>>  pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
>>   count, align);
>>  
>> -if (!count)
>> +if (!count || count > INT_MAX)
>>  return NULL;
>>  
>>  mask = (1 << align) - 1;
>
On Thu, Dec 27 2012, David Rientjes  wrote:
> How is this different than leaving the formal to have a signed type, i.e. 
> drop your patch, and testing for count <= 0 instead?

Not much different I guess.  I don't have strong opinions to be honest,
except that I feel unsigned is the proper type to use, on top of which
I think bitmap_set() should use unsigned, so in case anyone ever bothers
to change it, CMA will be ready. :P

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp0BAwjJmwz7.pgp
Description: PGP signature

[PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

From: Michal Nazarewicz 

Specifying negative size of buffer makes no sense and thus this commit
changes the type of the count argument to unsigned.

Signed-off-by: Michal Nazarewicz 
---
 arch/arm/mm/dma-mapping.c  |   12 ++--
 drivers/base/dma-contiguous.c  |6 +++---
 include/linux/dma-contiguous.h |8 
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 6b2fb87..77e292e 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device 
*dev, size_t size,
  gfp_t gfp, struct dma_attrs *attrs)
 {
struct page **pages;
-   int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
-   int i = 0;
+   unsigned int count = size >> PAGE_SHIFT;
+   unsigned int array_size = count * sizeof(struct page *);
+   unsigned int i = 0;
 
if (array_size <= PAGE_SIZE)
pages = kzalloc(array_size, gfp);
@@ -1102,9 +1102,9 @@ error:
 static int __iommu_free_buffer(struct device *dev, struct page **pages,
   size_t size, struct dma_attrs *attrs)
 {
-   int count = size >> PAGE_SHIFT;
-   int array_size = count * sizeof(struct page *);
-   int i;
+   unsigned int count = size >> PAGE_SHIFT;
+   unsigned int array_size = count * sizeof(struct page *);
+   unsigned int i;
 
if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
dma_release_from_contiguous(dev, pages[0], count);
diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index 0ca5442..e34e3e0 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -303,7 +303,7 @@ err:
  * global one. Requires architecture specific get_dev_cma_area() helper
  * function.
  */
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
   unsigned int align)
 {
unsigned long mask, pfn, pageno, start = 0;
@@ -317,7 +317,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, 
int count,
if (align > CONFIG_CMA_ALIGNMENT)
align = CONFIG_CMA_ALIGNMENT;
 
-   pr_debug("%s(cma %p, count %d, align %d)\n", __func__, (void *)cma,
+   pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
 count, align);
 
if (!count)
@@ -364,7 +364,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, 
int count,
  * true otherwise.
  */
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-int count)
+unsigned int count)
 {
struct cma *cma = dev_get_cma_area(dev);
unsigned long pfn;
diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
index 01b5c84..040501a 100644
--- a/include/linux/dma-contiguous.h
+++ b/include/linux/dma-contiguous.h
@@ -71,10 +71,10 @@ void dma_contiguous_reserve(phys_addr_t addr_limit);
 int dma_declare_contiguous(struct device *dev, phys_addr_t size,
   phys_addr_t base, phys_addr_t limit);
 
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
   unsigned int order);
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-int count);
+unsigned int count);
 
 #else
 
@@ -90,7 +90,7 @@ int dma_declare_contiguous(struct device *dev, phys_addr_t 
size,
 }
 
 static inline
-struct page *dma_alloc_from_contiguous(struct device *dev, int count,
+struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
   unsigned int order)
 {
return NULL;
@@ -98,7 +98,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, 
int count,
 
 static inline
 bool dma_release_from_contiguous(struct device *dev, struct page *pages,
-int count)
+unsigned int count)
 {
return false;
 }
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: compare MIGRATE_ISOLATE selectively

[Michal Nazarewicz]

On Thu, Dec 20 2012, Minchan Kim wrote:
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index a92061e..4ada4ef 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -1,6 +1,25 @@
>  #ifndef __LINUX_PAGEISOLATION_H
>  #define __LINUX_PAGEISOLATION_H
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +static inline bool page_isolated_pageblock(struct page *page)
> +{
> + return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
> +}
> +static inline bool mt_isolated_pageblock(int migratetype)
> +{
> + return migratetype == MIGRATE_ISOLATE;
> +}

Perhaps “is_migrate_isolate” to match already existing “is_migrate_cma”?
Especially as the “mt_isolated_pageblock” sound confusing to me, it
implies that it works on pageblocks which it does not.

> +#else
> +static inline bool page_isolated_pageblock(struct page *page)
> +{
> + return false;
> +}
> +static inline bool mt_isolated_pageblock(int migratetype)
> +{
> + return false;
> +}
> +#endif
>  
>  bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>bool skip_hwpoisoned_pages);
-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgph8cives84r.pgp
Description: PGP signature

Re: [PATCH] mm: compare MIGRATE_ISOLATE selectively

[Michal Nazarewicz]

> On Thu, Dec 20, 2012 at 04:49:44PM +0100, Michal Nazarewicz wrote:
>> Perhaps “is_migrate_isolate” to match already existing “is_migrate_cma”?

On Fri, Dec 21 2012, Minchan Kim wrote:
> Good poking. In fact, while I made this patch, I was very tempted by renaming
> is_migrate_cma to cma_pageblock.
>
> is_migrate_cma(mt)
>
> I don't know who start to use "mt" instead of "migratetype" but anyway, it's
> not a good idea.
>
> is_migrate_cma(migratetype)
>
> It's very clear for me because migratetype is per pageblock, we can know the
> function works per pageblock unit.
>
>> Especially as the “mt_isolated_pageblock” sound confusing to me, it
>> implies that it works on pageblocks which it does not.
>
> -ENOPARSE.
>
> migratetype works on pageblock.

migratetype is a number, which can be assigned to a pageblock.  In some
transitional cases, the migratetype associated with a page can differ
from the migratetype associated with the pageblock the page is in.  As
such, I think it's confusing to add “pageblock” to the name of the
function which does not read migratetype from pageblock but rather
operates on the number it is provided.

> I admit mt is really dirty but I used page_alloc.c already has lots of
> mt, SIGH.

I don't really have an issue with “mt” myself, especially since the few
times “mt” is used in page_alloc.c it is a local variable which I don't
think needs a long descriptive name since context is all there.

> How about this?
>
> 1. Let's change all "mt" with "migratetype" again.
> 2. use is_migrate_isolate and is_migrate_cma for "migratetype".
> 3. use is_migrate_isolate_page instead of page_isolated_pageblock for
>"page".

Like I've said.  Personally I don't really think 1 is needed, but 2 and
3 look good to me.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpr2eK2L3xNS.pgp
Description: PGP signature

Re: [PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz]

On Fri, Dec 21 2012, David Rientjes wrote:
> > Specifying negative size of buffer makes no sense and thus this commit
> > changes the type of the count argument to unsigned.
> > 
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct 
> > device *dev, size_t size,
> >   gfp_t gfp, struct dma_attrs *attrs)
> >  {
> > struct page **pages;
> > -   int count = size >> PAGE_SHIFT;
> > -   int array_size = count * sizeof(struct page *);
> > -   int i = 0;
> > +   unsigned int count = size >> PAGE_SHIFT;
> > +   unsigned int array_size = count * sizeof(struct page *);
> > +   unsigned int i = 0;

> I didn't ack this because there's no bounds checking on 
> dma_alloc_from_contiguous() and bitmap_set() has a dangerous side-effect 
> when called with an overflowed nr since it takes a signed argument.  

Mystery solved.  I recalled that there was some reason why the count is
specified as a signed int and thought bitmap_find_next_zero_area() was
the culprit, but now it seems that bitmap_set() was the reason.

> Marek, is there some sane upper bound we can put on count?

INT_MAX would be sufficient.  After all, it maps to a 8 TiB buffer (if
page is 4 KiB).

Moreover, in reality, the few places that call
dma_alloc_from_contiguous() pass a value that cannot be higher than
INT_MAX, ie. (listings heavily stripped):

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device 
*dev, size_t size,
arch/arm/mm/dma-mapping.c-   pgprot_t prot, 
struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-  size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:  page = dma_alloc_from_contiguous(dev, count, 
order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device 
*dev, size_t size,
arch/arm/mm/dma-mapping.c-   pgprot_t prot, 
struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-  size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:  page = dma_alloc_from_contiguous(dev, count, 
order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static struct page **__iommu_alloc_buffer(struct 
device *dev, size_t size,
arch/arm/mm/dma-mapping.c-gfp_t gfp, 
struct dma_attrs *attrs)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-  unsigned int count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c-  if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, 
attrs)) {
arch/arm/mm/dma-mapping.c:  page = dma_alloc_from_contiguous(dev, 
count, order);
arch/arm/mm/dma-mapping.c-  }
arch/arm/mm/dma-mapping.c-}

arch/x86/kernel/pci-dma.c-void *dma_generic_alloc_coherent(struct device *dev, 
size_t size,
arch/x86/kernel/pci-dma.c-   dma_addr_t *dma_addr, 
gfp_t flag,
arch/x86/kernel/pci-dma.c-   struct dma_attrs 
*attrs)
arch/x86/kernel/pci-dma.c-{
arch/x86/kernel/pci-dma.c-  unsigned int count = PAGE_ALIGN(size) >> 
PAGE_SHIFT;
arch/x86/kernel/pci-dma.c-  if (!(flag & GFP_ATOMIC))
arch/x86/kernel/pci-dma.c:  page = dma_alloc_from_contiguous(dev, 
count, get_order(size));
arch/x86/kernel/pci-dma.c-}

So I think just adding the following, should be sufficient to make
everyone happy:

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index e34e3e0..e91743b 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, 
unsigned int count,
pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
 count, align);
 
-   if (!count)
+   if (!count || count > INT_MAX)
return NULL;
 
mask = (1 << align) - 1;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQ7LBVmPKDx.pgp
Description: PGP signature

Re: [PATCH] mm: cma: allocate pages from CMA if NR_FREE_PAGES approaches low water mark

[Michal Nazarewicz]

On Tue, Nov 20 2012, Marek Szyprowski wrote:
> Right now running out of 'plain' movable pages is the only possibility to
> get movable pages allocated from CMA. On the other hand running out of
> 'plain' movable pages is very deadly for the system, as movable pageblocks
> are also the main fallbacks for reclaimable and non-movable pages.
>
> Then, once we run out of movable pages and kernel needs non-mobable or
> reclaimable page (what happens quite often), it usually triggers OOM to
> satisfy the memory needs. Such OOM is very strange, especially on a system
> with dozen of megabytes of CMA memory, having most of them free at the OOM
> event. By high memory pressure I mean the high memory usage.

Would it make sense to *always* use MIGRATE_CMA for movable allocations
before MIGRATE_MOVABLE?  Ie. how about this patch (not tested):

- >8 -
>From 790a3b5743414f2770e413e5e8866679de2920b4 Mon Sep 17 00:00:00 2001
Message-Id: 
<790a3b5743414f2770e413e5e8866679de2920b4.1353425911.git.min...@mina86.com>
From: Michal Nazarewicz 
Date: Tue, 20 Nov 2012 16:37:50 +0100
Subject: [PATCH] mm: cma: on movable allocations try MIGRATE_CMA first

It has been observed that system tends to keep a lot of CMA free pages
even in very high memory pressure use cases.  The CMA fallback for
movable pages is used very rarely, only when system is completely
pruned from MOVABLE pages.  This means that the out-of-memory is
triggered for unmovable allocations even when there are many CMA pages
available.  This problem was not observed previously since movable
pages were used as a fallback for unmovable allocations.

To avoid such situation this commit changes the allocation order so
that on movable allocations the MIGRATE_CMA pageblocks are used first.

This change means that the MIGRATE_CMA can be removed from fallback
path of the MIGRATE_MOVABLE type.  This means that the
__rmqueue_fallback() function will never deal with CMA pages and thus
all the checks around MIGRATE_CMA can be removed from that function.

Signed-off-by: Michal Nazarewicz 
Reported-by: Marek Szyprowski 
Cc: Kyungmin Park 
---
 mm/page_alloc.c |   55 +--
 1 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bb90971..b60bd75 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -893,14 +893,12 @@ struct page *__rmqueue_smallest(struct zone *zone, 
unsigned int order,
  * This array describes the order lists are fallen back to when
  * the free lists for the desirable migrate type are depleted
  */
-static int fallbacks[MIGRATE_TYPES][4] = {
+static int fallbacks[MIGRATE_TYPES][3] = {
[MIGRATE_UNMOVABLE]   = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE, 
MIGRATE_RESERVE },
[MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE,   MIGRATE_MOVABLE, 
MIGRATE_RESERVE },
+   [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   
MIGRATE_RESERVE },
 #ifdef CONFIG_CMA
-   [MIGRATE_MOVABLE] = { MIGRATE_CMA, MIGRATE_RECLAIMABLE, 
MIGRATE_UNMOVABLE, MIGRATE_RESERVE },
[MIGRATE_CMA] = { MIGRATE_RESERVE }, /* Never used */
-#else
-   [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   
MIGRATE_RESERVE },
 #endif
[MIGRATE_RESERVE] = { MIGRATE_RESERVE }, /* Never used */
[MIGRATE_ISOLATE] = { MIGRATE_RESERVE }, /* Never used */
@@ -1019,17 +1017,10 @@ __rmqueue_fallback(struct zone *zone, int order, int 
start_migratetype)
 * pages to the preferred allocation list. If falling
 * back for a reclaimable kernel allocation, be more
 * aggressive about taking ownership of free pages
-*
-* On the other hand, never change migration
-* type of MIGRATE_CMA pageblocks nor move CMA
-* pages on different free lists. We don't
-* want unmovable pages to be allocated from
-* MIGRATE_CMA areas.
 */
-   if (!is_migrate_cma(migratetype) &&
-   (unlikely(current_order >= pageblock_order / 2) ||
-start_migratetype == MIGRATE_RECLAIMABLE ||
-page_group_by_mobility_disabled)) {
+   if (unlikely(current_order >= pageblock_order / 2) ||
+   start_migratetype == MIGRATE_RECLAIMABLE ||
+   page_group_by_mobility_disabled) {
int pages;
pages = move_freepages_block(zone, page,

start_migratetype);
@@ -1048,14 +1

Re: [Part3 PATCH v2 1/4] bootmem, mem-hotplug: Register local pagetable pages with LOCAL_NODE_DATA when freeing bootmem.

[Michal Nazarewicz]

On Thu, Jun 13 2013, Tang Chen wrote:
> As Yinghai suggested, even if a node is movable node, which has only
> ZONE_MOVABLE, pagetables should be put in the local node.
>
> In memory hot-remove logic, it offlines all pages first, and then
> removes pagetables. But the local pagetable pages cannot be offlined
> because they are used by kernel.
>
> So we should skip this kind of pages in offline procedure. But first
> of all, we need to mark them.
>
> This patch marks local node data pages in the same way as we mark the
> SECTION_INFO and MIX_SECTION_INFO data pages. We introduce a new type
> of bootmem: LOCAL_NODE_DATA. And use page->lru.next to mark this type
> of memory.
>
> Signed-off-by: Tang Chen 
> ---
>  arch/x86/mm/init_64.c  |2 +
>  include/linux/memblock.h   |   22 +
>  include/linux/memory_hotplug.h |   13 -
>  mm/memblock.c  |   52 
> 
>  mm/memory_hotplug.c|   26 
>  5 files changed, 113 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index bb00c46..25de304 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1053,6 +1053,8 @@ static void __init register_page_bootmem_info(void)
>  
>   for_each_online_node(i)
>   register_page_bootmem_info_node(NODE_DATA(i));
> +
> + register_page_bootmem_local_node();
>  #endif
>  }
>  
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index a85ced9..8a38eef 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -131,6 +131,28 @@ void __next_free_mem_range_rev(u64 *idx, int nid, 
> phys_addr_t *out_start,
>i != (u64)ULLONG_MAX;  \
>__next_free_mem_range_rev(&i, nid, p_start, p_end, p_nid))
>  
> +void __next_local_node_mem_range(int *idx, int nid, phys_addr_t *out_start,
> +  phys_addr_t *out_end, int *out_nid);

Why not make it return int?

> +
> +/**
> + * for_each_local_node_mem_range - iterate memblock areas storing local node
> + * data
> + * @i: int used as loop variable
> + * @nid: node selector, %MAX_NUMNODES for all nodes
> + * @p_start: ptr to phys_addr_t for start address of the range, can be %NULL
> + * @p_end: ptr to phys_addr_t for end address of the range, can be %NULL
> + * @p_nid: ptr to int for nid of the range, can be %NULL
> + *
> + * Walks over memblock areas storing local node data. Since all the local 
> node
> + * areas will be reserved by memblock, this iterator will only iterate
> + * memblock.reserve. Available as soon as memblock is initialized.
> + */
> +#define for_each_local_node_mem_range(i, nid, p_start, p_end, p_nid) \
> + for (i = -1,\
> +  __next_local_node_mem_range(&i, nid, p_start, p_end, p_nid);   \
> +  i != -1;   \
> +  __next_local_node_mem_range(&i, nid, p_start, p_end, p_nid))
> +

If __next_local_node_mem_range() returned int, this would be easier:

+#define for_each_local_node_mem_range(i, nid, p_start, p_end, p_nid) \
+   for (i = -1;
+(i = __next_local_node_mem_range(i, nid, p_start, p_end, p_nid)) 
!= -1; )

>  #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
>  int memblock_set_node(phys_addr_t base, phys_addr_t size, int nid);
>  
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 0b21e54..c0c4107 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h

> +/**
> + * __next_local_node_mem_range - next function for
> + *   for_each_local_node_mem_range()
> + * @idx: pointer to int loop variable
> + * @nid: node selector, %MAX_NUMNODES for all nodes
> + * @out_start: ptr to phys_addr_t for start address of the range, can be 
> %NULL
> + * @out_end: ptr to phys_addr_t for end address of the range, can be %NULL
> + * @out_nid: ptr to int for nid of the range, can be %NULL
> + */
> +void __init_memblock __next_local_node_mem_range(int *idx, int nid,
> + phys_addr_t *out_start,
> + phys_addr_t *out_end, int *out_nid)
> +{
> + __next_flag_mem_range(idx, nid, MEMBLK_LOCAL_NODE,
> +   out_start, out_end, out_nid);
> +}

static inline in a header file perhaps?

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

signature.asc
Description: PGP signature

Re: [PATCH 1/1] module: fix sprintf format specifier in param_get_byte()

[Michal Nazarewicz]

On Wed, Aug 07 2013, Rusty Russell wrote:
> Christoph Jaeger  writes:
>> In param_get_byte(), to which the macro STANDARD_PARAM_DEF(byte, ...) 
>> expands,
>> "%c" is used to print an unsigned char. So it gets printed as a character 
>> what
>> is not intended here. Use "%hhu" instead.
>>
>> Signed-off-by: Christoph Jaeger 

Acked-by: Michal Nazarewicz 

for g_ffs.c:

> drivers/usb/gadget/g_ffs.c:95:module_param_named(bDeviceClass,
> gfs_dev_desc.bDeviceClass,byte,   0644);
> drivers/usb/gadget/g_ffs.c:97:module_param_named(bDeviceSubClass, 
> gfs_dev_desc.bDeviceSubClass, byte,   0644);
> drivers/usb/gadget/g_ffs.c:99:module_param_named(bDeviceProtocol, 
> gfs_dev_desc.bDeviceProtocol, byte,   0644);

I don't think it breaks anything for g_ffs since those properties are
most likely write-only and I don't expect many people reading them.

>> ---
>>  kernel/params.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/params.c b/kernel/params.c
>> index 440e65d..59f7ac7 100644
>> --- a/kernel/params.c
>> +++ b/kernel/params.c
>> @@ -252,7 +252,7 @@ int parse_args(const char *doing,
>>  EXPORT_SYMBOL(param_ops_##name)
>>  
>>  
>> -STANDARD_PARAM_DEF(byte, unsigned char, "%c", unsigned long, 
>> strict_strtoul);
>> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", unsigned long, 
>> strict_strtoul);
>>  STANDARD_PARAM_DEF(short, short, "%hi", long, strict_strtol);
>>  STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", unsigned long, 
>> strict_strtoul);

Actually, are those “h” specifiers even necessary?  I'm fairly certain
that “%u” for byte, “%i” for short, and “%u” for ushort would work just
fine since the argument gets promoted to (unsigned) int anyway and
indeed vsnprintf reads an int for all those types.

>>  STANDARD_PARAM_DEF(int, int, "%i", long, strict_strtol);

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

signature.asc
Description: PGP signature

Re: [PATCH] tools/usb: adapt ffs-test.c to new layout of includes (UAPI)

[Michal Nazarewicz]

On Fri, Mar 01 2013, Andrzej Pietrasiewicz wrote:
This fix has already been mailed:

https://patchwork.kernel.org/patch/2169791/

> Signed-off-by: Andrzej Pietrasiewicz 
> Signed-off-by: Kyungmin Park 
> ---
>  tools/usb/ffs-test.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/usb/ffs-test.c b/tools/usb/ffs-test.c
> index 8674b9e..fe1e66b 100644
> --- a/tools/usb/ffs-test.c
> +++ b/tools/usb/ffs-test.c
> @@ -38,7 +38,7 @@
>  #include 
>  #include 
>  
> -#include "../../include/linux/usb/functionfs.h"
> +#include "../../include/uapi/linux/usb/functionfs.h"
>  
>  
>  / Little Endian Handling 
> /
> -- 

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp2oVLr2Jdei.pgp
Description: PGP signature

Re: [PATCH] usb/gadget: initialize gadget functions helper befor functions & gadgets

[Michal Nazarewicz]

On Mon, Mar 04 2013, Sebastian Andrzej Siewior wrote:
> Fengguang Wu run into a kernel ops after he complied dummy_hcd and g_cdc
> into the kernel. The problem was that u_serial was used by g_cdc before
> u_serial was initialized. In the module case eveything is initialized in
> the correct order but if we compile it into the kernel we rely on
> Makefile order which I did wrong and now I correct this.
>
> Cc: Fengguang Wu 

Should that be Reported-by?

> Signed-off-by: Sebastian Andrzej Siewior 

Acked-by: Michal Nazarewicz 

> ---
> I kinda assumed that this was already fixed (i.e. a patch like this was sent)
> but it seems not to be that case.
>
>  drivers/usb/gadget/Makefile |   12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 97a13c3..1ded3d4 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -35,6 +35,12 @@ mv_udc-y   := mv_udc_core.o
>  obj-$(CONFIG_USB_FUSB300)+= fusb300_udc.o
>  obj-$(CONFIG_USB_MV_U3D) += mv_u3d_core.o
>  
> +# USB Functions

Should there be a comment explaining why USB functions must be placed
before gadgets?

> +obj-$(CONFIG_USB_U_SERIAL)   += u_serial.o
> +obj-$(CONFIG_USB_F_ACM)  += f_acm.o
> +f_ss_lb-y:= f_loopback.o f_sourcesink.o
> +obj-$(CONFIG_USB_F_SS_LB)+= f_ss_lb.o
> +
>  #
>  # USB gadget drivers
>  #
> @@ -74,9 +80,3 @@ obj-$(CONFIG_USB_G_WEBCAM)  += g_webcam.o
>  obj-$(CONFIG_USB_G_NCM)  += g_ncm.o
>  obj-$(CONFIG_USB_G_ACM_MS)   += g_acm_ms.o
>  obj-$(CONFIG_USB_GADGET_TARGET)  += tcm_usb_gadget.o
> -
> -# USB Functions
> -obj-$(CONFIG_USB_F_ACM)  += f_acm.o
> -f_ss_lb-y:= f_loopback.o f_sourcesink.o
> -obj-$(CONFIG_USB_F_SS_LB)+= f_ss_lb.o
> -obj-$(CONFIG_USB_U_SERIAL)   += u_serial.o



-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpHi_4pCUApv.pgp
Description: PGP signature

Re: [PATCH] usb: gadget: multi: Mark {cdc,rndis}_config_register() __init

[Michal Nazarewicz]

On Wed, Apr 24 2013, Geert Uytterhoeven wrote:
> If gcc (e.g. 4.1.2) decides not to inline cdc_config_register() and
> rndis_config_register(), this will cause section mismatch warnings:
>
> WARNING: drivers/usb/gadget/g_multi.o(.text+0x32f6): Section mismatch in 
> reference from the function cdc_config_register() to the function 
> .init.text:cdc_do_config()
> The function cdc_config_register() references
> the function __init cdc_do_config().
> This is often because cdc_config_register lacks a __init
> annotation or the annotation of cdc_do_config is wrong.
>
> WARNING: drivers/usb/gadget/g_multi.o(.text+0x3326): Section mismatch in 
> reference from the function rndis_config_register() to the function 
> .init.text:rndis_do_config()
> The function rndis_config_register() references
> the function __init rndis_do_config().
> This is often because rndis_config_register lacks a __init
> annotation or the annotation of rndis_do_config is wrong.
>
> Fortunately they're harmless, as {cdc,rndis}_config_register() are only
> called from multi_bind(), which is annotated with __ref.
>
> Mark {cdc,rndis}_config_register() with the __init annotation to kill the
> warning.
>
> Signed-off-by: Geert Uytterhoeven 

Acked-by: Michal Nazarewicz 

With transition to configfs all those __inits and __refs will probably
go away in the long term though.

> ---
> Note: I did not verify that multi_bind() is rightfully marked __ref.

It should be.  It's only referenced in multi_driver object as a bind
callback which is called by composite_bind() which is referenced in
composite_driver_template as bind callback which is called by
usb_gadget_probe_driver() which is called by usb_composite_probe() which
is called by multi_init() which is __init and called as module_init.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgphj11_9fMlI.pgp
Description: PGP signature

Re: [PATCH 2/2] dma-buf: add helpers for attacher dma-parms

[Michal Nazarewicz]

Tomasz Stanislawski  writes:
> I recommend to change the semantics for unlimited number of segments
> from 'value 0' to:
>
> #define DMA_SEGMENTS_COUNT_UNLIMITED ((unsigned long)INT_MAX)
>
> Using INT_MAX will allow using safe conversions between signed and
> unsigned integers.

LONG_MAX seems cleaner regardless.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpQ7aEwh5ehY.pgp
Description: PGP signature

Re: [RFC] mm: support MIGRATE_DISCARD

[Michal Nazarewicz]

Minchan Kim  writes:
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drops *unmapped clean cache pages* instead of migration so that
> migration latency could be reduced by avoiding (memcpy + page remapping).
> It's useful for CMA because latency of migration is very important rather
> than eviction of background processes's workingset. In addition, it needs
> less free pages for migration targets so it could avoid memory reclaiming
> to get free pages, which is another factor increase latency.
>
> Cc: Marek Szyprowski 
> Cc: Michal Nazarewicz 
> Cc: Rik van Riel 
> Cc: Mel Gorman 
> Signed-off-by: Minchan Kim 

Other than just a few minor comments below, the idea behind the code
looks good to me.

> ---
>  include/linux/migrate_mode.h |   11 ++---
>  mm/migrate.c |   56 
> ++
>  mm/page_alloc.c  |2 +-
>  3 files changed, 55 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
> index ebf3d89..8e44e30 100644
> --- a/include/linux/migrate_mode.h
> +++ b/include/linux/migrate_mode.h
> @@ -6,11 +6,16 @@
>   *   on most operations but not ->writepage as the potential stall time
>   *   is too significant
>   * MIGRATE_SYNC will block when migrating pages
> + * MIGRTATE_DISCARD will discard clean cache page instead of migration
> + *
> + * MIGRATE_ASYNC, MIGRATE_SYNC_LIGHT, MIGRATE_SYNC shouldn't be used
> + * together with OR flag.
>   */
>  enum migrate_mode {
> - MIGRATE_ASYNC,
> - MIGRATE_SYNC_LIGHT,
> - MIGRATE_SYNC,
> + MIGRATE_ASYNC = 1 << 0,
> + MIGRATE_SYNC_LIGHT = 1 << 1,
> + MIGRATE_SYNC = 1 << 2,
> + MIGRATE_DISCARD = 1 << 3,
>  };
>  
>  #endif   /* MIGRATE_MODE_H_INCLUDED */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 77ed2d7..90be7a9 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -225,7 +225,7 @@ static bool buffer_migrate_lock_buffers(struct 
> buffer_head *head,
>   struct buffer_head *bh = head;
>  
>   /* Simple case, sync compaction */
> - if (mode != MIGRATE_ASYNC) {
> + if (!(mode & MIGRATE_ASYNC)) {

You're doing bit operations on enum type and except enum type to have
values which are not defined within the enum type (ie. MIGRATE_SYNC |
MIGRATE_DISCARD does not map to any enum value).  I feel that the
variable should be changed to be “unsigned” (maybe with __bitwise)
rather than “enum migrate_mode”.

>   do {
>   get_bh(bh);
>   lock_buffer(bh);
> @@ -313,7 +313,7 @@ static int migrate_page_move_mapping(struct address_space 
> *mapping,
>* the mapping back due to an elevated page count, we would have to
>* block waiting on other references to be dropped.
>*/
> - if (mode == MIGRATE_ASYNC && head &&
> + if (mode & MIGRATE_ASYNC && head &&

Please use parens around bit operations. :)

>   !buffer_migrate_lock_buffers(head, mode)) {
>   page_unfreeze_refs(page, expected_count);
>   spin_unlock_irq(&mapping->tree_lock);
> @@ -521,7 +521,7 @@ int buffer_migrate_page(struct address_space *mapping,
>* with an IRQ-safe spinlock held. In the sync case, the buffers
>* need to be locked now
>*/
> - if (mode != MIGRATE_ASYNC)
> + if (!(mode & MIGRATE_ASYNC))
>   BUG_ON(!buffer_migrate_lock_buffers(head, mode));
>  
>   ClearPagePrivate(page);
> @@ -603,7 +603,7 @@ static int fallback_migrate_page(struct address_space 
> *mapping,
>  {
>   if (PageDirty(page)) {
>   /* Only writeback pages in full synchronous migration */
> - if (mode != MIGRATE_SYNC)
> + if (!(mode & MIGRATE_SYNC))
>   return -EBUSY;
>   return writeout(mapping, page);
>   }
> @@ -678,6 +678,19 @@ static int move_to_new_page(struct page *newpage, struct 
> page *page,
>   return rc;
>  }
>  
> +static int discard_page(struct page *page)
> +{
> + int ret = -EAGAIN;
> +
> + struct address_space *mapping = page_mapping(page);
> + if (page_has_private(page))
> + if (!try_to_release_page(page, GFP_KERNEL))
> + return ret;
> + if (remove_mapping(mapping, page))
> + ret = 0;
> + return ret;
> +}
> +
>  static int __unmap_and_move(struct page *page, struct page *newpage,
>   int force, bool offlining, enum migrate_mode mode)
>  {
> @@ -685,9 +698,12 @@ static int __unmap

Re: [PATCH] mm: cma: fix alignment requirements for contiguous regions

[Michal Nazarewicz]

Marek Szyprowski  writes:
> Contiguous Memory Allocator requires each of its regions to be aligned
> in such a way that it is possible to change migration type for all
> pageblocks holding it and then isolate page of largest possible order from
> the buddy allocator (which is MAX_ORDER-1). This patch relaxes alignment
> requirements by one order, because MAX_ORDER alignment is not really
> needed.
>
> Signed-off-by: Marek Szyprowski 
> CC: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/base/dma-contiguous.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
> index 78efb03..34d94c7 100644
> --- a/drivers/base/dma-contiguous.c
> +++ b/drivers/base/dma-contiguous.c
> @@ -250,7 +250,7 @@ int __init dma_declare_contiguous(struct device *dev, 
> unsigned long size,
>   return -EINVAL;
>  
>   /* Sanitise input arguments */
> - alignment = PAGE_SIZE << max(MAX_ORDER, pageblock_order);
> + alignment = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order);
>   base = ALIGN(base, alignment);
>   size = ALIGN(size, alignment);
>   limit &= ~(alignment - 1);


-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpjkjoPSp7fy.pgp
Description: PGP signature

[PATCH] usb: gadget: get rid of USB_GADGET_{DUAL,SUPER}SPEED

[Michal Nazarewicz]

From: Michal Nazarewicz 

This commit removes USB_GADGET_DUALSPEED and USB_GADGET_SUPERSPEED
Kconfig options.  Since now kernel allows many UDC drivers to be
compiled, those options may turn to no longer be valid.  For
instance, if someone decides to build UDC that supports super
speed and UDC that supports high speed only, the latter will be
"assumed" to support super speed since USB_GADGET_SUPERSPEED will
be selected by the former.

The test of whether CONFIG_USB_GADGET_*SPEED was defined was just
an optimisation which removed otherwise dead code (ie. if UDC is
not dual speed, there is no need to handle cases that can happen
if speed is high).  This commit removes those checks.

Signed-off-by: Michal Nazarewicz 
---
 Hi,

 I've sent this patch quite a while ago but it did not got picked up
 for some reason.  Looking at the archives[1] I could not figure out
 a reason why this patch should be omitted so I'm resending it.

 This is rebase on top of v3.6-rc3.

 [1] 
https://groups.google.com/forum/?fromgroups=#!topic/linux.kernel/cUOFAhPGNhE

 drivers/usb/chipidea/Kconfig   |1 -
 drivers/usb/dwc3/Kconfig   |2 --
 drivers/usb/gadget/Kconfig |   25 -
 drivers/usb/gadget/composite.c |9 +
 drivers/usb/gadget/inode.c |   15 ---
 drivers/usb/gadget/u_ether.c   |7 ---
 drivers/usb/musb/Kconfig   |1 -
 include/linux/usb/gadget.h |   19 ++-
 8 files changed, 7 insertions(+), 72 deletions(-)

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index 47e499c..1ea932a 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -13,7 +13,6 @@ if USB_CHIPIDEA
 config USB_CHIPIDEA_UDC
bool "ChipIdea device controller"
depends on USB_GADGET=y || USB_GADGET=USB_CHIPIDEA
-   select USB_GADGET_DUALSPEED
help
  Say Y here to enable device controller functionality of the
  ChipIdea driver.
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index d13c60f..f6a6e07 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -2,8 +2,6 @@ config USB_DWC3
tristate "DesignWare USB3 DRD Core Support"
depends on (USB && USB_GADGET)
select USB_OTG_UTILS
-   select USB_GADGET_DUALSPEED
-   select USB_GADGET_SUPERSPEED
select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
help
  Say Y or M here if your system has a Dual Role SuperSpeed
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 51ab5fd..2ba0d0e 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -154,7 +154,6 @@ config USB_LPC32XX
 
 config USB_ATMEL_USBA
tristate "Atmel USBA"
-   select USB_GADGET_DUALSPEED
depends on AVR32 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
  USBA is the integrated high-speed USB Device controller on
@@ -163,7 +162,6 @@ config USB_ATMEL_USBA
 config USB_FSL_USB2
tristate "Freescale Highspeed USB DR Peripheral Controller"
depends on FSL_SOC || ARCH_MXC
-   select USB_GADGET_DUALSPEED
select USB_FSL_MPH_DR_OF if OF
help
   Some of Freescale PowerPC and i.MX processors have a High Speed
@@ -179,7 +177,6 @@ config USB_FSL_USB2
 config USB_FUSB300
tristate "Faraday FUSB300 USB Peripheral Controller"
depends on !PHYS_ADDR_T_64BIT
-   select USB_GADGET_DUALSPEED
help
   Faraday usb device controller FUSB300 driver
 
@@ -227,7 +224,6 @@ config USB_PXA25X_SMALL
 
 config USB_R8A66597
tristate "Renesas R8A66597 USB Peripheral Controller"
-   select USB_GADGET_DUALSPEED
help
   R8A66597 is a discrete USB host and peripheral controller chip that
   supports both full and high speed USB 2.0 data transfers.
@@ -240,7 +236,6 @@ config USB_R8A66597
 config USB_RENESAS_USBHS_UDC
tristate 'Renesas USBHS controller'
depends on USB_RENESAS_USBHS
-   select USB_GADGET_DUALSPEED
help
   Renesas USBHS is a discrete USB host and peripheral controller chip
   that supports both full and high speed USB 2.0 data transfers.
@@ -268,7 +263,6 @@ config USB_PXA27X
 config USB_S3C_HSOTG
tristate "S3C HS/OtG USB Device controller"
depends on S3C_DEV_USB_HSOTG
-   select USB_GADGET_DUALSPEED
help
  The Samsung S3C64XX USB2.0 high-speed gadget controller
  integrated into the S3C64XX series SoC.
@@ -305,7 +299,6 @@ config USB_S3C2410_DEBUG
 config USB_S3C_HSUDC
tristate "S3C2416, S3C2443 and S3C2450 USB Device Controller"
depends on ARCH_S3C24XX
-   select USB_GADGET_DUALSPEED
help
  Samsung's S3C2416, S3C2443 and S3C2450 is an ARM9 based SoC
  integrated with d

Re: [RFC 2/2] cma: support MIGRATE_DISCARD

[Michal Nazarewicz]

Minchan Kim  writes:
> This patch introudes MIGRATE_DISCARD mode in migration.
> It drop clean cache pages instead of migration so that
> migration latency could be reduced. Of course, it could
> evict code pages but latency of big contiguous memory
> is more important than some background application's slow down
> in mobile embedded enviroment.
>
> Signed-off-by: Minchan Kim 

This looks good to me.

> ---
>  include/linux/migrate_mode.h |   11 +++---
>  mm/migrate.c |   50 
> +-
>  mm/page_alloc.c  |2 +-
>  3 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
> index ebf3d89..04ca19c 100644
> --- a/include/linux/migrate_mode.h
> +++ b/include/linux/migrate_mode.h
> @@ -6,11 +6,16 @@
>   *   on most operations but not ->writepage as the potential stall time
>   *   is too significant
>   * MIGRATE_SYNC will block when migrating pages
> + * MIGRTATE_DISCARD will discard clean cache page instead of migration
> + *
> + * MIGRATE_ASYNC, MIGRATE_SYNC_LIGHT, MIGRATE_SYNC shouldn't be used
> + * together as OR flag.
>   */
>  enum migrate_mode {
> - MIGRATE_ASYNC,
> - MIGRATE_SYNC_LIGHT,
> - MIGRATE_SYNC,
> + MIGRATE_ASYNC = 1 << 0,
> + MIGRATE_SYNC_LIGHT = 1 << 1,
> + MIGRATE_SYNC = 1 << 2,
> + MIGRATE_DISCARD = 1 << 3,
>  };

Since CMA is the only user of MIGRATE_DISCARD it may be worth it to
guard it inside an #ifdef, eg:

#ifdef CONFIG_CMA
MIGRATE_DISCARD = 1 << 3,
#define is_migrate_discard(mode) (((mode) & MIGRATE_DISCARD) == MIGRATE_DISCARD)
#endif

  
>  #endif   /* MIGRATE_MODE_H_INCLUDED */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 77ed2d7..8119a59 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -685,9 +685,12 @@ static int __unmap_and_move(struct page *page, struct 
> page *newpage,
>   int remap_swapcache = 1;
>   struct mem_cgroup *mem;
>   struct anon_vma *anon_vma = NULL;
> + enum ttu_flags ttu_flags;
> + bool discard_mode = false;
> + bool file = false;
>  
>   if (!trylock_page(page)) {
> - if (!force || mode == MIGRATE_ASYNC)
> + if (!force || mode & MIGRATE_ASYNC)

+   if (!force || (mode & MIGRATE_ASYNC))

>   goto out;
>  
>   /*


-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpLT4dHqBYb9.pgp
Description: PGP signature

Re: [RFC 2/2] cma: support MIGRATE_DISCARD

[Michal Nazarewicz]

> On Tue, Aug 14, 2012 at 04:19:55PM +0200, Michal Nazarewicz wrote:
>> Since CMA is the only user of MIGRATE_DISCARD it may be worth it to
>> guard it inside an #ifdef, eg:

Minchan Kim  writes:
> In summary, I want to open it for potential usecases in future if anyone
> doesn't oppose strongly.

Fair enough.

>>> if (!trylock_page(page)) {
>>> -   if (!force || mode == MIGRATE_ASYNC)
>>> +   if (!force || mode & MIGRATE_ASYNC)

> It's not wrong technically but for readability, NP.

Yep, that was my point, thanks. :)

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpnlrtx0V4E8.pgp
Description: PGP signature

Re: [PATCH 04/13] rbtree: move some implementation details from rbtree.h to rbtree.c

[Michal Nazarewicz]

On Tue, 10 Jul 2012 01:35:14 +0200, Michel Lespinasse  wrote:


rbtree users must use the documented APIs to manipulate the tree
structure.  Low-level helpers to manipulate node colors and parenthood
are not part of that API, so move them to lib/rbtree.c

Signed-off-by: Michel Lespinasse 
---
 include/linux/rbtree.h |   16 
 lib/rbtree.c   |   18 ++
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index 2049087..a06c044 100644
--- a/include/linux/rbtree.h
+++ b/include/linux/rbtree.h
@@ -35,8 +35,6 @@
 struct rb_node
 {
unsigned long  rb_parent_color;
-#defineRB_RED  0
-#defineRB_BLACK1
struct rb_node *rb_right;
struct rb_node *rb_left;
 } __attribute__((aligned(sizeof(long;
@@ -49,20 +47,6 @@ struct rb_root
#define rb_parent(r)   ((struct rb_node *)((r)->rb_parent_color & ~3))
-#define rb_color(r)   ((r)->rb_parent_color & 1)
-#define rb_is_red(r)   (!rb_color(r))
-#define rb_is_black(r) rb_color(r)
-#define rb_set_red(r)  do { (r)->rb_parent_color &= ~1; } while (0)
-#define rb_set_black(r)  do { (r)->rb_parent_color |= 1; } while (0)
-
-static inline void rb_set_parent(struct rb_node *rb, struct rb_node *p)
-{
-   rb->rb_parent_color = (rb->rb_parent_color & 3) | (unsigned long)p;
-}
-static inline void rb_set_color(struct rb_node *rb, int color)
-{
-   rb->rb_parent_color = (rb->rb_parent_color & ~1) | color;
-}
#define RB_ROOT (struct rb_root) { NULL, }
 #definerb_entry(ptr, type, member) container_of(ptr, type, member)
diff --git a/lib/rbtree.c b/lib/rbtree.c
index fe43c8c..d0ec339 100644
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -23,6 +23,24 @@
 #include 
 #include 
+#defineRB_RED  0
+#defineRB_BLACK1


Interestingly, those are almost never used. RB_BLACK is used only once.
Should we get rid of those instead?  Or change the code (like rb_is_red())
to use them?


+
+#define rb_color(r)   ((r)->rb_parent_color & 1)
+#define rb_is_red(r)   (!rb_color(r))
+#define rb_is_black(r) rb_color(r)
+#define rb_set_red(r)  do { (r)->rb_parent_color &= ~1; } while (0)
+#define rb_set_black(r)  do { (r)->rb_parent_color |= 1; } while (0)
+
+static inline void rb_set_parent(struct rb_node *rb, struct rb_node *p)
+{
+   rb->rb_parent_color = (rb->rb_parent_color & 3) | (unsigned long)p;
+}
+static inline void rb_set_color(struct rb_node *rb, int color)
+{
+   rb->rb_parent_color = (rb->rb_parent_color & ~1) | color;
+}
+
 static void __rb_rotate_left(struct rb_node *node, struct rb_root *root)
 {
struct rb_node *right = node->rb_right;



--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 05/13] rbtree: performance and correctness test

[Michal Nazarewicz]

On Tue, 10 Jul 2012 01:35:15 +0200, Michel Lespinasse  wrote:


This small module helps measure the performance of rbtree insert and erase.

Additionally, we run a few correctness tests to check that the rbtrees have
all desired properties:
- contains the right number of nodes in the order desired,
- never two consecutive red nodes on any path,
- all paths to leaf nodes have the same number of black nodes,
- root node is black

Signed-off-by: Michel Lespinasse 
---
 tests/rbtree_test.c |  135 +++
 1 files changed, 135 insertions(+), 0 deletions(-)
 create mode 100644 tests/rbtree_test.c

diff --git a/tests/rbtree_test.c b/tests/rbtree_test.c
new file mode 100644
index 000..2e3944d
--- /dev/null
+++ b/tests/rbtree_test.c
@@ -0,0 +1,135 @@
+#include 
+#include 
+#include 
+#include 
+
+#define NODES   100
+#define PERF_LOOPS  10
+#define CHECK_LOOPS 100
+
+struct test_node {
+   struct rb_node rb;
+   u32 key;
+};
+
+static struct rb_root root = RB_ROOT;
+static struct test_node nodes[NODES];
+
+static struct rnd_state rnd;
+
+static void insert(struct test_node *node, struct rb_root *root)
+{
+   struct rb_node **new = &root->rb_node, *parent = NULL;
+
+   while (*new) {
+   parent = *new;
+   if (node->key < rb_entry(parent, struct test_node, rb)->key)
+   new = &parent->rb_left;
+   else
+   new = &parent->rb_right;
+   }
+
+   rb_link_node(&node->rb, parent, new);
+   rb_insert_color(&node->rb, root);
+}
+
+static inline void erase(struct test_node *node, struct rb_root *root)
+{
+   rb_erase(&node->rb, root);
+}
+
+static void init(void)
+{
+   int i;
+   for (i = 0; i < NODES; i++)


s/NODES/ARRAY_SIZE(nodes)/ perhaps?  Same goes for the rest of the code.


+   nodes[i].key = prandom32(&rnd);
+}
+
+static bool is_red(struct rb_node *rb)
+{
+   return rb->rb_parent_color == (unsigned long)rb_parent(rb);


Why not !(rb->rb_parent_color & 1) which to me seems more intuitive.


+}
+
+static int black_path_count(struct rb_node *rb)
+{
+   int count;
+   for (count = 0; rb; rb = rb_parent(rb))
+   count += !is_red(rb);
+   return count;
+}
+
+static void check(int nr_nodes)
+{
+   struct rb_node *rb;
+   int count = 0;
+   int blacks;
+   u32 prev_key = 0;
+
+   for (rb = rb_first(&root); rb; rb = rb_next(rb)) {
+   struct test_node *node = rb_entry(rb, struct test_node, rb);
+   WARN_ON_ONCE(node->key < prev_key);


What if for some reason we generate node with key equal zero or two keys
with the same value?  It may not be the case for current code, but someone
might change it in the future.  I think <= is safer here.


+   WARN_ON_ONCE(is_red(rb) &&
+(!rb_parent(rb) || is_red(rb_parent(rb;
+   if (!count)
+   blacks = black_path_count(rb);
+   else
+   WARN_ON_ONCE((!rb->rb_left || !rb->rb_right) &&
+blacks != black_path_count(rb));
+   prev_key = node->key;
+   count++;
+   }
+   WARN_ON_ONCE(count != nr_nodes);
+}
+
+static int rbtree_test_init(void)
+{
+   int i, j;
+   cycles_t time1, time2, time;
+
+   printk(KERN_ALERT "rbtree testing");
+
+   prandom32_seed(&rnd, 3141592653589793238);
+   init();
+
+   time1 = get_cycles();
+
+   for (i = 0; i < PERF_LOOPS; i++) {
+   for (j = 0; j < NODES; j++)
+   insert(nodes + j, &root);
+   for (j = 0; j < NODES; j++)
+   erase(nodes + j, &root);
+   }
+
+   time2 = get_cycles();
+   time = time2 - time1;
+
+   time = div_u64(time, PERF_LOOPS);
+   printk(" -> %llu cycles\n", time);
+
+   for (i = 0; i < CHECK_LOOPS; i++) {
+   init();


Is this init() needed?


+   for (j = 0; j < NODES; j++) {
+   check(j);
+   insert(nodes + j, &root);
+   }
+   for (j = 0; j < NODES; j++) {
+   check(NODES - j);
+   erase(nodes + j, &root);
+   }
+   check(0);
+   }
+
+   return -EAGAIN; /* Fail will directly unload the module */
+}


--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/6] USB: serial: Changes to conform with checkpatch.

[Michal Nazarewicz]

On Wed, 11 Jul 2012 16:10:14 +0200, Ben Minerds  wrote:

Removed various checkpatch.sh warnings and errors.


You've meant chekpatch.pl, right?


Split patch by warning/error type.
Corrected line wraps in emails.

Signed-off-by: Ben Minerds 


This 0/6 also confuses me -- like Greg have said the 0/n mail should not
contain any diff, just a summary -- but all the other patches look good
to me as they are only whitespace changes -- if only there was a diff
tool that understood C and compared stream of C tokens rather then a plain
text file.

--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 04/13] rbtree: move some implementation details from rbtree.h to rbtree.c

[Michal Nazarewicz]

On Wed, 11 Jul 2012 01:12:54 +0200, Michel Lespinasse  wrote:


On Tue, Jul 10, 2012 at 5:19 AM, Michal Nazarewicz  wrote:

On Tue, 10 Jul 2012 01:35:14 +0200, Michel Lespinasse  wrote:

+#defineRB_RED  0
+#defineRB_BLACK1


Interestingly, those are almost never used. RB_BLACK is used only once.
Should we get rid of those instead?  Or change the code (like rb_is_red())
to use them?


I'm actually making heavier use of RB_RED / RB_BLACK later on in the patch set.


Yeah, I've just noticed.  Disregard my comment.


But agree, rb_is_red() / rb_is_black() could use these too.


--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: cma: allocate pages from CMA if NR_FREE_PAGES approaches low water mark

[Michal Nazarewicz]

On Wed, Nov 21 2012, Minchan Kim wrote:
> So your concern is that too many free pages in MIGRATE_CMA when OOM happens
> is odd? It's natural with considering CMA design which kernel never fallback
> non-movable page allocation to CMA area. I guess it's not a your concern.
>
> Let's think below extreme cases.
>
> = Before =
>
> * 1000M DRAM system.
> * 400M kernel used pages.
> * 300M movable used pages.
> * 300M cma freed pages.
>
> 1. kernel want to request 400M non-movable memory, additionally.
> 2. VM start to reclaim 300M movable pages.
> 3. But it's not enough to meet 400M request.
> 4. go to OOM. (It's natural)
>
> = After(with your patch) =
>
> * 1000M DRAM system.
> * 400M kernel used pages.
> * 300M movable *freed* pages.
> * 300M cma used pages(by your patch, I simplified your concept)
>
> 1. kernel want to request 400M non-movable memory.
> 2. 300M movable freed pages isn't enough to meet 400M request.
> 3. Also, there is no point to reclaim CMA pages for non-movable allocation.
> 4. go to OOM. (It's natural)
>
> There is no difference between before and after in allocation POV.
> Let's think another example.
>
> = Before =
>
> * 1000M DRAM system.
> * 400M kernel used pages.
> * 300M movable used pages.
> * 300M cma freed pages.
>
> 1. kernel want to request 300M non-movable memory.
> 2. VM start to reclaim 300M movable pages.
> 3. It's enough to meet 300M request.
> 4. happy end
>
> = After(with your patch) =
>
> * 1000M DRAM system.
> * 400M kernel used pages.
> * 300M movable *freed* pages.
> * 300M cma used pages(by your patch, I simplified your concept)
>
> 1. kernel want to request 300M non-movable memory.
> 2. 300M movable freed pages is enough to meet 300M request.
> 3. happy end.
>
> There is no difference in allocation POV, too.

The difference thou is that before 30% of memory is wasted (ie. free),
whereas after all memory is used.  The main point of CMA is to make the
memory useful if devices are not using it.  Having it not allocated is
defeating that purpose.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpS3YMu3PGIk.pgp
Description: PGP signature

Re: [RFC][PATCH] fs: configfs: programmatically create config groups

[Michal Nazarewicz]

On Mon, Nov 26 2012, Andrzej Pietrasiewicz  wrote:
> In some parts of the kernel (e.g. planned configfs integration into usb
> gadget) there is a need to programmatically create config groups
> (directories) but it would be preferable to disallow creating them by
> the user. This is more or less what default_groups used to be for.
> But e.g. in the mass storage gadget, after storing the number of
> luns (logical units) into some configfs attribute, the corresponding lun#
> directories should be created, their number is not known up front so
> default_groups are no good for this.
>
> Example:
>
> $ echo 3 > /cfg//mass_storage/luns
>
> causes
>
> /cfg/../mass_storage/lun0
> /cfg/../mass_storage/lun1
> /cfg/../mass_storage/lun2
>
> to be created. Yet
>
> $ mkdir /cfg//mass_storage/
>
> should not be allowed.
>
> With create_group exported it is very easily achieved: make_group and 
> make_item
> are set to NULL in mass_storage's config_group, yet the kernel can
> create_groups at will.

I think the above description should be part of the commit message. :)

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpAM5Vcctf6D.pgp
Description: PGP signature

Re: [PATCH v2 1/2] Fix wrong EOF compare

[Michal Nazarewicz]

On Fri, Jan 11 2013, Minchan Kim  wrote:
> The C standards allows the character type char to be singed or unsinged,
> depending on the platform and compiler. Most of systems uses signed char,
> but those based on PowerPC and ARM processors typically use unsigned char.
> This can lead to unexpected results when the variable is used to compare
> with EOF(-1). It happens my ARM system and this patch fixes it.
>
> Cc: Mel Gorman 
> Cc: Andy Whitcroft 
> Cc: Alexander Nyberg 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> Cc: Randy Dunlap 
> Signed-off-by: Minchan Kim 
> ---
>  Documentation/page_owner.c |7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index f0156e1..43dde96 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -32,12 +32,13 @@ int read_block(char *buf, FILE *fin)
>  {
>   int ret = 0;
>   int hit = 0;
> + int val;
>   char *curr = buf;
>  
>   for (;;) {
> - *curr = getc(fin);
> - if (*curr == EOF) return -1;
> -
> + val = getc(fin);
> + if (val == EOF) return -1;
> + *curr = val;
>   ret++;
>   if (*curr == '\n' && hit == 1)
>   return ret - 1;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpG8UbalpaQ7.pgp
Description: PGP signature

Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

[Michal Nazarewicz]

It occurred to me -- and I know it will sound like a heresy -- that
maybe providing an overly long example in C is not the best option here.
Why not page_owner.py with the following content instead (not tested):


#!/usr/bin/python
import collections
import sys

counts = collections.defaultdict(int)

txt = ''
for line in sys.stdin:
if line == '\n':
counts[txt] += 1
txt = ''
else:
txt += line
counts[txt] += 1

for txt, num in sorted(counts.items(), txt=lambda x: x[1]):
if len(txt) > 1:
print '%d times:\n%s' % num, txt


And it's so “long” only because I chose not to read the whole file at
once as in:


counts = collections.defaultdict(int)
for txt in sys.stdin.read().split('\n\n'):
counts[txt] += 1


On Fri, Jan 11 2013, Minchan Kim wrote:
> The read_block reads char one by one until meeting two newline.
> It's not good for the performance and current code isn't good shape
> for readability.
>
> This patch enhances speed and clean up.
>
> Cc: Mel Gorman 
> Cc: Andy Whitcroft 
> Cc: Alexander Nyberg 
> Cc: Randy Dunlap 
> Signed-off-by: Michal Nazarewicz 
> Signed-off-by: Minchan Kim 
> ---
>  Documentation/page_owner.c |   34 +-
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index 43dde96..96bf481 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -28,26 +28,17 @@ static int max_size;
>  
>  struct block_list *block_head;
>  
> -int read_block(char *buf, FILE *fin)
> +int read_block(char *buf, int buf_size, FILE *fin)
>  {
> - int ret = 0;
> - int hit = 0;
> - int val;
> - char *curr = buf;
> -
> - for (;;) {
> - val = getc(fin);
> - if (val == EOF) return -1;
> - *curr = val;
> - ret++;
> - if (*curr == '\n' && hit == 1)
> - return ret - 1;
> - else if (*curr == '\n')
> - hit = 1;
> - else
> - hit = 0;
> - curr++;
> + char *curr = buf, *const buf_end = buf + buf_size;
> +
> + while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
> + if (*curr == '\n') /* empty line */
> + return curr - buf;
> + curr += strlen(curr);
>   }
> +
> + return -1; /* EOF or no space left in buf. */
>  }
>  
>  static int compare_txt(struct block_list *l1, struct block_list *l2)
> @@ -84,10 +75,12 @@ static void add_list(char *buf, int len)
>   }
>  }
>  
> +#define BUF_SIZE 1024
> +
>  int main(int argc, char **argv)
>  {
>   FILE *fin, *fout;
> - char buf[1024];
> + char buf[BUF_SIZE];
>   int ret, i, count;
>   struct block_list *list2;
>   struct stat st;
> @@ -106,11 +99,10 @@ int main(int argc, char **argv)
>   list = malloc(max_size * sizeof(*list));
>  
>   for(;;) {
> - ret = read_block(buf, fin);
> + ret = read_block(buf, BUF_SIZE, fin);
>   if (ret < 0)
>   break;
>  
> - buf[ret] = '\0';
>   add_list(buf, ret);
>   }
>  
> -- 
> 1.7.9.5
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgprD7KpwW2uI.pgp
Description: PGP signature

Re: [PATCH v2 2/2] Enhance read_block of page_owner.c

[Michal Nazarewicz]

On Mon, Jan 14 2013, Minchan Kim  wrote:
> I'm not familar with Python but I can see the point of the program.
> It's very short and good for maintainace but I have a concern about the size.
> For working it in embedded side, we have to port python in that
> machine. :(  [...]
> In case of that, just small C program when we release product would be
> good choice.

But is this program intended to be used as is? Or rather to serve as an
example?  If the former, than I think it should be in tools/ rather than
in Documentation/.  If the latter, than I think it does not really
matter whether it's C or some scripting language, since the purpose is
to show how /proc/page_owner can be used, and in fact showing the
general idea may be simpler with a shorter program which does not have
to deal with memory management.

And if Python is not your fancy, you can always use some shell: ;)

awk -vRS= '{ gsub("\n", "\\n"); print $0 }' |sort |uniq -c

> But I'm not strong aginst on your simple python program. If it is merged,
> we will just continue to use C program instead of python's one.
> If you have a strong opinion, send it to akpm as separate patch.

Not at all.  I'm just throwing ideas.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpHhPIzZy2ZG.pgp
Description: PGP signature

Re: [PATCH v2] mm: remove MIGRATE_ISOLATE check in hotpath

[Michal Nazarewicz]

On Tue, Jan 15 2013, Minchan Kim wrote:
> Now mm several functions test MIGRATE_ISOLATE and some of those
> are hotpath but MIGRATE_ISOLATE is used only if we enable
> CONFIG_MEMORY_ISOLATION(ie, CMA, memory-hotplug and memory-failure)
> which are not common config option. So let's not add unnecessary
> overhead and code when we don't enable CONFIG_MEMORY_ISOLATION.
>
> Cc: KOSAKI Motohiro 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> Signed-off-by: Minchan Kim 
> ---
>  include/linux/mmzone.h |2 ++
>  include/linux/page-isolation.h |   19 +++
>  mm/compaction.c|6 +-
>  mm/page_alloc.c|   16 ++--
>  mm/vmstat.c|2 ++
>  5 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 73b64a3..4f4c8c2 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -57,7 +57,9 @@ enum {
>*/
>   MIGRATE_CMA,
>  #endif
> +#ifdef CONFIG_MEMORY_ISOLATION
>   MIGRATE_ISOLATE,/* can't allocate from here */
> +#endif
>   MIGRATE_TYPES
>  };
>  
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index a92061e..3fff8e7 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -1,6 +1,25 @@
>  #ifndef __LINUX_PAGEISOLATION_H
>  #define __LINUX_PAGEISOLATION_H
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +static inline bool is_migrate_isolate_page(struct page *page)
> +{
> + return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
> +}
> +static inline bool is_migrate_isolate(int migratetype)
> +{
> + return migratetype == MIGRATE_ISOLATE;
> +}
> +#else
> +static inline bool is_migrate_isolate_page(struct page *page)
> +{
> + return false;
> +}
> +static inline bool is_migrate_isolate(int migratetype)
> +{
> + return false;
> +}
> +#endif
>  
>  bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
>bool skip_hwpoisoned_pages);
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 675937c..bb2a655 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "internal.h"
>  
>  #ifdef CONFIG_COMPACTION
> @@ -215,7 +216,10 @@ static bool suitable_migration_target(struct page *page)
>   int migratetype = get_pageblock_migratetype(page);
>  
>   /* Don't interfere with memory hot-remove or the min_free_kbytes blocks 
> */
> - if (migratetype == MIGRATE_ISOLATE || migratetype == MIGRATE_RESERVE)
> + if (migratetype == MIGRATE_RESERVE)
> + return false;
> +
> + if (is_migrate_isolate(migratetype))
>   return false;
>  
>   /* If the page is a large free page, then allow migration */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 82117f5..319a8f0 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -665,7 +665,7 @@ static void free_pcppages_bulk(struct zone *zone, int 
> count,
>   /* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
>   __free_one_page(page, zone, 0, mt);
>   trace_mm_page_pcpu_drain(page, 0, mt);
> - if (likely(get_pageblock_migratetype(page) != 
> MIGRATE_ISOLATE)) {
> + if (likely(!is_migrate_isolate_page(page))) {
>   __mod_zone_page_state(zone, NR_FREE_PAGES, 1);
>   if (is_migrate_cma(mt))
>   __mod_zone_page_state(zone, 
> NR_FREE_CMA_PAGES, 1);
> @@ -683,7 +683,7 @@ static void free_one_page(struct zone *zone, struct page 
> *page, int order,
>   zone->pages_scanned = 0;
>  
>   __free_one_page(page, zone, order, migratetype);
> - if (unlikely(migratetype != MIGRATE_ISOLATE))
> + if (unlikely(!is_migrate_isolate(migratetype)))
>   __mod_zone_freepage_state(zone, 1 << order, migratetype);
>   spin_unlock(&zone->lock);
>  }
> @@ -911,7 +911,9 @@ static int fallbacks[MIGRATE_TYPES][4] = {
>   [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE,   
> MIGRATE_RESERVE },
>  #endif
>   [MIGRATE_RESERVE] = { MIGRATE_RESERVE }, /* Never used */
> +#ifdef CONFIG_MEMORY_ISOLATION
>   [MIGRATE_ISOLATE] = { MIGRATE_RESERVE }, /* Never used */
> +#endif
>  };
>  
>  /*
> @@ -1137,7 +1139,7 @@ static int rmqueue_bulk(struct zone *zone, unsigned int 
> order,
>   

Re: [PATCH] mm: compaction: fix echo 1 > compact_memory return error issue

[Michal Nazarewicz]

On Sun, Jan 06 2013, Simon Jeons wrote:
>> >write(1, "1\n", 2)  = 3

>> Here it tells it.  

> On Sun, 2013-01-06 at 08:48 +, Liu Hui-R64343 wrote:
> Why this value trouble you?

Because write() is supposed to return the number of bytes successfully
written.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp4yAFLMATWi.pgp
Description: PGP signature

Re: [PATCH] usb: gadget: FunctionFS: Fix missing braces in parse_opts

[Michal Nazarewicz]

On Wed, Jan 09 2013, Benoit Goby  wrote:
> Add missing braces around an if block in ffs_fs_parse_opts. This broke
> parsing the uid/gid mount options and causes mount to fail when using
> uid/gid. This has been introduced by commit b9b73f7c (userns: Convert usb
> functionfs to use kuid/kgid where appropriate) in 3.7.
>
> Signed-off-by: Benoit Goby 

Acked-by: Michal Nazarewicz 
Cc: sta...@kernel.org

> ---
>  drivers/usb/gadget/f_fs.c |6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 4a6961c..8c2f251 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -1153,15 +1153,15 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data 
> *data, char *opts)
>   pr_err("%s: unmapped value: %lu\n", 
> opts, value);
>   return -EINVAL;
>   }
> - }
> - else if (!memcmp(opts, "gid", 3))
> + } else if (!memcmp(opts, "gid", 3)) {
>   data->perms.gid = make_kgid(current_user_ns(), 
> value);
>   if (!gid_valid(data->perms.gid)) {
>   pr_err("%s: unmapped value: %lu\n", 
> opts, value);
>   return -EINVAL;
>   }
> - else
> + } else {
>   goto invalid;
> + }
>   break;
>  
>   default:

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgp3oJjhdjUty.pgp
Description: PGP signature

[PATCH 1/2] usb: gadget: FunctionFS: Use kstrtoul()

[Michal Nazarewicz]

From: Michal Nazarewicz 

kstrtoul() checks for overflow which simple_strtoul() does not pluss
it has “*end == 0” check in it as well.  As a side effect, a new
line character is now accepted, but this should not be an issue.

Signed-off-by: Michal Nazarewicz 
---
 Patch on top of v3.5 with Benoit Goby's “Fix missing braces in
 parse_opts” patch
 () applied.

 drivers/usb/gadget/f_fs.c |5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 8c2f251..38388d7 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1103,8 +1103,8 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data 
*data, char *opts)
return 0;
 
for (;;) {
-   char *end, *eq, *comma;
unsigned long value;
+   char *eq, *comma;
 
/* Option limit */
comma = strchr(opts, ',');
@@ -1120,8 +1120,7 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data 
*data, char *opts)
*eq = 0;
 
/* Parse value */
-   value = simple_strtoul(eq + 1, &end, 0);
-   if (unlikely(*end != ',' && *end != 0)) {
+   if (kstrtoul(eq + 1, 0, &value)) {
pr_err("%s: invalid value: %s\n", opts, eq + 1);
return -EINVAL;
}
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] usb: gadget: FunctionFS: Refactor option parsing

[Michal Nazarewicz]

From: Michal Nazarewicz 

The use of memcmp() is clever and all and maybe even it makes parsing a
bit faster (since only options with given length need to be checked) but
option parsing is hardly a critical path and the additional code
complexity is not worth it.

Signed-off-by: Michal Nazarewicz 
---
 drivers/usb/gadget/f_fs.c |   55 ++--
 1 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 38388d7..6a7e187 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -1126,45 +1126,26 @@ static int ffs_fs_parse_opts(struct ffs_sb_fill_data 
*data, char *opts)
}
 
/* Interpret option */
-   switch (eq - opts) {
-   case 5:
-   if (!memcmp(opts, "rmode", 5))
-   data->root_mode  = (value & 0555) | S_IFDIR;
-   else if (!memcmp(opts, "fmode", 5))
-   data->perms.mode = (value & 0666) | S_IFREG;
-   else
-   goto invalid;
-   break;
-
-   case 4:
-   if (!memcmp(opts, "mode", 4)) {
-   data->root_mode  = (value & 0555) | S_IFDIR;
-   data->perms.mode = (value & 0666) | S_IFREG;
-   } else {
-   goto invalid;
+   if (!strcmp(opts, "rmode")) {
+   data->root_mode  = (value & 0555) | S_IFDIR;
+   } else if (!strcmp(opts, "fmode")) {
+   data->perms.mode = (value & 0666) | S_IFREG;
+   } else if (!strcmp(opts, "mode")) {
+   data->root_mode  = (value & 0555) | S_IFDIR;
+   data->perms.mode = (value & 0666) | S_IFREG;
+   } else if (!strcmp(opts, "uid")) {
+   data->perms.uid = make_kuid(current_user_ns(), value);
+   if (!uid_valid(data->perms.uid)) {
+   pr_err("%s: unmapped value: %lu\n", opts, 
value);
+   return -EINVAL;
}
-   break;
-
-   case 3:
-   if (!memcmp(opts, "uid", 3)) {
-   data->perms.uid = make_kuid(current_user_ns(), 
value);
-   if (!uid_valid(data->perms.uid)) {
-   pr_err("%s: unmapped value: %lu\n", 
opts, value);
-   return -EINVAL;
-   }
-   } else if (!memcmp(opts, "gid", 3)) {
-   data->perms.gid = make_kgid(current_user_ns(), 
value);
-   if (!gid_valid(data->perms.gid)) {
-   pr_err("%s: unmapped value: %lu\n", 
opts, value);
-   return -EINVAL;
-   }
-   } else {
-   goto invalid;
+   } else if (!strcmp(opts, "gid")) {
+   data->perms.gid = make_kgid(current_user_ns(), value);
+   if (!gid_valid(data->perms.gid)) {
+   pr_err("%s: unmapped value: %lu\n", opts, 
value);
+   return -EINVAL;
}
-   break;
-
-   default:
-invalid:
+   } else {
pr_err("%s: invalid option\n", opts);
return -EINVAL;
}
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix wrong EOF compare

[Michal Nazarewicz]

On Thu, Jan 10 2013, Minchan Kim  wrote:
> getc returns "int" so EOF could be -1 but storing getc's return
> value to char directly makes the vaule to 255 so below condition
> is always false.

Technically, this is implementation defined and I believe on many
systems char is signed thus the loop will end on EOF or byte 255.

Either way, my point is the patch is correct, but the comment is not. ;)

Of course, even better if the function just used fgets(), ie. something
like:

int read_block(char *buf, int buf_size, FILE *fin)
{
char *curr = buf, *const buf_end = buf + buf_size;

while (buf_end - curr > 1 && fgets(curr, buf_end - curr, fin)) {
if (*curr == '\n') /* empty line */
return curr - buf;
curr += strlen(curr);
}

return -1; /* EOF or no space left in buf. */
}

which is much shorter and does not have buffer overflow issues.

> It happens in my ARM system so loop is not ended, then segfaulted.
> This patch fixes it.
>
> *curr = getc(fin); // *curr = 255
> if (*curr == EOF) return -1; // if ( 255 == -1)
>
> Cc: Mel Gorman 
> Cc: Andy Whitcroft 
> Cc: Alexander Nyberg 
> Signed-off-by: Minchan Kim 
> ---
>  Documentation/page_owner.c |6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/page_owner.c b/Documentation/page_owner.c
> index f0156e1..b777fb6 100644
> --- a/Documentation/page_owner.c
> +++ b/Documentation/page_owner.c
> @@ -32,12 +32,14 @@ int read_block(char *buf, FILE *fin)
>  {
>   int ret = 0;
>   int hit = 0;
> + int vaule;
>   char *curr = buf;
>  
>   for (;;) {
> - *curr = getc(fin);
> - if (*curr == EOF) return -1;
> + value = getc(fin);
> + if (value == EOF) return -1;
>  
> + *curr = value;
>   ret++;
>   if (*curr == '\n' && hit == 1)
>   return ret - 1;

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--

pgpDZLemBEghh.pgp
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: use complete() instead complete_all()

[Michal Nazarewicz]

On Thu, Sep 22 2016, Daniel Wagner wrote:
> From: Daniel Wagner 
>
> There is only one waiter for the completion, therefore there
> is no need to use complete_all(). Let's make that clear by
> using complete() instead of complete_all().
>
> The usage pattern of the completion is:
>
> waiter context  waker context
>   reinit_completion()
>   usb_esp_queue()
>   wait_for_completion_interruptible()
>
>   ffs_ep0_complete()
> complete()
>
> Signed-off-by: Daniel Wagner 
> Cc: Felipe Balbi 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 5c8429f..0c29039 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -211,7 +211,7 @@ static void ffs_ep0_complete(struct usb_ep *ep, struct 
> usb_request *req)
>  {
>   struct ffs_data *ffs = req->context;
>  
> - complete_all(&ffs->ep0req_completion);
> + complete(&ffs->ep0req_completion);
>  }
>  
>  static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
> -- 
> 2.7.4

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 4/9] usb: gadget: f_midi: defaults buflen sizes to 512

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> 512 is the value used by wMaxPacketSize, as specified by the USB Spec. This
> makes sure this driver uses, by default, the most optimal value for IN and OUT
> endpoint requests.
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_midi.c | 2 +-
>  drivers/usb/gadget/legacy/gmidi.c| 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 39018dea7035..a7b50ac947f8 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1122,7 +1122,7 @@ static struct usb_function_instance 
> *f_midi_alloc_inst(void)
>   opts->func_inst.free_func_inst = f_midi_free_inst;
>   opts->index = SNDRV_DEFAULT_IDX1;
>   opts->id = SNDRV_DEFAULT_STR1;
> - opts->buflen = 256;
> + opts->buflen = 512;
>   opts->qlen = 32;
>   opts->in_ports = 1;
>   opts->out_ports = 1;
> diff --git a/drivers/usb/gadget/legacy/gmidi.c 
> b/drivers/usb/gadget/legacy/gmidi.c
> index fc2ac150f5ff..0bf39c3ccdb1 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -47,7 +47,7 @@ static char *id = SNDRV_DEFAULT_STR1;
>  module_param(id, charp, S_IRUGO);
>  MODULE_PARM_DESC(id, "ID string for the USB MIDI Gadget adapter.");
>  
> -static unsigned int buflen = 256;
> +static unsigned int buflen = 512;
>  module_param(buflen, uint, S_IRUGO);
>  MODULE_PARM_DESC(buflen, "MIDI buffer length");
>  
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> USB spec specifies wMaxPacketSize to be little endian (as other properties),
> so when using this variable in the driver we should convert to the current
> CPU endianness if necessary.
>
> This patch also introduces usb_ep_align() which does always returns the
> aligned buffer size for an endpoint. This is useful to be used by USB requests
> allocator functions.
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  include/linux/usb/gadget.h | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 612dbdfa388e..b8fa6901b881 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -418,8 +418,20 @@ static inline struct usb_gadget 
> *dev_to_usb_gadget(struct device *dev)
>   list_for_each_entry(tmp, &(gadget)->ep_list, ep_list)
>  
>  /**
> + * usb_ep_align - returns @len aligned to ep's maxpacketsize.
> + * @ep: the endpoint whose maxpacketsize is used to align @len
> + * @len: buffer size's length to align to @ep's maxpacketsize
> + *
> + * This helper is used to align buffer's size to an ep's maxpacketsize.
> + */
> +static inline size_t usb_ep_align(struct usb_ep *ep, size_t len)
> +{
> + return round_up(len, (size_t)le16_to_cpu(ep->desc->wMaxPacketSize));
> +}
> +
> +/**
>   * usb_ep_align_maybe - returns @len aligned to ep's maxpacketsize if gadget
> - *   requires quirk_ep_out_aligned_size, otherwise reguens len.
> + *   requires quirk_ep_out_aligned_size, otherwise returns len.
>   * @g: controller to check for quirk
>   * @ep: the endpoint whose maxpacketsize is used to align @len
>   * @len: buffer size's length to align to @ep's maxpacketsize
> @@ -430,8 +442,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct 
> device *dev)
>  static inline size_t
>  usb_ep_align_maybe(struct usb_gadget *g, struct usb_ep *ep, size_t len)
>  {
> - return !g->quirk_ep_out_aligned_size ? len :
> - round_up(len, (size_t)ep->desc->wMaxPacketSize);
> + return !g->quirk_ep_out_aligned_size ? len : usb_ep_align(ep, len);

I’d drop the negation:

+   return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;

>  }
>  
>  /**
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
> always aligned with wMaxPacketSize (512 usually). This makes sure
> that no buffer has the wrong size, which can cause nasty bugs.
>
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/u_f.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
> index 4bc7eea8bfc8..d1933b0b76c3 100644
> --- a/drivers/usb/gadget/u_f.c
> +++ b/drivers/usb/gadget/u_f.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "u_f.h"
> +#include 
>  
>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
>  {
> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 
> len, int default_len)
>   req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>   if (req) {
>   req->length = len ?: default_len;
> + if (usb_endpoint_dir_out(ep->desc))
> + req->length = usb_ep_align(ep, req->length);
>   req->buf = kmalloc(req->length, GFP_ATOMIC);
>   if (!req->buf) {
>   usb_ep_free_request(ep, req);

I’m a bit scared of this change.

Drivers which call alloc_ep_req and then ignore req->length using the
same length they passed to the function will silently drop data.

Drivers which do not ignore req->length may end up overwriting some
other buffer, e.g.:

some_buffer = kmalloc(length, GFP_KERNEL);
req = alloc_ep_req(ep, length, 0);
… later …
memcpy(some_buffer, req->buf, req->length);

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> This parameter was not really necessary and gadget drivers would almost always

I personally like when commit messages can be read without subject, so
perhaps:

The default_length parameter of alloc_ep_req was not …

But that’s just me.

> create an inline function to pass the same value to len and default_len.
>
> So this patch also removes duplicate code from few drivers.
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_hid.c| 10 ++
>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>  drivers/usb/gadget/u_f.c   |  7 +++
>  drivers/usb/gadget/u_f.h   |  2 +-
>  6 files changed, 11 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index 51980c50546d..e82a7468252e 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
> *fd)
>  /*-*/
>  /*usb_function */
>  
> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
> *req)
>  {
>   struct f_hidg *hidg = (struct f_hidg *) req->context;
> @@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
> intf, unsigned alt)
>*/
>   for (i = 0; i < hidg->qlen && status == 0; i++) {
>   struct usb_request *req =
> - hidg_alloc_ep_req(hidg->out_ep,
> -   hidg->report_length);
> + alloc_ep_req(hidg->out_ep,
> + hidg->report_length);
>   if (req) {
>   req->complete = hidg_set_report_complete;
>   req->context  = hidg;
> diff --git a/drivers/usb/gadget/function/f_loopback.c 
> b/drivers/usb/gadget/function/f_loopback.c
> index 3a9f8f9c77bd..701ee0f11c33 100644
> --- a/drivers/usb/gadget/function/f_loopback.c
> +++ b/drivers/usb/gadget/function/f_loopback.c
> @@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
>   VDBG(cdev, "%s disabled\n", loop->function.name);
>  }
>  
> -static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
> -{
> - struct f_loopback   *loop = ep->driver_data;
> -
> - return alloc_ep_req(ep, len, loop->buflen);
> -}
> -
>  static int alloc_requests(struct usb_composite_dev *cdev,
> struct f_loopback *loop)
>  {
> @@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
>   if (!in_req)
>   goto fail;
>  
> - out_req = lb_alloc_ep_req(loop->out_ep, 0);
> + out_req = alloc_ep_req(loop->out_ep, loop->buflen);
>   if (!out_req)
>   goto fail_in;
>  
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 3a47596afcab..abf26364b46f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
>   NULL,
>  };
>  
> -static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static const uint8_t f_midi_cin_length[] = {
>   0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>  };
> @@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   /* pre-allocate write usb requests to use on f_midi_transmit. */
>   while (kfifo_avail(&midi->in_req_fifo)) {
>   struct usb_request *req =
> - midi_alloc_ep_req(midi->in_ep, midi->buflen);
> + alloc_ep_req(midi->in_ep, midi->buflen);
>  
>   if (req == NULL)
>   return -

Re: [PATCH 8/9] usb: gadget: f_hid: use free_ep_req()

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> We should always use free_ep_req() when allocating requests with
> alloc_ep_req().
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_hid.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index e82a7468252e..a010496e4e05 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -671,11 +671,8 @@ fail_free_descs:
>   usb_free_all_descriptors(f);
>  fail:
>   ERROR(f->config->cdev, "hidg_bind FAILED\n");
> - if (hidg->req != NULL) {
> - kfree(hidg->req->buf);
> - if (hidg->in_ep != NULL)
> - usb_ep_free_request(hidg->in_ep, hidg->req);
> - }
> + if (hidg->req != NULL)
> + free_ep_req(hidg->in_ep, hidg->req);
>  
>   return status;
>  }
> @@ -904,8 +901,7 @@ static void hidg_unbind(struct usb_configuration *c, 
> struct usb_function *f)
>  
>   /* disable/free request and end point */
>   usb_ep_disable(hidg->in_ep);
> - kfree(hidg->req->buf);
> - usb_ep_free_request(hidg->in_ep, hidg->req);
> + free_ep_req(hidg->in_ep, hidg->req);
>  
>   usb_free_all_descriptors(f);
>  }
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH 9/9] usb: gadget: f_hid: use alloc_ep_req()

[Michal Nazarewicz]

On Tue, Jul 26 2016, Felipe F. Tonello wrote:
> Use gadget's framework allocation function instead of directly calling
> usb_ep_alloc_request().
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_hid.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index a010496e4e05..89d2e9a5a04f 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
> struct usb_function *f)
>  
>   /* preallocate request and buffer */
>   status = -ENOMEM;
> - hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
> + hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>   if (!hidg->req)
>   goto fail;
>  
> - hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
> - if (!hidg->req->buf)
> - goto fail;
> -
>   /* set descriptor dynamic values */
>   hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>   hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH v3 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Michal Nazarewicz]

On Fri, Aug 05 2016, Felipe F. Tonello wrote:
> USB spec specifies wMaxPacketSize to be little endian (as other properties),
> so when using this variable in the driver we should convert to the current
> CPU endianness if necessary.
>
> This patch also introduces usb_ep_align() which does always returns the
> aligned buffer size for an endpoint. This is useful to be used by USB requests
> allocator functions.
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  include/linux/usb/gadget.h | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 612dbdfa388e..3cc93237ff98 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -418,8 +418,20 @@ static inline struct usb_gadget 
> *dev_to_usb_gadget(struct device *dev)
>   list_for_each_entry(tmp, &(gadget)->ep_list, ep_list)
>  
>  /**
> + * usb_ep_align - returns @len aligned to ep's maxpacketsize.
> + * @ep: the endpoint whose maxpacketsize is used to align @len
> + * @len: buffer size's length to align to @ep's maxpacketsize
> + *
> + * This helper is used to align buffer's size to an ep's maxpacketsize.
> + */
> +static inline size_t usb_ep_align(struct usb_ep *ep, size_t len)
> +{
> + return round_up(len, (size_t)le16_to_cpu(ep->desc->wMaxPacketSize));
> +}
> +
> +/**
>   * usb_ep_align_maybe - returns @len aligned to ep's maxpacketsize if gadget
> - *   requires quirk_ep_out_aligned_size, otherwise reguens len.
> + *   requires quirk_ep_out_aligned_size, otherwise returns len.
>   * @g: controller to check for quirk
>   * @ep: the endpoint whose maxpacketsize is used to align @len
>   * @len: buffer size's length to align to @ep's maxpacketsize
> @@ -430,8 +442,7 @@ static inline struct usb_gadget *dev_to_usb_gadget(struct 
> device *dev)
>  static inline size_t
>  usb_ep_align_maybe(struct usb_gadget *g, struct usb_ep *ep, size_t len)
>  {
> - return !g->quirk_ep_out_aligned_size ? len :
> - round_up(len, (size_t)ep->desc->wMaxPacketSize);
> + return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;
>  }
>  
>  /**
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH v3 4/9] usb: gadget: f_midi: defaults buflen sizes to 512

[Michal Nazarewicz]

On Fri, Aug 05 2016, Felipe F. Tonello wrote:
> 512 is the value used by wMaxPacketSize, as specified by the USB Spec. This
> makes sure this driver uses, by default, the most optimal value for IN and OUT
> endpoint requests.
>
> Signed-off-by: Felipe F. Tonello 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_midi.c | 2 +-
>  drivers/usb/gadget/legacy/gmidi.c| 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 39018dea7035..a7b50ac947f8 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1122,7 +1122,7 @@ static struct usb_function_instance 
> *f_midi_alloc_inst(void)
>   opts->func_inst.free_func_inst = f_midi_free_inst;
>   opts->index = SNDRV_DEFAULT_IDX1;
>   opts->id = SNDRV_DEFAULT_STR1;
> - opts->buflen = 256;
> + opts->buflen = 512;
>   opts->qlen = 32;
>   opts->in_ports = 1;
>   opts->out_ports = 1;
> diff --git a/drivers/usb/gadget/legacy/gmidi.c 
> b/drivers/usb/gadget/legacy/gmidi.c
> index fc2ac150f5ff..0bf39c3ccdb1 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -47,7 +47,7 @@ static char *id = SNDRV_DEFAULT_STR1;
>  module_param(id, charp, S_IRUGO);
>  MODULE_PARM_DESC(id, "ID string for the USB MIDI Gadget adapter.");
>  
> -static unsigned int buflen = 256;
> +static unsigned int buflen = 512;
>  module_param(buflen, uint, S_IRUGO);
>  MODULE_PARM_DESC(buflen, "MIDI buffer length");
>  
> -- 
> 2.9.0
>

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [RFC PATCH 3/3] mm/map_contig: Add mmap(MAP_CONTIG) support

[Michal Nazarewicz]

On Sun, Oct 15 2017, Guy Shattah wrote:
> Why have several driver specific implementation if you can generalize
> the idea and implement an already existing POSIX standard?

Why is there a need for contiguous allocation?

CPU cares only to the point of huge pages and there’s already an effort
in the kernel to allocate huge pages transparently without user space
being aware of it.

If not CPU than various devices all of which may have very different
needs.  Some may be behind an IO MMU.  Some may support DMA.  Some may
indeed require physically continuous memory.  How is user space to know?

Furthermore, user space does not care whether allocation is physically
contiguous or not.  What it cares about is whether given allocation can
be passed as a buffer to a particular device.

If generalisation is the issue, then the solution is to define a common
API where user-space can allocate memory *in the context of* a device.
This provides a ‘give me memory I can use for this device’ request which
is what user space really wants.

So yeah, like others in this thread, the reason for this change alludes
me.  On the other hand, I don’t care much so I’ll limit myself to this
one message.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [RFC PATCH 3/3] mm/map_contig: Add mmap(MAP_CONTIG) support

[Michal Nazarewicz]

On Tue, Oct 17 2017, Guy Shattah wrote:
> Are you going to be OK with kernel API which implements contiguous
> memory allocation?  Possibly with mmap style?  Many drivers could
> utilize it instead of having their own weird and possibly non-standard
> way to allocate contiguous memory.  Such API won't be available for
> user space.

What you describe sounds like CMA.  It may be far from perfect but it’s
there already and drivers which need contiguous memory can allocate it.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] usb/gadget/snps_udc_core: Convert timers to use timer_setup()

[Michal Nazarewicz]

On Mon, Oct 16 2017, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> If the probe fails, udc_remove() will not be called, so there is no
> reason to make del_timer_sync() calls conditional. As a result, use of
> the .data field can be dropped, in support of making removing this field
> entirely from struct timer_list.
>
> Cc: Felipe Balbi 
> Cc: Greg Kroah-Hartman 
> Cc: Raviteja Garimella 
> Cc: Michal Nazarewicz 

Acked-by: Michal Nazarewicz 

> Cc: "Gustavo A. R. Silva" 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Kees Cook 
> ---
>  drivers/usb/gadget/udc/snps_udc_core.c | 19 ++-
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/snps_udc_core.c 
> b/drivers/usb/gadget/udc/snps_udc_core.c
> index 47df99dbaef4..2f5e788dd978 100644
> --- a/drivers/usb/gadget/udc/snps_udc_core.c
> +++ b/drivers/usb/gadget/udc/snps_udc_core.c
> @@ -1733,7 +1733,7 @@ static void udc_soft_reset(struct udc *dev)
>  }
>  
>  /* RDE timer callback to set RDE bit */
> -static void udc_timer_function(unsigned long v)
> +static void udc_timer_function(struct timer_list *unused)
>  {
>   u32 tmp;
>  
> @@ -1813,7 +1813,7 @@ static void udc_handle_halt_state(struct udc_ep *ep)
>  }
>  
>  /* Stall timer callback to poll S bit and set it again after */
> -static void udc_pollstall_timer_function(unsigned long v)
> +static void udc_pollstall_timer_function(struct timer_list *unused)
>  {
>   struct udc_ep *ep;
>   int halted = 0;
> @@ -3067,14 +3067,12 @@ void udc_remove(struct udc *dev)
>   stop_timer++;
>   if (timer_pending(&udc_timer))
>   wait_for_completion(&on_exit);
> - if (udc_timer.data)
> - del_timer_sync(&udc_timer);
> + del_timer_sync(&udc_timer);
>   /* remove pollstall timer */
>   stop_pollstall_timer++;
>   if (timer_pending(&udc_pollstall_timer))
>   wait_for_completion(&on_pollstall_exit);
> - if (udc_pollstall_timer.data)
> - del_timer_sync(&udc_pollstall_timer);
> + del_timer_sync(&udc_pollstall_timer);
>   udc = NULL;
>  }
>  EXPORT_SYMBOL_GPL(udc_remove);
> @@ -3164,10 +3162,6 @@ int udc_probe(struct udc *dev)
>   u32 reg;
>   int retval;
>  
> - /* mark timer as not initialized */
> - udc_timer.data = 0;
> - udc_pollstall_timer.data = 0;
> -
>   /* device struct setup */
>   dev->gadget.ops = &udc_ops;
>  
> @@ -3207,9 +3201,8 @@ int udc_probe(struct udc *dev)
>   goto finished;
>  
>   /* timer init */
> - setup_timer(&udc_timer, udc_timer_function, 1);
> - /* timer pollstall init */
> - setup_timer(&udc_pollstall_timer, udc_pollstall_timer_function, 1);
> + timer_setup(&udc_timer, udc_timer_function, 0);
> + timer_setup(&udc_pollstall_timer, udc_pollstall_timer_function, 0);
>  
>   /* set SD */
>   reg = readl(&dev->regs->ctl);
> -- 
> 2.7.4

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] usb: gadget: f_fs: use memdup_user

[Michal Nazarewicz]

On Sat, May 13 2017, Geliang Tang wrote:
> Use memdup_user() helper instead of open-coding to simplify the code.
>
> Signed-off-by: Geliang Tang 

Acked-by: Michal Nazarewicz 

> ---
>  drivers/usb/gadget/function/f_fs.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 71dd27c..5754538 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -3692,14 +3692,9 @@ static char *ffs_prepare_buffer(const char __user 
> *buf, size_t len)
>   if (unlikely(!len))
>   return NULL;
>  
> - data = kmalloc(len, GFP_KERNEL);
> - if (unlikely(!data))
> - return ERR_PTR(-ENOMEM);
> -
> - if (unlikely(copy_from_user(data, buf, len))) {
> - kfree(data);
> - return ERR_PTR(-EFAULT);
> - }
> + data = memdup_user(buf, len);
> + if (unlikely(IS_ERR(data)))
> + return data;
>  
>   pr_vdebug("Buffer from user space:\n");
>   ffs_dump_mem("", data, len);

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

Re: [PATCH] CMA: add the amount of cma memory in meminfo

[Michal Nazarewicz]

On Thu, Dec 04 2014, Xishi Qiu  wrote:
> Add the amount of cma memory in the following meminfo.
> /proc/meminfo
> /sys/devices/system/node/nodeXX/meminfo
>
> Signed-off-by: Xishi Qiu 
> ---
>  drivers/base/node.c | 16 ++--
>  fs/proc/meminfo.c   | 12 +---
>  2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 472168c..a27e4e0 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -120,6 +120,9 @@ static ssize_t node_read_meminfo(struct device *dev,
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  "Node %d AnonHugePages:  %8lu kB\n"
>  #endif
> +#ifdef CONFIG_CMA
> +"Node %d FreeCMAPages:   %8lu kB\n"
> +#endif
>   ,
>  nid, K(node_page_state(nid, NR_FILE_DIRTY)),
>  nid, K(node_page_state(nid, NR_WRITEBACK)),
> @@ -136,14 +139,15 @@ static ssize_t node_read_meminfo(struct device *dev,
>  nid, K(node_page_state(nid, NR_SLAB_RECLAIMABLE) +
>   node_page_state(nid, NR_SLAB_UNRECLAIMABLE)),
>  nid, K(node_page_state(nid, NR_SLAB_RECLAIMABLE)),
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  nid, K(node_page_state(nid, NR_SLAB_UNRECLAIMABLE))

Why is this line suddenly out of “#ifdef CONFIG_TRANSPARENT_HUGEPAGE”?

> - , nid,
> - K(node_page_state(nid, NR_ANON_TRANSPARENT_HUGEPAGES) *
> - HPAGE_PMD_NR));
> -#else
> -nid, K(node_page_state(nid, NR_SLAB_UNRECLAIMABLE)));
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +, nid, K(node_page_state(nid,
> + NR_ANON_TRANSPARENT_HUGEPAGES) * HPAGE_PMD_NR)

This is mere white-space change which is confusing.

> +#endif
> +#ifdef CONFIG_CMA
> +, nid, K(node_page_state(nid, NR_FREE_CMA_PAGES))
>  #endif
> + );
>   n += hugetlb_report_node_meminfo(nid, buf + n);
>   return n;
>  }
> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
> index aa1eee0..d42e082 100644
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -138,6 +138,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   "AnonHugePages:  %8lu kB\n"
>  #endif
> +#ifdef CONFIG_CMA
> + "FreeCMAPages:   %8lu kB\n"
> +#endif
>   ,
>   K(i.totalram),
>   K(i.freeram),
> @@ -187,11 +190,14 @@ static int meminfo_proc_show(struct seq_file *m, void 
> *v)
>   vmi.used >> 10,
>   vmi.largest_chunk >> 10
>  #ifdef CONFIG_MEMORY_FAILURE
> - ,atomic_long_read(&num_poisoned_pages) << (PAGE_SHIFT - 10)
> + , atomic_long_read(&num_poisoned_pages) << (PAGE_SHIFT - 10)
>  #endif
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - ,K(global_page_state(NR_ANON_TRANSPARENT_HUGEPAGES) *
> -HPAGE_PMD_NR)
> + , K(global_page_state(NR_ANON_TRANSPARENT_HUGEPAGES) *
> + HPAGE_PMD_NR)
> +#endif

Again, please don't include white space changes.  They are confusing.

> +#ifdef CONFIG_CMA
> + , K(global_page_state(NR_FREE_CMA_PAGES))
>  #endif
>   );
>  
> -- 
> 2.0.0
>
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] CMA: add the amount of cma memory in meminfo

[Michal Nazarewicz]

On Thu, Dec 04 2014, Xishi Qiu  wrote:
> Add the amount of cma memory in the following meminfo.
> /proc/meminfo
> /sys/devices/system/node/nodeXX/meminfo
>
> Signed-off-by: Xishi Qiu 

No second look:

Acked-by: Michal Nazarewicz 

> ---
>  drivers/base/node.c | 16 ++--
>  fs/proc/meminfo.c   | 12 +---
>  2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 472168c..a27e4e0 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -120,6 +120,9 @@ static ssize_t node_read_meminfo(struct device *dev,
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  "Node %d AnonHugePages:  %8lu kB\n"
>  #endif
> +#ifdef CONFIG_CMA
> +"Node %d FreeCMAPages:   %8lu kB\n"
> +#endif
>   ,
>  nid, K(node_page_state(nid, NR_FILE_DIRTY)),
>  nid, K(node_page_state(nid, NR_WRITEBACK)),
> @@ -136,14 +139,15 @@ static ssize_t node_read_meminfo(struct device *dev,
>  nid, K(node_page_state(nid, NR_SLAB_RECLAIMABLE) +
>   node_page_state(nid, NR_SLAB_UNRECLAIMABLE)),
>  nid, K(node_page_state(nid, NR_SLAB_RECLAIMABLE)),
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  nid, K(node_page_state(nid, NR_SLAB_UNRECLAIMABLE))
> - , nid,
> - K(node_page_state(nid, NR_ANON_TRANSPARENT_HUGEPAGES) *
> - HPAGE_PMD_NR));
> -#else
> -nid, K(node_page_state(nid, NR_SLAB_UNRECLAIMABLE)));
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +, nid, K(node_page_state(nid,
> + NR_ANON_TRANSPARENT_HUGEPAGES) * HPAGE_PMD_NR)
> +#endif
> +#ifdef CONFIG_CMA
> +, nid, K(node_page_state(nid, NR_FREE_CMA_PAGES))
>  #endif
> + );
>   n += hugetlb_report_node_meminfo(nid, buf + n);
>   return n;
>  }
> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
> index aa1eee0..d42e082 100644
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -138,6 +138,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   "AnonHugePages:  %8lu kB\n"
>  #endif
> +#ifdef CONFIG_CMA
> + "FreeCMAPages:   %8lu kB\n"
> +#endif
>   ,
>   K(i.totalram),
>   K(i.freeram),
> @@ -187,11 +190,14 @@ static int meminfo_proc_show(struct seq_file *m, void 
> *v)
>   vmi.used >> 10,
>   vmi.largest_chunk >> 10
>  #ifdef CONFIG_MEMORY_FAILURE
> - ,atomic_long_read(&num_poisoned_pages) << (PAGE_SHIFT - 10)
> + , atomic_long_read(&num_poisoned_pages) << (PAGE_SHIFT - 10)
>  #endif
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> - ,K(global_page_state(NR_ANON_TRANSPARENT_HUGEPAGES) *
> -HPAGE_PMD_NR)
> + , K(global_page_state(NR_ANON_TRANSPARENT_HUGEPAGES) *
> + HPAGE_PMD_NR)
> +#endif
> +#ifdef CONFIG_CMA
> + , K(global_page_state(NR_FREE_CMA_PAGES))
>  #endif
>   );
>  
> -- 
> 2.0.0
>
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] usb: gadget: ffs: Fix sparse error

[Michal Nazarewicz]

On Wed, Dec 24 2014, Rohith Seelaboyina wrote:
> This patch fixes the sparse error in functionfs
> driver.
>
> drivers/usb/gadget/function/f_fs.c:400:44: error: bad
> constant experssion.
>
> Dynamic memory allocation through kcalloc is more safer
> than declaring variable array size, Fix this error by
> using kcalloc for memory allocation, Check if memory
> allocation is successful and return -ENOMEM on failure.
>
> Signed-off-by: Rohith Seelaboyina 

This has already been addressed in a way that does not require dynamic
allocation: <https://lkml.org/lkml/2014/9/10/513>.  I'd rather patch
attached at the end would be applied.

> ---
>  drivers/usb/gadget/function/f_fs.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 63314ede7ba6..8af3f7906c83 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -397,10 +397,13 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data 
> *ffs, char __user *buf,
>* We are holding ffs->ev.waitq.lock and ffs->mutex and we need
>* to release them.
>*/
> - struct usb_functionfs_event events[n];
>   unsigned i = 0;
> + int ret;
> + struct usb_functionfs_event *events;
>  
> - memset(events, 0, sizeof events);
> + events = kcalloc(n, sizeof(*events), GFP_KERNEL);
> + if (!events)
> + return -ENOMEM;
>  
>   do {
>   events[i].type = ffs->ev.types[i];
> @@ -421,8 +424,10 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data 
> *ffs, char __user *buf,
>   spin_unlock_irq(&ffs->ev.waitq.lock);
>   mutex_unlock(&ffs->mutex);
>  
> - return unlikely(__copy_to_user(buf, events, sizeof events))
> - ? -EFAULT : sizeof events;
> + ret = unlikely(__copy_to_user(buf, events, n * sizeof(*events)))
> + ? -EFAULT : n * sizeof(*events);
> + kfree(events);
> + return ret;
>  }
>  
>  static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
> -- 
> 1.9.1
>

>From 8c813b2563cae23c69895833972da1c1ee92d8dd Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz 
Date: Wed, 10 Sep 2014 17:50:24 +0200
Subject: [PATCHv2] usb: f_fs: refactor and document __ffs_ep0_read_events better

Instead of using variable length array, use a static length equal to
the size of the ffs->ev.types array.  This gets rid of a sparse warning:

drivers/usb/gadget/function/f_fs.c:401:44: warning:
Variable length array is used.

and makes it more explicit that the array has a very tight upper size
limit.  Also add some more documentation about the ev.types array and
how its size is limited and affects the rest of the code.

Reported-by: Dan Carpenter 
Signed-off-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_fs.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 63314ed..a00ee97 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -390,17 +390,20 @@ done_spin:
return ret;
 }
 
+/* Called with ffs->ev.waitq.lock and ffs->mutex held, both released on exit. 
*/
 static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 size_t n)
 {
/*
-* We are holding ffs->ev.waitq.lock and ffs->mutex and we need
-* to release them.
+* n cannot be bigger than ffs->ev.count, which cannot be bigger than
+* size of ffs->ev.types array (which is four) so that's how much space
+* we reserve.
 */
-   struct usb_functionfs_event events[n];
+   struct usb_functionfs_event events[ARRAY_SIZE(ffs->ev.types)];
+   const size_t size = n * sizeof *events;
unsigned i = 0;
 
-   memset(events, 0, sizeof events);
+   memset(events, 0, size);
 
do {
events[i].type = ffs->ev.types[i];
@@ -410,19 +413,15 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data 
*ffs, char __user *buf,
}
} while (++i < n);
 
-   if (n < ffs->ev.count) {
-   ffs->ev.count -= n;
+   ffs->ev.count -= n;
+   if (ffs->ev.count)
memmove(ffs->ev.types, ffs->ev.types + n,
ffs->ev.count * sizeof *ffs->ev.types);
-   } else {
-   ffs->ev.count = 0;
-   }
 
spin_unlock_irq(&ffs->ev.waitq.lock);
mutex_unlock(&ffs->mutex);
 
-   return unlikely(__copy_to_user(buf, events, sizeof events))
-   ? -EFAULT : sizeof events;
+   re

Re: [PATCH v5] usb: gadget: f_fs: add "no_disconnect" mode

[Michal Nazarewicz]

On Thu, Dec 18 2014, Robert Baldyga wrote:
> Since we can compose gadgets from many functions, there is the problem
> related to gadget breakage while FunctionFS daemon being closed. FFS
> function is userspace code so there is no way to know when it will close
> files (it doesn't matter what is the reason of this situation, it can
> be daemon logic, program breakage, process kill or any other). So when
> we have another function in gadget which, for example, sends some amount
> of data, does some software update or implements some real-time functionality,
> we may want to keep the gadget connected despite FFS function is no longer
> functional.
>
> We can't just remove one of functions from gadget since it has been
> enumerated, so the only way to keep entire gadget working is to make
> broken FFS function deactivated but still visible to host. For this
> purpose this patch introduces "no_disconnect" mode. It can be enabled
> by setting mount option "no_disconnect=1", and results with defering
> function disconnect to the moment of reopen ep0 file or filesystem
> unmount. After closing all endpoint files, FunctionFS is set to state
> FFS_DEACTIVATED.
>
> When ffs->state == FFS_DEACTIVATED:
> - function is still bound and visible to host,
> - setup requests are automatically stalled,
> - transfers on other endpoints are refused,
> - epfiles, except ep0, are deleted from the filesystem,
> - opening ep0 causes the function to be closed, and then FunctionFS
>   is ready for descriptors and string write,
> - altsetting change causes the function to be closed - we want to keep
>   function alive until another functions are potentialy used, altsetting
>   change means that another configuration is being selected or USB cable
>   was unplugged, which indicates that we don't need to stay longer in
>   FFS_DEACTIVATED state
> - unmounting of the FunctionFS instance causes the function to be closed.
>
> Signed-off-by: Robert Baldyga 

Acked-by: Michal Nazarewicz 

> @@ -1417,7 +1429,11 @@ static void ffs_data_opened(struct ffs_data *ffs)
>   ENTER();
>  
>   atomic_inc(&ffs->ref);
> - atomic_inc(&ffs->opened);
> + if (atomic_add_return(1, &ffs->opened) == 1)
> + if (ffs->state == FFS_DEACTIVATED) {
> + ffs->state = FFS_CLOSING;
> + ffs_data_reset(ffs);
> + }

Wrong indention.  Why not:

+   if (atomic_add_return(1, &ffs->opened) == 1 &&
+   ffs->state == FFS_DEACTIVATED) {
+   ffs->state = FFS_CLOSING;
+   ffs_data_reset(ffs);
+   }

>  }
>  
>  static void ffs_data_put(struct ffs_data *ffs)
> @@ -1439,6 +1455,21 @@ static void ffs_data_closed(struct ffs_data *ffs)

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] mm: cma: introduce /proc/cmainfo

[Michal Nazarewicz]

On Fri, Dec 26 2014, "Stefan I. Strogin"  wrote:
> /proc/cmainfo contains a list of currently allocated CMA buffers for every
> CMA area when CONFIG_CMA_DEBUG is enabled.
>
> Format is:
>
>  -  ( kB), allocated by \
>   (), latency  us
>  
>
> Signed-off-by: Stefan I. Strogin 
> ---
>  mm/cma.c | 202 
> +++
>  1 file changed, 202 insertions(+)
>
> diff --git a/mm/cma.c b/mm/cma.c
> index a85ae28..ffaea26 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -34,6 +34,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  struct cma {
>   unsigned long   base_pfn;
> @@ -41,8 +45,25 @@ struct cma {
>   unsigned long   *bitmap;
>   unsigned int order_per_bit; /* Order of pages represented by one bit */
>   struct mutexlock;
> +#ifdef CONFIG_CMA_DEBUG
> + struct list_head buffers_list;
> + struct mutexlist_lock;
> +#endif
>  };
>  
> +#ifdef CONFIG_CMA_DEBUG
> +struct cma_buffer {
> + unsigned long pfn;
> + unsigned long count;
> + pid_t pid;
> + char comm[TASK_COMM_LEN];
> + unsigned int latency;
> + unsigned long trace_entries[16];
> + unsigned int nr_entries;
> + struct list_head list;
> +};
> +#endif
> +
>  static struct cma cma_areas[MAX_CMA_AREAS];
>  static unsigned cma_area_count;
>  static DEFINE_MUTEX(cma_mutex);
> @@ -132,6 +153,10 @@ static int __init cma_activate_area(struct cma *cma)
>   } while (--i);
>  
>   mutex_init(&cma->lock);
> +#ifdef CONFIG_CMA_DEBUG
> + INIT_LIST_HEAD(&cma->buffers_list);
> + mutex_init(&cma->list_lock);
> +#endif
>   return 0;
>  
>  err:
> @@ -347,6 +372,86 @@ err:
>   return ret;
>  }
>  
> +#ifdef CONFIG_CMA_DEBUG
> +/**
> + * cma_buffer_list_add() - add a new entry to a list of allocated buffers
> + * @cma: Contiguous memory region for which the allocation is performed.
> + * @pfn: Base PFN of the allocated buffer.
> + * @count:   Number of allocated pages.
> + * @latency: Nanoseconds spent to allocate the buffer.
> + *
> + * This function adds a new entry to the list of allocated contiguous memory
> + * buffers in a CMA area. It uses the CMA area specificated by the device
> + * if available or the default global one otherwise.
> + */
> +static int cma_buffer_list_add(struct cma *cma, unsigned long pfn,
> +int count, s64 latency)
> +{
> + struct cma_buffer *cmabuf;
> + struct stack_trace trace;
> +
> + cmabuf = kmalloc(sizeof(struct cma_buffer), GFP_KERNEL);

cmabuf = kmalloc(sizeof *cmabuf, GFP_KERNEL);

> + if (!cmabuf)
> + return -ENOMEM;
> +
> + trace.nr_entries = 0;
> + trace.max_entries = ARRAY_SIZE(cmabuf->trace_entries);
> + trace.entries = &cmabuf->trace_entries[0];
> + trace.skip = 2;
> + save_stack_trace(&trace);
> +
> + cmabuf->pfn = pfn;
> + cmabuf->count = count;
> + cmabuf->pid = task_pid_nr(current);
> + cmabuf->nr_entries = trace.nr_entries;
> + get_task_comm(cmabuf->comm, current);
> + cmabuf->latency = (unsigned int) div_s64(latency, NSEC_PER_USEC);
> +
> + mutex_lock(&cma->list_lock);
> + list_add_tail(&cmabuf->list, &cma->buffers_list);
> + mutex_unlock(&cma->list_lock);
> +
> + return 0;
> +}
> +
> +/**
> + * cma_buffer_list_del() - delete an entry from a list of allocated buffers
> + * @cma:   Contiguous memory region for which the allocation was performed.
> + * @pfn:   Base PFN of the released buffer.
> + *
> + * This function deletes a list entry added by cma_buffer_list_add().
> + */
> +static void cma_buffer_list_del(struct cma *cma, unsigned long pfn)
> +{
> + struct cma_buffer *cmabuf;
> +
> + mutex_lock(&cma->list_lock);
> +
> + list_for_each_entry(cmabuf, &cma->buffers_list, list)
> + if (cmabuf->pfn == pfn) {
> + list_del(&cmabuf->list);
> + kfree(cmabuf);
> + goto out;
> + }

You do not have guarantee that CMA deallocations will match allocations
exactly.  User may allocate CMA region and then free it chunks.  I'm not
saying that the debug code must handle than case but at least I would
like to see a comment describing this shortcoming.

> +
> + pr_err("%s(pfn %lu): couldn't find buffers list entry\n",
> +__func__, pfn);
> +
> +out:
> + mutex_unlock(&cma->list_lock);
> +}
> +#else
> +static int cma_buffer_list_add(struct cma *cma, unsigned long pfn,
> +int count, s64 latency)
> +{
> + return 0;
> +}
> +
> +static void cma_buffer_list_del(struct cma *cma, unsigned long pfn)
> +{
> +}
> +#endif /* CONFIG_CMA_DEBUG */
> +
>  /**
>   * cma_alloc() - allocate pages from contiguous area
>   * @cma:   Contiguous memory region for which the allocation is performed.
> @@ -361,11 +466,15 @@ struct page *cma_alloc(struct cma *cma, int count, 
> unsigned int align)
>   

Re: [PATCH 3/3] cma: add functions to get region pages counters

[Michal Nazarewicz]

On Fri, Dec 26 2014, "Stefan I. Strogin"  wrote:
> From: Dmitry Safonov 
>
> Here are two functions that provide interface to compute/get used size
> and size of biggest free chunk in cma region.
> Added that information in cmainfo.
>
> Signed-off-by: Dmitry Safonov 

Acked-by: Michal Nazarewicz 

> ---
>  include/linux/cma.h |  2 ++
>  mm/cma.c| 34 ++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index 9384ba6..855e6f2 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -18,6 +18,8 @@ struct cma;
>  extern unsigned long totalcma_pages;
>  extern phys_addr_t cma_get_base(struct cma *cma);
>  extern unsigned long cma_get_size(struct cma *cma);
> +extern unsigned long cma_get_used(struct cma *cma);
> +extern unsigned long cma_get_maxchunk(struct cma *cma);
>  
>  extern int __init cma_declare_contiguous(phys_addr_t base,
>   phys_addr_t size, phys_addr_t limit,
> diff --git a/mm/cma.c b/mm/cma.c
> index ffaea26..5e560ed 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -78,6 +78,36 @@ unsigned long cma_get_size(struct cma *cma)
>   return cma->count << PAGE_SHIFT;
>  }
>  
> +unsigned long cma_get_used(struct cma *cma)
> +{
> + unsigned long ret = 0;
> +
> + mutex_lock(&cma->lock);
> + /* pages counter is smaller than sizeof(int) */
> + ret = bitmap_weight(cma->bitmap, (int)cma->count);
> + mutex_unlock(&cma->lock);
> +
> + return ret << (PAGE_SHIFT + cma->order_per_bit);
> +}
> +
> +unsigned long cma_get_maxchunk(struct cma *cma)
> +{
> + unsigned long maxchunk = 0;
> + unsigned long start, end = 0;
> +
> + mutex_lock(&cma->lock);
> + for (;;) {
> + start = find_next_zero_bit(cma->bitmap, cma->count, end);
> + if (start >= cma->count)
> + break;
> + end = find_next_bit(cma->bitmap, cma->count, start);
> + maxchunk = max(end - start, maxchunk);
> + }
> + mutex_unlock(&cma->lock);
> +
> + return maxchunk << (PAGE_SHIFT + cma->order_per_bit);
> +}
> +
>  static unsigned long cma_bitmap_aligned_mask(struct cma *cma, int 
> align_order)
>  {
>   if (align_order <= cma->order_per_bit)
> @@ -591,6 +621,10 @@ static int s_show(struct seq_file *m, void *p)
>   struct cma_buffer *cmabuf;
>   struct stack_trace trace;
>  
> + seq_printf(m, "CMARegion stat: %8lu kB total, %8lu kB used, %8lu kB max 
> contiguous chunk\n\n",
> +cma_get_size(cma) >> 10,
> +cma_get_used(cma) >> 10,
> +cma_get_maxchunk(cma) >> 10);
>   mutex_lock(&cma->list_lock);
>  
>   list_for_each_entry(cmabuf, &cma->buffers_list, list) {
> -- 
> 2.1.0
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] mm: cma: introduce /proc/cmainfo

[Michal Nazarewicz]

>> On Fri, Dec 26 2014, "Stefan I. Strogin"  
>> wrote:
>>> +   if (ret) {
>>> +   pr_warn("%s(): cma_buffer_list_add() returned %d\n",
>>> +   __func__, ret);
>>> +   cma_release(cma, page, count);
>>> +   page = NULL;

> On 12/26/2014 07:02 PM, Michal Nazarewicz wrote:
>> Harsh, but ok, if you want.

On Mon, Dec 29 2014, Stefan Strogin wrote:
> Excuse me, maybe you could suggest how to make a nicer fallback?
> Or sure OK?

I would leave the allocation succeed and print warning that the debug
information is invalid.  You could have a “dirty” flag which is set if
that happens (or on a partial release discussed earlier) which, if set,
would add “Some debug information missing” message at the beginning of
the procfs file.  In my opinion CMA succeeding is more important than
having correct debug information.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] cma: add functions to get region pages counters

[Michal Nazarewicz]

> On Fri, Dec 26, 2014 at 05:39:04PM +0300, Stefan I. Strogin wrote:
>> From: Dmitry Safonov 
>> @@ -591,6 +621,10 @@ static int s_show(struct seq_file *m, void *p)
>>  struct cma_buffer *cmabuf;
>>  struct stack_trace trace;
>>  
>> +seq_printf(m, "CMARegion stat: %8lu kB total, %8lu kB used, %8lu kB max 
>> contiguous chunk\n\n",
>> +   cma_get_size(cma) >> 10,
>> +   cma_get_used(cma) >> 10,
>> +   cma_get_maxchunk(cma) >> 10);
>>  mutex_lock(&cma->list_lock);
>>  
>>  list_for_each_entry(cmabuf, &cma->buffers_list, list) {

On Tue, Dec 30 2014, Joonsoo Kim  wrote:
> How about changing printing format like as meminfo or zoneinfo?
>
> CMARegion #
> Total: XXX
> Used: YYY
> MaxContig: ZZZ

+1.  I was also thinking about this actually.

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] usb: f_fs: refactor and document __ffs_ep0_read_events better

[Michal Nazarewicz]

Instead of using variable length array, use a static length equal to
the size of the ffs->ev.types array.  This gets rid of a sparse warning:

drivers/usb/gadget/function/f_fs.c:401:44: warning:
Variable length array is used.

and makes it more explicit that the array has a very tight upper size
limit.  Also add some more documentation about the ev.types array and
how its size is limited and affects the rest of the code.

Reported-by: Dan Carpenter 
Reported-by: Rohith Seelaboyina 
Signed-off-by: Michal Nazarewicz 
---
 drivers/usb/gadget/function/f_fs.c | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 63314ed..a00ee97 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -390,17 +390,20 @@ done_spin:
return ret;
 }
 
+/* Called with ffs->ev.waitq.lock and ffs->mutex held, both released on exit. 
*/
 static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs, char __user *buf,
 size_t n)
 {
/*
-* We are holding ffs->ev.waitq.lock and ffs->mutex and we need
-* to release them.
+* n cannot be bigger than ffs->ev.count, which cannot be bigger than
+* size of ffs->ev.types array (which is four) so that's how much space
+* we reserve.
 */
-   struct usb_functionfs_event events[n];
+   struct usb_functionfs_event events[ARRAY_SIZE(ffs->ev.types)];
+   const size_t size = n * sizeof *events;
unsigned i = 0;
 
-   memset(events, 0, sizeof events);
+   memset(events, 0, size);
 
do {
events[i].type = ffs->ev.types[i];
@@ -410,19 +413,15 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data 
*ffs, char __user *buf,
}
} while (++i < n);
 
-   if (n < ffs->ev.count) {
-   ffs->ev.count -= n;
+   ffs->ev.count -= n;
+   if (ffs->ev.count)
memmove(ffs->ev.types, ffs->ev.types + n,
ffs->ev.count * sizeof *ffs->ev.types);
-   } else {
-   ffs->ev.count = 0;
-   }
 
spin_unlock_irq(&ffs->ev.waitq.lock);
mutex_unlock(&ffs->mutex);
 
-   return unlikely(__copy_to_user(buf, events, sizeof events))
-   ? -EFAULT : sizeof events;
+   return unlikely(__copy_to_user(buf, events, size)) ? -EFAULT : size;
 }
 
 static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
@@ -2377,6 +2376,13 @@ static void __ffs_event_add(struct ffs_data *ffs,
if (ffs->setup_state == FFS_SETUP_PENDING)
ffs->setup_state = FFS_SETUP_CANCELLED;
 
+   /*
+* Logic of this function guarantees that there are at most four pending
+* evens on ffs->ev.types queue.  This is important because the queue
+* has space for four elements only and __ffs_ep0_read_events function
+* depends on that limit as well.  If more event types are added, those
+* limits have to be revisited or guaranteed to still hold.
+*/
switch (type) {
case FUNCTIONFS_RESUME:
rem_type2 = FUNCTIONFS_SUSPEND;
-- 
2.2.0.rc0.207.ga3a616c
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: cma: Fix totalcma_pages to include DT defined CMA regions

[Michal Nazarewicz]

On Fri, Jan 09 2015, "George G. Davis"  wrote:
> The totalcma_pages variable is not updated to account for CMA regions
> defined via device tree reserved-memory sub-nodes.  Fix this omission by
> moving the calculation of totalcma_pages into cma_init_reserved_mem()
> instead of cma_declare_contiguous() such that it will include reserved
> memory used by all CMA regions.
>
> Signed-off-by: George G. Davis 

Acked-by: Michal Nazarewicz 

> ---
>  mm/cma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/cma.c b/mm/cma.c
> index a85ae28..75016fd 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -199,6 +199,7 @@ int __init cma_init_reserved_mem(phys_addr_t base, 
> phys_addr_t size,
>   cma->order_per_bit = order_per_bit;
>   *res_cma = cma;
>   cma_area_count++;
> + totalcma_pages += (size / PAGE_SIZE);
>  
>   return 0;
>  }
> @@ -337,7 +338,6 @@ int __init cma_declare_contiguous(phys_addr_t base,
>   if (ret)
>   goto err;
>  
> - totalcma_pages += (size / PAGE_SIZE);
>   pr_info("Reserved %ld MiB at %pa\n", (unsigned long)size / SZ_1M,
>   &base);
>   return 0;
> -- 
> 1.9.3
>

-- 
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of  o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz(o o)
ooo +--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/