Re: [PATCH v9 00/18] Add VT-d Posted-Interrupts support - including prerequisite series

2015-09-25 Thread Paolo Bonzini 


On 25/09/2015 03:49, Wu, Feng wrote:
> Hi Paolo,
> 
> Thanks for your review on this series! I'd like to confirm this series (plus
> the patch fixing the compilation error) is okay to you and I don't need to
> do extra things for it, right?

Yes, can you check if branch vtd-pi of
git://git.kernel.org/pub/scm/virt/kvm/kvm.git works for you?  If so I'll
merge it.

Paolo

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Viresh Kumar 
global_lock is defined as an unsigned long and accessing only its lower
32 bits from sysfs is incorrect, as we need to consider other 32 bits
for big endian 64 bit systems. There are no such platforms yet, but the
code needs to be robust for such a case.

Fix that by passing a local variable to debugfs_create_bool() and
assigning its value to global_lock later.

Signed-off-by: Viresh Kumar 
---
V3->V4:
- Create a local variable instead of changing type of global_lock
  (Rafael)
- Drop the stable tag
- BCC'd a lot of people (rather than cc'ing them) to make sure
  - the series reaches them
  - mailing lists do not block the patchset due to long cc list
  - and we don't spam the BCC'd people for every reply
---
 drivers/acpi/ec_sys.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
index b4c216bab22b..b44b91331a56 100644
--- a/drivers/acpi/ec_sys.c
+++ b/drivers/acpi/ec_sys.c
@@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned 
int ec_device_count)
struct dentry *dev_dir;
char name[64];
umode_t mode = 0400;
+   u32 val;
 
if (ec_device_count == 0) {
acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
@@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, 
unsigned int ec_device_count)
 
if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
goto error;
-   if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
-(u32 *)&first_ec->global_lock))
+   if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
goto error;
 
+   first_ec->global_lock = val;
+
if (write_support)
mode = 0600;
if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
-- 
2.4.0

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH V4 2/2] debugfs: Pass bool pointer to debugfs_create_bool()

2015-09-25 Thread Viresh Kumar 
Its a bit odd that debugfs_create_bool() takes 'u32 *' as an argument,
when all it needs is a boolean pointer.

It would be better to update this API to make it accept 'bool *'
instead, as that will make it more consistent and often more convenient.
Over that bool takes just a byte (mostly).

That required updates to all user sites as well, in the same commit
updating the API. regmap core was also using
debugfs_{read|write}_file_bool(), directly and variable types were
updated for that to be bool as well.

Signed-off-by: Viresh Kumar 
Acked-by: Mark Brown 
Acked-by: Charles Keepax 
---
V3->V4:
- Updated commit log to make it clear
- Drop 'global_lock = !!tmp' change, as that's not required at all.
- s/u32/bool for ec_sys.c
- Added Ack from Charles.
- BCC'd a lot of people (rather than cc'ing them) to make sure
  - the series reaches them
  - mailing lists do not block the patchset due to long cc list
  - and we don't spam the BCC'd people for every reply
---
 Documentation/filesystems/debugfs.txt  |  2 +-
 arch/arm64/kernel/debug-monitors.c |  4 ++--
 drivers/acpi/ec_sys.c  |  2 +-
 drivers/acpi/internal.h|  2 +-
 drivers/base/regmap/internal.h |  6 +++---
 drivers/base/regmap/regcache-lzo.c |  4 ++--
 drivers/base/regmap/regcache.c | 24 
 drivers/bluetooth/hci_qca.c|  4 ++--
 drivers/iommu/amd_iommu_init.c |  2 +-
 drivers/iommu/amd_iommu_types.h|  2 +-
 drivers/misc/mei/mei_dev.h |  2 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h |  4 ++--
 drivers/net/wireless/ath/ath10k/core.h |  2 +-
 drivers/net/wireless/ath/ath5k/ath5k.h |  2 +-
 drivers/net/wireless/ath/ath9k/hw.c|  2 +-
 drivers/net/wireless/ath/ath9k/hw.h|  4 ++--
 drivers/net/wireless/b43/debugfs.c | 18 +-
 drivers/net/wireless/b43/debugfs.h |  2 +-
 drivers/net/wireless/b43legacy/debugfs.c   | 10 +-
 drivers/net/wireless/b43legacy/debugfs.h   |  2 +-
 drivers/net/wireless/iwlegacy/common.h |  6 +++---
 drivers/net/wireless/iwlwifi/mvm/mvm.h |  6 +++---
 drivers/scsi/snic/snic_trc.c   |  4 ++--
 drivers/scsi/snic/snic_trc.h   |  2 +-
 drivers/uwb/uwb-debug.c|  2 +-
 fs/debugfs/file.c  |  6 +++---
 include/linux/debugfs.h|  4 ++--
 include/linux/edac.h   |  2 +-
 include/linux/fault-inject.h   |  2 +-
 kernel/futex.c |  4 ++--
 lib/dma-debug.c|  2 +-
 mm/failslab.c  |  8 
 mm/page_alloc.c|  8 
 sound/soc/codecs/wm_adsp.h |  2 +-
 34 files changed, 79 insertions(+), 79 deletions(-)

