Re: fsl-dcu not works on latest "drm-next"

[Stefan Agner]

On 2016-05-26 02:11, Alexander Stein wrote:
> On Thursday 26 May 2016 08:23:42, Meng Yi wrote:
>> Hi Mark,
>>
>> > You've not specifically described the problem here - what are the
>> > endiannesses of both the CPU and the device you're talking to?  What
>> > specifically is the endianess problem you are seeing, what are you seeing
>> > and what do you expect to see?
>>
>> The CPU is little endian and the device DCU is big endian, specified
>> big-endian in DTS,
>>
>> And here is my DTS and regmap_config,
>>
>> Specified "big-endian" in DTS,
>>
>> dcu: dcu@2ce {
>> compatible = "fsl,ls1021a-dcu";
>> reg = <0x0 0x2ce 0x0 0x1>;
>> interrupts = ;
>> clocks = <_clk 0>;
>> clock-names = "dcu";
>> big-endian;
>> status = "disabled";
>> };
>>
>> I can't tell the difference of "reg_format_endian" and " val_format_endian
>> ", so I had tried four conditions. And all failed.
>>
>> static const struct regmap_config fsl_dcu_regmap_config = {
>> .reg_bits = 32,
>> .reg_stride = 4,
>> .val_bits = 32,
>> .cache_type = REGCACHE_RBTREE,
> 
> This needs to be a flat cache. See
> https://lists.freedesktop.org/archives/dri-devel/2016-January/099121.html
> or https://lkml.org/lkml/2016/3/24/281
> max_register also needs an appropriate value.

FWIW, the latest patch which addresses this issue is Patch 5/6 of this
patchset:
https://lkml.org/lkml/2016/4/19/617

Unfortunately, that patchset missed the 4.7 merge window. I still miss
an Ack for the first patch of this patchset. But should go into the next
release...

--
Stefan

Re: fsl-dcu not works on latest "drm-next"

[Stefan Agner]

On 2016-05-26 02:11, Alexander Stein wrote:
> On Thursday 26 May 2016 08:23:42, Meng Yi wrote:
>> Hi Mark,
>>
>> > You've not specifically described the problem here - what are the
>> > endiannesses of both the CPU and the device you're talking to?  What
>> > specifically is the endianess problem you are seeing, what are you seeing
>> > and what do you expect to see?
>>
>> The CPU is little endian and the device DCU is big endian, specified
>> big-endian in DTS,
>>
>> And here is my DTS and regmap_config,
>>
>> Specified "big-endian" in DTS,
>>
>> dcu: dcu@2ce {
>> compatible = "fsl,ls1021a-dcu";
>> reg = <0x0 0x2ce 0x0 0x1>;
>> interrupts = ;
>> clocks = <_clk 0>;
>> clock-names = "dcu";
>> big-endian;
>> status = "disabled";
>> };
>>
>> I can't tell the difference of "reg_format_endian" and " val_format_endian
>> ", so I had tried four conditions. And all failed.
>>
>> static const struct regmap_config fsl_dcu_regmap_config = {
>> .reg_bits = 32,
>> .reg_stride = 4,
>> .val_bits = 32,
>> .cache_type = REGCACHE_RBTREE,
> 
> This needs to be a flat cache. See
> https://lists.freedesktop.org/archives/dri-devel/2016-January/099121.html
> or https://lkml.org/lkml/2016/3/24/281
> max_register also needs an appropriate value.

FWIW, the latest patch which addresses this issue is Patch 5/6 of this
patchset:
https://lkml.org/lkml/2016/4/19/617

Unfortunately, that patchset missed the 4.7 merge window. I still miss
an Ack for the first patch of this patchset. But should go into the next
release...

--
Stefan

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Heiko Carstens]

On Wed, May 25, 2016 at 12:30:17PM -0700, David Miller wrote:
> From: Yury Norov 
> Date: Tue, 24 May 2016 03:04:30 +0300
> 
> > +To clear that top halves, automatic wrappers are introduced. They clear all
> > +required registers before passing control to regular syscall handler.
> 
> Why have one of these for every single compat system call, rather than
> simply clearing the top half of all of these registers unconditionally
> in the 32-bit system call trap before the system call is invoked?
> 
> That's what we do on sparc64.
> 
> And with that, you only need wrappers for the case where there needs
> to be proper sign extention of a 32-bit signed argument.

That would probably also work for arm. On s390 we still have these odd 31
bit pointers in compat mode which require us to clear 33 bits instead of 32
bits. That makes up for appr. one third of all system calls.

The additional wrappers are only for zero/sign extension, where I count a
total of 27 on s390.

The reason for doing this in C was the constant copy-paste error rate, when
adding new system calls plus I got a rid of a lot of unnecessary asm code.

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Heiko Carstens]

On Wed, May 25, 2016 at 12:30:17PM -0700, David Miller wrote:
> From: Yury Norov 
> Date: Tue, 24 May 2016 03:04:30 +0300
> 
> > +To clear that top halves, automatic wrappers are introduced. They clear all
> > +required registers before passing control to regular syscall handler.
> 
> Why have one of these for every single compat system call, rather than
> simply clearing the top half of all of these registers unconditionally
> in the 32-bit system call trap before the system call is invoked?
> 
> That's what we do on sparc64.
> 
> And with that, you only need wrappers for the case where there needs
> to be proper sign extention of a 32-bit signed argument.

That would probably also work for arm. On s390 we still have these odd 31
bit pointers in compat mode which require us to clear 33 bits instead of 32
bits. That makes up for appr. one third of all system calls.

The additional wrappers are only for zero/sign extension, where I count a
total of 27 on s390.

The reason for doing this in C was the constant copy-paste error rate, when
adding new system calls plus I got a rid of a lot of unnecessary asm code.

Re: [PATCH v7 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

Hi CK,

Reply in line.

On Thu, 2016-05-26 at 15:28 +0800, CK Hu wrote:
> Hi, HS:
> 
> Replay inline.
> 
> On Tue, 2016-05-24 at 20:27 +0800, Horng-Shyang Liao wrote:
> > Hi CK,
> > 
> > Reply in line.
> > 
> > On Tue, 2016-05-24 at 11:05 +0800, CK Hu wrote:
> > > Hi, HS:
> > > 
> > > Some comments below.
> > > 
> > ...
> > > > +static void cmdq_thread_irq_handler(struct cmdq *cmdq, int tid)
> > > > +{
> > > > +   struct cmdq_thread *thread = >thread[tid];
> > > > +   unsigned long flags = 0L;
> > > > +   int value;
> > > > +
> > > > +   spin_lock_irqsave(>exec_lock, flags);
> > > > +
> > > > +   /*
> > > > +* it is possible for another CPU core
> > > > +* to run "release task" right before we acquire the spin lock
> > > > +* and thus reset / disable this HW thread
> > > > +* so we check both the IRQ flag and the enable bit of this 
> > > > thread
> > > > +*/
> > > > +   value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
> > > > +   if (!(value & CMDQ_THR_IRQ_MASK)) {
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +   return;
> > > > +   }
> > > 
> > > If this case happen and just return without clearing irq status, the irq
> > > would keep triggering and system hang up. So just remove this checking
> > > and go down to clear irq status.
> > 
> > This case is safe because irq status is cleared.
> > But, next if condition has the problem which you mentioned.
> > 
> > I will change it as below.
> > 
> > if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> >   CMDQ_THR_ENABLED)) {
> > cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > spin_unlock_irqrestore(>exec_lock, flags);
> > return;
> > }
> > 
> > If thread is disabled, tasks must be all finished.
> > Therefore, just clear irq status and return.
> > 
> 
> For irq status checking part, if irq status & irq mask is zero, remove
> this checking and let it go down, it still do nothing because value &
> CMDQ_THR_IRQ_ERROR is zero and value & CMDQ_THR_IRQ_DONE is zero. So you
> can just remove this checking and get the same result.
> 
> In general HW design, once a HW is not enable, it does not trigger
> interrupt any more. Be sure that it's necessary to clear irq status even
> though thread is disable.
> 

I Will remove first if condition,
so rewrite first two if condition parts as below.

value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);

if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
CMDQ_THR_ENABLED))
value = 0;

> > > > +
> > > > +   if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> > > > + CMDQ_THR_ENABLED)) {
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +   return;
> > > > +   }
> > > > +
> > > > +   cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > > > +
> > > > +   if (value & CMDQ_THR_IRQ_ERROR)
> > > > +   cmdq_handle_error_done(cmdq, thread, true);
> > > > +   else if (value & CMDQ_THR_IRQ_DONE)
> > > > +   cmdq_handle_error_done(cmdq, thread, false);
> > > 
> > > These irq status checking and clearing code here is the same as those in
> > > cmdq_task_handle_error_result(). To move the checking and clearing code
> > > into cmdq_handle_error_done() and here just to call
> > > cmdq_handle_error_done(cmdq, thread) would reduce duplicated code.
> > 
> > Will do.
> > 
> > > > +
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +}
> > ...
> > ...
> > > 
> > > Regards,
> > > CK
> > 
> > Thanks,
> > HS
> > 
> 
> Regards,
> CK
> 
> 

Thanks,
HS

Re: [PATCH v7 2/4] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

Hi CK,

Reply in line.

On Thu, 2016-05-26 at 15:28 +0800, CK Hu wrote:
> Hi, HS:
> 
> Replay inline.
> 
> On Tue, 2016-05-24 at 20:27 +0800, Horng-Shyang Liao wrote:
> > Hi CK,
> > 
> > Reply in line.
> > 
> > On Tue, 2016-05-24 at 11:05 +0800, CK Hu wrote:
> > > Hi, HS:
> > > 
> > > Some comments below.
> > > 
> > ...
> > > > +static void cmdq_thread_irq_handler(struct cmdq *cmdq, int tid)
> > > > +{
> > > > +   struct cmdq_thread *thread = >thread[tid];
> > > > +   unsigned long flags = 0L;
> > > > +   int value;
> > > > +
> > > > +   spin_lock_irqsave(>exec_lock, flags);
> > > > +
> > > > +   /*
> > > > +* it is possible for another CPU core
> > > > +* to run "release task" right before we acquire the spin lock
> > > > +* and thus reset / disable this HW thread
> > > > +* so we check both the IRQ flag and the enable bit of this 
> > > > thread
> > > > +*/
> > > > +   value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
> > > > +   if (!(value & CMDQ_THR_IRQ_MASK)) {
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +   return;
> > > > +   }
> > > 
> > > If this case happen and just return without clearing irq status, the irq
> > > would keep triggering and system hang up. So just remove this checking
> > > and go down to clear irq status.
> > 
> > This case is safe because irq status is cleared.
> > But, next if condition has the problem which you mentioned.
> > 
> > I will change it as below.
> > 
> > if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> >   CMDQ_THR_ENABLED)) {
> > cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > spin_unlock_irqrestore(>exec_lock, flags);
> > return;
> > }
> > 
> > If thread is disabled, tasks must be all finished.
> > Therefore, just clear irq status and return.
> > 
> 
> For irq status checking part, if irq status & irq mask is zero, remove
> this checking and let it go down, it still do nothing because value &
> CMDQ_THR_IRQ_ERROR is zero and value & CMDQ_THR_IRQ_DONE is zero. So you
> can just remove this checking and get the same result.
> 
> In general HW design, once a HW is not enable, it does not trigger
> interrupt any more. Be sure that it's necessary to clear irq status even
> though thread is disable.
> 

I Will remove first if condition,
so rewrite first two if condition parts as below.

value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);

if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
CMDQ_THR_ENABLED))
value = 0;

> > > > +
> > > > +   if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> > > > + CMDQ_THR_ENABLED)) {
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +   return;
> > > > +   }
> > > > +
> > > > +   cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > > > +
> > > > +   if (value & CMDQ_THR_IRQ_ERROR)
> > > > +   cmdq_handle_error_done(cmdq, thread, true);
> > > > +   else if (value & CMDQ_THR_IRQ_DONE)
> > > > +   cmdq_handle_error_done(cmdq, thread, false);
> > > 
> > > These irq status checking and clearing code here is the same as those in
> > > cmdq_task_handle_error_result(). To move the checking and clearing code
> > > into cmdq_handle_error_done() and here just to call
> > > cmdq_handle_error_done(cmdq, thread) would reduce duplicated code.
> > 
> > Will do.
> > 
> > > > +
> > > > +   spin_unlock_irqrestore(>exec_lock, flags);
> > > > +}
> > ...
> > ...
> > > 
> > > Regards,
> > > CK
> > 
> > Thanks,
> > HS
> > 
> 
> Regards,
> CK
> 
> 

Thanks,
HS

Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

[Wanpeng Li]

2016-05-26 10:53 GMT+08:00 Steve Muckle :
> The slow-path frequency transition path is relatively expensive as it
> requires waking up a thread to do work. Should support be added for
> remote CPU cpufreq updates that is also expensive since it requires an
> IPI. These activities should be avoided if they are not necessary.
>
> To that end, calculate the actual driver-supported frequency required by
> the new utilization value in schedutil by using the recently added
> cpufreq_driver_resolve_freq callback. If it is the same as the
> previously requested driver frequency then there is no need to continue
> with the update assuming the cpu frequency limits have not changed. This
> will have additional benefits should the semantics of the rate limit be
> changed to apply solely to frequency transitions rather than to
> frequency calculations in schedutil.

sugov_should_update_freq() still be called before get_nex_freq() after
the patch applied, so rate limit still apply to both frequency
transitions and frequency calculations, right?

Regards,
Wanpeng Li

Re: [PATCH v2 3/3] cpufreq: schedutil: map raw required frequency to driver frequency

[Wanpeng Li]

2016-05-26 10:53 GMT+08:00 Steve Muckle :
> The slow-path frequency transition path is relatively expensive as it
> requires waking up a thread to do work. Should support be added for
> remote CPU cpufreq updates that is also expensive since it requires an
> IPI. These activities should be avoided if they are not necessary.
>
> To that end, calculate the actual driver-supported frequency required by
> the new utilization value in schedutil by using the recently added
> cpufreq_driver_resolve_freq callback. If it is the same as the
> previously requested driver frequency then there is no need to continue
> with the update assuming the cpu frequency limits have not changed. This
> will have additional benefits should the semantics of the rate limit be
> changed to apply solely to frequency transitions rather than to
> frequency calculations in schedutil.

sugov_should_update_freq() still be called before get_nex_freq() after
the patch applied, so rate limit still apply to both frequency
transitions and frequency calculations, right?

Regards,
Wanpeng Li

Re: [PATCH v3 5/6] mm/cma: remove MIGRATE_CMA

[Joonsoo Kim]

On Fri, May 27, 2016 at 09:42:24AM +0800, Chen Feng wrote:
> Hi Joonsoo,
> > -/* Free whole pageblock and set its migration type to MIGRATE_CMA. */
> > +/* Free whole pageblock and set its migration type to MIGRATE_MOVABLE. */
> >  void __init init_cma_reserved_pageblock(struct page *page)
> >  {
> > unsigned i = pageblock_nr_pages;
> > @@ -1605,7 +1602,7 @@ void __init init_cma_reserved_pageblock(struct page 
> > *page)
> >  
> > adjust_present_page_count(page, pageblock_nr_pages);
> >  
> > -   set_pageblock_migratetype(page, MIGRATE_CMA);
> > +   set_pageblock_migratetype(page, MIGRATE_MOVABLE);
> 
> I have a question here, if the ZONE_CMA pages are all movable.
> 
> Then the unmovable alloc will also use CMA memory. Is this right?

No, previous patch changes that the CMA memory is on separate zone,
ZONE_CMA. We allow that zone when gfp is GFP_HIGHUSER_MOVABLE so
unmovable allocation cannot happen on CMA memory.

Thanks.

Re: [PATCH v3 5/6] mm/cma: remove MIGRATE_CMA

[Joonsoo Kim]

On Fri, May 27, 2016 at 09:42:24AM +0800, Chen Feng wrote:
> Hi Joonsoo,
> > -/* Free whole pageblock and set its migration type to MIGRATE_CMA. */
> > +/* Free whole pageblock and set its migration type to MIGRATE_MOVABLE. */
> >  void __init init_cma_reserved_pageblock(struct page *page)
> >  {
> > unsigned i = pageblock_nr_pages;
> > @@ -1605,7 +1602,7 @@ void __init init_cma_reserved_pageblock(struct page 
> > *page)
> >  
> > adjust_present_page_count(page, pageblock_nr_pages);
> >  
> > -   set_pageblock_migratetype(page, MIGRATE_CMA);
> > +   set_pageblock_migratetype(page, MIGRATE_MOVABLE);
> 
> I have a question here, if the ZONE_CMA pages are all movable.
> 
> Then the unmovable alloc will also use CMA memory. Is this right?

No, previous patch changes that the CMA memory is on separate zone,
ZONE_CMA. We allow that zone when gfp is GFP_HIGHUSER_MOVABLE so
unmovable allocation cannot happen on CMA memory.

Thanks.

Re: [PATCH v3 0/6] Introduce ZONE_CMA

[Joonsoo Kim]

On Thu, May 26, 2016 at 04:04:54PM +0800, Feng Tang wrote:
> On Thu, May 26, 2016 at 02:22:22PM +0800, js1...@gmail.com wrote:
> > From: Joonsoo Kim 
> 
> Hi Joonsoo,
> 
> Nice work!

Thanks!

> > FYI, there is another attempt [3] trying to solve this problem in lkml.
> > And, as far as I know, Qualcomm also has out-of-tree solution for this
> > problem.
> 
> This may be a little off-topic :) Actually, we have used another way in
> our products, that we disable the fallback from MIGRATETYE_MOVABLE to
> MIGRATETYPE_CMA completely, and only allow free CMA memory to be used
> by file page cache (which is easy to be reclaimed by its nature). 
> We did it by adding a GFP_PAGE_CACHE to every allocation request for
> page cache, and the MM will try to pick up an available free CMA page
> first, and goes to normal path when fail. 

Just wonder, why do you allow CMA memory to file page cache rather
than anonymous page? I guess that anonymous pages would be more easily
migrated/reclaimed than file page cache. In fact, some of our product
uses anonymous page adaptation to satisfy similar requirement by
introducing GFP_CMA. AFAIK, some of chip vendor also uses "anonymous
page first adaptation" to get better success rate.

> It works fine on our products, though we still see some cases that
> some page can't be reclaimed. 
> 
> Our product has a special user case of CMA, that sometimes it will
> need to use the whole CMA memory (say 256MB on a phone), then all

I don't think this usecase is so special. Our product also has similar
usecase. And, I already knows one another.

> share out CMA pages need to be reclaimed all at once. Don't know if
> this new ZONE_CMA approach could meet this request? (our page cache
> solution can't ganrantee to meet this request all the time).

This ZONE_CMA approach would be better than before, since CMA memory
is not be used for blockdev page cache. Blockdev page cache is one of
the frequent failure points in my experience.

I'm not sure that ZONE_CMA works better than your GFP_PAGE_CACHE
adaptation for your system. In ZONE_CMA, CMA memory is used for file
page cache or anonymous pages. If my assumption that anonymous pages
are easier to be migrated/reclaimed is correct, ZONE_CMA would work
better than your adaptation since there is less file page cache pages
in CMA memory.

Anyway, it also doesn't guarantee to succeed all the time. There is
different kind of problem that prevents CMA allocation success and we
need to solve it. I will try it after problems that this patchset try
to fix is solved.

Thanks.

Re: [PATCH v3 0/6] Introduce ZONE_CMA

[Joonsoo Kim]

On Thu, May 26, 2016 at 04:04:54PM +0800, Feng Tang wrote:
> On Thu, May 26, 2016 at 02:22:22PM +0800, js1...@gmail.com wrote:
> > From: Joonsoo Kim 
> 
> Hi Joonsoo,
> 
> Nice work!

Thanks!

> > FYI, there is another attempt [3] trying to solve this problem in lkml.
> > And, as far as I know, Qualcomm also has out-of-tree solution for this
> > problem.
> 
> This may be a little off-topic :) Actually, we have used another way in
> our products, that we disable the fallback from MIGRATETYE_MOVABLE to
> MIGRATETYPE_CMA completely, and only allow free CMA memory to be used
> by file page cache (which is easy to be reclaimed by its nature). 
> We did it by adding a GFP_PAGE_CACHE to every allocation request for
> page cache, and the MM will try to pick up an available free CMA page
> first, and goes to normal path when fail. 

Just wonder, why do you allow CMA memory to file page cache rather
than anonymous page? I guess that anonymous pages would be more easily
migrated/reclaimed than file page cache. In fact, some of our product
uses anonymous page adaptation to satisfy similar requirement by
introducing GFP_CMA. AFAIK, some of chip vendor also uses "anonymous
page first adaptation" to get better success rate.

> It works fine on our products, though we still see some cases that
> some page can't be reclaimed. 
> 
> Our product has a special user case of CMA, that sometimes it will
> need to use the whole CMA memory (say 256MB on a phone), then all

I don't think this usecase is so special. Our product also has similar
usecase. And, I already knows one another.

> share out CMA pages need to be reclaimed all at once. Don't know if
> this new ZONE_CMA approach could meet this request? (our page cache
> solution can't ganrantee to meet this request all the time).

This ZONE_CMA approach would be better than before, since CMA memory
is not be used for blockdev page cache. Blockdev page cache is one of
the frequent failure points in my experience.

I'm not sure that ZONE_CMA works better than your GFP_PAGE_CACHE
adaptation for your system. In ZONE_CMA, CMA memory is used for file
page cache or anonymous pages. If my assumption that anonymous pages
are easier to be migrated/reclaimed is correct, ZONE_CMA would work
better than your adaptation since there is less file page cache pages
in CMA memory.

Anyway, it also doesn't guarantee to succeed all the time. There is
different kind of problem that prevents CMA allocation success and we
need to solve it. I will try it after problems that this patchset try
to fix is solved.

Thanks.

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Thu, May 26, 2016 at 1:33 PM, Michal Marek  wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild.git kbuild

This pull results in new warnings.

I get new "may be uninitialized" warnings now for me allmodconfig
build, and while I didn't look at them all, the one I looked at was
just entirely crap:

   fs/gfs2/dir.c: In function ‘dir_split_leaf.isra.16’:
   fs/gfs2/dir.c:1021:8: warning: ‘leaf_no’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
 error = get_leaf(dip, leaf_no, );
   ^

yeah no, leaf_no is initialized a few lines up by

error = get_leaf_nr(dip, index, _no);

and the fact that gcc can't follow the trivial  error handling is not
our fault. It looks *so* trivial that I wonder why.

That said, I don't see exactly what in the pull request causes this.

My reading of the diff seems to say that you are actually adding
*more* cases of -Wno-maybe-uninitialized, not less.

So I suspect it's almost accidental in just how the Kconfig option
CC_OPTIMIZE_FOR_PERFORMANCE happened, which in turn probably just
changes the options for "make allmiodconfig", and it now picks a
non-size-optimized build that always showed those warnings and I just
didn't see them.

Annoying. I've pulled it, but I wish you would look at this.

  Linus

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Thu, May 26, 2016 at 1:33 PM, Michal Marek  wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild.git kbuild

This pull results in new warnings.

I get new "may be uninitialized" warnings now for me allmodconfig
build, and while I didn't look at them all, the one I looked at was
just entirely crap:

   fs/gfs2/dir.c: In function ‘dir_split_leaf.isra.16’:
   fs/gfs2/dir.c:1021:8: warning: ‘leaf_no’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
 error = get_leaf(dip, leaf_no, );
   ^

yeah no, leaf_no is initialized a few lines up by

error = get_leaf_nr(dip, index, _no);

and the fact that gcc can't follow the trivial  error handling is not
our fault. It looks *so* trivial that I wonder why.

That said, I don't see exactly what in the pull request causes this.

My reading of the diff seems to say that you are actually adding
*more* cases of -Wno-maybe-uninitialized, not less.

So I suspect it's almost accidental in just how the Kconfig option
CC_OPTIMIZE_FOR_PERFORMANCE happened, which in turn probably just
changes the options for "make allmiodconfig", and it now picks a
non-size-optimized build that always showed those warnings and I just
didn't see them.

Annoying. I've pulled it, but I wish you would look at this.

  Linus

Re: [PATCH] mm: check the return value of lookup_page_ext for all call sites

[Minchan Kim]

On Thu, May 26, 2016 at 04:15:28PM -0700, Shi, Yang wrote:
> On 5/25/2016 5:37 PM, Minchan Kim wrote:
> >On Tue, May 24, 2016 at 11:58:11AM +0900, Minchan Kim wrote:
> >>On Mon, May 23, 2016 at 10:16:08AM -0700, Yang Shi wrote:
> >>>Per the discussion with Joonsoo Kim [1], we need check the return value of
> >>>lookup_page_ext() for all call sites since it might return NULL in some 
> >>>cases,
> >>>although it is unlikely, i.e. memory hotplug.
> >>>
> >>>Tested with ltp with "page_owner=0".
> >>>
> >>>[1] http://lkml.kernel.org/r/20160519002809.GA10245@js1304-P5Q-DELUXE
> >>>
> >>>Signed-off-by: Yang Shi 
> >>
> >>I didn't read code code in detail to see how page_ext memory space
> >>allocated in boot code and memory hotplug but to me, it's not good
> >>to check NULL whenever we calls lookup_page_ext.
> >>
> >>More dangerous thing is now page_ext is used by optionable feature(ie, not
> >>critical for system stability) but if we want to use page_ext as
> >>another important tool for the system in future,
> >>it could be a serious problem.
> >>
> >>Can we put some hooks of page_ext into memory-hotplug so guarantee
> >>that page_ext memory space is allocated with memmap space at the
> >>same time? IOW, once every PFN wakers find a page is valid, page_ext
> >>is valid, too so lookup_page_ext never returns NULL on valid page
> >>by design.
> >>
> >>I hope we consider this direction, too.
> >
> >Yang, Could you think about this?
> 
> Thanks a lot for the suggestion. Sorry for the late reply, I was
> busy on preparing patches. I do agree this is a direction we should
> look into, but I haven't got time to think about it deeper. I hope
> Joonsoo could chime in too since he is the original author for page
> extension.
> 
> >
> >Even, your patch was broken, I think.
> >It doesn't work with !CONFIG_DEBUG_VM && !CONFIG_PAGE_POISONING because
> >lookup_page_ext doesn't return NULL in that case.
> 
> Actually, I think the #ifdef should be removed if lookup_page_ext()
> is possible to return NULL. It sounds not make sense returning NULL
> only when DEBUG_VM is enabled. It should return NULL no matter what
> debug config is selected. If Joonsoo agrees with me I'm going to
> come up with a patch to fix it.

I don't know what lock protects race section->page_ext storing/tearing
during memory hotplug while random thread accesses pege_ext,
for example, kpageflags->page_is_idle.

Please consider that, too if you want to go with this approach.

> 
> Regards,
> Yang
> 
> >
> >>
> >>Thanks.
> >>
> >>>---
> >>> include/linux/page_idle.h | 43 ---
> >>> mm/page_alloc.c   |  6 ++
> >>> mm/page_owner.c   | 27 +++
> >>> mm/page_poison.c  |  8 +++-
> >>> mm/vmstat.c   |  2 ++
> >>> 5 files changed, 78 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h
> >>>index bf268fa..8f5d4ad 100644
> >>>--- a/include/linux/page_idle.h
> >>>+++ b/include/linux/page_idle.h
> >>>@@ -46,33 +46,62 @@ extern struct page_ext_operations page_idle_ops;
> >>>
> >>> static inline bool page_is_young(struct page *page)
> >>> {
> >>>-  return test_bit(PAGE_EXT_YOUNG, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline void set_page_young(struct page *page)
> >>> {
> >>>-  set_bit(PAGE_EXT_YOUNG, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return;
> >>>+
> >>>+  set_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline bool test_and_clear_page_young(struct page *page)
> >>> {
> >>>-  return test_and_clear_bit(PAGE_EXT_YOUNG,
> >>>-_page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_and_clear_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline bool page_is_idle(struct page *page)
> >>> {
> >>>-  return test_bit(PAGE_EXT_IDLE, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_bit(PAGE_EXT_IDLE, _ext->flags);
> >>> }
> >>>
> >>> static inline void set_page_idle(struct page *page)
> >>> {
> >>>-  set_bit(PAGE_EXT_IDLE, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return;
> >>>+
> >>>+  set_bit(PAGE_EXT_IDLE, _ext->flags);
> >>> }
> >>>
> >>> static inline void clear_page_idle(struct page *page)
> >>> {
> >>>-  clear_bit(PAGE_EXT_IDLE, 

Re: [PATCH] mm: check the return value of lookup_page_ext for all call sites

[Minchan Kim]

On Thu, May 26, 2016 at 04:15:28PM -0700, Shi, Yang wrote:
> On 5/25/2016 5:37 PM, Minchan Kim wrote:
> >On Tue, May 24, 2016 at 11:58:11AM +0900, Minchan Kim wrote:
> >>On Mon, May 23, 2016 at 10:16:08AM -0700, Yang Shi wrote:
> >>>Per the discussion with Joonsoo Kim [1], we need check the return value of
> >>>lookup_page_ext() for all call sites since it might return NULL in some 
> >>>cases,
> >>>although it is unlikely, i.e. memory hotplug.
> >>>
> >>>Tested with ltp with "page_owner=0".
> >>>
> >>>[1] http://lkml.kernel.org/r/20160519002809.GA10245@js1304-P5Q-DELUXE
> >>>
> >>>Signed-off-by: Yang Shi 
> >>
> >>I didn't read code code in detail to see how page_ext memory space
> >>allocated in boot code and memory hotplug but to me, it's not good
> >>to check NULL whenever we calls lookup_page_ext.
> >>
> >>More dangerous thing is now page_ext is used by optionable feature(ie, not
> >>critical for system stability) but if we want to use page_ext as
> >>another important tool for the system in future,
> >>it could be a serious problem.
> >>
> >>Can we put some hooks of page_ext into memory-hotplug so guarantee
> >>that page_ext memory space is allocated with memmap space at the
> >>same time? IOW, once every PFN wakers find a page is valid, page_ext
> >>is valid, too so lookup_page_ext never returns NULL on valid page
> >>by design.
> >>
> >>I hope we consider this direction, too.
> >
> >Yang, Could you think about this?
> 
> Thanks a lot for the suggestion. Sorry for the late reply, I was
> busy on preparing patches. I do agree this is a direction we should
> look into, but I haven't got time to think about it deeper. I hope
> Joonsoo could chime in too since he is the original author for page
> extension.
> 
> >
> >Even, your patch was broken, I think.
> >It doesn't work with !CONFIG_DEBUG_VM && !CONFIG_PAGE_POISONING because
> >lookup_page_ext doesn't return NULL in that case.
> 
> Actually, I think the #ifdef should be removed if lookup_page_ext()
> is possible to return NULL. It sounds not make sense returning NULL
> only when DEBUG_VM is enabled. It should return NULL no matter what
> debug config is selected. If Joonsoo agrees with me I'm going to
> come up with a patch to fix it.

I don't know what lock protects race section->page_ext storing/tearing
during memory hotplug while random thread accesses pege_ext,
for example, kpageflags->page_is_idle.

Please consider that, too if you want to go with this approach.

> 
> Regards,
> Yang
> 
> >
> >>
> >>Thanks.
> >>
> >>>---
> >>> include/linux/page_idle.h | 43 ---
> >>> mm/page_alloc.c   |  6 ++
> >>> mm/page_owner.c   | 27 +++
> >>> mm/page_poison.c  |  8 +++-
> >>> mm/vmstat.c   |  2 ++
> >>> 5 files changed, 78 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h
> >>>index bf268fa..8f5d4ad 100644
> >>>--- a/include/linux/page_idle.h
> >>>+++ b/include/linux/page_idle.h
> >>>@@ -46,33 +46,62 @@ extern struct page_ext_operations page_idle_ops;
> >>>
> >>> static inline bool page_is_young(struct page *page)
> >>> {
> >>>-  return test_bit(PAGE_EXT_YOUNG, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline void set_page_young(struct page *page)
> >>> {
> >>>-  set_bit(PAGE_EXT_YOUNG, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return;
> >>>+
> >>>+  set_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline bool test_and_clear_page_young(struct page *page)
> >>> {
> >>>-  return test_and_clear_bit(PAGE_EXT_YOUNG,
> >>>-_page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_and_clear_bit(PAGE_EXT_YOUNG, _ext->flags);
> >>> }
> >>>
> >>> static inline bool page_is_idle(struct page *page)
> >>> {
> >>>-  return test_bit(PAGE_EXT_IDLE, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return false;
> >>>+
> >>>+  return test_bit(PAGE_EXT_IDLE, _ext->flags);
> >>> }
> >>>
> >>> static inline void set_page_idle(struct page *page)
> >>> {
> >>>-  set_bit(PAGE_EXT_IDLE, _page_ext(page)->flags);
> >>>+  struct page_ext *page_ext;
> >>>+  page_ext = lookup_page_ext(page);
> >>>+  if (unlikely(!page_ext)
> >>>+  return;
> >>>+
> >>>+  set_bit(PAGE_EXT_IDLE, _ext->flags);
> >>> }
> >>>
> >>> static inline void clear_page_idle(struct page *page)
> >>> {
> >>>-  clear_bit(PAGE_EXT_IDLE, _page_ext(page)->flags);
> 

RE: [PATCH v4] extcon: gpio: Add the support for Device tree bindings

[Venkat Reddy Talla]

Hi Choi,

Sorry for changing author, will update author field with your name.

Regarding Rob Herring comments, You had already replied.

I felt separate compatible for each external connector is not required,
as client driver can detect the type of external cable(sdp,dcp, microphone) on 
receiving notification from extcon provider,
I have also added more description for wakeup-source.

Do you see any other changes required on top of v4 patch?

Regards,
Venkat

> -Original Message-
> From: Chanwoo Choi [mailto:cwcho...@gmail.com]
> Sent: Thursday, May 26, 2016 6:52 PM
> To: Venkat Reddy Talla
> Cc: MyungJoo Ham; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
> devicetree; Kumar Gala; linux-kernel; Laxman Dewangan
> Subject: Re: [PATCH v4] extcon: gpio: Add the support for Device tree
> bindings
> 
> Hi Venkat,
> 
> There is some miscommunication. On previous my reply, I don't mean that
> the author of the patch[1] is changed from me to you.
> 
> I'd like you to remain the original author(me) for the patch[1] without
> changing the author.
> [1] https://lkml.org/lkml/2015/10/21/8
> - [PATCH v3] extcon: gpio: Add the support for Device tree bindings
> 
> You can use the patch[1] as based patch and you can add new feature on
> base patch[1]. Also, If you ok, you can modify the extccon-gpio.c driver as
> Rob comment[2].
> 
> But, Rob Herring gave me the some comment[2].
> [2] https://lkml.org/lkml/2015/10/21/906
> 
> 
> Thanks,
> Chanwoo Choi
> 
> 
> On Thu, May 26, 2016 at 8:47 PM, Venkat Reddy Talla
>  wrote:
> > Add the support for Device tree bindings of extcon-gpio driver.
> > The extcon-gpio device tree node must include the both 'extcon-id' and
> > 'gpios' property.
> >
> > For example:
> > usb_cable: extcon-gpio-0 {
> > compatible = "extcon-gpio";
> > extcon-id = ;
> > gpios = < 1 GPIO_ACTIVE_HIGH>;
> > }
> > ta_cable: extcon-gpio-1 {
> > compatible = "extcon-gpio";
> > extcon-id = ;
> > gpios = < 2 GPIO_ACTIVE_LOW>;
> > debounce-ms = <50>; /* 50 millisecond */
> > wakeup-source;
> > }
> > _usb {
> > extcon = <_cable>;
> > };
> >  {
> > extcon = <_cable>;
> > };
> >
> > Signed-off-by: Venkat Reddy Talla 
> > Signed-off-by: Chanwoo Choi 
> > Signed-off-by: MyungJoo Ham 
> >
> > ---
> > changes from v3:
> > - add description for wakeup-source in documentation
> > - change dts property extcon-gpio name to gpios
> > - use of_get_named_gpio_flags to get gpio number and flags Changes
> > from v2:
> > - Add the more description for 'extcon-id' property in documentation
> > Changes from v1:
> > - Create the include/dt-bindings/extcon/extcon.h including the
> > identification of external connector. These definitions are used in dts 
> > file.
> > - Fix error if CONFIG_OF is disabled.
> > - Add signed-off tag by Myungjoo Ham
> > ---
> > ---
> >  .../devicetree/bindings/extcon/extcon-gpio.txt |  48 +
> >  drivers/extcon/extcon-gpio.c   | 109 
> > +
> >  include/dt-bindings/extcon/extcon.h|  47 +
> >  include/linux/extcon/extcon-gpio.h |   8 +-
> >  4 files changed, 189 insertions(+), 23 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> >  create mode 100644 include/dt-bindings/extcon/extcon.h
> >
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > new file mode 100644
> > index 000..81f7932
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > @@ -0,0 +1,48 @@
> > +GPIO Extcon device
> > +
> > +This is a virtual device used to generate the specific cable states
> > +from the GPIO pin.
> > +
> > +Required properties:
> > +- compatible: Must be "extcon-gpio".
> > +- extcon-id: The extcon support the various type of external
> > +connector to check
> > +  whether connector is attached or detached. The each external
> > +connector has
> > +  the unique number to identify it. So this property includes the
> > +unique number
> > +  which indicates the specific external connector. When external
> > +connector is
> > +  attached or detached, GPIO pin detect the changed state. See
> > +include/
> > +  dt-bindings/extcon/extcon.h which defines the unique number for
> > +supported
> > +  external connector from extcon framework.
> > +- gpios: GPIO pin to detect the external connector. See gpio binding.
> > +
> > +Optional properties:
> > +- debounce-ms: the debounce dealy for GPIO pin in millisecond.
> > +- wakeup-source: Boolean, extcon can wake-up the system from
> suspend.
> > +  if gpio provided in extcon-gpio DT node is 

RE: [PATCH v4] extcon: gpio: Add the support for Device tree bindings

[Venkat Reddy Talla]

Hi Choi,

Sorry for changing author, will update author field with your name.

Regarding Rob Herring comments, You had already replied.

I felt separate compatible for each external connector is not required,
as client driver can detect the type of external cable(sdp,dcp, microphone) on 
receiving notification from extcon provider,
I have also added more description for wakeup-source.

Do you see any other changes required on top of v4 patch?

Regards,
Venkat

> -Original Message-
> From: Chanwoo Choi [mailto:cwcho...@gmail.com]
> Sent: Thursday, May 26, 2016 6:52 PM
> To: Venkat Reddy Talla
> Cc: MyungJoo Ham; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
> devicetree; Kumar Gala; linux-kernel; Laxman Dewangan
> Subject: Re: [PATCH v4] extcon: gpio: Add the support for Device tree
> bindings
> 
> Hi Venkat,
> 
> There is some miscommunication. On previous my reply, I don't mean that
> the author of the patch[1] is changed from me to you.
> 
> I'd like you to remain the original author(me) for the patch[1] without
> changing the author.
> [1] https://lkml.org/lkml/2015/10/21/8
> - [PATCH v3] extcon: gpio: Add the support for Device tree bindings
> 
> You can use the patch[1] as based patch and you can add new feature on
> base patch[1]. Also, If you ok, you can modify the extccon-gpio.c driver as
> Rob comment[2].
> 
> But, Rob Herring gave me the some comment[2].
> [2] https://lkml.org/lkml/2015/10/21/906
> 
> 
> Thanks,
> Chanwoo Choi
> 
> 
> On Thu, May 26, 2016 at 8:47 PM, Venkat Reddy Talla
>  wrote:
> > Add the support for Device tree bindings of extcon-gpio driver.
> > The extcon-gpio device tree node must include the both 'extcon-id' and
> > 'gpios' property.
> >
> > For example:
> > usb_cable: extcon-gpio-0 {
> > compatible = "extcon-gpio";
> > extcon-id = ;
> > gpios = < 1 GPIO_ACTIVE_HIGH>;
> > }
> > ta_cable: extcon-gpio-1 {
> > compatible = "extcon-gpio";
> > extcon-id = ;
> > gpios = < 2 GPIO_ACTIVE_LOW>;
> > debounce-ms = <50>; /* 50 millisecond */
> > wakeup-source;
> > }
> > _usb {
> > extcon = <_cable>;
> > };
> >  {
> > extcon = <_cable>;
> > };
> >
> > Signed-off-by: Venkat Reddy Talla 
> > Signed-off-by: Chanwoo Choi 
> > Signed-off-by: MyungJoo Ham 
> >
> > ---
> > changes from v3:
> > - add description for wakeup-source in documentation
> > - change dts property extcon-gpio name to gpios
> > - use of_get_named_gpio_flags to get gpio number and flags Changes
> > from v2:
> > - Add the more description for 'extcon-id' property in documentation
> > Changes from v1:
> > - Create the include/dt-bindings/extcon/extcon.h including the
> > identification of external connector. These definitions are used in dts 
> > file.
> > - Fix error if CONFIG_OF is disabled.
> > - Add signed-off tag by Myungjoo Ham
> > ---
> > ---
> >  .../devicetree/bindings/extcon/extcon-gpio.txt |  48 +
> >  drivers/extcon/extcon-gpio.c   | 109 
> > +
> >  include/dt-bindings/extcon/extcon.h|  47 +
> >  include/linux/extcon/extcon-gpio.h |   8 +-
> >  4 files changed, 189 insertions(+), 23 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> >  create mode 100644 include/dt-bindings/extcon/extcon.h
> >
> > diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > new file mode 100644
> > index 000..81f7932
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> > @@ -0,0 +1,48 @@
> > +GPIO Extcon device
> > +
> > +This is a virtual device used to generate the specific cable states
> > +from the GPIO pin.
> > +
> > +Required properties:
> > +- compatible: Must be "extcon-gpio".
> > +- extcon-id: The extcon support the various type of external
> > +connector to check
> > +  whether connector is attached or detached. The each external
> > +connector has
> > +  the unique number to identify it. So this property includes the
> > +unique number
> > +  which indicates the specific external connector. When external
> > +connector is
> > +  attached or detached, GPIO pin detect the changed state. See
> > +include/
> > +  dt-bindings/extcon/extcon.h which defines the unique number for
> > +supported
> > +  external connector from extcon framework.
> > +- gpios: GPIO pin to detect the external connector. See gpio binding.
> > +
> > +Optional properties:
> > +- debounce-ms: the debounce dealy for GPIO pin in millisecond.
> > +- wakeup-source: Boolean, extcon can wake-up the system from
> suspend.
> > +  if gpio provided in extcon-gpio DT node is registered as interrupt,
> > +  then extcon can wake-up the system from suspend if wakeup-source
> > 

Re: [PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Michal Suchanek]

On 27 May 2016 at 05:41, Peter Chen  wrote:
> On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
>> Hello,
>>
>> I was updating my config by make oldconfig for a while and noticed that my 
>> USB
>> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
>> over time.
>>
>> Looking through defconfigs some have it included and some which seem in need 
>> of
>> it don't.
>>
>> Since the dependency is not obvious I think it would be better to select it
>> where possible.
>
> From Documentation/kbuild/kconfig-language.txt
> In general use select only for non-visible symbols
> (no prompts anywhere) and for symbols with no dependencies.
>
> But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
> NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
> dependency problem in future, so unless it is necessary, use it
> as least as possible.
>
> If you are using new code, and it adds new dependency code, it is
> reasonable you may need to update your defconfig.

If the driver gets split into multiple parts that need to be
configured separately that's reasonable.

If the newly required option is some obscure feature internal to the
Linux implementation like NOP_USB_XCEIV it's not reasonable in my
book.

Thanks

Michal

Re: [PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Michal Suchanek]

On 27 May 2016 at 05:41, Peter Chen  wrote:
> On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
>> Hello,
>>
>> I was updating my config by make oldconfig for a while and noticed that my 
>> USB
>> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
>> over time.
>>
>> Looking through defconfigs some have it included and some which seem in need 
>> of
>> it don't.
>>
>> Since the dependency is not obvious I think it would be better to select it
>> where possible.
>
> From Documentation/kbuild/kconfig-language.txt
> In general use select only for non-visible symbols
> (no prompts anywhere) and for symbols with no dependencies.
>
> But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
> NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
> dependency problem in future, so unless it is necessary, use it
> as least as possible.
>
> If you are using new code, and it adds new dependency code, it is
> reasonable you may need to update your defconfig.

If the driver gets split into multiple parts that need to be
configured separately that's reasonable.

If the newly required option is some obscure feature internal to the
Linux implementation like NOP_USB_XCEIV it's not reasonable in my
book.

Thanks

Michal

Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

[Julian Calaby]

Hi Michal,

On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek  wrote:
> On 27 May 2016 at 04:05, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek  wrote:
>>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>>> actual time the transfer is supposed to take and multiply by 2 for good
>>> measure.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>
>>> v2:
>>> - fix build error
>>> - use unsigned instead of int in max_t
>>> - use tfr->speed_hz instead of sspi->max_speed_hz
>>> ---
>>>  drivers/spi/spi-sun4i.c | 11 ++-
>>>  drivers/spi/spi-sun6i.c | 11 ++-
>>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>> index 1ddd9e2..fe63bbd 100644
>>> --- a/drivers/spi/spi-sun4i.c
>>> +++ b/drivers/spi/spi-sun4i.c
>>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master 
>>> *master,
>>>  {
>>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>>> unsigned int mclk_rate, div, timeout;
>>> +   unsigned int start, end, tx_time;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> u32 reg;
>>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master 
>>> *master,
>>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>>
>>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> You should probably use "unsigned int" instead of just "unsigned" here.
>>
>>> +   100);
>
> Or just 100U constant and avoid max_t altogether.
>
>>> +   start = jiffies;
>>> timeout = wait_for_completion_timeout(>done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> +   end = jiffies;
>>> if (!timeout) {
>>> +   dev_warn(>dev,
>>> +"%s: timeout transferring %u bytes@%iHz for 
>>> %i(%i)ms",
>>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>>> +jiffies_to_msecs(end - start), tx_time);
>>
>> Should the debug changes be in a separate patch?
>
> Is this so big of a change that it needs to be split?

Some people get touchy about changes not being mentioned in the commit
log. Technically this is two changes: fixing the timeout and adding
the timeout debugging, however they're related, so maybe just mention
that you've added this debugging in the commit log.

>
>>
>>> ret = -ETIMEDOUT;
>>> goto out;
>>> }
>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>> index 42e2c4b..8be5c5c 100644
>>> --- a/drivers/spi/spi-sun6i.c
>>> +++ b/drivers/spi/spi-sun6i.c
>>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master 
>>> *master,
>>> unsigned int mclk_rate, div, timeout;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> +   unsigned int start, end, tx_time;
>>> u32 reg;
>>>
>>> /* We don't support transfer larger than the FIFO */
>>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master 
>>> *master,
>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>>
>>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> Ditto, "unsigned int" instead of "unsigned"?
>>
>>> +   100);
>>> +   start = jiffies;
>>> timeout = wait_for_completion_timeout(>done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> +   end = jiffies;
>>> if (!timeout) {
>>> +   dev_warn(>dev,
>>> +"%s: timeout transferring %u bytes@%iHz for 
>>> %i(%i)ms",
>>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>>> +jiffies_to_msecs(end - start), tx_time);
>>
>> Ditto, separate patch?
>>
>> Also, should the changes for the drivers be in two separate patches also?
>
> That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

[Julian Calaby]

Hi Michal,

On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek  wrote:
> On 27 May 2016 at 04:05, Julian Calaby  wrote:
>> Hi Michal,
>>
>> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek  wrote:
>>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>>> actual time the transfer is supposed to take and multiply by 2 for good
>>> measure.
>>>
>>> Signed-off-by: Michal Suchanek 
>>> ---
>>>
>>> v2:
>>> - fix build error
>>> - use unsigned instead of int in max_t
>>> - use tfr->speed_hz instead of sspi->max_speed_hz
>>> ---
>>>  drivers/spi/spi-sun4i.c | 11 ++-
>>>  drivers/spi/spi-sun6i.c | 11 ++-
>>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>> index 1ddd9e2..fe63bbd 100644
>>> --- a/drivers/spi/spi-sun4i.c
>>> +++ b/drivers/spi/spi-sun4i.c
>>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master 
>>> *master,
>>>  {
>>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>>> unsigned int mclk_rate, div, timeout;
>>> +   unsigned int start, end, tx_time;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> u32 reg;
>>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master 
>>> *master,
>>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>>
>>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> You should probably use "unsigned int" instead of just "unsigned" here.
>>
>>> +   100);
>
> Or just 100U constant and avoid max_t altogether.
>
>>> +   start = jiffies;
>>> timeout = wait_for_completion_timeout(>done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> +   end = jiffies;
>>> if (!timeout) {
>>> +   dev_warn(>dev,
>>> +"%s: timeout transferring %u bytes@%iHz for 
>>> %i(%i)ms",
>>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>>> +jiffies_to_msecs(end - start), tx_time);
>>
>> Should the debug changes be in a separate patch?
>
> Is this so big of a change that it needs to be split?

Some people get touchy about changes not being mentioned in the commit
log. Technically this is two changes: fixing the timeout and adding
the timeout debugging, however they're related, so maybe just mention
that you've added this debugging in the commit log.

>
>>
>>> ret = -ETIMEDOUT;
>>> goto out;
>>> }
>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>> index 42e2c4b..8be5c5c 100644
>>> --- a/drivers/spi/spi-sun6i.c
>>> +++ b/drivers/spi/spi-sun6i.c
>>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master 
>>> *master,
>>> unsigned int mclk_rate, div, timeout;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> +   unsigned int start, end, tx_time;
>>> u32 reg;
>>>
>>> /* We don't support transfer larger than the FIFO */
>>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master 
>>> *master,
>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>>
>>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> Ditto, "unsigned int" instead of "unsigned"?
>>
>>> +   100);
>>> +   start = jiffies;
>>> timeout = wait_for_completion_timeout(>done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> +   end = jiffies;
>>> if (!timeout) {
>>> +   dev_warn(>dev,
>>> +"%s: timeout transferring %u bytes@%iHz for 
>>> %i(%i)ms",
>>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>>> +jiffies_to_msecs(end - start), tx_time);
>>
>> Ditto, separate patch?
>>
>> Also, should the changes for the drivers be in two separate patches also?
>
> That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

[Michal Suchanek]

On 27 May 2016 at 04:05, Julian Calaby  wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek  wrote:
>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>> actual time the transfer is supposed to take and multiply by 2 for good
>> measure.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>
>> v2:
>> - fix build error
>> - use unsigned instead of int in max_t
>> - use tfr->speed_hz instead of sspi->max_speed_hz
>> ---
>>  drivers/spi/spi-sun4i.c | 11 ++-
>>  drivers/spi/spi-sun6i.c | 11 ++-
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 1ddd9e2..fe63bbd 100644
>> --- a/drivers/spi/spi-sun4i.c
>> +++ b/drivers/spi/spi-sun4i.c
>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master 
>> *master,
>>  {
>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>> unsigned int mclk_rate, div, timeout;
>> +   unsigned int start, end, tx_time;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> u32 reg;
>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master 
>> *master,
>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>
>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> You should probably use "unsigned int" instead of just "unsigned" here.
>
>> +   100);

Or just 100U constant and avoid max_t altogether.

>> +   start = jiffies;
>> timeout = wait_for_completion_timeout(>done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> +   end = jiffies;
>> if (!timeout) {
>> +   dev_warn(>dev,
>> +"%s: timeout transferring %u bytes@%iHz for 
>> %i(%i)ms",
>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>> +jiffies_to_msecs(end - start), tx_time);
>
> Should the debug changes be in a separate patch?

Is this so big of a change that it needs to be split?

>
>> ret = -ETIMEDOUT;
>> goto out;
>> }
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 42e2c4b..8be5c5c 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master 
>> *master,
>> unsigned int mclk_rate, div, timeout;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> +   unsigned int start, end, tx_time;
>> u32 reg;
>>
>> /* We don't support transfer larger than the FIFO */
>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master 
>> *master,
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>
>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> Ditto, "unsigned int" instead of "unsigned"?
>
>> +   100);
>> +   start = jiffies;
>> timeout = wait_for_completion_timeout(>done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> +   end = jiffies;
>> if (!timeout) {
>> +   dev_warn(>dev,
>> +"%s: timeout transferring %u bytes@%iHz for 
>> %i(%i)ms",
>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>> +jiffies_to_msecs(end - start), tx_time);
>
> Ditto, separate patch?
>
> Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Thanks

Michal

Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout

[Michal Suchanek]

On 27 May 2016 at 04:05, Julian Calaby  wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek  wrote:
>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>> actual time the transfer is supposed to take and multiply by 2 for good
>> measure.
>>
>> Signed-off-by: Michal Suchanek 
>> ---
>>
>> v2:
>> - fix build error
>> - use unsigned instead of int in max_t
>> - use tfr->speed_hz instead of sspi->max_speed_hz
>> ---
>>  drivers/spi/spi-sun4i.c | 11 ++-
>>  drivers/spi/spi-sun6i.c | 11 ++-
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 1ddd9e2..fe63bbd 100644
>> --- a/drivers/spi/spi-sun4i.c
>> +++ b/drivers/spi/spi-sun4i.c
>> @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master 
>> *master,
>>  {
>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>> unsigned int mclk_rate, div, timeout;
>> +   unsigned int start, end, tx_time;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> u32 reg;
>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master 
>> *master,
>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>
>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> You should probably use "unsigned int" instead of just "unsigned" here.
>
>> +   100);

Or just 100U constant and avoid max_t altogether.

>> +   start = jiffies;
>> timeout = wait_for_completion_timeout(>done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> +   end = jiffies;
>> if (!timeout) {
>> +   dev_warn(>dev,
>> +"%s: timeout transferring %u bytes@%iHz for 
>> %i(%i)ms",
>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>> +jiffies_to_msecs(end - start), tx_time);
>
> Should the debug changes be in a separate patch?

Is this so big of a change that it needs to be split?

>
>> ret = -ETIMEDOUT;
>> goto out;
>> }
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 42e2c4b..8be5c5c 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master 
>> *master,
>> unsigned int mclk_rate, div, timeout;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> +   unsigned int start, end, tx_time;
>> u32 reg;
>>
>> /* We don't support transfer larger than the FIFO */
>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master 
>> *master,
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>
>> +   tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> Ditto, "unsigned int" instead of "unsigned"?
>
>> +   100);
>> +   start = jiffies;
>> timeout = wait_for_completion_timeout(>done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> +   end = jiffies;
>> if (!timeout) {
>> +   dev_warn(>dev,
>> +"%s: timeout transferring %u bytes@%iHz for 
>> %i(%i)ms",
>> +dev_name(>dev), tfr->len, tfr->speed_hz,
>> +jiffies_to_msecs(end - start), tx_time);
>
> Ditto, separate patch?
>
> Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Thanks

Michal

Re: [PATCH 5/7] zram: use crypto api to check alg availability

[Minchan Kim]

Hello Sergey,

I want to know more how it works so below questions goes.

On Wed, May 25, 2016 at 11:30:04PM +0900, Sergey Senozhatsky wrote:
> There is no way to get a string with all the crypto comp
> algorithms supported by the crypto comp engine, so we need
> to maintain our own backends list. At the same time we
> additionally need to use crypto_has_comp() to make sure
> that the user has requested a compression algorithm that is
> recognized by the crypto comp engine. Relying on /proc/crypto
> is not an options here, because it does not show not-yet-inserted
> compression modules.
> 
> Example:
> 
>  modprobe zram
>  cat /proc/crypto | grep -i lz4
>  modprobe lz4
>  cat /proc/crypto | grep -i lz4
> name : lz4
> driver   : lz4-generic
> module   : lz4
> 
> So the user can't tell exactly if the lz4 is really supported
> from /proc/crypto output, unless someone or something has loaded
> it.
> 
> This patch also adds crypto_has_comp() to zcomp_available_show().

crypto_has_comp works regardless of that whether module is loading or not?
IOW, currently, if lz4 modules is not loading, but crypto_has_comp return
true about lz4 module.
Right?

> We store all the compression algorithms names in zcomp's `backends'
> array, regardless the CONFIG_CRYPTO_FOO configuration, but show

Then, you mean we should add new string into backend array whenever
adding new crypto compatible compression algorithm?

> only those that are also supported by crypto engine. This helps
> user to know the exact list of compression algorithms that can be
> used.
> 
> Example:
>   module lz4 is not loaded yet, but is supported by the crypto
>   engine. /proc/crypto has no information on this module, while
>   zram's `comp_algorithm' lists it:
> 
>  cat /proc/crypto | grep -i lz4
> 
>  cat /sys/block/zram0/comp_algorithm
> [lzo] lz4 deflate lz4hc 842

So, when lzo module is loading?

> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  drivers/block/zram/zcomp.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 79b30d7..a8593e9 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -29,12 +29,16 @@ static const char * const backends[] = {
>  static const char *find_backend(const char *compress)
>  {
>   int i = 0;
> +
>   while (backends[i]) {
>   if (sysfs_streq(compress, backends[i]))
>   break;
>   i++;
>   }
> - return backends[i];
> +
> + if (backends[i] && crypto_has_comp(backends[i], 0, 0))
> + return backends[i];
> + return NULL;
>  }
>  
>  static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> @@ -74,14 +78,16 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
>   ssize_t sz = 0;
>   int i = 0;
>  
> - while (backends[i]) {
> + for (; backends[i]; i++) {
> + if (!crypto_has_comp(backends[i], 0, 0))
> + continue;
> +
>   if (!strcmp(comp, backends[i]))
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>   "[%s] ", backends[i]);
>   else
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>   "%s ", backends[i]);
> - i++;
>   }
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
>   return sz;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [PATCH 5/7] zram: use crypto api to check alg availability

[Minchan Kim]

Hello Sergey,

I want to know more how it works so below questions goes.

On Wed, May 25, 2016 at 11:30:04PM +0900, Sergey Senozhatsky wrote:
> There is no way to get a string with all the crypto comp
> algorithms supported by the crypto comp engine, so we need
> to maintain our own backends list. At the same time we
> additionally need to use crypto_has_comp() to make sure
> that the user has requested a compression algorithm that is
> recognized by the crypto comp engine. Relying on /proc/crypto
> is not an options here, because it does not show not-yet-inserted
> compression modules.
> 
> Example:
> 
>  modprobe zram
>  cat /proc/crypto | grep -i lz4
>  modprobe lz4
>  cat /proc/crypto | grep -i lz4
> name : lz4
> driver   : lz4-generic
> module   : lz4
> 
> So the user can't tell exactly if the lz4 is really supported
> from /proc/crypto output, unless someone or something has loaded
> it.
> 
> This patch also adds crypto_has_comp() to zcomp_available_show().

crypto_has_comp works regardless of that whether module is loading or not?
IOW, currently, if lz4 modules is not loading, but crypto_has_comp return
true about lz4 module.
Right?

> We store all the compression algorithms names in zcomp's `backends'
> array, regardless the CONFIG_CRYPTO_FOO configuration, but show

Then, you mean we should add new string into backend array whenever
adding new crypto compatible compression algorithm?

> only those that are also supported by crypto engine. This helps
> user to know the exact list of compression algorithms that can be
> used.
> 
> Example:
>   module lz4 is not loaded yet, but is supported by the crypto
>   engine. /proc/crypto has no information on this module, while
>   zram's `comp_algorithm' lists it:
> 
>  cat /proc/crypto | grep -i lz4
> 
>  cat /sys/block/zram0/comp_algorithm
> [lzo] lz4 deflate lz4hc 842

So, when lzo module is loading?

> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  drivers/block/zram/zcomp.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 79b30d7..a8593e9 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -29,12 +29,16 @@ static const char * const backends[] = {
>  static const char *find_backend(const char *compress)
>  {
>   int i = 0;
> +
>   while (backends[i]) {
>   if (sysfs_streq(compress, backends[i]))
>   break;
>   i++;
>   }
> - return backends[i];
> +
> + if (backends[i] && crypto_has_comp(backends[i], 0, 0))
> + return backends[i];
> + return NULL;
>  }
>  
>  static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
> @@ -74,14 +78,16 @@ ssize_t zcomp_available_show(const char *comp, char *buf)
>   ssize_t sz = 0;
>   int i = 0;
>  
> - while (backends[i]) {
> + for (; backends[i]; i++) {
> + if (!crypto_has_comp(backends[i], 0, 0))
> + continue;
> +
>   if (!strcmp(comp, backends[i]))
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>   "[%s] ", backends[i]);
>   else
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz - 2,
>   "%s ", backends[i]);
> - i++;
>   }
>   sz += scnprintf(buf + sz, PAGE_SIZE - sz, "\n");
>   return sz;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [f2fs-dev] [PATCH 1/4] f2fs: propagate error given by f2fs_find_entry

[He YunLei]

On 2016/5/27 8:25, Jaegeuk Kim wrote:

If we get ENOMEM or EIO in f2fs_find_entry, we should stop right away.
Otherwise, for example, we can get duplicate directory entry by ->chash and
->clevel.

Signed-off-by: Jaegeuk Kim 
---
  fs/f2fs/dir.c| 23 ---
  fs/f2fs/inline.c |  4 +++-
  fs/f2fs/namei.c  |  5 +
  3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 24d1308..ae37543 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -185,8 +185,13 @@ static struct f2fs_dir_entry *find_in_level(struct inode 
*dir,
/* no need to allocate new dentry pages to all the indices */
dentry_page = find_data_page(dir, bidx);
if (IS_ERR(dentry_page)) {
-   room = true;
-   continue;
+   if (PTR_ERR(dentry_page) == -ENOENT) {
+   room = true;
+   continue;
+   } else {
+   *res_page = dentry_page;
+   break;
+   }
}

de = find_in_block(dentry_page, fname, namehash, _slots,
@@ -223,19 +228,22 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
struct fscrypt_name fname;
int err;

-   *res_page = NULL;
-
err = fscrypt_setup_filename(dir, child, 1, );
-   if (err)
+   if (err) {
+   *res_page = ERR_PTR(-ENOMEM);
return NULL;
+   }

if (f2fs_has_inline_dentry(dir)) {
+   *res_page = NULL;
de = find_in_inline_dir(dir, , res_page);
goto out;
}

-   if (npages == 0)
+   if (npages == 0) {
+   *res_page = NULL;
goto out;
+   }

max_depth = F2FS_I(dir)->i_current_depth;
if (unlikely(max_depth > MAX_DIR_HASH_DEPTH)) {
@@ -247,8 +255,9 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
}

for (level = 0; level < max_depth; level++) {
+   *res_page = NULL;
de = find_in_level(dir, level, , res_page);
-   if (de)
+   if (de || IS_ERR(*res_page))
break;
}


Hi, kim
Here, we return NULL for the error of find_data_page, it means
the file looked up is not exist to vfs, but may be the file has already exist
behind the block read error. So maybe we 'd better reported the error to vfs.

Thanks.


  out:
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 77c9c24..1eb3043 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -286,8 +286,10 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode 
*dir,
f2fs_hash_t namehash;

ipage = get_node_page(sbi, dir->i_ino);
-   if (IS_ERR(ipage))
+   if (IS_ERR(ipage)) {
+   *res_page = ipage;
return NULL;
+   }

namehash = f2fs_dentry_hash();

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 496f4e3..3f6119e 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -232,6 +232,9 @@ static int __recover_dot_dentries(struct inode *dir, nid_t 
pino)
if (de) {
f2fs_dentry_kunmap(dir, page);
f2fs_put_page(page, 0);
+   } else if (IS_ERR(page)) {
+   err = PTR_ERR(page);
+   goto out;
} else {
err = __f2fs_add_link(dir, , NULL, dir->i_ino, S_IFDIR);
if (err)
@@ -242,6 +245,8 @@ static int __recover_dot_dentries(struct inode *dir, nid_t 
pino)
if (de) {
f2fs_dentry_kunmap(dir, page);
f2fs_put_page(page, 0);
+   } else if (IS_ERR(page)) {
+   err = PTR_ERR(page);
} else {
err = __f2fs_add_link(dir, , NULL, pino, S_IFDIR);
}

Re: [f2fs-dev] [PATCH 1/4] f2fs: propagate error given by f2fs_find_entry

[He YunLei]

On 2016/5/27 8:25, Jaegeuk Kim wrote:

If we get ENOMEM or EIO in f2fs_find_entry, we should stop right away.
Otherwise, for example, we can get duplicate directory entry by ->chash and
->clevel.

Signed-off-by: Jaegeuk Kim 
---
  fs/f2fs/dir.c| 23 ---
  fs/f2fs/inline.c |  4 +++-
  fs/f2fs/namei.c  |  5 +
  3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 24d1308..ae37543 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -185,8 +185,13 @@ static struct f2fs_dir_entry *find_in_level(struct inode 
*dir,
/* no need to allocate new dentry pages to all the indices */
dentry_page = find_data_page(dir, bidx);
if (IS_ERR(dentry_page)) {
-   room = true;
-   continue;
+   if (PTR_ERR(dentry_page) == -ENOENT) {
+   room = true;
+   continue;
+   } else {
+   *res_page = dentry_page;
+   break;
+   }
}

de = find_in_block(dentry_page, fname, namehash, _slots,
@@ -223,19 +228,22 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
struct fscrypt_name fname;
int err;

-   *res_page = NULL;
-
err = fscrypt_setup_filename(dir, child, 1, );
-   if (err)
+   if (err) {
+   *res_page = ERR_PTR(-ENOMEM);
return NULL;
+   }

if (f2fs_has_inline_dentry(dir)) {
+   *res_page = NULL;
de = find_in_inline_dir(dir, , res_page);
goto out;
}

-   if (npages == 0)
+   if (npages == 0) {
+   *res_page = NULL;
goto out;
+   }

max_depth = F2FS_I(dir)->i_current_depth;
if (unlikely(max_depth > MAX_DIR_HASH_DEPTH)) {
@@ -247,8 +255,9 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
}

for (level = 0; level < max_depth; level++) {
+   *res_page = NULL;
de = find_in_level(dir, level, , res_page);
-   if (de)
+   if (de || IS_ERR(*res_page))
break;
}


Hi, kim
Here, we return NULL for the error of find_data_page, it means
the file looked up is not exist to vfs, but may be the file has already exist
behind the block read error. So maybe we 'd better reported the error to vfs.

Thanks.


  out:
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 77c9c24..1eb3043 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -286,8 +286,10 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode 
*dir,
f2fs_hash_t namehash;

ipage = get_node_page(sbi, dir->i_ino);
-   if (IS_ERR(ipage))
+   if (IS_ERR(ipage)) {
+   *res_page = ipage;
return NULL;
+   }

namehash = f2fs_dentry_hash();

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 496f4e3..3f6119e 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -232,6 +232,9 @@ static int __recover_dot_dentries(struct inode *dir, nid_t 
pino)
if (de) {
f2fs_dentry_kunmap(dir, page);
f2fs_put_page(page, 0);
+   } else if (IS_ERR(page)) {
+   err = PTR_ERR(page);
+   goto out;
} else {
err = __f2fs_add_link(dir, , NULL, dir->i_ino, S_IFDIR);
if (err)
@@ -242,6 +245,8 @@ static int __recover_dot_dentries(struct inode *dir, nid_t 
pino)
if (de) {
f2fs_dentry_kunmap(dir, page);
f2fs_put_page(page, 0);
+   } else if (IS_ERR(page)) {
+   err = PTR_ERR(page);
} else {
err = __f2fs_add_link(dir, , NULL, pino, S_IFDIR);
}

Re: BLKZEROOUT not zeroing md dev on VMDK

[Sitsofe Wheeler]

On 27 May 2016 at 05:18, Darrick J. Wong  wrote:
>
> It's possible that the pvscsi device advertised WRITE SAME, but if the device
> sends back ILLEGAL REQUEST then the SCSI disk driver will set
> write_same_max_bytes=0.  Subsequent BLKZEROOUT attempts will then issue writes
> of zeroes to the drive.

Thanks for following up on this but that's not what happens on the md
device - you can go on to issue as many BLKZEROOUT requests as you
like but the md disk is never zeroed nor is an error returned.

I filed a bug at https://bugzilla.kernel.org/show_bug.cgi?id=118581
(see https://bugzilla.kernel.org/show_bug.cgi?id=118581#c6 for
alternative reproduction steps that use scsi_debug and can be reworked
to impact device mapper) and Shaohua Li noted that
blkdev_issue_write_same could return 0 even when the disk didn't
support write same (see
https://bugzilla.kernel.org/show_bug.cgi?id=118581#c8 ).

Shaohua went on to create a patch for this ("block: correctly fallback
for zeroout" - https://patchwork.kernel.org/patch/9137311/ ) which has
yet to be reviewed.

-- 
Sitsofe | http://sucs.org/~sits/

Re: BLKZEROOUT not zeroing md dev on VMDK

[Sitsofe Wheeler]

On 27 May 2016 at 05:18, Darrick J. Wong  wrote:
>
> It's possible that the pvscsi device advertised WRITE SAME, but if the device
> sends back ILLEGAL REQUEST then the SCSI disk driver will set
> write_same_max_bytes=0.  Subsequent BLKZEROOUT attempts will then issue writes
> of zeroes to the drive.

Thanks for following up on this but that's not what happens on the md
device - you can go on to issue as many BLKZEROOUT requests as you
like but the md disk is never zeroed nor is an error returned.

I filed a bug at https://bugzilla.kernel.org/show_bug.cgi?id=118581
(see https://bugzilla.kernel.org/show_bug.cgi?id=118581#c6 for
alternative reproduction steps that use scsi_debug and can be reworked
to impact device mapper) and Shaohua Li noted that
blkdev_issue_write_same could return 0 even when the disk didn't
support write same (see
https://bugzilla.kernel.org/show_bug.cgi?id=118581#c8 ).

Shaohua went on to create a patch for this ("block: correctly fallback
for zeroout" - https://patchwork.kernel.org/patch/9137311/ ) which has
yet to be reviewed.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Andy Lutomirski]

On Thu, May 26, 2016 at 7:41 PM, Kees Cook  wrote:
> On Thu, May 26, 2016 at 7:10 PM, Andy Lutomirski  wrote:
>> On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
>>> One problem with seccomp was that ptrace could be used to change a
>>> syscall after seccomp filtering had completed. This was a well documented
>>> limitation, and it was recommended to block ptrace when defining a filter
>>> to avoid this problem. This can be quite a limitation for containers or
>>> other places where ptrace is desired even under seccomp filters.
>>>
>>> Since seccomp filtering has been split into pre-trace and trace phases
>>> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
>>> after ptrace. This makes that change, and updates the test suite for
>>> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.
>>
>> I like fixing the hole, but I don't like this fix.
>>
>> The two-phase seccomp mechanism is messy.  I wrote it because it was a
>> huge speedup.  Since then, I've made a ton of changes to the way that
>> x86 syscalls work, and there are two relevant effects: the slow path
>> is quite fast, and the phase-1-only path isn't really a win any more.
>>
>> I suggest that we fix the by simplifying the code instead of making it
>> even more complicated.  Let's back out the two-phase mechanism (but
>> keep the ability for arch code to supply seccomp_data) and then just
>> reorder it so that seccomp happens after ptrace.  The result should be
>> considerably simpler.  (We'll still have to answer the question of
>> what happens when a SECCOMP_RET_TRACE event changes the syscall, but
>> maybe the answer is to just let it through -- after all,
>> SECCOMP_RET_TRACE might be a request by a tracer to do its own
>> internal filtering.)
>
> I'm really against this. I think seccomp needs to stay first,

Why?  What use case is improved with it going first?

> and I
> like the two-phase split because it gives us a lot of flexibility on
> other architectures.

I thought so too when I wrote it, and I even tried a bit to evangelize
it to other arch maintainers.  So far, it's used *only* in x86, and it
would IMO be a cleanup to stop using it in x86 now.  Given my
experience cleanup up the x86 syscall path, my current advice to other
arch maintainers would be to try hard to avoid having a context in
which syscall args are known but ptrace can't be invoked (as x86 had
before Linux 4.5).

> And we can't just let through RET_TRACE because
> we'll have exactly the same problem: a process can add a RET_TRACE
> filter for some syscall and then change it arbitrarily to escape the
> filtering. The non-trace returns of seccomp need to be check first and
> after ptrace manipulations. The patch seems like the best approach and
> it covers all the corners.

But RET_TRACE really is special.

Suppose you have a tracer and you use SECCOMP_RET_TRACE.  If the
tracer sees a syscall, approves, and calls PTRACE_CONT, then the
syscall will be allowed, whereas the effect of SECCOMP_RET_TRACE run
anew would be to either force -ENOSYS or to trap back to the tracer,
depending on whether there is a tracer.  Your patch has a
SECCOMP_RET_TRACE special case, whereas my approach wouldn't need a
special case.

I think your patch also has a minor hole: if you have
SECCOMP_RET_TRACE *and* a tracer that's catching syscalls directly
(PTRACE_SYSCALL), then the PTRACE_SYSCALL action can modify a syscall
after TRACE does its thing but before recheck, and can then redirect
to another RET_TRACE action that would otherwise be denied.  This is
minor because it could only happen if the tracer actively fights with
itself.

Finally, I think that the your approach would break an existing valid
use case.  Suppose I have a tracer that wants to intercept some
syscall sys_foo (using SECCOMP_RET_TRACE) and, when it sees a sys_foo
attempt, it will implement it by redirecting it so some other syscall
that wouldn't be allowed if called directly (i.e. it would return
SECCOMP_RET_KILL or similar).  Currently, it'll work.  With your
patch, it will kill the tracee.  I think the former behavior is
better.  On the flip side, if you write a program that uses
SECCOMP_RET_TRACE, you more or less have to trust the tracer to begin
with.

One more reason to prefer my approach: currently, if you strace a
process that gets killed by SECCOMP_RET_KILL, you can't tell what
killed it.  For example:

prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, {len = 1, filter =
0x7ffe7b2b7d30}) = 0
+++ killed by SIGSYS +++
Bad system call (core dumped)

With my approach, strace will have the IMO much more sensible behavior
of showing the fatal syscall entry before showing the "killed by
SIGSYS".

--Andy

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Andy Lutomirski]

On Thu, May 26, 2016 at 7:41 PM, Kees Cook  wrote:
> On Thu, May 26, 2016 at 7:10 PM, Andy Lutomirski  wrote:
>> On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
>>> One problem with seccomp was that ptrace could be used to change a
>>> syscall after seccomp filtering had completed. This was a well documented
>>> limitation, and it was recommended to block ptrace when defining a filter
>>> to avoid this problem. This can be quite a limitation for containers or
>>> other places where ptrace is desired even under seccomp filters.
>>>
>>> Since seccomp filtering has been split into pre-trace and trace phases
>>> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
>>> after ptrace. This makes that change, and updates the test suite for
>>> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.
>>
>> I like fixing the hole, but I don't like this fix.
>>
>> The two-phase seccomp mechanism is messy.  I wrote it because it was a
>> huge speedup.  Since then, I've made a ton of changes to the way that
>> x86 syscalls work, and there are two relevant effects: the slow path
>> is quite fast, and the phase-1-only path isn't really a win any more.
>>
>> I suggest that we fix the by simplifying the code instead of making it
>> even more complicated.  Let's back out the two-phase mechanism (but
>> keep the ability for arch code to supply seccomp_data) and then just
>> reorder it so that seccomp happens after ptrace.  The result should be
>> considerably simpler.  (We'll still have to answer the question of
>> what happens when a SECCOMP_RET_TRACE event changes the syscall, but
>> maybe the answer is to just let it through -- after all,
>> SECCOMP_RET_TRACE might be a request by a tracer to do its own
>> internal filtering.)
>
> I'm really against this. I think seccomp needs to stay first,

Why?  What use case is improved with it going first?

> and I
> like the two-phase split because it gives us a lot of flexibility on
> other architectures.

I thought so too when I wrote it, and I even tried a bit to evangelize
it to other arch maintainers.  So far, it's used *only* in x86, and it
would IMO be a cleanup to stop using it in x86 now.  Given my
experience cleanup up the x86 syscall path, my current advice to other
arch maintainers would be to try hard to avoid having a context in
which syscall args are known but ptrace can't be invoked (as x86 had
before Linux 4.5).

> And we can't just let through RET_TRACE because
> we'll have exactly the same problem: a process can add a RET_TRACE
> filter for some syscall and then change it arbitrarily to escape the
> filtering. The non-trace returns of seccomp need to be check first and
> after ptrace manipulations. The patch seems like the best approach and
> it covers all the corners.

But RET_TRACE really is special.

Suppose you have a tracer and you use SECCOMP_RET_TRACE.  If the
tracer sees a syscall, approves, and calls PTRACE_CONT, then the
syscall will be allowed, whereas the effect of SECCOMP_RET_TRACE run
anew would be to either force -ENOSYS or to trap back to the tracer,
depending on whether there is a tracer.  Your patch has a
SECCOMP_RET_TRACE special case, whereas my approach wouldn't need a
special case.

I think your patch also has a minor hole: if you have
SECCOMP_RET_TRACE *and* a tracer that's catching syscalls directly
(PTRACE_SYSCALL), then the PTRACE_SYSCALL action can modify a syscall
after TRACE does its thing but before recheck, and can then redirect
to another RET_TRACE action that would otherwise be denied.  This is
minor because it could only happen if the tracer actively fights with
itself.

Finally, I think that the your approach would break an existing valid
use case.  Suppose I have a tracer that wants to intercept some
syscall sys_foo (using SECCOMP_RET_TRACE) and, when it sees a sys_foo
attempt, it will implement it by redirecting it so some other syscall
that wouldn't be allowed if called directly (i.e. it would return
SECCOMP_RET_KILL or similar).  Currently, it'll work.  With your
patch, it will kill the tracee.  I think the former behavior is
better.  On the flip side, if you write a program that uses
SECCOMP_RET_TRACE, you more or less have to trust the tracer to begin
with.

One more reason to prefer my approach: currently, if you strace a
process that gets killed by SECCOMP_RET_KILL, you can't tell what
killed it.  For example:

prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, {len = 1, filter =
0x7ffe7b2b7d30}) = 0
+++ killed by SIGSYS +++
Bad system call (core dumped)

With my approach, strace will have the IMO much more sensible behavior
of showing the fatal syscall entry before showing the "killed by
SIGSYS".

--Andy

Re: [PATCH] usb: core: fix a double free in the usb driver

[gre...@linuxfoundation.org]

On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> There is a double free problem in the usb driver.

Which driver?

> This is caused by delayed deregister for scsi device.
> <*> at Insert USB Storage
> -  USB bus #1 register
>  usb_create_hcd (primary-kref==1)
>  * primary-bandwidth_mutex(alloc))
> usb_get_hcd(primary-kref==2)
> -  USB bus #2 register
> usb_create_hcd (second-kref==1)
>  * second-bandwidth_mutex==primary-bandwidth_mutex
>  usb_get_hcd(second-kref==2)
> -  scsi_device_get
> usb_get_hcd(second-kref==3)
> 
> <*> at remove USB Storage (Normal)
> -  scsi_device_put
> usb_put_hcd(second-kref==2)
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==1)
> usb_release_dev(second-kref==0)  -> hcd_release()
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
> *(primary-bandwidth_mutex free)
> 
> at remove USB Storage 
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==2)
> usb_release_dev(second-kref==1)
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
>  *(primary-bandwidth_mutex free)
> -  scsi_device_put
>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>  * at this, second->primary==0 therefore try to
> free the primary-bandwidth_mutex.(already freed)

The formatting for this is all confused, can you fix it up?

> 
> To fix this problem kfree(hcd->bandwidth_mutex);
> should be executed at only (hcd->primary_hcd==hcd).
> 
> Signed-off-by: Chunggeol Kim 

We need an email address at the end of this line, look at how the
commits in the kernel git history look like for examples.

> ---
> drivers/usb/core/hcd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..60077f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> 
> mutex_lock(_port_peer_mutex);
> - if (usb_hcd_is_primary_hcd(hcd)) {
> + if (hcd == hcd->primary_hcd) {

That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
check, what are you changing here?

> kfree(hcd->address0_mutex);
> kfree(hcd->bandwidth_mutex);
> }

Your patch itself is also corrupted, and can't be applied, can you also
resolve this and resend?

thanks,

greg k-h

Re: [PATCH] usb: core: fix a double free in the usb driver

[gre...@linuxfoundation.org]

On Fri, May 27, 2016 at 01:38:17AM +, Chung-Geol Kim wrote:
> There is a double free problem in the usb driver.

Which driver?

> This is caused by delayed deregister for scsi device.
> <*> at Insert USB Storage
> -  USB bus #1 register
>  usb_create_hcd (primary-kref==1)
>  * primary-bandwidth_mutex(alloc))
> usb_get_hcd(primary-kref==2)
> -  USB bus #2 register
> usb_create_hcd (second-kref==1)
>  * second-bandwidth_mutex==primary-bandwidth_mutex
>  usb_get_hcd(second-kref==2)
> -  scsi_device_get
> usb_get_hcd(second-kref==3)
> 
> <*> at remove USB Storage (Normal)
> -  scsi_device_put
> usb_put_hcd(second-kref==2)
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==1)
> usb_release_dev(second-kref==0)  -> hcd_release()
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
> *(primary-bandwidth_mutex free)
> 
> at remove USB Storage 
> -  USB bus #2 deregister
>  usb_release_dev(second-kref==2)
> usb_release_dev(second-kref==1)
> -  USB bus #1 deregister
>  usb_release_dev(primary-kref==1)
>  usb_release_dev(primary-kref==0) -> hcd_release()
>  *(primary-bandwidth_mutex free)
> -  scsi_device_put
>  usb_put_hcd(second-kref==0)  -> hcd_release(*)
>  * at this, second->primary==0 therefore try to
> free the primary-bandwidth_mutex.(already freed)

The formatting for this is all confused, can you fix it up?

> 
> To fix this problem kfree(hcd->bandwidth_mutex);
> should be executed at only (hcd->primary_hcd==hcd).
> 
> Signed-off-by: Chunggeol Kim 

We need an email address at the end of this line, look at how the
commits in the kernel git history look like for examples.

> ---
> drivers/usb/core/hcd.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..60077f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref)
> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
> 
> mutex_lock(_port_peer_mutex);
> - if (usb_hcd_is_primary_hcd(hcd)) {
> + if (hcd == hcd->primary_hcd) {

That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this
check, what are you changing here?

> kfree(hcd->address0_mutex);
> kfree(hcd->bandwidth_mutex);
> }

Your patch itself is also corrupted, and can't be applied, can you also
resolve this and resend?

thanks,

greg k-h

Re: [PATCH 4/7] zram: align zcomp interface to crypto comp API

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:03PM +0900, Sergey Senozhatsky wrote:
> A cosmetic change: use the same datatypes as crypto API does.
> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  drivers/block/zram/zcomp.c| 5 ++---
>  drivers/block/zram/zcomp.h| 5 ++---
>  drivers/block/zram/zram_drv.c | 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 6e16fa2..79b30d7 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -103,7 +103,7 @@ void zcomp_stream_put(struct zcomp *comp)
>  }
>  
>  int zcomp_compress(struct zcomp_strm *zstrm,
> - const unsigned char *src, unsigned int *dst_len)
> + const u8 *src, unsigned int *dst_len)

Nitpick:

The zcomp doesn't need to depend on implementation(i.e., crypto) so
zcomp_compress should pass void * for src and dst.

I'm not strong aginst changing u8 but your description makes me
think about that. 


>  {
>   /*
>* Our dst memory (zstrm->buffer) is always `2 * PAGE_SIZE' sized
> @@ -127,8 +127,7 @@ int zcomp_compress(struct zcomp_strm *zstrm,
>  }
>  
>  int zcomp_decompress(struct zcomp_strm *zstrm,
> - const unsigned char *src,
> - size_t src_len, unsigned char *dst)
> + const u8 *src, unsigned int src_len, u8 *dst)
>  {
>   unsigned int dst_len = PAGE_SIZE;
>  
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 3fb9dc4..dcc0951 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -54,11 +54,10 @@ struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
>  void zcomp_stream_put(struct zcomp *comp);
>  
>  int zcomp_compress(struct zcomp_strm *zstrm,
> - const unsigned char *src, unsigned int *dst_len);
> + const u8 *src, unsigned int *dst_len);
>  
>  int zcomp_decompress(struct zcomp_strm *zstrm,
> - const unsigned char *src,
> - size_t src_len, unsigned char *dst);
> + const u8 *src, unsigned int src_len, u8 *dst);
>  
>  bool zcomp_set_max_streams(struct zcomp *comp, int num_strm);
>  #endif /* _ZCOMP_H_ */
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 99c8be7..65d1403 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -563,7 +563,7 @@ static int zram_decompress_page(struct zram *zram, char 
> *mem, u32 index)
>   unsigned char *cmem;
>   struct zram_meta *meta = zram->meta;
>   unsigned long handle;
> - size_t size;
> + unsigned int size;
>  
>   bit_spin_lock(ZRAM_ACCESS, >table[index].value);
>   handle = meta->table[index].handle;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [PATCH 4/7] zram: align zcomp interface to crypto comp API

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:03PM +0900, Sergey Senozhatsky wrote:
> A cosmetic change: use the same datatypes as crypto API does.
> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 
> ---
>  drivers/block/zram/zcomp.c| 5 ++---
>  drivers/block/zram/zcomp.h| 5 ++---
>  drivers/block/zram/zram_drv.c | 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 6e16fa2..79b30d7 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -103,7 +103,7 @@ void zcomp_stream_put(struct zcomp *comp)
>  }
>  
>  int zcomp_compress(struct zcomp_strm *zstrm,
> - const unsigned char *src, unsigned int *dst_len)
> + const u8 *src, unsigned int *dst_len)

Nitpick:

The zcomp doesn't need to depend on implementation(i.e., crypto) so
zcomp_compress should pass void * for src and dst.

I'm not strong aginst changing u8 but your description makes me
think about that. 


>  {
>   /*
>* Our dst memory (zstrm->buffer) is always `2 * PAGE_SIZE' sized
> @@ -127,8 +127,7 @@ int zcomp_compress(struct zcomp_strm *zstrm,
>  }
>  
>  int zcomp_decompress(struct zcomp_strm *zstrm,
> - const unsigned char *src,
> - size_t src_len, unsigned char *dst)
> + const u8 *src, unsigned int src_len, u8 *dst)
>  {
>   unsigned int dst_len = PAGE_SIZE;
>  
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index 3fb9dc4..dcc0951 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -54,11 +54,10 @@ struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
>  void zcomp_stream_put(struct zcomp *comp);
>  
>  int zcomp_compress(struct zcomp_strm *zstrm,
> - const unsigned char *src, unsigned int *dst_len);
> + const u8 *src, unsigned int *dst_len);
>  
>  int zcomp_decompress(struct zcomp_strm *zstrm,
> - const unsigned char *src,
> - size_t src_len, unsigned char *dst);
> + const u8 *src, unsigned int src_len, u8 *dst);
>  
>  bool zcomp_set_max_streams(struct zcomp *comp, int num_strm);
>  #endif /* _ZCOMP_H_ */
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 99c8be7..65d1403 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -563,7 +563,7 @@ static int zram_decompress_page(struct zram *zram, char 
> *mem, u32 index)
>   unsigned char *cmem;
>   struct zram_meta *meta = zram->meta;
>   unsigned long handle;
> - size_t size;
> + unsigned int size;
>  
>   bit_spin_lock(ZRAM_ACCESS, >table[index].value);
>   handle = meta->table[index].handle;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [PATCH v2 1/2] dt: binding: Add Qualcomm WCNSS control binding

[Bjorn Andersson]

On Thu 26 May 20:58 PDT 2016, Rob Herring wrote:

> +dt list
> 
> On Thu, May 12, 2016 at 6:17 PM, Bjorn Andersson
>  wrote:
[..]
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt 
> > b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> > new file mode 100644
> > index ..5413098a1e1a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> > @@ -0,0 +1,115 @@
> > +Qualcomm WCNSS Binding
> > +
> > +This binding describes the Qualcomm WCNSS hardware. It consists of control
> > +block and a BT, WiFi and FM radio block, all useing SMD as command 
> > channels.
> 
> s/useing/using/
> 

Right.

> > +
> > +- compatible:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: must be: "qcom,wcnss",
> > +
> > +- qcom,smd-channel:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: standard SMD property specifying the SMD channel used 
> > for
> > +   communication with the WiFi firmware
> 
> Is the string value defined somewhere?
> 

It's never been anything else than WCNSS_CTRL so I guess I could
document that.

> > +
> > +- qcom,mmio:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: reference to a node specifying the wcnss "ccu" and "dxe"
> > +   register blocks. The node must be compatible with one of
> > +   the following:
> > +   "qcom,riva",
> > +   "qcom,pronto"
> > +
[..]
> > +
> > +- qcom,state:
> 
> This is kind of generic.
> 

We can rename it qcom,smem-state to make it less likely to collide in
the future. I believe this is the first binding referencing one of
those.

> > +   Usage: required
> > +   Value type: 
> > +   Definition: should reference the tx-enable and tx-rings-empty SMEM 
> > states
> > +
> > +- qcom,state-names:

And then qcom,smem-state-names here

> > +   Usage: required
> > +   Value type: 
> > +   Definition: must contain "tx-enable" and "tx-rings-empty"
> > +
> > += EXAMPLE
> > +The following example represents a SMD node, with one edge representing the
> > +"pronto" subsystem, with the wcnss device and its wcn3680 BT and WiFi 
> > blocks
> > +described; as found on the 8974 platform.
> 
> So somewhere in here I'd expect to see 8974 in a compatible string.
> 

The "Wireless CoNnectivity Sub-System" version is "pronto" and this is
the only thing that has a hardware dependency. So the only part that
makes this 8974 and not 8016 is the placing of those blocks in the
memory space (and the interrupts).

And the compatible qcom,iris would make this a 8064 node.

> > +
> > +smd {
> > +   compatible = "qcom,smd";
> > +
> > +   pronto {
> > +   interrupts = <0 142 1>;
> > +
> > +   qcom,ipc = < 8 17>;
> > +   qcom,smd-edge = <6>;
> > +
> > +   wcnss {
> > +   compatible = "qcom,wcnss";
> > +   qcom,smd-channels = "WCNSS_CTRL";
> > +
> > +   #address-cells = <1>;
> > +   #size-cells = <1>;
> > +
> > +   qcom,wcnss = <>;
> 
> Not documented and poorly named. The property name should reflect what
> this is for or points too.
> 

It's documented as qcom,mmio, so the example is wrong.

> > +
> > +   bt {
> > +   compatible = "qcom,wcnss-bt";
> > +   };
> > +
> > +   wlan {
> > +   compatible = "qcom,wcnss-wlan";
> > +
> > +   interrupts = <0 145 0>, <0 146 0>;
> > +   interrupt-names = "tx", "rx";
> > +
> > +   qcom,state = <_smsm 10>, <_smsm 
> > 9>;
> > +   qcom,state-names = "tx-enable", 
> > "tx-rings-empty";
> > +   };
> > +   };
> > +   };
> > +};
> > +
> > +soc {
> > +   pronto: pronto {
> 
> 2 pronto nodes? That's confusing...
> 

This one denotes the pronto register space, the one above denotes the
communication channel to the firmware running on the cpu in the wcnss.

But in SMD terms the one above is the "pronto edge", so I can change
that to pronto-edge.

> > +   compatible = "qcom,pronto";
> > +
> > +   reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 
> > 0x3000>;
> > +   reg-names = "ccu", "dxe", "pmu";
> > +   };
> > +};

Thanks for your review.

Regards,
Bjorn

Re: [PATCH v2 1/2] dt: binding: Add Qualcomm WCNSS control binding

[Bjorn Andersson]

On Thu 26 May 20:58 PDT 2016, Rob Herring wrote:

> +dt list
> 
> On Thu, May 12, 2016 at 6:17 PM, Bjorn Andersson
>  wrote:
[..]
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt 
> > b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> > new file mode 100644
> > index ..5413098a1e1a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> > @@ -0,0 +1,115 @@
> > +Qualcomm WCNSS Binding
> > +
> > +This binding describes the Qualcomm WCNSS hardware. It consists of control
> > +block and a BT, WiFi and FM radio block, all useing SMD as command 
> > channels.
> 
> s/useing/using/
> 

Right.

> > +
> > +- compatible:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: must be: "qcom,wcnss",
> > +
> > +- qcom,smd-channel:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: standard SMD property specifying the SMD channel used 
> > for
> > +   communication with the WiFi firmware
> 
> Is the string value defined somewhere?
> 

It's never been anything else than WCNSS_CTRL so I guess I could
document that.

> > +
> > +- qcom,mmio:
> > +   Usage: required
> > +   Value type: 
> > +   Definition: reference to a node specifying the wcnss "ccu" and "dxe"
> > +   register blocks. The node must be compatible with one of
> > +   the following:
> > +   "qcom,riva",
> > +   "qcom,pronto"
> > +
[..]
> > +
> > +- qcom,state:
> 
> This is kind of generic.
> 

We can rename it qcom,smem-state to make it less likely to collide in
the future. I believe this is the first binding referencing one of
those.

> > +   Usage: required
> > +   Value type: 
> > +   Definition: should reference the tx-enable and tx-rings-empty SMEM 
> > states
> > +
> > +- qcom,state-names:

And then qcom,smem-state-names here

> > +   Usage: required
> > +   Value type: 
> > +   Definition: must contain "tx-enable" and "tx-rings-empty"
> > +
> > += EXAMPLE
> > +The following example represents a SMD node, with one edge representing the
> > +"pronto" subsystem, with the wcnss device and its wcn3680 BT and WiFi 
> > blocks
> > +described; as found on the 8974 platform.
> 
> So somewhere in here I'd expect to see 8974 in a compatible string.
> 

The "Wireless CoNnectivity Sub-System" version is "pronto" and this is
the only thing that has a hardware dependency. So the only part that
makes this 8974 and not 8016 is the placing of those blocks in the
memory space (and the interrupts).

And the compatible qcom,iris would make this a 8064 node.

> > +
> > +smd {
> > +   compatible = "qcom,smd";
> > +
> > +   pronto {
> > +   interrupts = <0 142 1>;
> > +
> > +   qcom,ipc = < 8 17>;
> > +   qcom,smd-edge = <6>;
> > +
> > +   wcnss {
> > +   compatible = "qcom,wcnss";
> > +   qcom,smd-channels = "WCNSS_CTRL";
> > +
> > +   #address-cells = <1>;
> > +   #size-cells = <1>;
> > +
> > +   qcom,wcnss = <>;
> 
> Not documented and poorly named. The property name should reflect what
> this is for or points too.
> 

It's documented as qcom,mmio, so the example is wrong.

> > +
> > +   bt {
> > +   compatible = "qcom,wcnss-bt";
> > +   };
> > +
> > +   wlan {
> > +   compatible = "qcom,wcnss-wlan";
> > +
> > +   interrupts = <0 145 0>, <0 146 0>;
> > +   interrupt-names = "tx", "rx";
> > +
> > +   qcom,state = <_smsm 10>, <_smsm 
> > 9>;
> > +   qcom,state-names = "tx-enable", 
> > "tx-rings-empty";
> > +   };
> > +   };
> > +   };
> > +};
> > +
> > +soc {
> > +   pronto: pronto {
> 
> 2 pronto nodes? That's confusing...
> 

This one denotes the pronto register space, the one above denotes the
communication channel to the firmware running on the cpu in the wcnss.

But in SMD terms the one above is the "pronto edge", so I can change
that to pronto-edge.

> > +   compatible = "qcom,pronto";
> > +
> > +   reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 
> > 0x3000>;
> > +   reg-names = "ccu", "dxe", "pmu";
> > +   };
> > +};

Thanks for your review.

Regards,
Bjorn

Re: [PATCH 3/7] zram: drop zcomp param from compress/decompress

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:02PM +0900, Sergey Senozhatsky wrote:
> We don't need to supply a zcomp pointer to compress/decompress
> functions, drop it.
> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 

Could you fold this patch into previous one?

> ---
>  drivers/block/zram/zcomp.c| 5 ++---
>  drivers/block/zram/zcomp.h| 4 ++--
>  drivers/block/zram/zram_drv.c | 4 ++--
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 82b568e..6e16fa2 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -102,7 +102,7 @@ void zcomp_stream_put(struct zcomp *comp)
>   put_cpu_ptr(comp->stream);
>  }
>  
> -int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_compress(struct zcomp_strm *zstrm,
>   const unsigned char *src, unsigned int *dst_len)
>  {
>   /*
> @@ -126,8 +126,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm 
> *zstrm,
>   zstrm->buffer, dst_len);
>  }
>  
> -int zcomp_decompress(struct zcomp *comp,
> - struct zcomp_strm *zstrm,
> +int zcomp_decompress(struct zcomp_strm *zstrm,
>   const unsigned char *src,
>   size_t src_len, unsigned char *dst)
>  {
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index ba36bde..3fb9dc4 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -53,10 +53,10 @@ void zcomp_destroy(struct zcomp *comp);
>  struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
>  void zcomp_stream_put(struct zcomp *comp);
>  
> -int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_compress(struct zcomp_strm *zstrm,
>   const unsigned char *src, unsigned int *dst_len);
>  
> -int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_decompress(struct zcomp_strm *zstrm,
>   const unsigned char *src,
>   size_t src_len, unsigned char *dst);
>  
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index fa22a32..99c8be7 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -581,7 +581,7 @@ static int zram_decompress_page(struct zram *zram, char 
> *mem, u32 index)
>   } else {
>   struct zcomp_strm *zstrm = zcomp_stream_get(zram->comp);
>  
> - ret = zcomp_decompress(zram->comp, zstrm, cmem, size, mem);
> + ret = zcomp_decompress(zstrm, cmem, size, mem);
>   zcomp_stream_put(zram->comp);
>   }
>   zs_unmap_object(meta->mem_pool, handle);
> @@ -700,7 +700,7 @@ compress_again:
>   }
>  
>   zstrm = zcomp_stream_get(zram->comp);
> - ret = zcomp_compress(zram->comp, zstrm, uncmem, );
> + ret = zcomp_compress(zstrm, uncmem, );
>   if (!is_partial_io(bvec)) {
>   kunmap_atomic(user_mem);
>   user_mem = NULL;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [PATCH 3/7] zram: drop zcomp param from compress/decompress

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:02PM +0900, Sergey Senozhatsky wrote:
> We don't need to supply a zcomp pointer to compress/decompress
> functions, drop it.
> 
> Signed-off-by: Sergey Senozhatsky 
> Cc: Minchan Kim 
> Cc: Joonsoo Kim 

Could you fold this patch into previous one?

> ---
>  drivers/block/zram/zcomp.c| 5 ++---
>  drivers/block/zram/zcomp.h| 4 ++--
>  drivers/block/zram/zram_drv.c | 4 ++--
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 82b568e..6e16fa2 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -102,7 +102,7 @@ void zcomp_stream_put(struct zcomp *comp)
>   put_cpu_ptr(comp->stream);
>  }
>  
> -int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_compress(struct zcomp_strm *zstrm,
>   const unsigned char *src, unsigned int *dst_len)
>  {
>   /*
> @@ -126,8 +126,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm 
> *zstrm,
>   zstrm->buffer, dst_len);
>  }
>  
> -int zcomp_decompress(struct zcomp *comp,
> - struct zcomp_strm *zstrm,
> +int zcomp_decompress(struct zcomp_strm *zstrm,
>   const unsigned char *src,
>   size_t src_len, unsigned char *dst)
>  {
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index ba36bde..3fb9dc4 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -53,10 +53,10 @@ void zcomp_destroy(struct zcomp *comp);
>  struct zcomp_strm *zcomp_stream_get(struct zcomp *comp);
>  void zcomp_stream_put(struct zcomp *comp);
>  
> -int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_compress(struct zcomp_strm *zstrm,
>   const unsigned char *src, unsigned int *dst_len);
>  
> -int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
> +int zcomp_decompress(struct zcomp_strm *zstrm,
>   const unsigned char *src,
>   size_t src_len, unsigned char *dst);
>  
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index fa22a32..99c8be7 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -581,7 +581,7 @@ static int zram_decompress_page(struct zram *zram, char 
> *mem, u32 index)
>   } else {
>   struct zcomp_strm *zstrm = zcomp_stream_get(zram->comp);
>  
> - ret = zcomp_decompress(zram->comp, zstrm, cmem, size, mem);
> + ret = zcomp_decompress(zstrm, cmem, size, mem);
>   zcomp_stream_put(zram->comp);
>   }
>   zs_unmap_object(meta->mem_pool, handle);
> @@ -700,7 +700,7 @@ compress_again:
>   }
>  
>   zstrm = zcomp_stream_get(zram->comp);
> - ret = zcomp_compress(zram->comp, zstrm, uncmem, );
> + ret = zcomp_compress(zstrm, uncmem, );
>   if (!is_partial_io(bvec)) {
>   kunmap_atomic(user_mem);
>   user_mem = NULL;
> -- 
> 2.8.3.394.g3916adf
> 

Re: [PATCH 2/7] zram: switch to crypto compress API

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:01PM +0900, Sergey Senozhatsky wrote:
> We don't have an idle zstreams list anymore and our write path
> now works absolutely differently, preventing preemption during
> compression. This removes possibilities of read paths preempting
> writes at wrong places (which could badly affect the performance
> of both paths) and at the same time opens the door for a move
> from custom LZO/LZ4 compression backends implementation to a more
> generic one, using crypto compress API.
> 
> Joonsoo Kim [1] attempted to do this a while ago, but faced with
> the need of introducing a new crypto API interface. The root cause
> was the fact that crypto API compression algorithms require a
> compression stream structure (in zram terminology) for both
> compression and decompression ops, while in reality only several
> of compression algorithms really need it. This resulted in a
> concept of context-less crypto API compression backends [2]. Both
> write and read paths, though, would have been executed with the
> preemption enabled, which in the worst case could have resulted
> in a decreased worst-case performance, e.g. consider the
> following case:
> 
>   CPU0
> 
>   zram_write()
> spin_lock()
>   take the last idle stream
> spin_unlock()
> 
>   << preempted >>
> 
>   zram_read()
> spin_lock()
>  no idle streams
> spin_unlock()
> schedule()
> 
>   resuming zram_write compression()
> 
> but it took me some time to realize that, and it took even longer
> to evolve zram and to make it ready for crypto API. The key turned
> out to be -- drop the idle streams list entirely. With out the

Without

> idle streams list we are free to use compression algorithms that
> require compression stream for decompression (read), because streams
> are now placed in per-cpu data and each write path has to disable
> preemption for compression op, almost completely eliminating the
> aforementioned case (technically, we still have a small chance,
> because write path has a fast and a slow paths and the slow path
> is executed with the preemption enabled; but the frequency of
> failed fast path is too low).

Nice description.

> 
> TEST
> 
> 
> - 4 CPUs, x86_64 system
> - 3G zram, lzo
> - fio tests: read, randread, write, randwrite, rw, randrw
> 
> test script [3] command:
>  ZRAM_SIZE=3G LOG_SUFFIX= FIO_LOOPS=5 ./zram-fio-test.sh
> 
>BASE   PATCHED
> jobs1
> READ:   2527.2MB/s 2482.7MB/s
> READ:   2102.7MB/s 2045.0MB/s
> WRITE:  1284.3MB/s 1324.3MB/s
> WRITE:  1080.7MB/s 1101.9MB/s
> READ:   430125KB/s 437498KB/s
> WRITE:  430538KB/s 437919KB/s
> READ:   399593KB/s 403987KB/s
> WRITE:  399910KB/s 404308KB/s
> jobs2
> READ:   8133.5MB/s 7854.8MB/s
> READ:   7086.6MB/s 6912.8MB/s
> WRITE:  3177.2MB/s 3298.3MB/s
> WRITE:  2810.2MB/s 2871.4MB/s
> READ:   1017.6MB/s 1023.4MB/s
> WRITE:  1018.2MB/s 1023.1MB/s
> READ:   977836KB/s 984205KB/s
> WRITE:  979435KB/s 985814KB/s
> jobs3
> READ:   13557MB/s  13391MB/s
> READ:   11876MB/s  11752MB/s
> WRITE:  4641.5MB/s 4682.1MB/s
> WRITE:  4164.9MB/s 4179.3MB/s
> READ:   1453.8MB/s 1455.1MB/s
> WRITE:  1455.1MB/s 1458.2MB/s
> READ:   1387.7MB/s 1395.7MB/s
> WRITE:  1386.1MB/s 1394.9MB/s
> jobs4
> READ:   20271MB/s  20078MB/s
> READ:   18033MB/s  17928MB/s
> WRITE:  6176.8MB/s 6180.5MB/s
> WRITE:  5686.3MB/s 5705.3MB/s
> READ:   2009.4MB/s 2006.7MB/s
> WRITE:  2007.5MB/s 2004.9MB/s
> READ:   1929.7MB/s 1935.6MB/s
> WRITE:  1926.8MB/s 1932.6MB/s
> jobs5
> READ:   18823MB/s  19024MB/s
> READ:   18968MB/s  19071MB/s
> WRITE:  6191.6MB/s 6372.1MB/s
> WRITE:  5818.7MB/s 5787.1MB/s
> READ:   2011.7MB/s 1981.3MB/s
> WRITE:  2011.4MB/s 1980.1MB/s
> READ:   1949.3MB/s 1935.7MB/s
> WRITE:  1940.4MB/s 1926.1MB/s
> jobs6
> READ:   21870MB/s  21715MB/s
> READ:   19957MB/s  19879MB/s
> WRITE:  6528.4MB/s 6537.6MB/s
> WRITE:  6098.9MB/s 6073.6MB/s
> READ:   2048.6MB/s 2049.9MB/s
> WRITE:  2041.7MB/s 2042.9MB/s
> READ:   2013.4MB/s 1990.4MB/s
> WRITE:  2009.4MB/s 1986.5MB/s
> jobs7
> READ:   21359MB/s  21124MB/s
> READ:   19746MB/s  19293MB/s
> WRITE:  6660.4MB/s 6518.8MB/s
> WRITE:  6211.6MB/s 6193.1MB/s
> READ:   2089.7MB/s 2080.6MB/s
> WRITE:  

Re: [PATCH 2/7] zram: switch to crypto compress API

[Minchan Kim]

On Wed, May 25, 2016 at 11:30:01PM +0900, Sergey Senozhatsky wrote:
> We don't have an idle zstreams list anymore and our write path
> now works absolutely differently, preventing preemption during
> compression. This removes possibilities of read paths preempting
> writes at wrong places (which could badly affect the performance
> of both paths) and at the same time opens the door for a move
> from custom LZO/LZ4 compression backends implementation to a more
> generic one, using crypto compress API.
> 
> Joonsoo Kim [1] attempted to do this a while ago, but faced with
> the need of introducing a new crypto API interface. The root cause
> was the fact that crypto API compression algorithms require a
> compression stream structure (in zram terminology) for both
> compression and decompression ops, while in reality only several
> of compression algorithms really need it. This resulted in a
> concept of context-less crypto API compression backends [2]. Both
> write and read paths, though, would have been executed with the
> preemption enabled, which in the worst case could have resulted
> in a decreased worst-case performance, e.g. consider the
> following case:
> 
>   CPU0
> 
>   zram_write()
> spin_lock()
>   take the last idle stream
> spin_unlock()
> 
>   << preempted >>
> 
>   zram_read()
> spin_lock()
>  no idle streams
> spin_unlock()
> schedule()
> 
>   resuming zram_write compression()
> 
> but it took me some time to realize that, and it took even longer
> to evolve zram and to make it ready for crypto API. The key turned
> out to be -- drop the idle streams list entirely. With out the

Without

> idle streams list we are free to use compression algorithms that
> require compression stream for decompression (read), because streams
> are now placed in per-cpu data and each write path has to disable
> preemption for compression op, almost completely eliminating the
> aforementioned case (technically, we still have a small chance,
> because write path has a fast and a slow paths and the slow path
> is executed with the preemption enabled; but the frequency of
> failed fast path is too low).

Nice description.

> 
> TEST
> 
> 
> - 4 CPUs, x86_64 system
> - 3G zram, lzo
> - fio tests: read, randread, write, randwrite, rw, randrw
> 
> test script [3] command:
>  ZRAM_SIZE=3G LOG_SUFFIX= FIO_LOOPS=5 ./zram-fio-test.sh
> 
>BASE   PATCHED
> jobs1
> READ:   2527.2MB/s 2482.7MB/s
> READ:   2102.7MB/s 2045.0MB/s
> WRITE:  1284.3MB/s 1324.3MB/s
> WRITE:  1080.7MB/s 1101.9MB/s
> READ:   430125KB/s 437498KB/s
> WRITE:  430538KB/s 437919KB/s
> READ:   399593KB/s 403987KB/s
> WRITE:  399910KB/s 404308KB/s
> jobs2
> READ:   8133.5MB/s 7854.8MB/s
> READ:   7086.6MB/s 6912.8MB/s
> WRITE:  3177.2MB/s 3298.3MB/s
> WRITE:  2810.2MB/s 2871.4MB/s
> READ:   1017.6MB/s 1023.4MB/s
> WRITE:  1018.2MB/s 1023.1MB/s
> READ:   977836KB/s 984205KB/s
> WRITE:  979435KB/s 985814KB/s
> jobs3
> READ:   13557MB/s  13391MB/s
> READ:   11876MB/s  11752MB/s
> WRITE:  4641.5MB/s 4682.1MB/s
> WRITE:  4164.9MB/s 4179.3MB/s
> READ:   1453.8MB/s 1455.1MB/s
> WRITE:  1455.1MB/s 1458.2MB/s
> READ:   1387.7MB/s 1395.7MB/s
> WRITE:  1386.1MB/s 1394.9MB/s
> jobs4
> READ:   20271MB/s  20078MB/s
> READ:   18033MB/s  17928MB/s
> WRITE:  6176.8MB/s 6180.5MB/s
> WRITE:  5686.3MB/s 5705.3MB/s
> READ:   2009.4MB/s 2006.7MB/s
> WRITE:  2007.5MB/s 2004.9MB/s
> READ:   1929.7MB/s 1935.6MB/s
> WRITE:  1926.8MB/s 1932.6MB/s
> jobs5
> READ:   18823MB/s  19024MB/s
> READ:   18968MB/s  19071MB/s
> WRITE:  6191.6MB/s 6372.1MB/s
> WRITE:  5818.7MB/s 5787.1MB/s
> READ:   2011.7MB/s 1981.3MB/s
> WRITE:  2011.4MB/s 1980.1MB/s
> READ:   1949.3MB/s 1935.7MB/s
> WRITE:  1940.4MB/s 1926.1MB/s
> jobs6
> READ:   21870MB/s  21715MB/s
> READ:   19957MB/s  19879MB/s
> WRITE:  6528.4MB/s 6537.6MB/s
> WRITE:  6098.9MB/s 6073.6MB/s
> READ:   2048.6MB/s 2049.9MB/s
> WRITE:  2041.7MB/s 2042.9MB/s
> READ:   2013.4MB/s 1990.4MB/s
> WRITE:  2009.4MB/s 1986.5MB/s
> jobs7
> READ:   21359MB/s  21124MB/s
> READ:   19746MB/s  19293MB/s
> WRITE:  6660.4MB/s 6518.8MB/s
> WRITE:  6211.6MB/s 6193.1MB/s
> READ:   2089.7MB/s 2080.6MB/s
> WRITE:  

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

[Rob Herring]

On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
 wrote:
>
>
> On 2016/5/26 21:13, Rob Herring wrote:
>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>>> For a normal memory@ devicetree node, its reg property can contains more
>>> memory blocks.
>>>
>>> Because we don't known how many memory blocks maybe contained, so we try
>>> from index=0, increase 1 until error returned(the end).
>>>
>>> Signed-off-by: Zhen Lei 
>>> ---
>>>  drivers/of/of_numa.c | 30 --
>>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
>>> index 21d831f..2c5f249 100644
>>> --- a/drivers/of/of_numa.c
>>> +++ b/drivers/of/of_numa.c
>>> @@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
>>>  struct device_node *np = NULL;
>>>  struct resource rsrc;
>>>  u32 nid;
>>> -int r = 0;
>>> +int i, r = 0;
>>>
>>>  for (;;) {
>>>  np = of_find_node_by_type(np, "memory");
>>> @@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
>>>  /* some other error */
>>>  break;
>>>
>>> -r = of_address_to_resource(np, 0, );
>>> -if (r) {
>>> -pr_err("NUMA: bad reg property in memory node\n");
>>> -break;
>>> +for (i = 0; ; i++) {
>>> +r = of_address_to_resource(np, i, );
>>> +if (r) {
>>> +/* reached the end of of_address */
>>> +if (i > 0) {
>>> +r = 0;
>>> +break;
>>> +}
>>> +
>>> +pr_err("NUMA: bad reg property in memory 
>>> node\n");
>>> +goto finished;
>>> +}
>>> +
>>> +r = numa_add_memblk(nid, rsrc.start,
>>> +rsrc.end - rsrc.start + 1);
>>> +if (r)
>>> +goto finished;
>>>  }
>>> -
>>> -r = numa_add_memblk(nid, rsrc.start,
>>> -rsrc.end - rsrc.start + 1);
>>> -if (r)
>>> -break;
>>>  }
>>> +
>>> +finished:
>>>  of_node_put(np);
>>
>> This function can be simplified down to:
>>
>>   for_each_node_by_type(np, "memory") {
> OK, That's good.
>
>>   r = of_property_read_u32(np, "numa-node-id", );
>>   if (r == -EINVAL)
>>   /*
>>* property doesn't exist if -EINVAL, continue
>>* looking for more memory nodes with
>>* "numa-node-id" property
>>*/
>>   continue;
> Hi, everybody:
> If some "memory" node contains "numa-node-id", but some others missed. 
> Can we simply ignored it?
> I think we should break out too, and faking to only have node0.

Continuing to work is probably better than not.

>
>>   else if (r)
>>   /* some other error */
>>   break;
>>
>>   r = of_address_to_resource(np, 0, );
>>   for (i = 0; !r; i++, r = of_address_to_resource(np, i,
>
> But r(non-zero) is just break this loop, the original is break the outer for 
> (;;) loop

It is not really the kernel's job to validate the DT. If there's
random things in it then kernel's behavior is undefined.

>
> How about as below?
>
> for_each_node_by_type(np, "memory") {
> ... ...
>
> for (i = 0; !of_address_to_resource(np, i, ); i++) {
> r = numa_add_memblk(nid, rsrc.start,
> rsrc.end - rsrc.start + 1);
> if (r)
> goto finished;
> }
>
> if (!i)
> pr_err("NUMA: bad reg property in memory node\n");
> }
>
> finished:

Please try to avoid the goto. You can check r in the outer loop too.

Rob

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

[Rob Herring]

On Thu, May 26, 2016 at 10:36 PM, Leizhen (ThunderTown)
 wrote:
>
>
> On 2016/5/26 21:13, Rob Herring wrote:
>> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>>> For a normal memory@ devicetree node, its reg property can contains more
>>> memory blocks.
>>>
>>> Because we don't known how many memory blocks maybe contained, so we try
>>> from index=0, increase 1 until error returned(the end).
>>>
>>> Signed-off-by: Zhen Lei 
>>> ---
>>>  drivers/of/of_numa.c | 30 --
>>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
>>> index 21d831f..2c5f249 100644
>>> --- a/drivers/of/of_numa.c
>>> +++ b/drivers/of/of_numa.c
>>> @@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
>>>  struct device_node *np = NULL;
>>>  struct resource rsrc;
>>>  u32 nid;
>>> -int r = 0;
>>> +int i, r = 0;
>>>
>>>  for (;;) {
>>>  np = of_find_node_by_type(np, "memory");
>>> @@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
>>>  /* some other error */
>>>  break;
>>>
>>> -r = of_address_to_resource(np, 0, );
>>> -if (r) {
>>> -pr_err("NUMA: bad reg property in memory node\n");
>>> -break;
>>> +for (i = 0; ; i++) {
>>> +r = of_address_to_resource(np, i, );
>>> +if (r) {
>>> +/* reached the end of of_address */
>>> +if (i > 0) {
>>> +r = 0;
>>> +break;
>>> +}
>>> +
>>> +pr_err("NUMA: bad reg property in memory 
>>> node\n");
>>> +goto finished;
>>> +}
>>> +
>>> +r = numa_add_memblk(nid, rsrc.start,
>>> +rsrc.end - rsrc.start + 1);
>>> +if (r)
>>> +goto finished;
>>>  }
>>> -
>>> -r = numa_add_memblk(nid, rsrc.start,
>>> -rsrc.end - rsrc.start + 1);
>>> -if (r)
>>> -break;
>>>  }
>>> +
>>> +finished:
>>>  of_node_put(np);
>>
>> This function can be simplified down to:
>>
>>   for_each_node_by_type(np, "memory") {
> OK, That's good.
>
>>   r = of_property_read_u32(np, "numa-node-id", );
>>   if (r == -EINVAL)
>>   /*
>>* property doesn't exist if -EINVAL, continue
>>* looking for more memory nodes with
>>* "numa-node-id" property
>>*/
>>   continue;
> Hi, everybody:
> If some "memory" node contains "numa-node-id", but some others missed. 
> Can we simply ignored it?
> I think we should break out too, and faking to only have node0.

Continuing to work is probably better than not.

>
>>   else if (r)
>>   /* some other error */
>>   break;
>>
>>   r = of_address_to_resource(np, 0, );
>>   for (i = 0; !r; i++, r = of_address_to_resource(np, i,
>
> But r(non-zero) is just break this loop, the original is break the outer for 
> (;;) loop

It is not really the kernel's job to validate the DT. If there's
random things in it then kernel's behavior is undefined.

>
> How about as below?
>
> for_each_node_by_type(np, "memory") {
> ... ...
>
> for (i = 0; !of_address_to_resource(np, i, ); i++) {
> r = numa_add_memblk(nid, rsrc.start,
> rsrc.end - rsrc.start + 1);
> if (r)
> goto finished;
> }
>
> if (!i)
> pr_err("NUMA: bad reg property in memory node\n");
> }
>
> finished:

Please try to avoid the goto. You can check r in the outer loop too.

Rob

RE: [PATCH] Use the correct size to set block max sectors

[Long Li]

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@sandisk.com]
> Sent: Thursday, May 26, 2016 7:19 PM
> To: Long Li ; James E.J. Bottomley
> ; Martin K. Petersen
> 
> Cc: KY Srinivasan ; linux-s...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] Use the correct size to set block max sectors
> 
> On 05/26/16 17:08, Long Li wrote:
> > The block sector size should be in unit of 512 bytes, not in bytes.
> >
> > Signed-off-by: Long Li 
> >
> > ---
> >  drivers/scsi/sd.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > 428c03e..4bce17e 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
> > if (sdkp->opt_xfer_blocks &&
> > sdkp->opt_xfer_blocks <= dev_max &&
> > sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> > -   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
> > -   rw_max = q->limits.io_opt =
> > +   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
> > +   q->limits.io_opt =
> > sdkp->opt_xfer_blocks * sdp->sector_size;
> > +   rw_max = (q->limits.io_opt >> 9);
> > +   }
> > else
> > rw_max = BLK_DEF_MAX_SECTORS;
> 
> Isn't this a duplicate of a patch Martin Petersen posted three weeks ago? See
> also
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fthread.g
> mane.org%2fgmane.linux.scsi%2f113746=01%7c01%7clongli%40micros
> oft.com%7c4396718e6f9749d178bf08d385d53bba%7c72f988bf86f141af91ab2
> d7cd011db47%7c1=UpiYwEdYMtqcwuNS1llVuXcQ6riFT3b5%2b44Sn56
> Bl14%3d.
> 
> Bart.
> 
> 

Yes, this has been fixed in that patch.

Please drop this one.

RE: [PATCH] Use the correct size to set block max sectors

[Long Li]

> -Original Message-
> From: Bart Van Assche [mailto:bart.vanass...@sandisk.com]
> Sent: Thursday, May 26, 2016 7:19 PM
> To: Long Li ; James E.J. Bottomley
> ; Martin K. Petersen
> 
> Cc: KY Srinivasan ; linux-s...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH] Use the correct size to set block max sectors
> 
> On 05/26/16 17:08, Long Li wrote:
> > The block sector size should be in unit of 512 bytes, not in bytes.
> >
> > Signed-off-by: Long Li 
> >
> > ---
> >  drivers/scsi/sd.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > 428c03e..4bce17e 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
> > if (sdkp->opt_xfer_blocks &&
> > sdkp->opt_xfer_blocks <= dev_max &&
> > sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> > -   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
> > -   rw_max = q->limits.io_opt =
> > +   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
> > +   q->limits.io_opt =
> > sdkp->opt_xfer_blocks * sdp->sector_size;
> > +   rw_max = (q->limits.io_opt >> 9);
> > +   }
> > else
> > rw_max = BLK_DEF_MAX_SECTORS;
> 
> Isn't this a duplicate of a patch Martin Petersen posted three weeks ago? See
> also
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fthread.g
> mane.org%2fgmane.linux.scsi%2f113746=01%7c01%7clongli%40micros
> oft.com%7c4396718e6f9749d178bf08d385d53bba%7c72f988bf86f141af91ab2
> d7cd011db47%7c1=UpiYwEdYMtqcwuNS1llVuXcQ6riFT3b5%2b44Sn56
> Bl14%3d.
> 
> Bart.
> 
> 

Yes, this has been fixed in that patch.

Please drop this one.

Re: BLKZEROOUT not zeroing md dev on VMDK

[Darrick J. Wong]

On Wed, May 18, 2016 at 11:39:30PM +0100, Sitsofe Wheeler wrote:
> Hi,
> 
> With Ubuntu's 4.4.0-22-generic kernel and a Fedora 23
> 4.6.0-1.vanilla.knurd.1.fc23.x86_64 kernel I've found that the
> BLKZEROOUT syscall can malfunction and not zero data.
> 
> When BLKZEROOUT is issued to an MD device atop a PVSCSI controller
> supplied VMDK from ESXi 6.0 the call returns immediately and with a zero
> return code. Unfortunately, inspecting the data on the MD device shows
> that it has not been zeroed and is in fact untouched. The easiest way to
> see this behaviour is to boot the VM, create an mdadm device atop
> /dev/sd?, scribble some non-zero value on the disk and then use
> blkdiscard --zeroout /dev/md??? . If you then inspect the MD disk (e.g.
> with hexdump) you will still see the old data and using POSIX_FADV_DONTNEED
> on the MD device doesn't change the outcome.
> 
> The only clue I've seen is that
> /sys/block/sd?/queue/write_same_max_bytes starts out being 33553920 but
> after a WRITE SAME is issued it becomes 0. If the MD device is created
> after write_same_max_bytes has become 0 on the backing disk then
> BLKZEROOUT seems to work correctly.

It's possible that the pvscsi device advertised WRITE SAME, but if the device
sends back ILLEGAL REQUEST then the SCSI disk driver will set
write_same_max_bytes=0.  Subsequent BLKZEROOUT attempts will then issue writes
of zeroes to the drive.

--D

> 
> -- 
> Sitsofe | http://sucs.org/~sits/

Re: BLKZEROOUT not zeroing md dev on VMDK

[Darrick J. Wong]

On Wed, May 18, 2016 at 11:39:30PM +0100, Sitsofe Wheeler wrote:
> Hi,
> 
> With Ubuntu's 4.4.0-22-generic kernel and a Fedora 23
> 4.6.0-1.vanilla.knurd.1.fc23.x86_64 kernel I've found that the
> BLKZEROOUT syscall can malfunction and not zero data.
> 
> When BLKZEROOUT is issued to an MD device atop a PVSCSI controller
> supplied VMDK from ESXi 6.0 the call returns immediately and with a zero
> return code. Unfortunately, inspecting the data on the MD device shows
> that it has not been zeroed and is in fact untouched. The easiest way to
> see this behaviour is to boot the VM, create an mdadm device atop
> /dev/sd?, scribble some non-zero value on the disk and then use
> blkdiscard --zeroout /dev/md??? . If you then inspect the MD disk (e.g.
> with hexdump) you will still see the old data and using POSIX_FADV_DONTNEED
> on the MD device doesn't change the outcome.
> 
> The only clue I've seen is that
> /sys/block/sd?/queue/write_same_max_bytes starts out being 33553920 but
> after a WRITE SAME is issued it becomes 0. If the MD device is created
> after write_same_max_bytes has become 0 on the backing disk then
> BLKZEROOUT seems to work correctly.

It's possible that the pvscsi device advertised WRITE SAME, but if the device
sends back ILLEGAL REQUEST then the SCSI disk driver will set
write_same_max_bytes=0.  Subsequent BLKZEROOUT attempts will then issue writes
of zeroes to the drive.

--D

> 
> -- 
> Sitsofe | http://sucs.org/~sits/

Re: [PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

在 2016/5/27 11:46, Jaehoon Chung 写道:

Hi Shawn,

On 05/27/2016 12:02 PM, Shawn Lin wrote:

The main reason to add this check is to avoid unnecessary
mmc_request like the on-going cmd and the corresponding sbc
if the card is removed. Although we have already checked this in
dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
for noremovable devices, but there is a timing gap before it really
calls dw_mci_get_cd as mmc_detect_change needs some delay here.

Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..8eb7898 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);

 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)
 */
spin_lock_bh(>lock);

-   if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
+   if (!dw_mci_get_cd(mmc)) {
spin_unlock_bh(>lock);
mrq->cmd->error = -ENOMEDIUM;
mmc_request_done(mmc, mrq);


I think spin_lock/unlock_bh() should be removed at here.


yes, we should hole the lock before dw_mci_queue_request as
dw_mci_get_cd release it, in which state we do the minmum changes
for achieving what we need here.

I will fix it.


Instead, Can it locate before calling dw_mci_queue_request()?

Anyway, it looks good to me.

Best Regards,
Jaehoon Chung











--
Best Regards
Shawn Lin

Re: [PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

在 2016/5/27 11:46, Jaehoon Chung 写道:

Hi Shawn,

On 05/27/2016 12:02 PM, Shawn Lin wrote:

The main reason to add this check is to avoid unnecessary
mmc_request like the on-going cmd and the corresponding sbc
if the card is removed. Although we have already checked this in
dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
for noremovable devices, but there is a timing gap before it really
calls dw_mci_get_cd as mmc_detect_change needs some delay here.

Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..8eb7898 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);

 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)
 */
spin_lock_bh(>lock);

-   if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
+   if (!dw_mci_get_cd(mmc)) {
spin_unlock_bh(>lock);
mrq->cmd->error = -ENOMEDIUM;
mmc_request_done(mmc, mrq);


I think spin_lock/unlock_bh() should be removed at here.


yes, we should hole the lock before dw_mci_queue_request as
dw_mci_get_cd release it, in which state we do the minmum changes
for achieving what we need here.

I will fix it.


Instead, Can it locate before calling dw_mci_queue_request()?

Anyway, it looks good to me.

Best Regards,
Jaehoon Chung











--
Best Regards
Shawn Lin

Re: [PATCH 1/1] net: nps_enet: Disable interrupts before napi reschedule

[Vineet Gupta]

Hi Elad, Noam,

On Thursday 26 May 2016 11:23 PM, Alexey Brodkin wrote:

> 
> We just bumped into the same problem (data exchange hangs on the very first 
> "ping")
> with released Linux v4.6 and linux-next on our nSIM OSCI virtual platform.
> 
> I believe it was commit 05c00d82f4d1 ("net: nps_enet: bug fix - handle lost 
> tx interrupts")
> that introduced the problem. At least reverting it I got networking working.
> 
> And indeed that patch fixes mentioned issue.
> In other words...
> 
> Tested-by: Alexey Brodkin 

FWIW, we now actively use the same driver (and same systemc model) in one of our
our simulation platforms used for testing regressions. So please try to keep arc
mailing list on CC for any nps_enet driver patches so we are in loop and know 
what
is going on !

Thx,
-Vineet

Re: [PATCH 1/1] net: nps_enet: Disable interrupts before napi reschedule

[Vineet Gupta]

Hi Elad, Noam,

On Thursday 26 May 2016 11:23 PM, Alexey Brodkin wrote:

> 
> We just bumped into the same problem (data exchange hangs on the very first 
> "ping")
> with released Linux v4.6 and linux-next on our nSIM OSCI virtual platform.
> 
> I believe it was commit 05c00d82f4d1 ("net: nps_enet: bug fix - handle lost 
> tx interrupts")
> that introduced the problem. At least reverting it I got networking working.
> 
> And indeed that patch fixes mentioned issue.
> In other words...
> 
> Tested-by: Alexey Brodkin 

FWIW, we now actively use the same driver (and same systemc model) in one of our
our simulation platforms used for testing regressions. So please try to keep arc
mailing list on CC for any nps_enet driver patches so we are in loop and know 
what
is going on !

Thx,
-Vineet

Re: [PATCH v2 1/2] dt: binding: Add Qualcomm WCNSS control binding

[Rob Herring]

+dt list

On Thu, May 12, 2016 at 6:17 PM, Bjorn Andersson
 wrote:
> This binding describes the control interface for the Qualcomm WCNSS.
>
> Signed-off-by: Bjorn Andersson 
> ---
>
> Changes since v1:
> - Introduce reference to wcnss block node for register block definition
> - Use wcnss block node compatible for hw version detection (riva vs pronto)
> - Clean up compatible name for bt node
>
>  .../devicetree/bindings/soc/qcom/qcom,wcnss.txt| 115 
> +
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt 
> b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> new file mode 100644
> index ..5413098a1e1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> @@ -0,0 +1,115 @@
> +Qualcomm WCNSS Binding
> +
> +This binding describes the Qualcomm WCNSS hardware. It consists of control
> +block and a BT, WiFi and FM radio block, all useing SMD as command channels.

s/useing/using/

> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be: "qcom,wcnss",
> +
> +- qcom,smd-channel:
> +   Usage: required
> +   Value type: 
> +   Definition: standard SMD property specifying the SMD channel used for
> +   communication with the WiFi firmware

Is the string value defined somewhere?

> +
> +- qcom,mmio:
> +   Usage: required
> +   Value type: 
> +   Definition: reference to a node specifying the wcnss "ccu" and "dxe"
> +   register blocks. The node must be compatible with one of
> +   the following:
> +   "qcom,riva",
> +   "qcom,pronto"
> +
> += SUBNODES
> +The subnodes of the wcnss node are optional and describe the individual 
> blocks in
> +the WCNSS.
> +
> +== Bluetooth
> +The following properties are defined to the bluetooth node:
> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be:
> +   "qcom,wcnss-bt"
> +
> +== WiFi
> +The following properties are defined to the WiFi node:
> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be one of:
> +   "qcom,wcnss-wlan",
> +
> +- interrupts:
> +   Usage: required
> +   Value type: 
> +   Definition: should specify the "rx" and "tx" interrupts
> +
> +- interrupt-names:
> +   Usage: required
> +   Value type: 
> +   Definition: must contain "rx" and "tx"
> +
> +- qcom,state:

This is kind of generic.

> +   Usage: required
> +   Value type: 
> +   Definition: should reference the tx-enable and tx-rings-empty SMEM 
> states
> +
> +- qcom,state-names:
> +   Usage: required
> +   Value type: 
> +   Definition: must contain "tx-enable" and "tx-rings-empty"
> +
> += EXAMPLE
> +The following example represents a SMD node, with one edge representing the
> +"pronto" subsystem, with the wcnss device and its wcn3680 BT and WiFi blocks
> +described; as found on the 8974 platform.

So somewhere in here I'd expect to see 8974 in a compatible string.

> +
> +smd {
> +   compatible = "qcom,smd";
> +
> +   pronto {
> +   interrupts = <0 142 1>;
> +
> +   qcom,ipc = < 8 17>;
> +   qcom,smd-edge = <6>;
> +
> +   wcnss {
> +   compatible = "qcom,wcnss";
> +   qcom,smd-channels = "WCNSS_CTRL";
> +
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   qcom,wcnss = <>;

Not documented and poorly named. The property name should reflect what
this is for or points too.

> +
> +   bt {
> +   compatible = "qcom,wcnss-bt";
> +   };
> +
> +   wlan {
> +   compatible = "qcom,wcnss-wlan";
> +
> +   interrupts = <0 145 0>, <0 146 0>;
> +   interrupt-names = "tx", "rx";
> +
> +   qcom,state = <_smsm 10>, <_smsm 9>;
> +   qcom,state-names = "tx-enable", 
> "tx-rings-empty";
> +   };
> +   };
> +   };
> +};
> +
> +soc {
> +   pronto: pronto {

2 pronto nodes? That's confusing...

> +   compatible = "qcom,pronto";
> +
> +   reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 
> 0x3000>;
> +   reg-names = "ccu", "dxe", "pmu";
> +   };
> +};
> --
> 2.5.0
>

Re: [PATCH v2 1/2] dt: binding: Add Qualcomm WCNSS control binding

[Rob Herring]

+dt list

On Thu, May 12, 2016 at 6:17 PM, Bjorn Andersson
 wrote:
> This binding describes the control interface for the Qualcomm WCNSS.
>
> Signed-off-by: Bjorn Andersson 
> ---
>
> Changes since v1:
> - Introduce reference to wcnss block node for register block definition
> - Use wcnss block node compatible for hw version detection (riva vs pronto)
> - Clean up compatible name for bt node
>
>  .../devicetree/bindings/soc/qcom/qcom,wcnss.txt| 115 
> +
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt 
> b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> new file mode 100644
> index ..5413098a1e1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> @@ -0,0 +1,115 @@
> +Qualcomm WCNSS Binding
> +
> +This binding describes the Qualcomm WCNSS hardware. It consists of control
> +block and a BT, WiFi and FM radio block, all useing SMD as command channels.

s/useing/using/

> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be: "qcom,wcnss",
> +
> +- qcom,smd-channel:
> +   Usage: required
> +   Value type: 
> +   Definition: standard SMD property specifying the SMD channel used for
> +   communication with the WiFi firmware

Is the string value defined somewhere?

> +
> +- qcom,mmio:
> +   Usage: required
> +   Value type: 
> +   Definition: reference to a node specifying the wcnss "ccu" and "dxe"
> +   register blocks. The node must be compatible with one of
> +   the following:
> +   "qcom,riva",
> +   "qcom,pronto"
> +
> += SUBNODES
> +The subnodes of the wcnss node are optional and describe the individual 
> blocks in
> +the WCNSS.
> +
> +== Bluetooth
> +The following properties are defined to the bluetooth node:
> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be:
> +   "qcom,wcnss-bt"
> +
> +== WiFi
> +The following properties are defined to the WiFi node:
> +
> +- compatible:
> +   Usage: required
> +   Value type: 
> +   Definition: must be one of:
> +   "qcom,wcnss-wlan",
> +
> +- interrupts:
> +   Usage: required
> +   Value type: 
> +   Definition: should specify the "rx" and "tx" interrupts
> +
> +- interrupt-names:
> +   Usage: required
> +   Value type: 
> +   Definition: must contain "rx" and "tx"
> +
> +- qcom,state:

This is kind of generic.

> +   Usage: required
> +   Value type: 
> +   Definition: should reference the tx-enable and tx-rings-empty SMEM 
> states
> +
> +- qcom,state-names:
> +   Usage: required
> +   Value type: 
> +   Definition: must contain "tx-enable" and "tx-rings-empty"
> +
> += EXAMPLE
> +The following example represents a SMD node, with one edge representing the
> +"pronto" subsystem, with the wcnss device and its wcn3680 BT and WiFi blocks
> +described; as found on the 8974 platform.

So somewhere in here I'd expect to see 8974 in a compatible string.

> +
> +smd {
> +   compatible = "qcom,smd";
> +
> +   pronto {
> +   interrupts = <0 142 1>;
> +
> +   qcom,ipc = < 8 17>;
> +   qcom,smd-edge = <6>;
> +
> +   wcnss {
> +   compatible = "qcom,wcnss";
> +   qcom,smd-channels = "WCNSS_CTRL";
> +
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   qcom,wcnss = <>;

Not documented and poorly named. The property name should reflect what
this is for or points too.

> +
> +   bt {
> +   compatible = "qcom,wcnss-bt";
> +   };
> +
> +   wlan {
> +   compatible = "qcom,wcnss-wlan";
> +
> +   interrupts = <0 145 0>, <0 146 0>;
> +   interrupt-names = "tx", "rx";
> +
> +   qcom,state = <_smsm 10>, <_smsm 9>;
> +   qcom,state-names = "tx-enable", 
> "tx-rings-empty";
> +   };
> +   };
> +   };
> +};
> +
> +soc {
> +   pronto: pronto {

2 pronto nodes? That's confusing...

> +   compatible = "qcom,pronto";
> +
> +   reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 
> 0x3000>;
> +   reg-names = "ccu", "dxe", "pmu";
> +   };
> +};
> --
> 2.5.0
>

RE: [PATCH] ACPICA / hardware: Fix address check in acpi_hw_get_access_bit_width()

[Zheng, Lv]

Hi, Boris

Can you help to confirm if this can fix all of the issues you saw in the older 
qemu.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Subject: [PATCH] ACPICA / hardware: Fix address check in
> acpi_hw_get_access_bit_width()
> 
> The address check in acpi_hw_get_access_bit_width() should be byte size
> based, not bit width based. This patch fixes this mistake.
> 
> Reported-by: Boris Ostrovsky 
> Suggested-by: Jan Beulich 
> Signed-off-by: Lv Zheng 
> ---
>  drivers/acpi/acpica/hwregs.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index 0f18dbc..74a591b 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -95,7 +95,7 @@ acpi_hw_get_access_bit_width(struct
> acpi_generic_address *reg, u8 max_bit_width)
>   if (!reg->bit_offset && reg->bit_width &&
>   ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
>   ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> - ACPI_IS_ALIGNED(address, reg->bit_width)) {
> + ACPI_IS_ALIGNED(address, reg->bit_width >> 3)) {
>   return (reg->bit_width);
>   } else {
>   if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> {
> --
> 1.7.10

RE: [PATCH] ACPICA / hardware: Fix address check in acpi_hw_get_access_bit_width()

[Zheng, Lv]

Hi, Boris

Can you help to confirm if this can fix all of the issues you saw in the older 
qemu.

Thanks and best regards
-Lv

> From: Zheng, Lv
> Subject: [PATCH] ACPICA / hardware: Fix address check in
> acpi_hw_get_access_bit_width()
> 
> The address check in acpi_hw_get_access_bit_width() should be byte size
> based, not bit width based. This patch fixes this mistake.
> 
> Reported-by: Boris Ostrovsky 
> Suggested-by: Jan Beulich 
> Signed-off-by: Lv Zheng 
> ---
>  drivers/acpi/acpica/hwregs.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index 0f18dbc..74a591b 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -95,7 +95,7 @@ acpi_hw_get_access_bit_width(struct
> acpi_generic_address *reg, u8 max_bit_width)
>   if (!reg->bit_offset && reg->bit_width &&
>   ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
>   ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> - ACPI_IS_ALIGNED(address, reg->bit_width)) {
> + ACPI_IS_ALIGNED(address, reg->bit_width >> 3)) {
>   return (reg->bit_width);
>   } else {
>   if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO)
> {
> --
> 1.7.10

Re: [PATCH] perf script: Fix display inconsitency when call-graph config is used

[Hekuang]

ping..

在 2016/5/16 12:51, He Kuang 写道:

There's a display inconsistency when 'call-graph' config event appears
in different position. The problem can be reproduced like this:

We record signal_deliver with call-graph and signal_generate without it.

   $ perf record -g -a -e signal:signal_deliver -e 
signal:signal_generate/call-graph=no/

   [ perf record: Captured and wrote 0.017 MB perf.data (2 samples) ]

   $ perf script

   kworker/u2:113 [000]  6563.875949: signal:signal_generate: sig=2 errno=0 
code=128 comm=perf pid=1313 grp=1 res=0 ff61cc __send_signal+0x3ec 
([kernel.kallsyms])
   perf  1313 [000]  6563.877584:  signal:signal_deliver: sig=2 errno=0 
code=128 sa_handler=43115e sa_flags=1400
   7314 get_signal+0x80007f0023a4 ([kernel.kallsyms])
   7fffe358 do_signal+0x80007f002028 ([kernel.kallsyms])
   7fffa5e8 exit_to_usermode_loop+0x80007f002053 ([kernel.kallsyms])
   ...

Then we exchange the order of these two events in commandline, and keep
signal_generate without call-graph.

   $ perf record -g -a -e signal:signal_generate/call-graph=no/ -e 
signal:signal_deliver

   [ perf record: Captured and wrote 0.017 MB perf.data (2 samples) ]

   $ perf script

 kworker/u2:2  1314 [000]  6933.353060: signal:signal_generate: sig=2 
errno=0 code=128 comm=perf pid=1321 grp=1 res=0
 perf  1321 [000]  6933.353872:  signal:signal_deliver: sig=2 
errno=0 code=128 sa_handler=43115e sa_flags=1400

This time, the callchain of the event signal_deliver disappeared. The
problem is caused by that perf only checks for the first evsel in evlist
and decides if callchain should be printed.

This patch travseres all evsels in evlist to see if any of them have
callchains, and shows the right result:

   $ perf script

   kworker/u2:2  1314 [000]  6933.353060: signal:signal_generate: sig=2 errno=0 
code=128 comm=perf pid=1321 grp=1 res=0 ff61cc __send_signal+0x3ec 
([kernel.kallsyms])
   perf  1321 [000]  6933.353872:  signal:signal_deliver: sig=2 errno=0 
code=128 sa_handler=43115e sa_flags=1400
   7314 get_signal+0x80007f0023a4 ([kernel.kallsyms])
   7fffe358 do_signal+0x80007f002028 ([kernel.kallsyms])
   7fffa5e8 exit_to_usermode_loop+0x80007f002053 ([kernel.kallsyms])
   ...

Signed-off-by: He Kuang 
---
  tools/perf/builtin-script.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index efca816..7a18b92 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -339,7 +339,7 @@ static void set_print_ip_opts(struct perf_event_attr *attr)
   */
  static int perf_session__check_output_opt(struct perf_session *session)
  {
-   int j;
+   unsigned int j;
struct perf_evsel *evsel;
  
  	for (j = 0; j < PERF_TYPE_MAX; ++j) {

@@ -388,17 +388,20 @@ static int perf_session__check_output_opt(struct 
perf_session *session)
struct perf_event_attr *attr;
  
  		j = PERF_TYPE_TRACEPOINT;

-   evsel = perf_session__find_first_evtype(session, j);
-   if (evsel == NULL)
-   goto out;
  
-		attr = >attr;

+   evlist__for_each(session->evlist, evsel) {
+   if (evsel->attr.type != j)
+   continue;
+
+   attr = >attr;
  
-		if (attr->sample_type & PERF_SAMPLE_CALLCHAIN) {

-   output[j].fields |= PERF_OUTPUT_IP;
-   output[j].fields |= PERF_OUTPUT_SYM;
-   output[j].fields |= PERF_OUTPUT_DSO;
-   set_print_ip_opts(attr);
+   if (attr->sample_type & PERF_SAMPLE_CALLCHAIN) {
+   output[j].fields |= PERF_OUTPUT_IP;
+   output[j].fields |= PERF_OUTPUT_SYM;
+   output[j].fields |= PERF_OUTPUT_DSO;
+   set_print_ip_opts(attr);
+   goto out;
+   }
}
}
  

Re: [PATCH] perf script: Fix display inconsitency when call-graph config is used

[Hekuang]

ping..

在 2016/5/16 12:51, He Kuang 写道:

There's a display inconsistency when 'call-graph' config event appears
in different position. The problem can be reproduced like this:

We record signal_deliver with call-graph and signal_generate without it.

   $ perf record -g -a -e signal:signal_deliver -e 
signal:signal_generate/call-graph=no/

   [ perf record: Captured and wrote 0.017 MB perf.data (2 samples) ]

   $ perf script

   kworker/u2:113 [000]  6563.875949: signal:signal_generate: sig=2 errno=0 
code=128 comm=perf pid=1313 grp=1 res=0 ff61cc __send_signal+0x3ec 
([kernel.kallsyms])
   perf  1313 [000]  6563.877584:  signal:signal_deliver: sig=2 errno=0 
code=128 sa_handler=43115e sa_flags=1400
   7314 get_signal+0x80007f0023a4 ([kernel.kallsyms])
   7fffe358 do_signal+0x80007f002028 ([kernel.kallsyms])
   7fffa5e8 exit_to_usermode_loop+0x80007f002053 ([kernel.kallsyms])
   ...

Then we exchange the order of these two events in commandline, and keep
signal_generate without call-graph.

   $ perf record -g -a -e signal:signal_generate/call-graph=no/ -e 
signal:signal_deliver

   [ perf record: Captured and wrote 0.017 MB perf.data (2 samples) ]

   $ perf script

 kworker/u2:2  1314 [000]  6933.353060: signal:signal_generate: sig=2 
errno=0 code=128 comm=perf pid=1321 grp=1 res=0
 perf  1321 [000]  6933.353872:  signal:signal_deliver: sig=2 
errno=0 code=128 sa_handler=43115e sa_flags=1400

This time, the callchain of the event signal_deliver disappeared. The
problem is caused by that perf only checks for the first evsel in evlist
and decides if callchain should be printed.

This patch travseres all evsels in evlist to see if any of them have
callchains, and shows the right result:

   $ perf script

   kworker/u2:2  1314 [000]  6933.353060: signal:signal_generate: sig=2 errno=0 
code=128 comm=perf pid=1321 grp=1 res=0 ff61cc __send_signal+0x3ec 
([kernel.kallsyms])
   perf  1321 [000]  6933.353872:  signal:signal_deliver: sig=2 errno=0 
code=128 sa_handler=43115e sa_flags=1400
   7314 get_signal+0x80007f0023a4 ([kernel.kallsyms])
   7fffe358 do_signal+0x80007f002028 ([kernel.kallsyms])
   7fffa5e8 exit_to_usermode_loop+0x80007f002053 ([kernel.kallsyms])
   ...

Signed-off-by: He Kuang 
---
  tools/perf/builtin-script.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index efca816..7a18b92 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -339,7 +339,7 @@ static void set_print_ip_opts(struct perf_event_attr *attr)
   */
  static int perf_session__check_output_opt(struct perf_session *session)
  {
-   int j;
+   unsigned int j;
struct perf_evsel *evsel;
  
  	for (j = 0; j < PERF_TYPE_MAX; ++j) {

@@ -388,17 +388,20 @@ static int perf_session__check_output_opt(struct 
perf_session *session)
struct perf_event_attr *attr;
  
  		j = PERF_TYPE_TRACEPOINT;

-   evsel = perf_session__find_first_evtype(session, j);
-   if (evsel == NULL)
-   goto out;
  
-		attr = >attr;

+   evlist__for_each(session->evlist, evsel) {
+   if (evsel->attr.type != j)
+   continue;
+
+   attr = >attr;
  
-		if (attr->sample_type & PERF_SAMPLE_CALLCHAIN) {

-   output[j].fields |= PERF_OUTPUT_IP;
-   output[j].fields |= PERF_OUTPUT_SYM;
-   output[j].fields |= PERF_OUTPUT_DSO;
-   set_print_ip_opts(attr);
+   if (attr->sample_type & PERF_SAMPLE_CALLCHAIN) {
+   output[j].fields |= PERF_OUTPUT_IP;
+   output[j].fields |= PERF_OUTPUT_SYM;
+   output[j].fields |= PERF_OUTPUT_DSO;
+   set_print_ip_opts(attr);
+   goto out;
+   }
}
}
  

Re: [PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Peter Chen]

On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
> Hello,
> 
> I was updating my config by make oldconfig for a while and noticed that my USB
> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
> over time.
> 
> Looking through defconfigs some have it included and some which seem in need 
> of
> it don't.
> 
> Since the dependency is not obvious I think it would be better to select it
> where possible.

>From Documentation/kbuild/kconfig-language.txt
In general use select only for non-visible symbols
(no prompts anywhere) and for symbols with no dependencies.

But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
dependency problem in future, so unless it is necessary, use it
as least as possible.

If you are using new code, and it adds new dependency code, it is
reasonable you may need to update your defconfig.

Peter
> 
> Attaching a patch.
> 
> Thanks
> 
> Michal
> 
> 8<--
> NOP_USB_XCEIV is non-obvious dependency for MUSB and other drivers.
> 
> This is a virtual driver in the sense that there is no actual piece of
> hardware you can point at and say you did not include driver for this so
> it won't work.
> 
> So just change all depends on it to select.
> 
> Signed-off-by: Michal Suchanek 
> ---
>  drivers/usb/chipidea/Kconfig |  3 ++-
>  drivers/usb/musb/Kconfig | 19 +--
>  drivers/usb/phy/Kconfig  |  2 ++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 3644a35..8d08ebd 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -19,7 +19,8 @@ config USB_CHIPIDEA_OF
>  config USB_CHIPIDEA_PCI
>   tristate
>   depends on PCI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   default USB_CHIPIDEA
>  
>  config USB_CHIPIDEA_UDC
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 886526b..91717b9 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -66,7 +66,8 @@ comment "Platform Glue Layer"
>  config USB_MUSB_SUNXI
>   tristate "Allwinner (sunxi)"
>   depends on ARCH_SUNXI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on PHY_SUN4I_USB
>   depends on EXTCON
>   depends on GENERIC_PHY
> @@ -75,13 +76,15 @@ config USB_MUSB_SUNXI
>  config USB_MUSB_DAVINCI
>   tristate "DaVinci"
>   depends on ARCH_DAVINCI_DMx
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_DA8XX
>   tristate "DA8xx/OMAP-L1x"
>   depends on ARCH_DAVINCI_DA8XX
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_TUSB6010
> @@ -89,6 +92,7 @@ config USB_MUSB_TUSB6010
>   depends on HAS_IOMEM
>   depends on ARCH_OMAP2PLUS || COMPILE_TEST
>   depends on NOP_USB_XCEIV = USB_MUSB_HDRC # both built-in or both modules
> + # cannot select NOP_USB_XCEIV because of the dependency above
>  
>  config USB_MUSB_OMAP2PLUS
>   tristate "OMAP2430 and onwards"
> @@ -99,7 +103,8 @@ config USB_MUSB_OMAP2PLUS
>  config USB_MUSB_AM35X
>   tristate "AM35x"
>   depends on ARCH_OMAP
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_DSPS
>   tristate "TI DSPS platforms"
> @@ -110,7 +115,8 @@ config USB_MUSB_DSPS
>  config USB_MUSB_BLACKFIN
>   tristate "Blackfin"
>   depends on (BF54x && !BF544) || (BF52x && ! BF522 && !BF523)
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_UX500
>   tristate "Ux500 platforms"
> @@ -118,7 +124,8 @@ config USB_MUSB_UX500
>  
>  config USB_MUSB_JZ4740
>   tristate "JZ4740"
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on MACH_JZ4740 || COMPILE_TEST
>   depends on USB_MUSB_GADGET
>   depends on USB_OTG_BLACKLIST_HUB
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index c690474..a0bdfd3 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -57,6 +57,8 @@ config NOP_USB_XCEIV
> built-in with usb ip or which are autonomous and doesn't require any
> phy programming such as ISP1x04 etc.
>  
> +   Should be automatically selected by the relevant driver.
> +
>  config AM335X_CONTROL_USB
>   tristate
>  
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter 

Re: [PATCH] usb: select NOP_USB_XCEIV by drivers that require it

[Peter Chen]

On Thu, May 26, 2016 at 07:25:23PM -, Michal Suchanek wrote:
> Hello,
> 
> I was updating my config by make oldconfig for a while and noticed that my USB
> OTG controller is not working. Apparently it grew dependency on NOP_USB_XCEIV
> over time.
> 
> Looking through defconfigs some have it included and some which seem in need 
> of
> it don't.
> 
> Since the dependency is not obvious I think it would be better to select it
> where possible.

>From Documentation/kbuild/kconfig-language.txt
In general use select only for non-visible symbols
(no prompts anywhere) and for symbols with no dependencies.

But NOP_USB_XCEIV is a visible symbol and can be chosen, besides,
NOP_USB_XCEIV has already selected USB_PHY. Using select may cause
dependency problem in future, so unless it is necessary, use it
as least as possible.

If you are using new code, and it adds new dependency code, it is
reasonable you may need to update your defconfig.

Peter
> 
> Attaching a patch.
> 
> Thanks
> 
> Michal
> 
> 8<--
> NOP_USB_XCEIV is non-obvious dependency for MUSB and other drivers.
> 
> This is a virtual driver in the sense that there is no actual piece of
> hardware you can point at and say you did not include driver for this so
> it won't work.
> 
> So just change all depends on it to select.
> 
> Signed-off-by: Michal Suchanek 
> ---
>  drivers/usb/chipidea/Kconfig |  3 ++-
>  drivers/usb/musb/Kconfig | 19 +--
>  drivers/usb/phy/Kconfig  |  2 ++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 3644a35..8d08ebd 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -19,7 +19,8 @@ config USB_CHIPIDEA_OF
>  config USB_CHIPIDEA_PCI
>   tristate
>   depends on PCI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   default USB_CHIPIDEA
>  
>  config USB_CHIPIDEA_UDC
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 886526b..91717b9 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -66,7 +66,8 @@ comment "Platform Glue Layer"
>  config USB_MUSB_SUNXI
>   tristate "Allwinner (sunxi)"
>   depends on ARCH_SUNXI
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on PHY_SUN4I_USB
>   depends on EXTCON
>   depends on GENERIC_PHY
> @@ -75,13 +76,15 @@ config USB_MUSB_SUNXI
>  config USB_MUSB_DAVINCI
>   tristate "DaVinci"
>   depends on ARCH_DAVINCI_DMx
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_DA8XX
>   tristate "DA8xx/OMAP-L1x"
>   depends on ARCH_DAVINCI_DA8XX
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on BROKEN
>  
>  config USB_MUSB_TUSB6010
> @@ -89,6 +92,7 @@ config USB_MUSB_TUSB6010
>   depends on HAS_IOMEM
>   depends on ARCH_OMAP2PLUS || COMPILE_TEST
>   depends on NOP_USB_XCEIV = USB_MUSB_HDRC # both built-in or both modules
> + # cannot select NOP_USB_XCEIV because of the dependency above
>  
>  config USB_MUSB_OMAP2PLUS
>   tristate "OMAP2430 and onwards"
> @@ -99,7 +103,8 @@ config USB_MUSB_OMAP2PLUS
>  config USB_MUSB_AM35X
>   tristate "AM35x"
>   depends on ARCH_OMAP
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_DSPS
>   tristate "TI DSPS platforms"
> @@ -110,7 +115,8 @@ config USB_MUSB_DSPS
>  config USB_MUSB_BLACKFIN
>   tristate "Blackfin"
>   depends on (BF54x && !BF544) || (BF52x && ! BF522 && !BF523)
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>  
>  config USB_MUSB_UX500
>   tristate "Ux500 platforms"
> @@ -118,7 +124,8 @@ config USB_MUSB_UX500
>  
>  config USB_MUSB_JZ4740
>   tristate "JZ4740"
> - depends on NOP_USB_XCEIV
> + select NOP_USB_XCEIV
> + select USB_PHY
>   depends on MACH_JZ4740 || COMPILE_TEST
>   depends on USB_MUSB_GADGET
>   depends on USB_OTG_BLACKLIST_HUB
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index c690474..a0bdfd3 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -57,6 +57,8 @@ config NOP_USB_XCEIV
> built-in with usb ip or which are autonomous and doesn't require any
> phy programming such as ISP1x04 etc.
>  
> +   Should be automatically selected by the relevant driver.
> +
>  config AM335X_CONTROL_USB
>   tristate
>  
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Jaehoon Chung]

Hi Shawn,

On 05/27/2016 12:02 PM, Shawn Lin wrote:
> The main reason to add this check is to avoid unnecessary
> mmc_request like the on-going cmd and the corresponding sbc
> if the card is removed. Although we have already checked this in
> dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
> for noremovable devices, but there is a timing gap before it really
> calls dw_mci_get_cd as mmc_detect_change needs some delay here.
> 
> Another gain here is that we could save some checkings of card status
> after sd card been removed.
> 
> Signed-off-by: Shawn Lin 
> ---
> 
>  drivers/mmc/host/dw_mmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index cb30e91..8eb7898 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -105,6 +105,7 @@ struct idmac_desc {
>  static bool dw_mci_reset(struct dw_mci *host);
>  static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>  static int dw_mci_card_busy(struct mmc_host *mmc);
> +static int dw_mci_get_cd(struct mmc_host *mmc);
>  
>  #if defined(CONFIG_DEBUG_FS)
>  static int dw_mci_req_show(struct seq_file *s, void *v)
> @@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
> mmc_request *mrq)
>*/
>   spin_lock_bh(>lock);
>  
> - if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
> + if (!dw_mci_get_cd(mmc)) {
>   spin_unlock_bh(>lock);
>   mrq->cmd->error = -ENOMEDIUM;
>   mmc_request_done(mmc, mrq);

I think spin_lock/unlock_bh() should be removed at here.
Instead, Can it locate before calling dw_mci_queue_request()?

Anyway, it looks good to me.

Best Regards,
Jaehoon Chung

> 

Re: [PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Jaehoon Chung]

Hi Shawn,

On 05/27/2016 12:02 PM, Shawn Lin wrote:
> The main reason to add this check is to avoid unnecessary
> mmc_request like the on-going cmd and the corresponding sbc
> if the card is removed. Although we have already checked this in
> dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
> for noremovable devices, but there is a timing gap before it really
> calls dw_mci_get_cd as mmc_detect_change needs some delay here.
> 
> Another gain here is that we could save some checkings of card status
> after sd card been removed.
> 
> Signed-off-by: Shawn Lin 
> ---
> 
>  drivers/mmc/host/dw_mmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index cb30e91..8eb7898 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -105,6 +105,7 @@ struct idmac_desc {
>  static bool dw_mci_reset(struct dw_mci *host);
>  static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
>  static int dw_mci_card_busy(struct mmc_host *mmc);
> +static int dw_mci_get_cd(struct mmc_host *mmc);
>  
>  #if defined(CONFIG_DEBUG_FS)
>  static int dw_mci_req_show(struct seq_file *s, void *v)
> @@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
> mmc_request *mrq)
>*/
>   spin_lock_bh(>lock);
>  
> - if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
> + if (!dw_mci_get_cd(mmc)) {
>   spin_unlock_bh(>lock);
>   mrq->cmd->error = -ENOMEDIUM;
>   mmc_request_done(mmc, mrq);

I think spin_lock/unlock_bh() should be removed at here.
Instead, Can it locate before calling dw_mci_queue_request()?

Anyway, it looks good to me.

Best Regards,
Jaehoon Chung

> 

Re: [PATCH 2/2] sched/fair: Skip detach and attach load avgs for new group task

[Yuyang Du]

Hi Vincent,

On Thu, May 26, 2016 at 01:50:56PM +0200, Vincent Guittot wrote:
> On 26 May 2016 at 03:14, Yuyang Du  wrote:
> > Vincent reported that the first task to a new task group's cfs_rq will
> > be attached in attach_task_cfs_rq() and once more when it is enqueued
> > (see https://lkml.org/lkml/2016/5/25/388).
> >
> > Actually, it is worse, attach_task_cfs_rq() is called for new task even
> > way before init_entity_runnable_average().
> >
> > Solve this by avoiding attach as well as detach new task's sched avgs
> > in task_move_group_fair(). To do it, we need to know whether the task
> > is forked or not, so we pass this info all the way from sched_move_task()
> > to attach_task_cfs_rq().
> 
> Not sure that this is the right way to solve this problem because you
> continue to attach the task twice without detaching it in the mean
> time:
> - once during the copy of the process in cpu_cgroup_fork (you skip the
> attach of load average but the task is still attached to the local
> cpu)

Sorry, the task's what is still attached, and how? You mean the vruntime
thingy? But the load/util avgs are not.

> In the mean time, sched_entity is initialized and the last_update_time is 
> reset

last_update_time is set to 0 in initialization, and this is the first time
it is touched, no?

> - one more time when the task is enqueued because the last_update_time
> has been reset (this time you don't skip the attache of load_avg

This is expected/wanted. We don't skip this because this will be the first-time
attach.

> Should you better detach the sched_entity with a copy of its parent
> metrics before initializing it and attaching it to the new cpu ?

Thanks,
Yuyang

Re: [PATCH 2/2] sched/fair: Skip detach and attach load avgs for new group task

[Yuyang Du]

Hi Vincent,

On Thu, May 26, 2016 at 01:50:56PM +0200, Vincent Guittot wrote:
> On 26 May 2016 at 03:14, Yuyang Du  wrote:
> > Vincent reported that the first task to a new task group's cfs_rq will
> > be attached in attach_task_cfs_rq() and once more when it is enqueued
> > (see https://lkml.org/lkml/2016/5/25/388).
> >
> > Actually, it is worse, attach_task_cfs_rq() is called for new task even
> > way before init_entity_runnable_average().
> >
> > Solve this by avoiding attach as well as detach new task's sched avgs
> > in task_move_group_fair(). To do it, we need to know whether the task
> > is forked or not, so we pass this info all the way from sched_move_task()
> > to attach_task_cfs_rq().
> 
> Not sure that this is the right way to solve this problem because you
> continue to attach the task twice without detaching it in the mean
> time:
> - once during the copy of the process in cpu_cgroup_fork (you skip the
> attach of load average but the task is still attached to the local
> cpu)

Sorry, the task's what is still attached, and how? You mean the vruntime
thingy? But the load/util avgs are not.

> In the mean time, sched_entity is initialized and the last_update_time is 
> reset

last_update_time is set to 0 in initialization, and this is the first time
it is touched, no?

> - one more time when the task is enqueued because the last_update_time
> has been reset (this time you don't skip the attache of load_avg

This is expected/wanted. We don't skip this because this will be the first-time
attach.

> Should you better detach the sched_entity with a copy of its parent
> metrics before initializing it and attaching it to the new cpu ?

Thanks,
Yuyang

[PATCH] ACPICA / hardware: Fix address check in acpi_hw_get_access_bit_width()

[Lv Zheng]

The address check in acpi_hw_get_access_bit_width() should be byte size
based, not bit width based. This patch fixes this mistake.

Reported-by: Boris Ostrovsky 
Suggested-by: Jan Beulich 
Signed-off-by: Lv Zheng 
---
 drivers/acpi/acpica/hwregs.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
index 0f18dbc..74a591b 100644
--- a/drivers/acpi/acpica/hwregs.c
+++ b/drivers/acpi/acpica/hwregs.c
@@ -95,7 +95,7 @@ acpi_hw_get_access_bit_width(struct acpi_generic_address 
*reg, u8 max_bit_width)
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8) &&
-   ACPI_IS_ALIGNED(address, reg->bit_width)) {
+   ACPI_IS_ALIGNED(address, reg->bit_width >> 3)) {
return (reg->bit_width);
} else {
if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-- 
1.7.10

[PATCH] ACPICA / hardware: Fix address check in acpi_hw_get_access_bit_width()

[Lv Zheng]

The address check in acpi_hw_get_access_bit_width() should be byte size
based, not bit width based. This patch fixes this mistake.

Reported-by: Boris Ostrovsky 
Suggested-by: Jan Beulich 
Signed-off-by: Lv Zheng 
---
 drivers/acpi/acpica/hwregs.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
index 0f18dbc..74a591b 100644
--- a/drivers/acpi/acpica/hwregs.c
+++ b/drivers/acpi/acpica/hwregs.c
@@ -95,7 +95,7 @@ acpi_hw_get_access_bit_width(struct acpi_generic_address 
*reg, u8 max_bit_width)
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8) &&
-   ACPI_IS_ALIGNED(address, reg->bit_width)) {
+   ACPI_IS_ALIGNED(address, reg->bit_width >> 3)) {
return (reg->bit_width);
} else {
if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-- 
1.7.10

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

[Leizhen (ThunderTown)]

On 2016/5/26 21:13, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>> For a normal memory@ devicetree node, its reg property can contains more
>> memory blocks.
>>
>> Because we don't known how many memory blocks maybe contained, so we try
>> from index=0, increase 1 until error returned(the end).
>>
>> Signed-off-by: Zhen Lei 
>> ---
>>  drivers/of/of_numa.c | 30 --
>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
>> index 21d831f..2c5f249 100644
>> --- a/drivers/of/of_numa.c
>> +++ b/drivers/of/of_numa.c
>> @@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
>>  struct device_node *np = NULL;
>>  struct resource rsrc;
>>  u32 nid;
>> -int r = 0;
>> +int i, r = 0;
>>
>>  for (;;) {
>>  np = of_find_node_by_type(np, "memory");
>> @@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
>>  /* some other error */
>>  break;
>>
>> -r = of_address_to_resource(np, 0, );
>> -if (r) {
>> -pr_err("NUMA: bad reg property in memory node\n");
>> -break;
>> +for (i = 0; ; i++) {
>> +r = of_address_to_resource(np, i, );
>> +if (r) {
>> +/* reached the end of of_address */
>> +if (i > 0) {
>> +r = 0;
>> +break;
>> +}
>> +
>> +pr_err("NUMA: bad reg property in memory 
>> node\n");
>> +goto finished;
>> +}
>> +
>> +r = numa_add_memblk(nid, rsrc.start,
>> +rsrc.end - rsrc.start + 1);
>> +if (r)
>> +goto finished;
>>  }
>> -
>> -r = numa_add_memblk(nid, rsrc.start,
>> -rsrc.end - rsrc.start + 1);
>> -if (r)
>> -break;
>>  }
>> +
>> +finished:
>>  of_node_put(np);
> 
> This function can be simplified down to:
> 
>   for_each_node_by_type(np, "memory") {
OK, That's good.

>   r = of_property_read_u32(np, "numa-node-id", );
>   if (r == -EINVAL)
>   /*
>* property doesn't exist if -EINVAL, continue
>* looking for more memory nodes with
>* "numa-node-id" property
>*/
>   continue;
Hi, everybody:
If some "memory" node contains "numa-node-id", but some others missed. Can 
we simply ignored it?
I think we should break out too, and faking to only have node0.

>   else if (r)
>   /* some other error */
>   break;
> 
>   r = of_address_to_resource(np, 0, );
>   for (i = 0; !r; i++, r = of_address_to_resource(np, i, 

But r(non-zero) is just break this loop, the original is break the outer for 
(;;) loop

How about as below?

for_each_node_by_type(np, "memory") {
... ...

for (i = 0; !of_address_to_resource(np, i, ); i++) {
r = numa_add_memblk(nid, rsrc.start,
rsrc.end - rsrc.start + 1);
if (r)
goto finished;
}

if (!i)
pr_err("NUMA: bad reg property in memory node\n");
}

finished:


> )) {
>   r = numa_add_memblk(nid, rsrc.start,
>   rsrc.end - rsrc.start + 1);
>   }
>   }
>   of_node_put(np);
> 
>   return r;
> 
> 
> Perhaps with a "if (!i && r) pr_err()" for an error message at the end.
> 
> Rob
> 
> .
> 

Re: [PATCH 2/3] of/numa: fix a memory@ dt node can only contains one memory block

[Leizhen (ThunderTown)]

On 2016/5/26 21:13, Rob Herring wrote:
> On Thu, May 26, 2016 at 10:43:58AM +0800, Zhen Lei wrote:
>> For a normal memory@ devicetree node, its reg property can contains more
>> memory blocks.
>>
>> Because we don't known how many memory blocks maybe contained, so we try
>> from index=0, increase 1 until error returned(the end).
>>
>> Signed-off-by: Zhen Lei 
>> ---
>>  drivers/of/of_numa.c | 30 --
>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
>> index 21d831f..2c5f249 100644
>> --- a/drivers/of/of_numa.c
>> +++ b/drivers/of/of_numa.c
>> @@ -63,7 +63,7 @@ static int __init of_numa_parse_memory_nodes(void)
>>  struct device_node *np = NULL;
>>  struct resource rsrc;
>>  u32 nid;
>> -int r = 0;
>> +int i, r = 0;
>>
>>  for (;;) {
>>  np = of_find_node_by_type(np, "memory");
>> @@ -82,17 +82,27 @@ static int __init of_numa_parse_memory_nodes(void)
>>  /* some other error */
>>  break;
>>
>> -r = of_address_to_resource(np, 0, );
>> -if (r) {
>> -pr_err("NUMA: bad reg property in memory node\n");
>> -break;
>> +for (i = 0; ; i++) {
>> +r = of_address_to_resource(np, i, );
>> +if (r) {
>> +/* reached the end of of_address */
>> +if (i > 0) {
>> +r = 0;
>> +break;
>> +}
>> +
>> +pr_err("NUMA: bad reg property in memory 
>> node\n");
>> +goto finished;
>> +}
>> +
>> +r = numa_add_memblk(nid, rsrc.start,
>> +rsrc.end - rsrc.start + 1);
>> +if (r)
>> +goto finished;
>>  }
>> -
>> -r = numa_add_memblk(nid, rsrc.start,
>> -rsrc.end - rsrc.start + 1);
>> -if (r)
>> -break;
>>  }
>> +
>> +finished:
>>  of_node_put(np);
> 
> This function can be simplified down to:
> 
>   for_each_node_by_type(np, "memory") {
OK, That's good.

>   r = of_property_read_u32(np, "numa-node-id", );
>   if (r == -EINVAL)
>   /*
>* property doesn't exist if -EINVAL, continue
>* looking for more memory nodes with
>* "numa-node-id" property
>*/
>   continue;
Hi, everybody:
If some "memory" node contains "numa-node-id", but some others missed. Can 
we simply ignored it?
I think we should break out too, and faking to only have node0.

>   else if (r)
>   /* some other error */
>   break;
> 
>   r = of_address_to_resource(np, 0, );
>   for (i = 0; !r; i++, r = of_address_to_resource(np, i, 

But r(non-zero) is just break this loop, the original is break the outer for 
(;;) loop

How about as below?

for_each_node_by_type(np, "memory") {
... ...

for (i = 0; !of_address_to_resource(np, i, ); i++) {
r = numa_add_memblk(nid, rsrc.start,
rsrc.end - rsrc.start + 1);
if (r)
goto finished;
}

if (!i)
pr_err("NUMA: bad reg property in memory node\n");
}

finished:


> )) {
>   r = numa_add_memblk(nid, rsrc.start,
>   rsrc.end - rsrc.start + 1);
>   }
>   }
>   of_node_put(np);
> 
>   return r;
> 
> 
> Perhaps with a "if (!i && r) pr_err()" for an error message at the end.
> 
> Rob
> 
> .
> 

Re: [RFC 4/5] ARM: tegra: Enable UDC on Dalmore

[Peter Chen]

On Thu, May 26, 2016 at 05:40:04PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Override the compatible string of the first USB controller to enable
> device mode.
> 
> Signed-off-by: Thierry Reding 
> ---
>  arch/arm/boot/dts/tegra114-dalmore.dts | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts 
> b/arch/arm/boot/dts/tegra114-dalmore.dts
> index c970bf65c74c..53d664f718ff 100644
> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
> @@ -1122,6 +1122,17 @@
>   non-removable;
>   };
>  
> + usb@7d00 {
> + compatible = "nvidia,tegra114-udc";
> + status = "okay";
> + dr_mode = "otg";
> + };
> +
> + usb-phy@7d00 {
> + status = "okay";
> + dr_mode = "otg";
> + };
> +

It is a USB PHY node, you don't need to set dr_mode for it.

-- 

Best Regards,
Peter Chen

linux-next: Tree for May 27

[Stephen Rothwell]

Hi all,

Please do not add any v4.8 destined material to your linux-next included
branches until after v4.7-rc1 has been released.

Changes since 20160526:

My fixes tree contains this:
mm/cma: silence warnings due to max() usage

Non-merge commits (relative to Linus' tree): 883
 837 files changed, 28447 insertions(+), 10538 deletions(-)



I have created today's linux-next tree at
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(patches at http://www.kernel.org/pub/linux/kernel/next/ ).  If you
are tracking the linux-next tree using git, you should not use "git pull"
to do so as that will try to merge the new linux-next release with the
old one.  You should use "git fetch" and checkout or reset to the new
master.

You can see which trees have been included by looking in the Next/Trees
file in the source.  There are also quilt-import.log and merge.log
files in the Next directory.  Between each merge, the tree was built
with a ppc64_defconfig for powerpc and an allmodconfig (with
CONFIG_BUILD_DOCSRC=n) for x86_64, a multi_v7_defconfig for arm and a
native build of tools/perf. After the final fixups (if any), I do an
x86_64 modules_install followed by builds for x86_64 allnoconfig,
powerpc allnoconfig (32 and 64 bit), ppc44x_defconfig, allyesconfig
(this fails its final link) and pseries_le_defconfig and i386, sparc
and sparc64 defconfig.

Below is a summary of the state of the merge.

I am currently merging 236 trees (counting Linus' and 35 trees of patches
pending for Linus' tree).

Stats about the size of the tree over time can be seen at
http://neuling.org/linux-next-size.html .

Status of my local build tests will be at
http://kisskb.ellerman.id.au/linux-next .  If maintainers want to give
advice about cross compilers/configs that work, we are always open to add
more builds.

Thanks to Randy Dunlap for doing many randconfig builds.  And to Paul
Gortmaker for triage and bug fixes.

-- 
Cheers,
Stephen Rothwell

$ git checkout master
$ git reset --hard stable
Merging origin/master (a10c38a4f385 Merge branch 'for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/sage/ceph-client)
Merging fixes/master (2f8d08a2e029 mm/cma: silence warnings due to max() usage)
Merging kbuild-current/rc-fixes (3d1450d54a4f Makefile: Force gzip and xz on 
module install)
Merging arc-current/for-curr (44549e8f5eea Linux 4.6-rc7)
Merging arm-current/fixes (ec953b70f368 ARM: 8573/1: domain: move 
{set,get}_domain under config guard)
Merging m68k-current/for-linus (9a6462763b17 m68k/mvme16x: Include generic 
)
Merging metag-fixes/fixes (0164a711c97b metag: Fix ioremap_wc/ioremap_cached 
build errors)
Merging powerpc-fixes/fixes (b4c112114aab powerpc: Fix bad inline asm 
constraint in create_zero_mask())
Merging powerpc-merge-mpe/fixes (bc0195aad0da Linux 4.2-rc2)
Merging sparc/master (9ea46abe2255 sparc64: Take ctx_alloc_lock properly in 
hugetlb_setup().)
Merging net/master (b7e7ad611e24 Merge branch 'qed-fixes')
Merging ipsec/master (d6af1a31cc72 vti: Add pmtu handling to vti_xmit.)
Merging ipvs/master (f28f20da704d Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net)
Merging wireless-drivers/master (cbbba30f1ac9 Merge tag 
'iwlwifi-for-kalle-2016-05-04' of 
https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes)
Merging mac80211/master (e6436be21e77 mac80211: fix statistics leak if 
dev_alloc_name() fails)
Merging sound-current/for-linus (86c72d1ce91d ALSA: hda - Fix headset mic 
detection problem for one Dell machine)
Merging pci-current/for-linus (9a2a5a638f8e PCI: Do not treat EPROBE_DEFER as 
device attach failure)
Merging driver-core.current/driver-core-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging tty.current/tty-linus (5469dc270cd4 Merge branch 'akpm' (patches from 
Andrew))
Merging usb.current/usb-linus (5469dc270cd4 Merge branch 'akpm' (patches from 
Andrew))
Merging usb-gadget-fixes/fixes (38740a5b87d5 usb: gadget: f_fs: Fix 
use-after-free)
Merging usb-serial-fixes/usb-linus (74d2a91aec97 USB: serial: option: add even 
more ZTE device ids)
Merging usb-chipidea-fixes/ci-for-usb-stable (d144dfea8af7 usb: chipidea: otg: 
change workqueue ci_otg as freezable)
Merging staging.current/staging-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging char-misc.current/char-misc-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging input-current/for-linus (affa80bd97f7 Input: uinput - handle compat 
ioctl for UI_SET_PHYS)
Merging crypto-current/master (ab6a11a7c8ef crypto: ccp - Fix AES XTS error for 
request sizes above 4096)
Merging ide/master (1993b176a822 Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide)
Merging devicetree-current/devicetree/merge (f76502aa9140 of/dynamic: Fix test 
for PPC_PSERIES)
Merging rr-fixes/fixes (8244062ef1e5 modules: fix longstanding /proc/kallsyms 
vs module insertion

Re: [RFC 4/5] ARM: tegra: Enable UDC on Dalmore

[Peter Chen]

On Thu, May 26, 2016 at 05:40:04PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Override the compatible string of the first USB controller to enable
> device mode.
> 
> Signed-off-by: Thierry Reding 
> ---
>  arch/arm/boot/dts/tegra114-dalmore.dts | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts 
> b/arch/arm/boot/dts/tegra114-dalmore.dts
> index c970bf65c74c..53d664f718ff 100644
> --- a/arch/arm/boot/dts/tegra114-dalmore.dts
> +++ b/arch/arm/boot/dts/tegra114-dalmore.dts
> @@ -1122,6 +1122,17 @@
>   non-removable;
>   };
>  
> + usb@7d00 {
> + compatible = "nvidia,tegra114-udc";
> + status = "okay";
> + dr_mode = "otg";
> + };
> +
> + usb-phy@7d00 {
> + status = "okay";
> + dr_mode = "otg";
> + };
> +

It is a USB PHY node, you don't need to set dr_mode for it.

-- 

Best Regards,
Peter Chen

linux-next: Tree for May 27

[Stephen Rothwell]

Hi all,

Please do not add any v4.8 destined material to your linux-next included
branches until after v4.7-rc1 has been released.

Changes since 20160526:

My fixes tree contains this:
mm/cma: silence warnings due to max() usage

Non-merge commits (relative to Linus' tree): 883
 837 files changed, 28447 insertions(+), 10538 deletions(-)



I have created today's linux-next tree at
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(patches at http://www.kernel.org/pub/linux/kernel/next/ ).  If you
are tracking the linux-next tree using git, you should not use "git pull"
to do so as that will try to merge the new linux-next release with the
old one.  You should use "git fetch" and checkout or reset to the new
master.

You can see which trees have been included by looking in the Next/Trees
file in the source.  There are also quilt-import.log and merge.log
files in the Next directory.  Between each merge, the tree was built
with a ppc64_defconfig for powerpc and an allmodconfig (with
CONFIG_BUILD_DOCSRC=n) for x86_64, a multi_v7_defconfig for arm and a
native build of tools/perf. After the final fixups (if any), I do an
x86_64 modules_install followed by builds for x86_64 allnoconfig,
powerpc allnoconfig (32 and 64 bit), ppc44x_defconfig, allyesconfig
(this fails its final link) and pseries_le_defconfig and i386, sparc
and sparc64 defconfig.

Below is a summary of the state of the merge.

I am currently merging 236 trees (counting Linus' and 35 trees of patches
pending for Linus' tree).

Stats about the size of the tree over time can be seen at
http://neuling.org/linux-next-size.html .

Status of my local build tests will be at
http://kisskb.ellerman.id.au/linux-next .  If maintainers want to give
advice about cross compilers/configs that work, we are always open to add
more builds.

Thanks to Randy Dunlap for doing many randconfig builds.  And to Paul
Gortmaker for triage and bug fixes.

-- 
Cheers,
Stephen Rothwell

$ git checkout master
$ git reset --hard stable
Merging origin/master (a10c38a4f385 Merge branch 'for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/sage/ceph-client)
Merging fixes/master (2f8d08a2e029 mm/cma: silence warnings due to max() usage)
Merging kbuild-current/rc-fixes (3d1450d54a4f Makefile: Force gzip and xz on 
module install)
Merging arc-current/for-curr (44549e8f5eea Linux 4.6-rc7)
Merging arm-current/fixes (ec953b70f368 ARM: 8573/1: domain: move 
{set,get}_domain under config guard)
Merging m68k-current/for-linus (9a6462763b17 m68k/mvme16x: Include generic 
)
Merging metag-fixes/fixes (0164a711c97b metag: Fix ioremap_wc/ioremap_cached 
build errors)
Merging powerpc-fixes/fixes (b4c112114aab powerpc: Fix bad inline asm 
constraint in create_zero_mask())
Merging powerpc-merge-mpe/fixes (bc0195aad0da Linux 4.2-rc2)
Merging sparc/master (9ea46abe2255 sparc64: Take ctx_alloc_lock properly in 
hugetlb_setup().)
Merging net/master (b7e7ad611e24 Merge branch 'qed-fixes')
Merging ipsec/master (d6af1a31cc72 vti: Add pmtu handling to vti_xmit.)
Merging ipvs/master (f28f20da704d Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net)
Merging wireless-drivers/master (cbbba30f1ac9 Merge tag 
'iwlwifi-for-kalle-2016-05-04' of 
https://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes)
Merging mac80211/master (e6436be21e77 mac80211: fix statistics leak if 
dev_alloc_name() fails)
Merging sound-current/for-linus (86c72d1ce91d ALSA: hda - Fix headset mic 
detection problem for one Dell machine)
Merging pci-current/for-linus (9a2a5a638f8e PCI: Do not treat EPROBE_DEFER as 
device attach failure)
Merging driver-core.current/driver-core-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging tty.current/tty-linus (5469dc270cd4 Merge branch 'akpm' (patches from 
Andrew))
Merging usb.current/usb-linus (5469dc270cd4 Merge branch 'akpm' (patches from 
Andrew))
Merging usb-gadget-fixes/fixes (38740a5b87d5 usb: gadget: f_fs: Fix 
use-after-free)
Merging usb-serial-fixes/usb-linus (74d2a91aec97 USB: serial: option: add even 
more ZTE device ids)
Merging usb-chipidea-fixes/ci-for-usb-stable (d144dfea8af7 usb: chipidea: otg: 
change workqueue ci_otg as freezable)
Merging staging.current/staging-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging char-misc.current/char-misc-linus (5469dc270cd4 Merge branch 'akpm' 
(patches from Andrew))
Merging input-current/for-linus (affa80bd97f7 Input: uinput - handle compat 
ioctl for UI_SET_PHYS)
Merging crypto-current/master (ab6a11a7c8ef crypto: ccp - Fix AES XTS error for 
request sizes above 4096)
Merging ide/master (1993b176a822 Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide)
Merging devicetree-current/devicetree/merge (f76502aa9140 of/dynamic: Fix test 
for PPC_PSERIES)
Merging rr-fixes/fixes (8244062ef1e5 modules: fix longstanding /proc/kallsyms 
vs module insertion

RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support

[Zheng, Lv]

Hi,

> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> Sent: Friday, May 27, 2016 12:56 AM
> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access
> bit width support
> 
> On 05/26/2016 12:26 PM, Jan Beulich wrote:
>  Boris Ostrovsky  05/25/16 9:17
> PM >>>
> >> On 05/05/2016 12:58 AM, Lv Zheng wrote:
> >>> +static u8
> >>> +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg, u8
> max_bit_width)
> >>> +{
> >>> +u64 address;
> >>> +
> >>> +if (!reg->access_width) {
> >>> +/*
> >>> + * Detect old register descriptors where only the bit_width field
> >>> + * makes senses. The target address is copied to handle possible
> >>> + * alignment issues.
> >>> + */
> >>> +ACPI_MOVE_64_TO_64(, >address);
> >>> +if (!reg->bit_offset && reg->bit_width &&
> >>> +ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
> >>> +ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> >>> +ACPI_IS_ALIGNED(address, reg->bit_width)) {
> >>> +return (reg->bit_width);
> >>> +} else {
> >>> +if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> >>> +return (32);
> >> This (together with "... Add access_width/bit_offset support in
> >> acpi_hw_write") breaks Xen guests using older QEMU which doesn't
> support
> >> 4-byte IO accesses.
> >>
> >> Why not return "reg->bit_width?:max_bit_width" ? This will preserve
> >> original behavior.
> > Did you figure out why we get here in the first place, instead of taking
> the
> > first "return"? I.e. isn't the issue the apparently wrong use of the second
> > ACPI_IS_ALIGNED() above? Afaict it ought to be
> > ACPI_IS_ALIGNED(address, reg->bit_width / 8)...
> 
> We are trying to access address 0x...b004 (PM1a control) so yes, fixing
> alignment check would probably resolve the problem that we are seeing
> now.
> 
> However, for compatibility purposes we may consider not doing any
> checks
> and simply return bit_width if access_width is not available.
[Lv Zheng] 

There might be GAS defined with AccessWidth = 0.
And its BitOffset can still make senses.
That's why I put the check here, making it a tuning rather than a regression 
safe check.

Thanks for the information, I'll submit the fix of the address check.
To convert it to:
ACPI_IS_ALIGNED(address, reg->bit_width / 8)
This is my fault.

Thanks
-Lv

> 
> -boris
> 

RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support

[Zheng, Lv]

Hi,

> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> Sent: Friday, May 27, 2016 12:56 AM
> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access
> bit width support
> 
> On 05/26/2016 12:26 PM, Jan Beulich wrote:
>  Boris Ostrovsky  05/25/16 9:17
> PM >>>
> >> On 05/05/2016 12:58 AM, Lv Zheng wrote:
> >>> +static u8
> >>> +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg, u8
> max_bit_width)
> >>> +{
> >>> +u64 address;
> >>> +
> >>> +if (!reg->access_width) {
> >>> +/*
> >>> + * Detect old register descriptors where only the bit_width field
> >>> + * makes senses. The target address is copied to handle possible
> >>> + * alignment issues.
> >>> + */
> >>> +ACPI_MOVE_64_TO_64(, >address);
> >>> +if (!reg->bit_offset && reg->bit_width &&
> >>> +ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
> >>> +ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> >>> +ACPI_IS_ALIGNED(address, reg->bit_width)) {
> >>> +return (reg->bit_width);
> >>> +} else {
> >>> +if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> >>> +return (32);
> >> This (together with "... Add access_width/bit_offset support in
> >> acpi_hw_write") breaks Xen guests using older QEMU which doesn't
> support
> >> 4-byte IO accesses.
> >>
> >> Why not return "reg->bit_width?:max_bit_width" ? This will preserve
> >> original behavior.
> > Did you figure out why we get here in the first place, instead of taking
> the
> > first "return"? I.e. isn't the issue the apparently wrong use of the second
> > ACPI_IS_ALIGNED() above? Afaict it ought to be
> > ACPI_IS_ALIGNED(address, reg->bit_width / 8)...
> 
> We are trying to access address 0x...b004 (PM1a control) so yes, fixing
> alignment check would probably resolve the problem that we are seeing
> now.
> 
> However, for compatibility purposes we may consider not doing any
> checks
> and simply return bit_width if access_width is not available.
[Lv Zheng] 

There might be GAS defined with AccessWidth = 0.
And its BitOffset can still make senses.
That's why I put the check here, making it a tuning rather than a regression 
safe check.

Thanks for the information, I'll submit the fix of the address check.
To convert it to:
ACPI_IS_ALIGNED(address, reg->bit_width / 8)
This is my fault.

Thanks
-Lv

> 
> -boris
> 

Re: [PATCH v2] ARM: avoid duplicated "Kernel: arch/arm/boot/*Image is ready" log

[Masahiro Yamada]

Hi.

Assuming no objection to this patch,
I will put it into Russell's patch tracker after v4.7-rc1 is out.



2016-04-04 11:29 GMT+09:00 Masahiro Yamada :
> Commit 3939f3345050 ("ARM: 8418/1: add boot image dependencies to not
> generate invalid images") fixed bad image generation in case of
> parallel building, but as a side effect, Kbuild now descends into
> arch/arm/boot/ again and again, duplicating the log messages.  It is
> clumsy, so let's display the same message only once.
>
> I moved the log rules from arch/arm/boot/Makefile to arch/arm/Makefile
> rather than completely deleting them, because *Image are the final
> targets that users are interested in.
>
> Without this commit, the log of incremental build is like follows:
>
> $ make ARCH=arm UIMAGE_LOADADDR=0x80208000 uImage
>   CHK include/config/kernel.release
>   CHK include/generated/uapi/linux/version.h
>   CHK include/generated/utsrelease.h
>   CHK include/generated/bounds.h
>   CHK include/generated/timeconst.h
>   CHK include/generated/asm-offsets.h
>   CALLscripts/checksyscalls.sh
> :1310:2: warning: #warning syscall preadv2 not implemented [-Wcpp]
> :1313:2: warning: #warning syscall pwritev2 not implemented [-Wcpp]
>   CHK include/generated/compile.h
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/zImage is ready
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/zImage is ready
>   Image arch/arm/boot/uImage is ready
>
> With this commit:
>
> $ make ARCH=arm UIMAGE_LOADADDR=0x80208000 uImage
>   CHK include/config/kernel.release
>   CHK include/generated/uapi/linux/version.h
>   CHK include/generated/utsrelease.h
>   CHK include/generated/bounds.h
>   CHK include/generated/timeconst.h
>   CHK include/generated/asm-offsets.h
>   CALLscripts/checksyscalls.sh
> :1310:2: warning: #warning syscall preadv2 not implemented [-Wcpp]
> :1313:2: warning: #warning syscall pwritev2 not implemented [-Wcpp]
>   CHK include/generated/compile.h
>   Kernel: Image is ready
>   Kernel: zImage is ready
>   Kernel: uImage is ready
>
> Signed-off-by: Masahiro Yamada 


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] ARM: avoid duplicated "Kernel: arch/arm/boot/*Image is ready" log

[Masahiro Yamada]

Hi.

Assuming no objection to this patch,
I will put it into Russell's patch tracker after v4.7-rc1 is out.



2016-04-04 11:29 GMT+09:00 Masahiro Yamada :
> Commit 3939f3345050 ("ARM: 8418/1: add boot image dependencies to not
> generate invalid images") fixed bad image generation in case of
> parallel building, but as a side effect, Kbuild now descends into
> arch/arm/boot/ again and again, duplicating the log messages.  It is
> clumsy, so let's display the same message only once.
>
> I moved the log rules from arch/arm/boot/Makefile to arch/arm/Makefile
> rather than completely deleting them, because *Image are the final
> targets that users are interested in.
>
> Without this commit, the log of incremental build is like follows:
>
> $ make ARCH=arm UIMAGE_LOADADDR=0x80208000 uImage
>   CHK include/config/kernel.release
>   CHK include/generated/uapi/linux/version.h
>   CHK include/generated/utsrelease.h
>   CHK include/generated/bounds.h
>   CHK include/generated/timeconst.h
>   CHK include/generated/asm-offsets.h
>   CALLscripts/checksyscalls.sh
> :1310:2: warning: #warning syscall preadv2 not implemented [-Wcpp]
> :1313:2: warning: #warning syscall pwritev2 not implemented [-Wcpp]
>   CHK include/generated/compile.h
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/zImage is ready
>   Kernel: arch/arm/boot/Image is ready
>   Kernel: arch/arm/boot/zImage is ready
>   Image arch/arm/boot/uImage is ready
>
> With this commit:
>
> $ make ARCH=arm UIMAGE_LOADADDR=0x80208000 uImage
>   CHK include/config/kernel.release
>   CHK include/generated/uapi/linux/version.h
>   CHK include/generated/utsrelease.h
>   CHK include/generated/bounds.h
>   CHK include/generated/timeconst.h
>   CHK include/generated/asm-offsets.h
>   CALLscripts/checksyscalls.sh
> :1310:2: warning: #warning syscall preadv2 not implemented [-Wcpp]
> :1313:2: warning: #warning syscall pwritev2 not implemented [-Wcpp]
>   CHK include/generated/compile.h
>   Kernel: Image is ready
>   Kernel: zImage is ready
>   Kernel: uImage is ready
>
> Signed-off-by: Masahiro Yamada 


-- 
Best Regards
Masahiro Yamada

Re: [RFC 0/5] usb: chipidea: Add support for Tegra20 through Tegra124

[Peter Chen]

On Thu, May 26, 2016 at 05:40:00PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> All Tegra SoC generations from Tegra20 through Tegra124 have a ChipIdea
> USB device controller. This set of patches adds very rudimentary support
> for it to the existing ChipIdea driver and enables them on the set of
> boards that I could easily test on.
> 
> I'm sending this out as RFC because I'm not sure yet how to merge this.
> While the driver seems to work fine (tested by exporting a USB driver or
> eMMC via the mass storage function) I don't yet understand how to make
> the driver switch between host and device modes dynamically. It might be
> useful to get this merged before, but I'd like to have some feedback on
> this, because doing so would mean that we need to use device mode on the
> devices where it's enabled and can't use the USBD port in host mode.
> 

Chipidea driver supports many ways to switch between host and device
mode. It can support switching with/without disconnecting cable.

Most of cases need to disconnect cable (Micro-AB) to switch between
host and device mode, I just take this as an example:

Using ID pin which is at Micro-B receptacle on the board to determine host (ID 
= 0)
or device (ID = 1 )mode.

- ID pin connects to CPU, and ID interrupt and value can be get through
register OTGSC.
- ID pin does not connect to CPU, and there is a dedicated GPIO for ID.

-- 

Best Regards,
Peter Chen

Re: [RFC 0/5] usb: chipidea: Add support for Tegra20 through Tegra124

[Peter Chen]

On Thu, May 26, 2016 at 05:40:00PM +0200, Thierry Reding wrote:
> From: Thierry Reding 
> 
> All Tegra SoC generations from Tegra20 through Tegra124 have a ChipIdea
> USB device controller. This set of patches adds very rudimentary support
> for it to the existing ChipIdea driver and enables them on the set of
> boards that I could easily test on.
> 
> I'm sending this out as RFC because I'm not sure yet how to merge this.
> While the driver seems to work fine (tested by exporting a USB driver or
> eMMC via the mass storage function) I don't yet understand how to make
> the driver switch between host and device modes dynamically. It might be
> useful to get this merged before, but I'd like to have some feedback on
> this, because doing so would mean that we need to use device mode on the
> devices where it's enabled and can't use the USBD port in host mode.
> 

Chipidea driver supports many ways to switch between host and device
mode. It can support switching with/without disconnecting cable.

Most of cases need to disconnect cable (Micro-AB) to switch between
host and device mode, I just take this as an example:

Using ID pin which is at Micro-B receptacle on the board to determine host (ID 
= 0)
or device (ID = 1 )mode.

- ID pin connects to CPU, and ID interrupt and value can be get through
register OTGSC.
- ID pin does not connect to CPU, and there is a dedicated GPIO for ID.

-- 

Best Regards,
Peter Chen

RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support

[Zheng, Lv]

Hi,

> From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-
> ow...@vger.kernel.org] On Behalf Of Boris Ostrovsky
> Sent: Thursday, May 26, 2016 3:17 AM
> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit
> width support
> 
> On 05/05/2016 12:58 AM, Lv Zheng wrote:
> > ACPICA commit c49a751b4dae7baec1790748a2b4b6e8ab599f51
> >
> > For Access Size = 0, it actually can use user expected access bit width.
> > This patch implements this.
> >
> > Besides of the ACPICA upstream commit, this patch also includes a fix fixing
> > the issue reported by the FreeBSD community.
> > The old register descriptors are translated in 
> > acpi_tb_init_generic_address()
> > with access_width being filled with 0. This breaks code in
> > acpi_hw_get_access_bit_width() when the registers are 16-bit IO ports and
> their
> > bit_width fields are filled with 16. The rapid fix is meant to make code
> > written for acpi_hw_get_access_bit_width() regression safer before the issue
> is
> > correctly fixed from acpi_tb_init_generic_address(). Reported by
> > John Baldwin , fixed by Lv Zheng ,
> tested
> > by Jung-uk Kim .
> >
> > Link: https://github.com/acpica/acpica/commit/c49a751b
> > Reported-by: John Baldwin 
> > Tested-by Jung-uk Kim .
> > Signed-off-by: Lv Zheng 
> > Signed-off-by: Bob Moore 
> > ---
> >  drivers/acpi/acpica/hwregs.c |   49
> --
> >  1 file changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> > index 035fb52..892e677 100644
> > --- a/drivers/acpi/acpica/hwregs.c
> > +++ b/drivers/acpi/acpica/hwregs.c
> > @@ -51,6 +51,10 @@ ACPI_MODULE_NAME("hwregs")
> >
> >  #if (!ACPI_REDUCED_HARDWARE)
> >  /* Local Prototypes */
> > +static u8
> > +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg,
> > +u8 max_bit_width);
> > +
> >  static acpi_status
> >  acpi_hw_read_multiple(u32 *value,
> >   struct acpi_generic_address *register_a,
> > @@ -65,6 +69,48 @@ acpi_hw_write_multiple(u32 value,
> >
> >
> /***
> ***
> >   *
> > + * FUNCTION:acpi_hw_get_access_bit_width
> > + *
> > + * PARAMETERS:  reg - GAS register structure
> > + *  max_bit_width   - Max bit_width supported (32 or 64)
> > + *
> > + * RETURN:  Status
> > + *
> > + * DESCRIPTION: Obtain optimal access bit width
> > + *
> > +
> 
> **/
> > +
> > +static u8
> > +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg, u8
> max_bit_width)
> > +{
> > +   u64 address;
> > +
> > +   if (!reg->access_width) {
> > +   /*
> > +* Detect old register descriptors where only the bit_width 
> > field
> > +* makes senses. The target address is copied to handle
> possible
> > +* alignment issues.
> > +*/
> > +   ACPI_MOVE_64_TO_64(, >address);
> > +   if (!reg->bit_offset && reg->bit_width &&
> > +   ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
> > +   ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> > +   ACPI_IS_ALIGNED(address, reg->bit_width)) {
> > +   return (reg->bit_width);
> > +   } else {
> > +   if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > +   return (32);
> 
> This (together with "... Add access_width/bit_offset support in
> acpi_hw_write") breaks Xen guests using older QEMU which doesn't support
> 4-byte IO accesses.
> 
> Why not return "reg->bit_width?:max_bit_width" ? This will preserve
> original behavior.
[Lv Zheng] 

Let me ask 2 questions:

A. Was this a bug of the older qemu?
If so, you only need a quirk mechanism for old qemu to work.
And it would be better to provide the quirk inside of the older qemu.

B. Could you provide the detailed register description?
The case I saw from the freebsd community around the qemu is a bit_width = 16 
case.
Which is a conversion result of acpi_tb_init_generic_address().
Thus:
1. reg->access_width is always 0
2. reg->bit_offset is always 0
3. reg->bit_width is either 8 or 16
So they can be covered by this check:
if (reg->access_width) {
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8))
I just enhanced it with the address check: ACPI_IS_ALIGNED(address, 
reg->bit_width)

For your case, what the values of "address/access_width" are?
Can it be fixed by:
1. Either removing ACPI_IS_ALIGNED(address, reg->bit_width), or
2. moving the entire check out of if (!reg->access_width) to form:
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
 

RE: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit width support

[Zheng, Lv]

Hi,

> From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-
> ow...@vger.kernel.org] On Behalf Of Boris Ostrovsky
> Sent: Thursday, May 26, 2016 3:17 AM
> Subject: Re: [PATCH v2 08/13] ACPICA: Hardware: Add optimized access bit
> width support
> 
> On 05/05/2016 12:58 AM, Lv Zheng wrote:
> > ACPICA commit c49a751b4dae7baec1790748a2b4b6e8ab599f51
> >
> > For Access Size = 0, it actually can use user expected access bit width.
> > This patch implements this.
> >
> > Besides of the ACPICA upstream commit, this patch also includes a fix fixing
> > the issue reported by the FreeBSD community.
> > The old register descriptors are translated in 
> > acpi_tb_init_generic_address()
> > with access_width being filled with 0. This breaks code in
> > acpi_hw_get_access_bit_width() when the registers are 16-bit IO ports and
> their
> > bit_width fields are filled with 16. The rapid fix is meant to make code
> > written for acpi_hw_get_access_bit_width() regression safer before the issue
> is
> > correctly fixed from acpi_tb_init_generic_address(). Reported by
> > John Baldwin , fixed by Lv Zheng ,
> tested
> > by Jung-uk Kim .
> >
> > Link: https://github.com/acpica/acpica/commit/c49a751b
> > Reported-by: John Baldwin 
> > Tested-by Jung-uk Kim .
> > Signed-off-by: Lv Zheng 
> > Signed-off-by: Bob Moore 
> > ---
> >  drivers/acpi/acpica/hwregs.c |   49
> --
> >  1 file changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> > index 035fb52..892e677 100644
> > --- a/drivers/acpi/acpica/hwregs.c
> > +++ b/drivers/acpi/acpica/hwregs.c
> > @@ -51,6 +51,10 @@ ACPI_MODULE_NAME("hwregs")
> >
> >  #if (!ACPI_REDUCED_HARDWARE)
> >  /* Local Prototypes */
> > +static u8
> > +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg,
> > +u8 max_bit_width);
> > +
> >  static acpi_status
> >  acpi_hw_read_multiple(u32 *value,
> >   struct acpi_generic_address *register_a,
> > @@ -65,6 +69,48 @@ acpi_hw_write_multiple(u32 value,
> >
> >
> /***
> ***
> >   *
> > + * FUNCTION:acpi_hw_get_access_bit_width
> > + *
> > + * PARAMETERS:  reg - GAS register structure
> > + *  max_bit_width   - Max bit_width supported (32 or 64)
> > + *
> > + * RETURN:  Status
> > + *
> > + * DESCRIPTION: Obtain optimal access bit width
> > + *
> > +
> 
> **/
> > +
> > +static u8
> > +acpi_hw_get_access_bit_width(struct acpi_generic_address *reg, u8
> max_bit_width)
> > +{
> > +   u64 address;
> > +
> > +   if (!reg->access_width) {
> > +   /*
> > +* Detect old register descriptors where only the bit_width 
> > field
> > +* makes senses. The target address is copied to handle
> possible
> > +* alignment issues.
> > +*/
> > +   ACPI_MOVE_64_TO_64(, >address);
> > +   if (!reg->bit_offset && reg->bit_width &&
> > +   ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
> > +   ACPI_IS_ALIGNED(reg->bit_width, 8) &&
> > +   ACPI_IS_ALIGNED(address, reg->bit_width)) {
> > +   return (reg->bit_width);
> > +   } else {
> > +   if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > +   return (32);
> 
> This (together with "... Add access_width/bit_offset support in
> acpi_hw_write") breaks Xen guests using older QEMU which doesn't support
> 4-byte IO accesses.
> 
> Why not return "reg->bit_width?:max_bit_width" ? This will preserve
> original behavior.
[Lv Zheng] 

Let me ask 2 questions:

A. Was this a bug of the older qemu?
If so, you only need a quirk mechanism for old qemu to work.
And it would be better to provide the quirk inside of the older qemu.

B. Could you provide the detailed register description?
The case I saw from the freebsd community around the qemu is a bit_width = 16 
case.
Which is a conversion result of acpi_tb_init_generic_address().
Thus:
1. reg->access_width is always 0
2. reg->bit_offset is always 0
3. reg->bit_width is either 8 or 16
So they can be covered by this check:
if (reg->access_width) {
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8))
I just enhanced it with the address check: ACPI_IS_ALIGNED(address, 
reg->bit_width)

For your case, what the values of "address/access_width" are?
Can it be fixed by:
1. Either removing ACPI_IS_ALIGNED(address, reg->bit_width), or
2. moving the entire check out of if (!reg->access_width) to form:
if (!reg->bit_offset && reg->bit_width &&
ACPI_IS_POWER_OF_TWO(reg->bit_width) &&
ACPI_IS_ALIGNED(reg->bit_width, 8) &&
ACPI_IS_ALIGNED(address, reg->bit_width)) {
return (reg->bit_width);
} else if 

linux-next: build warning after merge of the akpm-current tree

[Stephen Rothwell]

Hi Andrew,

After merging the akpm-current tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:

mm/oom_kill.c: In function '__oom_reap_task':
mm/oom_kill.c:537:2: warning: 'mm' may be used uninitialized in this function 
[-Wmaybe-uninitialized]
  mmput_async(mm);
  ^

Introduced by commit

  df1e2f56632d ("oom_reaper: close race with exiting task")

This is real - the first "goto unlock_oom" is before "mm" has been
assigned.

-- 
Cheers,
Stephen Rothwell

linux-next: build warning after merge of the akpm-current tree

[Stephen Rothwell]

Hi Andrew,

After merging the akpm-current tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:

mm/oom_kill.c: In function '__oom_reap_task':
mm/oom_kill.c:537:2: warning: 'mm' may be used uninitialized in this function 
[-Wmaybe-uninitialized]
  mmput_async(mm);
  ^

Introduced by commit

  df1e2f56632d ("oom_reaper: close race with exiting task")

This is real - the first "goto unlock_oom" is before "mm" has been
assigned.

-- 
Cheers,
Stephen Rothwell

[PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

The main reason to add this check is to avoid unnecessary
mmc_request like the on-going cmd and the corresponding sbc
if the card is removed. Although we have already checked this in
dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
for noremovable devices, but there is a timing gap before it really
calls dw_mci_get_cd as mmc_detect_change needs some delay here.

Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..8eb7898 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)
 */
spin_lock_bh(>lock);
 
-   if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
+   if (!dw_mci_get_cd(mmc)) {
spin_unlock_bh(>lock);
mrq->cmd->error = -ENOMEDIUM;
mmc_request_done(mmc, mrq);
-- 
2.3.7

[PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

The main reason to add this check is to avoid unnecessary
mmc_request like the on-going cmd and the corresponding sbc
if the card is removed. Although we have already checked this in
dw_mci_handle_cd for runtime usage of sd card and dw_mci_init_slot
for noremovable devices, but there is a timing gap before it really
calls dw_mci_get_cd as mmc_detect_change needs some delay here.

Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..8eb7898 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);
 
 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1255,7 +1256,7 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)
 */
spin_lock_bh(>lock);
 
-   if (!test_bit(DW_MMC_CARD_PRESENT, >flags)) {
+   if (!dw_mci_get_cd(mmc)) {
spin_unlock_bh(>lock);
mrq->cmd->error = -ENOMEDIUM;
mmc_request_done(mmc, mrq);
-- 
2.3.7

[PATCH 1/2] mmc: dw_mmc: remove redundant of set_bit and clear_bit

[Shawn Lin]

dw_mci_get_cd have already dealt with these for
both of internal card-detect and gpio card-detect.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 829a6ee..cb30e91 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2616,10 +2616,7 @@ static int dw_mci_init_slot(struct dw_mci *host, 
unsigned int id)
mmc->max_seg_size = mmc->max_req_size;
}
 
-   if (dw_mci_get_cd(mmc))
-   set_bit(DW_MMC_CARD_PRESENT, >flags);
-   else
-   clear_bit(DW_MMC_CARD_PRESENT, >flags);
+   dw_mci_get_cd(mmc);
 
ret = mmc_add_host(mmc);
if (ret)
-- 
2.3.7

[PATCH 1/2] mmc: dw_mmc: remove redundant of set_bit and clear_bit

[Shawn Lin]

dw_mci_get_cd have already dealt with these for
both of internal card-detect and gpio card-detect.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 829a6ee..cb30e91 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2616,10 +2616,7 @@ static int dw_mci_init_slot(struct dw_mci *host, 
unsigned int id)
mmc->max_seg_size = mmc->max_req_size;
}
 
-   if (dw_mci_get_cd(mmc))
-   set_bit(DW_MMC_CARD_PRESENT, >flags);
-   else
-   clear_bit(DW_MMC_CARD_PRESENT, >flags);
+   dw_mci_get_cd(mmc);
 
ret = mmc_add_host(mmc);
if (ret)
-- 
2.3.7

Re: [PATCH] Use the correct size to set block max sectors

[Bart Van Assche]

On 05/26/16 17:08, Long Li wrote:

The block sector size should be in unit of 512 bytes, not in bytes.

Signed-off-by: Long Li 

---
 drivers/scsi/sd.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 428c03e..4bce17e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
if (sdkp->opt_xfer_blocks &&
sdkp->opt_xfer_blocks <= dev_max &&
sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
-   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
-   rw_max = q->limits.io_opt =
+   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
+   q->limits.io_opt =
sdkp->opt_xfer_blocks * sdp->sector_size;
+   rw_max = (q->limits.io_opt >> 9);
+   }
else
rw_max = BLK_DEF_MAX_SECTORS;


Isn't this a duplicate of a patch Martin Petersen posted three weeks 
ago? See also http://thread.gmane.org/gmane.linux.scsi/113746.


Bart.

Re: [PATCH] Use the correct size to set block max sectors

[Bart Van Assche]

On 05/26/16 17:08, Long Li wrote:

The block sector size should be in unit of 512 bytes, not in bytes.

Signed-off-by: Long Li 

---
 drivers/scsi/sd.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 428c03e..4bce17e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
if (sdkp->opt_xfer_blocks &&
sdkp->opt_xfer_blocks <= dev_max &&
sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
-   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
-   rw_max = q->limits.io_opt =
+   sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
+   q->limits.io_opt =
sdkp->opt_xfer_blocks * sdp->sector_size;
+   rw_max = (q->limits.io_opt >> 9);
+   }
else
rw_max = BLK_DEF_MAX_SECTORS;


Isn't this a duplicate of a patch Martin Petersen posted three weeks 
ago? See also http://thread.gmane.org/gmane.linux.scsi/113746.


Bart.

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Kees Cook]

On Thu, May 26, 2016 at 7:10 PM, Andy Lutomirski  wrote:
> On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
>> One problem with seccomp was that ptrace could be used to change a
>> syscall after seccomp filtering had completed. This was a well documented
>> limitation, and it was recommended to block ptrace when defining a filter
>> to avoid this problem. This can be quite a limitation for containers or
>> other places where ptrace is desired even under seccomp filters.
>>
>> Since seccomp filtering has been split into pre-trace and trace phases
>> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
>> after ptrace. This makes that change, and updates the test suite for
>> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.
>
> I like fixing the hole, but I don't like this fix.
>
> The two-phase seccomp mechanism is messy.  I wrote it because it was a
> huge speedup.  Since then, I've made a ton of changes to the way that
> x86 syscalls work, and there are two relevant effects: the slow path
> is quite fast, and the phase-1-only path isn't really a win any more.
>
> I suggest that we fix the by simplifying the code instead of making it
> even more complicated.  Let's back out the two-phase mechanism (but
> keep the ability for arch code to supply seccomp_data) and then just
> reorder it so that seccomp happens after ptrace.  The result should be
> considerably simpler.  (We'll still have to answer the question of
> what happens when a SECCOMP_RET_TRACE event changes the syscall, but
> maybe the answer is to just let it through -- after all,
> SECCOMP_RET_TRACE might be a request by a tracer to do its own
> internal filtering.)

I'm really against this. I think seccomp needs to stay first, and I
like the two-phase split because it gives us a lot of flexibility on
other architectures. And we can't just let through RET_TRACE because
we'll have exactly the same problem: a process can add a RET_TRACE
filter for some syscall and then change it arbitrarily to escape the
filtering. The non-trace returns of seccomp need to be check first and
after ptrace manipulations. The patch seems like the best approach and
it covers all the corners.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Kees Cook]

On Thu, May 26, 2016 at 7:10 PM, Andy Lutomirski  wrote:
> On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
>> One problem with seccomp was that ptrace could be used to change a
>> syscall after seccomp filtering had completed. This was a well documented
>> limitation, and it was recommended to block ptrace when defining a filter
>> to avoid this problem. This can be quite a limitation for containers or
>> other places where ptrace is desired even under seccomp filters.
>>
>> Since seccomp filtering has been split into pre-trace and trace phases
>> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
>> after ptrace. This makes that change, and updates the test suite for
>> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.
>
> I like fixing the hole, but I don't like this fix.
>
> The two-phase seccomp mechanism is messy.  I wrote it because it was a
> huge speedup.  Since then, I've made a ton of changes to the way that
> x86 syscalls work, and there are two relevant effects: the slow path
> is quite fast, and the phase-1-only path isn't really a win any more.
>
> I suggest that we fix the by simplifying the code instead of making it
> even more complicated.  Let's back out the two-phase mechanism (but
> keep the ability for arch code to supply seccomp_data) and then just
> reorder it so that seccomp happens after ptrace.  The result should be
> considerably simpler.  (We'll still have to answer the question of
> what happens when a SECCOMP_RET_TRACE event changes the syscall, but
> maybe the answer is to just let it through -- after all,
> SECCOMP_RET_TRACE might be a request by a tracer to do its own
> internal filtering.)

I'm really against this. I think seccomp needs to stay first, and I
like the two-phase split because it gives us a lot of flexibility on
other architectures. And we can't just let through RET_TRACE because
we'll have exactly the same problem: a process can add a RET_TRACE
filter for some syscall and then change it arbitrarily to escape the
filtering. The non-trace returns of seccomp need to be check first and
after ptrace manipulations. The patch seems like the best approach and
it covers all the corners.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH 3/3] arm64/numa: fix type info

[Leizhen (ThunderTown)]

On 2016/5/27 1:12, David Daney wrote:
> The current patch to correct this problem is here:
> 
> https://lkml.org/lkml/2016/5/24/679
> 
> Since v7 of the ACPI/NUMA patches are likely going to be added to linux-next 
> as soon as the current merge window ends, further simplifications of the 
> informational prints should probably be rebased on top of it.
> 
> David Daney
> 

>> On Thu, 2016-05-26 at 09:22 -0700, Ganapatrao Kulkarni wrote:
>>> IIRC, it should be
>>> if (!numa_off)
>>> we want to print this message when we failed to find proper numa 
>>> configuration.
>>> when numa_off is set, we will not look for any numa configuration.
>>>

 +   pr_info("%s\n", "No NUMA configuration found");
>>


OK, I think I also missed some cases.

But my problem still have not been resolved by 
"https://lkml.org/lkml/2016/5/24/679;, see below. I will update my patches base 
on it.


[0.00] NUMA: Adding memblock [0x0 - 0x6aff] on node 0
[0.00] NUMA: parsing numa-distance-map-v1
[0.00] NUMA: Warning: invalid memblk node 4 [mem 0x6b00-0x7fbf] 
//My numa configuration is incorrect, but not "No ... found"
[0.00] No NUMA configuration found  
//Above warning is very detail, this can be removed
[0.00] NUMA: Faking a node at [mem 
0x-0x0017]

Re: [PATCH 3/3] arm64/numa: fix type info

[Leizhen (ThunderTown)]

On 2016/5/27 1:12, David Daney wrote:
> The current patch to correct this problem is here:
> 
> https://lkml.org/lkml/2016/5/24/679
> 
> Since v7 of the ACPI/NUMA patches are likely going to be added to linux-next 
> as soon as the current merge window ends, further simplifications of the 
> informational prints should probably be rebased on top of it.
> 
> David Daney
> 

>> On Thu, 2016-05-26 at 09:22 -0700, Ganapatrao Kulkarni wrote:
>>> IIRC, it should be
>>> if (!numa_off)
>>> we want to print this message when we failed to find proper numa 
>>> configuration.
>>> when numa_off is set, we will not look for any numa configuration.
>>>

 +   pr_info("%s\n", "No NUMA configuration found");
>>


OK, I think I also missed some cases.

But my problem still have not been resolved by 
"https://lkml.org/lkml/2016/5/24/679;, see below. I will update my patches base 
on it.


[0.00] NUMA: Adding memblock [0x0 - 0x6aff] on node 0
[0.00] NUMA: parsing numa-distance-map-v1
[0.00] NUMA: Warning: invalid memblk node 4 [mem 0x6b00-0x7fbf] 
//My numa configuration is incorrect, but not "No ... found"
[0.00] No NUMA configuration found  
//Above warning is very detail, this can be removed
[0.00] NUMA: Faking a node at [mem 
0x-0x0017]

Re: [RFC PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

Hi Jaehoon,

On 2016/5/27 8:53, Jaehoon Chung wrote:

Hi Shawn,

On 05/26/2016 12:08 PM, Shawn Lin wrote:

The main reason to add this check is to avoid unnecessary
mmc_request if the card is removed. Although we have already
check this in dw_mci_handle_cd for runtime usage of sd card and
dw_mci_init_slot for noremovable devices, but there is a timing
gap before it really calls dw_mci_get_cd as mmc_detect_change needs
some delay here.


What's unnecessary mmc_request?


the on-going read/write/ and the corresponding sbc like cmd13, etc.






Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..2940d24 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);

 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1248,6 +1249,8 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)

WARN_ON(slot->mrq);

+   dw_mci_get_cd(mmc);
+


If my understanding is right, it can be replaced

if (!test_bit(DW_MMC_CARD_PRESENT, >flags))

to

if (!dw_mci_get_cd(mmc))


yeah~ It looks better.




Best Regards,
Jaehoon Chung




/*
 * The check for card presence and queueing of the request must be
 * atomic, otherwise the card could be removed in between and the









--
Best Regards
Shawn Lin

Re: [RFC PATCH 2/2] mmc: dw_mmc: check card present before starting request

[Shawn Lin]

Hi Jaehoon,

On 2016/5/27 8:53, Jaehoon Chung wrote:

Hi Shawn,

On 05/26/2016 12:08 PM, Shawn Lin wrote:

The main reason to add this check is to avoid unnecessary
mmc_request if the card is removed. Although we have already
check this in dw_mci_handle_cd for runtime usage of sd card and
dw_mci_init_slot for noremovable devices, but there is a timing
gap before it really calls dw_mci_get_cd as mmc_detect_change needs
some delay here.


What's unnecessary mmc_request?


the on-going read/write/ and the corresponding sbc like cmd13, etc.






Another gain here is that we could save some checkings of card status
after sd card been removed.

Signed-off-by: Shawn Lin 
---

 drivers/mmc/host/dw_mmc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cb30e91..2940d24 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -105,6 +105,7 @@ struct idmac_desc {
 static bool dw_mci_reset(struct dw_mci *host);
 static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset);
 static int dw_mci_card_busy(struct mmc_host *mmc);
+static int dw_mci_get_cd(struct mmc_host *mmc);

 #if defined(CONFIG_DEBUG_FS)
 static int dw_mci_req_show(struct seq_file *s, void *v)
@@ -1248,6 +1249,8 @@ static void dw_mci_request(struct mmc_host *mmc, struct 
mmc_request *mrq)

WARN_ON(slot->mrq);

+   dw_mci_get_cd(mmc);
+


If my understanding is right, it can be replaced

if (!test_bit(DW_MMC_CARD_PRESENT, >flags))

to

if (!dw_mci_get_cd(mmc))


yeah~ It looks better.




Best Regards,
Jaehoon Chung




/*
 * The check for card presence and queueing of the request must be
 * atomic, otherwise the card could be removed in between and the









--
Best Regards
Shawn Lin

Re: [RFC PATCH 1/2] mmc: dw_mmc: remove redundant of set_bit and clear_bit

[Shawn Lin]

在 2016/5/27 8:53, Jaehoon Chung 写道:

Hi Shawn,

On 05/26/2016 12:07 PM, Shawn Lin wrote:

dw_mci_get_cd have already dealed with these for
both of internal card-detect and gpio card-detect.


s/dealed/dealt

This patch looks good to me. Could you resend the patch? not RFC.



Ok.


Best Regards,
Jaehoon Chung



Signed-off-by: Shawn Lin 

---

 drivers/mmc/host/dw_mmc.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 829a6ee..cb30e91 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2616,10 +2616,7 @@ static int dw_mci_init_slot(struct dw_mci *host, 
unsigned int id)
mmc->max_seg_size = mmc->max_req_size;
}

-   if (dw_mci_get_cd(mmc))
-   set_bit(DW_MMC_CARD_PRESENT, >flags);
-   else
-   clear_bit(DW_MMC_CARD_PRESENT, >flags);
+   dw_mci_get_cd(mmc);

ret = mmc_add_host(mmc);
if (ret)









--
Best Regards
Shawn Lin

Re: [RFC PATCH 1/2] mmc: dw_mmc: remove redundant of set_bit and clear_bit

[Shawn Lin]

在 2016/5/27 8:53, Jaehoon Chung 写道:

Hi Shawn,

On 05/26/2016 12:07 PM, Shawn Lin wrote:

dw_mci_get_cd have already dealed with these for
both of internal card-detect and gpio card-detect.


s/dealed/dealt

This patch looks good to me. Could you resend the patch? not RFC.



Ok.


Best Regards,
Jaehoon Chung



Signed-off-by: Shawn Lin 

---

 drivers/mmc/host/dw_mmc.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 829a6ee..cb30e91 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2616,10 +2616,7 @@ static int dw_mci_init_slot(struct dw_mci *host, 
unsigned int id)
mmc->max_seg_size = mmc->max_req_size;
}

-   if (dw_mci_get_cd(mmc))
-   set_bit(DW_MMC_CARD_PRESENT, >flags);
-   else
-   clear_bit(DW_MMC_CARD_PRESENT, >flags);
+   dw_mci_get_cd(mmc);

ret = mmc_add_host(mmc);
if (ret)









--
Best Regards
Shawn Lin

Re: [GIT PULL] Ceph updates for 4.7-rc1

[Linus Torvalds]

On Thu, May 26, 2016 at 2:46 PM, Sage Weil  wrote:
>
> One point of clarification, though: in the past I've squashed down fixes
> discovered during testing if the branch hasn't hit a stable tree yet
> (e.g., your tree).  AIUI this is(was?) standard procedure for things in
> -next.

Yes, rebasing with good reason is acceptable for branches that don't
have anybody else depend on them.

Adn the "good reason" ends up being a judgement call. If the merge
window hasn't even started yet, the "good" reason may not even be very
great, and might might be "oh, I screwed up the commit message, so
let's make the history look good".

If it's already inside the merge window, you should aim for having
increasingly higher barriers to rebasing your tree, and strive to
generally try to avoid it.

If it's about something mostly cosmetic and the merge windoe has
opened or is just about to, leave it be.

On the other hand, if it's really nasty problem and seriously will
hurt people who try to bisect - even if you have fixed the problem
then later in the history - you might choose to do it to not be in the
situation that people who use "git bisect" to find another bug will
then be left with data corruption or something like that because of a
major bug in the middle of the development history.

And in between those two extremes of "cosmetic" and "nasty data
corruption bug" there is obviously a graduation of issues. There can't
be any completely black-and-white rules.

But the corollary to that is that if you really had a major bug that
you feld had to be fixed not just at the tip, but going back, then you
then shouldn't immediately send the end result to me. Because you just
fixed something critical (by definition, if you chose to do it just
before you would have wanted to send to me), so now you need to retest
things.

So rebasing isn't some absolute wrong thing. Sometimes rebasing is
simply the right thing to do. For example, maybe I don't get the same
commits that were in -next, but I would have still seen that the code
was there.

  Linus

Re: [GIT PULL] Ceph updates for 4.7-rc1

[Linus Torvalds]

On Thu, May 26, 2016 at 2:46 PM, Sage Weil  wrote:
>
> One point of clarification, though: in the past I've squashed down fixes
> discovered during testing if the branch hasn't hit a stable tree yet
> (e.g., your tree).  AIUI this is(was?) standard procedure for things in
> -next.

Yes, rebasing with good reason is acceptable for branches that don't
have anybody else depend on them.

Adn the "good reason" ends up being a judgement call. If the merge
window hasn't even started yet, the "good" reason may not even be very
great, and might might be "oh, I screwed up the commit message, so
let's make the history look good".

If it's already inside the merge window, you should aim for having
increasingly higher barriers to rebasing your tree, and strive to
generally try to avoid it.

If it's about something mostly cosmetic and the merge windoe has
opened or is just about to, leave it be.

On the other hand, if it's really nasty problem and seriously will
hurt people who try to bisect - even if you have fixed the problem
then later in the history - you might choose to do it to not be in the
situation that people who use "git bisect" to find another bug will
then be left with data corruption or something like that because of a
major bug in the middle of the development history.

And in between those two extremes of "cosmetic" and "nasty data
corruption bug" there is obviously a graduation of issues. There can't
be any completely black-and-white rules.

But the corollary to that is that if you really had a major bug that
you feld had to be fixed not just at the tip, but going back, then you
then shouldn't immediately send the end result to me. Because you just
fixed something critical (by definition, if you chose to do it just
before you would have wanted to send to me), so now you need to retest
things.

So rebasing isn't some absolute wrong thing. Sometimes rebasing is
simply the right thing to do. For example, maybe I don't get the same
commits that were in -next, but I would have still seen that the code
was there.

  Linus

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Andy Lutomirski]

On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
> One problem with seccomp was that ptrace could be used to change a
> syscall after seccomp filtering had completed. This was a well documented
> limitation, and it was recommended to block ptrace when defining a filter
> to avoid this problem. This can be quite a limitation for containers or
> other places where ptrace is desired even under seccomp filters.
>
> Since seccomp filtering has been split into pre-trace and trace phases
> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
> after ptrace. This makes that change, and updates the test suite for
> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.

I like fixing the hole, but I don't like this fix.

The two-phase seccomp mechanism is messy.  I wrote it because it was a
huge speedup.  Since then, I've made a ton of changes to the way that
x86 syscalls work, and there are two relevant effects: the slow path
is quite fast, and the phase-1-only path isn't really a win any more.

I suggest that we fix the by simplifying the code instead of making it
even more complicated.  Let's back out the two-phase mechanism (but
keep the ability for arch code to supply seccomp_data) and then just
reorder it so that seccomp happens after ptrace.  The result should be
considerably simpler.  (We'll still have to answer the question of
what happens when a SECCOMP_RET_TRACE event changes the syscall, but
maybe the answer is to just let it through -- after all,
SECCOMP_RET_TRACE might be a request by a tracer to do its own
internal filtering.)

--Andy

Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Andy Lutomirski]

On Thu, May 26, 2016 at 2:04 PM, Kees Cook  wrote:
> One problem with seccomp was that ptrace could be used to change a
> syscall after seccomp filtering had completed. This was a well documented
> limitation, and it was recommended to block ptrace when defining a filter
> to avoid this problem. This can be quite a limitation for containers or
> other places where ptrace is desired even under seccomp filters.
>
> Since seccomp filtering has been split into pre-trace and trace phases
> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
> after ptrace. This makes that change, and updates the test suite for
> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.

I like fixing the hole, but I don't like this fix.

The two-phase seccomp mechanism is messy.  I wrote it because it was a
huge speedup.  Since then, I've made a ton of changes to the way that
x86 syscalls work, and there are two relevant effects: the slow path
is quite fast, and the phase-1-only path isn't really a win any more.

I suggest that we fix the by simplifying the code instead of making it
even more complicated.  Let's back out the two-phase mechanism (but
keep the ability for arch code to supply seccomp_data) and then just
reorder it so that seccomp happens after ptrace.  The result should be
considerably simpler.  (We'll still have to answer the question of
what happens when a SECCOMP_RET_TRACE event changes the syscall, but
maybe the answer is to just let it through -- after all,
SECCOMP_RET_TRACE might be a request by a tracer to do its own
internal filtering.)

--Andy