Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Miquel Raynal]

Hi Boris,

Just a few comments about the form.

Otherwise:
Reviewed-by: Miquel Raynal 


> diff --git a/drivers/mtd/devices/lart.c b/drivers/mtd/devices/lart.c
> index 555b94406e0b..3d6c8ffd351f 100644
> --- a/drivers/mtd/devices/lart.c
> +++ b/drivers/mtd/devices/lart.c
> @@ -415,7 +415,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> erase_info *instr)
>{
>   if (!erase_block (addr))
> {
> -  instr->state = MTD_ERASE_FAILED;
>return (-EIO);
> }

You can also safely remove these '{' '}'

>  
> @@ -425,9 +424,6 @@ static int flash_erase (struct mtd_info *mtd,struct 
> erase_info *instr)
>   if (addr == mtd->eraseregions[i].offset + 
> (mtd->eraseregions[i].erasesize * mtd->eraseregions[i].numblocks)) i++;
>}
>  
> -   instr->state = MTD_ERASE_DONE;
> -   mtd_erase_callback(instr);
> -
> return (0);
>  }
>  
> diff --git a/drivers/mtd/devices/mtd_dataflash.c 
> b/drivers/mtd/devices/mtd_dataflash.c
> index 5dc8bd042cc5..aaaeaae01e1d 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -220,10 +220,6 @@ static int dataflash_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   }
>   mutex_unlock(>lock);
>  
> - /* Inform MTD subsystem that erase is complete */
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);
> -
>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
> index 0bf4aeaf0cb8..efef43c6684b 100644
> --- a/drivers/mtd/devices/mtdram.c
> +++ b/drivers/mtd/devices/mtdram.c
> @@ -60,8 +60,6 @@ static int ram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   if (check_offs_len(mtd, instr->addr, instr->len))
>   return -EINVAL;
>   memset((char *)mtd->priv + instr->addr, 0xff, instr->len);
> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 7287696a21f9..a963c88d392d 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -44,8 +44,6 @@ static int phram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>* I don't feel at all ashamed. This kind of thing is possible anyway
>* with flash, but unlikely.
>*/

Not sure this comment is still relevant? Maybe you could remove it
or at least change it? 

> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
> index cadea0620cd0..5d842cbca3de 100644
> --- a/drivers/mtd/devices/pmc551.c
> +++ b/drivers/mtd/devices/pmc551.c
> @@ -184,12 +184,10 @@ static int pmc551_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>   }
>  
>out:
> - instr->state = MTD_ERASE_DONE;
>  #ifdef CONFIG_MTD_PMC551_DEBUG
>   printk(KERN_DEBUG "pmc551_erase() done\n");
>  #endif
>  
> - mtd_erase_callback(instr);
>   return 0;
>  }
>  
> diff --git a/drivers/mtd/devices/powernv_flash.c 
> b/drivers/mtd/devices/powernv_flash.c
> index 26f9feaa5d17..5f383630c16f 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -175,19 +175,12 @@ static int powernv_flash_erase(struct mtd_info *mtd, 
> struct erase_info *erase)
>  {
>   int rc;
>  
> - erase->state = MTD_ERASING;
> -
>   /* todo: register our own notifier to do a true async implementation */
>   rc =  powernv_flash_async_op(mtd, FLASH_OP_ERASE, erase->addr,
>   erase->len, NULL, NULL);

Are you sure this is still needed? Maybe this should go away in your
first patch?

> -
> - if (rc) {
> + if (rc)
>   erase->fail_addr = erase->addr;
> - erase->state = MTD_ERASE_FAILED;
> - } else {
> - erase->state = MTD_ERASE_DONE;
> - }
> - mtd_erase_callback(erase);
> +
>   return rc;
>  }
>  
> diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
> index 0ec85f316d24..2f05e1801047 100644
> --- a/drivers/mtd/devices/slram.c
> +++ b/drivers/mtd/devices/slram.c
> @@ -88,8 +88,6 @@ static int slram_erase(struct mtd_info *mtd, struct 
> erase_info *instr)
>* I don't feel at all ashamed. This kind of thing is possible anyway
>* with flash, but unlikely.
>*/

Same with this comment.

> - instr->state = MTD_ERASE_DONE;
> - mtd_erase_callback(instr);

Space ?

>   return(0);
>  }
>  




-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: KASAN: use-after-free Read in remove_wait_queue

[Martijn Coenen]

On Mon, Feb 12, 2018 at 7:31 PM, Al Viro  wrote:
> Any chance of bisecting it?

Perhaps my fix introduced another (related) problem, I'm looking into it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

staging: android: ion: potential coherency issue

[Alexey Skidanov]

Hello,

Correct me if I'm wrong, but there is no user space interface, similar
to the dma_buf_start_cpu_access()/dma_buf_end_cpu_access() to handle IO
coherency. That is, on the platforms, where the SW is responsible for IO
coherency handling, the following sequences:
- write to the buffer
- DMA read
or
- DMA write
- read from the buffer
may be problematic.

Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory

[Liam Mark]

On Mon, 12 Feb 2018, Laura Abbott wrote:

> On 02/09/2018 10:21 PM, Liam Mark wrote:
> > The ION begin_cpu_access and end_cpu_access functions use the
> > dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache
> > maintenance.
> > 
> > Currently it is possible to apply cache maintenance, via the
> > begin_cpu_access and end_cpu_access APIs, to ION buffers which are not
> > dma mapped.
> > 
> > The dma sync sg APIs should not be called on sg lists which have not been
> > dma mapped as this can result in cache maintenance being applied to the
> > wrong address. If an sg list has not been dma mapped then its dma_address
> > field has not been populated, some dma ops such as the swiotlb_dma_ops ops
> > use the dma_address field to calculate the address onto which to apply
> > cache maintenance.
> > 
> > Fix the ION begin_cpu_access and end_cpu_access functions to only apply
> > cache maintenance to buffers which have been dma mapped.
> > 
> I think this looks okay. I was initially concerned about concurrency and
> setting the dma_mapped flag but I think that should be handled by the
> caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate
> in the future)


I had convinced myself that concurrency wasn't a problem, but you are 
right it does need to be re-evaluated. For example the code could be at 
the point after the dma unmap call has completed but before dma_mapped has 
been set to false, and if userspace happened to slip in a call to begin/end cpu 
access cache maintenance would happen on memory which isn't dma mapped. 
So at least this would need to be addressed, maybe for this issue just 
move the setting of dma_mapped to the start of the ion_unmap_dma_buf function.

I can clean this up and any other concurrency issues we can identify.

> 
> I would like to hold on queuing this for just a little bit until I
> finish working on the Ion unit test (doing this in the complete opposite
> order of course). I'm assuming this passed your internal tests Liam?

Yes it has passed my internal ION unit tests, though I haven't given 
the change to internal ION clients yet.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Philippe Ombredanne]

On Mon, Feb 12, 2018 at 3:37 PM, Himanshu Jha
 wrote:
> On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote:
>> On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
>>  wrote:
>> > Move the adis16201 driver out of staging directory and merge to the
>> > mainline IIO directory.
>> >
>> > Signed-off-by: Himanshu Jha 
>>
>> 
>> > --- /dev/null
>> > +++ b/drivers/iio/accel/adis16201.c
>> > @@ -0,0 +1,315 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>>
>> 
>>
>> > +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>");
>> > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital 
>> > Inclinometer and Accelerometer");
>> > +MODULE_LICENSE("GPL v2");
>> > +MODULE_ALIAS("spi:adis16201");
>>
>> Your MODULE_LICENSE does not match your SPDX license id.
>> MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
>> MODULE_LICENSE("GPL"); means SPDX GPL-2.0+
>>
>
> I didn't knew about that! Thanks for pointing.

Sure thing. I reckon this can be a tad surprising.
FWIW, the reference for the SPDX tag is in
Documentation/process/license-rules.rst and for the MODULE_LICENSE
macro this is in module.h
It is unlikely module.h will ever evolve there as this would break all
third-party module loaders/tools that rely on the documented data
values and conventions

-- 
Cordially
Philippe Ombredanne
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:11 CET schrieb Boris Brezillon:
> MTD users are no longer checking erase_info->state to determine if the
> erase operation failed or succeeded. Moreover, mtd_erase_callback() is
> now a NOP.
> 
> We can safely get rid of all mtd_erase_callback() calls and all
> erase_info->state assignments. While at it, get rid of the
> erase_info->state field, all MTD_ERASE_XXX definitions and the
> mtd_erase_callback() function.
> 
> Signed-off-by: Boris Brezillon 

Reviewed-by: Richard Weinberger 

Thanks,
//richard

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/5] mtd: Unconditionally update ->fail_addr and ->addr in part_erase()

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:10 CET schrieb Boris Brezillon:
> ->fail_addr and ->addr can be updated no matter the result of
> parent->_erase(), we just need to remove the code doing the same thing
> in mtd_erase_callback() to avoid adjusting those fields twice.
> 
> Note that this can be done because all MTD users have been converted to
> not pass an erase_info->callback() and are thus only taking the
> ->addr_fail and ->addr fields into account after part_erase() has
> returned.
> 
> While we're at it, get rid of the erase_info->mtd field which was only
> needed to let mtd_erase_callback() get the partition device back.
> 
> Signed-off-by: Boris Brezillon 

Reviewed-by: Richard Weinberger 

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

usleep_range without a range

[Joe Perches]

scheduling can generally be better when these values are
not identical.  Perhaps these ranges should be expanded.

$ git grep -P -n "usleep_range\s*\(\s*([\w\.\>\-]+)\s*,\s*\1\s*\)"
drivers/clk/ux500/clk-sysctrl.c:45: 
usleep_range(clk->enable_delay_us, clk->enable_delay_us);
drivers/cpufreq/pmac64-cpufreq.c:140:   usleep_range(1000, 1000);
drivers/cpufreq/pmac64-cpufreq.c:239:   usleep_range(1, 1); /* should 
be faster , to fix */
drivers/cpufreq/pmac64-cpufreq.c:284:   usleep_range(500, 500);
drivers/media/i2c/smiapp/smiapp-core.c:1228:usleep_range(1000, 1000);
drivers/media/i2c/smiapp/smiapp-core.c:1235:usleep_range(1000, 1000);
drivers/media/i2c/smiapp/smiapp-core.c:1240:usleep_range(sleep, sleep);
drivers/media/i2c/smiapp/smiapp-core.c:1387:usleep_range(5000, 5000);
drivers/media/i2c/smiapp/smiapp-quirk.c:205:usleep_range(2000, 2000);
drivers/media/i2c/smiapp/smiapp-regs.c:279: usleep_range(2000, 
2000);
drivers/power/supply/ab8500_fg.c:643:   usleep_range(100, 100);
drivers/staging/rtl8192u/r819xU_phy.c:180:  usleep_range(1000, 1000);
drivers/staging/rtl8192u/r819xU_phy.c:736:  
usleep_range(1000, 1000);
drivers/staging/rtl8192u/r819xU_phy.c:740:  
usleep_range(1000, 1000);
sound/soc/codecs/ab8500-codec.c:1065:   
usleep_range(AB8500_ANC_SM_DELAY, AB8500_ANC_SM_DELAY);
sound/soc/codecs/ab8500-codec.c:1068:   
usleep_range(AB8500_ANC_SM_DELAY, AB8500_ANC_SM_DELAY);


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] mtd: Stop assuming mtd_erase() is asynchronous

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:09 CET schrieb Boris Brezillon:
> None of the mtd->_erase() implementations work in an asynchronous manner,
> so let's simplify MTD users that call mtd_erase(). All they need to do
> is check the value returned by mtd_erase() and assume that != 0 means
> failure.
> 
> Signed-off-by: Boris Brezillon 

Reviewed-by: Richard Weinberger 

Thanks,
//richard


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] mtd: Initialize ->fail_addr early in mtd_erase()

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:07 CET schrieb Boris Brezillon:
> mtd_erase() can return an error before ->fail_addr is initialized to
> MTD_FAIL_ADDR_UNKNOWN. Move this initialization at the very beginning
> of the function.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/mtd/mtdcore.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index a1c94526fb88..c87859ff338b 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -953,6 +953,8 @@ EXPORT_SYMBOL_GPL(__put_mtd_device);
>   */
>  int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>  {
> + instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> +
>   if (!mtd->erasesize || !mtd->_erase)
>   return -ENOTSUPP;
> 
> @@ -961,7 +963,6 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info
> *instr) if (!(mtd->flags & MTD_WRITEABLE))
>   return -EROFS;
> 
> - instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
>   if (!instr->len) {
>   instr->state = MTD_ERASE_DONE;
>   mtd_erase_callback(instr);

Reviewed-by: Richard Weinberger 

Thanks,
//richard
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/5] mtd: Get rid of unused fields in struct erase_info

[Richard Weinberger]