diff --git a/Documentation/filesystems/debugfs.txt 
b/Documentation/filesystems/debugfs.txt
index 463f595733e8..4f45f71149cb 100644
--- a/Documentation/filesystems/debugfs.txt
+++ b/Documentation/filesystems/debugfs.txt
@@ -105,7 +105,7 @@ a variable of type size_t.
 Boolean values can be placed in debugfs with:
 
 struct dentry *debugfs_create_bool(const char *name, umode_t mode,
-  struct dentry *parent, u32 *value);
+  struct dentry *parent, bool *value);
 
 A read on the resulting file will yield either Y (for non-zero values) or
 N, followed by a newline.  If written to, it will accept either upper- or
diff --git a/arch/arm64/kernel/debug-monitors.c 
b/arch/arm64/kernel/debug-monitors.c
index cebf78661a55..1c4cd4a0d7cc 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -58,7 +58,7 @@ static u32 mdscr_read(void)
  * Allow root to disable self-hosted debug from userspace.
  * This is useful if you want to connect an external JTAG debugger.
  */
-static u32 debug_enabled = 1;
+static bool debug_enabled = true;
 
 static int create_debug_debugfs_entry(void)
 {
@@ -69,7 +69,7 @@ fs_initcall(create_debug_debugfs_entry);
 
 static int __init early_debug_disable(char *buf)
 {
-   debug_enabled = 0;
+   debug_enabled = false;
return 0;
 }
 
diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
index b44b91331a56..f025b044a1cf 100644
--- a/drivers/acpi/ec_sys.c
+++ b/drivers/acpi/ec_sys.c
@@ -110,7 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, unsigned 
int ec_device_count)
struct dentry *dev_dir;
char name[64];
umode_t mode = 0400;
-   u32 val;
+   bool val;
 
if (ec_device_count == 0) {
acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 9e426210c2a8..5a72e2b140fc 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -138,7 +138,7 @@ struct acpi_ec {
unsigned long gpe;
unsig

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Johannes Berg 
On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote:

> Signed-off-by: Viresh Kumar 
> ---
> V3->V4:
> - Create a local variable instead of changing type of global_lock
>   (Rafael)

Err, surely that wasn't what Rafael meant, since it's clearly
impossible to use a pointer to the stack, assign to it once, and the
expect anything to wkr at all ...

johannes

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Viresh Kumar 
On 25-09-15, 19:42, Johannes Berg wrote:
> On Fri, 2015-09-25 at 09:41 -0700, Viresh Kumar wrote:
> 
> > Signed-off-by: Viresh Kumar 
> > ---
> > V3->V4:
> > - Create a local variable instead of changing type of global_lock
> >   (Rafael)
> 
> Err, surely that wasn't what Rafael meant, since it's clearly
> impossible to use a pointer to the stack, assign to it once, and the
> expect anything to wkr at all ...

Sorry, I am not sure on what wouldn't work with this patch but this is
what Rafael said, and I don't think I wrote it differently :)

Rafael wrote:
> Actually, what about adding a local u32 variable, say val, here and doing
> 
> > if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> > goto error;
> > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> > -(u32 *)&first_ec->global_lock))
> > +&first_ec->global_lock))
> 
>   if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
> 
> > goto error;
> 
>   first_ec->global_lock = val;
> 
> And then you can turn val into bool just fine without changing the structure
> definition.

-- 
viresh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Johannes Berg 

> Rafael wrote:
> > Actually, what about adding a local u32 variable, say val, here and 
> > doing
> > 
> > >   if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 
> > > *)&first_ec->gpe))
> > >   goto error;
> > >   if (!debugfs_create_bool("use_global_lock", 0444, 
> > > dev_dir,
> > > -  (u32 *)&first_ec->global_lock))
> > > +  &first_ec->global_lock))
> > 
> > if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, 
> > &val))
> > 
> > >   goto error;
> > 
> > first_ec->global_lock = val;
> > 
> > And then you can turn val into bool just fine without changing the 
> > structure
> > definition.

Ok, then, but that means Rafael is completely wrong ...
debugfs_create_bool() takes a *pointer* and it needs to be long-lived,
it can't be on the stack. You also don't get a call when it changes.

If you cannot change the struct definition then you must implement a
debugfs file with its own read/write handlers.

johannes
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Viresh Kumar 
On 25-09-15, 20:49, Johannes Berg wrote:
> Ok, then, but that means Rafael is completely wrong ...
> debugfs_create_bool() takes a *pointer* and it needs to be long-lived,
> it can't be on the stack. You also don't get a call when it changes.

Ahh, ofcourse. My bad as well...

I think we can change structure definition but will wait for Rafael's
comment before that.

-- 
viresh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
> global_lock is defined as an unsigned long and accessing only its lower
> 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> for big endian 64 bit systems. There are no such platforms yet, but the
> code needs to be robust for such a case.
> 
> Fix that by passing a local variable to debugfs_create_bool() and
> assigning its value to global_lock later.
> 
> Signed-off-by: Viresh Kumar 

Acked-by: Rafael J. Wysocki 

Greg, please take this one if the [2/2] looks good to you.

> ---
> V3->V4:
> - Create a local variable instead of changing type of global_lock
>   (Rafael)
> - Drop the stable tag
> - BCC'd a lot of people (rather than cc'ing them) to make sure
>   - the series reaches them
>   - mailing lists do not block the patchset due to long cc list
>   - and we don't spam the BCC'd people for every reply
> ---
>  drivers/acpi/ec_sys.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> index b4c216bab22b..b44b91331a56 100644
> --- a/drivers/acpi/ec_sys.c
> +++ b/drivers/acpi/ec_sys.c
> @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, 
> unsigned int ec_device_count)
>   struct dentry *dev_dir;
>   char name[64];
>   umode_t mode = 0400;
> + u32 val;
>  
>   if (ec_device_count == 0) {
>   acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
> @@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec, 
> unsigned int ec_device_count)
>  
>   if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
>   goto error;
> - if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> -  (u32 *)&first_ec->global_lock))
> + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
>   goto error;
>  
> + first_ec->global_lock = val;
> +
>   if (write_support)
>   mode = 0600;
>   if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
> 

Thanks,
Rafael

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Friday, September 25, 2015 10:18:13 PM Rafael J. Wysocki wrote:
> On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
> > global_lock is defined as an unsigned long and accessing only its lower
> > 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> > for big endian 64 bit systems. There are no such platforms yet, but the
> > code needs to be robust for such a case.
> > 
> > Fix that by passing a local variable to debugfs_create_bool() and
> > assigning its value to global_lock later.
> > 
> > Signed-off-by: Viresh Kumar 
> 
> Acked-by: Rafael J. Wysocki 
> 
> Greg, please take this one if the [2/2] looks good to you.

Ouch, turns out it was a bad idea.  Please scratch that.

Thanks,
Rafael

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote:
> On 25-09-15, 20:49, Johannes Berg wrote:
> > Ok, then, but that means Rafael is completely wrong ...
> > debugfs_create_bool() takes a *pointer* and it needs to be long-lived,
> > it can't be on the stack. You also don't get a call when it changes.
> 
> Ahh, ofcourse. My bad as well...

Well, sorry about the wrong suggestion.

> I think we can change structure definition but will wait for Rafael's
> comment before that.

OK, change the structure then.

Thanks,
Rafael

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Friday, September 25, 2015 10:26:22 PM Rafael J. Wysocki wrote:
> On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote:
> > On 25-09-15, 20:49, Johannes Berg wrote:
> > > Ok, then, but that means Rafael is completely wrong ...
> > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived,
> > > it can't be on the stack. You also don't get a call when it changes.
> > 
> > Ahh, ofcourse. My bad as well...
> 
> Well, sorry about the wrong suggestion.
> 
> > I think we can change structure definition but will wait for Rafael's
> > comment before that.
> 
> OK, change the structure then.

But here's a question.

You're going to change that into bool in the next patch, right?

So what if bool is a byte and the field is not word-aligned and changing
that byte requires a read-modify-write.  How do we ensure that things remain
consistent in that case?

Thanks,
Rafael

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Viresh Kumar 
On 25 September 2015 at 13:33, Rafael J. Wysocki  wrote:
> You're going to change that into bool in the next patch, right?

Yeah.

> So what if bool is a byte and the field is not word-aligned

Its between two 'unsigned long' variables today, and the struct isn't packed.
So, it will be aligned, isn't it?

> and changing
> that byte requires a read-modify-write.  How do we ensure that things remain
> consistent in that case?

I didn't understood why a read-modify-write is special here? That's
what will happen
to most of the non-word-sized fields anyway?

Probably I didn't understood what you meant..

--
viresh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote:
> On 25 September 2015 at 13:33, Rafael J. Wysocki  wrote:
> > You're going to change that into bool in the next patch, right?
> 
> Yeah.
> 
> > So what if bool is a byte and the field is not word-aligned
> 
> Its between two 'unsigned long' variables today, and the struct isn't packed.
> So, it will be aligned, isn't it?
> 
> > and changing
> > that byte requires a read-modify-write.  How do we ensure that things remain
> > consistent in that case?
> 
> I didn't understood why a read-modify-write is special here? That's
> what will happen
> to most of the non-word-sized fields anyway?
> 
> Probably I didn't understood what you meant..

Say you have three adjacent fields in a structure, x, y, z, each one byte long.
Initially, all of them are equal to 0.

CPU A writes 1 to x and CPU B writes 2 to y at the same time.

What's the result?

Thanks,
Rafael

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Viresh Kumar 
On 25-09-15, 22:58, Rafael J. Wysocki wrote:
> Say you have three adjacent fields in a structure, x, y, z, each one byte 
> long.
> Initially, all of them are equal to 0.
> 
> CPU A writes 1 to x and CPU B writes 2 to y at the same time.
> 
> What's the result?

But then two CPUs can update the same variable as well, and we must
have proper locking in place to fix that.

Anyway, that problem isn't here for sure as its between two
unsigned-longs. So, should I just move it to bool and resend ?

-- 
viresh
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'

2015-09-25 Thread Rafael J. Wysocki 
On Fri, Sep 25, 2015 at 11:44 PM, Viresh Kumar  wrote:
> On 25-09-15, 22:58, Rafael J. Wysocki wrote:
>> Say you have three adjacent fields in a structure, x, y, z, each one byte 
>> long.
>> Initially, all of them are equal to 0.
>>
>> CPU A writes 1 to x and CPU B writes 2 to y at the same time.
>>
>> What's the result?
>
> But then two CPUs can update the same variable as well, and we must
> have proper locking in place to fix that.

Right.

So if you allow something like debugfs to update your structure, how
do you make sure there is the proper locking?

Is that not a problem in all of the places modified by the [2/2]?

How can the future users of the API know what to do to avoid potential
problems with it?

>
> Anyway, that problem isn't here for sure as its between two
> unsigned-longs. So, should I just move it to bool and resend ?

I guess it might be more convenient to fold this into the other patch,
because we seem to be splitting hairs here.

Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/arm-smmu: Only return IRQ_NONE if FSR is not set

2015-09-25 Thread Mitchel Humpherys 
Currently we return IRQ_NONE from the context fault handler if the FSR
doesn't actually have the fault bit set (some sort of miswired
interrupt?) or if the client doesn't register an IOMMU fault handler.
However, registering a client fault handler is optional, so telling the
interrupt framework that the interrupt wasn't for this device if the
client doesn't register a handler isn't exactly accurate.  Fix this by
returning IRQ_HANDLED even if the client doesn't register a handler.

Signed-off-by: Mitchel Humpherys 
---
 drivers/iommu/arm-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 48a39dfa9777..95560d447a54 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -653,7 +653,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void 
*dev)
dev_err_ratelimited(smmu->dev,
"Unhandled context fault: iova=0x%08lx, fsynr=0x%x, 
cb=%d\n",
iova, fsynr, cfg->cbndx);
-   ret = IRQ_NONE;
+   ret = IRQ_HANDLED;
resume = RESUME_TERMINATE;
}
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu