Re: [RFC 1/3] x86/vdso: create vdso file, use it for mapping

[Al Viro]

On Tue, Aug 30, 2016 at 04:33:12PM +0200, Oleg Nesterov wrote:
> > +   inode = ramfs_get_inode(sb, NULL, S_IFREG | S_IRUGO | S_IXUGO, 0);

> Not sure... I think alloc_anon_inode() makes more sense.

Compared to this, you mean?  It's going to give you the wrong
permissions/i_op/a_ops, and you'll have to assign them manually.  ramfs
already gives the right ones for what's wanted, AFAICS.

Re: [PATCH v2 0/2] arm64: dts: rockchip: Support PMU for rk3399 SoCs

[Caesar Wang]

Heiko,

What do you think of it?
Maybe I need re-update these patches on next kernel, and re-test them.

On 2016年07月06日 16:05, Caesar Wang wrote:

Hello Heiko, Marc & ARM guys

When Jay first submitted the rk3399.dtsi upstream
 he had the PMU node in there,
but then took it out because the upstream binding wasn't done yet.
It looks as if the upstream stuff has landed, since in linux/master I see:
287e9357abcc DT/arm,gic-v3: Documment PPI partition support
e3825ba1af3a irqchip/gic-v3: Add support for partitioned PPIs
9e2c986cb460 irqchip: Add per-cpu interrupt partitioning library
222df54fd8b7 genirq: Allow the affinity of a percpu interrupt to be 
set/retrieved
651e8b54abde irqdomain: Allow domain matching on irq_fwspec

This series patches add to support the rk3399 SoCs PMU.
I pick up the https://patchwork.kernel.org/patch/9209369/.

As do some tests with ChromeOs for my rk3399 board.
Tested with linus master 4.7-rc6 kernel on rk3399 board.
https://github.com/Caesar-github/rockchip/tree/rk3399/pmu-upstream

localhost / # perf list

List of pre-defined events (to be used in -e):
cpu-cycles OR cycles [Hardware event]
instructions [Hardware event]
cache-references [Hardware event]
cache-misses [Hardware event]
branch-instructions OR branches [Hardware event]
branch-misses [Hardware event]
bus-cycles [Hardware event]
...

perf stat --cpu 0/1/2/3. to minitor
e.g. cpu0;

localhost / # perf stat --cpu 0

Performance counter stats for 'CPU(s) 0':

3374.917571 task-clock (msec) # 1.001 CPUs utilized [100.00%]
20 context-switches # 0.006 K/sec [100.00%]
2 cpu-migrations # 0.001 K/sec [100.00%]
55 page-faults # 0.016 K/sec
7151843 cycles # 0.002 GHz [100.00%]
 stalled-cycles-frontend
 stalled-cycles-backend
4272536 instructions # 0.60 insns per cycle [100.00%]
568406 branches # 0.168 M/sec [100.00%]
65652 branch-misses # 11.55% of all branches

Also, 'perf top' to monitor the PMU interrupts from cpus

-Caesar


Changes in v2:
- AS Mark comments on https://patchwork.kernel.org/patch/9209369/
   remove the interrupt-affinity property, we need depend on Marc' perf
   code on https://patchwork.kernel.org/patch/9209369/.

Caesar Wang (2):
   arm64: dts: rockchip: change all interrupts cells for 4 on rk3399 SoCs
   arm64: dts: rockchip: support the pmu node for rk3399

  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 118 ++-
  1 file changed, 69 insertions(+), 49 deletions(-)




--
caesar wang | software engineer | w...@rock-chip.com

Re: [PATCH v4 0/6] Support the rk3399 gmac and pd function

[David Miller]

From: Caesar Wang 
Date: Fri,  2 Sep 2016 01:49:58 +0800

> This patch have the following changes:
> 
> 7edf13e net: stmmac: dwmac-rk: add rk3366 & rk3399 specific data
> 26e004e net: stmmac: dwmac-rk: fixes the gmac resume after PD on/off
> b216c2f net: stmmac: dwmac-rk: add pd_gmac support for rk3399
> 848bb71 arm64: dts: rockchip: add the gmac power domain on rk3399
> 508e41f arm64: dts: rockchip: add the gmac needed node for rk3399
> fb26795 arm64: dts: rockchip: enable the gmac for rk3399 evb board
> 
> Hi David,
> The patch 1,2,3 is related to the rockchip net/stammc driver,
> 
> Hi Heiko,
> The patch 4,5,6 is related to the dts changes.

Patches 1, 2, and 3 applied to net-next, thanks.

Re: [PATCH v4 0/6] Support the rk3399 gmac and pd function

[David Miller]

From: Caesar Wang 
Date: Fri,  2 Sep 2016 01:49:58 +0800

> This patch have the following changes:
> 
> 7edf13e net: stmmac: dwmac-rk: add rk3366 & rk3399 specific data
> 26e004e net: stmmac: dwmac-rk: fixes the gmac resume after PD on/off
> b216c2f net: stmmac: dwmac-rk: add pd_gmac support for rk3399
> 848bb71 arm64: dts: rockchip: add the gmac power domain on rk3399
> 508e41f arm64: dts: rockchip: add the gmac needed node for rk3399
> fb26795 arm64: dts: rockchip: enable the gmac for rk3399 evb board
> 
> Hi David,
> The patch 1,2,3 is related to the rockchip net/stammc driver,
> 
> Hi Heiko,
> The patch 4,5,6 is related to the dts changes.

Patches 1, 2, and 3 applied to net-next, thanks.

Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)

[Vineet Gupta]

On 09/02/2016 04:33 PM, Chen Gang wrote:
> On 9/2/16 04:43, Al Viro wrote:
>> > On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote:
>> > 
>>> >> Could you provide the related proof?
>>> >>
>>> >> Or shall I try to analyze about it and get proof?
>> > 
>> > Can you show a proof that it actually improves anything?  He who proposes
>> > a patch gets to defend it, not the other way round...
>> > 
>> > Al, bloody annoyed
>> > 
> OK, what you said sounds reasonable to me.
>
> It makes the code more readable since they are really pure Boolean
> functions, and let the functions are precisely same in all archs. But
> really, I shall try to prove that it has no negative effect.
>
> e.g. for arc arch. now, I have built the arc raw compiler to build arc
> kernel, but excuse me, I plan to finish proof next week, because during
> these days, I have to work, buy house, and focus on my father's health.

Since you seem to be have so much stuff to do I decided to help. I did a quick
compile of kernel with and w/o your changes

bloat-o-meter vmlinux-v4.8rc4-baseline vmlinux-v4.8rc4-bool-in-atomics
add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
function old new   delta
vermagic  49  55  +6
Total: Before=5967447, After=5967453, chg 0.00%

I'm mildly surprised that there is no difference so yeah this change is fine as
far as I'm concerned.

-Vineet

Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)

[Vineet Gupta]

On 09/02/2016 04:33 PM, Chen Gang wrote:
> On 9/2/16 04:43, Al Viro wrote:
>> > On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote:
>> > 
>>> >> Could you provide the related proof?
>>> >>
>>> >> Or shall I try to analyze about it and get proof?
>> > 
>> > Can you show a proof that it actually improves anything?  He who proposes
>> > a patch gets to defend it, not the other way round...
>> > 
>> > Al, bloody annoyed
>> > 
> OK, what you said sounds reasonable to me.
>
> It makes the code more readable since they are really pure Boolean
> functions, and let the functions are precisely same in all archs. But
> really, I shall try to prove that it has no negative effect.
>
> e.g. for arc arch. now, I have built the arc raw compiler to build arc
> kernel, but excuse me, I plan to finish proof next week, because during
> these days, I have to work, buy house, and focus on my father's health.

Since you seem to be have so much stuff to do I decided to help. I did a quick
compile of kernel with and w/o your changes

bloat-o-meter vmlinux-v4.8rc4-baseline vmlinux-v4.8rc4-bool-in-atomics
add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6)
function old new   delta
vermagic  49  55  +6
Total: Before=5967447, After=5967453, chg 0.00%

I'm mildly surprised that there is no difference so yeah this change is fine as
far as I'm concerned.

-Vineet

Re: [RFC 1/3] x86/vdso: create vdso file, use it for mapping

[Al Viro]

On Mon, Aug 29, 2016 at 02:28:08AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 25, 2016 at 8:21 AM, Dmitry Safonov  
> wrote:
> > I added here a new in-kernel fs with ramfs-like options.
> > Created vdso file in this fs (yet for testing, only 64-bit vdso).
> > Mapped this file to process's mm on setup_additional_pages.
> > Just for testing purpose it's done only for specific UID.
> 
> I'm wondering whether all this code could be easily moved into the
> core special mapping helpers so that all special mappings get the same
> benefit.  We could embed a struct file * (or struct inode or whatever)
> in special_mapping if needed.
> 
> Also, could this be simplified to use anon_inode?

Please, don't.  anon_inode is for situations when you don't mind sharing
the _same_ inode for different things.  This one very clearly isn't that.

Re: [RFC 1/3] x86/vdso: create vdso file, use it for mapping

[Al Viro]

On Mon, Aug 29, 2016 at 02:28:08AM -0700, Andy Lutomirski wrote:
> On Thu, Aug 25, 2016 at 8:21 AM, Dmitry Safonov  
> wrote:
> > I added here a new in-kernel fs with ramfs-like options.
> > Created vdso file in this fs (yet for testing, only 64-bit vdso).
> > Mapped this file to process's mm on setup_additional_pages.
> > Just for testing purpose it's done only for specific UID.
> 
> I'm wondering whether all this code could be easily moved into the
> core special mapping helpers so that all special mappings get the same
> benefit.  We could embed a struct file * (or struct inode or whatever)
> in special_mapping if needed.
> 
> Also, could this be simplified to use anon_inode?

Please, don't.  anon_inode is for situations when you don't mind sharing
the _same_ inode for different things.  This one very clearly isn't that.

Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe

[Luis R. Rodriguez]

On Thu, Aug 25, 2016 at 09:41:33PM +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez  
> > wrote:
> > Some users want a fully running gfx stack 2s after power-on. There's
> > not even enough time to load an uefi or vga driver first. i915
> > directly initializes the gpu from power-on state on those.
> 
> I see.. thanks.
> 
> > >> I think what would work is loading the different subsystems of the
> > >> driver in parallel (we already do that largely)
> > >
> > > Init level stuff is actually pretty synchronous, and in fact both
> > > init and probe are called serially. There are a few subsystems which
> > > have been doing things a bit differently, but these are exceptions.
> > >
> > > When you say we already do this largely, can you describe a bit more
> > > precisely what you mean ?
> > 
> > Oh, this isn't subsystems as in linux device/driver model, but
> > different parts within the driver. We fire up a bunch of struct work
> > to get various bits done asynchronously.
> 
> Thanks for the clarification.

<-- snip -->

> > One of the proposals which would have worked for us died a while back:
> > 
> > https://lkml.org/lkml/2014/6/15/47
> 
> Interesting, the problem stated here, which comes from the fact (as clarified
> above) the rootfs comes up only *after* we run do_initcalls() as such there 
> are
> possible theoretical races even with request_firmware_nowait() -- as such that
> justifies even more the SmPL grammar rule to include even 
> request_firmware_nowait()
> on probe / init as this patch has.
> 
> Anyway yeah that patch has good intentions but its wrong for a slew of 
> reasons.
> The problem is the same as discussed with Bjorn recently. The solution
> discussed is that since only userspace knows when the *real* rootfs is ready
> (because of slew of reasons) the best we can do is have an event issued by
> userspace to inform us of that.

OK I have something I think we can build upon for this. Will send RFC.

  Luis

Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe

[Luis R. Rodriguez]

On Thu, Aug 25, 2016 at 09:41:33PM +0200, Luis R. Rodriguez wrote:
> On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez  
> > wrote:
> > Some users want a fully running gfx stack 2s after power-on. There's
> > not even enough time to load an uefi or vga driver first. i915
> > directly initializes the gpu from power-on state on those.
> 
> I see.. thanks.
> 
> > >> I think what would work is loading the different subsystems of the
> > >> driver in parallel (we already do that largely)
> > >
> > > Init level stuff is actually pretty synchronous, and in fact both
> > > init and probe are called serially. There are a few subsystems which
> > > have been doing things a bit differently, but these are exceptions.
> > >
> > > When you say we already do this largely, can you describe a bit more
> > > precisely what you mean ?
> > 
> > Oh, this isn't subsystems as in linux device/driver model, but
> > different parts within the driver. We fire up a bunch of struct work
> > to get various bits done asynchronously.
> 
> Thanks for the clarification.

<-- snip -->

> > One of the proposals which would have worked for us died a while back:
> > 
> > https://lkml.org/lkml/2014/6/15/47
> 
> Interesting, the problem stated here, which comes from the fact (as clarified
> above) the rootfs comes up only *after* we run do_initcalls() as such there 
> are
> possible theoretical races even with request_firmware_nowait() -- as such that
> justifies even more the SmPL grammar rule to include even 
> request_firmware_nowait()
> on probe / init as this patch has.
> 
> Anyway yeah that patch has good intentions but its wrong for a slew of 
> reasons.
> The problem is the same as discussed with Bjorn recently. The solution
> discussed is that since only userspace knows when the *real* rootfs is ready
> (because of slew of reasons) the best we can do is have an event issued by
> userspace to inform us of that.

OK I have something I think we can build upon for this. Will send RFC.

  Luis

Re: [PATCH v2 0/3] Add Platform MHU mailbox driver for Amlogic GXBB

[Kevin Hilman]

Hi Jassi,

Neil Armstrong  writes:

> In order to support Mailbox links for the Amlogic GXBB SoC, add a generic
> platform MHU driver based on arm_mhu.c.
>
> This patchset follows a RFC thread along the GXBB SCPI support at :
> http://lkml.kernel.org/r/1466503374-28841-1-git-send-email-narmstr...@baylibre.com
> And specific MHU discussions at :
> http://lkml.kernel.org/r/CABb+yY3HqJG2+GMWCWF9PomxobrwWGZ=tze5nvxpchmddlh...@mail.gmail.com
>
> Changes since v1 at 
> http://lkml.kernel.org/r/1470732737-18391-1-git-send-email-narmstr...@baylibre.com
>  :
>  - Fix irq to signed to detect platform_get_irq() failures
>  - Introduced back the secure channel
>  - Fixed indexes

How is this version looking to you?

With your review/ack on the driver, I'd be happy to take it via the
amlogic tree as there shouldn't be any conflicts with your tree.

Kevin

> Changes since RFC v2 :
>  - Rename to platform_mhu
>  - Sync all link functions with arm_mhu
>
> Neil Armstrong (3):
>   mailbox: Add Platform Message-Handling-Unit variant driver
>   dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
>   ARM64: dts: meson-gxbb: Add Meson MHU Node
>
>  .../devicetree/bindings/mailbox/meson-mhu.txt  |  34 
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi|   9 +
>  drivers/mailbox/Kconfig|  10 +
>  drivers/mailbox/Makefile   |   2 +
>  drivers/mailbox/platform_mhu.c | 205 
> +
>  5 files changed, 260 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
>  create mode 100644 drivers/mailbox/platform_mhu.c

Re: [PATCH v2 0/3] Add Platform MHU mailbox driver for Amlogic GXBB

[Kevin Hilman]

Hi Jassi,

Neil Armstrong  writes:

> In order to support Mailbox links for the Amlogic GXBB SoC, add a generic
> platform MHU driver based on arm_mhu.c.
>
> This patchset follows a RFC thread along the GXBB SCPI support at :
> http://lkml.kernel.org/r/1466503374-28841-1-git-send-email-narmstr...@baylibre.com
> And specific MHU discussions at :
> http://lkml.kernel.org/r/CABb+yY3HqJG2+GMWCWF9PomxobrwWGZ=tze5nvxpchmddlh...@mail.gmail.com
>
> Changes since v1 at 
> http://lkml.kernel.org/r/1470732737-18391-1-git-send-email-narmstr...@baylibre.com
>  :
>  - Fix irq to signed to detect platform_get_irq() failures
>  - Introduced back the secure channel
>  - Fixed indexes

How is this version looking to you?

With your review/ack on the driver, I'd be happy to take it via the
amlogic tree as there shouldn't be any conflicts with your tree.

Kevin

> Changes since RFC v2 :
>  - Rename to platform_mhu
>  - Sync all link functions with arm_mhu
>
> Neil Armstrong (3):
>   mailbox: Add Platform Message-Handling-Unit variant driver
>   dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
>   ARM64: dts: meson-gxbb: Add Meson MHU Node
>
>  .../devicetree/bindings/mailbox/meson-mhu.txt  |  34 
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi|   9 +
>  drivers/mailbox/Kconfig|  10 +
>  drivers/mailbox/Makefile   |   2 +
>  drivers/mailbox/platform_mhu.c | 205 
> +
>  5 files changed, 260 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
>  create mode 100644 drivers/mailbox/platform_mhu.c

Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)

[Chen Gang]

On 9/2/16 04:43, Al Viro wrote:
> On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote:
> 
>> Could you provide the related proof?
>>
>> Or shall I try to analyze about it and get proof?
> 
> Can you show a proof that it actually improves anything?  He who proposes
> a patch gets to defend it, not the other way round...
> 
> Al, bloody annoyed
> 

OK, what you said sounds reasonable to me.

It makes the code more readable since they are really pure Boolean
functions, and let the functions are precisely same in all archs. But
really, I shall try to prove that it has no negative effect.

e.g. for arc arch. now, I have built the arc raw compiler to build arc
kernel, but excuse me, I plan to finish proof next week, because during
these days, I have to work, buy house, and focus on my father's health.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)

[Chen Gang]

On 9/2/16 04:43, Al Viro wrote:
> On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote:
> 
>> Could you provide the related proof?
>>
>> Or shall I try to analyze about it and get proof?
> 
> Can you show a proof that it actually improves anything?  He who proposes
> a patch gets to defend it, not the other way round...
> 
> Al, bloody annoyed
> 

OK, what you said sounds reasonable to me.

It makes the code more readable since they are really pure Boolean
functions, and let the functions are precisely same in all archs. But
really, I shall try to prove that it has no negative effect.

e.g. for arc arch. now, I have built the arc raw compiler to build arc
kernel, but excuse me, I plan to finish proof next week, because during
these days, I have to work, buy house, and focus on my father's health.


Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

How does the size field work in IOCTL numbers?

[Keno Fischer]

Hi folks,

this is more of a general linux question, but since I noticed it
while looking perf_events code, I'm ccing perf_events folks
in case the answer is perf_events specific (hope that's ok).

uapi/linux/perf_event.h has the following:
#define PERF_EVENT_IOC_PERIOD _IOW('$', 4, __u64)
#define PERF_EVENT_IOC_SET_OUTPUT _IO ('$', 5)
#define PERF_EVENT_IOC_SET_FILTER _IOW('$', 6, char *)
#define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *)
#define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32)

Now, my question is how to interpret the last argument to the
_IO* macros. The man page for ioctl(2) says, it encodes the
"size of the argument argp in bytes", but I'm not sure how to
interpret that.

For example, IOC_PERIOD takes a pointer to a 64-bit value,
so the __u64 makes sense for me. But as far as I know,
IOC_ID also takes a pointer to a 64-bit value
(and writes the ID to it), but it has `__64 *` rather than
`__u64`. The we have IOC_SET_FILTER which as far as I know
takes a pointer to a variable-length string (but with the
above definition the size field is sizeof(char*)), and
IOC_SET_BPF which doesn't take a pointer at all,
but interprets the argument as a file descriptor (similar to
IOC_SET_OUTPUT, which doesn't have a size at all).

I don't understand what the rule is for what to put in that third
argument, or is it ioctl specific? Please let me know if I missed
something.

Thanks,
Keno

How does the size field work in IOCTL numbers?

[Keno Fischer]

Hi folks,

this is more of a general linux question, but since I noticed it
while looking perf_events code, I'm ccing perf_events folks
in case the answer is perf_events specific (hope that's ok).

uapi/linux/perf_event.h has the following:
#define PERF_EVENT_IOC_PERIOD _IOW('$', 4, __u64)
#define PERF_EVENT_IOC_SET_OUTPUT _IO ('$', 5)
#define PERF_EVENT_IOC_SET_FILTER _IOW('$', 6, char *)
#define PERF_EVENT_IOC_ID _IOR('$', 7, __u64 *)
#define PERF_EVENT_IOC_SET_BPF _IOW('$', 8, __u32)

Now, my question is how to interpret the last argument to the
_IO* macros. The man page for ioctl(2) says, it encodes the
"size of the argument argp in bytes", but I'm not sure how to
interpret that.

For example, IOC_PERIOD takes a pointer to a 64-bit value,
so the __u64 makes sense for me. But as far as I know,
IOC_ID also takes a pointer to a 64-bit value
(and writes the ID to it), but it has `__64 *` rather than
`__u64`. The we have IOC_SET_FILTER which as far as I know
takes a pointer to a variable-length string (but with the
above definition the size field is sizeof(char*)), and
IOC_SET_BPF which doesn't take a pointer at all,
but interprets the argument as a file descriptor (similar to
IOC_SET_OUTPUT, which doesn't have a size at all).

I don't understand what the rule is for what to put in that third
argument, or is it ioctl specific? Please let me know if I missed
something.

Thanks,
Keno

[PATCH] crypto: mv_cesa: remove NO_IRQ reference

[Arnd Bergmann]

Drivers should not use NO_IRQ, as we are trying to get rid of that.
In this case, the call to irq_of_parse_and_map() is both wrong
(as it returns '0' on failure, not NO_IRQ) and unnecessary
(as platform_get_irq() does the same thing)

This removes the call to irq_of_parse_and_map() and checks for
the error code correctly.

Signed-off-by: Arnd Bergmann 
---
 drivers/crypto/mv_cesa.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

It would be good if someone could test this on a machine that boots
from DT to ensure the conversion was correct.

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index e6b658faef63..104e9ce9400a 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -1091,11 +1091,8 @@ static int mv_probe(struct platform_device *pdev)
 
cp->max_req_size = cp->sram_size - SRAM_CFG_SPACE;
 
-   if (pdev->dev.of_node)
-   irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-   else
-   irq = platform_get_irq(pdev, 0);
-   if (irq < 0 || irq == NO_IRQ) {
+   irq = platform_get_irq(pdev, 0);
+   if (irq < 0) {
ret = irq;
goto err;
}
-- 
2.9.0

[PATCH] crypto: mv_cesa: remove NO_IRQ reference

[Arnd Bergmann]

Drivers should not use NO_IRQ, as we are trying to get rid of that.
In this case, the call to irq_of_parse_and_map() is both wrong
(as it returns '0' on failure, not NO_IRQ) and unnecessary
(as platform_get_irq() does the same thing)

This removes the call to irq_of_parse_and_map() and checks for
the error code correctly.

Signed-off-by: Arnd Bergmann 
---
 drivers/crypto/mv_cesa.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

It would be good if someone could test this on a machine that boots
from DT to ensure the conversion was correct.

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index e6b658faef63..104e9ce9400a 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -1091,11 +1091,8 @@ static int mv_probe(struct platform_device *pdev)
 
cp->max_req_size = cp->sram_size - SRAM_CFG_SPACE;
 
-   if (pdev->dev.of_node)
-   irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-   else
-   irq = platform_get_irq(pdev, 0);
-   if (irq < 0 || irq == NO_IRQ) {
+   irq = platform_get_irq(pdev, 0);
+   if (irq < 0) {
ret = irq;
goto err;
}
-- 
2.9.0

[PATCH] dmaengine: ipu: remove bogus NO_IRQ reference

[Arnd Bergmann]

A workaround for a warning introduced a use of the NO_IRQ
macro that should have been gone for a long time.

It is clear from the code that the value cannot actually
be used, but apparently there was a configuration at
some point that caused a warning, so instead of just
reverting that patch, this rearranges the code in a way that
the warning cannot reappear.

Signed-off-by: Arnd Bergmann 
Fixes: 6ef41cf6f721 ("dmaengine :ipu: change ipu_irq_handler() to remove 
compile warning")
---
 drivers/dma/ipu/ipu_irq.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/ipu/ipu_irq.c b/drivers/dma/ipu/ipu_irq.c
index 2bf37e68ad0f..dd184b50e5b4 100644
--- a/drivers/dma/ipu/ipu_irq.c
+++ b/drivers/dma/ipu/ipu_irq.c
@@ -286,22 +286,21 @@ static void ipu_irq_handler(struct irq_desc *desc)
raw_spin_unlock(_lock);
while ((line = ffs(status))) {
struct ipu_irq_map *map;
-   unsigned int irq = NO_IRQ;
+   unsigned int irq;
 
line--;
status &= ~(1UL << line);
 
raw_spin_lock(_lock);
map = src2map(32 * i + line);
-   if (map)
-   irq = map->irq;
-   raw_spin_unlock(_lock);
-
if (!map) {
+   raw_spin_unlock(_lock);
pr_err("IPU: Interrupt on unmapped source %u 
bank %d\n",
   line, i);
continue;
}
+   irq = map->irq;
+   raw_spin_unlock(_lock);
generic_handle_irq(irq);
}
}
-- 
2.9.0

[PATCH] dmaengine: ipu: remove bogus NO_IRQ reference

[Arnd Bergmann]

A workaround for a warning introduced a use of the NO_IRQ
macro that should have been gone for a long time.

It is clear from the code that the value cannot actually
be used, but apparently there was a configuration at
some point that caused a warning, so instead of just
reverting that patch, this rearranges the code in a way that
the warning cannot reappear.

Signed-off-by: Arnd Bergmann 
Fixes: 6ef41cf6f721 ("dmaengine :ipu: change ipu_irq_handler() to remove 
compile warning")
---
 drivers/dma/ipu/ipu_irq.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/ipu/ipu_irq.c b/drivers/dma/ipu/ipu_irq.c
index 2bf37e68ad0f..dd184b50e5b4 100644
--- a/drivers/dma/ipu/ipu_irq.c
+++ b/drivers/dma/ipu/ipu_irq.c
@@ -286,22 +286,21 @@ static void ipu_irq_handler(struct irq_desc *desc)
raw_spin_unlock(_lock);
while ((line = ffs(status))) {
struct ipu_irq_map *map;
-   unsigned int irq = NO_IRQ;
+   unsigned int irq;
 
line--;
status &= ~(1UL << line);
 
raw_spin_lock(_lock);
map = src2map(32 * i + line);
-   if (map)
-   irq = map->irq;
-   raw_spin_unlock(_lock);
-
if (!map) {
+   raw_spin_unlock(_lock);
pr_err("IPU: Interrupt on unmapped source %u 
bank %d\n",
   line, i);
continue;
}
+   irq = map->irq;
+   raw_spin_unlock(_lock);
generic_handle_irq(irq);
}
}
-- 
2.9.0

[PATCH] dmaengine: sirf: fix irq number error check

[Arnd Bergmann]

irq_of_parse_and_map() returns 0 on error, no NO_IRQ, so the
failure condition can never be met.

This changes the comparison to check for zero instead.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/sirf-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
index a96e4a480de5..8f62edad51be 100644
--- a/drivers/dma/sirf-dma.c
+++ b/drivers/dma/sirf-dma.c
@@ -866,7 +866,7 @@ static int sirfsoc_dma_probe(struct platform_device *op)
}
 
sdma->irq = irq_of_parse_and_map(dn, 0);
-   if (sdma->irq == NO_IRQ) {
+   if (!sdma->irq) {
dev_err(dev, "Error mapping IRQ!\n");
return -EINVAL;
}
-- 
2.9.0

[PATCH] dmaengine: sirf: fix irq number error check

[Arnd Bergmann]

irq_of_parse_and_map() returns 0 on error, no NO_IRQ, so the
failure condition can never be met.

This changes the comparison to check for zero instead.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/sirf-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
index a96e4a480de5..8f62edad51be 100644
--- a/drivers/dma/sirf-dma.c
+++ b/drivers/dma/sirf-dma.c
@@ -866,7 +866,7 @@ static int sirfsoc_dma_probe(struct platform_device *op)
}
 
sdma->irq = irq_of_parse_and_map(dn, 0);
-   if (sdma->irq == NO_IRQ) {
+   if (!sdma->irq) {
dev_err(dev, "Error mapping IRQ!\n");
return -EINVAL;
}
-- 
2.9.0

[PATCH] dmaengine: mxs: remove NO_IRQ check

[Arnd Bergmann]

The mxs_chan->chan_irq variable is guaranteed to never be NO_IRQ,
as it gets assigned the result of platform_get_irq() that returns
either a valid positive interrupt number, or a negative failure
code that leads to the channel not being used.

This removes the redundant check, eliminating one more instance
of NO_IRQ.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/mxs-dma.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 50e64e113ffb..e217268c7098 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -428,12 +428,10 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan 
*chan)
goto err_alloc;
}
 
-   if (mxs_chan->chan_irq != NO_IRQ) {
-   ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
-   0, "mxs-dma", mxs_dma);
-   if (ret)
-   goto err_irq;
-   }
+   ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
+ 0, "mxs-dma", mxs_dma);
+   if (ret)
+   goto err_irq;
 
ret = clk_prepare_enable(mxs_dma->clk);
if (ret)
-- 
2.9.0

[PATCH] dmaengine: mxs: remove NO_IRQ check

[Arnd Bergmann]

The mxs_chan->chan_irq variable is guaranteed to never be NO_IRQ,
as it gets assigned the result of platform_get_irq() that returns
either a valid positive interrupt number, or a negative failure
code that leads to the channel not being used.

This removes the redundant check, eliminating one more instance
of NO_IRQ.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/mxs-dma.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 50e64e113ffb..e217268c7098 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -428,12 +428,10 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan 
*chan)
goto err_alloc;
}
 
-   if (mxs_chan->chan_irq != NO_IRQ) {
-   ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
-   0, "mxs-dma", mxs_dma);
-   if (ret)
-   goto err_irq;
-   }
+   ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
+ 0, "mxs-dma", mxs_dma);
+   if (ret)
+   goto err_irq;
 
ret = clk_prepare_enable(mxs_dma->clk);
if (ret)
-- 
2.9.0

[PATCH 2/2] hso: Convert printk to pr_

[Joe Perches]

Use a more common logging style

Miscellanea:

o Add pr_fmt to prefix each output message
o Realign arguments

Signed-off-by: Joe Perches 
---
 drivers/net/usb/hso.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 6c37512..e7b5163 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -50,6 +50,8 @@
  *
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 #include 
 #include 
@@ -638,7 +640,7 @@ static int get_free_serial_index(void)
}
spin_unlock_irqrestore(_table_lock, flags);
 
-   printk(KERN_ERR "%s: no free serial devices in table\n", __func__);
+   pr_err("%s: no free serial devices in table\n", __func__);
return -1;
 }
 
@@ -1102,7 +1104,7 @@ static void _hso_serial_set_termios(struct tty_struct 
*tty,
struct hso_serial *serial = tty->driver_data;
 
if (!serial) {
-   printk(KERN_ERR "%s: no tty structures", __func__);
+   pr_err("%s: no tty structures", __func__);
return;
}
 
@@ -1347,7 +1349,7 @@ static int hso_serial_write(struct tty_struct *tty, const 
unsigned char *buf,
 
/* sanity check */
if (serial == NULL) {
-   printk(KERN_ERR "%s: serial is NULL\n", __func__);
+   pr_err("%s: serial is NULL\n", __func__);
return -ENODEV;
}
 
@@ -1773,7 +1775,7 @@ static int mux_device_request(struct hso_serial *serial, 
u8 type, u16 port,
 
/* Sanity check */
if (!serial || !ctrl_urb || !ctrl_req) {
-   printk(KERN_ERR "%s: Wrong arguments\n", __func__);
+   pr_err("%s: Wrong arguments\n", __func__);
return -EINVAL;
}
 
@@ -3220,7 +3222,7 @@ static int __init hso_init(void)
int result;
 
/* put it in the log */
-   printk(KERN_INFO "hso: %s\n", version);
+   pr_info("%s\n", version);
 
/* Initialise the serial table semaphore and table */
spin_lock_init(_table_lock);
@@ -3251,16 +3253,15 @@ static int __init hso_init(void)
/* register the tty driver */
result = tty_register_driver(tty_drv);
if (result) {
-   printk(KERN_ERR "%s - tty_register_driver failed(%d)\n",
-   __func__, result);
+   pr_err("%s - tty_register_driver failed(%d)\n",
+  __func__, result);
goto err_free_tty;
}
 
/* register this module as an usb driver */
result = usb_register(_driver);
if (result) {
-   printk(KERN_ERR "Could not register hso driver? error: %d\n",
-   result);
+   pr_err("Could not register hso driver - error: %d\n", result);
goto err_unreg_tty;
}
 
@@ -3275,7 +3276,7 @@ err_free_tty:
 
 static void __exit hso_exit(void)
 {
-   printk(KERN_INFO "hso: unloaded\n");
+   pr_info("unloaded\n");
 
tty_unregister_driver(tty_drv);
put_tty_driver(tty_drv);
-- 
2.10.0.rc2.1.g053435c

[PATCH 1/2] hso: Use a more common logging style

[Joe Perches]

Macros that end in an underscore are just odd.
Add hso_dbg(level, fmt, ...) and use it everwhere instead.

Several uses had additional unnecessary newlines as the
macro added a newline.  Remove the newline from the macro
and add newlines to each use as appropriate.

Remove now unused D macros.

Signed-off-by: Joe Perches 
---
 drivers/net/usb/hso.c | 97 +++
 1 file changed, 44 insertions(+), 53 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index c5544d3..6c37512 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -108,23 +108,12 @@
 /*/
 /* Debugging functions   */
 /*/
-#define D__(lvl_, fmt, arg...) \
-   do {\
-   printk(lvl_ "[%d:%s]: " fmt "\n",   \
-  __LINE__, __func__, ## arg); \
-   } while (0)
-
-#define D_(lvl, args...)   \
-   do {\
-   if (lvl & debug)\
-   D__(KERN_INFO, args);   \
-   } while (0)
-
-#define D1(args...)D_(0x01, ##args)
-#define D2(args...)D_(0x02, ##args)
-#define D3(args...)D_(0x04, ##args)
-#define D4(args...)D_(0x08, ##args)
-#define D5(args...)D_(0x10, ##args)
+#define hso_dbg(lvl, fmt, ...) \
+do {   \
+   if ((lvl) & debug)  \
+   pr_info("[%d:%s] " fmt, \
+   __LINE__, __func__, ##__VA_ARGS__); \
+} while (0)
 
 /*/
 /* Enumerators   */
@@ -709,7 +698,8 @@ static void handle_usb_error(int status, const char 
*function,
}
 
/* log a meaningful explanation of an USB status */
-   D1("%s: received USB status - %s (%d)", function, explanation, status);
+   hso_dbg(0x1, "%s: received USB status - %s (%d)\n",
+   function, explanation, status);
 }
 
 /* Network interface functions */
@@ -808,7 +798,7 @@ static netdev_tx_t hso_net_start_xmit(struct sk_buff *skb,
DUMP1(skb->data, skb->len);
/* Copy it from kernel memory to OUR memory */
memcpy(odev->mux_bulk_tx_buf, skb->data, skb->len);
-   D1("len: %d/%d", skb->len, MUX_BULK_TX_BUF_SIZE);
+   hso_dbg(0x1, "len: %d/%d\n", skb->len, MUX_BULK_TX_BUF_SIZE);
 
/* Fill in the URB for shipping it out. */
usb_fill_bulk_urb(odev->mux_bulk_tx_urb,
@@ -872,7 +862,7 @@ static void packetizeRx(struct hso_net *odev, unsigned char 
*ip_pkt,
unsigned char *tmp_rx_buf;
 
/* log if needed */
-   D1("Rx %d bytes", count);
+   hso_dbg(0x1, "Rx %d bytes\n", count);
DUMP(ip_pkt, min(128, (int)count));
 
while (count) {
@@ -912,7 +902,7 @@ static void packetizeRx(struct hso_net *odev, unsigned char 
*ip_pkt,
frame_len);
if (!odev->skb_rx_buf) {
/* We got no receive buffer. */
-   D1("could not allocate memory");
+   hso_dbg(0x1, "could not allocate 
memory\n");
odev->rx_parse_state = WAIT_SYNC;
continue;
}
@@ -972,11 +962,11 @@ static void packetizeRx(struct hso_net *odev, unsigned 
char *ip_pkt,
break;
 
case WAIT_SYNC:
-   D1(" W_S");
+   hso_dbg(0x1, " W_S\n");
count = 0;
break;
default:
-   D1(" ");
+   hso_dbg(0x1, "\n");
count--;
break;
}
@@ -1020,7 +1010,7 @@ static void read_bulk_callback(struct urb *urb)
 
/* Sanity check */
if (!odev || !test_bit(HSO_NET_RUNNING, >flags)) {
-   D1("BULK IN callback but driver is not active!");
+   hso_dbg(0x1, "BULK IN callback but driver is not active!\n");
return;
}
usb_mark_last_busy(urb->dev);
@@ -1116,7 +1106,7 @@ static void _hso_serial_set_termios(struct tty_struct 
*tty,
return;
}
 
-   D4("port %d", serial->minor);
+   hso_dbg(0x8, "port %d\n", serial->minor);
 
/*
 

[PATCH 2/2] hso: Convert printk to pr_

[Joe Perches]

Use a more common logging style

Miscellanea:

o Add pr_fmt to prefix each output message
o Realign arguments

Signed-off-by: Joe Perches 
---
 drivers/net/usb/hso.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 6c37512..e7b5163 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -50,6 +50,8 @@
  *
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 #include 
 #include 
@@ -638,7 +640,7 @@ static int get_free_serial_index(void)
}
spin_unlock_irqrestore(_table_lock, flags);
 
-   printk(KERN_ERR "%s: no free serial devices in table\n", __func__);
+   pr_err("%s: no free serial devices in table\n", __func__);
return -1;
 }
 
@@ -1102,7 +1104,7 @@ static void _hso_serial_set_termios(struct tty_struct 
*tty,
struct hso_serial *serial = tty->driver_data;
 
if (!serial) {
-   printk(KERN_ERR "%s: no tty structures", __func__);
+   pr_err("%s: no tty structures", __func__);
return;
}
 
@@ -1347,7 +1349,7 @@ static int hso_serial_write(struct tty_struct *tty, const 
unsigned char *buf,
 
/* sanity check */
if (serial == NULL) {
-   printk(KERN_ERR "%s: serial is NULL\n", __func__);
+   pr_err("%s: serial is NULL\n", __func__);
return -ENODEV;
}
 
@@ -1773,7 +1775,7 @@ static int mux_device_request(struct hso_serial *serial, 
u8 type, u16 port,
 
/* Sanity check */
if (!serial || !ctrl_urb || !ctrl_req) {
-   printk(KERN_ERR "%s: Wrong arguments\n", __func__);
+   pr_err("%s: Wrong arguments\n", __func__);
return -EINVAL;
}
 
@@ -3220,7 +3222,7 @@ static int __init hso_init(void)
int result;
 
/* put it in the log */
-   printk(KERN_INFO "hso: %s\n", version);
+   pr_info("%s\n", version);
 
/* Initialise the serial table semaphore and table */
spin_lock_init(_table_lock);
@@ -3251,16 +3253,15 @@ static int __init hso_init(void)
/* register the tty driver */
result = tty_register_driver(tty_drv);
if (result) {
-   printk(KERN_ERR "%s - tty_register_driver failed(%d)\n",
-   __func__, result);
+   pr_err("%s - tty_register_driver failed(%d)\n",
+  __func__, result);
goto err_free_tty;
}
 
/* register this module as an usb driver */
result = usb_register(_driver);
if (result) {
-   printk(KERN_ERR "Could not register hso driver? error: %d\n",
-   result);
+   pr_err("Could not register hso driver - error: %d\n", result);
goto err_unreg_tty;
}
 
@@ -3275,7 +3276,7 @@ err_free_tty:
 
 static void __exit hso_exit(void)
 {
-   printk(KERN_INFO "hso: unloaded\n");
+   pr_info("unloaded\n");
 
tty_unregister_driver(tty_drv);
put_tty_driver(tty_drv);
-- 
2.10.0.rc2.1.g053435c

[PATCH 1/2] hso: Use a more common logging style

[Joe Perches]

Macros that end in an underscore are just odd.
Add hso_dbg(level, fmt, ...) and use it everwhere instead.

Several uses had additional unnecessary newlines as the
macro added a newline.  Remove the newline from the macro
and add newlines to each use as appropriate.

Remove now unused D macros.

Signed-off-by: Joe Perches 
---
 drivers/net/usb/hso.c | 97 +++
 1 file changed, 44 insertions(+), 53 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index c5544d3..6c37512 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -108,23 +108,12 @@
 /*/
 /* Debugging functions   */
 /*/
-#define D__(lvl_, fmt, arg...) \
-   do {\
-   printk(lvl_ "[%d:%s]: " fmt "\n",   \
-  __LINE__, __func__, ## arg); \
-   } while (0)
-
-#define D_(lvl, args...)   \
-   do {\
-   if (lvl & debug)\
-   D__(KERN_INFO, args);   \
-   } while (0)
-
-#define D1(args...)D_(0x01, ##args)
-#define D2(args...)D_(0x02, ##args)
-#define D3(args...)D_(0x04, ##args)
-#define D4(args...)D_(0x08, ##args)
-#define D5(args...)D_(0x10, ##args)
+#define hso_dbg(lvl, fmt, ...) \
+do {   \
+   if ((lvl) & debug)  \
+   pr_info("[%d:%s] " fmt, \
+   __LINE__, __func__, ##__VA_ARGS__); \
+} while (0)
 
 /*/
 /* Enumerators   */
@@ -709,7 +698,8 @@ static void handle_usb_error(int status, const char 
*function,
}
 
/* log a meaningful explanation of an USB status */
-   D1("%s: received USB status - %s (%d)", function, explanation, status);
+   hso_dbg(0x1, "%s: received USB status - %s (%d)\n",
+   function, explanation, status);
 }
 
 /* Network interface functions */
@@ -808,7 +798,7 @@ static netdev_tx_t hso_net_start_xmit(struct sk_buff *skb,
DUMP1(skb->data, skb->len);
/* Copy it from kernel memory to OUR memory */
memcpy(odev->mux_bulk_tx_buf, skb->data, skb->len);
-   D1("len: %d/%d", skb->len, MUX_BULK_TX_BUF_SIZE);
+   hso_dbg(0x1, "len: %d/%d\n", skb->len, MUX_BULK_TX_BUF_SIZE);
 
/* Fill in the URB for shipping it out. */
usb_fill_bulk_urb(odev->mux_bulk_tx_urb,
@@ -872,7 +862,7 @@ static void packetizeRx(struct hso_net *odev, unsigned char 
*ip_pkt,
unsigned char *tmp_rx_buf;
 
/* log if needed */
-   D1("Rx %d bytes", count);
+   hso_dbg(0x1, "Rx %d bytes\n", count);
DUMP(ip_pkt, min(128, (int)count));
 
while (count) {
@@ -912,7 +902,7 @@ static void packetizeRx(struct hso_net *odev, unsigned char 
*ip_pkt,
frame_len);
if (!odev->skb_rx_buf) {
/* We got no receive buffer. */
-   D1("could not allocate memory");
+   hso_dbg(0x1, "could not allocate 
memory\n");
odev->rx_parse_state = WAIT_SYNC;
continue;
}
@@ -972,11 +962,11 @@ static void packetizeRx(struct hso_net *odev, unsigned 
char *ip_pkt,
break;
 
case WAIT_SYNC:
-   D1(" W_S");
+   hso_dbg(0x1, " W_S\n");
count = 0;
break;
default:
-   D1(" ");
+   hso_dbg(0x1, "\n");
count--;
break;
}
@@ -1020,7 +1010,7 @@ static void read_bulk_callback(struct urb *urb)
 
/* Sanity check */
if (!odev || !test_bit(HSO_NET_RUNNING, >flags)) {
-   D1("BULK IN callback but driver is not active!");
+   hso_dbg(0x1, "BULK IN callback but driver is not active!\n");
return;
}
usb_mark_last_busy(urb->dev);
@@ -1116,7 +1106,7 @@ static void _hso_serial_set_termios(struct tty_struct 
*tty,
return;
}
 
-   D4("port %d", serial->minor);
+   hso_dbg(0x8, "port %d\n", serial->minor);
 
/*
 *  

[PATCH 0/2] hso: neatening

[Joe Perches]

This seems to be the only code in the kernel that uses
macro defines with a trailing underscore.  Fix that.

Joe Perches (2):
  hso: Use a more common logging style
  hso: Convert printk to pr_

 drivers/net/usb/hso.c | 118 +++---
 1 file changed, 55 insertions(+), 63 deletions(-)

-- 
2.10.0.rc2.1.g053435c

[PATCH 0/2] hso: neatening

[Joe Perches]

This seems to be the only code in the kernel that uses
macro defines with a trailing underscore.  Fix that.

Joe Perches (2):
  hso: Use a more common logging style
  hso: Convert printk to pr_

 drivers/net/usb/hso.c | 118 +++---
 1 file changed, 55 insertions(+), 63 deletions(-)

-- 
2.10.0.rc2.1.g053435c

Re: [PATCH] virtio-blk: Generate uevent after attribute available

[Michael S. Tsirkin]

On Wed, Jun 29, 2016 at 09:24:15AM +0800, Fam Zheng wrote:
> On Tue, 06/28 04:45, Christoph Hellwig wrote:
> > On Tue, Jun 28, 2016 at 10:39:15AM +0800, Fam Zheng wrote:
> > > Userspace listens to the KOBJ_ADD uevent generated in add_disk. At that
> > > point we haven't created the serial attribute file, therefore depending
> > > on how fast udev reacts, the /dev/disk/by-id/ entry doesn't always get
> > > created.
> > > 
> > > This race condition can be easily reproduced by hot plugging a number of
> > > virtio-blk disks.
> > > 
> > > Also in systemd, there used to be a related workaround in udev rules
> > > called 'WAIT_FOR="serial"', but it is removed in later versions.
> > > 
> > > Now let's generate a KOBJ_CHANGE event after the attributes are ready.
> > 
> > The same race is present in other drivers as well, e.g. nvme.  Please
> > find a way to make this work properly without needing to hack every
> > driver to send events manually.
> 
> OK, I'll take a look today!
> 
> Fam

Was this fixed in the generic code?

-- 
MST

Re: [PATCH] virtio-blk: Generate uevent after attribute available

[Michael S. Tsirkin]

On Wed, Jun 29, 2016 at 09:24:15AM +0800, Fam Zheng wrote:
> On Tue, 06/28 04:45, Christoph Hellwig wrote:
> > On Tue, Jun 28, 2016 at 10:39:15AM +0800, Fam Zheng wrote:
> > > Userspace listens to the KOBJ_ADD uevent generated in add_disk. At that
> > > point we haven't created the serial attribute file, therefore depending
> > > on how fast udev reacts, the /dev/disk/by-id/ entry doesn't always get
> > > created.
> > > 
> > > This race condition can be easily reproduced by hot plugging a number of
> > > virtio-blk disks.
> > > 
> > > Also in systemd, there used to be a related workaround in udev rules
> > > called 'WAIT_FOR="serial"', but it is removed in later versions.
> > > 
> > > Now let's generate a KOBJ_CHANGE event after the attributes are ready.
> > 
> > The same race is present in other drivers as well, e.g. nvme.  Please
> > find a way to make this work properly without needing to hack every
> > driver to send events manually.
> 
> OK, I'll take a look today!
> 
> Fam

Was this fixed in the generic code?

-- 
MST

Re: screen rotation flipped in 4.8-rc

[Srinivas Pandruvada]

On Tue, 2016-08-30 at 22:42 +, Pandruvada, Srinivas wrote:
> Hi All,
> 
> I observed that using iio-sensor-proxy.service, the auto screen
> rotation flipped on my laptop (Normal -> vertical, vertical->normal)
> using kernel v4.8.
> 
> Anyone else has seen this?
> 
> I did a bisect and found a commit, which I am not sure how can it
> impact.
> 
> 
> commit 703b5faf22fbddf984a361e6555f3a03fdba63d9
> Author: George Spelvin 
> Date:   Fri Jun 10 00:22:12 2016 -0400
> 
[...]

It turns out to be some assumption user space program is making about
the traversing directory using glib call g_dir_read_name(). 

With the commit 703b5faf22fbddf984a361e6555f3a03fdba63d9 (fs/dcache.c:
Save one 32-bit multiply in dcache lookup)
 in kernel 4.8-rc, somehow the order is changed (so the in_accel_y was
appearing before in_accel_x )

I modified user space program to use correct iio scan element index to
determine byte offset instead depending on the glib_dir_read_name,
which doesn't guarantee any order.

I sent a pull request to author of iio-sensor-proxy to review.

Hadess,
Please look.

https://github.com/hadess/iio-sensor-proxy/pull/99/commits/de80c50b2678
2ba6e899ee5a95b31b28790c940d


Thanks,
Srinivas

Re: screen rotation flipped in 4.8-rc

[Srinivas Pandruvada]

On Tue, 2016-08-30 at 22:42 +, Pandruvada, Srinivas wrote:
> Hi All,
> 
> I observed that using iio-sensor-proxy.service, the auto screen
> rotation flipped on my laptop (Normal -> vertical, vertical->normal)
> using kernel v4.8.
> 
> Anyone else has seen this?
> 
> I did a bisect and found a commit, which I am not sure how can it
> impact.
> 
> 
> commit 703b5faf22fbddf984a361e6555f3a03fdba63d9
> Author: George Spelvin 
> Date:   Fri Jun 10 00:22:12 2016 -0400
> 
[...]

It turns out to be some assumption user space program is making about
the traversing directory using glib call g_dir_read_name(). 

With the commit 703b5faf22fbddf984a361e6555f3a03fdba63d9 (fs/dcache.c:
Save one 32-bit multiply in dcache lookup)
 in kernel 4.8-rc, somehow the order is changed (so the in_accel_y was
appearing before in_accel_x )

I modified user space program to use correct iio scan element index to
determine byte offset instead depending on the glib_dir_read_name,
which doesn't guarantee any order.

I sent a pull request to author of iio-sensor-proxy to review.

Hadess,
Please look.

https://github.com/hadess/iio-sensor-proxy/pull/99/commits/de80c50b2678
2ba6e899ee5a95b31b28790c940d


Thanks,
Srinivas

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 01:35:22AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > > it can be safely move to drivers/char/tpm/tpm.h.
> > 
> > No, this is the wrong direction.
> > 
> > The goal is to make things more like other subsystems, so we should be
> > moving struct tpm_chip into the public header, and that requires ops
> > to be in the public header.
> > 
> > This is why I put ops here in the first place.
> 
> I'm OK with it as long as you explain why this is necessary. I see no
> use for them outside the TPM subsystem.

That is because the users out side the subsystem are Doing it Wrong.

eg this:

 extern int tpm_is_tpm2(u32 chip_num);

Should be:

 extern int tpm_is_tpm2(struct tpm_chip *chip);

And same for all other examples.

The 'chip_num' thing is bonkers.

Jason

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 01:35:22AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > > it can be safely move to drivers/char/tpm/tpm.h.
> > 
> > No, this is the wrong direction.
> > 
> > The goal is to make things more like other subsystems, so we should be
> > moving struct tpm_chip into the public header, and that requires ops
> > to be in the public header.
> > 
> > This is why I put ops here in the first place.
> 
> I'm OK with it as long as you explain why this is necessary. I see no
> use for them outside the TPM subsystem.

That is because the users out side the subsystem are Doing it Wrong.

eg this:

 extern int tpm_is_tpm2(u32 chip_num);

Should be:

 extern int tpm_is_tpm2(struct tpm_chip *chip);

And same for all other examples.

The 'chip_num' thing is bonkers.

Jason

Re: [PATCH] doc: ioctl: Add some clarifications to botching-up-ioctls

[Arnd Bergmann]

On Friday, September 2, 2016 3:42:24 PM CEST Laura Abbott wrote:
> - The guide currently says to pad the structure to a multiple of
>   64-bits. This is not necessary in cases where the structure contains
>   no 64-bit types. Clarify this concept to avoid unnecessary padding.
> - When using __u64 to hold user pointers, blindly trying to do a cast to
>   a void __user * may generate a warning on 32-bit systems about a cast
>   from an integer to a pointer of different size. There is a macro to
>   deal with this which hides an ugly double cast. Add a reference to
>   this macro.
> 
> Signed-off-by: Laura Abbott 
> 

Looks good to me,

Acked-by: Arnd Bergmann 

Re: [PATCH] doc: ioctl: Add some clarifications to botching-up-ioctls

[Arnd Bergmann]

On Friday, September 2, 2016 3:42:24 PM CEST Laura Abbott wrote:
> - The guide currently says to pad the structure to a multiple of
>   64-bits. This is not necessary in cases where the structure contains
>   no 64-bit types. Clarify this concept to avoid unnecessary padding.
> - When using __u64 to hold user pointers, blindly trying to do a cast to
>   a void __user * may generate a warning on 32-bit systems about a cast
>   from an integer to a pointer of different size. There is a macro to
>   deal with this which hides an ugly double cast. Add a reference to
>   this macro.
> 
> Signed-off-by: Laura Abbott 
> 

Looks good to me,

Acked-by: Arnd Bergmann 

[PATCH] doc: ioctl: Add some clarifications to botching-up-ioctls

[Laura Abbott]

- The guide currently says to pad the structure to a multiple of
  64-bits. This is not necessary in cases where the structure contains
  no 64-bit types. Clarify this concept to avoid unnecessary padding.
- When using __u64 to hold user pointers, blindly trying to do a cast to
  a void __user * may generate a warning on 32-bit systems about a cast
  from an integer to a pointer of different size. There is a macro to
  deal with this which hides an ugly double cast. Add a reference to
  this macro.

Signed-off-by: Laura Abbott 
---
I can split these into two separate patches if necessary but it seemed simpler
just to colapse them into a single patch.
---
 Documentation/ioctl/botching-up-ioctls.txt | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/ioctl/botching-up-ioctls.txt 
b/Documentation/ioctl/botching-up-ioctls.txt
index cc30b14..36138c6 100644
--- a/Documentation/ioctl/botching-up-ioctls.txt
+++ b/Documentation/ioctl/botching-up-ioctls.txt
@@ -34,15 +34,18 @@ will need to add a a 32-bit compat layer:
64-bit platforms do. So we always need padding to the natural size to get
this right.
 
- * Pad the entire struct to a multiple of 64-bits - the structure size will
-   otherwise differ on 32-bit versus 64-bit. Having a different structure size
-   hurts when passing arrays of structures to the kernel, or if the kernel
-   checks the structure size, which e.g. the drm core does.
+ * Pad the entire struct to a multiple of 64-bits if the structure contains
+   64-bit types - the structure size will otherwise differ on 32-bit versus
+   64-bit. Having a different structure size hurts when passing arrays of
+   structures to the kernel, or if the kernel checks the structure size, which
+   e.g. the drm core does.
 
  * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
from/to a void __user * in the kernel. Try really hard not to delay this
conversion or worse, fiddle the raw __u64 through your code since that
-   diminishes the checking tools like sparse can provide.
+   diminishes the checking tools like sparse can provide. The macro
+   u64_to_user_ptr can be used in the kernel to avoid warnings about integers
+   and pointres of different sizes.
 
 
 Basics
-- 
2.7.4

[PATCH] doc: ioctl: Add some clarifications to botching-up-ioctls

[Laura Abbott]

- The guide currently says to pad the structure to a multiple of
  64-bits. This is not necessary in cases where the structure contains
  no 64-bit types. Clarify this concept to avoid unnecessary padding.
- When using __u64 to hold user pointers, blindly trying to do a cast to
  a void __user * may generate a warning on 32-bit systems about a cast
  from an integer to a pointer of different size. There is a macro to
  deal with this which hides an ugly double cast. Add a reference to
  this macro.

Signed-off-by: Laura Abbott 
---
I can split these into two separate patches if necessary but it seemed simpler
just to colapse them into a single patch.
---
 Documentation/ioctl/botching-up-ioctls.txt | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/ioctl/botching-up-ioctls.txt 
b/Documentation/ioctl/botching-up-ioctls.txt
index cc30b14..36138c6 100644
--- a/Documentation/ioctl/botching-up-ioctls.txt
+++ b/Documentation/ioctl/botching-up-ioctls.txt
@@ -34,15 +34,18 @@ will need to add a a 32-bit compat layer:
64-bit platforms do. So we always need padding to the natural size to get
this right.
 
- * Pad the entire struct to a multiple of 64-bits - the structure size will
-   otherwise differ on 32-bit versus 64-bit. Having a different structure size
-   hurts when passing arrays of structures to the kernel, or if the kernel
-   checks the structure size, which e.g. the drm core does.
+ * Pad the entire struct to a multiple of 64-bits if the structure contains
+   64-bit types - the structure size will otherwise differ on 32-bit versus
+   64-bit. Having a different structure size hurts when passing arrays of
+   structures to the kernel, or if the kernel checks the structure size, which
+   e.g. the drm core does.
 
  * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
from/to a void __user * in the kernel. Try really hard not to delay this
conversion or worse, fiddle the raw __u64 through your code since that
-   diminishes the checking tools like sparse can provide.
+   diminishes the checking tools like sparse can provide. The macro
+   u64_to_user_ptr can be used in the kernel to avoid warnings about integers
+   and pointres of different sizes.
 
 
 Basics
-- 
2.7.4

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > it can be safely move to drivers/char/tpm/tpm.h.
> 
> No, this is the wrong direction.
> 
> The goal is to make things more like other subsystems, so we should be
> moving struct tpm_chip into the public header, and that requires ops
> to be in the public header.
> 
> This is why I put ops here in the first place.

I'm OK with it as long as you explain why this is necessary. I see no
use for them outside the TPM subsystem.

> Jason

/Jarkko

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > it can be safely move to drivers/char/tpm/tpm.h.
> 
> No, this is the wrong direction.
> 
> The goal is to make things more like other subsystems, so we should be
> moving struct tpm_chip into the public header, and that requires ops
> to be in the public header.
> 
> This is why I put ops here in the first place.

I'm OK with it as long as you explain why this is necessary. I see no
use for them outside the TPM subsystem.

> Jason

/Jarkko

[GIT PULL] TPM bugfix

[James Morris]

Please pull this fix for the TPM code.


The following changes since commit 15301a570754c7af60335d094dd2d1808b0641a5:

  x86/paravirt: Do not trace _paravirt_ident_*() functions (2016-09-02 09:40:47 
-0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git 
for-linus

Jarkko Sakkinen (1):
  tpm: invalid self test error message

 drivers/char/tpm/tpm2-cmd.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

---

commit 85bd4528b88838a26b078ee3895c3cf3ad95b03c
Author: Jarkko Sakkinen 
Date:   Fri Sep 2 02:36:58 2016 +0300

tpm: invalid self test error message

The driver emits invalid self test error message even though the init
succeeds.

Signed-off-by: Jarkko Sakkinen 
Fixes: cae8b441fc20 ("tpm: Factor out common startup code")
Reviewed-by: James Morris 

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 08c7e23..0c75c3f 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -957,7 +957,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
goto out;
 
rc = tpm2_do_selftest(chip);
-   if (rc != TPM2_RC_INITIALIZE) {
+   if (rc != 0 && rc != TPM2_RC_INITIALIZE) {
dev_err(>dev, "TPM self test failed\n");
goto out;
}
@@ -974,7 +974,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
}
}
 
-   return rc;
 out:
if (rc > 0)
rc = -ENODEV;

[GIT PULL] TPM bugfix

[James Morris]

Please pull this fix for the TPM code.


The following changes since commit 15301a570754c7af60335d094dd2d1808b0641a5:

  x86/paravirt: Do not trace _paravirt_ident_*() functions (2016-09-02 09:40:47 
-0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git 
for-linus

Jarkko Sakkinen (1):
  tpm: invalid self test error message

 drivers/char/tpm/tpm2-cmd.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

---

commit 85bd4528b88838a26b078ee3895c3cf3ad95b03c
Author: Jarkko Sakkinen 
Date:   Fri Sep 2 02:36:58 2016 +0300

tpm: invalid self test error message

The driver emits invalid self test error message even though the init
succeeds.

Signed-off-by: Jarkko Sakkinen 
Fixes: cae8b441fc20 ("tpm: Factor out common startup code")
Reviewed-by: James Morris 

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 08c7e23..0c75c3f 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -957,7 +957,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
goto out;
 
rc = tpm2_do_selftest(chip);
-   if (rc != TPM2_RC_INITIALIZE) {
+   if (rc != 0 && rc != TPM2_RC_INITIALIZE) {
dev_err(>dev, "TPM self test failed\n");
goto out;
}
@@ -974,7 +974,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
}
}
 
-   return rc;
 out:
if (rc > 0)
rc = -ENODEV;

[RESEND PATCH] Fix sysrq emergency thaw

[Charles Gong]

"SYSRQ + J" triggers a call to emergency_thaw_all(). Currently, this
is an infinite loop. Once we trigger it, we'll need to do a hard
power-cycle. There are users reporting this bug from 2012 to 2016, for
example, at https://bugzilla.kernel.org/show_bug.cgi?id=47741.

This happens because thaw_bdev() fails to return -EINVAL in the
non-frozen case, so fix it so that do_one_thaw() can recognize this case
and quit from looping. I checked that none of the other thaw_bdev()
callers check the return value.

The regression was introduced in commit 4504230a7156 ("freeze_bdev: grab
active reference to frozen superblocks").

Reviewed-by: Chris Mason 
Acked-by: Pavel Machek 
Signed-off-by: Charles Gong 
---
 fs/block_dev.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 71ccab1..14f015e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -301,14 +301,12 @@ int thaw_bdev(struct block_device *bdev, struct 
super_block *sb)
error = sb->s_op->thaw_super(sb);
else
error = thaw_super(sb);
-   if (error) {
+   if (error)
bdev->bd_fsfreeze_count++;
-   mutex_unlock(>bd_fsfreeze_mutex);
-   return error;
-   }
+
 out:
mutex_unlock(>bd_fsfreeze_mutex);
-   return 0;
+   return error;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
-- 
2.8.0.rc2

[RESEND PATCH] Fix sysrq emergency thaw

[Charles Gong]

"SYSRQ + J" triggers a call to emergency_thaw_all(). Currently, this
is an infinite loop. Once we trigger it, we'll need to do a hard
power-cycle. There are users reporting this bug from 2012 to 2016, for
example, at https://bugzilla.kernel.org/show_bug.cgi?id=47741.

This happens because thaw_bdev() fails to return -EINVAL in the
non-frozen case, so fix it so that do_one_thaw() can recognize this case
and quit from looping. I checked that none of the other thaw_bdev()
callers check the return value.

The regression was introduced in commit 4504230a7156 ("freeze_bdev: grab
active reference to frozen superblocks").

Reviewed-by: Chris Mason 
Acked-by: Pavel Machek 
Signed-off-by: Charles Gong 
---
 fs/block_dev.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 71ccab1..14f015e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -301,14 +301,12 @@ int thaw_bdev(struct block_device *bdev, struct 
super_block *sb)
error = sb->s_op->thaw_super(sb);
else
error = thaw_super(sb);
-   if (error) {
+   if (error)
bdev->bd_fsfreeze_count++;
-   mutex_unlock(>bd_fsfreeze_mutex);
-   return error;
-   }
+
 out:
mutex_unlock(>bd_fsfreeze_mutex);
-   return 0;
+   return error;
 }
 EXPORT_SYMBOL(thaw_bdev);
 
-- 
2.8.0.rc2

Re: [PATCH v2 3/3] nvme: Enable autonomous power state transitions

[J Freyensee]

On Fri, 2016-09-02 at 14:43 -0700, Andy Lutomirski wrote:
> On Fri, Sep 2, 2016 at 2:15 PM, J Freyensee
>  wrote:
> > 
> > On Tue, 2016-08-30 at 14:59 -0700, Andy Lutomirski wrote:
> > > 
> > > NVME devices can advertise multiple power states.  These states
> > > can
> > > be either "operational" (the device is fully functional but
> > > possibly
> > > slow) or "non-operational" (the device is asleep until woken up).
> > > Some devices can automatically enter a non-operational state when
> > > idle for a specified amount of time and then automatically wake
> > > back
> > > up when needed.
> > > 
> > > The hardware configuration is a table.  For each state, an entry
> > > in
> > > the table indicates the next deeper non-operational state, if
> > > any,
> > > to autonomously transition to and the idle time required before
> > > transitioning.
> > > 
> > > This patch teaches the driver to program APST so that each
> > > successive non-operational state will be entered after an idle
> > > time
> > > equal to 100% of the total latency (entry plus exit) associated
> > > with
> > > that state.  A sysfs attribute 'apst_max_latency_us' gives the
> > > maximum acceptable latency in ns; non-operational states with
> > > total
> > > latency greater than this value will not be used.  As a special
> > > case, apst_max_latency_us=0 will disable APST entirely.
> > 
> > May I ask a dumb question?
> > 
> > How does this work with multiple NVMe devices plugged into a
> > system?  I
> > would have thought we'd want one apst_max_latency_us entry per NVMe
> > controller for individual control of each device?  I have two
> > Fultondale-class devices plugged into a system I tried these
> > patches on
> > (the 4.8-rc4 kernel) and I'm not sure how the single
> > /sys/module/nvme_core/parameters/apst_max_latency_us would work per
> > my
> > 2 devices (and the value is using the default 25000).
> > 
> 
> Ah, I faked you out :(
> 
> The module parameter (nvme_core/parameters/apst_max_latency_us) just
> sets the default for newly probed devices.  The actual setting is in
> /sys/devices/whatever (symlinked from /sys/block/nvme0n1/devices, for
> example).  Perhaps I should name the former
> default_apst_max_latency_us.

It would certainly be more describable to understand what the entry is
for, but then the name is also getting longer.

Just "default_apst_latency_us"? Or maybe it's probably fine as-is.

Re: [PATCH v2 3/3] nvme: Enable autonomous power state transitions

[J Freyensee]

On Fri, 2016-09-02 at 14:43 -0700, Andy Lutomirski wrote:
> On Fri, Sep 2, 2016 at 2:15 PM, J Freyensee
>  wrote:
> > 
> > On Tue, 2016-08-30 at 14:59 -0700, Andy Lutomirski wrote:
> > > 
> > > NVME devices can advertise multiple power states.  These states
> > > can
> > > be either "operational" (the device is fully functional but
> > > possibly
> > > slow) or "non-operational" (the device is asleep until woken up).
> > > Some devices can automatically enter a non-operational state when
> > > idle for a specified amount of time and then automatically wake
> > > back
> > > up when needed.
> > > 
> > > The hardware configuration is a table.  For each state, an entry
> > > in
> > > the table indicates the next deeper non-operational state, if
> > > any,
> > > to autonomously transition to and the idle time required before
> > > transitioning.
> > > 
> > > This patch teaches the driver to program APST so that each
> > > successive non-operational state will be entered after an idle
> > > time
> > > equal to 100% of the total latency (entry plus exit) associated
> > > with
> > > that state.  A sysfs attribute 'apst_max_latency_us' gives the
> > > maximum acceptable latency in ns; non-operational states with
> > > total
> > > latency greater than this value will not be used.  As a special
> > > case, apst_max_latency_us=0 will disable APST entirely.
> > 
> > May I ask a dumb question?
> > 
> > How does this work with multiple NVMe devices plugged into a
> > system?  I
> > would have thought we'd want one apst_max_latency_us entry per NVMe
> > controller for individual control of each device?  I have two
> > Fultondale-class devices plugged into a system I tried these
> > patches on
> > (the 4.8-rc4 kernel) and I'm not sure how the single
> > /sys/module/nvme_core/parameters/apst_max_latency_us would work per
> > my
> > 2 devices (and the value is using the default 25000).
> > 
> 
> Ah, I faked you out :(
> 
> The module parameter (nvme_core/parameters/apst_max_latency_us) just
> sets the default for newly probed devices.  The actual setting is in
> /sys/devices/whatever (symlinked from /sys/block/nvme0n1/devices, for
> example).  Perhaps I should name the former
> default_apst_max_latency_us.

It would certainly be more describable to understand what the entry is
for, but then the name is also getting longer.

Just "default_apst_latency_us"? Or maybe it's probably fine as-is.

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> Felipe, your tests will show whether my guess was totally off-base.  
> For the new people, Felipe is tracking down a problem that involves
> exactly the code sequence listed at the top of the email, where we know
> that the wakeup routine runs but nevertheless the task sleeps.  At
> least, that's what it looks like at the moment.

What arch are you seeing this on?

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> Felipe, your tests will show whether my guess was totally off-base.  
> For the new people, Felipe is tracking down a problem that involves
> exactly the code sequence listed at the top of the email, where we know
> that the wakeup routine runs but nevertheless the task sleeps.  At
> least, that's what it looks like at the moment.

What arch are you seeing this on?

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> > 
> > Actually, that's not entirely true (although presumably it works okay
> > for most architectures).
> 
> Yeah, all load-store archs (with exception of PowerPC and ARM64 and
> possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).
> 
> ( and MIPS doesn't use their fancy barriers in Linux )
> 
> PowerPC does the full fence for smp_mb__before_spinlock, which leaves
> ARM64, I'm not sure its correct, but I'm way too tired to think about
> that now.
> 
> The TSO archs imply full barriers with all atomic RmW ops and are
> therefore also good.
> 

Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
smp_mb() too?

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Fri, Sep 2, 2016 at 5:43 AM, Arnd Bergmann  wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
>>
>> Hi Felipe and Arnd,
>>
>> It has been a while since the last response to this discussion, but we
>> haven't reached an agreement yet!  Can we get to a conclusion on if it
>> is valid to create child platform device for abstraction purpose?  If
>> yes, can this child device do DMA by itself?
>
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.
>
> This used to be simpler back when we could configure the kernel for
> only one SoC platform at a time, and the platforms could provide their
> own overrides for the dma-mapping interfaces. These days, we rely on
> firmware or bootloader to describe various aspects of how DMA is done,
> so you can't assume that passing a device without an of_node pointer
> or ACPI data into those functions will do the right thing.

Can we use the firmware or bootloader information to provide the
default dma-mapping attributes for devices that doesn't have an
of_node pointer or ACPI data?  This will at least restore what we had
previously provided .  I'm concerned that changing all the drivers
that are creating child device will be a big effort.  Like I mentioned
in another thread, there are many instances of platform_device_add()
under the drivers/ directory.

- Leo

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> > 
> > Actually, that's not entirely true (although presumably it works okay
> > for most architectures).
> 
> Yeah, all load-store archs (with exception of PowerPC and ARM64 and
> possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).
> 
> ( and MIPS doesn't use their fancy barriers in Linux )
> 
> PowerPC does the full fence for smp_mb__before_spinlock, which leaves
> ARM64, I'm not sure its correct, but I'm way too tired to think about
> that now.
> 
> The TSO archs imply full barriers with all atomic RmW ops and are
> therefore also good.
> 

Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
smp_mb() too?

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Fri, Sep 2, 2016 at 5:43 AM, Arnd Bergmann  wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
>>
>> Hi Felipe and Arnd,
>>
>> It has been a while since the last response to this discussion, but we
>> haven't reached an agreement yet!  Can we get to a conclusion on if it
>> is valid to create child platform device for abstraction purpose?  If
>> yes, can this child device do DMA by itself?
>
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.
>
> This used to be simpler back when we could configure the kernel for
> only one SoC platform at a time, and the platforms could provide their
> own overrides for the dma-mapping interfaces. These days, we rely on
> firmware or bootloader to describe various aspects of how DMA is done,
> so you can't assume that passing a device without an of_node pointer
> or ACPI data into those functions will do the right thing.

Can we use the firmware or bootloader information to provide the
default dma-mapping attributes for devices that doesn't have an
of_node pointer or ACPI data?  This will at least restore what we had
previously provided .  I'm concerned that changing all the drivers
that are creating child device will be a big effort.  Like I mentioned
in another thread, there are many instances of platform_device_add()
under the drivers/ directory.

- Leo

Re: [PATCH v2 01/15] Remove unused symbols, unnecessary parens, other minor comments from

[Bjorn Helgaas]

On Fri, Sep 02, 2016 at 02:42:56PM -0700, Guenter Roeck wrote:
> On Fri, Sep 02, 2016 at 10:53:58AM -0500, Bjorn Helgaas wrote:
> > Guenter.
> 
> Kind of an odd patch description ;-)

Yeah :)  These are just things you commented on initially.  I'm going to
squash all these and throw away these crappy changelogs anyway.

Re: [PATCH v2 01/15] Remove unused symbols, unnecessary parens, other minor comments from

[Bjorn Helgaas]

On Fri, Sep 02, 2016 at 02:42:56PM -0700, Guenter Roeck wrote:
> On Fri, Sep 02, 2016 at 10:53:58AM -0500, Bjorn Helgaas wrote:
> > Guenter.
> 
> Kind of an odd patch description ;-)

Yeah :)  These are just things you commented on initially.  I'm going to
squash all these and throw away these crappy changelogs anyway.

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> 
> Actually, that's not entirely true (although presumably it works okay
> for most architectures).

Yeah, all load-store archs (with exception of PowerPC and ARM64 and
possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).

( and MIPS doesn't use their fancy barriers in Linux )

PowerPC does the full fence for smp_mb__before_spinlock, which leaves
ARM64, I'm not sure its correct, but I'm way too tired to think about
that now.

The TSO archs imply full barriers with all atomic RmW ops and are
therefore also good.

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> 
> Actually, that's not entirely true (although presumably it works okay
> for most architectures).

Yeah, all load-store archs (with exception of PowerPC and ARM64 and
possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).

( and MIPS doesn't use their fancy barriers in Linux )

PowerPC does the full fence for smp_mb__before_spinlock, which leaves
ARM64, I'm not sure its correct, but I'm way too tired to think about
that now.

The TSO archs imply full barriers with all atomic RmW ops and are
therefore also good.

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

On 09/02/2016 02:33 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:

On 09/02/2016 02:02 AM, Arnd Bergmann wrote:

On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:


--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,29 @@
 #include "ion_priv.h"
 #include "compat_ion.h"

+union ion_ioctl_arg {
+   struct ion_fd_data fd;
+   struct ion_allocation_data allocation;
+   struct ion_handle_data handle;
+   struct ion_custom_data custom;
+   struct ion_abi_version abi_version;
+};


Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).



This was just a structure inside ion_ioctl. I pulled it out for
the validate function. It's not an actual argument to any ioctl from
userspace. ion_ioctl copies using _IOC_SIZE.


Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.


+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+   int ret = 0;
+
+   switch (cmd) {
+   case ION_IOC_ABI_VERSION:
+   ret = arg->abi_version.reserved != 0;
+   break;
+   default:
+   break;
+   }
+
+   return ret ? -EINVAL : 0;
+}


I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.



The concern was trying ioctls that wouldn't actually fail or would
have some other unexpected side effect.

My conclusion from the other thread was that assuming we don't botch
up adding new ioctls in the future or make incompatible changes to
these in the future we shouldn't technically need it. I was still
trying to hedge my bets against the future but that might just be
making the problem worse?


We've had a number of cases where versioned ABIs just didn't work out.

The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.

I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.



Okay it sounds like the answer is to strive to never run into a case
where versioned ioctls are necessary. Shouldn't be too hard, right? ;)


+/**
+ * struct ion_abi_version
+ *
+ *  @version - current ABI version
+ */
+
+#define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
+
+struct ion_abi_version {
+   __u32 abi_version;
+   __u32 reserved;
+};
+


This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.



The botching-ioctls.txt document suggested everything should be aligned
to 64-bits. Was I interpreting that too literally?


I didn't even know that file existed ;-)

I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like

struct ioctl_arg {
__u64 first;
__u32 second;
};

which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.



Right, that's the problem it's trying to avoid.


If there is no 64-bit member in the struct, there is no need for padding
at the end.



That's what I thought as well. I think I'll submit a patch to the docs
clarifying a few things.



Arnd



Thanks,
Laura

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

On 09/02/2016 02:33 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:

On 09/02/2016 02:02 AM, Arnd Bergmann wrote:

On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:


--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,29 @@
 #include "ion_priv.h"
 #include "compat_ion.h"

+union ion_ioctl_arg {
+   struct ion_fd_data fd;
+   struct ion_allocation_data allocation;
+   struct ion_handle_data handle;
+   struct ion_custom_data custom;
+   struct ion_abi_version abi_version;
+};


Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).



This was just a structure inside ion_ioctl. I pulled it out for
the validate function. It's not an actual argument to any ioctl from
userspace. ion_ioctl copies using _IOC_SIZE.


Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.


+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+{
+   int ret = 0;
+
+   switch (cmd) {
+   case ION_IOC_ABI_VERSION:
+   ret = arg->abi_version.reserved != 0;
+   break;
+   default:
+   break;
+   }
+
+   return ret ? -EINVAL : 0;
+}


I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.



The concern was trying ioctls that wouldn't actually fail or would
have some other unexpected side effect.

My conclusion from the other thread was that assuming we don't botch
up adding new ioctls in the future or make incompatible changes to
these in the future we shouldn't technically need it. I was still
trying to hedge my bets against the future but that might just be
making the problem worse?


We've had a number of cases where versioned ABIs just didn't work out.

The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.

I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.



Okay it sounds like the answer is to strive to never run into a case
where versioned ioctls are necessary. Shouldn't be too hard, right? ;)


+/**
+ * struct ion_abi_version
+ *
+ *  @version - current ABI version
+ */
+
+#define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
+
+struct ion_abi_version {
+   __u32 abi_version;
+   __u32 reserved;
+};
+


This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.



The botching-ioctls.txt document suggested everything should be aligned
to 64-bits. Was I interpreting that too literally?


I didn't even know that file existed ;-)

I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like

struct ioctl_arg {
__u64 first;
__u32 second;
};

which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.



Right, that's the problem it's trying to avoid.


If there is no 64-bit member in the struct, there is no need for padding
at the end.



That's what I thought as well. I think I'll submit a patch to the docs
clarifying a few things.



Arnd



Thanks,
Laura

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> The struct tpm_class_ops is not used outside the TPM driver. Thus,
> it can be safely move to drivers/char/tpm/tpm.h.

No, this is the wrong direction.

The goal is to make things more like other subsystems, so we should be
moving struct tpm_chip into the public header, and that requires ops
to be in the public header.

This is why I put ops here in the first place.

Jason

Re: [PATCH v3] power: bq24735-charger: Request status GPIO with initial input setup

[Paul Kocialkowski]

Hi,

Le vendredi 02 septembre 2016 à 17:27 +0200, Sebastian Reichel a écrit :
> Hi Paul,
> 
> This looks mostly fine now. I have a few more comments, that I
> missed last time:

Thanks for the review and for the useful suggestions. I have applied them to v4.

> On Thu, Sep 01, 2016 at 11:27:00PM +0200, Paul Kocialkowski wrote:
> > 
> > This requests the status GPIO with initial input setup. it is required
> > to read the GPIO status at probe time and thus correctly avoid sending
> > i2c messages when AC is not plugged.
> > 
> > When requesting the GPIO without initial input setup, it always reads 0
> > which causes probe to fail as it assumes the charger is connected, sends
> > i2c messages and fails.
> > 
> > While at it, this switches the driver over to gpio consumer.
> > 
> > Signed-off-by: Paul Kocialkowski 
> > ---
> >  drivers/power/bq24735-charger.c   | 42 +---
> > ---
> >  include/linux/power/bq24735-charger.h |  4 +---
> >  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> Please rebase next version to power-supply's for-next branch:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h
> =for-next
> 
> > 
> > diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-
> > charger.c
> > index dc460bb..7f9b305 100644
> > --- a/drivers/power/bq24735-charger.c
> > +++ b/drivers/power/bq24735-charger.c
> > @@ -25,7 +25,8 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> > +#include 
> 
> No need for , all gpiod bits used by you are in
> 
> 
> > 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -178,11 +179,9 @@ static int bq24735_config_charger(struct bq24735
> > *charger)
> >  static bool bq24735_charger_is_present(struct bq24735 *charger)
> >  {
> >     struct bq24735_platform *pdata = charger->pdata;
> > -   int ret;
> >  
> > -   if (pdata->status_gpio_valid) {
> > -   ret = gpio_get_value_cansleep(pdata->status_gpio);
> > -   return ret ^= pdata->status_gpio_active_low == 0;
> > +   if (pdata->status_gpio) {
> > +   return !gpiod_get_value_cansleep(pdata->status_gpio);
> >     } else {
> >     int ac = 0;
> >  
> > @@ -308,7 +307,6 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >     struct device_node *np = client->dev.of_node;
> >     u32 val;
> >     int ret;
> > -   enum of_gpio_flags flags;
> >  
> >     pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >     if (!pdata) {
> > @@ -317,11 +315,13 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >     return NULL;
> >     }
> >  
> > -   pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect-
> > gpios",
> > -    0, );
> > -
> > -   if (flags & OF_GPIO_ACTIVE_LOW)
> > -   pdata->status_gpio_active_low = 1;
> > +   pdata->status_gpio = devm_gpiod_get_optional(>dev,
> > +   "ti,ac-detect", GPIOD_IN);
> > +   if (IS_ERR(pdata->status_gpio)) {
> > +   ret = PTR_ERR(pdata->status_gpio);
> > +   dev_err(>dev, "Getting gpio failed: %d\n", ret);
> > +   return (struct bq24735_platform *) pdata->status_gpio;
> > +   }
> >  
> >     ret = of_property_read_u32(np, "ti,charge-current", );
> >     if (!ret)
> > @@ -357,8 +357,11 @@ static int bq24735_charger_probe(struct i2c_client
> > *client,
> >     charger->charging = true;
> >     charger->pdata = client->dev.platform_data;
> >  
> > -   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client-
> > >dev.of_node)
> > +   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client-
> > >dev.of_node) {
> >     charger->pdata = bq24735_parse_dt_data(client);
> > +   if (IS_ERR(charger->pdata))
> > +   return PTR_ERR(charger->pdata);
> > +   }
> >  
> >     if (!charger->pdata) {
> >     dev_err(>dev, "no platform data provided\n");
> > @@ -396,20 +399,7 @@ static int bq24735_charger_probe(struct i2c_client
> > *client,
> >  
> >     i2c_set_clientdata(client, charger);
> >  
> > -   if (gpio_is_valid(charger->pdata->status_gpio)) {
> > -   ret = devm_gpio_request(>dev,
> > -   charger->pdata->status_gpio,
> > -   name);
> > -   if (ret) {
> > -   dev_err(>dev,
> > -   "Failed GPIO request for GPIO %d: %d\n",
> > -   charger->pdata->status_gpio, ret);
> > -   }
> > -
> > -   charger->pdata->status_gpio_valid = !ret;
> > -   }
> 
> Do the devm_gpiod_get_optional() call here and store the result in
> "struct bq24735" instead of in pdata. It's not DT specific. The
> gpiod interface can also get it's data from boardcode (or acpi, if
> that becomes important at some point).
> 
> (Check Documentation/gpio/board.txt for details)
> 
> > 
> > -
> > -   if 

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> The struct tpm_class_ops is not used outside the TPM driver. Thus,
> it can be safely move to drivers/char/tpm/tpm.h.

No, this is the wrong direction.

The goal is to make things more like other subsystems, so we should be
moving struct tpm_chip into the public header, and that requires ops
to be in the public header.

This is why I put ops here in the first place.

Jason

Re: [PATCH v3] power: bq24735-charger: Request status GPIO with initial input setup

[Paul Kocialkowski]

Hi,

Le vendredi 02 septembre 2016 à 17:27 +0200, Sebastian Reichel a écrit :
> Hi Paul,
> 
> This looks mostly fine now. I have a few more comments, that I
> missed last time:

Thanks for the review and for the useful suggestions. I have applied them to v4.

> On Thu, Sep 01, 2016 at 11:27:00PM +0200, Paul Kocialkowski wrote:
> > 
> > This requests the status GPIO with initial input setup. it is required
> > to read the GPIO status at probe time and thus correctly avoid sending
> > i2c messages when AC is not plugged.
> > 
> > When requesting the GPIO without initial input setup, it always reads 0
> > which causes probe to fail as it assumes the charger is connected, sends
> > i2c messages and fails.
> > 
> > While at it, this switches the driver over to gpio consumer.
> > 
> > Signed-off-by: Paul Kocialkowski 
> > ---
> >  drivers/power/bq24735-charger.c   | 42 +---
> > ---
> >  include/linux/power/bq24735-charger.h |  4 +---
> >  2 files changed, 17 insertions(+), 29 deletions(-)
> 
> Please rebase next version to power-supply's for-next branch:
> 
> https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/log/?h
> =for-next
> 
> > 
> > diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-
> > charger.c
> > index dc460bb..7f9b305 100644
> > --- a/drivers/power/bq24735-charger.c
> > +++ b/drivers/power/bq24735-charger.c
> > @@ -25,7 +25,8 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> > +#include 
> 
> No need for , all gpiod bits used by you are in
> 
> 
> > 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -178,11 +179,9 @@ static int bq24735_config_charger(struct bq24735
> > *charger)
> >  static bool bq24735_charger_is_present(struct bq24735 *charger)
> >  {
> >     struct bq24735_platform *pdata = charger->pdata;
> > -   int ret;
> >  
> > -   if (pdata->status_gpio_valid) {
> > -   ret = gpio_get_value_cansleep(pdata->status_gpio);
> > -   return ret ^= pdata->status_gpio_active_low == 0;
> > +   if (pdata->status_gpio) {
> > +   return !gpiod_get_value_cansleep(pdata->status_gpio);
> >     } else {
> >     int ac = 0;
> >  
> > @@ -308,7 +307,6 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >     struct device_node *np = client->dev.of_node;
> >     u32 val;
> >     int ret;
> > -   enum of_gpio_flags flags;
> >  
> >     pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >     if (!pdata) {
> > @@ -317,11 +315,13 @@ static struct bq24735_platform
> > *bq24735_parse_dt_data(struct i2c_client *client)
> >     return NULL;
> >     }
> >  
> > -   pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect-
> > gpios",
> > -    0, );
> > -
> > -   if (flags & OF_GPIO_ACTIVE_LOW)
> > -   pdata->status_gpio_active_low = 1;
> > +   pdata->status_gpio = devm_gpiod_get_optional(>dev,
> > +   "ti,ac-detect", GPIOD_IN);
> > +   if (IS_ERR(pdata->status_gpio)) {
> > +   ret = PTR_ERR(pdata->status_gpio);
> > +   dev_err(>dev, "Getting gpio failed: %d\n", ret);
> > +   return (struct bq24735_platform *) pdata->status_gpio;
> > +   }
> >  
> >     ret = of_property_read_u32(np, "ti,charge-current", );
> >     if (!ret)
> > @@ -357,8 +357,11 @@ static int bq24735_charger_probe(struct i2c_client
> > *client,
> >     charger->charging = true;
> >     charger->pdata = client->dev.platform_data;
> >  
> > -   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client-
> > >dev.of_node)
> > +   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client-
> > >dev.of_node) {
> >     charger->pdata = bq24735_parse_dt_data(client);
> > +   if (IS_ERR(charger->pdata))
> > +   return PTR_ERR(charger->pdata);
> > +   }
> >  
> >     if (!charger->pdata) {
> >     dev_err(>dev, "no platform data provided\n");
> > @@ -396,20 +399,7 @@ static int bq24735_charger_probe(struct i2c_client
> > *client,
> >  
> >     i2c_set_clientdata(client, charger);
> >  
> > -   if (gpio_is_valid(charger->pdata->status_gpio)) {
> > -   ret = devm_gpio_request(>dev,
> > -   charger->pdata->status_gpio,
> > -   name);
> > -   if (ret) {
> > -   dev_err(>dev,
> > -   "Failed GPIO request for GPIO %d: %d\n",
> > -   charger->pdata->status_gpio, ret);
> > -   }
> > -
> > -   charger->pdata->status_gpio_valid = !ret;
> > -   }
> 
> Do the devm_gpiod_get_optional() call here and store the result in
> "struct bq24735" instead of in pdata. It's not DT specific. The
> gpiod interface can also get it's data from boardcode (or acpi, if
> that becomes important at some point).
> 
> (Check Documentation/gpio/board.txt for details)
> 
> > 
> > -
> > -   if 

[PATCH v4] power: bq24735-charger: Request status GPIO with initial input setup

[Paul Kocialkowski]

This requests the status GPIO with initial input setup. it is required
to read the GPIO status at probe time and thus correctly avoid sending
i2c messages when AC is not plugged.

When requesting the GPIO without initial input setup, it always reads 0
which causes probe to fail as it assumes the charger is connected, sends
i2c messages and fails.

While at it, this switches the driver over to gpio consumer.

Signed-off-by: Paul Kocialkowski 
---
 drivers/power/supply/bq24735-charger.c | 44 +-
 include/linux/power/bq24735-charger.h  |  4 
 2 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/power/supply/bq24735-charger.c 
b/drivers/power/supply/bq24735-charger.c
index dc460bb..44e3659 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -25,7 +25,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 
@@ -49,6 +49,7 @@ struct bq24735 {
struct i2c_client   *client;
struct bq24735_platform *pdata;
struct mutexlock;
+   struct gpio_desc*status_gpio;
boolcharging;
 };
 
@@ -177,12 +178,8 @@ static int bq24735_config_charger(struct bq24735 *charger)
 
 static bool bq24735_charger_is_present(struct bq24735 *charger)
 {
-   struct bq24735_platform *pdata = charger->pdata;
-   int ret;
-
-   if (pdata->status_gpio_valid) {
-   ret = gpio_get_value_cansleep(pdata->status_gpio);
-   return ret ^= pdata->status_gpio_active_low == 0;
+   if (charger->status_gpio) {
+   return !gpiod_get_value_cansleep(charger->status_gpio);
} else {
int ac = 0;
 
@@ -308,7 +305,6 @@ static struct bq24735_platform 
*bq24735_parse_dt_data(struct i2c_client *client)
struct device_node *np = client->dev.of_node;
u32 val;
int ret;
-   enum of_gpio_flags flags;
 
pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
@@ -317,12 +313,6 @@ static struct bq24735_platform 
*bq24735_parse_dt_data(struct i2c_client *client)
return NULL;
}
 
-   pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect-gpios",
-0, );
-
-   if (flags & OF_GPIO_ACTIVE_LOW)
-   pdata->status_gpio_active_low = 1;
-
ret = of_property_read_u32(np, "ti,charge-current", );
if (!ret)
pdata->charge_current = val;
@@ -357,8 +347,11 @@ static int bq24735_charger_probe(struct i2c_client *client,
charger->charging = true;
charger->pdata = client->dev.platform_data;
 
-   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client->dev.of_node)
+   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client->dev.of_node) {
charger->pdata = bq24735_parse_dt_data(client);
+   if (IS_ERR(charger->pdata))
+   return PTR_ERR(charger->pdata);
+   }
 
if (!charger->pdata) {
dev_err(>dev, "no platform data provided\n");
@@ -396,21 +389,16 @@ static int bq24735_charger_probe(struct i2c_client 
*client,
 
i2c_set_clientdata(client, charger);
 
-   if (gpio_is_valid(charger->pdata->status_gpio)) {
-   ret = devm_gpio_request(>dev,
-   charger->pdata->status_gpio,
-   name);
-   if (ret) {
-   dev_err(>dev,
-   "Failed GPIO request for GPIO %d: %d\n",
-   charger->pdata->status_gpio, ret);
-   }
-
-   charger->pdata->status_gpio_valid = !ret;
+   charger->status_gpio = devm_gpiod_get_optional(>dev,
+  "ti,ac-detect",
+  GPIOD_IN);
+   if (IS_ERR(charger->status_gpio)) {
+   ret = PTR_ERR(charger->status_gpio);
+   dev_err(>dev, "Getting gpio failed: %d\n", ret);
+   return ret;
}
 
-   if (!charger->pdata->status_gpio_valid
-   || bq24735_charger_is_present(charger)) {
+   if (!charger->status_gpio || bq24735_charger_is_present(charger)) {
ret = bq24735_read_word(client, BQ24735_MANUFACTURER_ID);
if (ret < 0) {
dev_err(>dev, "Failed to read manufacturer id : 
%d\n",
diff --git a/include/linux/power/bq24735-charger.h 
b/include/linux/power/bq24735-charger.h
index 6b750c1a..b04be59 100644
--- a/include/linux/power/bq24735-charger.h
+++ b/include/linux/power/bq24735-charger.h
@@ -28,10 +28,6 @@ struct bq24735_platform {
 
const char *name;
 
-   int status_gpio;
-   int status_gpio_active_low;
-   bool status_gpio_valid;
-

[PATCH v4] power: bq24735-charger: Request status GPIO with initial input setup

[Paul Kocialkowski]

This requests the status GPIO with initial input setup. it is required
to read the GPIO status at probe time and thus correctly avoid sending
i2c messages when AC is not plugged.

When requesting the GPIO without initial input setup, it always reads 0
which causes probe to fail as it assumes the charger is connected, sends
i2c messages and fails.

While at it, this switches the driver over to gpio consumer.

Signed-off-by: Paul Kocialkowski 
---
 drivers/power/supply/bq24735-charger.c | 44 +-
 include/linux/power/bq24735-charger.h  |  4 
 2 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/drivers/power/supply/bq24735-charger.c 
b/drivers/power/supply/bq24735-charger.c
index dc460bb..44e3659 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -25,7 +25,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 
@@ -49,6 +49,7 @@ struct bq24735 {
struct i2c_client   *client;
struct bq24735_platform *pdata;
struct mutexlock;
+   struct gpio_desc*status_gpio;
boolcharging;
 };
 
@@ -177,12 +178,8 @@ static int bq24735_config_charger(struct bq24735 *charger)
 
 static bool bq24735_charger_is_present(struct bq24735 *charger)
 {
-   struct bq24735_platform *pdata = charger->pdata;
-   int ret;
-
-   if (pdata->status_gpio_valid) {
-   ret = gpio_get_value_cansleep(pdata->status_gpio);
-   return ret ^= pdata->status_gpio_active_low == 0;
+   if (charger->status_gpio) {
+   return !gpiod_get_value_cansleep(charger->status_gpio);
} else {
int ac = 0;
 
@@ -308,7 +305,6 @@ static struct bq24735_platform 
*bq24735_parse_dt_data(struct i2c_client *client)
struct device_node *np = client->dev.of_node;
u32 val;
int ret;
-   enum of_gpio_flags flags;
 
pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
@@ -317,12 +313,6 @@ static struct bq24735_platform 
*bq24735_parse_dt_data(struct i2c_client *client)
return NULL;
}
 
-   pdata->status_gpio = of_get_named_gpio_flags(np, "ti,ac-detect-gpios",
-0, );
-
-   if (flags & OF_GPIO_ACTIVE_LOW)
-   pdata->status_gpio_active_low = 1;
-
ret = of_property_read_u32(np, "ti,charge-current", );
if (!ret)
pdata->charge_current = val;
@@ -357,8 +347,11 @@ static int bq24735_charger_probe(struct i2c_client *client,
charger->charging = true;
charger->pdata = client->dev.platform_data;
 
-   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client->dev.of_node)
+   if (IS_ENABLED(CONFIG_OF) && !charger->pdata && client->dev.of_node) {
charger->pdata = bq24735_parse_dt_data(client);
+   if (IS_ERR(charger->pdata))
+   return PTR_ERR(charger->pdata);
+   }
 
if (!charger->pdata) {
dev_err(>dev, "no platform data provided\n");
@@ -396,21 +389,16 @@ static int bq24735_charger_probe(struct i2c_client 
*client,
 
i2c_set_clientdata(client, charger);
 
-   if (gpio_is_valid(charger->pdata->status_gpio)) {
-   ret = devm_gpio_request(>dev,
-   charger->pdata->status_gpio,
-   name);
-   if (ret) {
-   dev_err(>dev,
-   "Failed GPIO request for GPIO %d: %d\n",
-   charger->pdata->status_gpio, ret);
-   }
-
-   charger->pdata->status_gpio_valid = !ret;
+   charger->status_gpio = devm_gpiod_get_optional(>dev,
+  "ti,ac-detect",
+  GPIOD_IN);
+   if (IS_ERR(charger->status_gpio)) {
+   ret = PTR_ERR(charger->status_gpio);
+   dev_err(>dev, "Getting gpio failed: %d\n", ret);
+   return ret;
}
 
-   if (!charger->pdata->status_gpio_valid
-   || bq24735_charger_is_present(charger)) {
+   if (!charger->status_gpio || bq24735_charger_is_present(charger)) {
ret = bq24735_read_word(client, BQ24735_MANUFACTURER_ID);
if (ret < 0) {
dev_err(>dev, "Failed to read manufacturer id : 
%d\n",
diff --git a/include/linux/power/bq24735-charger.h 
b/include/linux/power/bq24735-charger.h
index 6b750c1a..b04be59 100644
--- a/include/linux/power/bq24735-charger.h
+++ b/include/linux/power/bq24735-charger.h
@@ -28,10 +28,6 @@ struct bq24735_platform {
 
const char *name;
 
-   int status_gpio;
-   int status_gpio_active_low;
-   bool status_gpio_valid;
-
bool ext_control;

Re: [PATCH v2 07/15] Simplify the confusing HIWORD_UPDATE scheme.

[Bjorn Helgaas]

On Fri, Sep 02, 2016 at 02:38:06PM -0700, Guenter Roeck wrote:
> On Fri, Sep 02, 2016 at 10:54:53AM -0500, Bjorn Helgaas wrote:

> > +#define HIWORD_UPDATE(mask, val)   ((mask << 16) | val)
> > +
> > +#define ENCODE_LANES(x)(((x >> 1) & 3) << 4)
> 
> (x) ?

Done, thanks!  (And for "mask" and "val")

Re: [PATCH v2 07/15] Simplify the confusing HIWORD_UPDATE scheme.

[Bjorn Helgaas]

On Fri, Sep 02, 2016 at 02:38:06PM -0700, Guenter Roeck wrote:
> On Fri, Sep 02, 2016 at 10:54:53AM -0500, Bjorn Helgaas wrote:

> > +#define HIWORD_UPDATE(mask, val)   ((mask << 16) | val)
> > +
> > +#define ENCODE_LANES(x)(((x >> 1) & 3) << 4)
> 
> (x) ?

Done, thanks!  (And for "mask" and "val")

[tip:timers/core 6/6] alarmtimer.c:undefined reference to `rtc_ktime_to_tm'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
head:   a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021
commit: a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021 [6/6] time: alarmtimer: Add 
tracepoints for alarmtimers
config: s390-allyesconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021
# save the attached .config to linux build tree
make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   kernel/built-in.o: In function `perf_trace_alarm_processing':
>> alarmtimer.c:(.text+0xcc918): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc92e): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc944): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc95a): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc970): undefined reference to `rtc_ktime_to_tm'
   kernel/built-in.o:alarmtimer.c:(.text+0xcc986): more undefined references to 
`rtc_ktime_to_tm' follow

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

[tip:timers/core 6/6] alarmtimer.c:undefined reference to `rtc_ktime_to_tm'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
head:   a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021
commit: a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021 [6/6] time: alarmtimer: Add 
tracepoints for alarmtimers
config: s390-allyesconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout a0a6e06d545a753740c9d8d5ce2c4fdd3ab1c021
# save the attached .config to linux build tree
make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   kernel/built-in.o: In function `perf_trace_alarm_processing':
>> alarmtimer.c:(.text+0xcc918): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc92e): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc944): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc95a): undefined reference to `rtc_ktime_to_tm'
   alarmtimer.c:(.text+0xcc970): undefined reference to `rtc_ktime_to_tm'
   kernel/built-in.o:alarmtimer.c:(.text+0xcc986): more undefined references to 
`rtc_ktime_to_tm' follow

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

[PATCH] ARM: fix linker call for ARM_MODULE_PLTS

[Wiebe, Wladislav (Nokia - DE/Ulm)]

module.lds script doesn't get called when
CONFIG_ARM_MODULE_PLTS is enabled.
Use KBUILD_LDFLAGS_MODULE to fix it.

Signed-off-by: Wladislav Wiebe 
---
 arch/arm/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 61f6ccc..01c6025 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -20,7 +20,7 @@ LDFLAGS_MODULE+= --be8
 endif
 
 ifeq ($(CONFIG_ARM_MODULE_PLTS),y)
-LDFLAGS_MODULE += -T $(srctree)/arch/arm/kernel/module.lds
+KBUILD_LDFLAGS_MODULE  += -T $(srctree)/arch/arm/kernel/module.lds
 endif
 
 OBJCOPYFLAGS   :=-O binary -R .comment -S
-- 
1.7.12.4

[PATCH] ARM: fix linker call for ARM_MODULE_PLTS

[Wiebe, Wladislav (Nokia - DE/Ulm)]

module.lds script doesn't get called when
CONFIG_ARM_MODULE_PLTS is enabled.
Use KBUILD_LDFLAGS_MODULE to fix it.

Signed-off-by: Wladislav Wiebe 
---
 arch/arm/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 61f6ccc..01c6025 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -20,7 +20,7 @@ LDFLAGS_MODULE+= --be8
 endif
 
 ifeq ($(CONFIG_ARM_MODULE_PLTS),y)
-LDFLAGS_MODULE += -T $(srctree)/arch/arm/kernel/module.lds
+KBUILD_LDFLAGS_MODULE  += -T $(srctree)/arch/arm/kernel/module.lds
 endif
 
 OBJCOPYFLAGS   :=-O binary -R .comment -S
-- 
1.7.12.4

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/02/2016 02:37 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:


All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

  * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
from/to a void __user * in the kernel. Try really hard not to delay this
conversion or worse, fiddle the raw __u64 through your code since that
diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?



I don't know a better way that two cast, doing

(struct ion_heap_data __user *)(uintptr_t)query->heaps;

It may help to put this into an inline function though.

Arnd



There already is one it turns out in kernel.h:

#define u64_to_user_ptr(x) (\
{   \
typecheck(u64, x);  \
(void __user *)(uintptr_t)x;\
}   \
)

At least this way I don't have to look at the double casts.

Thanks,
Laura

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/02/2016 02:37 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:


All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

  * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
from/to a void __user * in the kernel. Try really hard not to delay this
conversion or worse, fiddle the raw __u64 through your code since that
diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?



I don't know a better way that two cast, doing

(struct ion_heap_data __user *)(uintptr_t)query->heaps;

It may help to put this into an inline function though.

Arnd



There already is one it turns out in kernel.h:

#define u64_to_user_ptr(x) (\
{   \
typecheck(u64, x);  \
(void __user *)(uintptr_t)x;\
}   \
)

At least this way I don't have to look at the double casts.

Thanks,
Laura

Re: [PATCH v3] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

Since this patch has been on hold for a little bit, I did a bit of
thinking of how we could this a little more cleanly. Unfortunately I
couldn't think of a way, however I did think of an alternative
solution:

I'm planning on backporting all of the skl wm fixes already, so I'm
going to use this patch for that since it's very small. As for
mainline, I'm going to do a whole reorganization of the skl wm/ddb
structs in i915 like Matt had suggested before. Things might look a
little more like this (taken from my half-complete reorganization):

struct skl_plane_ddb_allocation {
struct skl_ddb_entry plane;
struct skl_ddb_entry y_plane;
};

struct skl_plane_wm_values {
struct skl_plane_ddb_allocation ddb;
uint32_t wm[8];
uint32_t trans_wm;
};

struct skl_pipe_wm_values {
struct skl_ddb_entry ddb;
uint32_t linetime;
};

struct skl_hw_wm_values { /* (only used for skl_get_hw_ddb and friends) */
struct skl_pipe_wm_values pipe[I915_MAX_PIPES];
struct skl_plane_wm_values plane[I915_MAX_PIPES][I915_MAX_PLANES];
};

As well, I'm also just going to completely remove the skl_results and
skl_hw structs from struct drm_i915_private. This makes sense for a lot
of reasons:
 * This completely gets rid of the need for a global watermark lock (on
   Skylake at least) and will make things a lot easier for atomic
   support in the future
 * Skylake doesn't have any actual global watermark hooks anyway, aside
   from skl_update_wm() which is now only used for writing watermarks
   for inactive pipes during haswell_crtc_enable()
 * This makes passing watermarks around way less of a mess
 * Saves a tiny bit of data, and so far being able to grab
   watermarks/ddbs right from the plane states seems to be a lot easier
   then messing with a large array

As for this fix, I'll probably still need someone to review it so I can
get it into 4.7.y.

Let me know what you think.

On Mon, 2016-08-29 at 12:31 -0400, Lyude wrote:
> i915 sometimes needs to disable planes in the middle of an atomic
> commit, and then reenable them later in the same commit. Because of
> this, we can't make the assumption that the state of the plane
> actually
> changed. Since the state of the plane hasn't actually changed,
> neither
> have it's watermarks. And if the watermarks hasn't changed then we
> haven't populated skl_results with anything, which means we'll end up
> zeroing out a plane's watermarks in the middle of the atomic commit
> without restoring them later.
> 
> Simple reproduction recipe:
>  - Get a SKL laptop, launch any kind of X session
>  - Get two extra monitors
>  - Keep hotplugging both displays (so that the display configuration
>    jumps from 1 active pipe to 3 active pipes and back)
>  - Eventually underrun
> 
> Changes since v1:
>  - Fix incorrect use of "it's"
> Changes since v2:
>  - Add reproduction recipe
> 
> Signed-off-by: Lyude 
> Cc: Maarten Lankhorst 
> Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks
> atomically
> during plane updates")
> 
> Signed-off-by: Lyude 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 9 +++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index e4e6141..13e47a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3448,7 +3448,12 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_crtc->pipe;
>  
> - skl_write_plane_wm(intel_crtc, _priv->wm.skl_results,
> 0);
> + /*
> +  * We only populate skl_results on watermark updates, and if
> the
> +  * plane's visiblity isn't actually changing neither is its
> watermarks.
> +  */
> + if (!crtc->primary->state->visible)
> + skl_write_plane_wm(intel_crtc, _priv-
> >wm.skl_results, 0);
>  
>   I915_WRITE(PLANE_CTL(pipe, 0), 0);
>   I915_WRITE(PLANE_SURF(pipe, 0), 0);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 0df783a..73a521f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>   const int pipe = intel_plane->pipe;
>   const int plane = intel_plane->plane + 1;
>  
> - skl_write_plane_wm(to_intel_crtc(crtc), _priv-
> >wm.skl_results,
> -    plane);
> + /*
> +  * We only populate skl_results on watermark updates, and if
> the
> +  * plane's visiblity isn't actually changing neither is its
> watermarks.
> +  */
> + if (!dplane->state->visible)
> + skl_write_plane_wm(to_intel_crtc(crtc),
> +

Re: [PATCH v3] drm/i915/skl: Don't try to update plane watermarks if they haven't changed

[Lyude]

Since this patch has been on hold for a little bit, I did a bit of
thinking of how we could this a little more cleanly. Unfortunately I
couldn't think of a way, however I did think of an alternative
solution:

I'm planning on backporting all of the skl wm fixes already, so I'm
going to use this patch for that since it's very small. As for
mainline, I'm going to do a whole reorganization of the skl wm/ddb
structs in i915 like Matt had suggested before. Things might look a
little more like this (taken from my half-complete reorganization):

struct skl_plane_ddb_allocation {
struct skl_ddb_entry plane;
struct skl_ddb_entry y_plane;
};

struct skl_plane_wm_values {
struct skl_plane_ddb_allocation ddb;
uint32_t wm[8];
uint32_t trans_wm;
};

struct skl_pipe_wm_values {
struct skl_ddb_entry ddb;
uint32_t linetime;
};

struct skl_hw_wm_values { /* (only used for skl_get_hw_ddb and friends) */
struct skl_pipe_wm_values pipe[I915_MAX_PIPES];
struct skl_plane_wm_values plane[I915_MAX_PIPES][I915_MAX_PLANES];
};

As well, I'm also just going to completely remove the skl_results and
skl_hw structs from struct drm_i915_private. This makes sense for a lot
of reasons:
 * This completely gets rid of the need for a global watermark lock (on
   Skylake at least) and will make things a lot easier for atomic
   support in the future
 * Skylake doesn't have any actual global watermark hooks anyway, aside
   from skl_update_wm() which is now only used for writing watermarks
   for inactive pipes during haswell_crtc_enable()
 * This makes passing watermarks around way less of a mess
 * Saves a tiny bit of data, and so far being able to grab
   watermarks/ddbs right from the plane states seems to be a lot easier
   then messing with a large array

As for this fix, I'll probably still need someone to review it so I can
get it into 4.7.y.

Let me know what you think.

On Mon, 2016-08-29 at 12:31 -0400, Lyude wrote:
> i915 sometimes needs to disable planes in the middle of an atomic
> commit, and then reenable them later in the same commit. Because of
> this, we can't make the assumption that the state of the plane
> actually
> changed. Since the state of the plane hasn't actually changed,
> neither
> have it's watermarks. And if the watermarks hasn't changed then we
> haven't populated skl_results with anything, which means we'll end up
> zeroing out a plane's watermarks in the middle of the atomic commit
> without restoring them later.
> 
> Simple reproduction recipe:
>  - Get a SKL laptop, launch any kind of X session
>  - Get two extra monitors
>  - Keep hotplugging both displays (so that the display configuration
>    jumps from 1 active pipe to 3 active pipes and back)
>  - Eventually underrun
> 
> Changes since v1:
>  - Fix incorrect use of "it's"
> Changes since v2:
>  - Add reproduction recipe
> 
> Signed-off-by: Lyude 
> Cc: Maarten Lankhorst 
> Fixes: 62e0fb880123 ("drm/i915/skl: Update plane watermarks
> atomically
> during plane updates")
> 
> Signed-off-by: Lyude 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  | 9 +++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index e4e6141..13e47a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3448,7 +3448,12 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   int pipe = intel_crtc->pipe;
>  
> - skl_write_plane_wm(intel_crtc, _priv->wm.skl_results,
> 0);
> + /*
> +  * We only populate skl_results on watermark updates, and if
> the
> +  * plane's visiblity isn't actually changing neither is its
> watermarks.
> +  */
> + if (!crtc->primary->state->visible)
> + skl_write_plane_wm(intel_crtc, _priv-
> >wm.skl_results, 0);
>  
>   I915_WRITE(PLANE_CTL(pipe, 0), 0);
>   I915_WRITE(PLANE_SURF(pipe, 0), 0);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 0df783a..73a521f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -292,8 +292,13 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>   const int pipe = intel_plane->pipe;
>   const int plane = intel_plane->plane + 1;
>  
> - skl_write_plane_wm(to_intel_crtc(crtc), _priv-
> >wm.skl_results,
> -    plane);
> + /*
> +  * We only populate skl_results on watermark updates, and if
> the
> +  * plane's visiblity isn't actually changing neither is its
> watermarks.
> +  */
> + if (!dplane->state->visible)
> + skl_write_plane_wm(to_intel_crtc(crtc),
> +    _priv->wm.skl_results,
> plane);
>  
>   

[PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

The struct tpm_class_ops is not used outside the TPM driver. Thus,
it can be safely move to drivers/char/tpm/tpm.h.

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm.h | 13 +
 include/linux/tpm.h| 14 --
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3e952fb..e1d277d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -138,6 +138,19 @@ enum tpm2_startup_types {
 
 #define TPM_PPI_VERSION_LEN3
 
+struct tpm_class_ops {
+   unsigned int flags;
+   const u8 req_complete_mask;
+   const u8 req_complete_val;
+   bool (*req_canceled)(struct tpm_chip *chip, u8 status);
+   int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
+   int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+   void (*cancel) (struct tpm_chip *chip);
+   u8 (*status) (struct tpm_chip *chip);
+   bool (*update_timeouts)(struct tpm_chip *chip,
+   unsigned long *timeout_cap);
+};
+
 enum tpm_chip_flags {
TPM_CHIP_FLAG_REGISTERED= BIT(0),
TPM_CHIP_FLAG_TPM2  = BIT(1),
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..76ba592 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -37,20 +37,6 @@ enum TPM_OPS_FLAGS {
TPM_OPS_AUTO_STARTUP = BIT(0),
 };
 
-struct tpm_class_ops {
-   unsigned int flags;
-   const u8 req_complete_mask;
-   const u8 req_complete_val;
-   bool (*req_canceled)(struct tpm_chip *chip, u8 status);
-   int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
-   int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
-   void (*cancel) (struct tpm_chip *chip);
-   u8 (*status) (struct tpm_chip *chip);
-   bool (*update_timeouts)(struct tpm_chip *chip,
-   unsigned long *timeout_cap);
-
-};
-
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(u32 chip_num);
-- 
2.7.4

[PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

The struct tpm_class_ops is not used outside the TPM driver. Thus,
it can be safely move to drivers/char/tpm/tpm.h.

Signed-off-by: Jarkko Sakkinen 
---
 drivers/char/tpm/tpm.h | 13 +
 include/linux/tpm.h| 14 --
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3e952fb..e1d277d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -138,6 +138,19 @@ enum tpm2_startup_types {
 
 #define TPM_PPI_VERSION_LEN3
 
+struct tpm_class_ops {
+   unsigned int flags;
+   const u8 req_complete_mask;
+   const u8 req_complete_val;
+   bool (*req_canceled)(struct tpm_chip *chip, u8 status);
+   int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
+   int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+   void (*cancel) (struct tpm_chip *chip);
+   u8 (*status) (struct tpm_chip *chip);
+   bool (*update_timeouts)(struct tpm_chip *chip,
+   unsigned long *timeout_cap);
+};
+
 enum tpm_chip_flags {
TPM_CHIP_FLAG_REGISTERED= BIT(0),
TPM_CHIP_FLAG_TPM2  = BIT(1),
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..76ba592 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -37,20 +37,6 @@ enum TPM_OPS_FLAGS {
TPM_OPS_AUTO_STARTUP = BIT(0),
 };
 
-struct tpm_class_ops {
-   unsigned int flags;
-   const u8 req_complete_mask;
-   const u8 req_complete_val;
-   bool (*req_canceled)(struct tpm_chip *chip, u8 status);
-   int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
-   int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
-   void (*cancel) (struct tpm_chip *chip);
-   u8 (*status) (struct tpm_chip *chip);
-   bool (*update_timeouts)(struct tpm_chip *chip,
-   unsigned long *timeout_cap);
-
-};
-
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(u32 chip_num);
-- 
2.7.4

Re: [PATCH] dt-binding: remoteproc: Document generic properties

[Suman Anna]

On 08/12/2016 05:42 PM, Bjorn Andersson wrote:
> On Fri 12 Aug 11:34 PDT 2016, Rob Herring wrote:
> 
>> On Wed, Aug 10, 2016 at 10:37:02AM -0700, Bjorn Andersson wrote:
>>> This documents the generic properties "rprocs" and "rproc-names", used
>>> for consumer drivers to reference a remoteproc node.
>>
>> How do you intend to use this? I wonder if it would not be better to 
>> expose a remote proc with existing bindings for a particular purpose 
>> (e.g. clocks, resets, etc.) rather than a generic connection. The client 
>> side would have to have specific knowledge as to what functions the 
>> remote proc provides.
>>
> 
> The remoteproc node represents the mechanism and resources needed to
> control the life cycle a co-processor, e.g. loading, booting, shutting
> gown a video encoder/decoder.
> 
> The proposed reference allows a separate thingie to assert control of
> the life cycle of that co-processor.
> 
> 
> I acknowledge that in some cases there is a fine line between what is
> the life cycle management and what is the actual functionality
> implemented by that remote processor. But as the remoteproc mechanism is
> reusable between various use cases I think it makes sense to not describe
> them as one unit.

What's the current state of this patch, not officially acked yet right?

While we are at this, can we agree upon an alias stem name as well, we
can stick to "rproc". Otherwise, I can submit an incremental patch on
top of this along with the code that adds an API to retrieve it for
client users.

regards
Suman

Re: [PATCH] dt-binding: remoteproc: Document generic properties

[Suman Anna]

On 08/12/2016 05:42 PM, Bjorn Andersson wrote:
> On Fri 12 Aug 11:34 PDT 2016, Rob Herring wrote:
> 
>> On Wed, Aug 10, 2016 at 10:37:02AM -0700, Bjorn Andersson wrote:
>>> This documents the generic properties "rprocs" and "rproc-names", used
>>> for consumer drivers to reference a remoteproc node.
>>
>> How do you intend to use this? I wonder if it would not be better to 
>> expose a remote proc with existing bindings for a particular purpose 
>> (e.g. clocks, resets, etc.) rather than a generic connection. The client 
>> side would have to have specific knowledge as to what functions the 
>> remote proc provides.
>>
> 
> The remoteproc node represents the mechanism and resources needed to
> control the life cycle a co-processor, e.g. loading, booting, shutting
> gown a video encoder/decoder.
> 
> The proposed reference allows a separate thingie to assert control of
> the life cycle of that co-processor.
> 
> 
> I acknowledge that in some cases there is a fine line between what is
> the life cycle management and what is the actual functionality
> implemented by that remote processor. But as the remoteproc mechanism is
> reusable between various use cases I think it makes sense to not describe
> them as one unit.

What's the current state of this patch, not officially acked yet right?

While we are at this, can we agree upon an alias stem name as well, we
can stick to "rproc". Otherwise, I can submit an incremental patch on
top of this along with the code that adds an API to retrieve it for
client users.

regards
Suman

[GIT PULL] ACPI fixes for v4.8-rc5

[Rafael J. Wysocki]

Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-4.8-rc5

to receive ACPI fixes for v4.8-rc5.

The top-most commit is 5331d9cab32ef640b4cd38a43b0858874fbb7168

 ACPI / drivers: replace acpi_probe_lock spinlock with mutex

and the base is commit 3eab887a55424fc2c27553b7bfe32330df83f7b8

 Linux 4.8-rc4

Included are two stable-candidate fixes for the ACPI early device
probing code added during the 4.4 cycle, one fixing a typo in a stub
macro used when CONFIG_ACPI is unset and one that prevents sleeping
functions from being called under a spinlock (Lorenzo Pieralisi).

Thanks!

---

Lorenzo Pieralisi (2):
  ACPI / drivers: fix typo in ACPI_DECLARE_PROBE_ENTRY macro
  ACPI / drivers: replace acpi_probe_lock spinlock with mutex

---

 drivers/acpi/scan.c  | 6 +++---
 include/linux/acpi.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Re: [PATCH v2 3/3] nvme: Enable autonomous power state transitions

[Andy Lutomirski]

On Fri, Sep 2, 2016 at 2:15 PM, J Freyensee
 wrote:
> On Tue, 2016-08-30 at 14:59 -0700, Andy Lutomirski wrote:
>> NVME devices can advertise multiple power states.  These states can
>> be either "operational" (the device is fully functional but possibly
>> slow) or "non-operational" (the device is asleep until woken up).
>> Some devices can automatically enter a non-operational state when
>> idle for a specified amount of time and then automatically wake back
>> up when needed.
>>
>> The hardware configuration is a table.  For each state, an entry in
>> the table indicates the next deeper non-operational state, if any,
>> to autonomously transition to and the idle time required before
>> transitioning.
>>
>> This patch teaches the driver to program APST so that each
>> successive non-operational state will be entered after an idle time
>> equal to 100% of the total latency (entry plus exit) associated with
>> that state.  A sysfs attribute 'apst_max_latency_us' gives the
>> maximum acceptable latency in ns; non-operational states with total
>> latency greater than this value will not be used.  As a special
>> case, apst_max_latency_us=0 will disable APST entirely.
>
> May I ask a dumb question?
>
> How does this work with multiple NVMe devices plugged into a system?  I
> would have thought we'd want one apst_max_latency_us entry per NVMe
> controller for individual control of each device?  I have two
> Fultondale-class devices plugged into a system I tried these patches on
> (the 4.8-rc4 kernel) and I'm not sure how the single
> /sys/module/nvme_core/parameters/apst_max_latency_us would work per my
> 2 devices (and the value is using the default 25000).
>

Ah, I faked you out :(

The module parameter (nvme_core/parameters/apst_max_latency_us) just
sets the default for newly probed devices.  The actual setting is in
/sys/devices/whatever (symlinked from /sys/block/nvme0n1/devices, for
example).  Perhaps I should name the former
default_apst_max_latency_us.

>
>>
>> On hardware without APST support, apst_max_latency_us will not be
>> exposed in sysfs.
>
> Not sure that is true, as from what I see so far, Fultondales don't
> support apst yet I still see:
>
> [root@nvme-fabric-host01 nvme-cli]# cat
> /sys/module/nvme_core/parameters/apst_max_latency_us
> 25000

That will be there regardless.  It's the value in the sysfs device
directory that won't be there, which is hopefully why you couldn't
find it.

>
>>
>> In theory, the device can expose "default" APST table, but this
>> doesn't seem to function correctly on my device (Samsung 950), nor
>> does it seem particularly useful.  There is also an optional
>> mechanism by which a configuration can be "saved" so it will be
>> automatically loaded on reset.  This can be configured from
>> userspace, but it doesn't seem useful to support in the driver.
>>
>> On my laptop, enabling APST seems to save nearly 1W.
>>
>> The hardware tables can be decoded in userspace with nvme-cli.
>> 'nvme id-ctrl /dev/nvmeN' will show the power state table and
>> 'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST
>> configuration.
>
> nvme get-feature -f 0x0c -H /dev/nvme0
>
> isn't working for me, I get a:
>
> [root@nvme-fabric-host01 nvme-cli]# ./nvme get-feature -f 0x0c -H
> /dev/nvme0
> NVMe Status:INVALID_FIELD(2)
>
> I don't have the time right now to investigate further, but I'll assume
> it's because I have Fultondales (though I would have thought this patch
> would have provided enough code for the latest nvme-cli code to do this
> new get-feature as-is).

I'm assuming it doesn't work because your hardware doesn't support
APST.  nvme get-feature works even without my patches, since it mostly
bypasses the driver.

--Andy

[GIT PULL] ACPI fixes for v4.8-rc5

[Rafael J. Wysocki]

Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-4.8-rc5

to receive ACPI fixes for v4.8-rc5.

The top-most commit is 5331d9cab32ef640b4cd38a43b0858874fbb7168

 ACPI / drivers: replace acpi_probe_lock spinlock with mutex

and the base is commit 3eab887a55424fc2c27553b7bfe32330df83f7b8

 Linux 4.8-rc4

Included are two stable-candidate fixes for the ACPI early device
probing code added during the 4.4 cycle, one fixing a typo in a stub
macro used when CONFIG_ACPI is unset and one that prevents sleeping
functions from being called under a spinlock (Lorenzo Pieralisi).

Thanks!

---

Lorenzo Pieralisi (2):
  ACPI / drivers: fix typo in ACPI_DECLARE_PROBE_ENTRY macro
  ACPI / drivers: replace acpi_probe_lock spinlock with mutex

---

 drivers/acpi/scan.c  | 6 +++---
 include/linux/acpi.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Re: [PATCH v2 3/3] nvme: Enable autonomous power state transitions

[Andy Lutomirski]

On Fri, Sep 2, 2016 at 2:15 PM, J Freyensee
 wrote:
> On Tue, 2016-08-30 at 14:59 -0700, Andy Lutomirski wrote:
>> NVME devices can advertise multiple power states.  These states can
>> be either "operational" (the device is fully functional but possibly
>> slow) or "non-operational" (the device is asleep until woken up).
>> Some devices can automatically enter a non-operational state when
>> idle for a specified amount of time and then automatically wake back
>> up when needed.
>>
>> The hardware configuration is a table.  For each state, an entry in
>> the table indicates the next deeper non-operational state, if any,
>> to autonomously transition to and the idle time required before
>> transitioning.
>>
>> This patch teaches the driver to program APST so that each
>> successive non-operational state will be entered after an idle time
>> equal to 100% of the total latency (entry plus exit) associated with
>> that state.  A sysfs attribute 'apst_max_latency_us' gives the
>> maximum acceptable latency in ns; non-operational states with total
>> latency greater than this value will not be used.  As a special
>> case, apst_max_latency_us=0 will disable APST entirely.
>
> May I ask a dumb question?
>
> How does this work with multiple NVMe devices plugged into a system?  I
> would have thought we'd want one apst_max_latency_us entry per NVMe
> controller for individual control of each device?  I have two
> Fultondale-class devices plugged into a system I tried these patches on
> (the 4.8-rc4 kernel) and I'm not sure how the single
> /sys/module/nvme_core/parameters/apst_max_latency_us would work per my
> 2 devices (and the value is using the default 25000).
>

Ah, I faked you out :(

The module parameter (nvme_core/parameters/apst_max_latency_us) just
sets the default for newly probed devices.  The actual setting is in
/sys/devices/whatever (symlinked from /sys/block/nvme0n1/devices, for
example).  Perhaps I should name the former
default_apst_max_latency_us.

>
>>
>> On hardware without APST support, apst_max_latency_us will not be
>> exposed in sysfs.
>
> Not sure that is true, as from what I see so far, Fultondales don't
> support apst yet I still see:
>
> [root@nvme-fabric-host01 nvme-cli]# cat
> /sys/module/nvme_core/parameters/apst_max_latency_us
> 25000

That will be there regardless.  It's the value in the sysfs device
directory that won't be there, which is hopefully why you couldn't
find it.

>
>>
>> In theory, the device can expose "default" APST table, but this
>> doesn't seem to function correctly on my device (Samsung 950), nor
>> does it seem particularly useful.  There is also an optional
>> mechanism by which a configuration can be "saved" so it will be
>> automatically loaded on reset.  This can be configured from
>> userspace, but it doesn't seem useful to support in the driver.
>>
>> On my laptop, enabling APST seems to save nearly 1W.
>>
>> The hardware tables can be decoded in userspace with nvme-cli.
>> 'nvme id-ctrl /dev/nvmeN' will show the power state table and
>> 'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST
>> configuration.
>
> nvme get-feature -f 0x0c -H /dev/nvme0
>
> isn't working for me, I get a:
>
> [root@nvme-fabric-host01 nvme-cli]# ./nvme get-feature -f 0x0c -H
> /dev/nvme0
> NVMe Status:INVALID_FIELD(2)
>
> I don't have the time right now to investigate further, but I'll assume
> it's because I have Fultondales (though I would have thought this patch
> would have provided enough code for the latest nvme-cli code to do this
> new get-feature as-is).

I'm assuming it doesn't work because your hardware doesn't support
APST.  nvme get-feature works even without my patches, since it mostly
bypasses the driver.

--Andy

Re: [PATCH v2 01/15] Remove unused symbols, unnecessary parens, other minor comments from

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:53:58AM -0500, Bjorn Helgaas wrote:
> Guenter.

Kind of an odd patch description ;-)

Guenter

> ---
>  drivers/pci/host/pcie-rockchip.c |   69 
> --
>  1 file changed, 21 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index e77aec3..a7006be 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -38,7 +38,6 @@
>  #include 
>  
>  #define PCIE_CLIENT_BASE 0x0
> -#define PCIE_RC_CONFIG_NORMAL_BASE   0x80
>  #define PCIE_RC_CONFIG_BASE  0xa0
>  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 0x90c
>  #define PCIE_RC_CONFIG_LCSR  0xd0
> @@ -126,9 +125,6 @@
>  #define RC_REGION_0_ADDR_TRANS_H 0x
>  #define RC_REGION_0_ADDR_TRANS_L 0x
>  #define RC_REGION_0_PASS_BITS(25 - 1)
> -#define RC_REGION_1_ADDR_TRANS_H 0x
> -#define RC_REGION_1_ADDR_TRANS_L 0x0040
> -#define RC_REGION_1_PASS_BITS(20 - 1)
>  #define MAX_AXI_WRAPPER_REGION_NUM   33
>  #define PCIE_CORE_LCSR_RETRAIN_LINK  BIT(5)
>  #define PCIE_CLIENT_CONF_ENABLE  BIT(0)
> @@ -147,15 +143,12 @@
>  #define PCIE_CLIENT_MODE_SHIFT   6
>  #define PCIE_CLIENT_MODE_MASK0x1
>  #define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_10
>  #define PCIE_CLIENT_GEN_SEL_SHIFT7
>  #define PCIE_CLIENT_GEN_SEL_MASK 0x1
>  #define PCIE_CLIENT_LINK_STATUS_UP   0x3
>  #define PCIE_CLIENT_LINK_STATUS_SHIFT20
>  #define PCIE_CLIENT_LINK_STATUS_MASK 0x3
> -#define PCIE_CORE_PL_CONF_SPEED_2_5G 0x0
>  #define PCIE_CORE_PL_CONF_SPEED_5G   0x1
> -#define PCIE_CORE_PL_CONF_SPEED_8G   0x2
>  #define PCIE_CORE_PL_CONF_SPEED_SHIFT3
>  #define PCIE_CORE_PL_CONF_SPEED_MASK 0x3
>  #define PCIE_CORE_PL_CONF_LANE_SHIFT 1
> @@ -174,11 +167,6 @@
>PCIE_CORE_INT_CT | PCIE_CORE_INT_UTC | \
>PCIE_CORE_INT_MMVC)
>  
> -#define PCIE_CLIENT_INT_SUBSYSTEM \
> - (PCIE_CLIENT_INT_PWR_STCG | PCIE_CLIENT_INT_HOT_PLUG | \
> - PCIE_CLIENT_INT_PHY | PCIE_CLIENT_INT_UDMA | \
> - PCIE_CLIENT_INT_LOCAL)
> -
>  #define PCIE_CLIENT_INT_LEGACY \
>   (PCIE_CLIENT_INT_INTA | PCIE_CLIENT_INT_INTB | \
>   PCIE_CLIENT_INT_INTC | PCIE_CLIENT_INT_INTD)
> @@ -191,8 +179,8 @@
>   PCIE_CLIENT_INT_PHY)
>  
>  struct rockchip_pcie_port {
> - void__iomem *reg_base;
> - void__iomem *apb_base;
> + void__iomem *reg_base;  /* DT axi-base */
> + void__iomem *apb_base;  /* DT apb-base */
>   struct  phy *phy;
>   struct  reset_control *core_rst;
>   struct  reset_control *mgmt_rst;
> @@ -240,7 +228,7 @@ static void rockchip_pcie_clr_bw_int(struct 
> rockchip_pcie_port *port)
>   pcie_write(port, status, PCIE_RC_CONFIG_BASE + PCIE_RC_CONFIG_LCSR);
>  }
>  
> -static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp,
> +static int rockchip_pcie_valid_device(struct rockchip_pcie_port *pp,
> struct pci_bus *bus, int dev)
>  {
>   /* access only one slot on each root port */
> @@ -286,7 +274,7 @@ static int rockchip_pcie_wr_own_conf(struct 
> rockchip_pcie_port *pp,
>  {
>   u32 mask, tmp, offset;
>  
> - offset = (where & (~0x3));
> + offset = where & ~0x3;
>  
>   if (size == 4) {
>   writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> @@ -357,7 +345,7 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 
> devfn, int where,
>  {
>   struct rockchip_pcie_port *pp = bus->sysdata;
>  
> - if (!rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn))) {
> + if (!rockchip_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
>   *val = 0x;
>   return PCIBIOS_DEVICE_NOT_FOUND;
>   }
> @@ -366,7 +354,6 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 
> devfn, int where,
>   return rockchip_pcie_rd_own_conf(pp, where, size, val);
>  
>   return rockchip_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
> -
>  }
>  
>  static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> @@ -374,7 +361,7 @@ static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 
> devfn,
>  {
>   struct rockchip_pcie_port *pp = bus->sysdata;
>  
> - if (!rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)))
> + if (!rockchip_pcie_valid_device(pp, bus, PCI_SLOT(devfn)))
>   return PCIBIOS_DEVICE_NOT_FOUND;
>  
>   if (bus->number == pp->root_bus_nr)
> @@ -516,7 +503,6 @@ static int rockchip_pcie_init_port(struct 
> rockchip_pcie_port *port)
>   

[GIT PULL] Power management fixes for v4.8-rc5

[Rafael J. Wysocki]

Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 pm-4.8-rc5

to receive power management fixes for v4.8-rc5.

The top-most commit is b654c62e7734cdcd507a5751cd940077be2ad373

 Merge branches 'pm-cpufreq-fixes' and 'pm-core-fixes'

and the base is commit 3eab887a55424fc2c27553b7bfe32330df83f7b8

 Linux 4.8-rc4

This includes a stable-candidate cpufreq-dt driver problem fix and
annotations of tracepoints in the runtime PM framework.

Specifics:

 - Fix the definition of the cpufreq-dt driver's machines table
   introduced during the 4.7 cycle that should be NULL-terminated,
   but the termination entry is missing from it (Wei Yongjun).

 - Annotate tracepoints in the runtime PM framework's core so as to
   allow the functions containing them to be called from the idle
   code path without causing RCU to complain about illegal usage
   (Paul McKenney).

Thanks!

---

Paul E. McKenney (2):
  PM / runtime: Add _rcuidle suffix to allow rpm_resume() to be
called from idle
  PM / runtime: Add _rcuidle suffix to allow rpm_idle() use from idle

Wei Yongjun (1):
  cpufreq: dt: Add terminate entry for of_device_id tables

---

 drivers/base/power/runtime.c | 10 +-
 drivers/cpufreq/cpufreq-dt-platdev.c |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

Re: [PATCH v2 01/15] Remove unused symbols, unnecessary parens, other minor comments from

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:53:58AM -0500, Bjorn Helgaas wrote:
> Guenter.

Kind of an odd patch description ;-)

Guenter

> ---
>  drivers/pci/host/pcie-rockchip.c |   69 
> --
>  1 file changed, 21 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index e77aec3..a7006be 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -38,7 +38,6 @@
>  #include 
>  
>  #define PCIE_CLIENT_BASE 0x0
> -#define PCIE_RC_CONFIG_NORMAL_BASE   0x80
>  #define PCIE_RC_CONFIG_BASE  0xa0
>  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 0x90c
>  #define PCIE_RC_CONFIG_LCSR  0xd0
> @@ -126,9 +125,6 @@
>  #define RC_REGION_0_ADDR_TRANS_H 0x
>  #define RC_REGION_0_ADDR_TRANS_L 0x
>  #define RC_REGION_0_PASS_BITS(25 - 1)
> -#define RC_REGION_1_ADDR_TRANS_H 0x
> -#define RC_REGION_1_ADDR_TRANS_L 0x0040
> -#define RC_REGION_1_PASS_BITS(20 - 1)
>  #define MAX_AXI_WRAPPER_REGION_NUM   33
>  #define PCIE_CORE_LCSR_RETRAIN_LINK  BIT(5)
>  #define PCIE_CLIENT_CONF_ENABLE  BIT(0)
> @@ -147,15 +143,12 @@
>  #define PCIE_CLIENT_MODE_SHIFT   6
>  #define PCIE_CLIENT_MODE_MASK0x1
>  #define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_10
>  #define PCIE_CLIENT_GEN_SEL_SHIFT7
>  #define PCIE_CLIENT_GEN_SEL_MASK 0x1
>  #define PCIE_CLIENT_LINK_STATUS_UP   0x3
>  #define PCIE_CLIENT_LINK_STATUS_SHIFT20
>  #define PCIE_CLIENT_LINK_STATUS_MASK 0x3
> -#define PCIE_CORE_PL_CONF_SPEED_2_5G 0x0
>  #define PCIE_CORE_PL_CONF_SPEED_5G   0x1
> -#define PCIE_CORE_PL_CONF_SPEED_8G   0x2
>  #define PCIE_CORE_PL_CONF_SPEED_SHIFT3
>  #define PCIE_CORE_PL_CONF_SPEED_MASK 0x3
>  #define PCIE_CORE_PL_CONF_LANE_SHIFT 1
> @@ -174,11 +167,6 @@
>PCIE_CORE_INT_CT | PCIE_CORE_INT_UTC | \
>PCIE_CORE_INT_MMVC)
>  
> -#define PCIE_CLIENT_INT_SUBSYSTEM \
> - (PCIE_CLIENT_INT_PWR_STCG | PCIE_CLIENT_INT_HOT_PLUG | \
> - PCIE_CLIENT_INT_PHY | PCIE_CLIENT_INT_UDMA | \
> - PCIE_CLIENT_INT_LOCAL)
> -
>  #define PCIE_CLIENT_INT_LEGACY \
>   (PCIE_CLIENT_INT_INTA | PCIE_CLIENT_INT_INTB | \
>   PCIE_CLIENT_INT_INTC | PCIE_CLIENT_INT_INTD)
> @@ -191,8 +179,8 @@
>   PCIE_CLIENT_INT_PHY)
>  
>  struct rockchip_pcie_port {
> - void__iomem *reg_base;
> - void__iomem *apb_base;
> + void__iomem *reg_base;  /* DT axi-base */
> + void__iomem *apb_base;  /* DT apb-base */
>   struct  phy *phy;
>   struct  reset_control *core_rst;
>   struct  reset_control *mgmt_rst;
> @@ -240,7 +228,7 @@ static void rockchip_pcie_clr_bw_int(struct 
> rockchip_pcie_port *port)
>   pcie_write(port, status, PCIE_RC_CONFIG_BASE + PCIE_RC_CONFIG_LCSR);
>  }
>  
> -static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp,
> +static int rockchip_pcie_valid_device(struct rockchip_pcie_port *pp,
> struct pci_bus *bus, int dev)
>  {
>   /* access only one slot on each root port */
> @@ -286,7 +274,7 @@ static int rockchip_pcie_wr_own_conf(struct 
> rockchip_pcie_port *pp,
>  {
>   u32 mask, tmp, offset;
>  
> - offset = (where & (~0x3));
> + offset = where & ~0x3;
>  
>   if (size == 4) {
>   writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> @@ -357,7 +345,7 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 
> devfn, int where,
>  {
>   struct rockchip_pcie_port *pp = bus->sysdata;
>  
> - if (!rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn))) {
> + if (!rockchip_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
>   *val = 0x;
>   return PCIBIOS_DEVICE_NOT_FOUND;
>   }
> @@ -366,7 +354,6 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 
> devfn, int where,
>   return rockchip_pcie_rd_own_conf(pp, where, size, val);
>  
>   return rockchip_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
> -
>  }
>  
>  static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> @@ -374,7 +361,7 @@ static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 
> devfn,
>  {
>   struct rockchip_pcie_port *pp = bus->sysdata;
>  
> - if (!rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)))
> + if (!rockchip_pcie_valid_device(pp, bus, PCI_SLOT(devfn)))
>   return PCIBIOS_DEVICE_NOT_FOUND;
>  
>   if (bus->number == pp->root_bus_nr)
> @@ -516,7 +503,6 @@ static int rockchip_pcie_init_port(struct 
> rockchip_pcie_port *port)
>   

[GIT PULL] Power management fixes for v4.8-rc5

[Rafael J. Wysocki]

Hi Linus,

Please pull from the tag

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 pm-4.8-rc5

to receive power management fixes for v4.8-rc5.

The top-most commit is b654c62e7734cdcd507a5751cd940077be2ad373

 Merge branches 'pm-cpufreq-fixes' and 'pm-core-fixes'

and the base is commit 3eab887a55424fc2c27553b7bfe32330df83f7b8

 Linux 4.8-rc4

This includes a stable-candidate cpufreq-dt driver problem fix and
annotations of tracepoints in the runtime PM framework.

Specifics:

 - Fix the definition of the cpufreq-dt driver's machines table
   introduced during the 4.7 cycle that should be NULL-terminated,
   but the termination entry is missing from it (Wei Yongjun).

 - Annotate tracepoints in the runtime PM framework's core so as to
   allow the functions containing them to be called from the idle
   code path without causing RCU to complain about illegal usage
   (Paul McKenney).

Thanks!

---

Paul E. McKenney (2):
  PM / runtime: Add _rcuidle suffix to allow rpm_resume() to be
called from idle
  PM / runtime: Add _rcuidle suffix to allow rpm_idle() use from idle

Wei Yongjun (1):
  cpufreq: dt: Add terminate entry for of_device_id tables

---

 drivers/base/power/runtime.c | 10 +-
 drivers/cpufreq/cpufreq-dt-platdev.c |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

[PATCH] dmaengine: moxart: remove NO_IRQ

[Arnd Bergmann]

The use of NO_IRQ is incorrect here and should never have been there,
as irq_of_parse_and_map() returns '0' on failure, not NO_IRQ.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/moxart-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
index a6e642792e5a..e1a5c2242f6f 100644
--- a/drivers/dma/moxart-dma.c
+++ b/drivers/dma/moxart-dma.c
@@ -579,7 +579,7 @@ static int moxart_probe(struct platform_device *pdev)
return -ENOMEM;
 
irq = irq_of_parse_and_map(node, 0);
-   if (irq == NO_IRQ) {
+   if (!irq) {
dev_err(dev, "no IRQ resource\n");
return -EINVAL;
}
-- 
2.9.0

[PATCH] dmaengine: moxart: remove NO_IRQ

[Arnd Bergmann]

The use of NO_IRQ is incorrect here and should never have been there,
as irq_of_parse_and_map() returns '0' on failure, not NO_IRQ.

Signed-off-by: Arnd Bergmann 
---
 drivers/dma/moxart-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/moxart-dma.c b/drivers/dma/moxart-dma.c
index a6e642792e5a..e1a5c2242f6f 100644
--- a/drivers/dma/moxart-dma.c
+++ b/drivers/dma/moxart-dma.c
@@ -579,7 +579,7 @@ static int moxart_probe(struct platform_device *pdev)
return -ENOMEM;
 
irq = irq_of_parse_and_map(node, 0);
-   if (irq == NO_IRQ) {
+   if (!irq) {
dev_err(dev, "no IRQ resource\n");
return -EINVAL;
}
-- 
2.9.0

Re: [PATCH v2 10/15] Group related CSR definitions together.

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:55:19AM -0500, Bjorn Helgaas wrote:
> 
> ---
>  drivers/pci/host/pcie-rockchip.c |  148 
> +++---
>  1 file changed, 74 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index 6edfce5..fe1b52f 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,21 +37,27 @@
>  #include 
>  #include 
>  
> -#define PCIE_CLIENT_BASE 0x0
> -#define PCIE_RC_CONFIG_BASE  0xa0
> -#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
> -#define PCIE_RC_CONFIG_LCS   (PCIE_RC_CONFIG_BASE + 0x0d0)
> -#define  PCIE_RC_CONFIG_LCS_RETRAIN_LINK BIT(5)
> -#define  PCIE_RC_CONFIG_LCS_LBMIEBIT(10)
> -#define  PCIE_RC_CONFIG_LCS_LABIEBIT(11)
> -#define  PCIE_RC_CONFIG_LCS_LBMS BIT(30)
> -#define  PCIE_RC_CONFIG_LCS_LAMS BIT(31)
> -#define PCIE_CORE_CTRL_MGMT_BASE 0x90
> -#define PCIE_CORE_AXI_CONF_BASE  0xc0
> -#define PCIE_CORE_AXI_INBOUND_BASE   0xc00800
> -#define PCIE_CLIENT_BASIC_STATUS1(PCIE_CLIENT_BASE + 0x48)
> -#define PCIE_CLIENT_INT_MASK (PCIE_CLIENT_BASE + 0x4c)
> -#define PCIE_CLIENT_INT_STATUS   (PCIE_CLIENT_BASE + 
> 0x50)
> +/*
> + * The upper 16 bits of PCIE_CLIENT_BASE are a write mask for the lower 16
> + * bits.  This allows atomic updates of the register without locking.
> + */
> +#define HIWORD_UPDATE(mask, val) ((mask << 16) | val)
> +
(mask), (val)

> +#define ENCODE_LANES(x)  (((x >> 1) & 3) << 4)

(x)

Guenter

Re: [PATCH v2 10/15] Group related CSR definitions together.

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:55:19AM -0500, Bjorn Helgaas wrote:
> 
> ---
>  drivers/pci/host/pcie-rockchip.c |  148 
> +++---
>  1 file changed, 74 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index 6edfce5..fe1b52f 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -37,21 +37,27 @@
>  #include 
>  #include 
>  
> -#define PCIE_CLIENT_BASE 0x0
> -#define PCIE_RC_CONFIG_BASE  0xa0
> -#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
> -#define PCIE_RC_CONFIG_LCS   (PCIE_RC_CONFIG_BASE + 0x0d0)
> -#define  PCIE_RC_CONFIG_LCS_RETRAIN_LINK BIT(5)
> -#define  PCIE_RC_CONFIG_LCS_LBMIEBIT(10)
> -#define  PCIE_RC_CONFIG_LCS_LABIEBIT(11)
> -#define  PCIE_RC_CONFIG_LCS_LBMS BIT(30)
> -#define  PCIE_RC_CONFIG_LCS_LAMS BIT(31)
> -#define PCIE_CORE_CTRL_MGMT_BASE 0x90
> -#define PCIE_CORE_AXI_CONF_BASE  0xc0
> -#define PCIE_CORE_AXI_INBOUND_BASE   0xc00800
> -#define PCIE_CLIENT_BASIC_STATUS1(PCIE_CLIENT_BASE + 0x48)
> -#define PCIE_CLIENT_INT_MASK (PCIE_CLIENT_BASE + 0x4c)
> -#define PCIE_CLIENT_INT_STATUS   (PCIE_CLIENT_BASE + 
> 0x50)
> +/*
> + * The upper 16 bits of PCIE_CLIENT_BASE are a write mask for the lower 16
> + * bits.  This allows atomic updates of the register without locking.
> + */
> +#define HIWORD_UPDATE(mask, val) ((mask << 16) | val)
> +
(mask), (val)

> +#define ENCODE_LANES(x)  (((x >> 1) & 3) << 4)

(x)

Guenter

Re: [PATCH v2 07/15] Simplify the confusing HIWORD_UPDATE scheme.

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:54:53AM -0500, Bjorn Helgaas wrote:
> 
> ---
>  drivers/pci/host/pcie-rockchip.c |   70 
> +-
>  1 file changed, 24 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index c0c3ad5..b204567 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -115,36 +115,26 @@
> (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
>  PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
>  
> -/*
> -  * The higher 16-bit of this register is used for write protection
> -  * only if BIT(x + 16) set to 1 the BIT(x) can be written.
> -  */
> -#define HIWORD_UPDATE(val, mask, shift) \
> - ((val) << (shift) | (mask) << ((shift) + 16))
> -
>  #define RC_REGION_0_ADDR_TRANS_H 0x
>  #define RC_REGION_0_ADDR_TRANS_L 0x
>  #define RC_REGION_0_PASS_BITS(25 - 1)
>  #define MAX_AXI_WRAPPER_REGION_NUM   33
>  #define PCIE_CORE_LCSR_RETRAIN_LINK  BIT(5)
> -#define PCIE_CLIENT_CONF_ENABLE  BIT(0)
> -#define PCIE_CLIENT_CONF_ENABLE_SHIFT0
> -#define PCIE_CLIENT_CONF_ENABLE_MASK 0x1
> -#define PCIE_CLIENT_LINK_TRAIN_ENABLE1
> -#define PCIE_CLIENT_LINK_TRAIN_SHIFT 1
> -#define PCIE_CLIENT_LINK_TRAIN_MASK  0x1
> -#define PCIE_CLIENT_ARI_ENABLE   BIT(0)
> -#define PCIE_CLIENT_ARI_ENABLE_SHIFT 3
> -#define PCIE_CLIENT_ARI_ENABLE_MASK  0x1
> -#define PCIE_CLIENT_CONF_LANE_NUM(x) (x / 2)
> -#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT  4
> -#define PCIE_CLIENT_CONF_LANE_NUM_MASK   0x3
> -#define PCIE_CLIENT_MODE_RC  BIT(0)
> -#define PCIE_CLIENT_MODE_SHIFT   6
> -#define PCIE_CLIENT_MODE_MASK0x1
> -#define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_SHIFT7
> -#define PCIE_CLIENT_GEN_SEL_MASK 0x1
> +
> +/*
> + * The upper 16 bits of the PCIE_CLIENT registers are a write mask for the
> + * lower 16 bits.  This allows atomic updates of the register without
> + * locking.
> + */
> +#define HIWORD_UPDATE(mask, val) ((mask << 16) | val)
> +
> +#define ENCODE_LANES(x)  (((x >> 1) & 3) << 4)

(x) ?

> +
> +#define PCIE_CLIENT_CONF_ENABLE  HIWORD_UPDATE(0x0001, 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLEHIWORD_UPDATE(0x0002, 0x0002)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
> +#define PCIE_CLIENT_GEN_SEL_2HIWORD_UPDATE(0x0040, 0x0040)
> +
>  #define PCIE_CLIENT_LINK_STATUS_UP   0x3
>  #define PCIE_CLIENT_LINK_STATUS_SHIFT20
>  #define PCIE_CLIENT_LINK_STATUS_MASK 0x3
> @@ -423,22 +413,13 @@ static int rockchip_pcie_init_port(struct rockchip_pcie 
> *rockchip)
>   }
>  
>   rockchip_pcie_write(rockchip,
> -HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE,
> -  PCIE_CLIENT_CONF_ENABLE_MASK,
> -  PCIE_CLIENT_CONF_ENABLE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes),
> -  PCIE_CLIENT_CONF_LANE_NUM_MASK,
> -  PCIE_CLIENT_CONF_LANE_NUM_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_MODE_RC,
> -  PCIE_CLIENT_MODE_MASK,
> -  PCIE_CLIENT_MODE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_ARI_ENABLE,
> -  PCIE_CLIENT_ARI_ENABLE_MASK,
> -  PCIE_CLIENT_ARI_ENABLE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_GEN_SEL_2,
> -  PCIE_CLIENT_GEN_SEL_MASK,
> -  PCIE_CLIENT_GEN_SEL_SHIFT),
> -PCIE_CLIENT_BASE);
> + PCIE_CLIENT_CONF_ENABLE |
> + PCIE_CLIENT_LINK_TRAIN_ENABLE |
> + PCIE_CLIENT_ARI_ENABLE |
> + PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) |
> + PCIE_CLIENT_MODE_RC |
> + PCIE_CLIENT_GEN_SEL_2,
> + PCIE_CLIENT_BASE);
>  

This is s much better ...

Guenter

>   err = phy_power_on(rockchip->phy);
>   if (err) {
> @@ -482,11 +463,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie 
> *rockchip)
>PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2);
>  
>   /* Enable Gen1 training */
> - rockchip_pcie_write(rockchip,
> -HIWORD_UPDATE(PCIE_CLIENT_LINK_TRAIN_ENABLE,
> -  PCIE_CLIENT_LINK_TRAIN_MASK,
> -  PCIE_CLIENT_LINK_TRAIN_SHIFT),
> -PCIE_CLIENT_BASE);
> + 

Re: [PATCH v2 07/15] Simplify the confusing HIWORD_UPDATE scheme.

[Guenter Roeck]

On Fri, Sep 02, 2016 at 10:54:53AM -0500, Bjorn Helgaas wrote:
> 
> ---
>  drivers/pci/host/pcie-rockchip.c |   70 
> +-
>  1 file changed, 24 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rockchip.c 
> b/drivers/pci/host/pcie-rockchip.c
> index c0c3ad5..b204567 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -115,36 +115,26 @@
> (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
>  PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
>  
> -/*
> -  * The higher 16-bit of this register is used for write protection
> -  * only if BIT(x + 16) set to 1 the BIT(x) can be written.
> -  */
> -#define HIWORD_UPDATE(val, mask, shift) \
> - ((val) << (shift) | (mask) << ((shift) + 16))
> -
>  #define RC_REGION_0_ADDR_TRANS_H 0x
>  #define RC_REGION_0_ADDR_TRANS_L 0x
>  #define RC_REGION_0_PASS_BITS(25 - 1)
>  #define MAX_AXI_WRAPPER_REGION_NUM   33
>  #define PCIE_CORE_LCSR_RETRAIN_LINK  BIT(5)
> -#define PCIE_CLIENT_CONF_ENABLE  BIT(0)
> -#define PCIE_CLIENT_CONF_ENABLE_SHIFT0
> -#define PCIE_CLIENT_CONF_ENABLE_MASK 0x1
> -#define PCIE_CLIENT_LINK_TRAIN_ENABLE1
> -#define PCIE_CLIENT_LINK_TRAIN_SHIFT 1
> -#define PCIE_CLIENT_LINK_TRAIN_MASK  0x1
> -#define PCIE_CLIENT_ARI_ENABLE   BIT(0)
> -#define PCIE_CLIENT_ARI_ENABLE_SHIFT 3
> -#define PCIE_CLIENT_ARI_ENABLE_MASK  0x1
> -#define PCIE_CLIENT_CONF_LANE_NUM(x) (x / 2)
> -#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT  4
> -#define PCIE_CLIENT_CONF_LANE_NUM_MASK   0x3
> -#define PCIE_CLIENT_MODE_RC  BIT(0)
> -#define PCIE_CLIENT_MODE_SHIFT   6
> -#define PCIE_CLIENT_MODE_MASK0x1
> -#define PCIE_CLIENT_GEN_SEL_21
> -#define PCIE_CLIENT_GEN_SEL_SHIFT7
> -#define PCIE_CLIENT_GEN_SEL_MASK 0x1
> +
> +/*
> + * The upper 16 bits of the PCIE_CLIENT registers are a write mask for the
> + * lower 16 bits.  This allows atomic updates of the register without
> + * locking.
> + */
> +#define HIWORD_UPDATE(mask, val) ((mask << 16) | val)
> +
> +#define ENCODE_LANES(x)  (((x >> 1) & 3) << 4)

(x) ?

> +
> +#define PCIE_CLIENT_CONF_ENABLE  HIWORD_UPDATE(0x0001, 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLEHIWORD_UPDATE(0x0002, 0x0002)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x) HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
> +#define PCIE_CLIENT_GEN_SEL_2HIWORD_UPDATE(0x0040, 0x0040)
> +
>  #define PCIE_CLIENT_LINK_STATUS_UP   0x3
>  #define PCIE_CLIENT_LINK_STATUS_SHIFT20
>  #define PCIE_CLIENT_LINK_STATUS_MASK 0x3
> @@ -423,22 +413,13 @@ static int rockchip_pcie_init_port(struct rockchip_pcie 
> *rockchip)
>   }
>  
>   rockchip_pcie_write(rockchip,
> -HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE,
> -  PCIE_CLIENT_CONF_ENABLE_MASK,
> -  PCIE_CLIENT_CONF_ENABLE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes),
> -  PCIE_CLIENT_CONF_LANE_NUM_MASK,
> -  PCIE_CLIENT_CONF_LANE_NUM_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_MODE_RC,
> -  PCIE_CLIENT_MODE_MASK,
> -  PCIE_CLIENT_MODE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_ARI_ENABLE,
> -  PCIE_CLIENT_ARI_ENABLE_MASK,
> -  PCIE_CLIENT_ARI_ENABLE_SHIFT) |
> -HIWORD_UPDATE(PCIE_CLIENT_GEN_SEL_2,
> -  PCIE_CLIENT_GEN_SEL_MASK,
> -  PCIE_CLIENT_GEN_SEL_SHIFT),
> -PCIE_CLIENT_BASE);
> + PCIE_CLIENT_CONF_ENABLE |
> + PCIE_CLIENT_LINK_TRAIN_ENABLE |
> + PCIE_CLIENT_ARI_ENABLE |
> + PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) |
> + PCIE_CLIENT_MODE_RC |
> + PCIE_CLIENT_GEN_SEL_2,
> + PCIE_CLIENT_BASE);
>  

This is s much better ...

Guenter

>   err = phy_power_on(rockchip->phy);
>   if (err) {
> @@ -482,11 +463,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie 
> *rockchip)
>PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2);
>  
>   /* Enable Gen1 training */
> - rockchip_pcie_write(rockchip,
> -HIWORD_UPDATE(PCIE_CLIENT_LINK_TRAIN_ENABLE,
> -  PCIE_CLIENT_LINK_TRAIN_MASK,
> -  PCIE_CLIENT_LINK_TRAIN_SHIFT),
> -PCIE_CLIENT_BASE);
> + 

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Arnd Bergmann]

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
> >
> > All warnings (new ones prefixed by >>):
> >
> >drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
> >>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from 
> >>> integer of different size [-Wint-to-pointer-cast]
> >   (struct ion_heap_data __user *)query->heaps;
> >   ^
> >
> 
> botching-up-ioctls.txt says:
> 
>   * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
> from/to a void __user * in the kernel. Try really hard not to delay this
> conversion or worse, fiddle the raw __u64 through your code since that
> diminishes the checking tools like sparse can provide.
> 
> This doesn't seem like it will work on 32-bit systems since you'll end up
> with the warning above. Is there a better option or did I misunderstand
> how that is supposed to work?
> 

I don't know a better way that two cast, doing

(struct ion_heap_data __user *)(uintptr_t)query->heaps;

It may help to put this into an inline function though.

Arnd

Re: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Arnd Bergmann]

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
> >
> > All warnings (new ones prefixed by >>):
> >
> >drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
> >>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from 
> >>> integer of different size [-Wint-to-pointer-cast]
> >   (struct ion_heap_data __user *)query->heaps;
> >   ^
> >
> 
> botching-up-ioctls.txt says:
> 
>   * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
> from/to a void __user * in the kernel. Try really hard not to delay this
> conversion or worse, fiddle the raw __u64 through your code since that
> diminishes the checking tools like sparse can provide.
> 
> This doesn't seem like it will work on 32-bit systems since you'll end up
> with the warning above. Is there a better option or did I misunderstand
> how that is supposed to work?
> 

I don't know a better way that two cast, doing

(struct ion_heap_data __user *)(uintptr_t)query->heaps;

It may help to put this into an inline function though.

Arnd

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Arnd Bergmann]

On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
> On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
> > On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
> >
> >> --- a/drivers/staging/android/ion/ion-ioctl.c
> >> +++ b/drivers/staging/android/ion/ion-ioctl.c
> >> @@ -22,6 +22,29 @@
> >>  #include "ion_priv.h"
> >>  #include "compat_ion.h"
> >>
> >> +union ion_ioctl_arg {
> >> +  struct ion_fd_data fd;
> >> +  struct ion_allocation_data allocation;
> >> +  struct ion_handle_data handle;
> >> +  struct ion_custom_data custom;
> >> +  struct ion_abi_version abi_version;
> >> +};
> >
> > Are you introducing this, or just clarifying the defintion of the
> > existing interface. For new interfaces, we should not have a union
> > as an ioctl argument. Instead each ioctl command should have one
> > specific structure (or better a scalar argument).
> >
> 
> This was just a structure inside ion_ioctl. I pulled it out for
> the validate function. It's not an actual argument to any ioctl from
> userspace. ion_ioctl copies using _IOC_SIZE.

Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.

> >> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> >> +{
> >> +  int ret = 0;
> >> +
> >> +  switch (cmd) {
> >> +  case ION_IOC_ABI_VERSION:
> >> +  ret = arg->abi_version.reserved != 0;
> >> +  break;
> >> +  default:
> >> +  break;
> >> +  }
> >> +
> >> +  return ret ? -EINVAL : 0;
> >> +}
> >
> > I agree with Greg, ioctl interfaces should normally not be versioned,
> > the usual way is to try a command and see if it fails or not.
> >
> 
> The concern was trying ioctls that wouldn't actually fail or would
> have some other unexpected side effect.
> 
> My conclusion from the other thread was that assuming we don't botch
> up adding new ioctls in the future or make incompatible changes to
> these in the future we shouldn't technically need it. I was still
> trying to hedge my bets against the future but that might just be
> making the problem worse?

We've had a number of cases where versioned ABIs just didn't work out.

The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.

I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.

> >> +/**
> >> + * struct ion_abi_version
> >> + *
> >> + *  @version - current ABI version
> >> + */
> >> +
> >> +#define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
> >> +
> >> +struct ion_abi_version {
> >> +  __u32 abi_version;
> >> +  __u32 reserved;
> >> +};
> >> +
> >
> > This interface doesn't really need a "reserved" field, you could
> > as well use a __u32 by itself. If you ever need a second field,
> > just add a new command number.
> >
> 
> The botching-ioctls.txt document suggested everything should be aligned
> to 64-bits. Was I interpreting that too literally?

I didn't even know that file existed ;-)

I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like

struct ioctl_arg {
__u64 first;
__u32 second;
};

which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.

If there is no 64-bit member in the struct, there is no need for padding
at the end.

Arnd

Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Arnd Bergmann]

On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
> On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
> > On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
> >
> >> --- a/drivers/staging/android/ion/ion-ioctl.c
> >> +++ b/drivers/staging/android/ion/ion-ioctl.c
> >> @@ -22,6 +22,29 @@
> >>  #include "ion_priv.h"
> >>  #include "compat_ion.h"
> >>
> >> +union ion_ioctl_arg {
> >> +  struct ion_fd_data fd;
> >> +  struct ion_allocation_data allocation;
> >> +  struct ion_handle_data handle;
> >> +  struct ion_custom_data custom;
> >> +  struct ion_abi_version abi_version;
> >> +};
> >
> > Are you introducing this, or just clarifying the defintion of the
> > existing interface. For new interfaces, we should not have a union
> > as an ioctl argument. Instead each ioctl command should have one
> > specific structure (or better a scalar argument).
> >
> 
> This was just a structure inside ion_ioctl. I pulled it out for
> the validate function. It's not an actual argument to any ioctl from
> userspace. ion_ioctl copies using _IOC_SIZE.

Ok, got it. This is fine from an interface point of view, just
a bit unusual in the way it's written.

> >> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> >> +{
> >> +  int ret = 0;
> >> +
> >> +  switch (cmd) {
> >> +  case ION_IOC_ABI_VERSION:
> >> +  ret = arg->abi_version.reserved != 0;
> >> +  break;
> >> +  default:
> >> +  break;
> >> +  }
> >> +
> >> +  return ret ? -EINVAL : 0;
> >> +}
> >
> > I agree with Greg, ioctl interfaces should normally not be versioned,
> > the usual way is to try a command and see if it fails or not.
> >
> 
> The concern was trying ioctls that wouldn't actually fail or would
> have some other unexpected side effect.
> 
> My conclusion from the other thread was that assuming we don't botch
> up adding new ioctls in the future or make incompatible changes to
> these in the future we shouldn't technically need it. I was still
> trying to hedge my bets against the future but that might just be
> making the problem worse?

We've had a number of cases where versioned ABIs just didn't work out.

The versions are either used to distinguish incompatible APIs, which
we should avoid to start with, or they are used for backwards-compatible
extensions that you should detect by checking whether an ioctl
succeeds. Relying on the API version number breaks if you get a partial
backport of features from a later version, and it's unclear what a
user space tool should expect when the kernel reports a newer ABI
than it knows.

I think the wireless extensions and KVM are examples of versioned
APIs that turned out to make things more complicated than they
would have been otherwise.

> >> +/**
> >> + * struct ion_abi_version
> >> + *
> >> + *  @version - current ABI version
> >> + */
> >> +
> >> +#define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
> >> +
> >> +struct ion_abi_version {
> >> +  __u32 abi_version;
> >> +  __u32 reserved;
> >> +};
> >> +
> >
> > This interface doesn't really need a "reserved" field, you could
> > as well use a __u32 by itself. If you ever need a second field,
> > just add a new command number.
> >
> 
> The botching-ioctls.txt document suggested everything should be aligned
> to 64-bits. Was I interpreting that too literally?

I didn't even know that file existed ;-)

I'm pretty sure the paragraph refers to the problem of x86 of having
a structure like

struct ioctl_arg {
__u64 first;
__u32 second;
};

which is 12 bytes long on x86, but 16 bytes long including implied
padding on all 64-bit architectures and most (maybe all) 32-bit ones
other than x86.

If there is no 64-bit member in the struct, there is no need for padding
at the end.

Arnd

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/01/2016 04:44 PM, kbuild test robot wrote:

Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

 * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
   from/to a void __user * in the kernel. Try really hard not to delay this
   conversion or worse, fiddle the raw __u64 through your code since that
   diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?

Thanks,
Laura

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/01/2016 04:44 PM, kbuild test robot wrote:

Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

 * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
   from/to a void __user * in the kernel. Try really hard not to delay this
   conversion or worse, fiddle the raw __u64 through your code since that
   diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?

Thanks,
Laura