Am Montag, 12. Februar 2018, 22:03:08 CET schrieb Boris Brezillon:
> Some fields are not used by MTD drivers, users or core code. Moreover,
> those fields are not documented, so get rid of them to avoid any
> confusion.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  include/linux/mtd/mtd.h | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 205ededccc60..2a407dc9beaa 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -48,14 +48,9 @@ struct erase_info {
>   uint64_t addr;
>   uint64_t len;
>   uint64_t fail_addr;
> - u_long time;
> - u_long retries;
> - unsigned dev;
> - unsigned cell;
>   void (*callback) (struct erase_info *self);
>   u_long priv;
>   u_char state;
> - struct erase_info *next;
>  };
> 
>  struct mtd_erase_region_info {

Reviewed-by: Richard Weinberger 

Thanks,
//richard

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 10:46 PM, Laura Abbott wrote:
> On 02/12/2018 12:22 PM, Alexey Skidanov wrote:
>>
>>
>> On 02/12/2018 09:52 PM, Laura Abbott wrote:
>>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:

 On 02/12/2018 08:42 PM, Laura Abbott wrote:
> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>> Current ion defined allocation ioctl doesn't allow to specify the
>> requested
>> allocation alignment. CMA heap allocates buffers aligned on buffer
>> size
>> page order.
>>
>> Sometimes, the alignment requirement is less restrictive. In such
>> cases,
>> providing specific alignment may reduce the external memory
>> fragmentation
>> and in some cases it may avoid the allocation request failure.
>>
> I really do not want to bring this back as part of the regular
> ABI.
 Yes, I know it was removed in 4.12.
 Having an alignment parameter that gets used for exactly
> one heap only leads to confusion (which is why it was removed
> from the ABI in the first place).
 You are correct regarding the CMA heap. But, probably it may be used by
 custom heap as well.
>>>
>>> I can think of a lot of instances where it could be used but
>>> ultimately there needs to be an actual in kernel user who wants
>>> it.
>>>
> The alignment came from the behavior of the DMA APIs. Do you
> actually need to specify any alignment from userspace or do
> you only need page size?
 Yes. If CMA gives it for free, I would suggest to let the ion user to
 decide
>>>
>>> I'm really not convinced changing the ABI yet again just to let
>>> the user decide is actually worth it. If we can manage it, I'd
>>> much rather see a proposal that doesn't change the ABI.
>> I didn't actually change the ABI - I just use the "unused" member:
>> struct ion_allocation_data {
>> @@ -80,7 +79,7 @@ struct ion_allocation_data {
>>  __u32 heap_id_mask;
>>  __u32 flags;
>>  __u32 fd;
>> -   __u32 unused;
>> +   __u32 align;
>>   };
>>
> 
> Something that was previously unused is now being used. That's a change
> in how the structure is interpreted which is an ABI change, especially
> since we haven't been checking that the __unused field is set
> to 0.
Yes you are correct. I just realized that it isn't even backward
compatible.
> 
>> As an alternative, I may add __u64 heap_specific_param - but this will
>> change the ABI. But, probably it makes the ABI more generic?
> 
> Why does the ABI need to be more generic? If we change the ABI
> we're stuck with it and I'd like to approach it as the very last
> resort.
> 
> Thanks,
> Laura
It seems, that there is no way to do it without some ABI change?

Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Initialize dma_address of new sg list

[Liam Mark]

On Mon, 12 Feb 2018, Dan Carpenter wrote:

> On Fri, Feb 09, 2018 at 10:16:56PM -0800, Liam Mark wrote:
> > Fix the dup_sg_table function to initialize the dma_address of the new
> > sg list entries instead of the source dma_address entries.
> > 
> > Fixes: 17fd283f3870 ("staging: android: ion: Duplicate sg_table")
> > Signed-off-by: Liam Mark 
> 
> How did you find this bug?  What are the user visible effects of this
> bug?  I'm probably going to ask you to send a v2 patch with a new
> changelog depending on the answers to these questions.

I noticed the bug when reading through the code, I haven’t seen any 
visible effects of this bug.

>From looking at the code I don’t see any obvious ways that this bug would 
introduce any issues with the current code base.

Liam

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] mtd: Unconditionally update ->fail_addr and ->addr in part_erase()

[Boris Brezillon]

->fail_addr and ->addr can be updated no matter the result of
parent->_erase(), we just need to remove the code doing the same thing
in mtd_erase_callback() to avoid adjusting those fields twice.

Note that this can be done because all MTD users have been converted to
not pass an erase_info->callback() and are thus only taking the
->addr_fail and ->addr fields into account after part_erase() has
returned.

While we're at it, get rid of the erase_info->mtd field which was only
needed to let mtd_erase_callback() get the partition device back.

Signed-off-by: Boris Brezillon 
---
 drivers/mtd/ftl.c |  1 -
 drivers/mtd/inftlmount.c  |  3 ---
 drivers/mtd/mtdblock.c|  1 -
 drivers/mtd/mtdchar.c |  1 -
 drivers/mtd/mtdconcat.c   |  1 -
 drivers/mtd/mtdoops.c |  1 -
 drivers/mtd/mtdpart.c | 16 
 drivers/mtd/mtdswap.c |  2 --
 drivers/mtd/nand/nand_base.c  |  1 -
 drivers/mtd/nand/nand_bbt.c   |  1 -
 drivers/mtd/nftlmount.c   |  1 -
 drivers/mtd/rfd_ftl.c |  1 -
 drivers/mtd/sm_ftl.c  |  1 -
 drivers/mtd/tests/mtd_test.c  |  1 -
 drivers/mtd/tests/speedtest.c |  1 -
 drivers/mtd/ubi/io.c  |  1 -
 fs/jffs2/erase.c  |  1 -
 include/linux/mtd/mtd.h   |  3 ++-
 18 files changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
index fcf9907e7987..0a6adfaec7b5 100644
--- a/drivers/mtd/ftl.c
+++ b/drivers/mtd/ftl.c
@@ -342,7 +342,6 @@ static int erase_xfer(partition_t *part,
 if (!erase)
 return -ENOMEM;
 
-erase->mtd = part->mbd.mtd;
 erase->addr = xfer->Offset;
 erase->len = 1 << part->header.EraseUnitSize;
 
diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
index 0f47be4834d8..aab4f68bd36f 100644
--- a/drivers/mtd/inftlmount.c
+++ b/drivers/mtd/inftlmount.c
@@ -208,8 +208,6 @@ static int find_boot_record(struct INFTLrecord *inftl)
if (ip->Reserved0 != ip->firstUnit) {
struct erase_info *instr = >instr;
 
-   instr->mtd = inftl->mbd.mtd;
-
/*
 *  Most likely this is using the
 *  undocumented qiuck mount feature.
@@ -385,7 +383,6 @@ int INFTL_formatblock(struct INFTLrecord *inftl, int block)
   _first_? */
 
/* Use async erase interface, test return code */
-   instr->mtd = inftl->mbd.mtd;
instr->addr = block * inftl->EraseSize;
instr->len = inftl->mbd.mtd->erasesize;
/* Erase one physical eraseblock at a time, even though the NAND api
diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
index 7b2b7f651181..a5b1933c0490 100644
--- a/drivers/mtd/mtdblock.c
+++ b/drivers/mtd/mtdblock.c
@@ -65,7 +65,6 @@ static int erase_write (struct mtd_info *mtd, unsigned long 
pos,
/*
 * First, let's erase the flash block.
 */
-   erase.mtd = mtd;
erase.addr = pos;
erase.len = len;
 
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 2beb22dd6bbb..c06b33f80e75 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -726,7 +726,6 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, 
u_long arg)
erase->addr = einfo32.start;
erase->len = einfo32.length;
}
-   erase->mtd = mtd;
 
ret = mtd_erase(mtd, erase);
kfree(erase);
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
index caa09bf6e572..93c47e56d9d8 100644
--- a/drivers/mtd/mtdconcat.c
+++ b/drivers/mtd/mtdconcat.c
@@ -427,7 +427,6 @@ static int concat_erase(struct mtd_info *mtd, struct 
erase_info *instr)
erase->len = length;
 
length -= erase->len;
-   erase->mtd = subdev;
if ((err = mtd_erase(subdev, erase))) {
/* sanity check: should never happen since
 * block alignment has been checked above */
diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 028ded59297b..9f25111fd559 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -94,7 +94,6 @@ static int mtdoops_erase_block(struct mtdoops_context *cxt, 
int offset)
int ret;
int page;
 
-   erase.mtd = mtd;
erase.addr = offset;
erase.len = mtd->erasesize;
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index ae1206633d9d..1c07a6f0dfe5 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -205,23 +205,15 @@ static int part_erase(struct mtd_info *mtd, struct 
erase_info *instr)
 
instr->addr += part->offset;
ret = part->parent->_erase(part->parent, instr);
-   if (ret) {
-   if (instr->fail_addr != 

[PATCH 0/5] mtd: Simplify erase handling

[Boris Brezillon]

Hello,

This series aims at simplifying erase handling both in MTD drivers and
MTD users code.

Historically, the erase operation has been designed to be asynchronous,
which, in theory, is a good thing since erasing a block usually takes
longer that reading/writing to a flash. In practice, all drivers are
implementing ->_erase() in a synchronous manner. Moreover, both drivers
and users are inconsistently updating/checking the erase_info fields.

In order to simplify things, let's assume ->_erase() is and will always
be synchronous. This also make error code checking more consistent and
allows us to get rid of a few hundred lines of code.

Regards,

Boris

Boris Brezillon (5):
  mtd: Initialize ->fail_addr early in mtd_erase()
  mtd: Get rid of unused fields in struct erase_info
  mtd: Stop assuming mtd_erase() is asynchronous
  mtd: Unconditionally update ->fail_addr and ->addr in part_erase()
  mtd: Stop updating erase_info->state and calling mtd_erase_callback()

 drivers/mtd/chips/cfi_cmdset_0001.c  | 16 +-
 drivers/mtd/chips/cfi_cmdset_0002.c  | 26 ++---
 drivers/mtd/chips/cfi_cmdset_0020.c  |  3 --
 drivers/mtd/chips/map_ram.c  |  2 -
 drivers/mtd/devices/bcm47xxsflash.c  | 12 +
 drivers/mtd/devices/block2mtd.c  |  7 +--
 drivers/mtd/devices/docg3.c  | 16 +-
 drivers/mtd/devices/lart.c   |  4 --
 drivers/mtd/devices/mtd_dataflash.c  |  4 --
 drivers/mtd/devices/mtdram.c |  2 -
 drivers/mtd/devices/phram.c  |  2 -
 drivers/mtd/devices/pmc551.c |  2 -
 drivers/mtd/devices/powernv_flash.c  | 11 +---
 drivers/mtd/devices/slram.c  |  2 -
 drivers/mtd/devices/spear_smi.c  |  3 --
 drivers/mtd/devices/sst25l.c |  3 --
 drivers/mtd/devices/st_spi_fsm.c |  4 --
 drivers/mtd/ftl.c| 52 +++---
 drivers/mtd/inftlmount.c |  8 ++-
 drivers/mtd/lpddr/lpddr2_nvm.c   | 10 +---
 drivers/mtd/lpddr/lpddr_cmds.c   |  2 -
 drivers/mtd/mtdblock.c   | 21 
 drivers/mtd/mtdchar.c| 34 +---
 drivers/mtd/mtdconcat.c  | 48 +
 drivers/mtd/mtdcore.c| 17 +++---
 drivers/mtd/mtdoops.c| 20 ---
 drivers/mtd/mtdpart.c| 23 ++--
 drivers/mtd/mtdswap.c| 34 
 drivers/mtd/nand/nand_base.c | 16 ++
 drivers/mtd/nand/nand_bbt.c  |  1 -
 drivers/mtd/nftlmount.c  |  5 +-
 drivers/mtd/onenand/onenand_base.c   | 17 --
 drivers/mtd/rfd_ftl.c| 93 ++--
 drivers/mtd/sm_ftl.c | 19 ---
 drivers/mtd/sm_ftl.h |  4 --
 drivers/mtd/spi-nor/spi-nor.c|  3 --
 drivers/mtd/tests/mtd_test.c |  5 --
 drivers/mtd/tests/speedtest.c|  7 ---
 drivers/mtd/ubi/gluebi.c |  3 --
 drivers/mtd/ubi/io.c | 36 -
 drivers/net/ethernet/sfc/falcon/mtd.c| 11 +---
 drivers/net/ethernet/sfc/mtd.c   | 11 +---
 drivers/staging/goldfish/goldfish_nand.c |  3 --
 fs/jffs2/erase.c | 37 ++---
 include/linux/mtd/mtd.h  | 19 +--
 45 files changed, 79 insertions(+), 599 deletions(-)

-- 
2.14.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/5] mtd: Stop updating erase_info->state and calling mtd_erase_callback()

[Boris Brezillon]

MTD users are no longer checking erase_info->state to determine if the
erase operation failed or succeeded. Moreover, mtd_erase_callback() is
now a NOP.

We can safely get rid of all mtd_erase_callback() calls and all
erase_info->state assignments. While at it, get rid of the
erase_info->state field, all MTD_ERASE_XXX definitions and the
mtd_erase_callback() function.

Signed-off-by: Boris Brezillon 
---
 drivers/mtd/chips/cfi_cmdset_0001.c  | 16 ++--
 drivers/mtd/chips/cfi_cmdset_0002.c  | 26 +++---
 drivers/mtd/chips/cfi_cmdset_0020.c  |  3 ---
 drivers/mtd/chips/map_ram.c  |  2 --
 drivers/mtd/devices/bcm47xxsflash.c  |  9 +
 drivers/mtd/devices/block2mtd.c  |  7 +--
 drivers/mtd/devices/docg3.c  | 16 ++--
 drivers/mtd/devices/lart.c   |  4 
 drivers/mtd/devices/mtd_dataflash.c  |  4 
 drivers/mtd/devices/mtdram.c |  2 --
 drivers/mtd/devices/phram.c  |  2 --
 drivers/mtd/devices/pmc551.c |  2 --
 drivers/mtd/devices/powernv_flash.c  | 11 ++-
 drivers/mtd/devices/slram.c  |  2 --
 drivers/mtd/devices/spear_smi.c  |  3 ---
 drivers/mtd/devices/sst25l.c |  3 ---
 drivers/mtd/devices/st_spi_fsm.c |  4 
 drivers/mtd/lpddr/lpddr2_nvm.c   | 10 ++
 drivers/mtd/lpddr/lpddr_cmds.c   |  2 --
 drivers/mtd/mtdconcat.c  |  1 -
 drivers/mtd/mtdcore.c|  6 ++
 drivers/mtd/mtdpart.c|  5 -
 drivers/mtd/nand/nand_base.c | 15 +++
 drivers/mtd/onenand/onenand_base.c   | 17 -
 drivers/mtd/spi-nor/spi-nor.c|  3 ---
 drivers/mtd/ubi/gluebi.c |  3 ---
 drivers/net/ethernet/sfc/falcon/mtd.c| 11 +--
 drivers/net/ethernet/sfc/mtd.c   | 11 +--
 drivers/staging/goldfish/goldfish_nand.c |  3 ---
 include/linux/mtd/mtd.h  |  9 -
 30 files changed, 20 insertions(+), 192 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c 
b/drivers/mtd/chips/cfi_cmdset_0001.c
index 5e1b68cbcd0a..d4c07b85f18e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -1993,20 +1993,8 @@ static int __xipram do_erase_oneblock(struct map_info 
*map, struct flchip *chip,
 
 static int cfi_intelext_erase_varsize(struct mtd_info *mtd, struct erase_info 
*instr)
 {
-   unsigned long ofs, len;
-   int ret;
-
-   ofs = instr->addr;
-   len = instr->len;
-
-   ret = cfi_varsize_frob(mtd, do_erase_oneblock, ofs, len, NULL);
-   if (ret)
-   return ret;
-
-   instr->state = MTD_ERASE_DONE;
-   mtd_erase_callback(instr);
-
-   return 0;
+   return cfi_varsize_frob(mtd, do_erase_oneblock, instr->addr,
+   instr->len, NULL);
 }
 
 static void cfi_intelext_sync (struct mtd_info *mtd)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c 
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 56aa6b75213d..668e2cbc155b 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2415,20 +2415,8 @@ static int __xipram do_erase_oneblock(struct map_info 
*map, struct flchip *chip,
 
 static int cfi_amdstd_erase_varsize(struct mtd_info *mtd, struct erase_info 
*instr)
 {
-   unsigned long ofs, len;
-   int ret;
-
-   ofs = instr->addr;
-   len = instr->len;
-
-   ret = cfi_varsize_frob(mtd, do_erase_oneblock, ofs, len, NULL);
-   if (ret)
-   return ret;
-
-   instr->state = MTD_ERASE_DONE;
-   mtd_erase_callback(instr);
-
-   return 0;
+   return cfi_varsize_frob(mtd, do_erase_oneblock, instr->addr,
+   instr->len, NULL);
 }
 
 
@@ -2436,7 +2424,6 @@ static int cfi_amdstd_erase_chip(struct mtd_info *mtd, 
struct erase_info *instr)
 {
struct map_info *map = mtd->priv;
struct cfi_private *cfi = map->fldrv_priv;
-   int ret = 0;
 
if (instr->addr != 0)
return -EINVAL;
@@ -2444,14 +2431,7 @@ static int cfi_amdstd_erase_chip(struct mtd_info *mtd, 
struct erase_info *instr)
if (instr->len != mtd->size)
return -EINVAL;
 
-   ret = do_erase_chip(map, >chips[0]);
-   if (ret)
-   return ret;
-
-   instr->state = MTD_ERASE_DONE;
-   mtd_erase_callback(instr);
-
-   return 0;
+   return do_erase_chip(map, >chips[0]);
 }
 
 static int do_atmel_lock(struct map_info *map, struct flchip *chip,
diff --git a/drivers/mtd/chips/cfi_cmdset_0020.c 
b/drivers/mtd/chips/cfi_cmdset_0020.c
index 7d342965f392..7b7658a05036 100644
--- a/drivers/mtd/chips/cfi_cmdset_0020.c
+++ b/drivers/mtd/chips/cfi_cmdset_0020.c
@@ -965,9 +965,6 @@ static int cfi_staa_erase_varsize(struct mtd_info *mtd,
}
}
 
-   

[PATCH 1/5] mtd: Initialize ->fail_addr early in mtd_erase()

[Boris Brezillon]

mtd_erase() can return an error before ->fail_addr is initialized to
MTD_FAIL_ADDR_UNKNOWN. Move this initialization at the very beginning
of the function.

Signed-off-by: Boris Brezillon 
---
 drivers/mtd/mtdcore.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index a1c94526fb88..c87859ff338b 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -953,6 +953,8 @@ EXPORT_SYMBOL_GPL(__put_mtd_device);
  */
 int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
+   instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+
if (!mtd->erasesize || !mtd->_erase)
return -ENOTSUPP;
 
@@ -961,7 +963,6 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info 
*instr)
if (!(mtd->flags & MTD_WRITEABLE))
return -EROFS;
 
-   instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
if (!instr->len) {
instr->state = MTD_ERASE_DONE;
mtd_erase_callback(instr);
-- 
2.14.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/5] mtd: Get rid of unused fields in struct erase_info

[Boris Brezillon]

Some fields are not used by MTD drivers, users or core code. Moreover,
those fields are not documented, so get rid of them to avoid any
confusion.

Signed-off-by: Boris Brezillon 
---
 include/linux/mtd/mtd.h | 5 -
 1 file changed, 5 deletions(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 205ededccc60..2a407dc9beaa 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -48,14 +48,9 @@ struct erase_info {
uint64_t addr;
uint64_t len;
uint64_t fail_addr;
-   u_long time;
-   u_long retries;
-   unsigned dev;
-   unsigned cell;
void (*callback) (struct erase_info *self);
u_long priv;
u_char state;
-   struct erase_info *next;
 };
 
 struct mtd_erase_region_info {
-- 
2.14.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/12/2018 12:22 PM, Alexey Skidanov wrote:



On 02/12/2018 09:52 PM, Laura Abbott wrote:

On 02/12/2018 11:11 AM, Alexey Skidanov wrote:


On 02/12/2018 08:42 PM, Laura Abbott wrote:

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the
requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such
cases,
providing specific alignment may reduce the external memory
fragmentation
and in some cases it may avoid the allocation request failure.


I really do not want to bring this back as part of the regular
ABI.

Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly

one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.


I can think of a lot of instances where it could be used but
ultimately there needs to be an actual in kernel user who wants
it.


The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Yes. If CMA gives it for free, I would suggest to let the ion user to
decide


I'm really not convinced changing the ABI yet again just to let
the user decide is actually worth it. If we can manage it, I'd
much rather see a proposal that doesn't change the ABI.

I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
 __u32 heap_id_mask;
 __u32 flags;
 __u32 fd;
-   __u32 unused;
+   __u32 align;
  };



Something that was previously unused is now being used. That's a change
in how the structure is interpreted which is an ABI change, especially
since we haven't been checking that the __unused field is set
to 0.


As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?


Why does the ABI need to be more generic? If we change the ABI
we're stuck with it and I'd like to approach it as the very last
resort.

Thanks,
Laura
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory

[Laura Abbott]

On 02/09/2018 10:21 PM, Liam Mark wrote:

The ION begin_cpu_access and end_cpu_access functions use the
dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache
maintenance.

Currently it is possible to apply cache maintenance, via the
begin_cpu_access and end_cpu_access APIs, to ION buffers which are not
dma mapped.

The dma sync sg APIs should not be called on sg lists which have not been
dma mapped as this can result in cache maintenance being applied to the
wrong address. If an sg list has not been dma mapped then its dma_address
field has not been populated, some dma ops such as the swiotlb_dma_ops ops
use the dma_address field to calculate the address onto which to apply
cache maintenance.

Fix the ION begin_cpu_access and end_cpu_access functions to only apply
cache maintenance to buffers which have been dma mapped.


I think this looks okay. I was initially concerned about concurrency and
setting the dma_mapped flag but I think that should be handled by the
caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate
in the future)

I would like to hold on queuing this for just a little bit until I
finish working on the Ion unit test (doing this in the complete opposite
order of course). I'm assuming this passed your internal tests Liam?

Thanks,
Laura


Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and 
mapping")
Signed-off-by: Liam Mark 
---
  drivers/staging/android/ion/ion.c | 16 
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885e346b..e5df5272823d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -214,6 +214,7 @@ struct ion_dma_buf_attachment {
  	struct device *dev;caller 
  	struct sg_table *table;

struct list_head list;
+   bool dma_mapped;
  };
  
  static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev,

@@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, 
struct device *dev,
  
  	a->table = table;

a->dev = dev;
+   a->dma_mapped = false;
INIT_LIST_HEAD(>list);
  
  	attachment->priv = a;

@@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct 
dma_buf_attachment *attachment,
direction))
return ERR_PTR(-ENOMEM);
  
+	a->dma_mapped = true;

return table;
  }
  
@@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,

  struct sg_table *table,
  enum dma_data_direction direction)
  {
+   struct ion_dma_buf_attachment *a = attachment->priv;
+
dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+   a->dma_mapped = false;
  }
  
  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)

@@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
-   direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_cpu(a->dev, a->table->sgl,
+   a->table->nents, direction);
}
mutex_unlock(>lock);
  
@@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
-  direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_device(a->dev, a->table->sgl,
+  a->table->nents, direction);
}
mutex_unlock(>lock);
  



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Restrict cache maintenance to dma mapped memory

[Laura Abbott]

On 02/09/2018 10:21 PM, Liam Mark wrote:

The ION begin_cpu_access and end_cpu_access functions use the
dma_sync_sg_for_cpu and dma_sync_sg_for_device APIs to perform cache
maintenance.

Currently it is possible to apply cache maintenance, via the
begin_cpu_access and end_cpu_access APIs, to ION buffers which are not
dma mapped.

The dma sync sg APIs should not be called on sg lists which have not been
dma mapped as this can result in cache maintenance being applied to the
wrong address. If an sg list has not been dma mapped then its dma_address
field has not been populated, some dma ops such as the swiotlb_dma_ops ops
use the dma_address field to calculate the address onto which to apply
cache maintenance.

Fix the ION begin_cpu_access and end_cpu_access functions to only apply
cache maintenance to buffers which have been dma mapped.


I think this looks okay. I was initially concerned about concurrency and
setting the dma_mapped flag but I think that should be handled by the
caller synchronizing map/unmap/cpu_access calls (we might need to re-evaluate
in the future)

I would like to hold on queuing this for just a little bit until I
finish working on the Ion unit test (doing this in the complete opposite
order of course). I'm assuming this passed your internal tests Liam?

Thanks,
Laura


Fixes: 2a55e7b5e544 ("staging: android: ion: Call dma_map_sg for syncing and 
mapping")
Signed-off-by: Liam Mark 
---
  drivers/staging/android/ion/ion.c | 16 
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index f480885e346b..e5df5272823d 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -214,6 +214,7 @@ struct ion_dma_buf_attachment {
  	struct device *dev;caller 
  	struct sg_table *table;

struct list_head list;
+   bool dma_mapped;
  };
  
  static int ion_dma_buf_attach(struct dma_buf *dmabuf, struct device *dev,

@@ -235,6 +236,7 @@ static int ion_dma_buf_attach(struct dma_buf *dmabuf, 
struct device *dev,
  
  	a->table = table;

a->dev = dev;
+   a->dma_mapped = false;
INIT_LIST_HEAD(>list);
  
  	attachment->priv = a;

@@ -272,6 +274,7 @@ static struct sg_table *ion_map_dma_buf(struct 
dma_buf_attachment *attachment,
direction))
return ERR_PTR(-ENOMEM);
  
+	a->dma_mapped = true;

return table;
  }
  
@@ -279,7 +282,10 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,

  struct sg_table *table,
  enum dma_data_direction direction)
  {
+   struct ion_dma_buf_attachment *a = attachment->priv;
+
dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
+   a->dma_mapped = false;
  }
  
  static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)

@@ -345,8 +351,9 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf 
*dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
-   direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_cpu(a->dev, a->table->sgl,
+   a->table->nents, direction);
}
mutex_unlock(>lock);
  
@@ -367,8 +374,9 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
  
  	mutex_lock(>lock);

list_for_each_entry(a, >attachments, list) {
-   dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
-  direction);
+   if (a->dma_mapped)
+   dma_sync_sg_for_device(a->dev, a->table->sgl,
+  a->table->nents, direction);
}
mutex_unlock(>lock);
  



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ion kernel mapping implementation

[Alexey Skidanov]

On 02/12/2018 10:09 PM, Laura Abbott wrote:
> On 02/12/2018 11:21 AM, Alexey Skidanov wrote:
>>
>>
>> On 02/12/2018 08:30 PM, Laura Abbott wrote:
>>> On 02/10/2018 01:43 AM, Alexey Skidanov wrote:
 Hi,

 Current ion kernel mapping implementation uses vmap() to map previously
 allocated buffers into kernel virtual address space. On 32 bit
 platforms, vmap() might fail on large enough buffers due to the limited
 available vmalloc space.

 dma_buf_kmap() should guarantee that only one page is mapped at a time.
 So, probably it's better to implement dma_buf_kmap() by kmap() and not
 by vmap()?

 Thanks,
 Alexey

>>>
>>> The short answer is yes.
>>>
>>> The long answer is that the conversion to the dma_buf APIs kept the
>>> existing Ion behavior which mapped the entire buffer. We got away
>>> with this because the in kernel mapping APIs are used very infrequently
>>> and with buffers that never triggered an exhaustion of vmalloc
>>> space.
>>>
>>> If we actually start seeing problems with this we can fix it up
>>> but I don't consider this a high priority item.
>> I have the patch fixing this potential bug. I would like to submit it
>> for review, if you are ok with it. Please, just let me know.
> 
> Yes, please submit from review and we can go from there.
> 
Ok. Will submit it.
>>>
>>> Thanks,
>>> Laura
>>
>> Thanks,
>> Alexey
>>
> 
Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 09:52 PM, Laura Abbott wrote:
> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>
>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
 Current ion defined allocation ioctl doesn't allow to specify the
 requested
 allocation alignment. CMA heap allocates buffers aligned on buffer size
 page order.

 Sometimes, the alignment requirement is less restrictive. In such
 cases,
 providing specific alignment may reduce the external memory
 fragmentation
 and in some cases it may avoid the allocation request failure.

>>> I really do not want to bring this back as part of the regular
>>> ABI.
>> Yes, I know it was removed in 4.12.
>> Having an alignment parameter that gets used for exactly
>>> one heap only leads to confusion (which is why it was removed
>>> from the ABI in the first place).
>> You are correct regarding the CMA heap. But, probably it may be used by
>> custom heap as well.
> 
> I can think of a lot of instances where it could be used but
> ultimately there needs to be an actual in kernel user who wants
> it.
> 
>>> The alignment came from the behavior of the DMA APIs. Do you
>>> actually need to specify any alignment from userspace or do
>>> you only need page size?
>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>> decide
> 
> I'm really not convinced changing the ABI yet again just to let
> the user decide is actually worth it. If we can manage it, I'd
> much rather see a proposal that doesn't change the ABI.
I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
-   __u32 unused;
+   __u32 align;
 };

As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?
> 
>>> Thanks,
>>> Laura
>>>
>> Thanks,
>> Alexey
>>
> 

Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ion kernel mapping implementation

[Laura Abbott]

On 02/12/2018 11:21 AM, Alexey Skidanov wrote:



On 02/12/2018 08:30 PM, Laura Abbott wrote:

On 02/10/2018 01:43 AM, Alexey Skidanov wrote:

Hi,

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space. On 32 bit
platforms, vmap() might fail on large enough buffers due to the limited
available vmalloc space.

dma_buf_kmap() should guarantee that only one page is mapped at a time.
So, probably it's better to implement dma_buf_kmap() by kmap() and not
by vmap()?

Thanks,
Alexey



The short answer is yes.

The long answer is that the conversion to the dma_buf APIs kept the
existing Ion behavior which mapped the entire buffer. We got away
with this because the in kernel mapping APIs are used very infrequently
and with buffers that never triggered an exhaustion of vmalloc
space.

If we actually start seeing problems with this we can fix it up
but I don't consider this a high priority item.

I have the patch fixing this potential bug. I would like to submit it
for review, if you are ok with it. Please, just let me know.


Yes, please submit from review and we can go from there.



Thanks,
Laura


Thanks,
Alexey



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/12/2018 11:11 AM, Alexey Skidanov wrote:


On 02/12/2018 08:42 PM, Laura Abbott wrote:

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the
requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.


I really do not want to bring this back as part of the regular
ABI.

Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly

one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.


I can think of a lot of instances where it could be used but
ultimately there needs to be an actual in kernel user who wants
it.


The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Yes. If CMA gives it for free, I would suggest to let the ion user to decide


I'm really not convinced changing the ABI yet again just to let
the user decide is actually worth it. If we can manage it, I'd
much rather see a proposal that doesn't change the ABI.


Thanks,
Laura


Thanks,
Alexey



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> > But these should be done when we have *more* instances.
> > 
> > For eg: 
> > I added a tab space in function static int adis16201_read_raw() argument
> > to match open parentheses in this patch. But I also added tabs while
> > removing and adding suitable suffix to the macros. So, should it also be
> > done in a separate patch ?
> 
> If you're changing a line of code and you fix a white space issue on
> that same line, then that's fine.  If it's just in the same function,
> then do it in a separate patch.  In other words, adding tabs when you're
> moving around macros is fine, but adding it to the arguments is
> unrelated.

I will try and divide my patches next time :)

> This patch was honestly pretty tricky to review.

I am sorry for that. Might be easy for IIO reviewers ;)

> Jonathan assumes reviewers have the datasheet in front of them and I
> assume that that they don't.  He's probably right...  But especially
> comments like this:
> 
>   *val2 = 22; /* 1.22 mV */

They are pretty obvious as you can see from the return statements
just below that which is :

return IIO_VAL_INT_PLUS_MICRO;

These comments are obvious because we know 'val1' will be responsible
for Integer part(1.0) and 'val2' for the Micro part(22 * 10^-6 = 0.22).
Isn't it ?

Although I am new to IIO please correct if I'm wrong.

-- 
Thanks
Himanshu Jha
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ion kernel mapping implementation

[Alexey Skidanov]

On 02/12/2018 08:30 PM, Laura Abbott wrote:
> On 02/10/2018 01:43 AM, Alexey Skidanov wrote:
>> Hi,
>>
>> Current ion kernel mapping implementation uses vmap() to map previously
>> allocated buffers into kernel virtual address space. On 32 bit
>> platforms, vmap() might fail on large enough buffers due to the limited
>> available vmalloc space.
>>
>> dma_buf_kmap() should guarantee that only one page is mapped at a time.
>> So, probably it's better to implement dma_buf_kmap() by kmap() and not
>> by vmap()?
>>
>> Thanks,
>> Alexey
>>
> 
> The short answer is yes.
> 
> The long answer is that the conversion to the dma_buf APIs kept the
> existing Ion behavior which mapped the entire buffer. We got away
> with this because the in kernel mapping APIs are used very infrequently
> and with buffers that never triggered an exhaustion of vmalloc
> space.
> 
> If we actually start seeing problems with this we can fix it up
> but I don't consider this a high priority item.
I have the patch fixing this potential bug. I would like to submit it
for review, if you are ok with it. Please, just let me know.
> 
> Thanks,
> Laura

Thanks,
Alexey
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

On 02/12/2018 08:42 PM, Laura Abbott wrote:
> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>> Current ion defined allocation ioctl doesn't allow to specify the
>> requested
>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>> page order.
>>
>> Sometimes, the alignment requirement is less restrictive. In such cases,
>> providing specific alignment may reduce the external memory fragmentation
>> and in some cases it may avoid the allocation request failure.
>>
> 
> I really do not want to bring this back as part of the regular
> ABI. 
Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly
> one heap only leads to confusion (which is why it was removed
> from the ABI in the first place).
You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.
> 
> The alignment came from the behavior of the DMA APIs. Do you
> actually need to specify any alignment from userspace or do
> you only need page size?
Yes. If CMA gives it for free, I would suggest to let the ion user to decide
> 
> Thanks,
> Laura
> 
Thanks,
Alexey

>> To fix this, we add an alignment parameter to the allocation ioctl.
>>
>> Signed-off-by: Alexey Skidanov 
>> ---
>>   drivers/staging/android/ion/ion-ioctl.c |  3 ++-
>>   drivers/staging/android/ion/ion.c   | 14 +-
>>   drivers/staging/android/ion/ion.h   |  9 ++---
>>   drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
>>   drivers/staging/android/ion/ion_chunk_heap.c    |  3 ++-
>>   drivers/staging/android/ion/ion_cma_heap.c  | 18 --
>>   drivers/staging/android/ion/ion_system_heap.c   |  6 --
>>   drivers/staging/android/uapi/ion.h  |  7 +++
>>   8 files changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index c789893..9fe022b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>     fd = ion_alloc(data.allocation.len,
>>  data.allocation.heap_id_mask,
>> -   data.allocation.flags);
>> +   data.allocation.flags,
>> +   data.allocation.align);
>>   if (fd < 0)
>>   return fd;
>>   diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index f480885..35ddc7a 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
>>   static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>>   struct ion_device *dev,
>>   unsigned long len,
>> -    unsigned long flags)
>> +    unsigned long flags,
>> +    unsigned int align)
>>   {
>>   struct ion_buffer *buffer;
>>   int ret;
>> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct
>> ion_heap *heap,
>>   buffer->heap = heap;
>>   buffer->flags = flags;
>>   -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>     if (ret) {
>>   if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
>>   goto err2;
>>     ion_heap_freelist_drain(heap, 0);
>> -    ret = heap->ops->allocate(heap, buffer, len, flags);
>> +    ret = heap->ops->allocate(heap, buffer, len, flags, align);
>>   if (ret)
>>   goto err2;
>>   }
>> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
>>   .unmap = ion_dma_buf_kunmap,
>>   };
>>   -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int
>> flags)
>> +int ion_alloc(size_t len,
>> +  unsigned int heap_id_mask,
>> +  unsigned int flags,
>> +  unsigned int align)
>>   {
>>   struct ion_device *dev = internal_dev;
>>   struct ion_buffer *buffer = NULL;
>> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int
>> heap_id_mask, unsigned int flags)
>>   /* if the caller didn't specify this heap id */
>>   if (!((1 << heap->id) & heap_id_mask))
>>   continue;
>> -    buffer = ion_buffer_create(heap, dev, len, flags);
>> +    buffer = ion_buffer_create(heap, dev, len, flags, align);
>>   if (!IS_ERR(buffer))
>>   break;
>>   }
>> diff --git a/drivers/staging/android/ion/ion.h
>> b/drivers/staging/android/ion/ion.h
>> index f5f9cd6..0c161d2 100644
>> --- a/drivers/staging/android/ion/ion.h
>> +++ b/drivers/staging/android/ion/ion.h
>> @@ -123,8 +123,10 @@ struct ion_device {
>>    */
>>   struct ion_heap_ops {
>>   int (*allocate)(struct 

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Dan Carpenter]

On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> But these should be done when we have *more* instances.
> 
> For eg: 
> I added a tab space in function static int adis16201_read_raw() argument
> to match open parentheses in this patch. But I also added tabs while
> removing and adding suitable suffix to the macros. So, should it also be
> done in a separate patch ?

If you're changing a line of code and you fix a white space issue on
that same line, then that's fine.  If it's just in the same function,
then do it in a separate patch.  In other words, adding tabs when you're
moving around macros is fine, but adding it to the arguments is
unrelated.

This patch was honestly pretty tricky to review.

Jonathan assumes reviewers have the datasheet in front of them and I
assume that that they don't.  He's probably right...  But especially
comments like this:

*val2 = 22; /* 1.22 mV */

They seem really helpful to me.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Laura Abbott]

On 02/10/2018 02:17 AM, Alexey Skidanov wrote:

Current ion defined allocation ioctl doesn't allow to specify the requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.

Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.



I really do not want to bring this back as part of the regular
ABI. Having an alignment parameter that gets used for exactly
one heap only leads to confusion (which is why it was removed
from the ABI in the first place).

The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?

Thanks,
Laura


To fix this, we add an alignment parameter to the allocation ioctl.

Signed-off-by: Alexey Skidanov 
---
  drivers/staging/android/ion/ion-ioctl.c |  3 ++-
  drivers/staging/android/ion/ion.c   | 14 +-
  drivers/staging/android/ion/ion.h   |  9 ++---
  drivers/staging/android/ion/ion_carveout_heap.c |  3 ++-
  drivers/staging/android/ion/ion_chunk_heap.c|  3 ++-
  drivers/staging/android/ion/ion_cma_heap.c  | 18 --
  drivers/staging/android/ion/ion_system_heap.c   |  6 --
  drivers/staging/android/uapi/ion.h  |  7 +++
  8 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c 
b/drivers/staging/android/ion/ion-ioctl.c
index c789893..9fe022b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
  
  		fd = ion_alloc(data.allocation.len,

   data.allocation.heap_id_mask,
-  data.allocation.flags);
+  data.allocation.flags,
+  data.allocation.align);
if (fd < 0)
return fd;
  
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c

index f480885..35ddc7a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
  static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
-   unsigned long flags)
+   unsigned long flags,
+   unsigned int align)
  {
struct ion_buffer *buffer;
int ret;
@@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap 
*heap,
buffer->heap = heap;
buffer->flags = flags;
  
-	ret = heap->ops->allocate(heap, buffer, len, flags);

+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
  
  	if (ret) {

if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
goto err2;
  
  		ion_heap_freelist_drain(heap, 0);

-   ret = heap->ops->allocate(heap, buffer, len, flags);
+   ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret)
goto err2;
}
@@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
  };
  
-int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)

+int ion_alloc(size_t len,
+ unsigned int heap_id_mask,
+ unsigned int flags,
+ unsigned int align)
  {
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, 
unsigned int flags)
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
-   buffer = ion_buffer_create(heap, dev, len, flags);
+   buffer = ion_buffer_create(heap, dev, len, flags, align);
if (!IS_ERR(buffer))
break;
}
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index f5f9cd6..0c161d2 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -123,8 +123,10 @@ struct ion_device {
   */
  struct ion_heap_ops {
int (*allocate)(struct ion_heap *heap,
-   struct ion_buffer *buffer, unsigned long len,
-   unsigned long flags);
+   struct ion_buffer *buffer,
+   unsigned long len,
+   unsigned long flags,
+   unsigned int 

Re: KASAN: use-after-free Read in remove_wait_queue

[Al Viro]

On Mon, Feb 12, 2018 at 06:11:02PM +0100, Dmitry Vyukov wrote:

> The commit on which it was triggered already includes this fix. So
> there must be another bug.

Any chance of bisecting it?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ion kernel mapping implementation

[Laura Abbott]

On 02/10/2018 01:43 AM, Alexey Skidanov wrote:

Hi,

Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space. On 32 bit
platforms, vmap() might fail on large enough buffers due to the limited
available vmalloc space.

dma_buf_kmap() should guarantee that only one page is mapped at a time.
So, probably it's better to implement dma_buf_kmap() by kmap() and not
by vmap()?

Thanks,
Alexey



The short answer is yes.

The long answer is that the conversion to the dma_buf APIs kept the
existing Ion behavior which mapped the entire buffer. We got away
with this because the in kernel mapping APIs are used very infrequently
and with buffers that never triggered an exhaustion of vmalloc
space.

If we actually start seeing problems with this we can fix it up
but I don't consider this a high priority item.

Thanks,
Laura
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Dan Carpenter]

On Mon, Feb 12, 2018 at 08:11:57PM +0530, Himanshu Jha wrote:
> On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote:
> > I think -M is prefered for these types of diffs?  Not sure.
> 
> I wrote about that in the cover letter if you missed. :)
> 

Yeah.  I seldom read cover letters.

> > > + ret = adis_init(st, indio_dev, spi, _data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > + if (ret)
> > > + return ret;
> > 
> > We should clean up the IRQ which we enabled in adis_init() instead of
> > returning directly.
> 
> I'm not sure about how to do that.
> 

I believe in you that you can figure it out.  :)

regards,
dan carpenter


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote:
> I think -M is prefered for these types of diffs?  Not sure.

I wrote about that in the cover letter if you missed. :)

> On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote:
> > +static int adis16201_probe(struct spi_device *spi)
> > +{
> > +   struct iio_dev *indio_dev;
> > +   struct adis *st;
> > +   int ret;
> > +
> > +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
> > +   if (!indio_dev)
> > +   return -ENOMEM;
> > +
> > +   st = iio_priv(indio_dev);
> > +   spi_set_drvdata(spi, indio_dev);
> > +
> > +   indio_dev->name = spi->dev.driver->name;
> > +   indio_dev->dev.parent = >dev;
> > +   indio_dev->info = _info;
> > +
> > +   indio_dev->channels = adis16201_channels;
> > +   indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> > +   indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +   ret = adis_init(st, indio_dev, spi, _data);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > +   if (ret)
> > +   return ret;
> 
> We should clean up the IRQ which we enabled in adis_init() instead of
> returning directly.

I'm not sure about how to do that.

-- 
Thanks
Himanshu Jha
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote:
> On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
>  wrote:
> > Move the adis16201 driver out of staging directory and merge to the
> > mainline IIO directory.
> >
> > Signed-off-by: Himanshu Jha 
> 
> 
> > --- /dev/null
> > +++ b/drivers/iio/accel/adis16201.c
> > @@ -0,0 +1,315 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> 
> 
> > +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>");
> > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital 
> > Inclinometer and Accelerometer");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:adis16201");
> 
> Your MODULE_LICENSE does not match your SPDX license id.
> MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
> MODULE_LICENSE("GPL"); means SPDX GPL-2.0+
> 

I didn't knew about that! Thanks for pointing.

-- 
Thanks
Himanshu Jha
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

Hi Dan,

On Mon, Feb 12, 2018 at 03:53:12PM +0300, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote:
> > Remove some unnecessary comments by giving appropriate name to macros.
> > Therefore, add _REG suffix for control registers. Also, align function
> > arguments to match open parentheses using tabs.
> > Group the control register and register field macros together.
> > 
> > Blank line before some returns improves code readability.
> > 
> > Signed-off-by: Himanshu Jha 
> 
> The most obvious response is that this needs to be broken up into
> multiple patches.
> 
> I think I liked almost all the comments...

Please note that all the changes are suggested by Joanathan in his TODO
https://marc.info/?l=linux-iio=151775804702998=2
I think it far more commenting as compared to other drivers in the
mainline IIO drivers. I grouped all the relevant control registers
together at one place with suitable comment.
For eg:

+/* Data Output Register Definitions */

The macro names are identical to the names in the datasheet if you can
look and one could easily refer to the datasheet easily. Also, I
grouped the control registers and their fields together make it more readable.
For eg:

+#define ADIS16201_MSC_CTRL_REG 0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2)


> > -/* Output, power supply */
> > -#define ADIS16201_SUPPLY_OUT 0x02
> > +#define ADIS16201_SUPPLY_OUT_REG   0x02
> 
> To me it seems useful to know that we're talking about the power supply.

For that purpose I already grouped data output register definitions.

> Adding _REG to the name seems not terribly useful and it makes the name
> longer so we're going to run into the 80 character limit.  I just
> checked and this patch does add some checkpatch warnings.

_REG prefix is used to differentiate between registers addresses and the
fields in the register as pointed in the above MSC_CTRL_REG and
MSC_CTRL_SELF_TEST_EN.

Yes by doing this it triggered 2 I think 80 character warning. For eg:

ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 
12),

But you know with 81 characters so I neglected it because it looked more
readable without breaking into lines IMHO.

> But aligning the arguments is fine, deleting unused macros is fine,
> adding blank lines is fine.
> 
> >  static int adis16201_probe(struct spi_device *spi)
> >  {
> > -   int ret;
> > -   struct adis *st;
> > struct iio_dev *indio_dev;
> > +   struct adis *st;
> > +   int ret;
> 
> Making this reverse Christmas tree is fine.  But these things should all
> be done in separate patches.

I am sometimes confused in dividing the patches. As per your advice I
should make separate patch for :

1. remove unnecessary comments.
2. add suitable suffix.
3. add tabs instead of space.
4. reverse christmas tree.

But these should be done when we have *more* instances.

For eg: 
I added a tab space in function static int adis16201_read_raw() argument
to match open parentheses in this patch. But I also added tabs while
removing and adding suitable suffix to the macros. So, should it also be
done in a separate patch ? Since there was only one instance of adding tabs
therefore I did it here without framing another patch for that purpose
while saving my time to plan 'what to include/exclude in the patch ?'

Also, given the above queries I was clueless as what to do first! Since
sometimes it happens that the patch series doesn't apply cleanly because
of incorrect ordering for framing the patches.

Thanks for taking your time to review the patch series.

-- 
Thanks
Himanshu Jha
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

DONATION FOR less-privileged

[Mrs. JULIETTE ROMUALDEZ]

My name is Mrs. Juliette Romualdez from Philippine. I am a sick woman who
has decided to donate what I have to charity through you. You may be
wondering why I chose you. But someone has to be chosen. I am 62 years old
and was diagnosed for cancer about 2 years ago, I inherited a total sum of
$6.150.000.00 million dollars from my late husband, This money is
deposited in a reliable bank here in MANILA PHPLILPINE; Presently .This
deposit was coded under a secret arrangement as a family treasure.I have
informed my Attorney, about my decision in WILLING this fund to you
.please get back to me immediately via my private email address (
julietteromual...@yahoo.com )Your beloved sister in Christ, Mrs. Juliette
Romualdez

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Philippe Ombredanne]

On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
 wrote:
> Move the adis16201 driver out of staging directory and merge to the
> mainline IIO directory.
>
> Signed-off-by: Himanshu Jha 


> --- /dev/null
> +++ b/drivers/iio/accel/adis16201.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0+



> +MODULE_AUTHOR("Barry Song <21cn...@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer 
> and Accelerometer");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:adis16201");

Your MODULE_LICENSE does not match your SPDX license id.
MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
MODULE_LICENSE("GPL"); means SPDX GPL-2.0+


-- 
Cordially
Philippe Ombredanne
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Dan Carpenter]

I think -M is prefered for these types of diffs?  Not sure.

On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote:
> +static int adis16201_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct adis *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + spi_set_drvdata(spi, indio_dev);
> +
> + indio_dev->name = spi->dev.driver->name;
> + indio_dev->dev.parent = >dev;
> + indio_dev->info = _info;
> +
> + indio_dev->channels = adis16201_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = adis_init(st, indio_dev, spi, _data);
> + if (ret)
> + return ret;
> +
> + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> + if (ret)
> + return ret;

We should clean up the IRQ which we enabled in adis_init() instead of
returning directly.

> +
> + /* Get the device into a sane initial state */
> + ret = adis_initial_startup(st);
> + if (ret)
> + goto error_cleanup_buffer_trigger;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + goto error_cleanup_buffer_trigger;
> +
> + return 0;
> +
> +error_cleanup_buffer_trigger:
> + adis_cleanup_buffer_and_trigger(st, indio_dev);
> +
> + return ret;
> +}

Otherwise it looks fine to my not-an-iio-expert eye.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Dan Carpenter]

On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> Use sign_extend32 function instead of manually coding it. Also, adjust a
^
> switch block to explicitly match channels and return -EINVAL as default
> case which improves code readability.

Greg likes to say something along the lines of "when you start your
sentence with "Also, " that could be a clue that it should be a separate
patch.".

> 
> Signed-off-by: Himanshu Jha 
> ---
>  drivers/staging/iio/accel/adis16201.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16201.c 
> b/drivers/staging/iio/accel/adis16201.c
> index 011d2c5..6800347 100644
> --- a/drivers/staging/iio/accel/adis16201.c
> +++ b/drivers/staging/iio/accel/adis16201.c
> @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
>   case IIO_CHAN_INFO_SCALE:
>   switch (chan->type) {
>   case IIO_VOLTAGE:
> - if (chan->channel == 0) {
> + switch (chan->channel) {
> + case 0:
>   *val = 1;
>   *val2 = 22;
> - } else {
> + break;
> + case 1:
>   *val = 0;
>   *val2 = 61;
> + break;
> + default:
> + return -EINVAL;
>   }

I don't think this improves readability.  The -EINVAL is to handle a
driver bug which we haven't introduced yet.  Probably we would be better
off printing a warning or something?  But it feels weird to introduce so
much code to handle a bug which would actually be pretty difficult to
write.  The original code is fine.

>   return IIO_VAL_INT_PLUS_MICRO;
>   case IIO_TEMP:
> @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
>   if (ret)
>   return ret;
>  
> - val16 &= (1 << bits) - 1;
> - val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> - *val = val16;
> + *val = sign_extend32(val16, bits - 1);

Yeah.  This is a nice clean up.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Dan Carpenter]

On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote:
> Remove some unnecessary comments by giving appropriate name to macros.
> Therefore, add _REG suffix for control registers. Also, align function
> arguments to match open parentheses using tabs.
> Group the control register and register field macros together.
> 
> Blank line before some returns improves code readability.
> 
> Signed-off-by: Himanshu Jha 

The most obvious response is that this needs to be broken up into
multiple patches.

I think I liked almost all the comments...

> -/* Output, power supply */
> -#define ADIS16201_SUPPLY_OUT 0x02
> +#define ADIS16201_SUPPLY_OUT_REG 0x02

To me it seems useful to know that we're talking about the power supply.

Adding _REG to the name seems not terribly useful and it makes the name
longer so we're going to run into the 80 character limit.  I just
checked and this patch does add some checkpatch warnings.

But aligning the arguments is fine, deleting unused macros is fine,
adding blank lines is fine.

>  static int adis16201_probe(struct spi_device *spi)
>  {
> - int ret;
> - struct adis *st;
>   struct iio_dev *indio_dev;
> + struct adis *st;
> + int ret;

Making this reverse Christmas tree is fine.  But these things should all
be done in separate patches.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: media: reformat line to 80 chars or less

[Parthiban Nallathambi]

This is a cleanup patch to fix line length issue found
by checkpatch.pl script.

In this patch, line 144 have been wrapped.

Signed-off-by: Parthiban Nallathambi 
---
 drivers/staging/media/imx/imx-media-capture.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c 
b/drivers/staging/media/imx/imx-media-capture.c
index 576bdc7e9c42..0ccabe04b0e1 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -141,7 +141,8 @@ static int capture_enum_frameintervals(struct file *file, 
void *fh,
 
fie.code = cc->codes[0];
 
-   ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, 
);
+   ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
+  NULL, );
if (ret)
return ret;
 
-- 
2.14.3

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

Remove some unnecessary comments by giving appropriate name to macros.
Therefore, add _REG suffix for control registers. Also, align function
arguments to match open parentheses using tabs.
Group the control register and register field macros together.

Blank line before some returns improves code readability.

Signed-off-by: Himanshu Jha 
---
 drivers/staging/iio/accel/adis16201.c | 226 --
 1 file changed, 79 insertions(+), 147 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c 
b/drivers/staging/iio/accel/adis16201.c
index b2145c9..011d2c5 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -19,135 +19,63 @@
 #include 
 #include 
 
-#define ADIS16201_STARTUP_DELAY220 /* ms */
-
-/* Flash memory write count */
-#define ADIS16201_FLASH_CNT  0x00
-
-/* Output, power supply */
-#define ADIS16201_SUPPLY_OUT 0x02
-
-/* Output, x-axis accelerometer */
-#define ADIS16201_XACCL_OUT  0x04
-
-/* Output, y-axis accelerometer */
-#define ADIS16201_YACCL_OUT  0x06
-
-/* Output, auxiliary ADC input */
-#define ADIS16201_AUX_ADC0x08
-
-/* Output, temperature */
-#define ADIS16201_TEMP_OUT   0x0A
-
-/* Output, x-axis inclination */
-#define ADIS16201_XINCL_OUT  0x0C
-
-/* Output, y-axis inclination */
-#define ADIS16201_YINCL_OUT  0x0E
-
-/* Calibration, x-axis acceleration offset */
-#define ADIS16201_XACCL_OFFS 0x10
-
-/* Calibration, y-axis acceleration offset */
-#define ADIS16201_YACCL_OFFS 0x12
-
-/* x-axis acceleration scale factor */
-#define ADIS16201_XACCL_SCALE0x14
-
-/* y-axis acceleration scale factor */
-#define ADIS16201_YACCL_SCALE0x16
-
-/* Calibration, x-axis inclination offset */
-#define ADIS16201_XINCL_OFFS 0x18
-
-/* Calibration, y-axis inclination offset */
-#define ADIS16201_YINCL_OFFS 0x1A
-
-/* x-axis inclination scale factor */
-#define ADIS16201_XINCL_SCALE0x1C
-
-/* y-axis inclination scale factor */
-#define ADIS16201_YINCL_SCALE0x1E
-
-/* Alarm 1 amplitude threshold */
-#define ADIS16201_ALM_MAG1   0x20
-
-/* Alarm 2 amplitude threshold */
-#define ADIS16201_ALM_MAG2   0x22
-
-/* Alarm 1, sample period */
-#define ADIS16201_ALM_SMPL1  0x24
-
-/* Alarm 2, sample period */
-#define ADIS16201_ALM_SMPL2  0x26
-
-/* Alarm control */
-#define ADIS16201_ALM_CTRL   0x28
-
-/* Auxiliary DAC data */
-#define ADIS16201_AUX_DAC0x30
-
-/* General-purpose digital input/output control */
-#define ADIS16201_GPIO_CTRL  0x32
-
-/* Miscellaneous control */
-#define ADIS16201_MSC_CTRL   0x34
-
-/* Internal sample period (rate) control */
-#define ADIS16201_SMPL_PRD   0x36
-
-/* Operation, filter configuration */
-#define ADIS16201_AVG_CNT0x38
-
-/* Operation, sleep mode control */
-#define ADIS16201_SLP_CNT0x3A
-
-/* Diagnostics, system status register */
-#define ADIS16201_DIAG_STAT  0x3C
-
-/* Operation, system command register */
-#define ADIS16201_GLOB_CMD   0x3E
+#define ADIS16201_STARTUP_DELAY_MS 220
+#define ADIS16201_FLASH_CNT_REG0x00
+
+/* Data Output Register Definitions */
+#define ADIS16201_SUPPLY_OUT_REG   0x02
+#define ADIS16201_XACCL_OUT_REG0x04
+#define ADIS16201_YACCL_OUT_REG0x06
+#define ADIS16201_AUX_ADC_REG  0x08
+#define ADIS16201_TEMP_OUT_REG 0x0A
+#define ADIS16201_XINCL_OUT_REG0x0C
+#define ADIS16201_YINCL_OUT_REG0x0E
+
+/* Calibration Register Definitions */
+#define ADIS16201_XACCL_OFFS_REG   0x10
+#define ADIS16201_YACCL_OFFS_REG   0x12
+#define ADIS16201_XACCL_SCALE_REG  0x14
+#define ADIS16201_YACCL_SCALE_REG  0x16
+#define ADIS16201_XINCL_OFFS_REG   0x18
+#define ADIS16201_YINCL_OFFS_REG   0x1A
+#define ADIS16201_XINCL_SCALE_REG  0x1C
+#define ADIS16201_YINCL_SCALE_REG  0x1E
+
+/* Alarm Register Definitions */
+#define ADIS16201_ALM_MAG1_REG 0x20
+#define ADIS16201_ALM_MAG2_REG 0x22
+#define ADIS16201_ALM_SMPL1_REG0x24
+#define ADIS16201_ALM_SMPL2_REG0x26
+#define ADIS16201_ALM_CTRL_REG 0x28
+
+#define ADIS16201_AUX_DAC_REG  0x30
+#define ADIS16201_GPIO_CTRL_REG0x32
+#define ADIS16201_SMPL_PRD_REG 0x36
+#define ADIS16201_AVG_CNT_REG  0x38
+#define ADIS16201_SLP_CNT_REG  0x3A
 
 /* MSC_CTRL */
-
-/* Self-test enable */
-#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8)
-
-/* Data-ready enable: 1 = enabled, 0 = disabled */
-#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2)
-
-/* Data-ready polarity: 1 = active high, 0 = active low */
-#define 

[PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier

[Himanshu Jha]

Use SPDX identifier format instead of GPLv2. Also, rearrange the headers
in the alphabetical order.

Signed-off-by: Himanshu Jha 
---
 drivers/staging/iio/accel/adis16201.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c 
b/drivers/staging/iio/accel/adis16201.c
index 2ebd275..b2145c9 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -1,19 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
  *
  * Copyright 2010 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
  */
 
 #include 
-#include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
-#include 
 
 #include 
 #include 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Himanshu Jha]

Use sign_extend32 function instead of manually coding it. Also, adjust a
switch block to explicitly match channels and return -EINVAL as default
case which improves code readability.

Signed-off-by: Himanshu Jha 
---
 drivers/staging/iio/accel/adis16201.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c 
b/drivers/staging/iio/accel/adis16201.c
index 011d2c5..6800347 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_VOLTAGE:
-   if (chan->channel == 0) {
+   switch (chan->channel) {
+   case 0:
*val = 1;
*val2 = 22;
-   } else {
+   break;
+   case 1:
*val = 0;
*val2 = 61;
+   break;
+   default:
+   return -EINVAL;
}
return IIO_VAL_INT_PLUS_MICRO;
case IIO_TEMP:
@@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
 
-   val16 &= (1 << bits) - 1;
-   val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-   *val = val16;
+   *val = sign_extend32(val16, bits - 1);
return IIO_VAL_INT;
}
 
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

Move the adis16201 driver out of staging directory and merge to the
mainline IIO directory.

Signed-off-by: Himanshu Jha 
---
 drivers/iio/accel/Kconfig |  12 ++
 drivers/iio/accel/Makefile|   1 +
 drivers/iio/accel/adis16201.c | 315 ++
 drivers/staging/iio/accel/Kconfig |  12 --
 drivers/staging/iio/accel/Makefile|   1 -
 drivers/staging/iio/accel/adis16201.c | 315 --
 6 files changed, 328 insertions(+), 328 deletions(-)
 create mode 100644 drivers/iio/accel/adis16201.c
 delete mode 100644 drivers/staging/iio/accel/adis16201.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index c6d9517..9416c6f 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -5,6 +5,18 @@
 
 menu "Accelerometers"
 
+config ADIS16201
+tristate "Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and 
Accelerometer"
+depends on SPI
+select IIO_ADIS_LIB
+select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
+help
+  Say Y here to build support for Analog Devices adis16201 dual-axis
+  digital inclinometer and accelerometer.
+
+  To compile this driver as a module, say M here: the module will
+  be called adis16201.
+
 config ADXL345
tristate
 
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 368aedb..7832ec9 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_ADIS16201) += adis16201.o
 obj-$(CONFIG_ADXL345) += adxl345_core.o
 obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
 obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
new file mode 100644
index 000..6800347
--- /dev/null
+++ b/drivers/iio/accel/adis16201.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
+ *
+ * Copyright 2010 Analog Devices Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define ADIS16201_STARTUP_DELAY_MS 220
+#define ADIS16201_FLASH_CNT_REG0x00
+
+/* Data Output Register Definitions */
+#define ADIS16201_SUPPLY_OUT_REG   0x02
+#define ADIS16201_XACCL_OUT_REG0x04
+#define ADIS16201_YACCL_OUT_REG0x06
+#define ADIS16201_AUX_ADC_REG  0x08
+#define ADIS16201_TEMP_OUT_REG 0x0A
+#define ADIS16201_XINCL_OUT_REG0x0C
+#define ADIS16201_YINCL_OUT_REG0x0E
+
+/* Calibration Register Definitions */
+#define ADIS16201_XACCL_OFFS_REG   0x10
+#define ADIS16201_YACCL_OFFS_REG   0x12
+#define ADIS16201_XACCL_SCALE_REG  0x14
+#define ADIS16201_YACCL_SCALE_REG  0x16
+#define ADIS16201_XINCL_OFFS_REG   0x18
+#define ADIS16201_YINCL_OFFS_REG   0x1A
+#define ADIS16201_XINCL_SCALE_REG  0x1C
+#define ADIS16201_YINCL_SCALE_REG  0x1E
+
+/* Alarm Register Definitions */
+#define ADIS16201_ALM_MAG1_REG 0x20
+#define ADIS16201_ALM_MAG2_REG 0x22
+#define ADIS16201_ALM_SMPL1_REG0x24
+#define ADIS16201_ALM_SMPL2_REG0x26
+#define ADIS16201_ALM_CTRL_REG 0x28
+
+#define ADIS16201_AUX_DAC_REG  0x30
+#define ADIS16201_GPIO_CTRL_REG0x32
+#define ADIS16201_SMPL_PRD_REG 0x36
+#define ADIS16201_AVG_CNT_REG  0x38
+#define ADIS16201_SLP_CNT_REG  0x3A
+
+/* MSC_CTRL */
+#define ADIS16201_MSC_CTRL_REG 0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_ENBIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN BIT(2)
+#define ADIS16201_MSC_CTRL_ACTIVE_HIGH BIT(1)
+#define ADIS16201_MSC_CTRL_DATA_RDY_DIO1   BIT(0)
+
+/* DIAG_STAT */
+#define ADIS16201_DIAG_STAT_REG0x3C
+#define ADIS16201_DIAG_STAT_ALARM2 BIT(9)
+#define ADIS16201_DIAG_STAT_ALARM1 BIT(8)
+#define ADIS16201_DIAG_STAT_SPI_FAIL_BIT   3
+#define ADIS16201_DIAG_STAT_FLASH_UPT_BIT  2
+#define ADIS16201_DIAG_STAT_POWER_HIGH_BIT 1
+#define ADIS16201_DIAG_STAT_POWER_LOW_BIT  0
+
+/* GLOB_CMD */
+#define ADIS16201_GLOB_CMD_REG 0x3E
+#define ADIS16201_GLOB_CMD_SW_RESETBIT(7)
+#define ADIS16201_GLOB_CMD_FACTORY_CAL BIT(1)
+
+#define ADIS16201_ERROR_ACTIVE BIT(14)
+
+enum adis16201_scan {
+   ADIS16201_SCAN_ACC_X,
+   ADIS16201_SCAN_ACC_Y,
+   ADIS16201_SCAN_INCLI_X,
+   ADIS16201_SCAN_INCLI_Y,
+   ADIS16201_SCAN_SUPPLY,

[PATCH 0/4] staging: iio: accel: adis16201 driver cleanup

[Himanshu Jha]

The following patch series cleans up miscellaneous code fragments and
then the driver is moved from staging to mainline IIO subsytem directory.
Note that the last patch to move driver is *not* generated using '-M' flag,
which is used for detecting renames, since it may help reviewers to
suggest more cleanups/fix while pointing inline to the patch the
necessary changes.

All the patches have been tested using 0-day test service with success.

For all the patches:
Suggested-by: Jonathan Cameron 

Himanshu Jha (4):
  staging: iio: accel: adis16201: Use SPDX identifier
  staging: iio: accel: Remove unnecessary comments and add suitable
suffix
  staging: iio: accel: Use sign_extend32 and adjust a switch statement
  staging: iio: accel: Move adis16201 driver out of staging

 drivers/iio/accel/Kconfig |  12 ++
 drivers/iio/accel/Makefile|   1 +
 drivers/iio/accel/adis16201.c | 315 
 drivers/staging/iio/accel/Kconfig |  12 --
 drivers/staging/iio/accel/Makefile|   1 -
 drivers/staging/iio/accel/adis16201.c | 381 --
 6 files changed, 328 insertions(+), 394 deletions(-)
 create mode 100644 drivers/iio/accel/adis16201.c
 delete mode 100644 drivers/staging/iio/accel/adis16201.c

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: android: ion: Cleanup ion_page_pool_alloc_pages

[Yisheng Xie]

Hi Greg,

JFYI, I have rebase this patchset to v4.15-rc1.[1]

[1] https://lkml.org/lkml/2018/2/12/204

Thanks
Yisheng

On 2018/2/7 11:59, Yisheng Xie wrote:
> ion_page_pool_alloc_pages calls alloc_pages to allocate pages for page
> pools. If alloc_pages return NULL, it will return NULL, or it will
> return the pages allocate from alloc_pages. So we can just return
> alloc_pages without any judgement.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  drivers/staging/android/ion/ion_page_pool.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion_page_pool.c 
> b/drivers/staging/android/ion/ion_page_pool.c
> index e3a6e32..6d2caf0 100644
> --- a/drivers/staging/android/ion/ion_page_pool.c
> +++ b/drivers/staging/android/ion/ion_page_pool.c
> @@ -11,13 +11,9 @@
>  
>  #include "ion.h"
>  
> -static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
> +static inline struct page *ion_page_pool_alloc_pages(struct ion_page_pool 
> *pool)
>  {
> - struct page *page = alloc_pages(pool->gfp_mask, pool->order);
> -
> - if (!page)
> - return NULL;
> - return page;
> + return alloc_pages(pool->gfp_mask, pool->order);
>  }
>  
>  static void ion_page_pool_free_pages(struct ion_page_pool *pool,
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: android: ion: Remove dead code in ion_page_pool_free

[Yisheng Xie]

Hi Greg,

JFYI, I have rebase this patchset to v4.15-rc1.[1]

[1] https://lkml.org/lkml/2018/2/12/204

Thanks
Yisheng

On 2018/2/5 11:26, Yisheng Xie wrote:
> ion_page_pool_add will always return 0, however ion_page_pool_free will
> call ion_page_pool_free_pages when ion_page_pool_add's return value is
> not 0, so it is a dead code which can be removed.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  drivers/staging/android/ion/ion_page_pool.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion_page_pool.c 
> b/drivers/staging/android/ion/ion_page_pool.c
> index 4452e28..150626f 100644
> --- a/drivers/staging/android/ion/ion_page_pool.c
> +++ b/drivers/staging/android/ion/ion_page_pool.c
> @@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool 
> *pool)
>  
>  void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
>  {
> - int ret;
> -
>   BUG_ON(pool->order != compound_order(page));
>  
> - ret = ion_page_pool_add(pool, page);
> - if (ret)
> - ion_page_pool_free_pages(pool, page);
> + ion_page_pool_add(pool, page);
>  }
>  
>  static int ion_page_pool_total(struct ion_page_pool *pool, bool high)
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: android: ion: Avoid NULL point in error path

[Yisheng Xie]

Hi Greg,

JFYI, I have rebase this patchset to v4.15-rc1.[1]

[1] https://lkml.org/lkml/2018/2/12/204

Thanks
Yisheng

On 2018/2/1 20:34, Yisheng Xie wrote:
> If we failed to create debugfs for ion at ion_device_create, the
> debug_root of ion_device will be NULL, and then when try to create debug
> file for shrinker of heap it will be create on the top of debugfs. If we
> also failed to create this the debug file, it call dentry_path to found
> the path of debug_root, then a NULL point will occur.
> 
> Fix this by avoiding call dentry_path, but show the debug name only when
> failed to create debug file for shrinker.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  drivers/staging/android/ion/ion.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index f480885..3e41644 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -570,13 +570,9 @@ void ion_device_add_heap(struct ion_heap *heap)
>   debug_file = debugfs_create_file(
>   debug_name, 0644, dev->debug_root, heap,
>   _shrink_fops);
> - if (!debug_file) {
> - char buf[256], *path;
> -
> - path = dentry_path(dev->debug_root, buf, 256);
> - pr_err("Failed to create heap shrinker debugfs at 
> %s/%s\n",
> -path, debug_name);
> - }
> + if (!debug_file)
> + pr_err("Failed to create ion heap shrinker debugfs at 
> %s\n",
> +debug_name);
>   }
>  
>   dev->heap_cnt++;
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH resend 1/3] staging: android: ion: Remove unused declaration ion_buffer_fault_user_mappings

[Yisheng Xie]

Hi Greg,

JFYI, I have rebase this patchset to v4.15-rc1.[1]

[1] https://lkml.org/lkml/2018/2/12/204

Thanks
Yisheng

On 2018/2/1 9:54, Yisheng Xie wrote:
> ion_buffer_fault_user_mappings's definition has been removed and not be
> used anymore, just remove its useless declaration.
> 
> Signed-off-by: Yisheng Xie 
> ---
>  drivers/staging/android/ion/ion.h | 9 -
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.h 
> b/drivers/staging/android/ion/ion.h
> index f5f9cd6..2160c35 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -201,15 +201,6 @@ struct ion_heap {
>  bool ion_buffer_cached(struct ion_buffer *buffer);
>  
>  /**
> - * ion_buffer_fault_user_mappings - fault in user mappings of this buffer
> - * @buffer:  buffer
> - *
> - * indicates whether userspace mappings of this buffer will be faulted
> - * in, this can affect how buffers are allocated from the heap.
> - */
> -bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
> -
> -/**
>   * ion_device_add_heap - adds a heap to the ion device
>   * @heap:the heap to add
>   */
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/9] staging: android: ion: Some cleanup about ion

[Yisheng Xie]

Hi Dan,

On 2018/2/12 19:17, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote:
>> Hi all,
>>
>> When I tried to review the code of ion, I find that there are many place
>> can be cleanup, so I post this patchset.
>>
>> I mark it as v2, because I have post them in different patchset before[1][2]
>> [3][4]. And now, I combine them in one patchset to make it easy to review,
>> Meanwhile, I add some acks and rebase them to v4.15-rc1.
>>
>> Thanks.
>>
>> [1] https://lkml.org/lkml/2018/1/31/711
>> [2] https://lkml.org/lkml/2018/2/1/240
>> [3] https://lkml.org/lkml/2018/2/4/320
>> [4] https://lkml.org/lkml/2018/2/6/949
>>
> 
> Could you go back and reply to these threads that you have redone the
> patches?  Otherwise, what's likely to happen is that Greg will apply
> as many as he can get to this series and it won't apply.

Sure! thanks

Yisheng
> 
> regards,
> dan carpenter
> 
> 
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: vt6656: Remove unnecessary 'out of memory' message

[Dileep Sankhla]

This patch removes the unnecessary out of memory message fixing the
following checkpatch.pl warning in usbpipe.c:
WARNING: Possible unnecessary 'out of memory' message

Signed-off-by: Dileep Sankhla 
---
Change in v2:
- changed commit message
---
 drivers/staging/vt6656/usbpipe.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
index 273176386a51..5bbc56f8779e 100644
--- a/drivers/staging/vt6656/usbpipe.c
+++ b/drivers/staging/vt6656/usbpipe.c
@@ -194,9 +194,6 @@ static void vnt_submit_rx_urb_complete(struct urb *urb)
if (vnt_rx_data(priv, rcb, urb->actual_length)) {
rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
if (!rcb->skb) {
-   dev_dbg(>usb->dev,
-   "Failed to re-alloc rx skb\n");
-
rcb->in_use = false;
return;
}
-- 
2.15.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/9] staging: android: ion: Some cleanup about ion

[Dan Carpenter]

On Mon, Feb 12, 2018 at 06:43:05PM +0800, Yisheng Xie wrote:
> Hi all,
> 
> When I tried to review the code of ion, I find that there are many place
> can be cleanup, so I post this patchset.
> 
> I mark it as v2, because I have post them in different patchset before[1][2]
> [3][4]. And now, I combine them in one patchset to make it easy to review,
> Meanwhile, I add some acks and rebase them to v4.15-rc1.
> 
> Thanks.
> 
> [1] https://lkml.org/lkml/2018/1/31/711
> [2] https://lkml.org/lkml/2018/2/1/240
> [3] https://lkml.org/lkml/2018/2/4/320
> [4] https://lkml.org/lkml/2018/2/6/949
> 

Could you go back and reply to these threads that you have redone the
patches?  Otherwise, what's likely to happen is that Greg will apply
as many as he can get to this series and it won't apply.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 9/9] staging: android: ion: Combine cache and uncache pools

[Yisheng Xie]

Now we call dma_map in the dma_buf API callbacks and handle explicit
caching by the dma_buf sync API, which make cache and uncache pools
in the same handling flow, which can be combined.

Acked-by: Sumit Semwal 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion.c |  5 --
 drivers/staging/android/ion/ion.h | 13 +
 drivers/staging/android/ion/ion_page_pool.c   |  5 +-
 drivers/staging/android/ion/ion_system_heap.c | 76 +--
 4 files changed, 16 insertions(+), 83 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 461b193..c094be2 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -33,11 +33,6 @@
 static struct ion_device *internal_dev;
 static int heap_id;
 
-bool ion_buffer_cached(struct ion_buffer *buffer)
-{
-   return !!(buffer->flags & ION_FLAG_CACHED);
-}
-
 /* this function should only be called while dev->lock is held */
 static void ion_buffer_add(struct ion_device *dev,
   struct ion_buffer *buffer)
diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index 1bc443f..ea08978 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -185,14 +185,6 @@ struct ion_heap {
 };
 
 /**
- * ion_buffer_cached - this ion buffer is cached
- * @buffer:buffer
- *
- * indicates whether this ion buffer is cached
- */
-bool ion_buffer_cached(struct ion_buffer *buffer);
-
-/**
  * ion_device_add_heap - adds a heap to the ion device
  * @heap:  the heap to add
  */
@@ -302,7 +294,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
  * @gfp_mask:  gfp_mask to use from alloc
  * @order: order of pages in the pool
  * @list:  plist node for list of pools
- * @cached:it's cached pool or not
  *
  * Allows you to keep a pool of pre allocated pages to use from your heap.
  * Keeping a pool of pages that is ready for dma, ie any cached mapping have
@@ -312,7 +303,6 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
 struct ion_page_pool {
int high_count;
int low_count;
-   bool cached;
struct list_head high_items;
struct list_head low_items;
struct mutex mutex;
@@ -321,8 +311,7 @@ struct ion_page_pool {
struct plist_node list;
 };
 
-struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
-  bool cached);
+struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order);
 void ion_page_pool_destroy(struct ion_page_pool *pool);
 struct page *ion_page_pool_alloc(struct ion_page_pool *pool);
 void ion_page_pool_free(struct ion_page_pool *pool, struct page *page);
diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index 6d2caf0..db8f614 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -123,8 +123,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t 
gfp_mask,
return freed;
 }
 
-struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order,
-  bool cached)
+struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, unsigned int order)
 {
struct ion_page_pool *pool = kmalloc(sizeof(*pool), GFP_KERNEL);
 
@@ -138,8 +137,6 @@ struct ion_page_pool *ion_page_pool_create(gfp_t gfp_mask, 
unsigned int order,
pool->order = order;
mutex_init(>mutex);
plist_node_init(>list, order);
-   if (cached)
-   pool->cached = true;
 
return pool;
 }
diff --git a/drivers/staging/android/ion/ion_system_heap.c 
b/drivers/staging/android/ion/ion_system_heap.c
index bc19cdd..701eb9f 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -41,31 +41,16 @@ static inline unsigned int order_to_size(int order)
 
 struct ion_system_heap {
struct ion_heap heap;
-   struct ion_page_pool *uncached_pools[NUM_ORDERS];
-   struct ion_page_pool *cached_pools[NUM_ORDERS];
+   struct ion_page_pool *pools[NUM_ORDERS];
 };
 
-/**
- * The page from page-pool are all zeroed before. We need do cache
- * clean for cached buffer. The uncached buffer are always non-cached
- * since it's allocated. So no need for non-cached pages.
- */
 static struct page *alloc_buffer_page(struct ion_system_heap *heap,
  struct ion_buffer *buffer,
  unsigned long order)
 {
-   bool cached = ion_buffer_cached(buffer);
-   struct ion_page_pool *pool;
-   struct page *page;
+   struct ion_page_pool *pool = heap->pools[order_to_index(order)];
 
-   if (!cached)
-   pool = 

[PATCH v2 8/9] staging: android: ion: Cleanup ion_page_pool_alloc_pages

[Yisheng Xie]

ion_page_pool_alloc_pages calls alloc_pages to allocate pages for page
pools. If alloc_pages return NULL, it will return NULL, or it will
return the pages allocate from alloc_pages. So we can just return
alloc_pages without any judgement.

Acked-by: Sumit Semwal 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion_page_pool.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index e3a6e32..6d2caf0 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -11,13 +11,9 @@
 
 #include "ion.h"
 
-static void *ion_page_pool_alloc_pages(struct ion_page_pool *pool)
+static inline struct page *ion_page_pool_alloc_pages(struct ion_page_pool 
*pool)
 {
-   struct page *page = alloc_pages(pool->gfp_mask, pool->order);
-
-   if (!page)
-   return NULL;
-   return page;
+   return alloc_pages(pool->gfp_mask, pool->order);
 }
 
 static void ion_page_pool_free_pages(struct ion_page_pool *pool,
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 7/9] staging: android: ion: Return void instead of int

[Yisheng Xie]

Now, nobody care about the return value of ion_page_pool_add, therefore
we can just make it return void.

Acked-by: Laura Abbott 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion_page_pool.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index 150626f..e3a6e32 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -26,7 +26,7 @@ static void ion_page_pool_free_pages(struct ion_page_pool 
*pool,
__free_pages(page, pool->order);
 }
 
-static int ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
+static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
 {
mutex_lock(>mutex);
if (PageHighMem(page)) {
@@ -37,7 +37,6 @@ static int ion_page_pool_add(struct ion_page_pool *pool, 
struct page *page)
pool->low_count++;
}
mutex_unlock(>mutex);
-   return 0;
 }
 
 static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 0/9] staging: android: ion: Some cleanup about ion

[Yisheng Xie]

Hi all,

When I tried to review the code of ion, I find that there are many place
can be cleanup, so I post this patchset.

I mark it as v2, because I have post them in different patchset before[1][2]
[3][4]. And now, I combine them in one patchset to make it easy to review,
Meanwhile, I add some acks and rebase them to v4.15-rc1.

Thanks.

[1] https://lkml.org/lkml/2018/1/31/711
[2] https://lkml.org/lkml/2018/2/1/240
[3] https://lkml.org/lkml/2018/2/4/320
[4] https://lkml.org/lkml/2018/2/6/949

Yisheng Xie (9):
  staging: android: ion: Remove unused declaration
ion_buffer_fault_user_mappings
  staging: android: ion: Remove unused include files for
ion_page_pool.c
  staging: android: ion: Nuke ion_page_pool_init
  staging: android: ion: Avoid NULL point in error path
  staging: android: ion: Remove lable debugfs_done
  staging: android: ion: Remove dead code in ion_page_pool_free
  staging: android: ion: Return void instead of int
  staging: android: ion: Cleanup ion_page_pool_alloc_pages
  staging: android: ion: Combine cache and uncache pools

 drivers/staging/android/ion/ion.c | 20 ++-
 drivers/staging/android/ion/ion.h | 22 +---
 drivers/staging/android/ion/ion_page_pool.c   | 33 ++--
 drivers/staging/android/ion/ion_system_heap.c | 76 +--
 4 files changed, 24 insertions(+), 127 deletions(-)

-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 5/9] staging: android: ion: Remove lable debugfs_done

[Yisheng Xie]

When failed to create debug_root, we will go on initail other part of
ion, so we can just info this message to user and do not need a lable
to jump.

Acked-by: Laura Abbott 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 4b69372..461b193 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -595,12 +595,9 @@ static int ion_device_create(void)
}
 
idev->debug_root = debugfs_create_dir("ion", NULL);
-   if (!idev->debug_root) {
+   if (!idev->debug_root)
pr_err("ion: failed to create debugfs root directory.\n");
-   goto debugfs_done;
-   }
 
-debugfs_done:
idev->buffers = RB_ROOT;
mutex_init(>buffer_lock);
init_rwsem(>lock);
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/9] staging: android: ion: Remove unused include files for ion_page_pool.c

[Yisheng Xie]

After rewrite of ion_page_pool, some of its include file is no need
anymore, just remove it.

Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion_page_pool.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index b3017f1..f0847b3 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -5,10 +5,6 @@
  * Copyright (C) 2011 Google, Inc.
  */
 
-#include 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 4/9] staging: android: ion: Avoid NULL point in error path

[Yisheng Xie]

If we failed to create debugfs for ion at ion_device_create, the
debug_root of ion_device will be NULL, and then when try to create debug
file for shrinker of heap it will be create on the top of debugfs. If we
also failed to create this the debug file, it call dentry_path to found
the path of debug_root, then a NULL point will occur.

Fix this by avoiding call dentry_path, but show the debug name only when
failed to create debug file for shrinker.

Acked-by: Laura Abbott 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c 
b/drivers/staging/android/ion/ion.c
index 57e0d80..4b69372 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -564,13 +564,9 @@ void ion_device_add_heap(struct ion_heap *heap)
debug_file = debugfs_create_file(debug_name,
 0644, dev->debug_root, heap,
 _shrink_fops);
-   if (!debug_file) {
-   char buf[256], *path;
-
-   path = dentry_path(dev->debug_root, buf, 256);
-   pr_err("Failed to create heap shrinker debugfs at 
%s/%s\n",
-  path, debug_name);
-   }
+   if (!debug_file)
+   pr_err("Failed to create ion heap shrinker debugfs at 
%s\n",
+  debug_name);
}
 
dev->heap_cnt++;
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/9] staging: android: ion: Remove unused declaration ion_buffer_fault_user_mappings

[Yisheng Xie]

ion_buffer_fault_user_mappings's definition has been removed and not be
used anymore, just remove its useless declaration.

Acked-by: Laura Abbott 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion.h | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/staging/android/ion/ion.h 
b/drivers/staging/android/ion/ion.h
index a238f23..1bc443f 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -193,15 +193,6 @@ struct ion_heap {
 bool ion_buffer_cached(struct ion_buffer *buffer);
 
 /**
- * ion_buffer_fault_user_mappings - fault in user mappings of this buffer
- * @buffer:buffer
- *
- * indicates whether userspace mappings of this buffer will be faulted
- * in, this can affect how buffers are allocated from the heap.
- */
-bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer);
-
-/**
  * ion_device_add_heap - adds a heap to the ion device
  * @heap:  the heap to add
  */
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 6/9] staging: android: ion: Remove dead code in ion_page_pool_free

[Yisheng Xie]

ion_page_pool_add will always return 0, however ion_page_pool_free will
call ion_page_pool_free_pages when ion_page_pool_add's return value is
not 0, so it is a dead code which can be removed.

Acked-by: Laura Abbott 
Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion_page_pool.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index 4452e28..150626f 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -79,13 +79,9 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool)
 
 void ion_page_pool_free(struct ion_page_pool *pool, struct page *page)
 {
-   int ret;
-
BUG_ON(pool->order != compound_order(page));
 
-   ret = ion_page_pool_add(pool, page);
-   if (ret)
-   ion_page_pool_free_pages(pool, page);
+   ion_page_pool_add(pool, page);
 }
 
 static int ion_page_pool_total(struct ion_page_pool *pool, bool high)
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/9] staging: android: ion: Nuke ion_page_pool_init

[Yisheng Xie]

ion_page_pool.c now is used to apply pool APIs for system heap, which do
not need do any initial at device_initcall. Therefore ion_page_pool_init
can be nuked.

Signed-off-by: Yisheng Xie 
---
 drivers/staging/android/ion/ion_page_pool.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/android/ion/ion_page_pool.c 
b/drivers/staging/android/ion/ion_page_pool.c
index f0847b3..4452e28 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -6,7 +6,6 @@
  */
 
 #include 
-#include 
 #include 
 #include 
 
@@ -158,9 +157,3 @@ void ion_page_pool_destroy(struct ion_page_pool *pool)
 {
kfree(pool);
 }
-
-static int __init ion_page_pool_init(void)
-{
-   return 0;
-}
-device_initcall(ion_page_pool_init);
-- 
1.7.12.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/12] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Dan Carpenter]

On Sun, Feb 11, 2018 at 05:33:16PM -0700, k...@exchange.microsoft.com wrote:
> @@ -116,9 +146,29 @@ static int hv_ce_set_oneshot(struct clock_event_device 
> *evt)
>  {
>   union hv_timer_config timer_cfg;
>  
> + timer_cfg.as_uint64 = 0;
>   timer_cfg.enable = 1;
>   timer_cfg.auto_enable = 1;
> - timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> + if (direct_mode_enabled)
> + /*
> +  * When it expires, the timer will directly interrupt
> +  * on the specified hardware vector/IRQ.
> +  */
> + {
> + timer_cfg.direct_mode = 1;
> + timer_cfg.apic_vector = stimer0_vector;
> + hv_enable_stimer0_percpu_irq(stimer0_irq);
> + }
> + else
> + /*
> +  * When it expires, the timer will generate a VMbus message,
> +  * to be handled by the normal VMbus interrupt handler.
> +  */
> + {
> + timer_cfg.direct_mode = 0;
> + timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> + }
> +

This indenting isn't right.  We should probably zero out .apic_vector
if .direct_mode is zero.  Or maybe it's fine.  I don't know if any
static analysis tools will complain...

>   hv_init_timer_config(HV_X64_MSR_STIMER0_CONFIG, timer_cfg.as_uint64);
>  
>   return 0;
> @@ -191,6 +241,10 @@ int hv_synic_alloc(void)
>   INIT_LIST_HEAD(_cpu->chan_list);
>   }
>  
> + if (direct_mode_enabled && hv_setup_stimer0_irq(
> + _irq, _vector, hv_stimer0_isr))
> + goto err;


Can you indent it like this:

if (direct_mode_enabled &&
hv_setup_stimer0_irq(_irq, _vector,
 hv_stimer0_isr))
goto err;


[ What follows is a long rant not directed at you ]

It's annoying because as soon as I see the "goto err;", I know the error
handling for this function is going to be buggy...

Some rules of error handling are:

1)  Each function should clean up after itself instead returning
partially allocated structures.
2)  Each allocation function should have a matching free function.
3)  Give meaningful label names based on what the label location so that
we can tell what the goto does just by looking at it, such as,
"goto free_some_variable".  This way we can just keep a mental tally
of the most recently allocated resource and verify based on the
"goto free_resource;" statemetn that it frees the correct thing.  We
don't need to scroll to the bottom of the function.

Using good names means that we should avoid do-nothing gotos
because, by definition, the label name for a do-nothing goto is
going to be vague.

In this case the label looks like this:

> +
>   return 0;
>  err:
>   return -ENOMEM;

We allocate a bunch of stuff in this function so at first glance this
looks like we leak everything but, actually, the cleanup is done in
vmbus_bus_init().  This is a layering violation.

Later on, we changed hv_synic_alloc() in 37cdd991fac8 ("vmbus: put
related per-cpu variable together") and we started allocating:

hv_cpu->clk_evt = kzalloc(...

but we forgot to update the error handling because it was in the wrong
place.  It's a very predictable, avoidable bug if we just use proper
error handling style.

Anyway...  Sorry for the long rant.  Summary:  Always distrust vague
label names.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel