Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Kalle Valo]

Pali Rohár  writes:

> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.
>
> Signed-off-by: Pali Rohár 

Why? What issue does this fix?

-- 
Kalle Valo

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Kalle Valo]

Pali Rohár  writes:

> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.
>
> Signed-off-by: Pali Rohár 

Why? What issue does this fix?

-- 
Kalle Valo

Re: [GIT PULL] soc: samsung: Drivers for v4.11

[Krzysztof Kozlowski]

On Thu, Jan 26, 2017 at 04:59:45PM +0100, Linus Walleij wrote:
> On Fri, Jan 20, 2017 at 7:13 PM, Krzysztof Kozlowski  wrote:
> 
> > The following changes since commit 0c744ea4f77d72b3dcebb7a8f2684633ec79be88:
> >
> >   Linux 4.10-rc2 (2017-01-01 14:31:53 -0800)
> >
> > are available in the git repository at:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git 
> > samsung-drivers-soc-pmu-4.11
> 
> OK I pulled it and applied the final three patches from Marek
> on top.
> 
> Grrr I was based in v4.10-rc1, well I could easily merge in v4.10-rc2
> so no big deal.

Sorry for that but it is all because of broken ARMv8 build at rc1.

Thanks for merging,
Krzysztof

Re: [GIT PULL] soc: samsung: Drivers for v4.11

[Krzysztof Kozlowski]

On Thu, Jan 26, 2017 at 04:59:45PM +0100, Linus Walleij wrote:
> On Fri, Jan 20, 2017 at 7:13 PM, Krzysztof Kozlowski  wrote:
> 
> > The following changes since commit 0c744ea4f77d72b3dcebb7a8f2684633ec79be88:
> >
> >   Linux 4.10-rc2 (2017-01-01 14:31:53 -0800)
> >
> > are available in the git repository at:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git 
> > samsung-drivers-soc-pmu-4.11
> 
> OK I pulled it and applied the final three patches from Marek
> on top.
> 
> Grrr I was based in v4.10-rc1, well I could easily merge in v4.10-rc2
> so no big deal.

Sorry for that but it is all because of broken ARMv8 build at rc1.

Thanks for merging,
Krzysztof

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Kalle Valo]

Pali Rohár  writes:

> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.
>
> This patch also move code for requesting NVS data from userspace to driver
> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.
>
> Calibration NVS data for wl1251 are model specific. Every one device with
> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
>
> Default example wl1251-nvs.bin data found in linux-firmware repository and
> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
> invalid as it is not real device specific address, just example one.
>
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
>
> Signed-off-by: Pali Rohár 

[...]

> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];

No magic numbers, please. Replace all nvs offsets with proper defines to
make the code more readable.

-- 
Kalle Valo

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Kalle Valo]

Pali Rohár  writes:

> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.
>
> This patch also move code for requesting NVS data from userspace to driver
> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.
>
> Calibration NVS data for wl1251 are model specific. Every one device with
> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
>
> Default example wl1251-nvs.bin data found in linux-firmware repository and
> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
> invalid as it is not real device specific address, just example one.
>
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
>
> Signed-off-by: Pali Rohár 

[...]

> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];

No magic numbers, please. Replace all nvs offsets with proper defines to
make the code more readable.

-- 
Kalle Valo

Re: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

[Masami Hiramatsu]

On Wed, 25 Jan 2017 22:07:16 -0800
Ricardo Neri  wrote:

> Hi Masami,
> 
> On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Jan 2017 12:23:47 -0800
> > Ricardo Neri  wrote:
> > 
> > > The function insn_get_reg_offset requires a type to indicate whether
> > > the returned offset is that given by by the ModRM or the SIB byte.
> > > Callers of this function would need the definition of the type struct.
> > > This is not needed. Instead, auxiliary functions can be defined for
> > > this purpose.
> > > 
> > > When the operand is a register, the emulation code for User-Mode
> > > Instruction Prevention needs to know the offset of the register indicated
> > > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > > function for this purpose.
> > 
> > Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?
> 
> Do you mean exporting the structure that I mention above? The problem
> that I am trying to solve is that callers sometimes want to know the
> offset of the register encoded in the SiB or the ModRM bytes. I could
> use something 
> 
> insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
> insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)
> 
> Instead, I opted for
> 
> insn_get_reg_offset_rm(insn, regs)
> insn_get_reg_offset_sib(insn, regs)
> 
> to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.

OK, if so, I think you should export both of them at once, not only
insn_get_reg_offset_rm().

Thank you,

> 
> If you feel that the former makes more sense, I can change the
> implementation.
> 
> Thanks and BR,
> Ricardo
> > 
> > Thank you,
> > 
> > > 
> > > Cc: Dave Hansen 
> > > Cc: Adam Buchbinder 
> > > Cc: Colin Ian King 
> > > Cc: Lorenzo Stoakes 
> > > Cc: Qiaowei Ren 
> > > Cc: Arnaldo Carvalho de Melo 
> > > Cc: Masami Hiramatsu 
> > > Cc: Adrian Hunter 
> > > Cc: Kees Cook 
> > > Cc: Thomas Garnier 
> > > Cc: Peter Zijlstra 
> > > Cc: Borislav Petkov 
> > > Cc: Dmitry Vyukov 
> > > Cc: Ravi V. Shankar 
> > > Cc: x...@kernel.org
> > > Signed-off-by: Ricardo Neri 
> > > ---
> > >  arch/x86/include/asm/insn-kernel.h | 1 +
> > >  arch/x86/lib/insn-kernel.c | 5 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/insn-kernel.h 
> > > b/arch/x86/include/asm/insn-kernel.h
> > > index aef416a..3f34649 100644
> > > --- a/arch/x86/include/asm/insn-kernel.h
> > > +++ b/arch/x86/include/asm/insn-kernel.h
> > > @@ -12,5 +12,6 @@
> > >  #include 
> > >  
> > >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> > >  
> > >  #endif /* _ASM_X86_INSN_KERNEL_H */
> > > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > > index 8072abe..267cab4 100644
> > > --- a/arch/x86/lib/insn-kernel.c
> > > +++ b/arch/x86/lib/insn-kernel.c
> > > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> > > pt_regs *regs,
> > >   return regoff[regno];
> > >  }
> > >  
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > > +{
> > > + return get_reg_offset(insn, regs, REG_TYPE_RM);
> > > +}
> > > +
> > >  /*
> > >   * return the address being referenced be instruction
> > >   * for rm=3 returning the content of the rm reg
> > > -- 
> > > 2.9.3
> > > 
> > 
> > 
> 
> 


-- 
Masami Hiramatsu 

Re: [Xen-devel] [PATCH v2] xen, input: try to read screen resolution for xen-kbdfront

[Oleksandr Andrushchenko]

On 01/27/2017 09:46 AM, Juergen Gross wrote:

On 27/01/17 08:21, Oleksandr Andrushchenko wrote:

On 01/27/2017 09:12 AM, Juergen Gross wrote:

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
   drivers/input/misc/xen-kbdfront.c | 15 ---
   1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   @@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void
*dev_id)
   static int xenkbd_probe(struct xenbus_device *dev,
 const struct xenbus_device_id *id)
   {
-int ret, i;
+int ret, i, width, height;
   unsigned int abs;
   struct xenkbd_info *info;
   struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
   ptr->id.product = 0xfffe;
 if (abs) {
+width = XENFB_WIDTH;
+height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+if (registered_fb[0]) {

This still will not help if FB gets registered after kbd+ptr

Hmm, so you think I should add a call to fb_register_client() to get
events for new registered framebuffer devices?

yes, but also pay attention to CONFIG_FB_NOTIFY: you may still
end up w/o notification.


This would probably work. I'll have a try.


Thanks,

Juergen

My bigger concern here is that we try to tie keyboard and pointer device
to the framebuffer. IMO, these are independent parts of the system and 
the relation
depends on the use-case. One can have graphics enabled w/o framebuffer 
at all, e.g.

DRM/KMS + OpenGLES + Weston + kbd + ptr...

Re: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM

[Masami Hiramatsu]

On Wed, 25 Jan 2017 22:07:16 -0800
Ricardo Neri  wrote:

> Hi Masami,
> 
> On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Jan 2017 12:23:47 -0800
> > Ricardo Neri  wrote:
> > 
> > > The function insn_get_reg_offset requires a type to indicate whether
> > > the returned offset is that given by by the ModRM or the SIB byte.
> > > Callers of this function would need the definition of the type struct.
> > > This is not needed. Instead, auxiliary functions can be defined for
> > > this purpose.
> > > 
> > > When the operand is a register, the emulation code for User-Mode
> > > Instruction Prevention needs to know the offset of the register indicated
> > > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > > function for this purpose.
> > 
> > Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?
> 
> Do you mean exporting the structure that I mention above? The problem
> that I am trying to solve is that callers sometimes want to know the
> offset of the register encoded in the SiB or the ModRM bytes. I could
> use something 
> 
> insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
> insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)
> 
> Instead, I opted for
> 
> insn_get_reg_offset_rm(insn, regs)
> insn_get_reg_offset_sib(insn, regs)
> 
> to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.

OK, if so, I think you should export both of them at once, not only
insn_get_reg_offset_rm().

Thank you,

> 
> If you feel that the former makes more sense, I can change the
> implementation.
> 
> Thanks and BR,
> Ricardo
> > 
> > Thank you,
> > 
> > > 
> > > Cc: Dave Hansen 
> > > Cc: Adam Buchbinder 
> > > Cc: Colin Ian King 
> > > Cc: Lorenzo Stoakes 
> > > Cc: Qiaowei Ren 
> > > Cc: Arnaldo Carvalho de Melo 
> > > Cc: Masami Hiramatsu 
> > > Cc: Adrian Hunter 
> > > Cc: Kees Cook 
> > > Cc: Thomas Garnier 
> > > Cc: Peter Zijlstra 
> > > Cc: Borislav Petkov 
> > > Cc: Dmitry Vyukov 
> > > Cc: Ravi V. Shankar 
> > > Cc: x...@kernel.org
> > > Signed-off-by: Ricardo Neri 
> > > ---
> > >  arch/x86/include/asm/insn-kernel.h | 1 +
> > >  arch/x86/lib/insn-kernel.c | 5 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/insn-kernel.h 
> > > b/arch/x86/include/asm/insn-kernel.h
> > > index aef416a..3f34649 100644
> > > --- a/arch/x86/include/asm/insn-kernel.h
> > > +++ b/arch/x86/include/asm/insn-kernel.h
> > > @@ -12,5 +12,6 @@
> > >  #include 
> > >  
> > >  void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> > >  
> > >  #endif /* _ASM_X86_INSN_KERNEL_H */
> > > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > > index 8072abe..267cab4 100644
> > > --- a/arch/x86/lib/insn-kernel.c
> > > +++ b/arch/x86/lib/insn-kernel.c
> > > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct 
> > > pt_regs *regs,
> > >   return regoff[regno];
> > >  }
> > >  
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > > +{
> > > + return get_reg_offset(insn, regs, REG_TYPE_RM);
> > > +}
> > > +
> > >  /*
> > >   * return the address being referenced be instruction
> > >   * for rm=3 returning the content of the rm reg
> > > -- 
> > > 2.9.3
> > > 
> > 
> > 
> 
> 


-- 
Masami Hiramatsu 

Re: [Xen-devel] [PATCH v2] xen, input: try to read screen resolution for xen-kbdfront

[Oleksandr Andrushchenko]

On 01/27/2017 09:46 AM, Juergen Gross wrote:

On 27/01/17 08:21, Oleksandr Andrushchenko wrote:

On 01/27/2017 09:12 AM, Juergen Gross wrote:

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
   drivers/input/misc/xen-kbdfront.c | 15 ---
   1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   @@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void
*dev_id)
   static int xenkbd_probe(struct xenbus_device *dev,
 const struct xenbus_device_id *id)
   {
-int ret, i;
+int ret, i, width, height;
   unsigned int abs;
   struct xenkbd_info *info;
   struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
   ptr->id.product = 0xfffe;
 if (abs) {
+width = XENFB_WIDTH;
+height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+if (registered_fb[0]) {

This still will not help if FB gets registered after kbd+ptr

Hmm, so you think I should add a call to fb_register_client() to get
events for new registered framebuffer devices?

yes, but also pay attention to CONFIG_FB_NOTIFY: you may still
end up w/o notification.


This would probably work. I'll have a try.


Thanks,

Juergen

My bigger concern here is that we try to tie keyboard and pointer device
to the framebuffer. IMO, these are independent parts of the system and 
the relation
depends on the use-case. One can have graphics enabled w/o framebuffer 
at all, e.g.

DRM/KMS + OpenGLES + Weston + kbd + ptr...

Re: [PATCH 2/2] base/memory, hotplug: fix a kernel oops in show_valid_zones()

[gre...@linuxfoundation.org]

On Thu, Jan 26, 2017 at 10:26:23PM +, Kani, Toshimitsu wrote:
> On Thu, 2017-01-26 at 13:52 -0800, Andrew Morton wrote:
> > On Thu, 26 Jan 2017 14:44:15 -0700 Toshi Kani 
> > wrote:
> > 
> > > Reading a sysfs memoryN/valid_zones file leads to the following
> > > oops when the first page of a range is not backed by struct page.
> > > show_valid_zones() assumes that 'start_pfn' is always valid for
> > > page_zone().
> > > 
> > >  BUG: unable to handle kernel paging request at ea017a00
> > >  IP: show_valid_zones+0x6f/0x160
> > > 
> > > Since test_pages_in_a_zone() already checks holes, extend this
> > > function to return 'valid_start' and 'valid_end' for a given range.
> > > show_valid_zones() then proceeds with the valid range.
> > 
> > This doesn't apply to current mainline due to changes in
> > zone_can_shift().  Please redo and resend.
> 
> Sorry, I will rebase to the -mm tree and resend the patches.
> 
> > Please also update the changelog to provide sufficient information
> > for others to decide which kernel(s) need the fix.  In particular:
> > under what circumstances will it occur?  On real machines which real
> > people own?
> 
> Yes, this issue happens on real x86 machines with 64GiB or more memory.
>  On such systems, the memory block size is bumped up to 2GiB. [1]
> 
> Here is an example system.  0x324000 is only aligned by 1GiB and
> its memory block starts from 0x32, which is not backed by
> struct page.
> 
>  BIOS-e820: [mem 0x00324000-0x00603fff] usable
> 
> I will add the descriptions to the patch.

Should it also be backported to the stable kernels to resolve the issue
there?

thanks,

greg k-h

Re: [PATCH 2/2] base/memory, hotplug: fix a kernel oops in show_valid_zones()

[gre...@linuxfoundation.org]

On Thu, Jan 26, 2017 at 10:26:23PM +, Kani, Toshimitsu wrote:
> On Thu, 2017-01-26 at 13:52 -0800, Andrew Morton wrote:
> > On Thu, 26 Jan 2017 14:44:15 -0700 Toshi Kani 
> > wrote:
> > 
> > > Reading a sysfs memoryN/valid_zones file leads to the following
> > > oops when the first page of a range is not backed by struct page.
> > > show_valid_zones() assumes that 'start_pfn' is always valid for
> > > page_zone().
> > > 
> > >  BUG: unable to handle kernel paging request at ea017a00
> > >  IP: show_valid_zones+0x6f/0x160
> > > 
> > > Since test_pages_in_a_zone() already checks holes, extend this
> > > function to return 'valid_start' and 'valid_end' for a given range.
> > > show_valid_zones() then proceeds with the valid range.
> > 
> > This doesn't apply to current mainline due to changes in
> > zone_can_shift().  Please redo and resend.
> 
> Sorry, I will rebase to the -mm tree and resend the patches.
> 
> > Please also update the changelog to provide sufficient information
> > for others to decide which kernel(s) need the fix.  In particular:
> > under what circumstances will it occur?  On real machines which real
> > people own?
> 
> Yes, this issue happens on real x86 machines with 64GiB or more memory.
>  On such systems, the memory block size is bumped up to 2GiB. [1]
> 
> Here is an example system.  0x324000 is only aligned by 1GiB and
> its memory block starts from 0x32, which is not backed by
> struct page.
> 
>  BIOS-e820: [mem 0x00324000-0x00603fff] usable
> 
> I will add the descriptions to the patch.

Should it also be backported to the stable kernels to resolve the issue
there?

thanks,

greg k-h

Re: [PATCH v4 3/3] p54: convert to sysdata API

[Greg KH]

On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> > On Thu, Jan 19, 2017 at 12:38:57PM +0100, Greg KH wrote:
> > > On Thu, Jan 12, 2017 at 07:02:44AM -0800, Luis R. Rodriguez wrote:
> > > > ---
> > > >  drivers/net/wireless/intersil/p54/eeprom.c |  2 +-
> > > >  drivers/net/wireless/intersil/p54/fwio.c   |  5 +-
> > > >  drivers/net/wireless/intersil/p54/led.c|  2 +-
> > > >  drivers/net/wireless/intersil/p54/main.c   |  2 +-
> > > >  drivers/net/wireless/intersil/p54/p54.h|  3 +-
> > > >  drivers/net/wireless/intersil/p54/p54pci.c | 26 ++
> > > >  drivers/net/wireless/intersil/p54/p54pci.h |  4 +-
> > > >  drivers/net/wireless/intersil/p54/p54spi.c | 80 
> > > > +++---
> > > >  drivers/net/wireless/intersil/p54/p54spi.h |  2 +-
> > > >  drivers/net/wireless/intersil/p54/p54usb.c | 18 +++
> > > >  drivers/net/wireless/intersil/p54/p54usb.h |  4 +-
> > > >  drivers/net/wireless/intersil/p54/txrx.c   |  2 +-
> > > >  12 files changed, 89 insertions(+), 61 deletions(-)
> > > 
> > > why does the "new" api require more lines?
> > 
> > This is a bare bones flexible API with only a few new tiny features to start
> > with, one of them was to enable the API do the freeing of the driver data 
> > for
> > you. In the kernel we have devres to help with this but devres only helps if
> > you would use the API call on probe. We want to support the ability to let 
> > the
> > API free the driver data for you even if your call is outside of probe, for 
> > this
> > to work we need a callback. For async calls this is rather trivial given we
> > already have a callback, for sync calls this means a new routine is needed.
> > Freeing the data for you is an option, but I decided to keep the callback
> > requirement even if you didn't want the free'ing to be done for you. The
> > addition of a callback is what accounts for the slight increase on this 
> > driver.
> > 
> > I could try avoiding the callback if no freeing is needed.
> 
> OK I've added a respective helper call which would map 1-1 with the
> old sync mechanism to enable a 1-1 change, this will be called
> driver_data_request_simple(), but let me know if there is a preference
> for something else.
> 
> With this the only visible delta now is from taking advantage of new
> features. In p54's case this would re-organize the mess in
> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
> larger for that file just because of this but I think in this case
> its very much worth the small additions. In this case two routines are
> added for handling the work through callbacks on a sync call.
> 
>  1 file changed, 38 insertions(+), 30 deletions(-)

I agree with Linus, as well as, look, it's still bigger, so you are
making driver developers do more work :(

>   /* FIXME: should driver use it's own struct device? */
> - ret = request_firmware(>firmware, "3826.arm", >spi->dev);
> -
> - if (ret < 0) {
> - dev_err(>spi->dev, "request_firmware() failed: %d", ret);
> + ret = driver_data_request_simple("3826.arm", >spi->dev,
> +  >firmware);
> + if (ret < 0)
>   return ret;
> - }

Hm, a FIXME that you aren't fixing :(

I still fail to see why this new api is worth it at all, sorry.

greg k-h

Re: [PATCH v4 3/3] p54: convert to sysdata API

[Greg KH]

On Thu, Jan 26, 2017 at 10:50:05PM +0100, Luis R. Rodriguez wrote:
> On Thu, Jan 19, 2017 at 05:27:51PM +0100, Luis R. Rodriguez wrote:
> > On Thu, Jan 19, 2017 at 12:38:57PM +0100, Greg KH wrote:
> > > On Thu, Jan 12, 2017 at 07:02:44AM -0800, Luis R. Rodriguez wrote:
> > > > ---
> > > >  drivers/net/wireless/intersil/p54/eeprom.c |  2 +-
> > > >  drivers/net/wireless/intersil/p54/fwio.c   |  5 +-
> > > >  drivers/net/wireless/intersil/p54/led.c|  2 +-
> > > >  drivers/net/wireless/intersil/p54/main.c   |  2 +-
> > > >  drivers/net/wireless/intersil/p54/p54.h|  3 +-
> > > >  drivers/net/wireless/intersil/p54/p54pci.c | 26 ++
> > > >  drivers/net/wireless/intersil/p54/p54pci.h |  4 +-
> > > >  drivers/net/wireless/intersil/p54/p54spi.c | 80 
> > > > +++---
> > > >  drivers/net/wireless/intersil/p54/p54spi.h |  2 +-
> > > >  drivers/net/wireless/intersil/p54/p54usb.c | 18 +++
> > > >  drivers/net/wireless/intersil/p54/p54usb.h |  4 +-
> > > >  drivers/net/wireless/intersil/p54/txrx.c   |  2 +-
> > > >  12 files changed, 89 insertions(+), 61 deletions(-)
> > > 
> > > why does the "new" api require more lines?
> > 
> > This is a bare bones flexible API with only a few new tiny features to start
> > with, one of them was to enable the API do the freeing of the driver data 
> > for
> > you. In the kernel we have devres to help with this but devres only helps if
> > you would use the API call on probe. We want to support the ability to let 
> > the
> > API free the driver data for you even if your call is outside of probe, for 
> > this
> > to work we need a callback. For async calls this is rather trivial given we
> > already have a callback, for sync calls this means a new routine is needed.
> > Freeing the data for you is an option, but I decided to keep the callback
> > requirement even if you didn't want the free'ing to be done for you. The
> > addition of a callback is what accounts for the slight increase on this 
> > driver.
> > 
> > I could try avoiding the callback if no freeing is needed.
> 
> OK I've added a respective helper call which would map 1-1 with the
> old sync mechanism to enable a 1-1 change, this will be called
> driver_data_request_simple(), but let me know if there is a preference
> for something else.
> 
> With this the only visible delta now is from taking advantage of new
> features. In p54's case this would re-organize the mess in
> drivers/net/wireless/intersil/p54/p54spi.c, the diff stat is a bit
> larger for that file just because of this but I think in this case
> its very much worth the small additions. In this case two routines are
> added for handling the work through callbacks on a sync call.
> 
>  1 file changed, 38 insertions(+), 30 deletions(-)

I agree with Linus, as well as, look, it's still bigger, so you are
making driver developers do more work :(

>   /* FIXME: should driver use it's own struct device? */
> - ret = request_firmware(>firmware, "3826.arm", >spi->dev);
> -
> - if (ret < 0) {
> - dev_err(>spi->dev, "request_firmware() failed: %d", ret);
> + ret = driver_data_request_simple("3826.arm", >spi->dev,
> +  >firmware);
> + if (ret < 0)
>   return ret;
> - }

Hm, a FIXME that you aren't fixing :(

I still fail to see why this new api is worth it at all, sorry.

greg k-h

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár  writes:

> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
>
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
>
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.
>
> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
>
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.
>
> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.
>
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
> b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
>   tristate "TI wl1251 driver support"
>   depends on MAC80211
>   select FW_LOADER
> + select FW_LOADER_USER_HELPER
>   select CRC7
>   ---help---
> This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>   struct device *dev = wiphy_dev(wl->hw->wiphy);
>   int ret;
>  
> - ret = request_firmware(, WL1251_NVS_NAME, dev);
> + ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);

I don't see the need for this. Just remove the default nvs file from
filesystem and the fallback user helper will be always used, right?

Like we discussed earlier, the default nvs file should not be used by
normal users.

-- 
Kalle Valo

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Kalle Valo]

Pali Rohár  writes:

> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
>
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
>
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.
>
> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
>
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.
>
> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.
>
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/Kconfig |1 +
>  drivers/net/wireless/ti/wl1251/main.c  |2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
> b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
>   tristate "TI wl1251 driver support"
>   depends on MAC80211
>   select FW_LOADER
> + select FW_LOADER_USER_HELPER
>   select CRC7
>   ---help---
> This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>   struct device *dev = wiphy_dev(wl->hw->wiphy);
>   int ret;
>  
> - ret = request_firmware(, WL1251_NVS_NAME, dev);
> + ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);

I don't see the need for this. Just remove the default nvs file from
filesystem and the fallback user helper will be always used, right?

Like we discussed earlier, the default nvs file should not be used by
normal users.

-- 
Kalle Valo

Re: [PATCH v2 0/4] USB support for Broadcom NSP SoC

[Kishon Vijay Abraham I]

Hi,

On Thursday 26 January 2017 10:57 PM, Florian Fainelli wrote:
> On 01/26/2017 07:34 AM, Kishon Vijay Abraham I wrote:
>>
>>
>> On Tuesday 17 January 2017 09:44 PM, Yendapally Reddy Dhananjaya Reddy wrote:
>>> This patch set contains the usb support for Broadcom NSP SoC. The
>>> usb3 phy is internal to the SoC and is accessed through mdio interface.
>>> The mdio interface can be used to access either internal usb3 phy or
>>> external ethernet phy using a multiplexer.
>>>
>>> The first patch provides the documentation details for usb3 phy. The
>>> second patch provides the changes to the mdio bus driver. The third
>>> patch provides the phy driver and fourth patch provides the enable
>>> method for usb.
>>
>> merged the 1st and 4th patch to linux-phy.
> 
> You mean 1st and 3rd here, right? 4th is a Device Tree change that I
> should take, and Patch 2 should be merged by David.

yeah, sorry!
> 
> What branch in your tree should we be looking at?

git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next

Thanks
Kishon

Re: [PATCH v2 0/4] USB support for Broadcom NSP SoC

[Kishon Vijay Abraham I]

Hi,

On Thursday 26 January 2017 10:57 PM, Florian Fainelli wrote:
> On 01/26/2017 07:34 AM, Kishon Vijay Abraham I wrote:
>>
>>
>> On Tuesday 17 January 2017 09:44 PM, Yendapally Reddy Dhananjaya Reddy wrote:
>>> This patch set contains the usb support for Broadcom NSP SoC. The
>>> usb3 phy is internal to the SoC and is accessed through mdio interface.
>>> The mdio interface can be used to access either internal usb3 phy or
>>> external ethernet phy using a multiplexer.
>>>
>>> The first patch provides the documentation details for usb3 phy. The
>>> second patch provides the changes to the mdio bus driver. The third
>>> patch provides the phy driver and fourth patch provides the enable
>>> method for usb.
>>
>> merged the 1st and 4th patch to linux-phy.
> 
> You mean 1st and 3rd here, right? 4th is a Device Tree change that I
> should take, and Patch 2 should be merged by David.

yeah, sorry!
> 
> What branch in your tree should we be looking at?

git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next

Thanks
Kishon

Re: [PATCH] drm/ttm: Make sure BOs being swapped out are cacheable

[Daniel Vetter]

On Fri, Jan 27, 2017 at 07:23:58AM +0100, Thomas Hellstrom wrote:
> On 01/27/2017 03:29 AM, Michel Dänzer wrote:
> > On 26/01/17 09:46 AM, Sinclair Yeh wrote:
> >> On Wed, Jan 25, 2017 at 10:49:33AM +0100, Christian König wrote:
> >>> Am 25.01.2017 um 10:25 schrieb Thomas Hellstrom:
>  On 01/25/2017 09:21 AM, Michel Dänzer wrote:
> > From: Michel Dänzer 
> >
> > The current caching state may not be tt_cached, even though the
> > placement contains TTM_PL_FLAG_CACHED, because placement can contain
> > multiple caching flags. Trying to swap out such a BO would trip up the
> >
> > BUG_ON(ttm->caching_state != tt_cached);
> >
> > in ttm_tt_swapout.
> >
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Michel Dänzer 
>  Reviewed-by: Thomas Hellstrom 
> >>> Reviewed-by: Christian König .
> >> Reviewed-by: Sinclair Yeh 
> > Thanks for the reviews! Via which tree should we merge this?
> >
> >
> I don't maintain a TTM tree any longer. Let's check with Daniel if he
> can merge it through drm-misc.

I'm trying very hard not to get volunteered for ttm maintainer :-)
Nominally Alex have drm-misc commit rights, but they haven't
used them yet. But I think merging through drm-misc would make sense,
there's regular pull request trains for both -next and -fixes. Or merge
through the amd tree with Dave's ack, but I'd really like to get amd folks
into the drm-misc group ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/ttm: Make sure BOs being swapped out are cacheable

[Daniel Vetter]

On Fri, Jan 27, 2017 at 07:23:58AM +0100, Thomas Hellstrom wrote:
> On 01/27/2017 03:29 AM, Michel Dänzer wrote:
> > On 26/01/17 09:46 AM, Sinclair Yeh wrote:
> >> On Wed, Jan 25, 2017 at 10:49:33AM +0100, Christian König wrote:
> >>> Am 25.01.2017 um 10:25 schrieb Thomas Hellstrom:
>  On 01/25/2017 09:21 AM, Michel Dänzer wrote:
> > From: Michel Dänzer 
> >
> > The current caching state may not be tt_cached, even though the
> > placement contains TTM_PL_FLAG_CACHED, because placement can contain
> > multiple caching flags. Trying to swap out such a BO would trip up the
> >
> > BUG_ON(ttm->caching_state != tt_cached);
> >
> > in ttm_tt_swapout.
> >
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Michel Dänzer 
>  Reviewed-by: Thomas Hellstrom 
> >>> Reviewed-by: Christian König .
> >> Reviewed-by: Sinclair Yeh 
> > Thanks for the reviews! Via which tree should we merge this?
> >
> >
> I don't maintain a TTM tree any longer. Let's check with Daniel if he
> can merge it through drm-misc.

I'm trying very hard not to get volunteered for ttm maintainer :-)
Nominally Alex have drm-misc commit rights, but they haven't
used them yet. But I think merging through drm-misc would make sense,
there's regular pull request trains for both -next and -fixes. Or merge
through the amd tree with Dave's ack, but I'd really like to get amd folks
into the drm-misc group ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: mm, vmscan: commit makes PAE kernel crash nightly (bisected)

[Michal Hocko]

On Thu 26-01-17 17:18:58, Trevor Cordes wrote:
> On 2017-01-24 Michal Hocko wrote:
> > On Sun 22-01-17 18:45:59, Trevor Cordes wrote:
> > [...]
> > > Also, completely separate from your patch I ran mhocko's 4.9 tree
> > > with mem=2G to see if lower ram amount would help, but it didn't.
> > > Even with 2G the system oom and hung same as usual.  So far the
> > > only thing that helps at all was the cgroup_disable=memory option,
> > > which makes the problem disappear completely for me.  
> > 
> > OK, can we reduce the problem space slightly more and could you boot
> > with kmem accounting enabled? cgroup.memory=nokmem,nosocket
> 
> I ran for 30 hours with cgroup.memory=nokmem,nosocket using vanilla
> 4.9.0+ and it oom'd during a big rdiff-backup at 9am.  My script was
> able to reboot it before it hung.  Only one oom occurred before the
> reboot, which is a bit odd, usually there is 5-50.  See attached
> messages log (oom6).
> 
> So, still, only cgroup_disable=memory mitigates this bug (so far).  If
> you need me to test cgroup.memory=nokmem,nosocket with your since-4.9
> branch specifically, let me know and I'll add it to the to-test list.

OK, that matches the theory that these OOMs are caused by the incorrect
active list aging fixed by b4536f0c829c ("mm, memcg: fix the active list
aging for lowmem requests when memcg is enabled")
-- 
Michal Hocko
SUSE Labs

Re: mm, vmscan: commit makes PAE kernel crash nightly (bisected)

[Michal Hocko]

On Thu 26-01-17 17:18:58, Trevor Cordes wrote:
> On 2017-01-24 Michal Hocko wrote:
> > On Sun 22-01-17 18:45:59, Trevor Cordes wrote:
> > [...]
> > > Also, completely separate from your patch I ran mhocko's 4.9 tree
> > > with mem=2G to see if lower ram amount would help, but it didn't.
> > > Even with 2G the system oom and hung same as usual.  So far the
> > > only thing that helps at all was the cgroup_disable=memory option,
> > > which makes the problem disappear completely for me.  
> > 
> > OK, can we reduce the problem space slightly more and could you boot
> > with kmem accounting enabled? cgroup.memory=nokmem,nosocket
> 
> I ran for 30 hours with cgroup.memory=nokmem,nosocket using vanilla
> 4.9.0+ and it oom'd during a big rdiff-backup at 9am.  My script was
> able to reboot it before it hung.  Only one oom occurred before the
> reboot, which is a bit odd, usually there is 5-50.  See attached
> messages log (oom6).
> 
> So, still, only cgroup_disable=memory mitigates this bug (so far).  If
> you need me to test cgroup.memory=nokmem,nosocket with your since-4.9
> branch specifically, let me know and I'll add it to the to-test list.

OK, that matches the theory that these OOMs are caused by the incorrect
active list aging fixed by b4536f0c829c ("mm, memcg: fix the active list
aging for lowmem requests when memcg is enabled")
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] ARM: dts: Odroid XU4: fix USB3.0 ports

[Richard Genoud]

On 25/01/2017 15:17, Krzysztof Kozlowski wrote:
> On Wed, Jan 25, 2017 at 3:48 PM, Marek Szyprowski
>  wrote:
>> Hi Krzysztof,
>>
>> On 2017-01-25 08:55, Krzysztof Kozlowski wrote:
>>>
>>> On Wed, Jan 25, 2017 at 7:51 AM, Anand Moon  wrote:

 On 24 January 2017 at 19:18, Richard Genoud 
 wrote:
>
> Since commit 2164a476205ccc ("usb: dwc3: set SUSPHY bit for all cores"),
> the USB ports on odroid-XU4 don't work anymore.
>
> Inserting an usb key (USB2.0) on the USB3.0 port result in:
> [ 64.488264] xhci-hcd xhci-hcd.2.auto: Port resume took longer than
> 2 msec, port status = 0xc400fe3
> [   74.568156] xhci-hcd xhci-hcd.2.auto: xHCI host not responding to
> stop endpoint command.
> [   74.574806] xhci-hcd xhci-hcd.2.auto: Assuming host is dying, halting
> host.
> [   74.601970] xhci-hcd xhci-hcd.2.auto: HC died; cleaning up
> [   74.606276] usb 3-1: USB disconnect, device number 2
> [   74.613565] usb 4-1: USB disconnect, device number 2
> [   74.621208] usb usb3-port1: couldn't allocate usb_device
> NB: it's not related to USB2.0 devices, I get the same result with an
> USB3.0 device (SATA to USB3 for instance).
> NB2: it doesn't happen on an odriod-XU3 board, that doesn't have the
> realtek RTL8153 chip.
>
> If the lines:
>  if (dwc->revision > DWC3_REVISION_194A)
>  reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> are commented out, the USB3.0 start working.
>
> For a full analyse: https://lkml.org/lkml/2017/1/18/678
>
> Felipe suggested that suspend should be disabled temporarily while
> what's wrong with DCW3 is figured out.
>
> Tested on Odroid XU4
>
> Suggested-by: Felipe Balbi 
> Tested-by: Richard Genoud 
> Signed-off-by: Richard Genoud 
> Cc: sta...@vger.kernel.org # 4.4+
> Fixes: 2164a476205ccc ("usb: dwc3: set SUSPHY bit for all cores")
> ---
>   arch/arm/boot/dts/exynos5422-odroidxu4.dts | 9 +
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> b/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> index 2faf88627a48..f62dbd9b5ad3 100644
> --- a/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> @@ -43,6 +43,15 @@
>  status = "okay";
>   };
>
> +_dwc3_0 {
> +   /*
> +* without that, usb devices are not recognized when
> +* they are plugged.
> +* cf: https://lkml.org/lkml/2017/1/18/678
> +*/
> +   snps,dis_u3_susphy_quirk;
> +};
> +
>   _dwc3_1 {
>  dr_mode = "host";
>   };
> --

 Thanks for this patch.
 But could you consider moving this changes as below.

 https://lkml.org/lkml/2015/3/18/400
>>>
>>> The patch above (and other DTS patches from the set) was not even sent
>>> to linux-samsung-soc nor to me... It is sad how easily one's work can
>>> disappear. Also, it is really worthless to solve the same problem
>>> twice.
>>
>>
>> Well, they were sent about 2 years ago... and you were also working on this
>> topic. ;)
> 
> Nope, I have never worked on this. That time I was in Korea so my
> tasks were completely different. Later when I got back, I took for a
> second U3 OTG which was not involving this thing.
> 
>>> Cc Marek and Bartlomiej,
>>> Guys, do you want to continue with Robert's patch (linked above by
>>> Anand)? If yes, please take the ownership (Robert's address is not
>>> valid anymore). If not, I will take Richard's patch after
>>> resubmission.
>>
>>
>> Take Richard's patch because it has a bit more details describing why such
>> quirk
>> is needed.
>>
>> Richard: in Roberts patch there is also a quirk for USB 2.0 mode
>> (snps,dis_u2_susphy_quirk). Could you check if it really needed?
>>
>> Maybe it would make sense to set those quirks for both DWC3 controllers, as
>> this
>> issue with PHY suspend seems to be a Exynos specific thing.
> 
> Okay,
> Richard, please continue with your patch.
Ok, I'll grab a XU3 & XU4 and do some more tests.
But I didn't recall having problems with the XU3 board. I'll try to add
a hub or something.

Re: [PATCH] ARM: dts: Odroid XU4: fix USB3.0 ports

[Richard Genoud]

On 25/01/2017 15:17, Krzysztof Kozlowski wrote:
> On Wed, Jan 25, 2017 at 3:48 PM, Marek Szyprowski
>  wrote:
>> Hi Krzysztof,
>>
>> On 2017-01-25 08:55, Krzysztof Kozlowski wrote:
>>>
>>> On Wed, Jan 25, 2017 at 7:51 AM, Anand Moon  wrote:

 On 24 January 2017 at 19:18, Richard Genoud 
 wrote:
>
> Since commit 2164a476205ccc ("usb: dwc3: set SUSPHY bit for all cores"),
> the USB ports on odroid-XU4 don't work anymore.
>
> Inserting an usb key (USB2.0) on the USB3.0 port result in:
> [ 64.488264] xhci-hcd xhci-hcd.2.auto: Port resume took longer than
> 2 msec, port status = 0xc400fe3
> [   74.568156] xhci-hcd xhci-hcd.2.auto: xHCI host not responding to
> stop endpoint command.
> [   74.574806] xhci-hcd xhci-hcd.2.auto: Assuming host is dying, halting
> host.
> [   74.601970] xhci-hcd xhci-hcd.2.auto: HC died; cleaning up
> [   74.606276] usb 3-1: USB disconnect, device number 2
> [   74.613565] usb 4-1: USB disconnect, device number 2
> [   74.621208] usb usb3-port1: couldn't allocate usb_device
> NB: it's not related to USB2.0 devices, I get the same result with an
> USB3.0 device (SATA to USB3 for instance).
> NB2: it doesn't happen on an odriod-XU3 board, that doesn't have the
> realtek RTL8153 chip.
>
> If the lines:
>  if (dwc->revision > DWC3_REVISION_194A)
>  reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> are commented out, the USB3.0 start working.
>
> For a full analyse: https://lkml.org/lkml/2017/1/18/678
>
> Felipe suggested that suspend should be disabled temporarily while
> what's wrong with DCW3 is figured out.
>
> Tested on Odroid XU4
>
> Suggested-by: Felipe Balbi 
> Tested-by: Richard Genoud 
> Signed-off-by: Richard Genoud 
> Cc: sta...@vger.kernel.org # 4.4+
> Fixes: 2164a476205ccc ("usb: dwc3: set SUSPHY bit for all cores")
> ---
>   arch/arm/boot/dts/exynos5422-odroidxu4.dts | 9 +
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> b/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> index 2faf88627a48..f62dbd9b5ad3 100644
> --- a/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu4.dts
> @@ -43,6 +43,15 @@
>  status = "okay";
>   };
>
> +_dwc3_0 {
> +   /*
> +* without that, usb devices are not recognized when
> +* they are plugged.
> +* cf: https://lkml.org/lkml/2017/1/18/678
> +*/
> +   snps,dis_u3_susphy_quirk;
> +};
> +
>   _dwc3_1 {
>  dr_mode = "host";
>   };
> --

 Thanks for this patch.
 But could you consider moving this changes as below.

 https://lkml.org/lkml/2015/3/18/400
>>>
>>> The patch above (and other DTS patches from the set) was not even sent
>>> to linux-samsung-soc nor to me... It is sad how easily one's work can
>>> disappear. Also, it is really worthless to solve the same problem
>>> twice.
>>
>>
>> Well, they were sent about 2 years ago... and you were also working on this
>> topic. ;)
> 
> Nope, I have never worked on this. That time I was in Korea so my
> tasks were completely different. Later when I got back, I took for a
> second U3 OTG which was not involving this thing.
> 
>>> Cc Marek and Bartlomiej,
>>> Guys, do you want to continue with Robert's patch (linked above by
>>> Anand)? If yes, please take the ownership (Robert's address is not
>>> valid anymore). If not, I will take Richard's patch after
>>> resubmission.
>>
>>
>> Take Richard's patch because it has a bit more details describing why such
>> quirk
>> is needed.
>>
>> Richard: in Roberts patch there is also a quirk for USB 2.0 mode
>> (snps,dis_u2_susphy_quirk). Could you check if it really needed?
>>
>> Maybe it would make sense to set those quirks for both DWC3 controllers, as
>> this
>> issue with PHY suspend seems to be a Exynos specific thing.
> 
> Okay,
> Richard, please continue with your patch.
Ok, I'll grab a XU3 & XU4 and do some more tests.
But I didn't recall having problems with the XU3 board. I'll try to add
a hub or something.

Re: [PATCH] staging: octeon-usb: Fix coding style issues

[Greg Kroah-Hartman]

On Thu, Jan 26, 2017 at 10:46:02PM -0500, William Blough wrote:
> Wrap lines that exceed 80 characters.
> 
> Signed-off-by: William Blough 
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
> b/drivers/staging/octeon-usb/octeon-hcd.c
> index 9a7858a..6b86bfb 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -2635,13 +2635,19 @@ static int cvmx_usb_poll_channel(struct octeon_hcd 
> *usb, int channel)
>   /* Disable all interrupts except CHHLTD */
>   hcintmsk.u32 = 0;
>   hcintmsk.s.chhltdmsk = 1;
> - cvmx_usb_write_csr32(usb,
> -  
> CVMX_USBCX_HCINTMSKX(channel, usb->index),
> -  hcintmsk.u32);
> + cvmx_usb_write_csr32(
> + usb,
> + CVMX_USBCX_HCINTMSKX(
> + channel,
> + usb->index),
> + hcintmsk.u32);
>   usbc_hcchar.s.chdis = 1;
> - cvmx_usb_write_csr32(usb,
> -  
> CVMX_USBCX_HCCHARX(channel, usb->index),
> -  usbc_hcchar.u32);
> + cvmx_usb_write_csr32(
> + usb,
> + CVMX_USBCX_HCCHARX(
> + channel,
> + usb->index),
> + usbc_hcchar.u32);
>   return 0;
>   } else if (usbc_hcint.s.xfercompl) {
>   /*

That's horrid, the original code looks better, please leave it as-is.

thanks,

greg k-h

Re: [PATCH] staging: octeon-usb: Fix coding style issues

[Greg Kroah-Hartman]

On Thu, Jan 26, 2017 at 10:46:02PM -0500, William Blough wrote:
> Wrap lines that exceed 80 characters.
> 
> Signed-off-by: William Blough 
> ---
>  drivers/staging/octeon-usb/octeon-hcd.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
> b/drivers/staging/octeon-usb/octeon-hcd.c
> index 9a7858a..6b86bfb 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -2635,13 +2635,19 @@ static int cvmx_usb_poll_channel(struct octeon_hcd 
> *usb, int channel)
>   /* Disable all interrupts except CHHLTD */
>   hcintmsk.u32 = 0;
>   hcintmsk.s.chhltdmsk = 1;
> - cvmx_usb_write_csr32(usb,
> -  
> CVMX_USBCX_HCINTMSKX(channel, usb->index),
> -  hcintmsk.u32);
> + cvmx_usb_write_csr32(
> + usb,
> + CVMX_USBCX_HCINTMSKX(
> + channel,
> + usb->index),
> + hcintmsk.u32);
>   usbc_hcchar.s.chdis = 1;
> - cvmx_usb_write_csr32(usb,
> -  
> CVMX_USBCX_HCCHARX(channel, usb->index),
> -  usbc_hcchar.u32);
> + cvmx_usb_write_csr32(
> + usb,
> + CVMX_USBCX_HCCHARX(
> + channel,
> + usb->index),
> + usbc_hcchar.u32);
>   return 0;
>   } else if (usbc_hcint.s.xfercompl) {
>   /*

That's horrid, the original code looks better, please leave it as-is.

thanks,

greg k-h

Re: [Xen-devel] [PATCH v2] xen, input: try to read screen resolution for xen-kbdfront

[Oleksandr Andrushchenko]

On 01/27/2017 09:12 AM, Juergen Gross wrote:

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
  drivers/input/misc/xen-kbdfront.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void *dev_id)

  static int xenkbd_probe(struct xenbus_device *dev,
  const struct xenbus_device_id *id)
  {
-   int ret, i;
+   int ret, i, width, height;
unsigned int abs;
struct xenkbd_info *info;
struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
ptr->id.product = 0xfffe;
  
  	if (abs) {

+   width = XENFB_WIDTH;
+   height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+   if (registered_fb[0]) {

This still will not help if FB gets registered after kbd+ptr

+   width = registered_fb[0]->var.xres_virtual;
+   height = registered_fb[0]->var.yres_virtual;
+   }
+#endif
__set_bit(EV_ABS, ptr->evbit);
-   input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
-   input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+   input_set_abs_params(ptr, ABS_X, 0, width, 0, 0);
+   input_set_abs_params(ptr, ABS_Y, 0, height, 0, 0);
} else {
input_set_capability(ptr, EV_REL, REL_X);
input_set_capability(ptr, EV_REL, REL_Y);

Re: [Xen-devel] [PATCH v2] xen, input: try to read screen resolution for xen-kbdfront

[Oleksandr Andrushchenko]

On 01/27/2017 09:12 AM, Juergen Gross wrote:

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
  drivers/input/misc/xen-kbdfront.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void *dev_id)

  static int xenkbd_probe(struct xenbus_device *dev,
  const struct xenbus_device_id *id)
  {
-   int ret, i;
+   int ret, i, width, height;
unsigned int abs;
struct xenkbd_info *info;
struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
ptr->id.product = 0xfffe;
  
  	if (abs) {

+   width = XENFB_WIDTH;
+   height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+   if (registered_fb[0]) {

This still will not help if FB gets registered after kbd+ptr

+   width = registered_fb[0]->var.xres_virtual;
+   height = registered_fb[0]->var.yres_virtual;
+   }
+#endif
__set_bit(EV_ABS, ptr->evbit);
-   input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
-   input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+   input_set_abs_params(ptr, ABS_X, 0, width, 0, 0);
+   input_set_abs_params(ptr, ABS_Y, 0, height, 0, 0);
} else {
input_set_capability(ptr, EV_REL, REL_X);
input_set_capability(ptr, EV_REL, REL_Y);

[PATCH v2] xen,input: try to read screen resolution for xen-kbdfront

[Juergen Gross]

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
 drivers/input/misc/xen-kbdfront.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void *dev_id)
 static int xenkbd_probe(struct xenbus_device *dev,
  const struct xenbus_device_id *id)
 {
-   int ret, i;
+   int ret, i, width, height;
unsigned int abs;
struct xenkbd_info *info;
struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
ptr->id.product = 0xfffe;
 
if (abs) {
+   width = XENFB_WIDTH;
+   height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+   if (registered_fb[0]) {
+   width = registered_fb[0]->var.xres_virtual;
+   height = registered_fb[0]->var.yres_virtual;
+   }
+#endif
__set_bit(EV_ABS, ptr->evbit);
-   input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
-   input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+   input_set_abs_params(ptr, ABS_X, 0, width, 0, 0);
+   input_set_abs_params(ptr, ABS_Y, 0, height, 0, 0);
} else {
input_set_capability(ptr, EV_REL, REL_X);
input_set_capability(ptr, EV_REL, REL_Y);
-- 
2.10.2

[PATCH v2] xen,input: try to read screen resolution for xen-kbdfront

[Juergen Gross]

Instead of using the default resolution of 800*600 for the pointing
device of xen-kbdfront try to read the resolution of the (virtual)
framebuffer device. Use the default as fallback only.

Signed-off-by: Juergen Gross 
---
V2: get framebuffer resolution only if CONFIG_FB (Dmitry Torokhov)
---
 drivers/input/misc/xen-kbdfront.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index 3900875..3aae9b4 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -108,7 +109,7 @@ static irqreturn_t input_handler(int rq, void *dev_id)
 static int xenkbd_probe(struct xenbus_device *dev,
  const struct xenbus_device_id *id)
 {
-   int ret, i;
+   int ret, i, width, height;
unsigned int abs;
struct xenkbd_info *info;
struct input_dev *kbd, *ptr;
@@ -173,9 +174,17 @@ static int xenkbd_probe(struct xenbus_device *dev,
ptr->id.product = 0xfffe;
 
if (abs) {
+   width = XENFB_WIDTH;
+   height = XENFB_HEIGHT;
+#ifdef CONFIG_FB
+   if (registered_fb[0]) {
+   width = registered_fb[0]->var.xres_virtual;
+   height = registered_fb[0]->var.yres_virtual;
+   }
+#endif
__set_bit(EV_ABS, ptr->evbit);
-   input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
-   input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+   input_set_abs_params(ptr, ABS_X, 0, width, 0, 0);
+   input_set_abs_params(ptr, ABS_Y, 0, height, 0, 0);
} else {
input_set_capability(ptr, EV_REL, REL_X);
input_set_capability(ptr, EV_REL, REL_Y);
-- 
2.10.2

Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

[Guenter Roeck]

On 01/26/2017 07:39 PM, Zhang Rui wrote:

On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote:

On 01/26/2017 05:37 PM, Zhang Rui wrote:


On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote:


On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:



Hi!
















Right.

Before reverting, can you please try if this patch
works
or not?

Not really. Revert now. Sorry.

Are you sure? This does not look equivalent to me at
all.

"name" file handling moved from drivers to the core,
which
added some
crazy checks what name can contain. Even if this
"works",
what is the
expected effect on the "name" file?


The hwmon name attribute must not include '-', as
documented
in
Documentation/hwmon/sysfs-interface. Is enforcing that
'crazy' ?
Maybe in your world, but not in mine.

Well, lets revert the patch and then we can discuss what to
do
with
the "name" problem.

Ok, so the patch is on the way in. What to do next?

pavel@n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel@n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery




To provide some detail: libsensors gets just as confused with
wildcards
and whitespace/newline as it does with '-' in the reported
name,
which
is why those are blocked by the new API.

Ok... Question is "does someone actually use hwmon*/name on
N900"?
If
so, we can't change it, but it is well possible that noone is.

IIRC hwmon is used on Nokia N900.

But I have not seen hwmon devices for bq27200 and rx51-battery
yet.
Those are power supply driver and auto-exporting them also via
hwmon
is
something new, right? If yes, then we can use any name for those
new
hwmon devices as they cannot break userspace... as there is no
userspace
application for them.


If this is the case, you'd better set
(struct thermal_zone_params)->no_hwmon when registering the thermal
zone device, in which case, the hwmon device will not be created.

In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not
create
hwmon I/F by default, and see if there is anyone using it. If yes,
we
can set the flag in soc thermal driver, explicitly, at meantime, a
hwmon compatible name is required.

But one foreseeable result is that we may get bug reports from end
user
that some sensors (acpitz, etc) are gone in 'sensors' output. And
TBH,
I'm not quite sure if this can be counted as a regression or not.


That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by
the ABI Police, but taking the entire device away is ok.


No. IMO, it depends on if the interface is used or not.
If hwmon I/F is used, we can not take it away, nor change its name.


Even if the use doesn't depend on that name ?


If thermal zone I/F is used, we can not change it's 'type' name to be
compatible with new hwmon API.



You mean you can not fix the name to be compatible with libsensors.

Makes me wonder if there shouldn't be a rule that exploits must not
be fixed if already exploited.

Guenter


Anyway, sounds good to me. No one will use something that isn't
there,
and no one will realize that it could have been there, so I don't
expect
anyone to complain.


Yes, I agree.

thanks,
rui

Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

[Guenter Roeck]

On 01/26/2017 07:39 PM, Zhang Rui wrote:

On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote:

On 01/26/2017 05:37 PM, Zhang Rui wrote:


On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote:


On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:



Hi!
















Right.

Before reverting, can you please try if this patch
works
or not?

Not really. Revert now. Sorry.

Are you sure? This does not look equivalent to me at
all.

"name" file handling moved from drivers to the core,
which
added some
crazy checks what name can contain. Even if this
"works",
what is the
expected effect on the "name" file?


The hwmon name attribute must not include '-', as
documented
in
Documentation/hwmon/sysfs-interface. Is enforcing that
'crazy' ?
Maybe in your world, but not in mine.

Well, lets revert the patch and then we can discuss what to
do
with
the "name" problem.

Ok, so the patch is on the way in. What to do next?

pavel@n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel@n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery




To provide some detail: libsensors gets just as confused with
wildcards
and whitespace/newline as it does with '-' in the reported
name,
which
is why those are blocked by the new API.

Ok... Question is "does someone actually use hwmon*/name on
N900"?
If
so, we can't change it, but it is well possible that noone is.

IIRC hwmon is used on Nokia N900.

But I have not seen hwmon devices for bq27200 and rx51-battery
yet.
Those are power supply driver and auto-exporting them also via
hwmon
is
something new, right? If yes, then we can use any name for those
new
hwmon devices as they cannot break userspace... as there is no
userspace
application for them.


If this is the case, you'd better set
(struct thermal_zone_params)->no_hwmon when registering the thermal
zone device, in which case, the hwmon device will not be created.

In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not
create
hwmon I/F by default, and see if there is anyone using it. If yes,
we
can set the flag in soc thermal driver, explicitly, at meantime, a
hwmon compatible name is required.

But one foreseeable result is that we may get bug reports from end
user
that some sensors (acpitz, etc) are gone in 'sensors' output. And
TBH,
I'm not quite sure if this can be counted as a regression or not.


That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by
the ABI Police, but taking the entire device away is ok.


No. IMO, it depends on if the interface is used or not.
If hwmon I/F is used, we can not take it away, nor change its name.


Even if the use doesn't depend on that name ?


If thermal zone I/F is used, we can not change it's 'type' name to be
compatible with new hwmon API.



You mean you can not fix the name to be compatible with libsensors.

Makes me wonder if there shouldn't be a rule that exploits must not
be fixed if already exploited.

Guenter


Anyway, sounds good to me. No one will use something that isn't
there,
and no one will realize that it could have been there, so I don't
expect
anyone to complain.


Yes, I agree.

thanks,
rui

Re: [PATCH 3/3] powerpc: enable support for GCC plugins

[Andrew Donnellan]

On 09/12/16 21:59, PaX Team wrote:

the specific problem addressed here can (and IMHO should) be solved in
another way: remove the inclusion of the offending headers in gcc-common.h
as neither tm.h nor c-common.h are needed by existing plugins. for background,


We can't build without tm.h: http://pastebin.com/W0azfCr0


you'll need to repeat the removal of dependent headers. based on a quick
test here across gcc 4.5-6.2, if you remove rtl.h, tm_p.h, hard-reg-set.h
and emit-rtl.h in addition to tm.h, the plugins should build fine.


OK, I finally have a chance to look at this series again.

basic-block.h includes tm.h, and I don't believe we can remove that. I'm 
not convinced there's a way around this.


--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

Re: [PATCH 3/3] powerpc: enable support for GCC plugins

[Andrew Donnellan]

On 09/12/16 21:59, PaX Team wrote:

the specific problem addressed here can (and IMHO should) be solved in
another way: remove the inclusion of the offending headers in gcc-common.h
as neither tm.h nor c-common.h are needed by existing plugins. for background,


We can't build without tm.h: http://pastebin.com/W0azfCr0


you'll need to repeat the removal of dependent headers. based on a quick
test here across gcc 4.5-6.2, if you remove rtl.h, tm_p.h, hard-reg-set.h
and emit-rtl.h in addition to tm.h, the plugins should build fine.


OK, I finally have a chance to look at this series again.

basic-block.h includes tm.h, and I don't believe we can remove that. I'm 
not convinced there's a way around this.


--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:45:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > > In a TPM2, sessions can be globally exhausted once there are
> > > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > > saved). The Strategy for handling this is to keep a global count of 
> > > all the sessions along with their creation time.  Then if we see 
> > > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > > first wait for one to become free, but if it doesn't, we forcibly 
> > > evict an existing one. The eviction strategy waits until the 
> > > current command is repeated to evict the session which should 
> > > guarantee there is an available slot.
> > > 
> > > On the force eviction case, we make sure that the victim session is 
> > > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > > for session slots is a FIFO one, ensuring that once we run out of
> > > sessions, everyone will get a session in a bounded time and once 
> > > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > > be subject to eviction.
> > > 
> > > Signed-off-by: James Bottomley <
> > > james.bottom...@hansenpartnership.com>
> > 
> > This is not a proper review yet. Just quick question: why do you need
> > a real time (i.e. created)? Maybe in the force eviction case it would
> > be enough to sleep lets say 500 ms and pick the victim with smallest
> > number? I.e. just have increasing u64 counter instead of real time.
> 
> So that if the oldest session has already been around for > 2s there's
> no need to wait.  In order to guarantee everyone gets a session for at
> least 2s without tracking the age of sessions, you'd have to sleep for
> 2s after you find the oldest session.
> 
> > That would simplify this patch a lot and does not prevent to refine
> > later on if workloads show need for more complex logic.
> 
> An increasing monotonic counter would actually not be much simpler: all
> you could really cut out would be the four lines and two comments at
> the bottom of the for loop in tpm2_session_wait() which check the age
> of the found session.
> 
> James

Right. Thanks for explaining this. I'll check the code with more detail
ASAP.

/Jarkko

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:45:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > > In a TPM2, sessions can be globally exhausted once there are
> > > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > > saved). The Strategy for handling this is to keep a global count of 
> > > all the sessions along with their creation time.  Then if we see 
> > > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > > first wait for one to become free, but if it doesn't, we forcibly 
> > > evict an existing one. The eviction strategy waits until the 
> > > current command is repeated to evict the session which should 
> > > guarantee there is an available slot.
> > > 
> > > On the force eviction case, we make sure that the victim session is 
> > > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > > for session slots is a FIFO one, ensuring that once we run out of
> > > sessions, everyone will get a session in a bounded time and once 
> > > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > > be subject to eviction.
> > > 
> > > Signed-off-by: James Bottomley <
> > > james.bottom...@hansenpartnership.com>
> > 
> > This is not a proper review yet. Just quick question: why do you need
> > a real time (i.e. created)? Maybe in the force eviction case it would
> > be enough to sleep lets say 500 ms and pick the victim with smallest
> > number? I.e. just have increasing u64 counter instead of real time.
> 
> So that if the oldest session has already been around for > 2s there's
> no need to wait.  In order to guarantee everyone gets a session for at
> least 2s without tracking the age of sessions, you'd have to sleep for
> 2s after you find the oldest session.
> 
> > That would simplify this patch a lot and does not prevent to refine
> > later on if workloads show need for more complex logic.
> 
> An increasing monotonic counter would actually not be much simpler: all
> you could really cut out would be the four lines and two comments at
> the bottom of the for loop in tpm2_session_wait() which check the age
> of the found session.
> 
> James

Right. Thanks for explaining this. I'll check the code with more detail
ASAP.

/Jarkko

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > > 
> > > > I.e. if there's any polling component then it would be reasonable to 
> > > > add an error 
> > > > component: poll the status and if it goes 'disconnected' then disable 
> > > > early-printk 
> > > > altogether in this case and trigger an emergency printk() so that 
> > > > there's chance 
> > > > that the user notices [if the system does not misbehave otherwise].
> > > 
> > > That'll be fun when printk() == early_printk() :-)
> > 
> > My suggestion would be to just print into the printk buffer directly in 
> > this case, 
> > without console output - the developer will notice it in 'dmesg'.
> 
> When you map printk() onto early_printk() dmesg will be empty, there
> will be nothing there, and therefore no reason what so ever to look
> there.

Unless you want a third layer of a console driver putting the debug message 
into 
dmesg isn't all that bad of a solution.

Let's admit it: something like USB that involves external pieces of hardware 
_does_ have failure modes, and troubleshooting messages instead of indefinite 
hangs are obviously more robust.

> I certainly don't ever look there.

You'll have to teach yourself that if the box boots up fine but there are no 
messages whatsoever from the early-printk console that you'll need to look at 
dmesg output or the syslog for more clues.

This should not be a common occurrance in any case - but when it happens it's 
very 
useful to have diagnostic messages. I don't think this is a controversial point 
in 
any fashion.

> Note that the printk buffer itself is a major part of why printk sucks donkey 
> balls.  Not to mention that you really cannot have an early_printk() 
> implementation that depends on printk().

There are several easy solutions to do that, my favorite would be to put it 
into 
the printk buffer totally unlocked. When your early-printk is active it's 
unused 
and in the end it's a known data structure after all:

/*
 * Just zap whatever's in the printk buffer and put your emergency message into 
 * it, prominently. No locking, no worries - don't generate emergency messages
 * while printk is active and syslogd is running - this facility is a poor 
man's 
 * fallback printk() when early-printk has taken over all kernel logging:
 */
void printk_emergency_puts(const char *str)
{
struct printk_log *msg, *msg_end;

msg = log_buf;

memset(msg, 0, sizeof(*msg));   
msg.text_len = strlen(str);

msg_end = (void *)msg + sizeof(*msg) + msg->text_len;

/* Zero ->len denotes end of log buffer: */
memset(msg_end, 0, sizeof(*msg_end));   

snprintf(ptr, str);
}

...
printk_emergency_puts"earlyprintk emergency: Hardware timed out, 
shutting down. Fix your debug cable?\n");
...

(Or so - totally untested, some details might be wrong.)

But yes, I agree with your wider point, I just looked at kernel/printk/printk.c 
and puked. Why did we merge that crappy piece of binary logging code, when we 
already have two other binary logging facilities in the kernel already, both of 
them better and cleaner than this?? Why did we mess up our nicely readable, 
simple, reliable ASCII log buffer printk code? :-(

> > > I myself wouldn't mind the system getting stuck until the link is 
> > > re-established. My own damn fault for taking that cable out etc.
> > 
> > That's fine too, although beyond the obvious "yanked the cable without 
> > realizing it" case there are corner cases where usability is increased 
> > massively if the kernel is more proactive about error conditions: for 
> > example 
> > there are sub-standard USB cables and there are too long USB pathways from 
> > overloaded USB hubs which can result in intermittent behavior, etc.
> > 
> > A clear diagnostic message in 'dmesg' that the USB host controller is 
> > unhappy 
> > about the USB-debug dongle device is a _lot_ more useful when 
> > troubleshooting 
> > such problems than the occasional weird, non-deterministic hang...
> 
> Sure, I'm just not sure what or where makes sense.
> 
> If your serial cable is bad you notice because you don't receive the right 
> amount of characters and or stuff gets mangled. You chuck the cable and get a 
> new one.
> 
> I think the most important part is re-establishing the link when the cable 
> gets 
> re-inserted. Maybe we should just drop all characters written when there's no 
> link and leave it at that, same as serial.

That would be fine with me too - but even in this case there should be a stat 
counter somewhere (in /proc or /debug) that counts the number of characters 
dropped. Maybe that file could also display an emergency string - avoiding the 
interaction with the printk buffer.

We can do better than passive-aggressive logging behavior...

Thanks,

Ingo

Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Ingo Molnar]

* Peter Zijlstra  wrote:

> On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > > 
> > > > I.e. if there's any polling component then it would be reasonable to 
> > > > add an error 
> > > > component: poll the status and if it goes 'disconnected' then disable 
> > > > early-printk 
> > > > altogether in this case and trigger an emergency printk() so that 
> > > > there's chance 
> > > > that the user notices [if the system does not misbehave otherwise].
> > > 
> > > That'll be fun when printk() == early_printk() :-)
> > 
> > My suggestion would be to just print into the printk buffer directly in 
> > this case, 
> > without console output - the developer will notice it in 'dmesg'.
> 
> When you map printk() onto early_printk() dmesg will be empty, there
> will be nothing there, and therefore no reason what so ever to look
> there.

Unless you want a third layer of a console driver putting the debug message 
into 
dmesg isn't all that bad of a solution.

Let's admit it: something like USB that involves external pieces of hardware 
_does_ have failure modes, and troubleshooting messages instead of indefinite 
hangs are obviously more robust.

> I certainly don't ever look there.

You'll have to teach yourself that if the box boots up fine but there are no 
messages whatsoever from the early-printk console that you'll need to look at 
dmesg output or the syslog for more clues.

This should not be a common occurrance in any case - but when it happens it's 
very 
useful to have diagnostic messages. I don't think this is a controversial point 
in 
any fashion.

> Note that the printk buffer itself is a major part of why printk sucks donkey 
> balls.  Not to mention that you really cannot have an early_printk() 
> implementation that depends on printk().

There are several easy solutions to do that, my favorite would be to put it 
into 
the printk buffer totally unlocked. When your early-printk is active it's 
unused 
and in the end it's a known data structure after all:

/*
 * Just zap whatever's in the printk buffer and put your emergency message into 
 * it, prominently. No locking, no worries - don't generate emergency messages
 * while printk is active and syslogd is running - this facility is a poor 
man's 
 * fallback printk() when early-printk has taken over all kernel logging:
 */
void printk_emergency_puts(const char *str)
{
struct printk_log *msg, *msg_end;

msg = log_buf;

memset(msg, 0, sizeof(*msg));   
msg.text_len = strlen(str);

msg_end = (void *)msg + sizeof(*msg) + msg->text_len;

/* Zero ->len denotes end of log buffer: */
memset(msg_end, 0, sizeof(*msg_end));   

snprintf(ptr, str);
}

...
printk_emergency_puts"earlyprintk emergency: Hardware timed out, 
shutting down. Fix your debug cable?\n");
...

(Or so - totally untested, some details might be wrong.)

But yes, I agree with your wider point, I just looked at kernel/printk/printk.c 
and puked. Why did we merge that crappy piece of binary logging code, when we 
already have two other binary logging facilities in the kernel already, both of 
them better and cleaner than this?? Why did we mess up our nicely readable, 
simple, reliable ASCII log buffer printk code? :-(

> > > I myself wouldn't mind the system getting stuck until the link is 
> > > re-established. My own damn fault for taking that cable out etc.
> > 
> > That's fine too, although beyond the obvious "yanked the cable without 
> > realizing it" case there are corner cases where usability is increased 
> > massively if the kernel is more proactive about error conditions: for 
> > example 
> > there are sub-standard USB cables and there are too long USB pathways from 
> > overloaded USB hubs which can result in intermittent behavior, etc.
> > 
> > A clear diagnostic message in 'dmesg' that the USB host controller is 
> > unhappy 
> > about the USB-debug dongle device is a _lot_ more useful when 
> > troubleshooting 
> > such problems than the occasional weird, non-deterministic hang...
> 
> Sure, I'm just not sure what or where makes sense.
> 
> If your serial cable is bad you notice because you don't receive the right 
> amount of characters and or stuff gets mangled. You chuck the cable and get a 
> new one.
> 
> I think the most important part is re-establishing the link when the cable 
> gets 
> re-inserted. Maybe we should just drop all characters written when there's no 
> link and leave it at that, same as serial.

That would be fine with me too - but even in this case there should be a stat 
counter somewhere (in /proc or /debug) that counts the number of characters 
dropped. Maybe that file could also display an emergency string - avoiding the 
interaction with the printk buffer.

We can do better than passive-aggressive logging behavior...

Thanks,

Ingo

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 08:26:19AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c48255e..b77fc60 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >   u32 context_tbl[3];
> > >   u8 *context_buf;
> > > + u32 session_tbl[6];
> > > + u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  u8 *cmd);
> > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > *space,
> > > u32 cc, u8 *buf, size_t bufsiz);
> > > +void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space
> > > *space);
> > 
> > Why the extra parameter?
> 
> Because it was called from tpms-dev in release to flush all the
> sessions still active.  At that point the work space is gone, so we
> have to call it with the real space.  However, I realised that callsite
> didn't hold the tpm_mutex like it should and that we don't need to
> flush the contexts because they'll be guaranteed empty, so I added a
> new function tpm2_kill_space() that does this.
> 
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index 7fd2fc5..ba4310a 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > > *chip, u8 *buf,
> > >  
> > >   rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> > > TPM_TRANSMIT_UNLOCKED, NULL);
> > > +
> > 
> > cruft
> 
> Removed.
> 
> [...] 
> > > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  
> > >   memcpy(>work_space.context_tbl, >context_tbl,
> > >  sizeof(space->context_tbl));
> > > + memcpy(>work_space.session_tbl, >session_tbl,
> > > +sizeof(space->session_tbl));
> > >   memcpy(chip->work_space.context_buf, space->context_buf,
> > > PAGE_SIZE);
> > > + memcpy(chip->work_space.session_buf, space->session_buf,
> > > PAGE_SIZE);
> > 
> > For transient objects the rollback is straight forward and totally
> > predictable. Given that with sessions you always keep some 
> > information in the TPM the rollback would be a bit more complicated.
> 
> There is basically no rollback unless you really want to understand
> what the commands did.  If we get a fault on the context save or load
> that isn't one we understand, like TPM_RC_HANDLE meaning the session
> won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
> then I think the only option is flushing everything.
> 
> I'd like us to agree on a hard failure model: if we get some
> unexplained error during our context loads or saves, we should clear
> out the entire space (which would allow us to use pointers instead of
> copyring) but we don't.  So we follow the soft failure.  However,
> sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
> get the equivalent of soft failure for transients and hard failure for
> sessions.
> 
> > Now your code seems to just keep the previous session_buf, doesn't
> > it? Does that always work or not?
> 
> Yes, after tpm_flush_space, the session memory is gone and all the
> sessions will refuse to load with RC_HANDLE, so we end up effectively
> clearing them out.  It seemed better to do it this way than to try to
> special case all the session stuff.

Maybe it would make sense to have a comment in code to state this?
Otherwise, I'm fine with this semantics.

> > PS. I have a high-level idea of attack vectors that are prevented by
> > having meta-data for session inside the TPM but can you point me to
> > the correct place in the TPM 2.0 specification that discusses about
> > this?
> 
> The problem is replay.  If I'm snooping your TPM commands and I capture
> your session context, if we don't have replay detection, I can re-load
> your session HMAC and replay your command because the session has the
> authorization or the encryption.  The TPM designers thought the only
> way to avoid replay was to use a counter which was part of the session
> context which increments every time a session is saved.  On loading you
> check that the counters match and fail if they don't.  The only way to
> implement this is to keep a memory of the counter in the TPM, hence the
> annoying vestigial sessions.
> 
> It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
> Handles (MSO=02_16 and 03_16 )"
> 
> James

Thank you.

Once these are in shape I think we have something that could be put into
a release, don't you think?

/Jarkko

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 08:26:19AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c48255e..b77fc60 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >   u32 context_tbl[3];
> > >   u8 *context_buf;
> > > + u32 session_tbl[6];
> > > + u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  u8 *cmd);
> > >  int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space
> > > *space,
> > > u32 cc, u8 *buf, size_t bufsiz);
> > > +void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space
> > > *space);
> > 
> > Why the extra parameter?
> 
> Because it was called from tpms-dev in release to flush all the
> sessions still active.  At that point the work space is gone, so we
> have to call it with the real space.  However, I realised that callsite
> didn't hold the tpm_mutex like it should and that we don't need to
> flush the contexts because they'll be guaranteed empty, so I added a
> new function tpm2_kill_space() that does this.
> 
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index 7fd2fc5..ba4310a 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > > *chip, u8 *buf,
> > >  
> > >   rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> > > TPM_TRANSMIT_UNLOCKED, NULL);
> > > +
> > 
> > cruft
> 
> Removed.
> 
> [...] 
> > > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  
> > >   memcpy(>work_space.context_tbl, >context_tbl,
> > >  sizeof(space->context_tbl));
> > > + memcpy(>work_space.session_tbl, >session_tbl,
> > > +sizeof(space->session_tbl));
> > >   memcpy(chip->work_space.context_buf, space->context_buf,
> > > PAGE_SIZE);
> > > + memcpy(chip->work_space.session_buf, space->session_buf,
> > > PAGE_SIZE);
> > 
> > For transient objects the rollback is straight forward and totally
> > predictable. Given that with sessions you always keep some 
> > information in the TPM the rollback would be a bit more complicated.
> 
> There is basically no rollback unless you really want to understand
> what the commands did.  If we get a fault on the context save or load
> that isn't one we understand, like TPM_RC_HANDLE meaning the session
> won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
> then I think the only option is flushing everything.
> 
> I'd like us to agree on a hard failure model: if we get some
> unexplained error during our context loads or saves, we should clear
> out the entire space (which would allow us to use pointers instead of
> copyring) but we don't.  So we follow the soft failure.  However,
> sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
> get the equivalent of soft failure for transients and hard failure for
> sessions.
> 
> > Now your code seems to just keep the previous session_buf, doesn't
> > it? Does that always work or not?
> 
> Yes, after tpm_flush_space, the session memory is gone and all the
> sessions will refuse to load with RC_HANDLE, so we end up effectively
> clearing them out.  It seemed better to do it this way than to try to
> special case all the session stuff.

Maybe it would make sense to have a comment in code to state this?
Otherwise, I'm fine with this semantics.

> > PS. I have a high-level idea of attack vectors that are prevented by
> > having meta-data for session inside the TPM but can you point me to
> > the correct place in the TPM 2.0 specification that discusses about
> > this?
> 
> The problem is replay.  If I'm snooping your TPM commands and I capture
> your session context, if we don't have replay detection, I can re-load
> your session HMAC and replay your command because the session has the
> authorization or the encryption.  The TPM designers thought the only
> way to avoid replay was to use a counter which was part of the session
> context which increments every time a session is saved.  On loading you
> check that the counters match and fail if they don't.  The only way to
> implement this is to keep a memory of the counter in the TPM, hence the
> annoying vestigial sessions.
> 
> It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
> Handles (MSO=02_16 and 03_16 )"
> 
> James

Thank you.

Once these are in shape I think we have something that could be put into
a release, don't you think?

/Jarkko

Re: [PATCH] tpm: fix RC value check in tpm2_seal_trusted

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 11:32:52AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 01:27:14PM +0200, Jarkko Sakkinen wrote:
> 
> > "The error code handling is bogus as any error code that has the bits
> > set that TPM_RC_HASH could pass. Implemented tpm2_rc_value() helper to
> > parse the error value from FMT0 and FMT1 error codes to use to check the
> > error so that these types of mistakes is prevented in the future."
> 
> Great thanks
> 
> Jason

Can I put your Reviewed-by? I would like to get this into 4.11.

/Jarkko

Re: [PATCH] tpm: fix RC value check in tpm2_seal_trusted

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 11:32:52AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 01:27:14PM +0200, Jarkko Sakkinen wrote:
> 
> > "The error code handling is bogus as any error code that has the bits
> > set that TPM_RC_HASH could pass. Implemented tpm2_rc_value() helper to
> > parse the error value from FMT0 and FMT1 error codes to use to check the
> > error so that these types of mistakes is prevented in the future."
> 
> Great thanks
> 
> Jason

Can I put your Reviewed-by? I would like to get this into 4.11.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager via a device link /dev/tpms

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:29:02PM -0800, James Bottomley wrote:
> On Wed, 2017-01-25 at 15:40 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 02:16:37PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-23 at 23:42 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 23, 2017 at 06:58:23PM +0200, Jarkko Sakkinen wrote:
> > > > > On Mon, Jan 23, 2017 at 04:09:42PM +0200, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley
> > > > > > wrote:
> > > > > > > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > > > > > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen
> > > > > > > > wrote:
> > > > > > > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko
> > > > > > > > > Sakkinen
> > > > > > > > > wrote:
> > > > > > > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James
> > > > > > > > > > Bottomley
> > > > > > > > > > wrote:
> > > > > > > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko
> > > > > > > > > > > > Sakkinen
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > 'tabrm4' branch has been now rebased. It's now
> > > > > > > > > > > > > on
> > > > > > > > > > > > > top of
> > > > > > > > > > > > > master
> > > > > > > > > > > > > branch that contains Stefan's latest patch (min
> > > > > > > > > > > > > body length
> > > > > > > > > > > > > check) 
> > > > > > > > > > > > > that I've reviewed and tested. It also contains
> > > > > > > > > > > > > your
> > > > > > > > > > > > > updated
> > > > > > > > > > > > > /dev/tpms patch.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I guess the 5 commits that are there now are
> > > > > > > > > > > > > such
> > > > > > > > > > > > > that we
> > > > > > > > > > > > > have 
> > > > > > > > > > > > > fairly good consensus, don't we? If so, can I
> > > > > > > > > > > > > add
> > > > > > > > > > > > > your
> > > > > > > > > > > > > reviewed-by 
> > > > > > > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > > > > > > 
> > > > > > > > > > > > We're still failing my test_transients.  This is
> > > > > > > > > > > > the
> > > > > > > > > > > > full
> > > > > > > > > > > > python of 
> > > > > > > > > > > > the test case:
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > def test_transients(self):
> > > > > > > > > > > > k = self.open_transients()
> > > > > > > > > > > > self.c.flush_context(k[0])
> > > > > > > > > > > > self.c.change_auth(self.c.SRK, k[1],
> > > > > > > > > > > > None,
> > > > > > > > > > > > pwd1)
> > > > > > > > > > > > ...
> > > > > > > > > > > > 
> > > > > > > > > > > > It's failing at self.c.flush_context(k[0]) with
> > > > > > > > > > > > TPM_RC_VALUE.
> > > > > > > > > > > >   It's 
> > > > > > > > > > > > the same problem Ken complained about:
> > > > > > > > > > > > TPM2_FlushContext
> > > > > > > > > > > > doesn't have 
> > > > > > > > > > > > a declared handle area so we don't translate the
> > > > > > > > > > > > handle being
> > > > > > > > > > > > sent
> > > > > > > > > > > > down.  We have to fix this either by intercepting
> > > > > > > > > > > > the
> > > > > > > > > > > > flush
> > > > > > > > > > > > and 
> > > > > > > > > > > > manually translating the context, or by being
> > > > > > > > > > > > dangerously
> > > > > > > > > > > > clever and 
> > > > > > > > > > > > marking flush as a command which takes one
> > > > > > > > > > > > handle.
> > > > > > > > > > > 
> > > > > > > > > > > This is what the dangerously clever fix looks like.
> > > > > > > > > > >  With this
> > > > > > > > > > > and a
> > > > > > > > > > > few other changes, my smoke tests now pass.
> > > > > > > > > > > 
> > > > > > > > > > > James
> > > > > > > > > > 
> > > > > > > > > > I don't want to be clever here. I will rather
> > > > > > > > > > intercept
> > > > > > > > > > the body
> > > > > > > > > > and
> > > > > > > > > > try to keep the core code simple and easy to
> > > > > > > > > > understand.
> > > > > > > > > 
> > > > > > > > > It came out quite clean actually.
> > > > > > > > > 
> > > > > > > > > I just encapsulated handle mapping and have this in the
> > > > > > > > > beginning
> > > > > > > > > of
> > > > > > > > > tpm2_map_command:
> > > > > > > > > 
> > > > > > > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > > > > > >   return tpm2_map_to_phandle(space,
> > > > > > > > > [TPM_HEADER_SIZE]);
> > > > > > > > > 
> > > > > > > > > I think this documents better what is actually going on
> > > > > > > > > than
> > > > > > > > > tinkering
> > > > > > > > > cc_attr_tbl.
> > > > > > > > > 
> > > > > > > > > /Jarkko
> > > > > > > > 
> > > > > > > > Actually what you suggested is much better idea because
> > > > > > > > it
> > > > > > > > will also
> > > > > > > > take care of validation.
> > > > > > > 
> > > > > > > Yes, that's why it's clever ... I'm just always wary of
> > > > > 

Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager via a device link /dev/tpms

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:29:02PM -0800, James Bottomley wrote:
> On Wed, 2017-01-25 at 15:40 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 02:16:37PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-23 at 23:42 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 23, 2017 at 06:58:23PM +0200, Jarkko Sakkinen wrote:
> > > > > On Mon, Jan 23, 2017 at 04:09:42PM +0200, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley
> > > > > > wrote:
> > > > > > > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > > > > > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen
> > > > > > > > wrote:
> > > > > > > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko
> > > > > > > > > Sakkinen
> > > > > > > > > wrote:
> > > > > > > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James
> > > > > > > > > > Bottomley
> > > > > > > > > > wrote:
> > > > > > > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko
> > > > > > > > > > > > Sakkinen
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > 'tabrm4' branch has been now rebased. It's now
> > > > > > > > > > > > > on
> > > > > > > > > > > > > top of
> > > > > > > > > > > > > master
> > > > > > > > > > > > > branch that contains Stefan's latest patch (min
> > > > > > > > > > > > > body length
> > > > > > > > > > > > > check) 
> > > > > > > > > > > > > that I've reviewed and tested. It also contains
> > > > > > > > > > > > > your
> > > > > > > > > > > > > updated
> > > > > > > > > > > > > /dev/tpms patch.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I guess the 5 commits that are there now are
> > > > > > > > > > > > > such
> > > > > > > > > > > > > that we
> > > > > > > > > > > > > have 
> > > > > > > > > > > > > fairly good consensus, don't we? If so, can I
> > > > > > > > > > > > > add
> > > > > > > > > > > > > your
> > > > > > > > > > > > > reviewed-by 
> > > > > > > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > > > > > > 
> > > > > > > > > > > > We're still failing my test_transients.  This is
> > > > > > > > > > > > the
> > > > > > > > > > > > full
> > > > > > > > > > > > python of 
> > > > > > > > > > > > the test case:
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > def test_transients(self):
> > > > > > > > > > > > k = self.open_transients()
> > > > > > > > > > > > self.c.flush_context(k[0])
> > > > > > > > > > > > self.c.change_auth(self.c.SRK, k[1],
> > > > > > > > > > > > None,
> > > > > > > > > > > > pwd1)
> > > > > > > > > > > > ...
> > > > > > > > > > > > 
> > > > > > > > > > > > It's failing at self.c.flush_context(k[0]) with
> > > > > > > > > > > > TPM_RC_VALUE.
> > > > > > > > > > > >   It's 
> > > > > > > > > > > > the same problem Ken complained about:
> > > > > > > > > > > > TPM2_FlushContext
> > > > > > > > > > > > doesn't have 
> > > > > > > > > > > > a declared handle area so we don't translate the
> > > > > > > > > > > > handle being
> > > > > > > > > > > > sent
> > > > > > > > > > > > down.  We have to fix this either by intercepting
> > > > > > > > > > > > the
> > > > > > > > > > > > flush
> > > > > > > > > > > > and 
> > > > > > > > > > > > manually translating the context, or by being
> > > > > > > > > > > > dangerously
> > > > > > > > > > > > clever and 
> > > > > > > > > > > > marking flush as a command which takes one
> > > > > > > > > > > > handle.
> > > > > > > > > > > 
> > > > > > > > > > > This is what the dangerously clever fix looks like.
> > > > > > > > > > >  With this
> > > > > > > > > > > and a
> > > > > > > > > > > few other changes, my smoke tests now pass.
> > > > > > > > > > > 
> > > > > > > > > > > James
> > > > > > > > > > 
> > > > > > > > > > I don't want to be clever here. I will rather
> > > > > > > > > > intercept
> > > > > > > > > > the body
> > > > > > > > > > and
> > > > > > > > > > try to keep the core code simple and easy to
> > > > > > > > > > understand.
> > > > > > > > > 
> > > > > > > > > It came out quite clean actually.
> > > > > > > > > 
> > > > > > > > > I just encapsulated handle mapping and have this in the
> > > > > > > > > beginning
> > > > > > > > > of
> > > > > > > > > tpm2_map_command:
> > > > > > > > > 
> > > > > > > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > > > > > >   return tpm2_map_to_phandle(space,
> > > > > > > > > [TPM_HEADER_SIZE]);
> > > > > > > > > 
> > > > > > > > > I think this documents better what is actually going on
> > > > > > > > > than
> > > > > > > > > tinkering
> > > > > > > > > cc_attr_tbl.
> > > > > > > > > 
> > > > > > > > > /Jarkko
> > > > > > > > 
> > > > > > > > Actually what you suggested is much better idea because
> > > > > > > > it
> > > > > > > > will also
> > > > > > > > take care of validation.
> > > > > > > 
> > > > > > > Yes, that's why it's clever ... I'm just always wary of
> > > > > 

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 11:05:06AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 01:14:03PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 25, 2017 at 03:11:36PM -0700, Jason Gunthorpe wrote:
> > > On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > There should be anyway someway to limit what commands can be sent but
> > > > I understand your point.
> > > 
> > > What is the filter for?
> > > 
> > > James and I talked about a filter to create a safer cdev for use by
> > > users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
> > > access' path.
> > 
> > What do you mean by "safer cdev"?
> 
> 'safer cdev' is this concept of limiting privileges you are describing
> below.
> 
> > > I also suggested a filter in the kernel to ensure that the RM is only
> > > passing commands it actually knows it handles properly. eg you would
> > > filter out list handles. That is hardwired into the kernel, and does
> > > not ge to be configured by user space.
> >
> > In many cases you would want to limit the set of operations that client
> > can use.  For example, not every client needs NV operations. In general
> > you might want to have mechanism for limiting privileges. I haven't
> > really considered this from the perspective that you've been discussing
> > but more from the "principle of least privilege" perspective.
> 
> What does that mean? The kernel needs to provide an unrestricted
> access path to the TPM and the RM - typically for use by root. I don't
> think there is any debate on this point.
> 
> The kernel *could* provide restricted access to the TPM and the RM -
> typically for use by a user.
> 
> These are *different* things and they should not both exist at once on
> /dev/tpms0 (that is not the unix model).
> 
> IMHO this patch series should focus entirely on the unrestricted
> access path. Otherwise the debate is too large and complex.

Agreed. We can add more granular access control later on.

For the rest of the response I understand your point of view but lets
continue after we have basic building blocks in place :-)

/Jarkko

Re: [PATCH RFC] tpm: define a command filter

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 11:05:06AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 26, 2017 at 01:14:03PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 25, 2017 at 03:11:36PM -0700, Jason Gunthorpe wrote:
> > > On Wed, Jan 25, 2017 at 10:21:37PM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > There should be anyway someway to limit what commands can be sent but
> > > > I understand your point.
> > > 
> > > What is the filter for?
> > > 
> > > James and I talked about a filter to create a safer cdev for use by
> > > users. However tpms0 cannot be that 'safer' cdev - it is now the 'all
> > > access' path.
> > 
> > What do you mean by "safer cdev"?
> 
> 'safer cdev' is this concept of limiting privileges you are describing
> below.
> 
> > > I also suggested a filter in the kernel to ensure that the RM is only
> > > passing commands it actually knows it handles properly. eg you would
> > > filter out list handles. That is hardwired into the kernel, and does
> > > not ge to be configured by user space.
> >
> > In many cases you would want to limit the set of operations that client
> > can use.  For example, not every client needs NV operations. In general
> > you might want to have mechanism for limiting privileges. I haven't
> > really considered this from the perspective that you've been discussing
> > but more from the "principle of least privilege" perspective.
> 
> What does that mean? The kernel needs to provide an unrestricted
> access path to the TPM and the RM - typically for use by root. I don't
> think there is any debate on this point.
> 
> The kernel *could* provide restricted access to the TPM and the RM -
> typically for use by a user.
> 
> These are *different* things and they should not both exist at once on
> /dev/tpms0 (that is not the unix model).
> 
> IMHO this patch series should focus entirely on the unrestricted
> access path. Otherwise the debate is too large and complex.

Agreed. We can add more granular access control later on.

For the rest of the response I understand your point of view but lets
continue after we have basic building blocks in place :-)

/Jarkko

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:18:49AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > > sessions are different from transient objects in that their handles
> > > may not be virtualized (because they're used for some hmac
> > > calculations).  Additionally when a session is context saved, a
> > > vestigial memory remains in the TPM and if it is also flushed, that
> > > will be lost and the session context will refuse to load next time, 
> > > so the code is updated to flush only transient objects after a 
> > > context save.  Add a separate array (chip->session_tbl) to save and 
> > > restore sessions by handle.  Use the failure of a context save or 
> > > load to signal that the session has been flushed from the TPM and 
> > > we can remove its memory from chip->session_tbl.
> > > 
> > > Sessions are also isolated during each instance of a tpm space. 
> > >  This means that spaces shouldn't be able to see each other's 
> > > sessions and is enforced by ensuring that a space user may only 
> > > refer to sessions handles that are present in their own chip
> > > ->session_tbl.  Finally when a space is closed, all the sessions 
> > > belonging to it should be flushed so the handles may be re-used by
> > > other spaces.
> > > 
> > > Signed-off-by: James Bottomley <
> > > james.bottom...@hansenpartnership.com>
> > 
> > I'm wondering if you ever need more than two sessions at once? If we
> > would limit the number of sessions to that you could probably 
> > simplify a lot.
> 
> Three seems to be the agreed maximum: hmac authority, parameter
> encryption and command audit.
> 
> I'll fix up the rest
> 
> James

Right. I've also set the limit for trasient objects to three.

/Jarkko

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:18:49AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > > sessions are different from transient objects in that their handles
> > > may not be virtualized (because they're used for some hmac
> > > calculations).  Additionally when a session is context saved, a
> > > vestigial memory remains in the TPM and if it is also flushed, that
> > > will be lost and the session context will refuse to load next time, 
> > > so the code is updated to flush only transient objects after a 
> > > context save.  Add a separate array (chip->session_tbl) to save and 
> > > restore sessions by handle.  Use the failure of a context save or 
> > > load to signal that the session has been flushed from the TPM and 
> > > we can remove its memory from chip->session_tbl.
> > > 
> > > Sessions are also isolated during each instance of a tpm space. 
> > >  This means that spaces shouldn't be able to see each other's 
> > > sessions and is enforced by ensuring that a space user may only 
> > > refer to sessions handles that are present in their own chip
> > > ->session_tbl.  Finally when a space is closed, all the sessions 
> > > belonging to it should be flushed so the handles may be re-used by
> > > other spaces.
> > > 
> > > Signed-off-by: James Bottomley <
> > > james.bottom...@hansenpartnership.com>
> > 
> > I'm wondering if you ever need more than two sessions at once? If we
> > would limit the number of sessions to that you could probably 
> > simplify a lot.
> 
> Three seems to be the agreed maximum: hmac authority, parameter
> encryption and command audit.
> 
> I'll fix up the rest
> 
> James

Right. I've also set the limit for trasient objects to three.

/Jarkko

Re: fs, net: deadlock between bind/splice on af_unix

[Mateusz Guzik]

On Thu, Jan 26, 2017 at 09:11:07PM -0800, Cong Wang wrote:
> On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik  wrote:
> > On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
> >> On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov  wrote:
> >> > On Fri, Dec 9, 2016 at 7:41 AM, Al Viro  wrote:
> >> >> On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
> >> >>
> >> >>> > Why do we do autobind there, anyway, and why is it conditional on
> >> >>> > SOCK_PASSCRED?  Note that e.g. for SOCK_STREAM we can bloody well get
> >> >>> > to sending stuff without autobind ever done - just use socketpair()
> >> >>> > to create that sucker and we won't be going through the connect()
> >> >>> > at all.
> >> >>>
> >> >>> In the case Dmitry reported, unix_dgram_sendmsg() calls 
> >> >>> unix_autobind(),
> >> >>> not SOCK_STREAM.
> >> >>
> >> >> Yes, I've noticed.  What I'm asking is what in there needs autobind 
> >> >> triggered
> >> >> on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
> >> >>
> >> >>> I guess some lock, perhaps the u->bindlock could be dropped before
> >> >>> acquiring the next one (sb_writer), but I need to double check.
> >> >>
> >> >> Bad idea, IMO - do you *want* autobind being able to come through while
> >> >> bind(2) is busy with mknod?
> >> >
> >> >
> >> > Ping. This is still happening on HEAD.
> >> >
> >>
> >> Thanks for your reminder. Mind to give the attached patch (compile only)
> >> a try? I take another approach to fix this deadlock, which moves the
> >> unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
> >> impact with this way.
> >>
> >
> > I don't think this is the right approach.
> >
> > Currently the file creation is potponed until unix_bind can no longer
> > fail otherwise. With it reordered, it may be someone races you with a
> > different path and now you are left with a file to clean up. Except it
> > is quite unclear for me if you can unlink it.
> 
> What races do you mean here? If you mean someone could get a
> refcount of that file, it could happen no matter we have bindlock or not
> since it is visible once created. The filesystem layer should take care of
> the file refcount so all we need to do here is calling path_put() as in my
> patch. Or if you mean two threads calling unix_bind() could race without
> binlock, only one of them should succeed the other one just fails out.

Two threads can race and one fails with EINVAL.

With your patch there is a new file created and it is unclear what to
do with it - leaving it as it is sounds like the last resort and
unlinking it sounds extremely fishy as it opens you to games played by
the user.

Re: fs, net: deadlock between bind/splice on af_unix

[Mateusz Guzik]

On Thu, Jan 26, 2017 at 09:11:07PM -0800, Cong Wang wrote:
> On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik  wrote:
> > On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
> >> On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov  wrote:
> >> > On Fri, Dec 9, 2016 at 7:41 AM, Al Viro  wrote:
> >> >> On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
> >> >>
> >> >>> > Why do we do autobind there, anyway, and why is it conditional on
> >> >>> > SOCK_PASSCRED?  Note that e.g. for SOCK_STREAM we can bloody well get
> >> >>> > to sending stuff without autobind ever done - just use socketpair()
> >> >>> > to create that sucker and we won't be going through the connect()
> >> >>> > at all.
> >> >>>
> >> >>> In the case Dmitry reported, unix_dgram_sendmsg() calls 
> >> >>> unix_autobind(),
> >> >>> not SOCK_STREAM.
> >> >>
> >> >> Yes, I've noticed.  What I'm asking is what in there needs autobind 
> >> >> triggered
> >> >> on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
> >> >>
> >> >>> I guess some lock, perhaps the u->bindlock could be dropped before
> >> >>> acquiring the next one (sb_writer), but I need to double check.
> >> >>
> >> >> Bad idea, IMO - do you *want* autobind being able to come through while
> >> >> bind(2) is busy with mknod?
> >> >
> >> >
> >> > Ping. This is still happening on HEAD.
> >> >
> >>
> >> Thanks for your reminder. Mind to give the attached patch (compile only)
> >> a try? I take another approach to fix this deadlock, which moves the
> >> unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
> >> impact with this way.
> >>
> >
> > I don't think this is the right approach.
> >
> > Currently the file creation is potponed until unix_bind can no longer
> > fail otherwise. With it reordered, it may be someone races you with a
> > different path and now you are left with a file to clean up. Except it
> > is quite unclear for me if you can unlink it.
> 
> What races do you mean here? If you mean someone could get a
> refcount of that file, it could happen no matter we have bindlock or not
> since it is visible once created. The filesystem layer should take care of
> the file refcount so all we need to do here is calling path_put() as in my
> patch. Or if you mean two threads calling unix_bind() could race without
> binlock, only one of them should succeed the other one just fails out.

Two threads can race and one fails with EINVAL.

With your patch there is a new file created and it is unclear what to
do with it - leaving it as it is sounds like the last resort and
unlinking it sounds extremely fishy as it opens you to games played by
the user.

[PATCH] spi: qup: Fix runtime and system PM callbacks.

[Pramod Gurav]

The SPI clocks were being turned on every suspend/resume cycle.
This was increamenting the prepare/enable count after every resume.
Fix the same.

Signed-off-by: Pramod Gurav 
---
Tested on db410c

 drivers/spi/spi-qup.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 1bfa889..a9731e8 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -956,8 +956,10 @@ static int spi_qup_pm_resume_runtime(struct device *device)
return ret;
 
ret = clk_prepare_enable(controller->cclk);
-   if (ret)
+   if (ret) {
+   clk_disable_unprepare(controller->iclk);
return ret;
+   }
 
/* Disable clocks auto gaiting */
config = readl_relaxed(controller->base + QUP_CONFIG);
@@ -983,8 +985,7 @@ static int spi_qup_suspend(struct device *device)
return ret;
 
if (!pm_runtime_suspended(device)) {
-   clk_disable_unprepare(controller->cclk);
-   clk_disable_unprepare(controller->iclk);
+   pm_runtime_put(device);
}
return 0;
 }
@@ -995,18 +996,17 @@ static int spi_qup_resume(struct device *device)
struct spi_qup *controller = spi_master_get_devdata(master);
int ret;
 
-   ret = clk_prepare_enable(controller->iclk);
-   if (ret)
-   return ret;
-
-   ret = clk_prepare_enable(controller->cclk);
-   if (ret)
+   ret = pm_runtime_get_sync(device);
+   if (ret < 0) {
+   dev_err(device, "pm runtime failed in resume\n");
return ret;
+   }
 
ret = spi_qup_set_state(controller, QUP_STATE_RESET);
if (ret)
return ret;
 
+   pm_runtime_put(device);
return spi_master_resume(master);
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.10.2

[PATCH] spi: qup: Fix runtime and system PM callbacks.

[Pramod Gurav]

The SPI clocks were being turned on every suspend/resume cycle.
This was increamenting the prepare/enable count after every resume.
Fix the same.

Signed-off-by: Pramod Gurav 
---
Tested on db410c

 drivers/spi/spi-qup.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 1bfa889..a9731e8 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -956,8 +956,10 @@ static int spi_qup_pm_resume_runtime(struct device *device)
return ret;
 
ret = clk_prepare_enable(controller->cclk);
-   if (ret)
+   if (ret) {
+   clk_disable_unprepare(controller->iclk);
return ret;
+   }
 
/* Disable clocks auto gaiting */
config = readl_relaxed(controller->base + QUP_CONFIG);
@@ -983,8 +985,7 @@ static int spi_qup_suspend(struct device *device)
return ret;
 
if (!pm_runtime_suspended(device)) {
-   clk_disable_unprepare(controller->cclk);
-   clk_disable_unprepare(controller->iclk);
+   pm_runtime_put(device);
}
return 0;
 }
@@ -995,18 +996,17 @@ static int spi_qup_resume(struct device *device)
struct spi_qup *controller = spi_master_get_devdata(master);
int ret;
 
-   ret = clk_prepare_enable(controller->iclk);
-   if (ret)
-   return ret;
-
-   ret = clk_prepare_enable(controller->cclk);
-   if (ret)
+   ret = pm_runtime_get_sync(device);
+   if (ret < 0) {
+   dev_err(device, "pm runtime failed in resume\n");
return ret;
+   }
 
ret = spi_qup_set_state(controller, QUP_STATE_RESET);
if (ret)
return ret;
 
+   pm_runtime_put(device);
return spi_master_resume(master);
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.10.2

Re: [tpmdd-devel] [PATCH v6 1/2] tpm: implement TPM 2.0 capability to get active PCR banks

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:23:15AM -0500, Stefan Berger wrote:
> On 01/20/2017 12:05 PM, Nayna Jain wrote:
> > This patch implements the TPM 2.0 capability TPM_CAP_PCRS to
> > retrieve the active PCR banks from the TPM. This is needed
> > to enable extending all active banks as recommended by TPM 2.0
> > TCG Specification.
> >
> > Signed-off-by: Nayna Jain 
> > Reviewed-by: Jarkko Sakkinen 
> > ---
> >   drivers/char/tpm/tpm.h  |  5 
> >   drivers/char/tpm/tpm2-cmd.c | 59 
> > +
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 1ae9768..c291f19 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -97,6 +97,7 @@ enum tpm2_return_codes {
> >   };
> >
> >   enum tpm2_algorithms {
> > +   TPM2_ALG_ERROR  = 0x,
> > TPM2_ALG_SHA1   = 0x0004,
> > TPM2_ALG_KEYEDHASH  = 0x0008,
> > TPM2_ALG_SHA256 = 0x000B,
> > @@ -127,6 +128,7 @@ enum tpm2_permanent_handles {
> >   };
> >
> >   enum tpm2_capabilities {
> > +   TPM2_CAP_PCRS   = 5,
> > TPM2_CAP_TPM_PROPERTIES = 6,
> >   };
> >
> > @@ -187,6 +189,8 @@ struct tpm_chip {
> >
> > const struct attribute_group *groups[3];
> > unsigned int groups_cnt;
> > +
> > +   u16 active_banks[7];
> >   #ifdef CONFIG_ACPI
> > acpi_handle acpi_dev_handle;
> > char ppi_version[TPM_PPI_VERSION_LEN + 1];
> > @@ -545,4 +549,5 @@ int tpm2_auto_startup(struct tpm_chip *chip);
> >   void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> >   unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 
> > ordinal);
> >   int tpm2_probe(struct tpm_chip *chip);
> > +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
> >   #endif
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 6eda239..0e000a3 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -998,3 +998,62 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> > rc = -ENODEV;
> > return rc;
> >   }
> > +
> > +struct tpm2_pcr_selection {
> > +   __be16  hash_alg;
> > +   u8  size_of_select;
> > +   u8  pcr_select[3];
> > +} __packed;
> > +
> > +/**
> > + * tpm2_get_pcr_allocation() - get TPM active PCR banks.
> > + *
> > + * @chip: TPM chip to use.
> > + *
> > + * Return: Same as with tpm_transmit_cmd.
> > + */
> > +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> > +{
> > +   struct tpm2_pcr_selection pcr_selection;
> > +   struct tpm_buf buf;
> > +   void *marker;
> > +   unsigned int count = 0;
> > +   int rc;
> > +   int i;
> > +
> > +   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> > +   if (rc)
> > +   return rc;
> > +
> > +   tpm_buf_append_u32(, TPM2_CAP_PCRS);
> > +   tpm_buf_append_u32(, 0);
> > +   tpm_buf_append_u32(, 1);
> > +
> > +   rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0,
> > + "get tpm pcr allocation");
> > +   if (rc < 0)
> > +   goto out;
> > +
> > +   count = be32_to_cpup(
> > +   (__be32 *)[TPM_HEADER_SIZE + 5]);
> > +
> > +   if (count > ARRAY_SIZE(chip->active_banks)) {
> > +   rc = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   marker = [TPM_HEADER_SIZE + 9];
> 
> Now that we are checking access to the returned buffer, we should do an 
> additional check here:
> 
> end = [TPM_HEADER_SIZE + rc];
> 
> 
> > +   for (i = 0; i < count; i++) {
> 
> if (marker + sizeof(pcr_selection) >= end) {
>rc = -EFAULT;
>goto out;
> }
> 
> 
> > +   memcpy(_selection, marker, sizeof(pcr_selection));
> > +   chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
> > +   marker = marker + sizeof(struct tpm2_pcr_selection);
> > +   }
> > +
> > +out:
> > +   if (count < ARRAY_SIZE(chip->active_banks))
> 
> if (rc < 0 || count < ARRAY_SIZE(...))
> 
> 
> I can send a separate patch for this. Let me know.
> 
> > +   chip->active_banks[count] = TPM2_ALG_ERROR;
> > +
> > +   tpm_buf_destroy();
> > +
> > +   return rc;
> > +}

Can you submit a fixup that I could squash into existing patch? Thanks.

/Jarkko

Re: [tpmdd-devel] [PATCH v6 1/2] tpm: implement TPM 2.0 capability to get active PCR banks

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:23:15AM -0500, Stefan Berger wrote:
> On 01/20/2017 12:05 PM, Nayna Jain wrote:
> > This patch implements the TPM 2.0 capability TPM_CAP_PCRS to
> > retrieve the active PCR banks from the TPM. This is needed
> > to enable extending all active banks as recommended by TPM 2.0
> > TCG Specification.
> >
> > Signed-off-by: Nayna Jain 
> > Reviewed-by: Jarkko Sakkinen 
> > ---
> >   drivers/char/tpm/tpm.h  |  5 
> >   drivers/char/tpm/tpm2-cmd.c | 59 
> > +
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 1ae9768..c291f19 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -97,6 +97,7 @@ enum tpm2_return_codes {
> >   };
> >
> >   enum tpm2_algorithms {
> > +   TPM2_ALG_ERROR  = 0x,
> > TPM2_ALG_SHA1   = 0x0004,
> > TPM2_ALG_KEYEDHASH  = 0x0008,
> > TPM2_ALG_SHA256 = 0x000B,
> > @@ -127,6 +128,7 @@ enum tpm2_permanent_handles {
> >   };
> >
> >   enum tpm2_capabilities {
> > +   TPM2_CAP_PCRS   = 5,
> > TPM2_CAP_TPM_PROPERTIES = 6,
> >   };
> >
> > @@ -187,6 +189,8 @@ struct tpm_chip {
> >
> > const struct attribute_group *groups[3];
> > unsigned int groups_cnt;
> > +
> > +   u16 active_banks[7];
> >   #ifdef CONFIG_ACPI
> > acpi_handle acpi_dev_handle;
> > char ppi_version[TPM_PPI_VERSION_LEN + 1];
> > @@ -545,4 +549,5 @@ int tpm2_auto_startup(struct tpm_chip *chip);
> >   void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> >   unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 
> > ordinal);
> >   int tpm2_probe(struct tpm_chip *chip);
> > +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
> >   #endif
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 6eda239..0e000a3 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -998,3 +998,62 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> > rc = -ENODEV;
> > return rc;
> >   }
> > +
> > +struct tpm2_pcr_selection {
> > +   __be16  hash_alg;
> > +   u8  size_of_select;
> > +   u8  pcr_select[3];
> > +} __packed;
> > +
> > +/**
> > + * tpm2_get_pcr_allocation() - get TPM active PCR banks.
> > + *
> > + * @chip: TPM chip to use.
> > + *
> > + * Return: Same as with tpm_transmit_cmd.
> > + */
> > +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> > +{
> > +   struct tpm2_pcr_selection pcr_selection;
> > +   struct tpm_buf buf;
> > +   void *marker;
> > +   unsigned int count = 0;
> > +   int rc;
> > +   int i;
> > +
> > +   rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> > +   if (rc)
> > +   return rc;
> > +
> > +   tpm_buf_append_u32(, TPM2_CAP_PCRS);
> > +   tpm_buf_append_u32(, 0);
> > +   tpm_buf_append_u32(, 1);
> > +
> > +   rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0,
> > + "get tpm pcr allocation");
> > +   if (rc < 0)
> > +   goto out;
> > +
> > +   count = be32_to_cpup(
> > +   (__be32 *)[TPM_HEADER_SIZE + 5]);
> > +
> > +   if (count > ARRAY_SIZE(chip->active_banks)) {
> > +   rc = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   marker = [TPM_HEADER_SIZE + 9];
> 
> Now that we are checking access to the returned buffer, we should do an 
> additional check here:
> 
> end = [TPM_HEADER_SIZE + rc];
> 
> 
> > +   for (i = 0; i < count; i++) {
> 
> if (marker + sizeof(pcr_selection) >= end) {
>rc = -EFAULT;
>goto out;
> }
> 
> 
> > +   memcpy(_selection, marker, sizeof(pcr_selection));
> > +   chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
> > +   marker = marker + sizeof(struct tpm2_pcr_selection);
> > +   }
> > +
> > +out:
> > +   if (count < ARRAY_SIZE(chip->active_banks))
> 
> if (rc < 0 || count < ARRAY_SIZE(...))
> 
> 
> I can send a separate patch for this. Let me know.
> 
> > +   chip->active_banks[count] = TPM2_ALG_ERROR;
> > +
> > +   tpm_buf_destroy();
> > +
> > +   return rc;
> > +}

Can you submit a fixup that I could squash into existing patch? Thanks.

/Jarkko

Re: [PATCH v4 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

[Vivek Gautam]

On 01/26/2017 11:45 PM, Bjorn Andersson wrote:

On Tue 24 Jan 01:19 PST 2017, Kishon Vijay Abraham I wrote:

On Monday 23 January 2017 03:43 PM, Vivek Gautam wrote:

On Wed, Jan 18, 2017 at 11:33 PM, Bjorn Andersson

[..]

Yes, that's correct. The QMP and QUSB2 phy init sequences are a bunch
of static values for a particular IP version. These values hardly give a
meaningful data to put few phy bindings that could be referenced
to configure the phy further.

Not really. You can have clearly defined phy binding to give meaningful data.
Every driver doing the same configuration bloats the driver and these
configuration values are just magic values which hardly can be reviewed by 
anyone.

Further more moving this blob to devicetree will not allow us to treat
the various QMP configurations as one HW block, as there are other
differences as well.

Like many other drivers it's possible to create a generic version that
has every bit of logic driven by configuration from devicetree, but like
most of those cases this is not the way we split things.

And this has the side effect of keeping the dts files human readable,
human understandable and human maintainable.

right. That's why I recommend having clearly defined bindings.
phy,tx- = 
phy,tx- = 
phy,tx- = 

There's no doubt that this table needs to be encoded somewhere, so the
question is should we hard code this in a C file or in a DTSI file.

Skimming through [1] I see examples of things that differs based on how
the specific component is integrated in a SoC or on a particular board -
properties that are relevant to a "system integrator".

As far as I can tell this blob will, if ever, only be changed by a
driver developer and as such it's not carry information about how this
component relates to the rest of the system and should as such not be
part of the device tree.


If there are properties of the hardware that is affected by how the
component is integrated in the system I really would like for those to
be exposed as human-readable properties that I can understand and alter
without deep knowledge about the register map of the hardware block.


I am reaching out to our internal teams to get more information
on different possible phy configurations, based on which the registers
values are decided.
This is something that i tried to understand in the past as well, but 
couldn't

grab much information that time.
Will come back with relevant information on this.


Regards
Vivek


[1] -> https://www.linuxplumbersconf.org/2016/ocw/proposals/4047

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

[Vivek Gautam]

On 01/26/2017 11:45 PM, Bjorn Andersson wrote:

On Tue 24 Jan 01:19 PST 2017, Kishon Vijay Abraham I wrote:

On Monday 23 January 2017 03:43 PM, Vivek Gautam wrote:

On Wed, Jan 18, 2017 at 11:33 PM, Bjorn Andersson

[..]

Yes, that's correct. The QMP and QUSB2 phy init sequences are a bunch
of static values for a particular IP version. These values hardly give a
meaningful data to put few phy bindings that could be referenced
to configure the phy further.

Not really. You can have clearly defined phy binding to give meaningful data.
Every driver doing the same configuration bloats the driver and these
configuration values are just magic values which hardly can be reviewed by 
anyone.

Further more moving this blob to devicetree will not allow us to treat
the various QMP configurations as one HW block, as there are other
differences as well.

Like many other drivers it's possible to create a generic version that
has every bit of logic driven by configuration from devicetree, but like
most of those cases this is not the way we split things.

And this has the side effect of keeping the dts files human readable,
human understandable and human maintainable.

right. That's why I recommend having clearly defined bindings.
phy,tx- = 
phy,tx- = 
phy,tx- = 

There's no doubt that this table needs to be encoded somewhere, so the
question is should we hard code this in a C file or in a DTSI file.

Skimming through [1] I see examples of things that differs based on how
the specific component is integrated in a SoC or on a particular board -
properties that are relevant to a "system integrator".

As far as I can tell this blob will, if ever, only be changed by a
driver developer and as such it's not carry information about how this
component relates to the rest of the system and should as such not be
part of the device tree.


If there are properties of the hardware that is affected by how the
component is integrated in the system I really would like for those to
be exposed as human-readable properties that I can understand and alter
without deep knowledge about the register map of the hardware block.


I am reaching out to our internal teams to get more information
on different possible phy configurations, based on which the registers
values are decided.
This is something that i tried to understand in the past as well, but 
couldn't

grab much information that time.
Will come back with relevant information on this.


Regards
Vivek


[1] -> https://www.linuxplumbersconf.org/2016/ocw/proposals/4047

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] drm/ttm: Make sure BOs being swapped out are cacheable

[Thomas Hellstrom]

On 01/27/2017 03:29 AM, Michel Dänzer wrote:
> On 26/01/17 09:46 AM, Sinclair Yeh wrote:
>> On Wed, Jan 25, 2017 at 10:49:33AM +0100, Christian König wrote:
>>> Am 25.01.2017 um 10:25 schrieb Thomas Hellstrom:
 On 01/25/2017 09:21 AM, Michel Dänzer wrote:
> From: Michel Dänzer 
>
> The current caching state may not be tt_cached, even though the
> placement contains TTM_PL_FLAG_CACHED, because placement can contain
> multiple caching flags. Trying to swap out such a BO would trip up the
>
>   BUG_ON(ttm->caching_state != tt_cached);
>
> in ttm_tt_swapout.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Michel Dänzer 
 Reviewed-by: Thomas Hellstrom 
>>> Reviewed-by: Christian König .
>> Reviewed-by: Sinclair Yeh 
> Thanks for the reviews! Via which tree should we merge this?
>
>
I don't maintain a TTM tree any longer. Let's check with Daniel if he
can merge it through drm-misc.

/Thomas

Re: [PATCH] drm/ttm: Make sure BOs being swapped out are cacheable

[Thomas Hellstrom]

On 01/27/2017 03:29 AM, Michel Dänzer wrote:
> On 26/01/17 09:46 AM, Sinclair Yeh wrote:
>> On Wed, Jan 25, 2017 at 10:49:33AM +0100, Christian König wrote:
>>> Am 25.01.2017 um 10:25 schrieb Thomas Hellstrom:
 On 01/25/2017 09:21 AM, Michel Dänzer wrote:
> From: Michel Dänzer 
>
> The current caching state may not be tt_cached, even though the
> placement contains TTM_PL_FLAG_CACHED, because placement can contain
> multiple caching flags. Trying to swap out such a BO would trip up the
>
>   BUG_ON(ttm->caching_state != tt_cached);
>
> in ttm_tt_swapout.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Michel Dänzer 
 Reviewed-by: Thomas Hellstrom 
>>> Reviewed-by: Christian König .
>> Reviewed-by: Sinclair Yeh 
> Thanks for the reviews! Via which tree should we merge this?
>
>
I don't maintain a TTM tree any longer. Let's check with Daniel if he
can merge it through drm-misc.

/Thomas

Re: [PATCH 8/8] Revert "ext4: fix wrong gfp type under transaction"

[Theodore Ts'o]

On Thu, Jan 26, 2017 at 08:44:55AM +0100, Michal Hocko wrote:
> > > I'm convinced the current series is OK, only real life will tell us 
> > > whether
> > > we missed something or not ;)
> > 
> > I would like to extend the changelog of "jbd2: mark the transaction
> > context with the scope GFP_NOFS context".
> > 
> > "
> > Please note that setups without journal do not suffer from potential
> > recursion problems and so they do not need the scope protection because
> > neither ->releasepage nor ->evict_inode (which are the only fs entry
> > points from the direct reclaim) can reenter a locked context which is
> > doing the allocation currently.
> > "
> 
> Could you comment on this Ted, please?

I guess   so there still is one way this could screw us, and it's this 
reason for GFP_NOFS:

- to prevent from stack overflows during the reclaim because
  the allocation is performed from a deep context already

The writepages call stack can be pretty deep.  (Especially if we're
using ext4 in no journal mode over, say, iSCSI.)

How much stack space can get consumed by a reclaim?

- Ted
 

Re: [PATCH 8/8] Revert "ext4: fix wrong gfp type under transaction"

[Theodore Ts'o]

On Thu, Jan 26, 2017 at 08:44:55AM +0100, Michal Hocko wrote:
> > > I'm convinced the current series is OK, only real life will tell us 
> > > whether
> > > we missed something or not ;)
> > 
> > I would like to extend the changelog of "jbd2: mark the transaction
> > context with the scope GFP_NOFS context".
> > 
> > "
> > Please note that setups without journal do not suffer from potential
> > recursion problems and so they do not need the scope protection because
> > neither ->releasepage nor ->evict_inode (which are the only fs entry
> > points from the direct reclaim) can reenter a locked context which is
> > doing the allocation currently.
> > "
> 
> Could you comment on this Ted, please?

I guess   so there still is one way this could screw us, and it's this 
reason for GFP_NOFS:

- to prevent from stack overflows during the reclaim because
  the allocation is performed from a deep context already

The writepages call stack can be pretty deep.  (Especially if we're
using ext4 in no journal mode over, say, iSCSI.)

How much stack space can get consumed by a reclaim?

- Ted
 

Re: [PATCH] xen,input: try to read screen resolution for xen-kbdfront

[Juergen Gross]

On 24/01/17 19:47, Dmitry Torokhov wrote:
> On Tue, Jan 24, 2017 at 01:09:55PM +0100, Juergen Gross wrote:
>> Instead of using the default resolution of 800*600 for the pointing
>> device of xen-kbdfront try to read the resolution of the (virtual)
>> framebuffer device. Use the default as fallback only.
>>
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Juergen Gross 
>> ---
>>  drivers/input/misc/xen-kbdfront.c | 15 ---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c 
>> b/drivers/input/misc/xen-kbdfront.c
>> index 3900875..0032c81 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -16,6 +16,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  
>> @@ -108,10 +109,11 @@ static irqreturn_t input_handler(int rq, void *dev_id)
>>  static int xenkbd_probe(struct xenbus_device *dev,
>>const struct xenbus_device_id *id)
>>  {
>> -int ret, i;
>> +int ret, i, width, height;
>>  unsigned int abs;
>>  struct xenkbd_info *info;
>>  struct input_dev *kbd, *ptr;
>> +struct fb_info *fb0;
>>  
>>  info = kzalloc(sizeof(*info), GFP_KERNEL);
>>  if (!info) {
>> @@ -173,9 +175,16 @@ static int xenkbd_probe(struct xenbus_device *dev,
>>  ptr->id.product = 0xfffe;
>>  
>>  if (abs) {
>> +width = XENFB_WIDTH;
>> +height = XENFB_HEIGHT;
>> +fb0 = registered_fb[0];
> 
> This will break if !CONFIG_FBi I think. While i see that xen.config has
> it on I wonder if it is still possible to turn it off (either randconfig
> or intentionally).

kbuild robot says it is. :-(

Sending V2.


Thanks,

Juergen

Re: [PATCH] xen,input: try to read screen resolution for xen-kbdfront

[Juergen Gross]

On 24/01/17 19:47, Dmitry Torokhov wrote:
> On Tue, Jan 24, 2017 at 01:09:55PM +0100, Juergen Gross wrote:
>> Instead of using the default resolution of 800*600 for the pointing
>> device of xen-kbdfront try to read the resolution of the (virtual)
>> framebuffer device. Use the default as fallback only.
>>
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Juergen Gross 
>> ---
>>  drivers/input/misc/xen-kbdfront.c | 15 ---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c 
>> b/drivers/input/misc/xen-kbdfront.c
>> index 3900875..0032c81 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -16,6 +16,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  
>> @@ -108,10 +109,11 @@ static irqreturn_t input_handler(int rq, void *dev_id)
>>  static int xenkbd_probe(struct xenbus_device *dev,
>>const struct xenbus_device_id *id)
>>  {
>> -int ret, i;
>> +int ret, i, width, height;
>>  unsigned int abs;
>>  struct xenkbd_info *info;
>>  struct input_dev *kbd, *ptr;
>> +struct fb_info *fb0;
>>  
>>  info = kzalloc(sizeof(*info), GFP_KERNEL);
>>  if (!info) {
>> @@ -173,9 +175,16 @@ static int xenkbd_probe(struct xenbus_device *dev,
>>  ptr->id.product = 0xfffe;
>>  
>>  if (abs) {
>> +width = XENFB_WIDTH;
>> +height = XENFB_HEIGHT;
>> +fb0 = registered_fb[0];
> 
> This will break if !CONFIG_FBi I think. While i see that xen.config has
> it on I wonder if it is still possible to turn it off (either randconfig
> or intentionally).

kbuild robot says it is. :-(

Sending V2.


Thanks,

Juergen

Re: [PATCH v3] gpio: aspeed: Add banks Y, Z, AA, AB and AC

[Joel Stanley]

On Fri, Jan 27, 2017 at 3:24 PM, Andrew Jeffery  wrote:
> This is less straight-forward than one would hope, as some banks only
> have 4 pins rather than 8, others are output only, yet more (W and
> X, already supported) are input-only, and in the case of the g4 SoC bank
> AC doesn't exist.
>
> Add some structs to describe the varying properties of different banks
> and integrate mechanisms to deny requests for unsupported
> configurations.
>
> Signed-off-by: Andrew Jeffery 

Acked-by: Joel Stanley 

Cheers,

Joel

> ---
>
> Since v2:
>
> Drop linux/gpio.h include and change away from using GPIOF_* macros
>
> Since v1:
>
> Fix types for for_each_clear_bit() iteration
>
>  drivers/gpio/gpio-aspeed.c | 173 
> +
>  1 file changed, 159 insertions(+), 14 deletions(-)

Re: [PATCH v3] gpio: aspeed: Add banks Y, Z, AA, AB and AC

[Joel Stanley]

On Fri, Jan 27, 2017 at 3:24 PM, Andrew Jeffery  wrote:
> This is less straight-forward than one would hope, as some banks only
> have 4 pins rather than 8, others are output only, yet more (W and
> X, already supported) are input-only, and in the case of the g4 SoC bank
> AC doesn't exist.
>
> Add some structs to describe the varying properties of different banks
> and integrate mechanisms to deny requests for unsupported
> configurations.
>
> Signed-off-by: Andrew Jeffery 

Acked-by: Joel Stanley 

Cheers,

Joel

> ---
>
> Since v2:
>
> Drop linux/gpio.h include and change away from using GPIOF_* macros
>
> Since v1:
>
> Fix types for for_each_clear_bit() iteration
>
>  drivers/gpio/gpio-aspeed.c | 173 
> +
>  1 file changed, 159 insertions(+), 14 deletions(-)

Re: [PATCH 3/3] powerpc: enable support for GCC plugins

[Andrew Donnellan]

On 27/01/17 16:52, Andrew Donnellan wrote:

basic-block.h includes tm.h, and I don't believe we can remove that. I'm
not convinced there's a way around this.


Includes via function.h, I should say.

--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

Re: [PATCH 3/3] powerpc: enable support for GCC plugins

[Andrew Donnellan]

On 27/01/17 16:52, Andrew Donnellan wrote:

basic-block.h includes tm.h, and I don't believe we can remove that. I'm
not convinced there's a way around this.


Includes via function.h, I should say.

--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM

[Harini Katakam]

Hi Rafal,

On Thu, Jan 26, 2017 at 8:45 PM, Rafal Ozieblo  wrote:
>> -Original Message-
>> From: Andrei Pistirica [mailto:andrei.pistir...@microchip.com]
>> Sent: 19 stycznia 2017 16:56
>> Subject: [PATCH net-next v2] macb: Common code to enable ptp support for 
>> MACB/GEM
>>
>>
>> +static inline bool gem_has_ptp(struct macb *bp)
>> +{
>> + return !!(bp->caps & MACB_CAPS_GEM_HAS_PTP);
>> +}
> Why don't you use hardware capabilities here? Would it be better to read it 
> from hardware instead adding it to many configuration?

If you are referring to TSU bit in DCFG5, then we will be relying on
Cadence IP's information irrespective of the SoC capability
and whether the PTP support was adequate.
I think the capability approach gives better control and
it is not really much to add.

Regards,
Harini

Re: [PATCH net-next v2] macb: Common code to enable ptp support for MACB/GEM

[Harini Katakam]

Hi Rafal,

On Thu, Jan 26, 2017 at 8:45 PM, Rafal Ozieblo  wrote:
>> -Original Message-
>> From: Andrei Pistirica [mailto:andrei.pistir...@microchip.com]
>> Sent: 19 stycznia 2017 16:56
>> Subject: [PATCH net-next v2] macb: Common code to enable ptp support for 
>> MACB/GEM
>>
>>
>> +static inline bool gem_has_ptp(struct macb *bp)
>> +{
>> + return !!(bp->caps & MACB_CAPS_GEM_HAS_PTP);
>> +}
> Why don't you use hardware capabilities here? Would it be better to read it 
> from hardware instead adding it to many configuration?

If you are referring to TSU bit in DCFG5, then we will be relying on
Cadence IP's information irrespective of the SoC capability
and whether the PTP support was adequate.
I think the capability approach gives better control and
it is not really much to add.

Regards,
Harini

hi

[cputrdoc]

hi 


http://www.mobilam.net/education.php?death=2afxa279semd4a




cputrdoc

hi

[cputrdoc]

hi 


http://www.mobilam.net/education.php?death=2afxa279semd4a




cputrdoc

Re: [PATCH v4 3/4] dt-bindings: phy: Add support for QMP phy

[Vivek Gautam]

On 01/27/2017 05:13 AM, Stephen Boyd wrote:

On 01/24, Vivek Gautam wrote:

Below is one binding that works for me.

phy@34000 {
 compatible = "qcom,msm8996-qmp-pcie-phy";
 reg = <0x034000 0x488>;
 #clock-cells = <1>;
 #address-cells = <1>;
 #size-cells = <1>;
 ranges;

 clocks = < GCC_PCIE_PHY_AUX_CLK>,
 < GCC_PCIE_PHY_CFG_AHB_CLK>,
 < GCC_PCIE_CLKREF_CLK>;
 clock-names = "aux", "cfg_ahb", "ref";

 vdda-phy-supply = <_l28>;
 vdda-pll-supply = <_l12>;

 resets = < GCC_PCIE_PHY_BCR>,
 < GCC_PCIE_PHY_COM_BCR>,
 < GCC_PCIE_PHY_COM_NOCSR_BCR>;
 reset-names = "phy", "common", "cfg";

 pciephy_p0: port@0 {

The unit address '@0' should be replaced with something from the
reg properties.


Sure, will take care of this.

  


Also 'port' and 'ports' are almost keywords in DT now with the
graph binding so we need to be careful when using them.


From "./Documentation/devicetree/bindings/graph.txt" -
"The device tree graph bindings described herein abstract more complex
devices that can have multiple specifiable ports, each of which can be
linked to one or more ports of other devices."

So, this means we use 'port', 'ports' and 'endpoint' for devices whose one
or more ports is connected to other device's one or more ports.

I can use 'lane' for the node name here.




 reg = <0x035000 0x130>,
 <0x035200 0x200>,
 <0x035400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_0_PIPE_CLK>;
 clock-names = "pipe0";
 resets = < GCC_PCIE_0_PHY_BCR>;
 reset-names = "lane0";
 };

   pciephy_p1: port@1 {
 reg = <0x036000 0x130>,
 <0x036200 0x200>,
 <0x036400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_1_PIPE_CLK>;
 clock-names = "pipe1";
 resets = < GCC_PCIE_1_PHY_BCR>;
 reset-names = "lane1";
 };

 pciephy_p2: port@2 {
 reg = <0x037000 0x130>,
 <0x037200 0x200>,
 <0x037400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_2_PIPE_CLK>;
 clock-names = "pipe2";
 resets = < GCC_PCIE_2_PHY_BCR>;
 reset-names = "lane2";
 };
 };


let me know if this looks okay.



What's the plan for non-pcie qmp phy binding? In that case we
don't have ports, so it gets folded into one node?


The non-pcie qmp phys still have one lane, that provides tx/rx.

I am of the opinion that we don't have two different ways to create
phys in the driver, and keep one port/lane for such phys in dt.


Regards
Vivek

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 3/4] dt-bindings: phy: Add support for QMP phy

[Vivek Gautam]

On 01/27/2017 05:13 AM, Stephen Boyd wrote:

On 01/24, Vivek Gautam wrote:

Below is one binding that works for me.

phy@34000 {
 compatible = "qcom,msm8996-qmp-pcie-phy";
 reg = <0x034000 0x488>;
 #clock-cells = <1>;
 #address-cells = <1>;
 #size-cells = <1>;
 ranges;

 clocks = < GCC_PCIE_PHY_AUX_CLK>,
 < GCC_PCIE_PHY_CFG_AHB_CLK>,
 < GCC_PCIE_CLKREF_CLK>;
 clock-names = "aux", "cfg_ahb", "ref";

 vdda-phy-supply = <_l28>;
 vdda-pll-supply = <_l12>;

 resets = < GCC_PCIE_PHY_BCR>,
 < GCC_PCIE_PHY_COM_BCR>,
 < GCC_PCIE_PHY_COM_NOCSR_BCR>;
 reset-names = "phy", "common", "cfg";

 pciephy_p0: port@0 {

The unit address '@0' should be replaced with something from the
reg properties.


Sure, will take care of this.

  


Also 'port' and 'ports' are almost keywords in DT now with the
graph binding so we need to be careful when using them.


From "./Documentation/devicetree/bindings/graph.txt" -
"The device tree graph bindings described herein abstract more complex
devices that can have multiple specifiable ports, each of which can be
linked to one or more ports of other devices."

So, this means we use 'port', 'ports' and 'endpoint' for devices whose one
or more ports is connected to other device's one or more ports.

I can use 'lane' for the node name here.




 reg = <0x035000 0x130>,
 <0x035200 0x200>,
 <0x035400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_0_PIPE_CLK>;
 clock-names = "pipe0";
 resets = < GCC_PCIE_0_PHY_BCR>;
 reset-names = "lane0";
 };

   pciephy_p1: port@1 {
 reg = <0x036000 0x130>,
 <0x036200 0x200>,
 <0x036400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_1_PIPE_CLK>;
 clock-names = "pipe1";
 resets = < GCC_PCIE_1_PHY_BCR>;
 reset-names = "lane1";
 };

 pciephy_p2: port@2 {
 reg = <0x037000 0x130>,
 <0x037200 0x200>,
 <0x037400 0x1dc>;
 #phy-cells = <0>;

 clocks = < GCC_PCIE_2_PIPE_CLK>;
 clock-names = "pipe2";
 resets = < GCC_PCIE_2_PHY_BCR>;
 reset-names = "lane2";
 };
 };


let me know if this looks okay.



What's the plan for non-pcie qmp phy binding? In that case we
don't have ports, so it gets folded into one node?


The non-pcie qmp phys still have one lane, that provides tx/rx.

I am of the opinion that we don't have two different ways to create
phys in the driver, and keep one port/lane for such phys in dt.


Regards
Vivek

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: fs, net: deadlock between bind/splice on af_unix

[Cong Wang]

On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik  wrote:
> On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
>> On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov  wrote:
>> > On Fri, Dec 9, 2016 at 7:41 AM, Al Viro  wrote:
>> >> On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
>> >>
>> >>> > Why do we do autobind there, anyway, and why is it conditional on
>> >>> > SOCK_PASSCRED?  Note that e.g. for SOCK_STREAM we can bloody well get
>> >>> > to sending stuff without autobind ever done - just use socketpair()
>> >>> > to create that sucker and we won't be going through the connect()
>> >>> > at all.
>> >>>
>> >>> In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
>> >>> not SOCK_STREAM.
>> >>
>> >> Yes, I've noticed.  What I'm asking is what in there needs autobind 
>> >> triggered
>> >> on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
>> >>
>> >>> I guess some lock, perhaps the u->bindlock could be dropped before
>> >>> acquiring the next one (sb_writer), but I need to double check.
>> >>
>> >> Bad idea, IMO - do you *want* autobind being able to come through while
>> >> bind(2) is busy with mknod?
>> >
>> >
>> > Ping. This is still happening on HEAD.
>> >
>>
>> Thanks for your reminder. Mind to give the attached patch (compile only)
>> a try? I take another approach to fix this deadlock, which moves the
>> unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
>> impact with this way.
>>
>
> I don't think this is the right approach.
>
> Currently the file creation is potponed until unix_bind can no longer
> fail otherwise. With it reordered, it may be someone races you with a
> different path and now you are left with a file to clean up. Except it
> is quite unclear for me if you can unlink it.

What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.

Re: fs, net: deadlock between bind/splice on af_unix

[Cong Wang]

On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik  wrote:
> On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
>> On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov  wrote:
>> > On Fri, Dec 9, 2016 at 7:41 AM, Al Viro  wrote:
>> >> On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
>> >>
>> >>> > Why do we do autobind there, anyway, and why is it conditional on
>> >>> > SOCK_PASSCRED?  Note that e.g. for SOCK_STREAM we can bloody well get
>> >>> > to sending stuff without autobind ever done - just use socketpair()
>> >>> > to create that sucker and we won't be going through the connect()
>> >>> > at all.
>> >>>
>> >>> In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
>> >>> not SOCK_STREAM.
>> >>
>> >> Yes, I've noticed.  What I'm asking is what in there needs autobind 
>> >> triggered
>> >> on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
>> >>
>> >>> I guess some lock, perhaps the u->bindlock could be dropped before
>> >>> acquiring the next one (sb_writer), but I need to double check.
>> >>
>> >> Bad idea, IMO - do you *want* autobind being able to come through while
>> >> bind(2) is busy with mknod?
>> >
>> >
>> > Ping. This is still happening on HEAD.
>> >
>>
>> Thanks for your reminder. Mind to give the attached patch (compile only)
>> a try? I take another approach to fix this deadlock, which moves the
>> unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
>> impact with this way.
>>
>
> I don't think this is the right approach.
>
> Currently the file creation is potponed until unix_bind can no longer
> fail otherwise. With it reordered, it may be someone races you with a
> different path and now you are left with a file to clean up. Except it
> is quite unclear for me if you can unlink it.

What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.

Re: [PATCH 1/5] DT: mfd: axp20x: Add AXP806 to list of current AXP20x family members

[Chen-Yu Tsai]

On Fri, Jan 27, 2017 at 5:23 AM, Rask Ingemann Lambertsen
 wrote:
> An entry for the AXP806 was forgotten, so add one.
>
> Signed-off-by: Rask Ingemann Lambertsen 
> Fixes: 204ae2963e10 ("mfd: axp20x: Add bindings for AXP806 PMIC")

Acked-by: Chen-Yu Tsai 

Re: [PATCH 1/5] DT: mfd: axp20x: Add AXP806 to list of current AXP20x family members

[Chen-Yu Tsai]

On Fri, Jan 27, 2017 at 5:23 AM, Rask Ingemann Lambertsen
 wrote:
> An entry for the AXP806 was forgotten, so add one.
>
> Signed-off-by: Rask Ingemann Lambertsen 
> Fixes: 204ae2963e10 ("mfd: axp20x: Add bindings for AXP806 PMIC")

Acked-by: Chen-Yu Tsai 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,


Am 26.01.2017 um 21:47 schrieb Finn Thain:

>> I hadn't considered that. Can PDMA for Falcon SCSI coexist with 
>> interrupt-using DMA for TT SCSI in the same driver (i.e. as runtime 
>> options)?
> 
> Sure, why not?
> 
>> How much overhead and latency would polling for DMA completion add?
>>
> 
> A polled DMA transfer should be faster than PDMA (i.e. mac_scsi, g_NCR5380 
> etc). mac_scsi gets about 0.5 MBps from PDMA with sg_tablesize == 1, and I 
> hope that DMA could get twice that (notwithstanding dumb hardware design). 

DMA contends with the processor use of the data bus but that's true for
many DMA designs. Don't think Atari made any more dumb decisions (the
opaque DMA FIFO is probably the worst feature there but that could be
worked around at considerable expense by programming the DMA and the
SCSI bus transfer for one additional 512 byte block. Let's not go there.)

> This would imply CPU overhead that is half of that which mac_scsi incurs. 
> That's the best case, but I see no reason to expect worse performance than 
> PDMA gets.

But how much more overhead would we have compared to using the SCSI
interrupt to signal DMA completion?

>> atari_irq_pending(IRQ_MFP_FSCSI) should show the interrupt pending 
>> condition if you want to poll for it.
> 
> The difficulty will be arranging for disabled FDC & IDE interrupt sources 
> during SCSI DMA, and disabled SCSI & IDE interrupt sources during FDC DMA. 
> (Not all 5380 interrupts can be disabled; no idea about the IDE device or 
> WD1772 FDC.)
> 
> But if that is impossible, we just have to detect the short DMA that might 
> result from an undesired interrupt.

I think that's infeasible - IDE interrupts could be disabled at disk
level as Geert suggests but I don't think there is a kernel API for
other drivers to do so?

At the IRQ controller level, it's all or none due to the wired-OR design
as you pointed out in the reply to Geert's mail.

>> That's actually given me another idea to pursue - if we can ensure the 
>> IDE interrupt handler is always run first,
> 
> There are no interrupts from the ATA driver you're testing, right? If you 
> would re-introduce them, the whole polled DMA idea is moot.

The libata driver currently does disable the IDE interrupt and uses
polling, but I'd like to change that if at all possible. Sorry I didn't
make that clear.

As far as I could see during my testing, the current libata driver
coexists just fine with interrupt driven SCSI operation. I've once seen
a 'lost arbitration' message in the very first test when loading the
SCSI driver, but that's not been repeated. No problems seen otherwise.

>> If we manage to separate interrupt sharing from DMA access locking, IDE 
>> would not need to take part in the locking. I'm assuming that IDE can 
>> cope with spurious interrupts and won't get confused by a SCSI 
>> interrupt.
>>
> 
> The ATA driver will never have to cope with a spurious interrupt under my 
> simplifying assumptions discussed earlier, so the spurious interrupt 
> question seems to belong to some alternative approach...

I was assuming IDE could have interrupts reenabled in libata for that
discussion.

>> I think it could work both ways - polling for DMA completion or avoiding 
>> to call the SCSI interrupt handler the interrupt was caused by IDE only. 
>> But it's indeed time to put that to the test.
>>
> 
> ... "Both ways"? I don't follow. I don't see how IDE can share the FDC and 
> SCSI interrupt line without sharing the stdma.c locking scheme. What is 
> the alternative approach (i.e not polled DMA) that you alude to?

Since IDE does not use the ST-DMA and does not share any registers with
ST-DMA, peeking at the IDE status register in order to decide whether
the interrupt was raised by the IDE interface won't hurt the running DMA
process (regardless of whether FDC or SCSI started it). Nor will
servicing the IDE interrupt.

If at the end of the IDE interrupt processing the interrupt status is
cleared in the IDE interface, the interrupt line should go high again
before the IDE inthandler returns.

If we can ensure that the FDC/SCSI interrupt handler runs after the IDE
handler, we can then make that handler check the interrupt line status
and bail out if there's nothing to be done. (For the sake of simplicity,
this check can be done in stdma_int() since we need to retain mutual
locking of the DMA interface by SCSI and FDC anyway.) We can ensure the
IDE interrupt is called first by using a special interrupt controller to
register separate IDE and FDC/SCSI interrupts with (I've done that to
provide distinct interrupt numbers and handlers for the timer D
interrupt that's used to poll ethernet and USB interface status on the
ROM port).

That way, we can ensure IDE interrupts do not step on the ST-DMA state,
and all that remains are premature SCSI interrupts terminating DMA
transfer (which we already face anyway).

Am I missing a potential race here? Does IDE send the next request off
to the disk 

Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support

[Michael Schmitz]

Hi Finn,


Am 26.01.2017 um 21:47 schrieb Finn Thain:

>> I hadn't considered that. Can PDMA for Falcon SCSI coexist with 
>> interrupt-using DMA for TT SCSI in the same driver (i.e. as runtime 
>> options)?
> 
> Sure, why not?
> 
>> How much overhead and latency would polling for DMA completion add?
>>
> 
> A polled DMA transfer should be faster than PDMA (i.e. mac_scsi, g_NCR5380 
> etc). mac_scsi gets about 0.5 MBps from PDMA with sg_tablesize == 1, and I 
> hope that DMA could get twice that (notwithstanding dumb hardware design). 

DMA contends with the processor use of the data bus but that's true for
many DMA designs. Don't think Atari made any more dumb decisions (the
opaque DMA FIFO is probably the worst feature there but that could be
worked around at considerable expense by programming the DMA and the
SCSI bus transfer for one additional 512 byte block. Let's not go there.)

> This would imply CPU overhead that is half of that which mac_scsi incurs. 
> That's the best case, but I see no reason to expect worse performance than 
> PDMA gets.

But how much more overhead would we have compared to using the SCSI
interrupt to signal DMA completion?

>> atari_irq_pending(IRQ_MFP_FSCSI) should show the interrupt pending 
>> condition if you want to poll for it.
> 
> The difficulty will be arranging for disabled FDC & IDE interrupt sources 
> during SCSI DMA, and disabled SCSI & IDE interrupt sources during FDC DMA. 
> (Not all 5380 interrupts can be disabled; no idea about the IDE device or 
> WD1772 FDC.)
> 
> But if that is impossible, we just have to detect the short DMA that might 
> result from an undesired interrupt.

I think that's infeasible - IDE interrupts could be disabled at disk
level as Geert suggests but I don't think there is a kernel API for
other drivers to do so?

At the IRQ controller level, it's all or none due to the wired-OR design
as you pointed out in the reply to Geert's mail.

>> That's actually given me another idea to pursue - if we can ensure the 
>> IDE interrupt handler is always run first,
> 
> There are no interrupts from the ATA driver you're testing, right? If you 
> would re-introduce them, the whole polled DMA idea is moot.

The libata driver currently does disable the IDE interrupt and uses
polling, but I'd like to change that if at all possible. Sorry I didn't
make that clear.

As far as I could see during my testing, the current libata driver
coexists just fine with interrupt driven SCSI operation. I've once seen
a 'lost arbitration' message in the very first test when loading the
SCSI driver, but that's not been repeated. No problems seen otherwise.

>> If we manage to separate interrupt sharing from DMA access locking, IDE 
>> would not need to take part in the locking. I'm assuming that IDE can 
>> cope with spurious interrupts and won't get confused by a SCSI 
>> interrupt.
>>
> 
> The ATA driver will never have to cope with a spurious interrupt under my 
> simplifying assumptions discussed earlier, so the spurious interrupt 
> question seems to belong to some alternative approach...

I was assuming IDE could have interrupts reenabled in libata for that
discussion.

>> I think it could work both ways - polling for DMA completion or avoiding 
>> to call the SCSI interrupt handler the interrupt was caused by IDE only. 
>> But it's indeed time to put that to the test.
>>
> 
> ... "Both ways"? I don't follow. I don't see how IDE can share the FDC and 
> SCSI interrupt line without sharing the stdma.c locking scheme. What is 
> the alternative approach (i.e not polled DMA) that you alude to?

Since IDE does not use the ST-DMA and does not share any registers with
ST-DMA, peeking at the IDE status register in order to decide whether
the interrupt was raised by the IDE interface won't hurt the running DMA
process (regardless of whether FDC or SCSI started it). Nor will
servicing the IDE interrupt.

If at the end of the IDE interrupt processing the interrupt status is
cleared in the IDE interface, the interrupt line should go high again
before the IDE inthandler returns.

If we can ensure that the FDC/SCSI interrupt handler runs after the IDE
handler, we can then make that handler check the interrupt line status
and bail out if there's nothing to be done. (For the sake of simplicity,
this check can be done in stdma_int() since we need to retain mutual
locking of the DMA interface by SCSI and FDC anyway.) We can ensure the
IDE interrupt is called first by using a special interrupt controller to
register separate IDE and FDC/SCSI interrupts with (I've done that to
provide distinct interrupt numbers and handlers for the timer D
interrupt that's used to poll ethernet and USB interface status on the
ROM port).

That way, we can ensure IDE interrupts do not step on the ST-DMA state,
and all that remains are premature SCSI interrupts terminating DMA
transfer (which we already face anyway).

Am I missing a potential race here? Does IDE send the next request off
to the disk 

[PATCH] staging: octeon-usb: Fix coding style issues

[William Blough]

Wrap lines that exceed 80 characters.

Signed-off-by: William Blough 
---
 drivers/staging/octeon-usb/octeon-hcd.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
b/drivers/staging/octeon-usb/octeon-hcd.c
index 9a7858a..6b86bfb 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2635,13 +2635,19 @@ static int cvmx_usb_poll_channel(struct octeon_hcd 
*usb, int channel)
/* Disable all interrupts except CHHLTD */
hcintmsk.u32 = 0;
hcintmsk.s.chhltdmsk = 1;
-   cvmx_usb_write_csr32(usb,
-
CVMX_USBCX_HCINTMSKX(channel, usb->index),
-hcintmsk.u32);
+   cvmx_usb_write_csr32(
+   usb,
+   CVMX_USBCX_HCINTMSKX(
+   channel,
+   usb->index),
+   hcintmsk.u32);
usbc_hcchar.s.chdis = 1;
-   cvmx_usb_write_csr32(usb,
-
CVMX_USBCX_HCCHARX(channel, usb->index),
-usbc_hcchar.u32);
+   cvmx_usb_write_csr32(
+   usb,
+   CVMX_USBCX_HCCHARX(
+   channel,
+   usb->index),
+   usbc_hcchar.u32);
return 0;
} else if (usbc_hcint.s.xfercompl) {
/*
-- 
2.1.4

[PATCH] staging: octeon-usb: Fix coding style issues

[William Blough]

Wrap lines that exceed 80 characters.

Signed-off-by: William Blough 
---
 drivers/staging/octeon-usb/octeon-hcd.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/octeon-usb/octeon-hcd.c 
b/drivers/staging/octeon-usb/octeon-hcd.c
index 9a7858a..6b86bfb 100644
--- a/drivers/staging/octeon-usb/octeon-hcd.c
+++ b/drivers/staging/octeon-usb/octeon-hcd.c
@@ -2635,13 +2635,19 @@ static int cvmx_usb_poll_channel(struct octeon_hcd 
*usb, int channel)
/* Disable all interrupts except CHHLTD */
hcintmsk.u32 = 0;
hcintmsk.s.chhltdmsk = 1;
-   cvmx_usb_write_csr32(usb,
-
CVMX_USBCX_HCINTMSKX(channel, usb->index),
-hcintmsk.u32);
+   cvmx_usb_write_csr32(
+   usb,
+   CVMX_USBCX_HCINTMSKX(
+   channel,
+   usb->index),
+   hcintmsk.u32);
usbc_hcchar.s.chdis = 1;
-   cvmx_usb_write_csr32(usb,
-
CVMX_USBCX_HCCHARX(channel, usb->index),
-usbc_hcchar.u32);
+   cvmx_usb_write_csr32(
+   usb,
+   CVMX_USBCX_HCCHARX(
+   channel,
+   usb->index),
+   usbc_hcchar.u32);
return 0;
} else if (usbc_hcint.s.xfercompl) {
/*
-- 
2.1.4

[PATCH v3] gpio: aspeed: Add banks Y, Z, AA, AB and AC

[Andrew Jeffery]

This is less straight-forward than one would hope, as some banks only
have 4 pins rather than 8, others are output only, yet more (W and
X, already supported) are input-only, and in the case of the g4 SoC bank
AC doesn't exist.

Add some structs to describe the varying properties of different banks
and integrate mechanisms to deny requests for unsupported
configurations.

Signed-off-by: Andrew Jeffery 
---

Since v2:

Drop linux/gpio.h include and change away from using GPIOF_* macros

Since v1:

Fix types for for_each_clear_bit() iteration

 drivers/gpio/gpio-aspeed.c | 173 +
 1 file changed, 159 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 20f6f8ae4671..fb16cc771c0d 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -18,11 +18,23 @@
 #include 
 #include 
 
+struct aspeed_bank_props {
+   unsigned int bank;
+   u32 input;
+   u32 output;
+};
+
+struct aspeed_gpio_config {
+   unsigned int nr_gpios;
+   const struct aspeed_bank_props *props;
+};
+
 struct aspeed_gpio {
struct gpio_chip chip;
spinlock_t lock;
void __iomem *base;
int irq;
+   const struct aspeed_gpio_config *config;
 };
 
 struct aspeed_gpio_bank {
@@ -62,11 +74,16 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
.irq_regs = 0x0148,
.names = { "U", "V", "W", "X" },
},
-   /*
-* A bank exists for { 'Y', 'Z', "AA", "AB" }, but is not implemented.
-* Only half of GPIOs Y support interrupt configuration, and none of Z,
-* AA or AB do as they are output only.
-*/
+   {
+   .val_regs = 0x01E0,
+   .irq_regs = 0x0178,
+   .names = { "Y", "Z", "AA", "AB" },
+   },
+   {
+   .val_regs = 0x01E8,
+   .irq_regs = 0x01A8,
+   .names = { "AC", "", "", "" },
+   },
 };
 
 #define GPIO_BANK(x)   ((x) >> 5)
@@ -90,6 +107,51 @@ static const struct aspeed_gpio_bank *to_bank(unsigned int 
offset)
return _gpio_banks[bank];
 }
 
+static inline bool is_bank_props_sentinel(const struct aspeed_bank_props 
*props)
+{
+   return !(props->input || props->output);
+}
+
+static inline const struct aspeed_bank_props *find_bank_props(
+   struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = gpio->config->props;
+
+   while (!is_bank_props_sentinel(props)) {
+   if (props->bank == GPIO_BANK(offset))
+   return props;
+   props++;
+   }
+
+   return NULL;
+}
+
+static inline bool have_gpio(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+   const struct aspeed_gpio_bank *bank = to_bank(offset);
+   unsigned int group = GPIO_OFFSET(offset) / 8;
+
+   return bank->names[group][0] != '\0' &&
+   (!props || ((props->input | props->output) & GPIO_BIT(offset)));
+}
+
+static inline bool have_input(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+   return !props || (props->input & GPIO_BIT(offset));
+}
+
+#define have_irq(g, o) have_input((g), (o))
+
+static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+   return !props || (props->output & GPIO_BIT(offset));
+}
+
 static void __iomem *bank_val_reg(struct aspeed_gpio *gpio,
const struct aspeed_gpio_bank *bank,
unsigned int reg)
@@ -152,6 +214,9 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, 
unsigned int offset)
unsigned long flags;
u32 reg;
 
+   if (!have_input(gpio, offset))
+   return -ENOTSUPP;
+
spin_lock_irqsave(>lock, flags);
 
reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
@@ -170,6 +235,9 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
unsigned long flags;
u32 reg;
 
+   if (!have_output(gpio, offset))
+   return -ENOTSUPP;
+
spin_lock_irqsave(>lock, flags);
 
reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
@@ -189,6 +257,12 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, 
unsigned int offset)
unsigned long flags;
u32 val;
 
+   if (!have_input(gpio, offset))
+   return 0;
+
+   if (!have_output(gpio, offset))
+   return 1;
+
spin_lock_irqsave(>lock, flags);
 
val = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)) & GPIO_BIT(offset);
@@ -205,10 +279,17 @@ static inline int irqd_to_aspeed_gpio_data(struct 
irq_data *d,
u32 *bit)
 {
int offset;
+   struct aspeed_gpio 

[PATCH v3] gpio: aspeed: Add banks Y, Z, AA, AB and AC

[Andrew Jeffery]

This is less straight-forward than one would hope, as some banks only
have 4 pins rather than 8, others are output only, yet more (W and
X, already supported) are input-only, and in the case of the g4 SoC bank
AC doesn't exist.

Add some structs to describe the varying properties of different banks
and integrate mechanisms to deny requests for unsupported
configurations.

Signed-off-by: Andrew Jeffery 
---

Since v2:

Drop linux/gpio.h include and change away from using GPIOF_* macros

Since v1:

Fix types for for_each_clear_bit() iteration

 drivers/gpio/gpio-aspeed.c | 173 +
 1 file changed, 159 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 20f6f8ae4671..fb16cc771c0d 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -18,11 +18,23 @@
 #include 
 #include 
 
+struct aspeed_bank_props {
+   unsigned int bank;
+   u32 input;
+   u32 output;
+};
+
+struct aspeed_gpio_config {
+   unsigned int nr_gpios;
+   const struct aspeed_bank_props *props;
+};
+
 struct aspeed_gpio {
struct gpio_chip chip;
spinlock_t lock;
void __iomem *base;
int irq;
+   const struct aspeed_gpio_config *config;
 };
 
 struct aspeed_gpio_bank {
@@ -62,11 +74,16 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
.irq_regs = 0x0148,
.names = { "U", "V", "W", "X" },
},
-   /*
-* A bank exists for { 'Y', 'Z', "AA", "AB" }, but is not implemented.
-* Only half of GPIOs Y support interrupt configuration, and none of Z,
-* AA or AB do as they are output only.
-*/
+   {
+   .val_regs = 0x01E0,
+   .irq_regs = 0x0178,
+   .names = { "Y", "Z", "AA", "AB" },
+   },
+   {
+   .val_regs = 0x01E8,
+   .irq_regs = 0x01A8,
+   .names = { "AC", "", "", "" },
+   },
 };
 
 #define GPIO_BANK(x)   ((x) >> 5)
@@ -90,6 +107,51 @@ static const struct aspeed_gpio_bank *to_bank(unsigned int 
offset)
return _gpio_banks[bank];
 }
 
+static inline bool is_bank_props_sentinel(const struct aspeed_bank_props 
*props)
+{
+   return !(props->input || props->output);
+}
+
+static inline const struct aspeed_bank_props *find_bank_props(
+   struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = gpio->config->props;
+
+   while (!is_bank_props_sentinel(props)) {
+   if (props->bank == GPIO_BANK(offset))
+   return props;
+   props++;
+   }
+
+   return NULL;
+}
+
+static inline bool have_gpio(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+   const struct aspeed_gpio_bank *bank = to_bank(offset);
+   unsigned int group = GPIO_OFFSET(offset) / 8;
+
+   return bank->names[group][0] != '\0' &&
+   (!props || ((props->input | props->output) & GPIO_BIT(offset)));
+}
+
+static inline bool have_input(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+   return !props || (props->input & GPIO_BIT(offset));
+}
+
+#define have_irq(g, o) have_input((g), (o))
+
+static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
+{
+   const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+   return !props || (props->output & GPIO_BIT(offset));
+}
+
 static void __iomem *bank_val_reg(struct aspeed_gpio *gpio,
const struct aspeed_gpio_bank *bank,
unsigned int reg)
@@ -152,6 +214,9 @@ static int aspeed_gpio_dir_in(struct gpio_chip *gc, 
unsigned int offset)
unsigned long flags;
u32 reg;
 
+   if (!have_input(gpio, offset))
+   return -ENOTSUPP;
+
spin_lock_irqsave(>lock, flags);
 
reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
@@ -170,6 +235,9 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
unsigned long flags;
u32 reg;
 
+   if (!have_output(gpio, offset))
+   return -ENOTSUPP;
+
spin_lock_irqsave(>lock, flags);
 
reg = ioread32(bank_val_reg(gpio, bank, GPIO_DIR));
@@ -189,6 +257,12 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, 
unsigned int offset)
unsigned long flags;
u32 val;
 
+   if (!have_input(gpio, offset))
+   return 0;
+
+   if (!have_output(gpio, offset))
+   return 1;
+
spin_lock_irqsave(>lock, flags);
 
val = ioread32(bank_val_reg(gpio, bank, GPIO_DIR)) & GPIO_BIT(offset);
@@ -205,10 +279,17 @@ static inline int irqd_to_aspeed_gpio_data(struct 
irq_data *d,
u32 *bit)
 {
int offset;
+   struct aspeed_gpio *internal;
 

Re: [PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

On 01/27/2017 03:58 AM, Arnaldo Carvalho de Melo wrote:

Em Thu, Jan 26, 2017 at 06:35:38PM +0900, Taeung Song escreveu:

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

# cat ~/.perfconfig
[ftrace]
tracer = function

# perf ftrace usleep 123456 | head -10
  <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
  <...>-14450 [002]  10089.284232: finish_wait <-pipe_wait
  <...>-14450 [002]  10089.284232: mutex_lock <-pipe_wait
  <...>-14450 [002]  10089.284232: _cond_resched <-mutex_lock
  <...>-14450 [002]  10089.284233: generic_pipe_buf_confirm <-pipe_read

Cc: Jiri Olsa 
Cc: Namhyung Kim 
Signed-off-by: Taeung Song 
---
 tools/perf/builtin-ftrace.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 41d..8df5416 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"


 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int 
argc, const char **argv)
return done ? 0 : -1;
 }

+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+   struct perf_ftrace *ftrace = cb;
+
+   if (!strcmp(var, "ftrace.tracer")) {
+   if (!strcmp(value, "function_graph"))
+   ftrace->tracer = DEFAULT_TRACER;
+   else if (!strcmp(value, "function"))
+   ftrace->tracer = "function";
+   else
+   return -1;


Please warn the user for invalid values and either just say that it is
ignoring that setting or return -1 to make perf_config() return that,
then make cmd_ftrace() check the return of perf_config() and bail out if
you're not ignoring the config value.

I think its better to do a: "pr_err()" in that "return -1" branch and
make 'perf ftrace' fail, so that the user can fix its config before
proceeding.




Arnaldo,

I modified this patch as below.

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 8df5416..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -208,8 +208,11 @@ static int perf_ftrace_config(const char *var, 
const char *value, void *cb)

 ftrace->tracer = DEFAULT_TRACER;
 else if (!strcmp(value, "function"))
 ftrace->tracer = "function";
-else
+else {
+pr_err("Please select function_graph(default)"
+   "or function to use tracer.\n");
 return -1;
+}
 return 0;
 }

@@ -236,7 +239,9 @@ int cmd_ftrace(int argc, const char **argv, const 
char *prefix __maybe_unused)

 OPT_END()
 };

-perf_config(perf_ftrace_config, );
+ret = perf_config(perf_ftrace_config, );
+if (ret < 0)
+return -1;

 argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);


But if setting wrong config value,
existing error message is printed in perf_config().

So output can be printed as follows

1) Two error messages

# perf ftrace usleep 123456
Please select function_graph(default)or function to use tracer.
Error: wrong config key-value pair ftrace.tracer=functionds

2) Or only one error message in perf_config()
(if we don't "pr_err()" in that "return -1" branch)

# perf ftrace usleep 123456
Error: wrong config key-value pair ftrace.tracer=functionds


Which do you like better, 1) or 2) ?

Thanks,
Taeung

Re: [PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

On 01/27/2017 03:58 AM, Arnaldo Carvalho de Melo wrote:

Em Thu, Jan 26, 2017 at 06:35:38PM +0900, Taeung Song escreveu:

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

# cat ~/.perfconfig
[ftrace]
tracer = function

# perf ftrace usleep 123456 | head -10
  <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
  <...>-14450 [002]  10089.284232: finish_wait <-pipe_wait
  <...>-14450 [002]  10089.284232: mutex_lock <-pipe_wait
  <...>-14450 [002]  10089.284232: _cond_resched <-mutex_lock
  <...>-14450 [002]  10089.284233: generic_pipe_buf_confirm <-pipe_read

Cc: Jiri Olsa 
Cc: Namhyung Kim 
Signed-off-by: Taeung Song 
---
 tools/perf/builtin-ftrace.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 41d..8df5416 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"


 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int 
argc, const char **argv)
return done ? 0 : -1;
 }

+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+   struct perf_ftrace *ftrace = cb;
+
+   if (!strcmp(var, "ftrace.tracer")) {
+   if (!strcmp(value, "function_graph"))
+   ftrace->tracer = DEFAULT_TRACER;
+   else if (!strcmp(value, "function"))
+   ftrace->tracer = "function";
+   else
+   return -1;


Please warn the user for invalid values and either just say that it is
ignoring that setting or return -1 to make perf_config() return that,
then make cmd_ftrace() check the return of perf_config() and bail out if
you're not ignoring the config value.

I think its better to do a: "pr_err()" in that "return -1" branch and
make 'perf ftrace' fail, so that the user can fix its config before
proceeding.




Arnaldo,

I modified this patch as below.

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 8df5416..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -208,8 +208,11 @@ static int perf_ftrace_config(const char *var, 
const char *value, void *cb)

 ftrace->tracer = DEFAULT_TRACER;
 else if (!strcmp(value, "function"))
 ftrace->tracer = "function";
-else
+else {
+pr_err("Please select function_graph(default)"
+   "or function to use tracer.\n");
 return -1;
+}
 return 0;
 }

@@ -236,7 +239,9 @@ int cmd_ftrace(int argc, const char **argv, const 
char *prefix __maybe_unused)

 OPT_END()
 };

-perf_config(perf_ftrace_config, );
+ret = perf_config(perf_ftrace_config, );
+if (ret < 0)
+return -1;

 argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);


But if setting wrong config value,
existing error message is printed in perf_config().

So output can be printed as follows

1) Two error messages

# perf ftrace usleep 123456
Please select function_graph(default)or function to use tracer.
Error: wrong config key-value pair ftrace.tracer=functionds

2) Or only one error message in perf_config()
(if we don't "pr_err()" in that "return -1" branch)

# perf ftrace usleep 123456
Error: wrong config key-value pair ftrace.tracer=functionds


Which do you like better, 1) or 2) ?

Thanks,
Taeung

[PATCH] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

# cat ~/.perfconfig
[ftrace]
tracer = function

# perf ftrace usleep 123456 | head -10
  <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
  <...>-14450 [002]  10089.284232: finish_wait <-pipe_wait
  <...>-14450 [002]  10089.284232: mutex_lock <-pipe_wait
  <...>-14450 [002]  10089.284232: _cond_resched <-mutex_lock
  <...>-14450 [002]  10089.284233: generic_pipe_buf_confirm <-pipe_read

Cc: Jiri Olsa 
Cc: Namhyung Kim 
Signed-off-by: Taeung Song 
---
 tools/perf/builtin-ftrace.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 41d..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"
 
 
 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,26 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int 
argc, const char **argv)
return done ? 0 : -1;
 }
 
+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+   struct perf_ftrace *ftrace = cb;
+
+   if (!strcmp(var, "ftrace.tracer")) {
+   if (!strcmp(value, "function_graph"))
+   ftrace->tracer = DEFAULT_TRACER;
+   else if (!strcmp(value, "function"))
+   ftrace->tracer = "function";
+   else {
+   pr_err("Please select function_graph(default)"
+  "or function to use tracer.\n");
+   return -1;
+   }
+   return 0;
+   }
+
+   return 0;
+}
+
 int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
int ret;
@@ -218,6 +239,10 @@ int cmd_ftrace(int argc, const char **argv, const char 
*prefix __maybe_unused)
OPT_END()
};
 
+   ret = perf_config(perf_ftrace_config, );
+   if (ret < 0)
+   return -1;
+
argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
if (!argc)
-- 
2.7.4

[PATCH] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

# cat ~/.perfconfig
[ftrace]
tracer = function

# perf ftrace usleep 123456 | head -10
  <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
  <...>-14450 [002]  10089.284232: finish_wait <-pipe_wait
  <...>-14450 [002]  10089.284232: mutex_lock <-pipe_wait
  <...>-14450 [002]  10089.284232: _cond_resched <-mutex_lock
  <...>-14450 [002]  10089.284233: generic_pipe_buf_confirm <-pipe_read

Cc: Jiri Olsa 
Cc: Namhyung Kim 
Signed-off-by: Taeung Song 
---
 tools/perf/builtin-ftrace.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 41d..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"
 
 
 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,26 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int 
argc, const char **argv)
return done ? 0 : -1;
 }
 
+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+   struct perf_ftrace *ftrace = cb;
+
+   if (!strcmp(var, "ftrace.tracer")) {
+   if (!strcmp(value, "function_graph"))
+   ftrace->tracer = DEFAULT_TRACER;
+   else if (!strcmp(value, "function"))
+   ftrace->tracer = "function";
+   else {
+   pr_err("Please select function_graph(default)"
+  "or function to use tracer.\n");
+   return -1;
+   }
+   return 0;
+   }
+
+   return 0;
+}
+
 int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
int ret;
@@ -218,6 +239,10 @@ int cmd_ftrace(int argc, const char **argv, const char 
*prefix __maybe_unused)
OPT_END()
};
 
+   ret = perf_config(perf_ftrace_config, );
+   if (ret < 0)
+   return -1;
+
argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
if (!argc)
-- 
2.7.4

GIT PULL] Thermal management updates for v4.10-rc6

[Zhang Rui]

Hi, Linus,

Please pull from
  git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git for-rc

to receive the latest Thermal Management updates for v4.10-rc6 with
top-most commit 3feb479cea37fc623cf4e705631b2e679cbfbd7a:

  Revert "thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()" (2017-01-25 09:51:08 +0800)

on top of commit 883af14e67e8b8702b5560aa64c888c0cd0bd66c:

  Merge branch 'akpm' (patches from Andrew) (2017-01-24 16:54:39 -0800)

Specifics:

commit 7611fb68062f ("thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()"), which is introduced in 4.10-rc5,
uses new hwmon API. But this breaks some soc thermal driver because the
new hwmon API has a strict rule for the hwmon device name.
Revert the offending commit as a quick solution for 4.10.

thanks,
rui


Fabio Estevam (1):
  Revert "thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()"

 drivers/thermal/thermal_hwmon.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

GIT PULL] Thermal management updates for v4.10-rc6

[Zhang Rui]

Hi, Linus,

Please pull from
  git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git for-rc

to receive the latest Thermal Management updates for v4.10-rc6 with
top-most commit 3feb479cea37fc623cf4e705631b2e679cbfbd7a:

  Revert "thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()" (2017-01-25 09:51:08 +0800)

on top of commit 883af14e67e8b8702b5560aa64c888c0cd0bd66c:

  Merge branch 'akpm' (patches from Andrew) (2017-01-24 16:54:39 -0800)

Specifics:

commit 7611fb68062f ("thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()"), which is introduced in 4.10-rc5,
uses new hwmon API. But this breaks some soc thermal driver because the
new hwmon API has a strict rule for the hwmon device name.
Revert the offending commit as a quick solution for 4.10.

thanks,
rui


Fabio Estevam (1):
  Revert "thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()"

 drivers/thermal/thermal_hwmon.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit addressing encodings

[Ricardo Neri]

On Thu, 2017-01-26 at 09:05 -0800, Andy Lutomirski wrote:
> On Wed, Jan 25, 2017 at 9:50 PM, Ricardo Neri
>  wrote:
> > On Wed, 2017-01-25 at 13:58 -0800, Andy Lutomirski wrote:
> >> On Wed, Jan 25, 2017 at 12:23 PM, Ricardo Neri
> >>  wrote:
> >> > Tasks running in virtual-8086 mode will use 16-bit addressing form
> >> > encodings as described in the Intel 64 and IA-32 Architecture Software
> >> > Developer's Manual Volume 2A Section 2.1.5. 16-bit addressing encodings
> >> > differ in several ways from the 32-bit/64-bit addressing form encodings:
> >> > the r/m part of the ModRM byte points to different registers and, in some
> >> > cases, addresses can be indicated by the addition of the value of two
> >> > registers. Also, there is no support for SiB bytes. Thus, a separate
> >> > function is needed to parse this form of addressing.
> >> >
> >> > Furthermore, virtual-8086 mode tasks will use real-mode addressing. This
> >> > implies that the segment selectors do not point to a segment descriptor
> >> > but are used to compute logical addresses. Hence, there is a need to
> >> > add support to compute addresses using the segment selectors. If segment-
> >> > override prefixes are present in the instructions, they take precedence.
> >> >
> >> > Lastly, it is important to note that when a tasks is running in virtual-
> >> > 8086 mode and an interrupt/exception occurs, the CPU pushes to the stack
> >> > the segment selectors for ds, es, fs and gs. These are accesible via the
> >> > struct kernel_vm86_regs rather than pt_regs.
> >> >
> >> > Code for 16-bit addressing encodings is likely to be used only by 
> >> > virtual-
> >> > 8086 mode tasks. Thus, this code is wrapped to be built only if the
> >> > option CONFIG_VM86 is selected.
> >>
> >> That's not true.  It's used in 16-bit protected mode, too.  And there
> >> are (ugh!) six possibilities:
> >
> > Thanks for the clarification. I will enable the decoding of addresses
> > for 16-bit as well... and test the emulation code.
> >>
> >>  - Normal 32-bit protected mode.  This should already work.
> >>  - Normal 64-bit protected mode.  This should also already work.  (I
> >> forget whether a 16-bit SS is either illegal or has no effect in this
> >> case.)
> >
> > For these two cases I am just taking the effective address that the user
> > space application provides, given that the segment selectors were set
> > beforehand (and with a base of 0).
> 
> What do you mean by the base being zero?  User code can set a nonzero
> DS base if it wants.  In 64-bit mode (user_64bit_mode(regs)), the base
> is ignored unless there's an fs or gs prefix, and in 32-bit mode the
> base is never ignored.

Yes, I take this back. At the time of writing I was thinking about the
__USER_CS and _USER_DS descriptors. You ar right, the base is not
ignored.
> 
> >
> >>  - Virtual 8086 mode
> >
> > In this case I calculate the linear address as:
> >  (segment_select << 4) + effective address.
> >
> >>  - Normal 16-bit protected mode, used by DOSEMU and Wine.  (16-bit CS,
> >> 16-bit address segment)
> >>  - 16-bit CS, 32-bit address segment.  IIRC this might be used by some
> >> 32-bit DOS programs to call BIOS.
> >>  - 32-bit CS, 16-bit address segment.  I don't know whether anything uses 
> >> this.
> >
> > In all these protected modes, are you referring to the size in bits of
> > the base address of in the descriptor selected in the CS register? In
> > such a case I would need to get the base address and add it to the
> > effective address given in the operands of the instructions, right?
> 
> No, I'm referring to the D/B bit.  I'm a bit fuzzy on exactly how the
> instruction encoding works, but I think that 16-bit x86 code is
> encoded just like real mode code except that the selectors are used
> for real.

I see. I have the logic to differentiate between 16-bit and 32-bit
addresses. What I am missing is code to look at the value of this bit
and any potential operand overrides. I will work on adding this.
> 
> >> size, but I suspect you'll need to handle 16-bit CS.
> >
> > Unless I am missing what is special with the 16-bit base address, I only
> > would need to add that base address to whatever effective address (aka,
> > offset) is encoded in the ModRM and displacement bytes.
> 
> Exactly.  (And make sure the instruction decoder can decode 16-bit
> instructions correctly.)

Will do. I have tested my 16-bit decoding extensively. I think it will
work.


Thanks and BR,
Ricardo

Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit addressing encodings

[Ricardo Neri]

On Thu, 2017-01-26 at 09:05 -0800, Andy Lutomirski wrote:
> On Wed, Jan 25, 2017 at 9:50 PM, Ricardo Neri
>  wrote:
> > On Wed, 2017-01-25 at 13:58 -0800, Andy Lutomirski wrote:
> >> On Wed, Jan 25, 2017 at 12:23 PM, Ricardo Neri
> >>  wrote:
> >> > Tasks running in virtual-8086 mode will use 16-bit addressing form
> >> > encodings as described in the Intel 64 and IA-32 Architecture Software
> >> > Developer's Manual Volume 2A Section 2.1.5. 16-bit addressing encodings
> >> > differ in several ways from the 32-bit/64-bit addressing form encodings:
> >> > the r/m part of the ModRM byte points to different registers and, in some
> >> > cases, addresses can be indicated by the addition of the value of two
> >> > registers. Also, there is no support for SiB bytes. Thus, a separate
> >> > function is needed to parse this form of addressing.
> >> >
> >> > Furthermore, virtual-8086 mode tasks will use real-mode addressing. This
> >> > implies that the segment selectors do not point to a segment descriptor
> >> > but are used to compute logical addresses. Hence, there is a need to
> >> > add support to compute addresses using the segment selectors. If segment-
> >> > override prefixes are present in the instructions, they take precedence.
> >> >
> >> > Lastly, it is important to note that when a tasks is running in virtual-
> >> > 8086 mode and an interrupt/exception occurs, the CPU pushes to the stack
> >> > the segment selectors for ds, es, fs and gs. These are accesible via the
> >> > struct kernel_vm86_regs rather than pt_regs.
> >> >
> >> > Code for 16-bit addressing encodings is likely to be used only by 
> >> > virtual-
> >> > 8086 mode tasks. Thus, this code is wrapped to be built only if the
> >> > option CONFIG_VM86 is selected.
> >>
> >> That's not true.  It's used in 16-bit protected mode, too.  And there
> >> are (ugh!) six possibilities:
> >
> > Thanks for the clarification. I will enable the decoding of addresses
> > for 16-bit as well... and test the emulation code.
> >>
> >>  - Normal 32-bit protected mode.  This should already work.
> >>  - Normal 64-bit protected mode.  This should also already work.  (I
> >> forget whether a 16-bit SS is either illegal or has no effect in this
> >> case.)
> >
> > For these two cases I am just taking the effective address that the user
> > space application provides, given that the segment selectors were set
> > beforehand (and with a base of 0).
> 
> What do you mean by the base being zero?  User code can set a nonzero
> DS base if it wants.  In 64-bit mode (user_64bit_mode(regs)), the base
> is ignored unless there's an fs or gs prefix, and in 32-bit mode the
> base is never ignored.

Yes, I take this back. At the time of writing I was thinking about the
__USER_CS and _USER_DS descriptors. You ar right, the base is not
ignored.
> 
> >
> >>  - Virtual 8086 mode
> >
> > In this case I calculate the linear address as:
> >  (segment_select << 4) + effective address.
> >
> >>  - Normal 16-bit protected mode, used by DOSEMU and Wine.  (16-bit CS,
> >> 16-bit address segment)
> >>  - 16-bit CS, 32-bit address segment.  IIRC this might be used by some
> >> 32-bit DOS programs to call BIOS.
> >>  - 32-bit CS, 16-bit address segment.  I don't know whether anything uses 
> >> this.
> >
> > In all these protected modes, are you referring to the size in bits of
> > the base address of in the descriptor selected in the CS register? In
> > such a case I would need to get the base address and add it to the
> > effective address given in the operands of the instructions, right?
> 
> No, I'm referring to the D/B bit.  I'm a bit fuzzy on exactly how the
> instruction encoding works, but I think that 16-bit x86 code is
> encoded just like real mode code except that the selectors are used
> for real.

I see. I have the logic to differentiate between 16-bit and 32-bit
addresses. What I am missing is code to look at the value of this bit
and any potential operand overrides. I will work on adding this.
> 
> >> size, but I suspect you'll need to handle 16-bit CS.
> >
> > Unless I am missing what is special with the 16-bit base address, I only
> > would need to add that base address to whatever effective address (aka,
> > offset) is encoded in the ModRM and displacement bytes.
> 
> Exactly.  (And make sure the instruction decoder can decode 16-bit
> instructions correctly.)

Will do. I have tested my 16-bit decoding extensively. I think it will
work.


Thanks and BR,
Ricardo

Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

[Zhang Rui]

On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote:
> On 01/26/2017 05:37 PM, Zhang Rui wrote:
> > 
> > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote:
> > > 
> > > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:
> > > > 
> > > > 
> > > > Hi!
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Right.
> > > > > > > > > 
> > > > > > > > > Before reverting, can you please try if this patch
> > > > > > > > > works
> > > > > > > > > or not?
> > > > > > > > Not really. Revert now. Sorry.
> > > > > > > > 
> > > > > > > > Are you sure? This does not look equivalent to me at
> > > > > > > > all.
> > > > > > > > 
> > > > > > > > "name" file handling moved from drivers to the core,
> > > > > > > > which
> > > > > > > > added some
> > > > > > > > crazy checks what name can contain. Even if this
> > > > > > > > "works",
> > > > > > > > what is the
> > > > > > > > expected effect on the "name" file?
> > > > > > > > 
> > > > > > > The hwmon name attribute must not include '-', as
> > > > > > > documented
> > > > > > > in
> > > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that
> > > > > > > 'crazy' ?
> > > > > > > Maybe in your world, but not in mine.
> > > > > > Well, lets revert the patch and then we can discuss what to
> > > > > > do
> > > > > > with
> > > > > > the "name" problem.
> > > > Ok, so the patch is on the way in. What to do next?
> > > > 
> > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name
> > > > bq27200-0
> > > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name
> > > > rx51-battery
> > > > 
> > > > > 
> > > > > 
> > > > > To provide some detail: libsensors gets just as confused with
> > > > > wildcards
> > > > > and whitespace/newline as it does with '-' in the reported
> > > > > name,
> > > > > which
> > > > > is why those are blocked by the new API.
> > > > Ok... Question is "does someone actually use hwmon*/name on
> > > > N900"?
> > > > If
> > > > so, we can't change it, but it is well possible that noone is.
> > > IIRC hwmon is used on Nokia N900.
> > > 
> > > But I have not seen hwmon devices for bq27200 and rx51-battery
> > > yet.
> > > Those are power supply driver and auto-exporting them also via
> > > hwmon
> > > is
> > > something new, right? If yes, then we can use any name for those
> > > new
> > > hwmon devices as they cannot break userspace... as there is no
> > > userspace
> > > application for them.
> > > 
> > If this is the case, you'd better set
> > (struct thermal_zone_params)->no_hwmon when registering the thermal
> > zone device, in which case, the hwmon device will not be created.
> > 
> > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not
> > create
> > hwmon I/F by default, and see if there is anyone using it. If yes,
> > we
> > can set the flag in soc thermal driver, explicitly, at meantime, a
> > hwmon compatible name is required.
> > 
> > But one foreseeable result is that we may get bug reports from end
> > user
> > that some sensors (acpitz, etc) are gone in 'sensors' output. And
> > TBH,
> > I'm not quite sure if this can be counted as a regression or not.
> > 
> That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by
> the ABI Police, but taking the entire device away is ok.
> 
No. IMO, it depends on if the interface is used or not.
If hwmon I/F is used, we can not take it away, nor change its name.
If thermal zone I/F is used, we can not change it's 'type' name to be
compatible with new hwmon API.

> Anyway, sounds good to me. No one will use something that isn't
> there,
> and no one will realize that it could have been there, so I don't
> expect
> anyone to complain.

Yes, I agree.

thanks,
rui

Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

[Zhang Rui]

On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote:
> On 01/26/2017 05:37 PM, Zhang Rui wrote:
> > 
> > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote:
> > > 
> > > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:
> > > > 
> > > > 
> > > > Hi!
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Right.
> > > > > > > > > 
> > > > > > > > > Before reverting, can you please try if this patch
> > > > > > > > > works
> > > > > > > > > or not?
> > > > > > > > Not really. Revert now. Sorry.
> > > > > > > > 
> > > > > > > > Are you sure? This does not look equivalent to me at
> > > > > > > > all.
> > > > > > > > 
> > > > > > > > "name" file handling moved from drivers to the core,
> > > > > > > > which
> > > > > > > > added some
> > > > > > > > crazy checks what name can contain. Even if this
> > > > > > > > "works",
> > > > > > > > what is the
> > > > > > > > expected effect on the "name" file?
> > > > > > > > 
> > > > > > > The hwmon name attribute must not include '-', as
> > > > > > > documented
> > > > > > > in
> > > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that
> > > > > > > 'crazy' ?
> > > > > > > Maybe in your world, but not in mine.
> > > > > > Well, lets revert the patch and then we can discuss what to
> > > > > > do
> > > > > > with
> > > > > > the "name" problem.
> > > > Ok, so the patch is on the way in. What to do next?
> > > > 
> > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name
> > > > bq27200-0
> > > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name
> > > > rx51-battery
> > > > 
> > > > > 
> > > > > 
> > > > > To provide some detail: libsensors gets just as confused with
> > > > > wildcards
> > > > > and whitespace/newline as it does with '-' in the reported
> > > > > name,
> > > > > which
> > > > > is why those are blocked by the new API.
> > > > Ok... Question is "does someone actually use hwmon*/name on
> > > > N900"?
> > > > If
> > > > so, we can't change it, but it is well possible that noone is.
> > > IIRC hwmon is used on Nokia N900.
> > > 
> > > But I have not seen hwmon devices for bq27200 and rx51-battery
> > > yet.
> > > Those are power supply driver and auto-exporting them also via
> > > hwmon
> > > is
> > > something new, right? If yes, then we can use any name for those
> > > new
> > > hwmon devices as they cannot break userspace... as there is no
> > > userspace
> > > application for them.
> > > 
> > If this is the case, you'd better set
> > (struct thermal_zone_params)->no_hwmon when registering the thermal
> > zone device, in which case, the hwmon device will not be created.
> > 
> > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not
> > create
> > hwmon I/F by default, and see if there is anyone using it. If yes,
> > we
> > can set the flag in soc thermal driver, explicitly, at meantime, a
> > hwmon compatible name is required.
> > 
> > But one foreseeable result is that we may get bug reports from end
> > user
> > that some sensors (acpitz, etc) are gone in 'sensors' output. And
> > TBH,
> > I'm not quite sure if this can be counted as a regression or not.
> > 
> That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by
> the ABI Police, but taking the entire device away is ok.
> 
No. IMO, it depends on if the interface is used or not.
If hwmon I/F is used, we can not take it away, nor change its name.
If thermal zone I/F is used, we can not change it's 'type' name to be
compatible with new hwmon API.

> Anyway, sounds good to me. No one will use something that isn't
> there,
> and no one will realize that it could have been there, so I don't
> expect
> anyone to complain.

Yes, I agree.

thanks,
rui

Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit addressing encodings (fwd)

[Ricardo Neri]

On Wed, 2017-01-25 at 22:49 +0100, Julia Lawall wrote:
> Check the type of seg on line 267.

Ah! I missed that. It makes sense. I will fix this issue.

Thanks and BR,
Ricardo
> 
> julia
> 
> -- Forwarded message --
> Date: Thu, 26 Jan 2017 05:24:40 +0800
> From: kbuild test robot <fengguang...@intel.com>
> To: kbu...@01.org
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Subject: Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit
> addressing encodings
> 
> In-Reply-To: <20170125202353.101059-6-ricardo.neri-calde...@linux.intel.com>
> 
> Hi Ricardo,
> 
> [auto build test WARNING on tip/auto-latest]
> [also build test WARNING on v4.10-rc5 next-20170125]
> [cannot apply to tip/x86/core]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-Enable-User-Mode-Instruction-Prevention/20170126-043345
> :: branch date: 51 minutes ago
> :: commit date: 51 minutes ago
> 
> >> arch/x86/lib/insn-kernel.c:267:6-9: WARNING: Unsigned expression compared 
> >> with zero: seg < 0
> 
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 574de0de238ef30c816985006f02f7a1dbba92aa
> vim +267 arch/x86/lib/insn-kernel.c
> 
> 574de0de Ricardo Neri 2017-01-25  251 addr = (seg << 4) + 
> regs_get_register(regs, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25  252 } else {
> 574de0de Ricardo Neri 2017-01-25  253 ret = 
> get_reg_offset_16(insn, regs, _offset1,
> 574de0de Ricardo Neri 2017-01-25  254 
> _offset2);
> 574de0de Ricardo Neri 2017-01-25  255 if (ret < 0)
> 574de0de Ricardo Neri 2017-01-25  256 goto out_err;
> 574de0de Ricardo Neri 2017-01-25  257 /*
> 574de0de Ricardo Neri 2017-01-25  258  * Don't fail on 
> invalid offset values. They might be invalid
> 574de0de Ricardo Neri 2017-01-25  259  * because they are not 
> supported. Instead, use them in the
> 574de0de Ricardo Neri 2017-01-25  260  * calculation only if 
> they contain a valid value.
> 574de0de Ricardo Neri 2017-01-25  261  */
> 574de0de Ricardo Neri 2017-01-25  262 if (addr_offset1 >= 0)
> 574de0de Ricardo Neri 2017-01-25  263 addr1 = 
> regs_get_register(regs, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25  264 if (addr_offset2 >= 0)
> 574de0de Ricardo Neri 2017-01-25  265 addr2 = 
> regs_get_register(regs, addr_offset2);
> 574de0de Ricardo Neri 2017-01-25  266 seg = 
> __get_segment_selector_16(regs, insn, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25 @267 if (seg < 0)
> 574de0de Ricardo Neri 2017-01-25  268 goto out_err;
> 574de0de Ricardo Neri 2017-01-25  269 addr =  (seg << 4) + 
> addr1 + addr2;
> 574de0de Ricardo Neri 2017-01-25  270 }
> 574de0de Ricardo Neri 2017-01-25  271 addr += 
> insn->displacement.value;
> 574de0de Ricardo Neri 2017-01-25  272
> 574de0de Ricardo Neri 2017-01-25  273 return (void __user *)addr;
> 574de0de Ricardo Neri 2017-01-25  274  out_err:
> 574de0de Ricardo Neri 2017-01-25  275 return (void __user *)-1;
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit addressing encodings (fwd)

[Ricardo Neri]

On Wed, 2017-01-25 at 22:49 +0100, Julia Lawall wrote:
> Check the type of seg on line 267.

Ah! I missed that. It makes sense. I will fix this issue.

Thanks and BR,
Ricardo
> 
> julia
> 
> -- Forwarded message --
> Date: Thu, 26 Jan 2017 05:24:40 +0800
> From: kbuild test robot 
> To: kbu...@01.org
> Cc: Julia Lawall 
> Subject: Re: [v3 PATCH 05/10] x86/insn-kernel: Add support to resolve 16-bit
> addressing encodings
> 
> In-Reply-To: <20170125202353.101059-6-ricardo.neri-calde...@linux.intel.com>
> 
> Hi Ricardo,
> 
> [auto build test WARNING on tip/auto-latest]
> [also build test WARNING on v4.10-rc5 next-20170125]
> [cannot apply to tip/x86/core]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-Enable-User-Mode-Instruction-Prevention/20170126-043345
> :: branch date: 51 minutes ago
> :: commit date: 51 minutes ago
> 
> >> arch/x86/lib/insn-kernel.c:267:6-9: WARNING: Unsigned expression compared 
> >> with zero: seg < 0
> 
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 574de0de238ef30c816985006f02f7a1dbba92aa
> vim +267 arch/x86/lib/insn-kernel.c
> 
> 574de0de Ricardo Neri 2017-01-25  251 addr = (seg << 4) + 
> regs_get_register(regs, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25  252 } else {
> 574de0de Ricardo Neri 2017-01-25  253 ret = 
> get_reg_offset_16(insn, regs, _offset1,
> 574de0de Ricardo Neri 2017-01-25  254 
> _offset2);
> 574de0de Ricardo Neri 2017-01-25  255 if (ret < 0)
> 574de0de Ricardo Neri 2017-01-25  256 goto out_err;
> 574de0de Ricardo Neri 2017-01-25  257 /*
> 574de0de Ricardo Neri 2017-01-25  258  * Don't fail on 
> invalid offset values. They might be invalid
> 574de0de Ricardo Neri 2017-01-25  259  * because they are not 
> supported. Instead, use them in the
> 574de0de Ricardo Neri 2017-01-25  260  * calculation only if 
> they contain a valid value.
> 574de0de Ricardo Neri 2017-01-25  261  */
> 574de0de Ricardo Neri 2017-01-25  262 if (addr_offset1 >= 0)
> 574de0de Ricardo Neri 2017-01-25  263 addr1 = 
> regs_get_register(regs, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25  264 if (addr_offset2 >= 0)
> 574de0de Ricardo Neri 2017-01-25  265 addr2 = 
> regs_get_register(regs, addr_offset2);
> 574de0de Ricardo Neri 2017-01-25  266 seg = 
> __get_segment_selector_16(regs, insn, addr_offset1);
> 574de0de Ricardo Neri 2017-01-25 @267 if (seg < 0)
> 574de0de Ricardo Neri 2017-01-25  268 goto out_err;
> 574de0de Ricardo Neri 2017-01-25  269 addr =  (seg << 4) + 
> addr1 + addr2;
> 574de0de Ricardo Neri 2017-01-25  270 }
> 574de0de Ricardo Neri 2017-01-25  271 addr += 
> insn->displacement.value;
> 574de0de Ricardo Neri 2017-01-25  272
> 574de0de Ricardo Neri 2017-01-25  273 return (void __user *)addr;
> 574de0de Ricardo Neri 2017-01-25  274  out_err:
> 574de0de Ricardo Neri 2017-01-25  275 return (void __user *)-1;
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

[PATCHv5 4/5] arm: mvebu: Add device tree for 98DX3236 SoCs

[Chris Packham]

The Marvell 98DX3236, 98DX3336, 98DX4521 and variants are switch ASICs
with integrated CPUs. They are similar to the Armada XP SoCs but have
different I/O interfaces.

Signed-off-by: Chris Packham 
Acked-by: Rob Herring 
---

Notes:
Changes in v2:
- Update devicetree binding documentation to reflect that 98DX3336 and
  984251 are supersets of 98DX3236.
- disable crypto block
- disable sdio for 98DX3236, enable for 98DX4251
Changes in v3:
- fix typo 4521 -> 4251
- document prestera bindings
- rework corediv-clock binding
- add label to packet processor node
- add new compatible string for DFX server
Changes in v4:
- Collect ack from Rob
Changes in v5:
- Fixup license text. Add labels to nodes.

 .../devicetree/bindings/arm/marvell/98dx3236.txt   |  23 ++
 .../devicetree/bindings/net/marvell,prestera.txt   |  50 
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi  | 254 +
 arch/arm/boot/dts/armada-xp-98dx3336.dtsi  |  76 ++
 arch/arm/boot/dts/armada-xp-98dx4251.dtsi  |  90 
 5 files changed, 493 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
 create mode 100644 Documentation/devicetree/bindings/net/marvell,prestera.txt
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3236.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3336.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx4251.dtsi

diff --git a/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt 
b/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
new file mode 100644
index ..64e8c73fc5ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
@@ -0,0 +1,23 @@
+Marvell 98DX3236, 98DX3336 and 98DX4251 Platforms Device Tree Bindings
+--
+
+Boards with a SoC of the Marvell 98DX3236, 98DX3336 and 98DX4251 families
+shall have the following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx3236"
+
+In addition, boards using the Marvell 98DX3336 SoC shall have the
+following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx3336"
+
+In addition, boards using the Marvell 98DX4251 SoC shall have the
+following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx4251"
diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.txt 
b/Documentation/devicetree/bindings/net/marvell,prestera.txt
new file mode 100644
index ..5fbab29718e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,prestera.txt
@@ -0,0 +1,50 @@
+Marvell Prestera Switch Chip bindings
+-
+
+Required properties:
+- compatible: one of the following
+   "marvell,prestera-98dx3236",
+   "marvell,prestera-98dx3336",
+   "marvell,prestera-98dx4251",
+- reg: address and length of the register set for the device.
+- interrupts: interrupt for the device
+
+Optional properties:
+- dfx: phandle reference to the "DFX Server" node
+
+Example:
+
+switch {
+   compatible = "simple-bus";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 MBUS_ID(0x03, 0x00) 0 0x10>;
+
+   packet-processor@0 {
+   compatible = "marvell,prestera-98dx3236";
+   reg = <0 0x400>;
+   interrupts = <33>, <34>, <35>;
+   dfx = <>;
+   };
+};
+
+DFX Server bindings
+---
+
+Required properties:
+- compatible: must be "marvell,dfx-server"
+- reg: address and length of the register set for the device.
+
+Example:
+
+dfx-registers {
+   compatible = "simple-bus";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 MBUS_ID(0x08, 0x00) 0 0x10>;
+
+   dfx: dfx@0 {
+   compatible = "marvell,dfx-server";
+   reg = <0 0x10>;
+   };
+};
diff --git a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi 
b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
new file mode 100644
index ..9461128fae24
--- /dev/null
+++ b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
@@ -0,0 +1,254 @@
+/*
+ * Device Tree Include file for Marvell 98dx3236 family SoC
+ *
+ * Copyright (C) 2016 Allied Telesis Labs
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be 

[PATCHv5 0/5] Support for Marvell switches with integrated CPUs

[Chris Packham]

The 98DX3236, 98DX3336 and 98DX4251 are a set of switch ASICs with
integrated CPUs. They CPU block is common within these product lines and
(as far as I can tell/have been told) is based on the Armada XP. There
are a few differences due to the fact they have to squeeze the CPU into
the same package as the switch.

I've rebased this series against linux-pinctrl/devel to get access to
mvebu_mmio_mpp_ctrl. Everything else still applies cleanly to v4.10.0-rc5.

Chris Packham (4):
  clk: mvebu: support for 98DX3236 SoC
Changes in v2:
- Update devicetree binding documentation for new compatible string
Changes in v3:
- Add 98dx3236 support to mvebu/clk-corediv.c rather than creating a new
  driver.
- Document mv98dx3236-corediv-clock binding
Changes in v4:
- None
Changes in v5:
- Collect ack from Rob
- Remove explicit initialisation of fields to 0 in mv98dx3236_coreclks
- Register dummy clock provider for marvell,mv98dx3236-cpu-clock
  arm: mvebu: support for SMP on 98DX3336 SoC
Changes in v2:
- Document new enable-method value
- Correct some references from 98DX4521 to 98DX3236
Changes in v3:
- Simplify mv98dx3236_resume_init by using of_io_request_and_map()
Changes in v4:
- integrate changes into platsmp.c instead of new init call
- avoid duplicated code.
- fix error return
- Collect ack from Rob
Changes in v5:
- Remove useless casts (thanks to Stephen Boyd)
  arm: mvebu: Add device tree for 98DX3236 SoCs
Changes in v2:
- Update devicetree binding documentation to reflect that 98DX3336 and
  984251 are supersets of 98DX3236.
- disable crypto block
- disable sdio for 98DX3236, enable for 98DX4251
Changes in v3:
- fix typo 4521 -> 4251
- document prestera bindings
- rework corediv-clock binding
- add label to packet processor node
- add new compatible string for DFX server
Changes in v4:
- Collect ack from Rob
Changes in v5:
- Fixup license text. Add labels to nodes.
  arm: mvebu: Add device tree for db-dxbc2 and db-xc3-24g4xg boards
Changes in v5:
- update license text
- use node labels

Kalyan Kinthada (1):
  pinctrl: mvebu: pinctrl driver for 98DX3236 SoC
Changes in v2:
- include sdio support for the 98DX4251
Changes in v3:
- None
Changes in v4:
- Correct some discrepencies between binding and driver.
- Collect acks from Rob and Sebastian
Changes in v5:
- Update bindings to reflect "gpo" pins
- Use mvebu_mmio_mpp_ctrl instead of armada_xp_mpp_ctrl (note this is 
reliant
  on changes queued in linux-pinctrl)

 Documentation/devicetree/bindings/arm/cpus.txt |   1 +
 .../bindings/arm/marvell/98dx3236-resume-ctrl.txt  |  16 ++
 .../devicetree/bindings/arm/marvell/98dx3236.txt   |  23 ++
 .../bindings/clock/mvebu-corediv-clock.txt |   1 +
 .../devicetree/bindings/clock/mvebu-cpu-clock.txt  |   1 +
 .../devicetree/bindings/net/marvell,prestera.txt   |  50 
 .../pinctrl/marvell,armada-98dx3236-pinctrl.txt|  46 
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi  | 254 +
 arch/arm/boot/dts/armada-xp-98dx3336.dtsi  |  76 ++
 arch/arm/boot/dts/armada-xp-98dx4251.dtsi  |  90 
 arch/arm/boot/dts/db-dxbc2.dts | 151 
 arch/arm/boot/dts/db-xc3-24g4xg.dts| 142 
 arch/arm/mach-mvebu/platsmp.c  |  86 +++
 drivers/clk/mvebu/armada-xp.c  |  39 
 drivers/clk/mvebu/clk-corediv.c|  23 ++
 drivers/clk/mvebu/clk-cpu.c|   8 +
 drivers/pinctrl/mvebu/pinctrl-armada-xp.c  | 156 +
 17 files changed, 1163 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/marvell/98dx3236-resume-ctrl.txt
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
 create mode 100644 Documentation/devicetree/bindings/net/marvell,prestera.txt
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3236.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3336.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx4251.dtsi
 create mode 100644 arch/arm/boot/dts/db-dxbc2.dts
 create mode 100644 arch/arm/boot/dts/db-xc3-24g4xg.dts

interdiff to v4:

 diff --git a/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt 
b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
 index 520562a7dc2a..c7b4e3a6b2c6 100644
@@ -85,7 +86,7 @@
  by address and length of the PMU DFS registers
  - #clock-cells : should be set to 1.
 diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
-index b3094315a3c0..0413bf8284e0 100644
+index b3094315a3c0..890a863ae0d0 100644
 --- a/drivers/clk/mvebu/armada-xp.c
 +++ b/drivers/clk/mvebu/armada-xp.c

[PATCHv5 4/5] arm: mvebu: Add device tree for 98DX3236 SoCs

[Chris Packham]

The Marvell 98DX3236, 98DX3336, 98DX4521 and variants are switch ASICs
with integrated CPUs. They are similar to the Armada XP SoCs but have
different I/O interfaces.

Signed-off-by: Chris Packham 
Acked-by: Rob Herring 
---

Notes:
Changes in v2:
- Update devicetree binding documentation to reflect that 98DX3336 and
  984251 are supersets of 98DX3236.
- disable crypto block
- disable sdio for 98DX3236, enable for 98DX4251
Changes in v3:
- fix typo 4521 -> 4251
- document prestera bindings
- rework corediv-clock binding
- add label to packet processor node
- add new compatible string for DFX server
Changes in v4:
- Collect ack from Rob
Changes in v5:
- Fixup license text. Add labels to nodes.

 .../devicetree/bindings/arm/marvell/98dx3236.txt   |  23 ++
 .../devicetree/bindings/net/marvell,prestera.txt   |  50 
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi  | 254 +
 arch/arm/boot/dts/armada-xp-98dx3336.dtsi  |  76 ++
 arch/arm/boot/dts/armada-xp-98dx4251.dtsi  |  90 
 5 files changed, 493 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
 create mode 100644 Documentation/devicetree/bindings/net/marvell,prestera.txt
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3236.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3336.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx4251.dtsi

diff --git a/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt 
b/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
new file mode 100644
index ..64e8c73fc5ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
@@ -0,0 +1,23 @@
+Marvell 98DX3236, 98DX3336 and 98DX4251 Platforms Device Tree Bindings
+--
+
+Boards with a SoC of the Marvell 98DX3236, 98DX3336 and 98DX4251 families
+shall have the following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx3236"
+
+In addition, boards using the Marvell 98DX3336 SoC shall have the
+following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx3336"
+
+In addition, boards using the Marvell 98DX4251 SoC shall have the
+following property:
+
+Required root node property:
+
+compatible: must contain "marvell,armadaxp-98dx4251"
diff --git a/Documentation/devicetree/bindings/net/marvell,prestera.txt 
b/Documentation/devicetree/bindings/net/marvell,prestera.txt
new file mode 100644
index ..5fbab29718e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,prestera.txt
@@ -0,0 +1,50 @@
+Marvell Prestera Switch Chip bindings
+-
+
+Required properties:
+- compatible: one of the following
+   "marvell,prestera-98dx3236",
+   "marvell,prestera-98dx3336",
+   "marvell,prestera-98dx4251",
+- reg: address and length of the register set for the device.
+- interrupts: interrupt for the device
+
+Optional properties:
+- dfx: phandle reference to the "DFX Server" node
+
+Example:
+
+switch {
+   compatible = "simple-bus";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 MBUS_ID(0x03, 0x00) 0 0x10>;
+
+   packet-processor@0 {
+   compatible = "marvell,prestera-98dx3236";
+   reg = <0 0x400>;
+   interrupts = <33>, <34>, <35>;
+   dfx = <>;
+   };
+};
+
+DFX Server bindings
+---
+
+Required properties:
+- compatible: must be "marvell,dfx-server"
+- reg: address and length of the register set for the device.
+
+Example:
+
+dfx-registers {
+   compatible = "simple-bus";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges = <0 MBUS_ID(0x08, 0x00) 0 0x10>;
+
+   dfx: dfx@0 {
+   compatible = "marvell,dfx-server";
+   reg = <0 0x10>;
+   };
+};
diff --git a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi 
b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
new file mode 100644
index ..9461128fae24
--- /dev/null
+++ b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
@@ -0,0 +1,254 @@
+/*
+ * Device Tree Include file for Marvell 98dx3236 family SoC
+ *
+ * Copyright (C) 2016 Allied Telesis Labs
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even 

[PATCHv5 0/5] Support for Marvell switches with integrated CPUs

[Chris Packham]

The 98DX3236, 98DX3336 and 98DX4251 are a set of switch ASICs with
integrated CPUs. They CPU block is common within these product lines and
(as far as I can tell/have been told) is based on the Armada XP. There
are a few differences due to the fact they have to squeeze the CPU into
the same package as the switch.

I've rebased this series against linux-pinctrl/devel to get access to
mvebu_mmio_mpp_ctrl. Everything else still applies cleanly to v4.10.0-rc5.

Chris Packham (4):
  clk: mvebu: support for 98DX3236 SoC
Changes in v2:
- Update devicetree binding documentation for new compatible string
Changes in v3:
- Add 98dx3236 support to mvebu/clk-corediv.c rather than creating a new
  driver.
- Document mv98dx3236-corediv-clock binding
Changes in v4:
- None
Changes in v5:
- Collect ack from Rob
- Remove explicit initialisation of fields to 0 in mv98dx3236_coreclks
- Register dummy clock provider for marvell,mv98dx3236-cpu-clock
  arm: mvebu: support for SMP on 98DX3336 SoC
Changes in v2:
- Document new enable-method value
- Correct some references from 98DX4521 to 98DX3236
Changes in v3:
- Simplify mv98dx3236_resume_init by using of_io_request_and_map()
Changes in v4:
- integrate changes into platsmp.c instead of new init call
- avoid duplicated code.
- fix error return
- Collect ack from Rob
Changes in v5:
- Remove useless casts (thanks to Stephen Boyd)
  arm: mvebu: Add device tree for 98DX3236 SoCs
Changes in v2:
- Update devicetree binding documentation to reflect that 98DX3336 and
  984251 are supersets of 98DX3236.
- disable crypto block
- disable sdio for 98DX3236, enable for 98DX4251
Changes in v3:
- fix typo 4521 -> 4251
- document prestera bindings
- rework corediv-clock binding
- add label to packet processor node
- add new compatible string for DFX server
Changes in v4:
- Collect ack from Rob
Changes in v5:
- Fixup license text. Add labels to nodes.
  arm: mvebu: Add device tree for db-dxbc2 and db-xc3-24g4xg boards
Changes in v5:
- update license text
- use node labels

Kalyan Kinthada (1):
  pinctrl: mvebu: pinctrl driver for 98DX3236 SoC
Changes in v2:
- include sdio support for the 98DX4251
Changes in v3:
- None
Changes in v4:
- Correct some discrepencies between binding and driver.
- Collect acks from Rob and Sebastian
Changes in v5:
- Update bindings to reflect "gpo" pins
- Use mvebu_mmio_mpp_ctrl instead of armada_xp_mpp_ctrl (note this is 
reliant
  on changes queued in linux-pinctrl)

 Documentation/devicetree/bindings/arm/cpus.txt |   1 +
 .../bindings/arm/marvell/98dx3236-resume-ctrl.txt  |  16 ++
 .../devicetree/bindings/arm/marvell/98dx3236.txt   |  23 ++
 .../bindings/clock/mvebu-corediv-clock.txt |   1 +
 .../devicetree/bindings/clock/mvebu-cpu-clock.txt  |   1 +
 .../devicetree/bindings/net/marvell,prestera.txt   |  50 
 .../pinctrl/marvell,armada-98dx3236-pinctrl.txt|  46 
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi  | 254 +
 arch/arm/boot/dts/armada-xp-98dx3336.dtsi  |  76 ++
 arch/arm/boot/dts/armada-xp-98dx4251.dtsi  |  90 
 arch/arm/boot/dts/db-dxbc2.dts | 151 
 arch/arm/boot/dts/db-xc3-24g4xg.dts| 142 
 arch/arm/mach-mvebu/platsmp.c  |  86 +++
 drivers/clk/mvebu/armada-xp.c  |  39 
 drivers/clk/mvebu/clk-corediv.c|  23 ++
 drivers/clk/mvebu/clk-cpu.c|   8 +
 drivers/pinctrl/mvebu/pinctrl-armada-xp.c  | 156 +
 17 files changed, 1163 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/marvell/98dx3236-resume-ctrl.txt
 create mode 100644 Documentation/devicetree/bindings/arm/marvell/98dx3236.txt
 create mode 100644 Documentation/devicetree/bindings/net/marvell,prestera.txt
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3236.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx3336.dtsi
 create mode 100644 arch/arm/boot/dts/armada-xp-98dx4251.dtsi
 create mode 100644 arch/arm/boot/dts/db-dxbc2.dts
 create mode 100644 arch/arm/boot/dts/db-xc3-24g4xg.dts

interdiff to v4:

 diff --git a/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt 
b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
 index 520562a7dc2a..c7b4e3a6b2c6 100644
@@ -85,7 +86,7 @@
  by address and length of the PMU DFS registers
  - #clock-cells : should be set to 1.
 diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
-index b3094315a3c0..0413bf8284e0 100644
+index b3094315a3c0..890a863ae0d0 100644
 --- a/drivers/clk/mvebu/armada-xp.c
 +++ b/drivers/clk/mvebu/armada-xp.c

[PATCHv5 3/5] pinctrl: mvebu: pinctrl driver for 98DX3236 SoC

[Chris Packham]

From: Kalyan Kinthada 

This pinctrl driver supports the 98DX3236, 98DX3336 and 98DX4251 SoCs
from Marvell.

Signed-off-by: Kalyan Kinthada 
Signed-off-by: Chris Packham 
Acked-by: Rob Herring 
Acked-by: Sebastian Hesselbarth 
---

Notes:
Changes in v2:
- include sdio support for the 98DX4251
Changes in v3:
- None
Changes in v4:
- Correct some discrepencies between binding and driver.
- Collect acks from Rob and Sebastian
Changes in v5:
- Update bindings to reflect "gpo" pins
- Use mvebu_mmio_mpp_ctrl instead of armada_xp_mpp_ctrl (note this is 
reliant
  on changes queued in linux-pinctrl)

 .../pinctrl/marvell,armada-98dx3236-pinctrl.txt|  46 ++
 drivers/pinctrl/mvebu/pinctrl-armada-xp.c  | 156 +
 2 files changed, 202 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt

diff --git 
a/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt 
b/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
new file mode 100644
index ..97aef67ee769
--- /dev/null
+++ 
b/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
@@ -0,0 +1,46 @@
+* Marvell 98dx3236 pinctrl driver for mpp
+
+Please refer to marvell,mvebu-pinctrl.txt in this directory for common binding
+part and usage
+
+Required properties:
+- compatible: "marvell,98dx3236-pinctrl" or "marvell,98dx4251-pinctrl"
+- reg: register specifier of MPP registers
+
+This driver supports all 98dx3236, 98dx3336 and 98dx4251 variants
+
+name  pins functions
+
+mpp0  0gpo, spi0(mosi), dev(ad8)
+mpp1  1gpio, spi0(miso), dev(ad9)
+mpp2  2gpo, spi0(sck), dev(ad10)
+mpp3  3gpio, spi0(cs0), dev(ad11)
+mpp4  4gpio, spi0(cs1), smi(mdc), dev(cs0)
+mpp5  5gpio, pex(rsto), sd0(cmd), dev(bootcs)
+mpp6  6gpo, sd0(clk), dev(a2)
+mpp7  7gpio, sd0(d0), dev(ale0)
+mpp8  8gpio, sd0(d1), dev(ale1)
+mpp9  9gpio, sd0(d2), dev(ready0)
+mpp10 10   gpio, sd0(d3), dev(ad12)
+mpp11 11   gpio, uart1(rxd), uart0(cts), dev(ad13)
+mpp12 12   gpo, uart1(txd), uart0(rts), dev(ad14)
+mpp13 13   gpio, intr(out), dev(ad15)
+mpp14 14   gpio, i2c0(sck)
+mpp15 15   gpio, i2c0(sda)
+mpp16 16   gpo, dev(oe)
+mpp17 17   gpo, dev(clkout)
+mpp18 18   gpio, uart1(txd)
+mpp19 19   gpio, uart1(rxd), dev(rb)
+mpp20 20   gpo, dev(we0)
+mpp21 21   gpo, dev(ad0)
+mpp22 22   gpo, dev(ad1)
+mpp23 23   gpo, dev(ad2)
+mpp24 24   gpo, dev(ad3)
+mpp25 25   gpo, dev(ad4)
+mpp26 26   gpo, dev(ad5)
+mpp27 27   gpo, dev(ad6)
+mpp28 28   gpo, dev(ad7)
+mpp29 29   gpo, dev(a0)
+mpp30 30   gpo, dev(a1)
+mpp31 31   gpio, slv_smi(mdc), smi(mdc), dev(we1)
+mpp32 32   gpio, slv_smi(mdio), smi(mdio), dev(cs1)
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-xp.c 
b/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
index 63e1bd506983..61cbc138703e 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
@@ -38,6 +38,10 @@ enum armada_xp_variant {
V_MV78460   = BIT(2),
V_MV78230_PLUS  = (V_MV78230 | V_MV78260 | V_MV78460),
V_MV78260_PLUS  = (V_MV78260 | V_MV78460),
+   V_98DX3236  = BIT(3),
+   V_98DX3336  = BIT(4),
+   V_98DX4251  = BIT(5),
+   V_98DX3236_PLUS = (V_98DX3236 | V_98DX3336 | V_98DX4251),
 };
 
 static struct mvebu_mpp_mode armada_xp_mpp_modes[] = {
@@ -349,6 +353,131 @@ static struct mvebu_mpp_mode armada_xp_mpp_modes[] = {
 MPP_VAR_FUNCTION(0x1, "dev", "ad31",   V_MV78260_PLUS)),
 };
 
+static struct mvebu_mpp_mode mv98dx3236_mpp_modes[] = {
+   MPP_MODE(0,
+MPP_VAR_FUNCTION(0x0, "gpo", NULL,  V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "mosi",   V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x4, "dev", "ad8", V_98DX3236_PLUS)),
+   MPP_MODE(1,
+MPP_VAR_FUNCTION(0x0, "gpio", NULL, V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "miso",   V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x4, "dev", "ad9", V_98DX3236_PLUS)),
+   MPP_MODE(2,
+MPP_VAR_FUNCTION(0x0, "gpo", NULL,  V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "sck",

[PATCHv5 3/5] pinctrl: mvebu: pinctrl driver for 98DX3236 SoC

[Chris Packham]

From: Kalyan Kinthada 

This pinctrl driver supports the 98DX3236, 98DX3336 and 98DX4251 SoCs
from Marvell.

Signed-off-by: Kalyan Kinthada 
Signed-off-by: Chris Packham 
Acked-by: Rob Herring 
Acked-by: Sebastian Hesselbarth 
---

Notes:
Changes in v2:
- include sdio support for the 98DX4251
Changes in v3:
- None
Changes in v4:
- Correct some discrepencies between binding and driver.
- Collect acks from Rob and Sebastian
Changes in v5:
- Update bindings to reflect "gpo" pins
- Use mvebu_mmio_mpp_ctrl instead of armada_xp_mpp_ctrl (note this is 
reliant
  on changes queued in linux-pinctrl)

 .../pinctrl/marvell,armada-98dx3236-pinctrl.txt|  46 ++
 drivers/pinctrl/mvebu/pinctrl-armada-xp.c  | 156 +
 2 files changed, 202 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt

diff --git 
a/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt 
b/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
new file mode 100644
index ..97aef67ee769
--- /dev/null
+++ 
b/Documentation/devicetree/bindings/pinctrl/marvell,armada-98dx3236-pinctrl.txt
@@ -0,0 +1,46 @@
+* Marvell 98dx3236 pinctrl driver for mpp
+
+Please refer to marvell,mvebu-pinctrl.txt in this directory for common binding
+part and usage
+
+Required properties:
+- compatible: "marvell,98dx3236-pinctrl" or "marvell,98dx4251-pinctrl"
+- reg: register specifier of MPP registers
+
+This driver supports all 98dx3236, 98dx3336 and 98dx4251 variants
+
+name  pins functions
+
+mpp0  0gpo, spi0(mosi), dev(ad8)
+mpp1  1gpio, spi0(miso), dev(ad9)
+mpp2  2gpo, spi0(sck), dev(ad10)
+mpp3  3gpio, spi0(cs0), dev(ad11)
+mpp4  4gpio, spi0(cs1), smi(mdc), dev(cs0)
+mpp5  5gpio, pex(rsto), sd0(cmd), dev(bootcs)
+mpp6  6gpo, sd0(clk), dev(a2)
+mpp7  7gpio, sd0(d0), dev(ale0)
+mpp8  8gpio, sd0(d1), dev(ale1)
+mpp9  9gpio, sd0(d2), dev(ready0)
+mpp10 10   gpio, sd0(d3), dev(ad12)
+mpp11 11   gpio, uart1(rxd), uart0(cts), dev(ad13)
+mpp12 12   gpo, uart1(txd), uart0(rts), dev(ad14)
+mpp13 13   gpio, intr(out), dev(ad15)
+mpp14 14   gpio, i2c0(sck)
+mpp15 15   gpio, i2c0(sda)
+mpp16 16   gpo, dev(oe)
+mpp17 17   gpo, dev(clkout)
+mpp18 18   gpio, uart1(txd)
+mpp19 19   gpio, uart1(rxd), dev(rb)
+mpp20 20   gpo, dev(we0)
+mpp21 21   gpo, dev(ad0)
+mpp22 22   gpo, dev(ad1)
+mpp23 23   gpo, dev(ad2)
+mpp24 24   gpo, dev(ad3)
+mpp25 25   gpo, dev(ad4)
+mpp26 26   gpo, dev(ad5)
+mpp27 27   gpo, dev(ad6)
+mpp28 28   gpo, dev(ad7)
+mpp29 29   gpo, dev(a0)
+mpp30 30   gpo, dev(a1)
+mpp31 31   gpio, slv_smi(mdc), smi(mdc), dev(we1)
+mpp32 32   gpio, slv_smi(mdio), smi(mdio), dev(cs1)
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-xp.c 
b/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
index 63e1bd506983..61cbc138703e 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-xp.c
@@ -38,6 +38,10 @@ enum armada_xp_variant {
V_MV78460   = BIT(2),
V_MV78230_PLUS  = (V_MV78230 | V_MV78260 | V_MV78460),
V_MV78260_PLUS  = (V_MV78260 | V_MV78460),
+   V_98DX3236  = BIT(3),
+   V_98DX3336  = BIT(4),
+   V_98DX4251  = BIT(5),
+   V_98DX3236_PLUS = (V_98DX3236 | V_98DX3336 | V_98DX4251),
 };
 
 static struct mvebu_mpp_mode armada_xp_mpp_modes[] = {
@@ -349,6 +353,131 @@ static struct mvebu_mpp_mode armada_xp_mpp_modes[] = {
 MPP_VAR_FUNCTION(0x1, "dev", "ad31",   V_MV78260_PLUS)),
 };
 
+static struct mvebu_mpp_mode mv98dx3236_mpp_modes[] = {
+   MPP_MODE(0,
+MPP_VAR_FUNCTION(0x0, "gpo", NULL,  V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "mosi",   V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x4, "dev", "ad8", V_98DX3236_PLUS)),
+   MPP_MODE(1,
+MPP_VAR_FUNCTION(0x0, "gpio", NULL, V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "miso",   V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x4, "dev", "ad9", V_98DX3236_PLUS)),
+   MPP_MODE(2,
+MPP_VAR_FUNCTION(0x0, "gpo", NULL,  V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x2, "spi0", "sck",V_98DX3236_PLUS),
+MPP_VAR_FUNCTION(0x4, "dev", "ad10",V_98DX3236_PLUS)),
+   MPP_MODE(3,
+MPP_VAR_FUNCTION(0x0, "gpio", NULL,