Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).


On the other hand the big advantage of modversions is that it also 
verifies the checksum during runtime (module loading). In other words, I 
believe that any other solution should still generate some form of 
checksum/watermark which can be easily checked for compatibility on 
module load.
It should not be hard to add to the DWARF based tools though. We'd just 
parse DWARF data instead of the C code.


Regards,
-Stanislav

Re: [PATCH] pci-error-recover: doc cleanup

[Cao jin]

On 12/09/2016 02:44 PM, Linas Vepstas wrote:
> On Fri, Dec 9, 2016 at 2:37 PM, Cao jin  wrote:
>>
>>
>> On 12/09/2016 02:24 PM, Linas Vepstas wrote:
>>> I suppose I'm confused, but I recall that link resets are non-fatal.
>>> Fatal errors typically require that the the pci adapter be completely
>>> reset, any adapter firmware to be reloaded from scratch, the device
>>> driver has to kill all device state and start from scratch. Its huge.
>>> If the fatal error is on pci device that is under a block device
>>> holding a file system, then (usually) there is no way to recover,
>>> because the block layer (and file system) cannot deal with a block
>>> device that disappeared and then reappeared some few seconds later.
>>> (maybe some future zfs or lvm or btrfs might be able to deal with
>>> this, but not today)
>>>
>>> By contrast, link resets are far more gentle: the device driver might
>>> have to discard some half-full FIFO's, or cancel some in-flight
>>> commands, but can otherwise gracefully recover without telling the
>>> higher layers that there were any problems.
>>>
>>> --linas
>>>
>>
>> I am little confused too, even not sure if we are talking the same
>> *fatal error*, I am talking the fatal error defined in PCI Express spec,
>> chapter 6.2.2.2.1:
>>
>> Fatal errors are uncorrectable error conditions which render the
>> particular Link and related hardware unreliable. For Fatal errors, a
>> reset of the components on the Link may be required to return to
>> reliable operation. Platform handling of Fatal errors, and any efforts
>> to limit the effects of these errors, is platform implementation specific.
>>
>> Link reset means set *secondary bus reset* bit in pci bridge config
>> space, can reset the link and device simultaneously, is the strongest
>> kind of reset as I know.
> 
> OK, well, its been far too many years, and I don't have the PCI spec
> at my fingertips.
> Isn't there a link reset that can be performed, without forcing a device 
> reset?
> 

At least I don't find the exact words saying that.

-- 
Sincerely,
Cao jin

> The intent was that some PCI link errors are due to vibration,
> ground-bounce, humidity, etc. and that these errors can be detected
> and do not corrupt the device state or the device driver state.  Since
> they are not associated with data corruption (or rather, the
> corruption is local to the link), these can be recovered by reseting
> just the link, without resetting the whole adapter. They may require
> reseting some device-driver state, but not all of it.
> 
> However, this was all decided before the PCI-E spec was written, so
> maybe the newer PCI-E specs now say something different.
> 
> --linas
> 
>>
>>> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:


 On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
> On Thu, 8 Dec 2016 16:16:14 +0800
> Cao jin  wrote:
>
>>  The platform resets the link, and then calls the link_reset() callback
>>  on all affected device drivers.  This is a PCI-Express specific state
>> -and is done whenever a non-fatal error has been detected that can be
>> +and is done whenever a fatal error has been detected that can be
>>  "solved" by resetting the link. This call informs the driver of the
>
> As far as I can tell, the original text was correct here; why do you
> think this change needs to be made?
>

 See do_recovery() in aer core, reset_link() is called only seeing fatal
 error.

 --
 Sincerely,
 Cao jin


>>>
>>>
>>>
>>
>> --
>> Sincerely,
>> Cao jin
>>
>>
> 
> 
> .
> 

Re: [PATCH] x86/kbuild: enable modversions for symbols exported from asm

[Stanislav Kozina]

The question is how to provide a similar guarantee if a different way?

As a tool to aid distro reviewers, modversions has some value, but the
debug info parsing tools that have been mentioned in this thread seem
superior (not that I've tested them).


On the other hand the big advantage of modversions is that it also 
verifies the checksum during runtime (module loading). In other words, I 
believe that any other solution should still generate some form of 
checksum/watermark which can be easily checked for compatibility on 
module load.
It should not be hard to add to the DWARF based tools though. We'd just 
parse DWARF data instead of the C code.


Regards,
-Stanislav

Re: [PATCH] pci-error-recover: doc cleanup

[Cao jin]

On 12/09/2016 02:44 PM, Linas Vepstas wrote:
> On Fri, Dec 9, 2016 at 2:37 PM, Cao jin  wrote:
>>
>>
>> On 12/09/2016 02:24 PM, Linas Vepstas wrote:
>>> I suppose I'm confused, but I recall that link resets are non-fatal.
>>> Fatal errors typically require that the the pci adapter be completely
>>> reset, any adapter firmware to be reloaded from scratch, the device
>>> driver has to kill all device state and start from scratch. Its huge.
>>> If the fatal error is on pci device that is under a block device
>>> holding a file system, then (usually) there is no way to recover,
>>> because the block layer (and file system) cannot deal with a block
>>> device that disappeared and then reappeared some few seconds later.
>>> (maybe some future zfs or lvm or btrfs might be able to deal with
>>> this, but not today)
>>>
>>> By contrast, link resets are far more gentle: the device driver might
>>> have to discard some half-full FIFO's, or cancel some in-flight
>>> commands, but can otherwise gracefully recover without telling the
>>> higher layers that there were any problems.
>>>
>>> --linas
>>>
>>
>> I am little confused too, even not sure if we are talking the same
>> *fatal error*, I am talking the fatal error defined in PCI Express spec,
>> chapter 6.2.2.2.1:
>>
>> Fatal errors are uncorrectable error conditions which render the
>> particular Link and related hardware unreliable. For Fatal errors, a
>> reset of the components on the Link may be required to return to
>> reliable operation. Platform handling of Fatal errors, and any efforts
>> to limit the effects of these errors, is platform implementation specific.
>>
>> Link reset means set *secondary bus reset* bit in pci bridge config
>> space, can reset the link and device simultaneously, is the strongest
>> kind of reset as I know.
> 
> OK, well, its been far too many years, and I don't have the PCI spec
> at my fingertips.
> Isn't there a link reset that can be performed, without forcing a device 
> reset?
> 

At least I don't find the exact words saying that.

-- 
Sincerely,
Cao jin

> The intent was that some PCI link errors are due to vibration,
> ground-bounce, humidity, etc. and that these errors can be detected
> and do not corrupt the device state or the device driver state.  Since
> they are not associated with data corruption (or rather, the
> corruption is local to the link), these can be recovered by reseting
> just the link, without resetting the whole adapter. They may require
> reseting some device-driver state, but not all of it.
> 
> However, this was all decided before the PCI-E spec was written, so
> maybe the newer PCI-E specs now say something different.
> 
> --linas
> 
>>
>>> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:


 On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
> On Thu, 8 Dec 2016 16:16:14 +0800
> Cao jin  wrote:
>
>>  The platform resets the link, and then calls the link_reset() callback
>>  on all affected device drivers.  This is a PCI-Express specific state
>> -and is done whenever a non-fatal error has been detected that can be
>> +and is done whenever a fatal error has been detected that can be
>>  "solved" by resetting the link. This call informs the driver of the
>
> As far as I can tell, the original text was correct here; why do you
> think this change needs to be made?
>

 See do_recovery() in aer core, reset_link() is called only seeing fatal
 error.

 --
 Sincerely,
 Cao jin


>>>
>>>
>>>
>>
>> --
>> Sincerely,
>> Cao jin
>>
>>
> 
> 
> .
> 

RE: [PATCH v10 1/6] drivers/platform/x86/p2sb: New Primary to Sideband bridge support driver for Intel SOC's

[Tan, Jui Nee]

> -Original Message-
> From: linux-gpio-ow...@vger.kernel.org [mailto:linux-gpio-
> ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
> Sent: Friday, November 11, 2016 12:07 AM
> To: Tan, Jui Nee ; mika.westerb...@linux.intel.com;
> heikki.kroge...@linux.intel.com; t...@linutronix.de; dvh...@infradead.org;
> mi...@redhat.com; h...@zytor.com; x...@kernel.org; pty...@xes-inc.com;
> lee.jo...@linaro.org; linus.wall...@linaro.org
> Cc: linux-g...@vger.kernel.org; platform-driver-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Yong, Jonathan ;
> Yu, Ong Hock ; Luck, Tony ;
> Wan Mohamad, Wan Ahmad Zainie ;
> Sun, Yunying 
> Subject: Re: [PATCH v10 1/6] drivers/platform/x86/p2sb: New Primary to
> Sideband bridge support driver for Intel SOC's
> 
> On Thu, 2016-11-10 at 17:00 +0800, Tan Jui Nee wrote:
> > From: Andy Shevchenko 
> >
> > There is already one and at least one more user coming which require
> > an access to Primary to Sideband bridge (P2SB) in order to get IO or
> > MMIO bar hidden by BIOS.
> > Create a driver to access P2SB for x86 devices.
> >
> > Signed-off-by: Yong, Jonathan 
> > Signed-off-by: Andy Shevchenko 
> 
> 
> > +int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
> > +   struct resource *res)
> > +{
> > +   u32 base_addr;
> > +   u64 base64_addr;
> > +   unsigned long flags;
> > +
> >
> 
> > +   if (!res)
> > +   return -EINVAL;
> 
> I don't remember the details, one version was quite changed, so, I think
> these lines are not needed anymore.
> 
Noted, these lines will be removed in next patch version (v12).
> > +   /* Get IO or MMIO BAR */
> > +   pci_bus_read_config_dword(pdev->bus, devfn, SBREG_BAR,
> > _addr);
> > +   if ((base_addr & PCI_BASE_ADDRESS_SPACE) ==
> > PCI_BASE_ADDRESS_SPACE_IO) {
> > +   flags = IORESOURCE_IO;
> > +   base64_addr = base_addr & PCI_BASE_ADDRESS_IO_MASK;
> > +   } else {
> > +   flags = IORESOURCE_MEM;
> > +   base64_addr = base_addr & PCI_BASE_ADDRESS_MEM_MASK;
> > +   if (base_addr & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +   flags |= IORESOURCE_MEM_64;
> >
> 
> > +   pci_bus_read_config_dword(pdev->bus, devfn,
> > +   SBREG_BAR + 4, _addr);
> 
> Fix indentation.
> 
Thanks for pointing that out. I will fix that in next patch version (v12). 
> > +   base64_addr |= (u64)base_addr << 32;
> > +   }
> > +   }
> > +
> > +   /* Hide the P2SB device */
> > +   pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE,
> > 0x01);
> > +
> > +   spin_unlock(_spinlock);
> > +
> 
> > +   /* User provides prefilled resources */
> 
> Not anymore as far I as I can see. You just return here the result.
> 
Current version is returning status of p2sb_bar function, i.e., 0 on success or 
appropriate errno value on error. Perhaps you could share the reason of return 
the result instead of status. 
> > +   res->start = (resource_size_t)base64_addr;
> > +   res->flags = flags;
> 
> --
> Andy Shevchenko 
> Intel Finland Oy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

RE: [PATCH v10 1/6] drivers/platform/x86/p2sb: New Primary to Sideband bridge support driver for Intel SOC's

[Tan, Jui Nee]

> -Original Message-
> From: linux-gpio-ow...@vger.kernel.org [mailto:linux-gpio-
> ow...@vger.kernel.org] On Behalf Of Andy Shevchenko
> Sent: Friday, November 11, 2016 12:07 AM
> To: Tan, Jui Nee ; mika.westerb...@linux.intel.com;
> heikki.kroge...@linux.intel.com; t...@linutronix.de; dvh...@infradead.org;
> mi...@redhat.com; h...@zytor.com; x...@kernel.org; pty...@xes-inc.com;
> lee.jo...@linaro.org; linus.wall...@linaro.org
> Cc: linux-g...@vger.kernel.org; platform-driver-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Yong, Jonathan ;
> Yu, Ong Hock ; Luck, Tony ;
> Wan Mohamad, Wan Ahmad Zainie ;
> Sun, Yunying 
> Subject: Re: [PATCH v10 1/6] drivers/platform/x86/p2sb: New Primary to
> Sideband bridge support driver for Intel SOC's
> 
> On Thu, 2016-11-10 at 17:00 +0800, Tan Jui Nee wrote:
> > From: Andy Shevchenko 
> >
> > There is already one and at least one more user coming which require
> > an access to Primary to Sideband bridge (P2SB) in order to get IO or
> > MMIO bar hidden by BIOS.
> > Create a driver to access P2SB for x86 devices.
> >
> > Signed-off-by: Yong, Jonathan 
> > Signed-off-by: Andy Shevchenko 
> 
> 
> > +int p2sb_bar(struct pci_dev *pdev, unsigned int devfn,
> > +   struct resource *res)
> > +{
> > +   u32 base_addr;
> > +   u64 base64_addr;
> > +   unsigned long flags;
> > +
> >
> 
> > +   if (!res)
> > +   return -EINVAL;
> 
> I don't remember the details, one version was quite changed, so, I think
> these lines are not needed anymore.
> 
Noted, these lines will be removed in next patch version (v12).
> > +   /* Get IO or MMIO BAR */
> > +   pci_bus_read_config_dword(pdev->bus, devfn, SBREG_BAR,
> > _addr);
> > +   if ((base_addr & PCI_BASE_ADDRESS_SPACE) ==
> > PCI_BASE_ADDRESS_SPACE_IO) {
> > +   flags = IORESOURCE_IO;
> > +   base64_addr = base_addr & PCI_BASE_ADDRESS_IO_MASK;
> > +   } else {
> > +   flags = IORESOURCE_MEM;
> > +   base64_addr = base_addr & PCI_BASE_ADDRESS_MEM_MASK;
> > +   if (base_addr & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +   flags |= IORESOURCE_MEM_64;
> >
> 
> > +   pci_bus_read_config_dword(pdev->bus, devfn,
> > +   SBREG_BAR + 4, _addr);
> 
> Fix indentation.
> 
Thanks for pointing that out. I will fix that in next patch version (v12). 
> > +   base64_addr |= (u64)base_addr << 32;
> > +   }
> > +   }
> > +
> > +   /* Hide the P2SB device */
> > +   pci_bus_write_config_byte(pdev->bus, devfn, SBREG_HIDE,
> > 0x01);
> > +
> > +   spin_unlock(_spinlock);
> > +
> 
> > +   /* User provides prefilled resources */
> 
> Not anymore as far I as I can see. You just return here the result.
> 
Current version is returning status of p2sb_bar function, i.e., 0 on success or 
appropriate errno value on error. Perhaps you could share the reason of return 
the result instead of status. 
> > +   res->start = (resource_size_t)base64_addr;
> > +   res->flags = flags;
> 
> --
> Andy Shevchenko 
> Intel Finland Oy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/8] power: supply: tps65217: Use 'poll_task' on unloading the module

[Milo Kim]

TPS65217 has two interrupt numbers so checking single IRQ number is not
appropriate when the module is removed.
Use the task_struct variable for running polling thread. If polling task
is activated, then use it to stop running thread.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 2000e59..55371d6 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -202,6 +202,7 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
struct tps65217_charger *charger;
struct power_supply_config cfg = {};
+   struct task_struct *poll_task;
int irq[NUM_CHARGER_IRQS];
int ret;
int i;
@@ -238,15 +239,16 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
 
/* Create a polling thread if an interrupt is invalid */
if (irq[0] < 0 || irq[1] < 0) {
-   charger->poll_task = kthread_run(tps65217_charger_poll_task,
-   charger, "ktps65217charger");
-   if (IS_ERR(charger->poll_task)) {
-   ret = PTR_ERR(charger->poll_task);
+   poll_task = kthread_run(tps65217_charger_poll_task,
+   charger, "ktps65217charger");
+   if (IS_ERR(poll_task)) {
+   ret = PTR_ERR(poll_task);
dev_err(charger->dev,
"Unable to run kthread err %d\n", ret);
return ret;
}
 
+   charger->poll_task = poll_task;
return 0;
}
 
@@ -274,7 +276,7 @@ static int tps65217_charger_remove(struct platform_device 
*pdev)
 {
struct tps65217_charger *charger = platform_get_drvdata(pdev);
 
-   if (charger->irq == -ENXIO)
+   if (charger->poll_task)
kthread_stop(charger->poll_task);
 
return 0;
-- 
2.9.3

[PATCH v2 7/8] power: supply: tps65217: Use generic name for get_property()

[Milo Kim]

Rename it as tps65217_charger_get_property().

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 79afeca..63c5556 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -115,9 +115,9 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
return 0;
 }
 
-static int tps65217_ac_get_property(struct power_supply *psy,
-   enum power_supply_property psp,
-   union power_supply_propval *val)
+static int tps65217_charger_get_property(struct power_supply *psy,
+enum power_supply_property psp,
+union power_supply_propval *val)
 {
struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
@@ -190,7 +190,7 @@ static int tps65217_charger_poll_task(void *data)
 static const struct power_supply_desc tps65217_charger_desc = {
.name   = "tps65217-ac",
.type   = POWER_SUPPLY_TYPE_MAINS,
-   .get_property   = tps65217_ac_get_property,
+   .get_property   = tps65217_charger_get_property,
.properties = tps65217_charger_props,
.num_properties = ARRAY_SIZE(tps65217_charger_props),
 };
-- 
2.9.3

[PATCH v2 5/8] power: supply: tps65217: Use generic name for power supply structure

[Milo Kim]

Replace 'ac' of tps65217_charger structure with 'psy'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 424a6d3..5daf361 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -42,7 +42,7 @@
 struct tps65217_charger {
struct tps65217 *tps;
struct device *dev;
-   struct power_supply *ac;
+   struct power_supply *psy;
 
int online;
int prev_online;
@@ -157,7 +157,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
}
 
if (charger->prev_online != charger->online)
-   power_supply_changed(charger->ac);
+   power_supply_changed(charger->psy);
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, );
if (ret < 0) {
@@ -218,12 +218,12 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
cfg.of_node = pdev->dev.of_node;
cfg.drv_data = charger;
 
-   charger->ac = devm_power_supply_register(>dev,
-_charger_desc,
-);
-   if (IS_ERR(charger->ac)) {
+   charger->psy = devm_power_supply_register(>dev,
+ _charger_desc,
+ );
+   if (IS_ERR(charger->psy)) {
dev_err(>dev, "failed: power supply register\n");
-   return PTR_ERR(charger->ac);
+   return PTR_ERR(charger->psy);
}
 
irq[0] = platform_get_irq_byname(pdev, "USB");
-- 
2.9.3

[PATCH v2 2/8] power: supply: tps65217: Use 'poll_task' on unloading the module

[Milo Kim]

TPS65217 has two interrupt numbers so checking single IRQ number is not
appropriate when the module is removed.
Use the task_struct variable for running polling thread. If polling task
is activated, then use it to stop running thread.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 2000e59..55371d6 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -202,6 +202,7 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
struct tps65217_charger *charger;
struct power_supply_config cfg = {};
+   struct task_struct *poll_task;
int irq[NUM_CHARGER_IRQS];
int ret;
int i;
@@ -238,15 +239,16 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
 
/* Create a polling thread if an interrupt is invalid */
if (irq[0] < 0 || irq[1] < 0) {
-   charger->poll_task = kthread_run(tps65217_charger_poll_task,
-   charger, "ktps65217charger");
-   if (IS_ERR(charger->poll_task)) {
-   ret = PTR_ERR(charger->poll_task);
+   poll_task = kthread_run(tps65217_charger_poll_task,
+   charger, "ktps65217charger");
+   if (IS_ERR(poll_task)) {
+   ret = PTR_ERR(poll_task);
dev_err(charger->dev,
"Unable to run kthread err %d\n", ret);
return ret;
}
 
+   charger->poll_task = poll_task;
return 0;
}
 
@@ -274,7 +276,7 @@ static int tps65217_charger_remove(struct platform_device 
*pdev)
 {
struct tps65217_charger *charger = platform_get_drvdata(pdev);
 
-   if (charger->irq == -ENXIO)
+   if (charger->poll_task)
kthread_stop(charger->poll_task);
 
return 0;
-- 
2.9.3

[PATCH v2 7/8] power: supply: tps65217: Use generic name for get_property()

[Milo Kim]

Rename it as tps65217_charger_get_property().

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 79afeca..63c5556 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -115,9 +115,9 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
return 0;
 }
 
-static int tps65217_ac_get_property(struct power_supply *psy,
-   enum power_supply_property psp,
-   union power_supply_propval *val)
+static int tps65217_charger_get_property(struct power_supply *psy,
+enum power_supply_property psp,
+union power_supply_propval *val)
 {
struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
@@ -190,7 +190,7 @@ static int tps65217_charger_poll_task(void *data)
 static const struct power_supply_desc tps65217_charger_desc = {
.name   = "tps65217-ac",
.type   = POWER_SUPPLY_TYPE_MAINS,
-   .get_property   = tps65217_ac_get_property,
+   .get_property   = tps65217_charger_get_property,
.properties = tps65217_charger_props,
.num_properties = ARRAY_SIZE(tps65217_charger_props),
 };
-- 
2.9.3

[PATCH v2 5/8] power: supply: tps65217: Use generic name for power supply structure

[Milo Kim]

Replace 'ac' of tps65217_charger structure with 'psy'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 424a6d3..5daf361 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -42,7 +42,7 @@
 struct tps65217_charger {
struct tps65217 *tps;
struct device *dev;
-   struct power_supply *ac;
+   struct power_supply *psy;
 
int online;
int prev_online;
@@ -157,7 +157,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
}
 
if (charger->prev_online != charger->online)
-   power_supply_changed(charger->ac);
+   power_supply_changed(charger->psy);
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, );
if (ret < 0) {
@@ -218,12 +218,12 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
cfg.of_node = pdev->dev.of_node;
cfg.drv_data = charger;
 
-   charger->ac = devm_power_supply_register(>dev,
-_charger_desc,
-);
-   if (IS_ERR(charger->ac)) {
+   charger->psy = devm_power_supply_register(>dev,
+ _charger_desc,
+ );
+   if (IS_ERR(charger->psy)) {
dev_err(>dev, "failed: power supply register\n");
-   return PTR_ERR(charger->ac);
+   return PTR_ERR(charger->psy);
}
 
irq[0] = platform_get_irq_byname(pdev, "USB");
-- 
2.9.3

[PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

[Milo Kim]

TPS65217 device supports two charger inputs - AC and USB.
Currently, only AC charger is supported. This patch-set adds USB charger 
feature. Tested on Beaglebone black.

Patch 1: Main patch
Patch 2, 3: Clean up for charger driver data
Patch 4 ~ 8: Naming changes for generic power supply class structure

v2:
  Regenerate the patchset for better code review

Milo Kim (8):
  power: supply: tps65217: Support USB charger interrupt
  power: supply: tps65217: Use 'poll_task' on unloading the module
  power: supply: tps65217: Remove IRQ data from driver data
  power: supply: tps65217: Use generic name for charger online
  power: supply: tps65217: Use generic name for power supply structure
  power: supply: tps65217: Use generic name for power supply property
  power: supply: tps65217: Use generic name for get_property()
  power: supply: tps65217: Use generic charger name

 drivers/power/supply/tps65217_charger.c | 99 ++---
 1 file changed, 53 insertions(+), 46 deletions(-)

-- 
2.9.3

[PATCH v2 8/8] power: supply: tps65217: Use generic charger name

[Milo Kim]

"tps65217-charger" is more appropriate name because the driver supports
not only AC but also USB charger.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 63c5556..29b61e8 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -188,7 +188,7 @@ static int tps65217_charger_poll_task(void *data)
 }
 
 static const struct power_supply_desc tps65217_charger_desc = {
-   .name   = "tps65217-ac",
+   .name   = "tps65217-charger",
.type   = POWER_SUPPLY_TYPE_MAINS,
.get_property   = tps65217_charger_get_property,
.properties = tps65217_charger_props,
-- 
2.9.3

[PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

[Milo Kim]

TPS65217 device supports two charger inputs - AC and USB.
Currently, only AC charger is supported. This patch-set adds USB charger 
feature. Tested on Beaglebone black.

Patch 1: Main patch
Patch 2, 3: Clean up for charger driver data
Patch 4 ~ 8: Naming changes for generic power supply class structure

v2:
  Regenerate the patchset for better code review

Milo Kim (8):
  power: supply: tps65217: Support USB charger interrupt
  power: supply: tps65217: Use 'poll_task' on unloading the module
  power: supply: tps65217: Remove IRQ data from driver data
  power: supply: tps65217: Use generic name for charger online
  power: supply: tps65217: Use generic name for power supply structure
  power: supply: tps65217: Use generic name for power supply property
  power: supply: tps65217: Use generic name for get_property()
  power: supply: tps65217: Use generic charger name

 drivers/power/supply/tps65217_charger.c | 99 ++---
 1 file changed, 53 insertions(+), 46 deletions(-)

-- 
2.9.3

[PATCH v2 8/8] power: supply: tps65217: Use generic charger name

[Milo Kim]

"tps65217-charger" is more appropriate name because the driver supports
not only AC but also USB charger.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 63c5556..29b61e8 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -188,7 +188,7 @@ static int tps65217_charger_poll_task(void *data)
 }
 
 static const struct power_supply_desc tps65217_charger_desc = {
-   .name   = "tps65217-ac",
+   .name   = "tps65217-charger",
.type   = POWER_SUPPLY_TYPE_MAINS,
.get_property   = tps65217_charger_get_property,
.properties = tps65217_charger_props,
-- 
2.9.3

[PATCH v2 1/8] power: supply: tps65217: Support USB charger interrupt

[Milo Kim]

TPS65217 has two charger interrupts - AC or USB power status change.

Interrupt handler:
  Check not only AC but also USB charger status.
  In both cases, enable charging operation.

Interrupt request:
  If an interrupt number is invalid, then use legacy polling thread.
  Otherwise, create IRQ threads to handle AC and USB charger event.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 47 +++--
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 9fd019f..2000e59 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -35,6 +35,8 @@
 #include 
 #include 
 
+#define CHARGER_STATUS_PRESENT (TPS65217_STATUS_ACPWR | TPS65217_STATUS_USBPWR)
+#define NUM_CHARGER_IRQS   2
 #define POLL_INTERVAL  (HZ * 2)
 
 struct tps65217_charger {
@@ -144,8 +146,8 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 
dev_dbg(charger->dev, "%s: 0x%x\n", __func__, val);
 
-   /* check for AC status bit */
-   if (val & TPS65217_STATUS_ACPWR) {
+   /* check for charger status bit */
+   if (val & CHARGER_STATUS_PRESENT) {
ret = tps65217_enable_charging(charger);
if (ret) {
dev_err(charger->dev,
@@ -200,8 +202,9 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
struct tps65217_charger *charger;
struct power_supply_config cfg = {};
-   int irq;
+   int irq[NUM_CHARGER_IRQS];
int ret;
+   int i;
 
dev_dbg(>dev, "%s\n", __func__);
 
@@ -224,10 +227,8 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
return PTR_ERR(charger->ac);
}
 
-   irq = platform_get_irq_byname(pdev, "AC");
-   if (irq < 0)
-   irq = -ENXIO;
-   charger->irq = irq;
+   irq[0] = platform_get_irq_byname(pdev, "USB");
+   irq[1] = platform_get_irq_byname(pdev, "AC");
 
ret = tps65217_config_charger(charger);
if (ret < 0) {
@@ -235,29 +236,35 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
return ret;
}
 
-   if (irq != -ENXIO) {
-   ret = devm_request_threaded_irq(>dev, irq, NULL,
+   /* Create a polling thread if an interrupt is invalid */
+   if (irq[0] < 0 || irq[1] < 0) {
+   charger->poll_task = kthread_run(tps65217_charger_poll_task,
+   charger, "ktps65217charger");
+   if (IS_ERR(charger->poll_task)) {
+   ret = PTR_ERR(charger->poll_task);
+   dev_err(charger->dev,
+   "Unable to run kthread err %d\n", ret);
+   return ret;
+   }
+
+   return 0;
+   }
+
+   /* Create IRQ threads for charger interrupts */
+   for (i = 0; i < NUM_CHARGER_IRQS; i++) {
+   ret = devm_request_threaded_irq(>dev, irq[i], NULL,
tps65217_charger_irq,
0, "tps65217-charger",
charger);
if (ret) {
dev_err(charger->dev,
-   "Unable to register irq %d err %d\n", irq,
+   "Unable to register irq %d err %d\n", irq[i],
ret);
return ret;
}
 
/* Check current state */
-   tps65217_charger_irq(irq, charger);
-   } else {
-   charger->poll_task = kthread_run(tps65217_charger_poll_task,
-   charger, "ktps65217charger");
-   if (IS_ERR(charger->poll_task)) {
-   ret = PTR_ERR(charger->poll_task);
-   dev_err(charger->dev,
-   "Unable to run kthread err %d\n", ret);
-   return ret;
-   }
+   tps65217_charger_irq(-1, charger);
}
 
return 0;
-- 
2.9.3

[PATCH v2 4/8] power: supply: tps65217: Use generic name for charger online

[Milo Kim]

This driver supports AC and USB chargers. Generic name is preferred.
Replace 'ac_online' with 'online'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 482ee9f..424a6d3 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -44,8 +44,8 @@ struct tps65217_charger {
struct device *dev;
struct power_supply *ac;
 
-   int ac_online;
-   int prev_ac_online;
+   int online;
+   int prev_online;
 
struct task_struct  *poll_task;
 };
@@ -95,7 +95,7 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
int ret;
 
/* charger already enabled */
-   if (charger->ac_online)
+   if (charger->online)
return 0;
 
dev_dbg(charger->dev, "%s: enable charging\n", __func__);
@@ -110,7 +110,7 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
return ret;
}
 
-   charger->ac_online = 1;
+   charger->online = 1;
 
return 0;
 }
@@ -122,7 +122,7 @@ static int tps65217_ac_get_property(struct power_supply 
*psy,
struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
if (psp == POWER_SUPPLY_PROP_ONLINE) {
-   val->intval = charger->ac_online;
+   val->intval = charger->online;
return 0;
}
return -EINVAL;
@@ -133,7 +133,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
int ret, val;
struct tps65217_charger *charger = dev;
 
-   charger->prev_ac_online = charger->ac_online;
+   charger->prev_online = charger->online;
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_STATUS, );
if (ret < 0) {
@@ -153,10 +153,10 @@ static irqreturn_t tps65217_charger_irq(int irq, void 
*dev)
return IRQ_HANDLED;
}
} else {
-   charger->ac_online = 0;
+   charger->online = 0;
}
 
-   if (charger->prev_ac_online != charger->ac_online)
+   if (charger->prev_online != charger->online)
power_supply_changed(charger->ac);
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, );
-- 
2.9.3

[PATCH v2 3/8] power: supply: tps65217: Remove IRQ data from driver data

[Milo Kim]

IRQ number is only used on requesting the interrupt, so no need to keep
it inside the driver data.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 55371d6..482ee9f 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -48,8 +48,6 @@ struct tps65217_charger {
int prev_ac_online;
 
struct task_struct  *poll_task;
-
-   int irq;
 };
 
 static enum power_supply_property tps65217_ac_props[] = {
-- 
2.9.3

[PATCH v2 6/8] power: supply: tps65217: Use generic name for power supply property

[Milo Kim]

Replace 'ac_props' with 'charger_props'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 5daf361..79afeca 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -50,7 +50,7 @@ struct tps65217_charger {
struct task_struct  *poll_task;
 };
 
-static enum power_supply_property tps65217_ac_props[] = {
+static enum power_supply_property tps65217_charger_props[] = {
POWER_SUPPLY_PROP_ONLINE,
 };
 
@@ -191,8 +191,8 @@ static const struct power_supply_desc tps65217_charger_desc 
= {
.name   = "tps65217-ac",
.type   = POWER_SUPPLY_TYPE_MAINS,
.get_property   = tps65217_ac_get_property,
-   .properties = tps65217_ac_props,
-   .num_properties = ARRAY_SIZE(tps65217_ac_props),
+   .properties = tps65217_charger_props,
+   .num_properties = ARRAY_SIZE(tps65217_charger_props),
 };
 
 static int tps65217_charger_probe(struct platform_device *pdev)
-- 
2.9.3

[PATCH v2 1/8] power: supply: tps65217: Support USB charger interrupt

[Milo Kim]

TPS65217 has two charger interrupts - AC or USB power status change.

Interrupt handler:
  Check not only AC but also USB charger status.
  In both cases, enable charging operation.

Interrupt request:
  If an interrupt number is invalid, then use legacy polling thread.
  Otherwise, create IRQ threads to handle AC and USB charger event.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 47 +++--
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 9fd019f..2000e59 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -35,6 +35,8 @@
 #include 
 #include 
 
+#define CHARGER_STATUS_PRESENT (TPS65217_STATUS_ACPWR | TPS65217_STATUS_USBPWR)
+#define NUM_CHARGER_IRQS   2
 #define POLL_INTERVAL  (HZ * 2)
 
 struct tps65217_charger {
@@ -144,8 +146,8 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 
dev_dbg(charger->dev, "%s: 0x%x\n", __func__, val);
 
-   /* check for AC status bit */
-   if (val & TPS65217_STATUS_ACPWR) {
+   /* check for charger status bit */
+   if (val & CHARGER_STATUS_PRESENT) {
ret = tps65217_enable_charging(charger);
if (ret) {
dev_err(charger->dev,
@@ -200,8 +202,9 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
struct tps65217_charger *charger;
struct power_supply_config cfg = {};
-   int irq;
+   int irq[NUM_CHARGER_IRQS];
int ret;
+   int i;
 
dev_dbg(>dev, "%s\n", __func__);
 
@@ -224,10 +227,8 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
return PTR_ERR(charger->ac);
}
 
-   irq = platform_get_irq_byname(pdev, "AC");
-   if (irq < 0)
-   irq = -ENXIO;
-   charger->irq = irq;
+   irq[0] = platform_get_irq_byname(pdev, "USB");
+   irq[1] = platform_get_irq_byname(pdev, "AC");
 
ret = tps65217_config_charger(charger);
if (ret < 0) {
@@ -235,29 +236,35 @@ static int tps65217_charger_probe(struct platform_device 
*pdev)
return ret;
}
 
-   if (irq != -ENXIO) {
-   ret = devm_request_threaded_irq(>dev, irq, NULL,
+   /* Create a polling thread if an interrupt is invalid */
+   if (irq[0] < 0 || irq[1] < 0) {
+   charger->poll_task = kthread_run(tps65217_charger_poll_task,
+   charger, "ktps65217charger");
+   if (IS_ERR(charger->poll_task)) {
+   ret = PTR_ERR(charger->poll_task);
+   dev_err(charger->dev,
+   "Unable to run kthread err %d\n", ret);
+   return ret;
+   }
+
+   return 0;
+   }
+
+   /* Create IRQ threads for charger interrupts */
+   for (i = 0; i < NUM_CHARGER_IRQS; i++) {
+   ret = devm_request_threaded_irq(>dev, irq[i], NULL,
tps65217_charger_irq,
0, "tps65217-charger",
charger);
if (ret) {
dev_err(charger->dev,
-   "Unable to register irq %d err %d\n", irq,
+   "Unable to register irq %d err %d\n", irq[i],
ret);
return ret;
}
 
/* Check current state */
-   tps65217_charger_irq(irq, charger);
-   } else {
-   charger->poll_task = kthread_run(tps65217_charger_poll_task,
-   charger, "ktps65217charger");
-   if (IS_ERR(charger->poll_task)) {
-   ret = PTR_ERR(charger->poll_task);
-   dev_err(charger->dev,
-   "Unable to run kthread err %d\n", ret);
-   return ret;
-   }
+   tps65217_charger_irq(-1, charger);
}
 
return 0;
-- 
2.9.3

[PATCH v2 4/8] power: supply: tps65217: Use generic name for charger online

[Milo Kim]

This driver supports AC and USB chargers. Generic name is preferred.
Replace 'ac_online' with 'online'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 482ee9f..424a6d3 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -44,8 +44,8 @@ struct tps65217_charger {
struct device *dev;
struct power_supply *ac;
 
-   int ac_online;
-   int prev_ac_online;
+   int online;
+   int prev_online;
 
struct task_struct  *poll_task;
 };
@@ -95,7 +95,7 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
int ret;
 
/* charger already enabled */
-   if (charger->ac_online)
+   if (charger->online)
return 0;
 
dev_dbg(charger->dev, "%s: enable charging\n", __func__);
@@ -110,7 +110,7 @@ static int tps65217_enable_charging(struct tps65217_charger 
*charger)
return ret;
}
 
-   charger->ac_online = 1;
+   charger->online = 1;
 
return 0;
 }
@@ -122,7 +122,7 @@ static int tps65217_ac_get_property(struct power_supply 
*psy,
struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
if (psp == POWER_SUPPLY_PROP_ONLINE) {
-   val->intval = charger->ac_online;
+   val->intval = charger->online;
return 0;
}
return -EINVAL;
@@ -133,7 +133,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
int ret, val;
struct tps65217_charger *charger = dev;
 
-   charger->prev_ac_online = charger->ac_online;
+   charger->prev_online = charger->online;
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_STATUS, );
if (ret < 0) {
@@ -153,10 +153,10 @@ static irqreturn_t tps65217_charger_irq(int irq, void 
*dev)
return IRQ_HANDLED;
}
} else {
-   charger->ac_online = 0;
+   charger->online = 0;
}
 
-   if (charger->prev_ac_online != charger->ac_online)
+   if (charger->prev_online != charger->online)
power_supply_changed(charger->ac);
 
ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, );
-- 
2.9.3

[PATCH v2 3/8] power: supply: tps65217: Remove IRQ data from driver data

[Milo Kim]

IRQ number is only used on requesting the interrupt, so no need to keep
it inside the driver data.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 55371d6..482ee9f 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -48,8 +48,6 @@ struct tps65217_charger {
int prev_ac_online;
 
struct task_struct  *poll_task;
-
-   int irq;
 };
 
 static enum power_supply_property tps65217_ac_props[] = {
-- 
2.9.3

[PATCH v2 6/8] power: supply: tps65217: Use generic name for power supply property

[Milo Kim]

Replace 'ac_props' with 'charger_props'.

Signed-off-by: Milo Kim 
---
 drivers/power/supply/tps65217_charger.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c 
b/drivers/power/supply/tps65217_charger.c
index 5daf361..79afeca 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -50,7 +50,7 @@ struct tps65217_charger {
struct task_struct  *poll_task;
 };
 
-static enum power_supply_property tps65217_ac_props[] = {
+static enum power_supply_property tps65217_charger_props[] = {
POWER_SUPPLY_PROP_ONLINE,
 };
 
@@ -191,8 +191,8 @@ static const struct power_supply_desc tps65217_charger_desc 
= {
.name   = "tps65217-ac",
.type   = POWER_SUPPLY_TYPE_MAINS,
.get_property   = tps65217_ac_get_property,
-   .properties = tps65217_ac_props,
-   .num_properties = ARRAY_SIZE(tps65217_ac_props),
+   .properties = tps65217_charger_props,
+   .num_properties = ARRAY_SIZE(tps65217_charger_props),
 };
 
 static int tps65217_charger_probe(struct platform_device *pdev)
-- 
2.9.3

Re: [PATCH] vfio/pci: Support error recovery

[Cao jin]

On 12/09/2016 12:30 AM, Michael S. Tsirkin wrote:
> On Thu, Dec 08, 2016 at 10:46:59PM +0800, Cao jin wrote:
>>
>>
>> On 12/06/2016 11:35 PM, Alex Williamson wrote:
>>> On Tue, 6 Dec 2016 18:46:04 +0800
>>> Cao jin  wrote:
>>>
 On 12/06/2016 12:59 PM, Alex Williamson wrote:
> On Tue, 6 Dec 2016 05:55:28 +0200
> "Michael S. Tsirkin"  wrote:
>   
>> On Mon, Dec 05, 2016 at 09:17:30AM -0700, Alex Williamson wrote:  
>>> If you're going to take the lead for these AER patches, I would
>>> certainly suggest that understanding the reasoning behind the bus reset
>>> behavior is a central aspect to this series.  This effort has dragged
>>> out for nearly two years and I apologize, but I don't really have a lot
>>> of patience for rehashing some of these issues if you're not going to
>>> read the previous discussions or consult with your colleagues to
>>> understand how we got to this point.  If you want to challenge some of
>>> the design points, that's great, it could use some new eyes, but please
>>> understand how we got here first.
>>
>> Well I'm guessing Cao jin here isn't the only one not
>> willing to plough through all historical versions of the patchset
>> just to figure out the motivation for some code.
>>
>> Including a summary of a high level architecture couldn't hurt.
>>
>> Any chance of writing such?  Alternatively, we can try to build it as
>> part of this thread.  Shouldn't be hard as it seems somewhat
>> straight-forward on the surface:
>>
>> - detect link error on the host, don't reset link as we would normally 
>> do  
>
> This is actually a new approach that I'm not sure I agree with.  By
> skipping the host directed link reset, vfio is taking responsibility
> for doing this, but then we just assume the user will do it.  I have
> issues with this.
>
> The previous approach was to use the error detected notifier to block
> access to the device, allowing the host to perform the link reset.  A
> subsequent notification in the AER process released the user access
> which allowed the user AER process to proceed.  This did result in both
> a host directed and a guest directed link reset, but other than
> coordinating the blocking of the user process during host reset, that
> hasn't been brought up as an issue previously.
>   

 Tests on previous versions didn't bring up issues as I find, I think
 that is because we didn't test it completely. As I know, before August
 of this year, we didn't have cable connected to NIC, let alone
 connecting NIC to gateway.
>>>
>>> Lack of testing has been a significant issue throughout the development
>>> of this series.
>>>
 Even if I fixed the guest oops issue in igb driver that Alex found in
 v9, v9 still cannot work in my test. And in my test, disable link
 reset(in host) in aer core for vfio-pci is the most significant step to
 get my test passed.
>>>
>>> But is it the correct step?  I'm not convinced.  Why did blocking guest
>>> access not work?  How do you plan to manage vfio taking the
>>> responsibility to perform a bus reset when you don't know whether QEMU
>>> is the user of the device or whether the user supports AER recovery?
>>>  
>>
>> Maybe currently we don't have enough proof to prove the correctness, but
>> I think I did find some facts to prove that link reset in host is a big
>> trouble, and can answer part of questions above.
>>
>> 1st, some thoughts:
>> In pci-error-recovery.txt and do_recovery() of kernel tree, we can see,
>> a recovery consists of several steps(callbacks), link reset is one of
>> them, and except link reset, the others are seems kind of device
>> specific. In our case, both host & guest will do recovery, I think the
>> host recovery actually is some kind of fake recovery, see vfio-pci
>> driver's error_detected & resume callback, they don't do anything
>> special, mainly signal error to user, but the link reset in host "fake
>> reset" does some serious work, in other words, I think host does the
>> recovery incompletely, so I was thinking, why not just drop incompletely
>> host recovery(drop link reset) for vfio-pci, and let the guest take care
>> of the whole serious recovery.  This is part of the reason of why my
>> version looks like this.  But yes, I admit the issue Alex mentioned,
>> vfio can't guarantee that user will do a bus reset, this is an issue I
>> will keep looking for a solution.
>>
>> 2nd, some facts and analyzation from test:
>> In host, the relationship between time and behviour in each component
>> roughly looks as following:
>>
>>  +   HW+  host kernel   + qemu  + guest kernel  +
>>  | |(error recovery)|   |   |
>>  | ||   |   |
>>  | | vfio-pci's  

Re: [PATCH] vfio/pci: Support error recovery

[Cao jin]

On 12/09/2016 12:30 AM, Michael S. Tsirkin wrote:
> On Thu, Dec 08, 2016 at 10:46:59PM +0800, Cao jin wrote:
>>
>>
>> On 12/06/2016 11:35 PM, Alex Williamson wrote:
>>> On Tue, 6 Dec 2016 18:46:04 +0800
>>> Cao jin  wrote:
>>>
 On 12/06/2016 12:59 PM, Alex Williamson wrote:
> On Tue, 6 Dec 2016 05:55:28 +0200
> "Michael S. Tsirkin"  wrote:
>   
>> On Mon, Dec 05, 2016 at 09:17:30AM -0700, Alex Williamson wrote:  
>>> If you're going to take the lead for these AER patches, I would
>>> certainly suggest that understanding the reasoning behind the bus reset
>>> behavior is a central aspect to this series.  This effort has dragged
>>> out for nearly two years and I apologize, but I don't really have a lot
>>> of patience for rehashing some of these issues if you're not going to
>>> read the previous discussions or consult with your colleagues to
>>> understand how we got to this point.  If you want to challenge some of
>>> the design points, that's great, it could use some new eyes, but please
>>> understand how we got here first.
>>
>> Well I'm guessing Cao jin here isn't the only one not
>> willing to plough through all historical versions of the patchset
>> just to figure out the motivation for some code.
>>
>> Including a summary of a high level architecture couldn't hurt.
>>
>> Any chance of writing such?  Alternatively, we can try to build it as
>> part of this thread.  Shouldn't be hard as it seems somewhat
>> straight-forward on the surface:
>>
>> - detect link error on the host, don't reset link as we would normally 
>> do  
>
> This is actually a new approach that I'm not sure I agree with.  By
> skipping the host directed link reset, vfio is taking responsibility
> for doing this, but then we just assume the user will do it.  I have
> issues with this.
>
> The previous approach was to use the error detected notifier to block
> access to the device, allowing the host to perform the link reset.  A
> subsequent notification in the AER process released the user access
> which allowed the user AER process to proceed.  This did result in both
> a host directed and a guest directed link reset, but other than
> coordinating the blocking of the user process during host reset, that
> hasn't been brought up as an issue previously.
>   

 Tests on previous versions didn't bring up issues as I find, I think
 that is because we didn't test it completely. As I know, before August
 of this year, we didn't have cable connected to NIC, let alone
 connecting NIC to gateway.
>>>
>>> Lack of testing has been a significant issue throughout the development
>>> of this series.
>>>
 Even if I fixed the guest oops issue in igb driver that Alex found in
 v9, v9 still cannot work in my test. And in my test, disable link
 reset(in host) in aer core for vfio-pci is the most significant step to
 get my test passed.
>>>
>>> But is it the correct step?  I'm not convinced.  Why did blocking guest
>>> access not work?  How do you plan to manage vfio taking the
>>> responsibility to perform a bus reset when you don't know whether QEMU
>>> is the user of the device or whether the user supports AER recovery?
>>>  
>>
>> Maybe currently we don't have enough proof to prove the correctness, but
>> I think I did find some facts to prove that link reset in host is a big
>> trouble, and can answer part of questions above.
>>
>> 1st, some thoughts:
>> In pci-error-recovery.txt and do_recovery() of kernel tree, we can see,
>> a recovery consists of several steps(callbacks), link reset is one of
>> them, and except link reset, the others are seems kind of device
>> specific. In our case, both host & guest will do recovery, I think the
>> host recovery actually is some kind of fake recovery, see vfio-pci
>> driver's error_detected & resume callback, they don't do anything
>> special, mainly signal error to user, but the link reset in host "fake
>> reset" does some serious work, in other words, I think host does the
>> recovery incompletely, so I was thinking, why not just drop incompletely
>> host recovery(drop link reset) for vfio-pci, and let the guest take care
>> of the whole serious recovery.  This is part of the reason of why my
>> version looks like this.  But yes, I admit the issue Alex mentioned,
>> vfio can't guarantee that user will do a bus reset, this is an issue I
>> will keep looking for a solution.
>>
>> 2nd, some facts and analyzation from test:
>> In host, the relationship between time and behviour in each component
>> roughly looks as following:
>>
>>  +   HW+  host kernel   + qemu  + guest kernel  +
>>  | |(error recovery)|   |   |
>>  | ||   |   |
>>  | | vfio-pci's |   |   |
>>

Re: [PATCH] vfio/pci: Support error recovery

[Cao jin]

On 12/08/2016 10:46 PM, Cao jin wrote:
> 
> 
> On 12/06/2016 11:35 PM, Alex Williamson wrote:
>> On Tue, 6 Dec 2016 18:46:04 +0800
>> Cao jin  wrote:
>>
>>> On 12/06/2016 12:59 PM, Alex Williamson wrote:
 On Tue, 6 Dec 2016 05:55:28 +0200
 "Michael S. Tsirkin"  wrote:
   
> On Mon, Dec 05, 2016 at 09:17:30AM -0700, Alex Williamson wrote:  
>> If you're going to take the lead for these AER patches, I would
>> certainly suggest that understanding the reasoning behind the bus reset
>> behavior is a central aspect to this series.  This effort has dragged
>> out for nearly two years and I apologize, but I don't really have a lot
>> of patience for rehashing some of these issues if you're not going to
>> read the previous discussions or consult with your colleagues to
>> understand how we got to this point.  If you want to challenge some of
>> the design points, that's great, it could use some new eyes, but please
>> understand how we got here first.
>
> Well I'm guessing Cao jin here isn't the only one not
> willing to plough through all historical versions of the patchset
> just to figure out the motivation for some code.
>
> Including a summary of a high level architecture couldn't hurt.
>
> Any chance of writing such?  Alternatively, we can try to build it as
> part of this thread.  Shouldn't be hard as it seems somewhat
> straight-forward on the surface:
>
> - detect link error on the host, don't reset link as we would normally do 
>  

 This is actually a new approach that I'm not sure I agree with.  By
 skipping the host directed link reset, vfio is taking responsibility
 for doing this, but then we just assume the user will do it.  I have
 issues with this.

 The previous approach was to use the error detected notifier to block
 access to the device, allowing the host to perform the link reset.  A
 subsequent notification in the AER process released the user access
 which allowed the user AER process to proceed.  This did result in both
 a host directed and a guest directed link reset, but other than
 coordinating the blocking of the user process during host reset, that
 hasn't been brought up as an issue previously.
   
>>>
>>> Tests on previous versions didn't bring up issues as I find, I think
>>> that is because we didn't test it completely. As I know, before August
>>> of this year, we didn't have cable connected to NIC, let alone
>>> connecting NIC to gateway.
>>
>> Lack of testing has been a significant issue throughout the development
>> of this series.
>>
>>> Even if I fixed the guest oops issue in igb driver that Alex found in
>>> v9, v9 still cannot work in my test. And in my test, disable link
>>> reset(in host) in aer core for vfio-pci is the most significant step to
>>> get my test passed.
>>
>> But is it the correct step?  I'm not convinced.  Why did blocking guest
>> access not work?  How do you plan to manage vfio taking the
>> responsibility to perform a bus reset when you don't know whether QEMU
>> is the user of the device or whether the user supports AER recovery?
>>  
> 
> Maybe currently we don't have enough proof to prove the correctness, but
> I think I did find some facts to prove that link reset in host is a big
> trouble, and can answer part of questions above.
> 
> 1st, some thoughts:
> In pci-error-recovery.txt and do_recovery() of kernel tree, we can see,
> a recovery consists of several steps(callbacks), link reset is one of
> them, and except link reset, the others are seems kind of device
> specific. In our case, both host & guest will do recovery, I think the
> host recovery actually is some kind of fake recovery, see vfio-pci
> driver's error_detected & resume callback, they don't do anything
> special, mainly signal error to user, but the link reset in host "fake
> reset" does some serious work, in other words, I think host does the
> recovery incompletely, so I was thinking, why not just drop incompletely
> host recovery(drop link reset) for vfio-pci, and let the guest take care
> of the whole serious recovery.  This is part of the reason of why my
> version looks like this.  But yes, I admit the issue Alex mentioned,
> vfio can't guarantee that user will do a bus reset, this is an issue I
> will keep looking for a solution.
> 
> 2nd, some facts and analyzation from test:
> In host, the relationship between time and behviour in each component
> roughly looks as following:
> 
>  +   HW+  host kernel   + qemu  + guest kernel  +
>  | |(error recovery)|   |   |
>  | ||   |   |
>  | | vfio-pci's |   |   |
>  | | error_detected |   |   |
>  | | +  |   |   

Re: [PATCH] pci-error-recover: doc cleanup

[Cao jin]

On 12/09/2016 02:24 PM, Linas Vepstas wrote:
> I suppose I'm confused, but I recall that link resets are non-fatal.
> Fatal errors typically require that the the pci adapter be completely
> reset, any adapter firmware to be reloaded from scratch, the device
> driver has to kill all device state and start from scratch. Its huge.
> If the fatal error is on pci device that is under a block device
> holding a file system, then (usually) there is no way to recover,
> because the block layer (and file system) cannot deal with a block
> device that disappeared and then reappeared some few seconds later.
> (maybe some future zfs or lvm or btrfs might be able to deal with
> this, but not today)
> 
> By contrast, link resets are far more gentle: the device driver might
> have to discard some half-full FIFO's, or cancel some in-flight
> commands, but can otherwise gracefully recover without telling the
> higher layers that there were any problems.
> 
> --linas
> 

I am little confused too, even not sure if we are talking the same
*fatal error*, I am talking the fatal error defined in PCI Express spec,
chapter 6.2.2.2.1:

Fatal errors are uncorrectable error conditions which render the
particular Link and related hardware unreliable. For Fatal errors, a
reset of the components on the Link may be required to return to
reliable operation. Platform handling of Fatal errors, and any efforts
to limit the effects of these errors, is platform implementation specific.

Link reset means set *secondary bus reset* bit in pci bridge config
space, can reset the link and device simultaneously, is the strongest
kind of reset as I know.

> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:
>>
>>
>> On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
>>> On Thu, 8 Dec 2016 16:16:14 +0800
>>> Cao jin  wrote:
>>>
  The platform resets the link, and then calls the link_reset() callback
  on all affected device drivers.  This is a PCI-Express specific state
 -and is done whenever a non-fatal error has been detected that can be
 +and is done whenever a fatal error has been detected that can be
  "solved" by resetting the link. This call informs the driver of the
>>>
>>> As far as I can tell, the original text was correct here; why do you
>>> think this change needs to be made?
>>>
>>
>> See do_recovery() in aer core, reset_link() is called only seeing fatal
>> error.
>>
>> --
>> Sincerely,
>> Cao jin
>>
>>
> 
> 
> 

-- 
Sincerely,
Cao jin

Re: [PATCH] pci-error-recover: doc cleanup

[Cao jin]

On 12/09/2016 02:24 PM, Linas Vepstas wrote:
> I suppose I'm confused, but I recall that link resets are non-fatal.
> Fatal errors typically require that the the pci adapter be completely
> reset, any adapter firmware to be reloaded from scratch, the device
> driver has to kill all device state and start from scratch. Its huge.
> If the fatal error is on pci device that is under a block device
> holding a file system, then (usually) there is no way to recover,
> because the block layer (and file system) cannot deal with a block
> device that disappeared and then reappeared some few seconds later.
> (maybe some future zfs or lvm or btrfs might be able to deal with
> this, but not today)
> 
> By contrast, link resets are far more gentle: the device driver might
> have to discard some half-full FIFO's, or cancel some in-flight
> commands, but can otherwise gracefully recover without telling the
> higher layers that there were any problems.
> 
> --linas
> 

I am little confused too, even not sure if we are talking the same
*fatal error*, I am talking the fatal error defined in PCI Express spec,
chapter 6.2.2.2.1:

Fatal errors are uncorrectable error conditions which render the
particular Link and related hardware unreliable. For Fatal errors, a
reset of the components on the Link may be required to return to
reliable operation. Platform handling of Fatal errors, and any efforts
to limit the effects of these errors, is platform implementation specific.

Link reset means set *secondary bus reset* bit in pci bridge config
space, can reset the link and device simultaneously, is the strongest
kind of reset as I know.

> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:
>>
>>
>> On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
>>> On Thu, 8 Dec 2016 16:16:14 +0800
>>> Cao jin  wrote:
>>>
  The platform resets the link, and then calls the link_reset() callback
  on all affected device drivers.  This is a PCI-Express specific state
 -and is done whenever a non-fatal error has been detected that can be
 +and is done whenever a fatal error has been detected that can be
  "solved" by resetting the link. This call informs the driver of the
>>>
>>> As far as I can tell, the original text was correct here; why do you
>>> think this change needs to be made?
>>>
>>
>> See do_recovery() in aer core, reset_link() is called only seeing fatal
>> error.
>>
>> --
>> Sincerely,
>> Cao jin
>>
>>
> 
> 
> 

-- 
Sincerely,
Cao jin

Re: [PATCH] vfio/pci: Support error recovery

[Cao jin]

On 12/08/2016 10:46 PM, Cao jin wrote:
> 
> 
> On 12/06/2016 11:35 PM, Alex Williamson wrote:
>> On Tue, 6 Dec 2016 18:46:04 +0800
>> Cao jin  wrote:
>>
>>> On 12/06/2016 12:59 PM, Alex Williamson wrote:
 On Tue, 6 Dec 2016 05:55:28 +0200
 "Michael S. Tsirkin"  wrote:
   
> On Mon, Dec 05, 2016 at 09:17:30AM -0700, Alex Williamson wrote:  
>> If you're going to take the lead for these AER patches, I would
>> certainly suggest that understanding the reasoning behind the bus reset
>> behavior is a central aspect to this series.  This effort has dragged
>> out for nearly two years and I apologize, but I don't really have a lot
>> of patience for rehashing some of these issues if you're not going to
>> read the previous discussions or consult with your colleagues to
>> understand how we got to this point.  If you want to challenge some of
>> the design points, that's great, it could use some new eyes, but please
>> understand how we got here first.
>
> Well I'm guessing Cao jin here isn't the only one not
> willing to plough through all historical versions of the patchset
> just to figure out the motivation for some code.
>
> Including a summary of a high level architecture couldn't hurt.
>
> Any chance of writing such?  Alternatively, we can try to build it as
> part of this thread.  Shouldn't be hard as it seems somewhat
> straight-forward on the surface:
>
> - detect link error on the host, don't reset link as we would normally do 
>  

 This is actually a new approach that I'm not sure I agree with.  By
 skipping the host directed link reset, vfio is taking responsibility
 for doing this, but then we just assume the user will do it.  I have
 issues with this.

 The previous approach was to use the error detected notifier to block
 access to the device, allowing the host to perform the link reset.  A
 subsequent notification in the AER process released the user access
 which allowed the user AER process to proceed.  This did result in both
 a host directed and a guest directed link reset, but other than
 coordinating the blocking of the user process during host reset, that
 hasn't been brought up as an issue previously.
   
>>>
>>> Tests on previous versions didn't bring up issues as I find, I think
>>> that is because we didn't test it completely. As I know, before August
>>> of this year, we didn't have cable connected to NIC, let alone
>>> connecting NIC to gateway.
>>
>> Lack of testing has been a significant issue throughout the development
>> of this series.
>>
>>> Even if I fixed the guest oops issue in igb driver that Alex found in
>>> v9, v9 still cannot work in my test. And in my test, disable link
>>> reset(in host) in aer core for vfio-pci is the most significant step to
>>> get my test passed.
>>
>> But is it the correct step?  I'm not convinced.  Why did blocking guest
>> access not work?  How do you plan to manage vfio taking the
>> responsibility to perform a bus reset when you don't know whether QEMU
>> is the user of the device or whether the user supports AER recovery?
>>  
> 
> Maybe currently we don't have enough proof to prove the correctness, but
> I think I did find some facts to prove that link reset in host is a big
> trouble, and can answer part of questions above.
> 
> 1st, some thoughts:
> In pci-error-recovery.txt and do_recovery() of kernel tree, we can see,
> a recovery consists of several steps(callbacks), link reset is one of
> them, and except link reset, the others are seems kind of device
> specific. In our case, both host & guest will do recovery, I think the
> host recovery actually is some kind of fake recovery, see vfio-pci
> driver's error_detected & resume callback, they don't do anything
> special, mainly signal error to user, but the link reset in host "fake
> reset" does some serious work, in other words, I think host does the
> recovery incompletely, so I was thinking, why not just drop incompletely
> host recovery(drop link reset) for vfio-pci, and let the guest take care
> of the whole serious recovery.  This is part of the reason of why my
> version looks like this.  But yes, I admit the issue Alex mentioned,
> vfio can't guarantee that user will do a bus reset, this is an issue I
> will keep looking for a solution.
> 
> 2nd, some facts and analyzation from test:
> In host, the relationship between time and behviour in each component
> roughly looks as following:
> 
>  +   HW+  host kernel   + qemu  + guest kernel  +
>  | |(error recovery)|   |   |
>  | ||   |   |
>  | | vfio-pci's |   |   |
>  | | error_detected |   |   |
>  | | +  |   |   |
>  | | | 

[git pull] single drm fix

[Dave Airlie]

Hi Linus,

Just a single fix for amdgpu to just suspend the gpu on "shutdown"
instead of shutting it down fully, as for some reason the hw was
getting upset in some situations.

Dave.

The following changes since commit 3e5de27e940d00d8d504dfb96625fb654f641509:

  Linux 4.9-rc8 (2016-12-04 12:50:51 -0800)

are available in the git repository at:

  git://people.freedesktop.org/~airlied/linux drm-fixes

for you to fetch changes up to 4e4f3e984954143fb0b8e5035df7ff22dd07bb6a:

  Merge branch 'drm-fixes-4.9' of
git://people.freedesktop.org/~agd5f/linux into drm-fixes (2016-12-08
10:32:27 +1000)


Alex Deucher (1):
  drm/amdgpu: just suspend the hw on pci shutdown

Dave Airlie (1):
  Merge branch 'drm-fixes-4.9' of
git://people.freedesktop.org/~agd5f/linux into drm-fixes

 drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 5 -
 3 files changed, 6 insertions(+), 2 deletions(-)

[git pull] single drm fix

[Dave Airlie]

Hi Linus,

Just a single fix for amdgpu to just suspend the gpu on "shutdown"
instead of shutting it down fully, as for some reason the hw was
getting upset in some situations.

Dave.

The following changes since commit 3e5de27e940d00d8d504dfb96625fb654f641509:

  Linux 4.9-rc8 (2016-12-04 12:50:51 -0800)

are available in the git repository at:

  git://people.freedesktop.org/~airlied/linux drm-fixes

for you to fetch changes up to 4e4f3e984954143fb0b8e5035df7ff22dd07bb6a:

  Merge branch 'drm-fixes-4.9' of
git://people.freedesktop.org/~agd5f/linux into drm-fixes (2016-12-08
10:32:27 +1000)


Alex Deucher (1):
  drm/amdgpu: just suspend the hw on pci shutdown

Dave Airlie (1):
  Merge branch 'drm-fixes-4.9' of
git://people.freedesktop.org/~agd5f/linux into drm-fixes

 drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 5 -
 3 files changed, 6 insertions(+), 2 deletions(-)

Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong

[John Stultz]

On Thu, Dec 8, 2016 at 11:09 PM, Chen Yu  wrote:
> On 2016/12/9 7:29, John Youn wrote:
>> On 12/8/2016 2:43 PM, John Stultz wrote:
>>> On Tue, Dec 6, 2016 at 7:52 PM, John Youn  wrote:
 On 12/6/2016 5:48 PM, John Stultz wrote:
> This patch works around the issue by re-reading the GOTGCTL
> state to check if the GOTGCTL_CONID_B is still set and if not
> restarting the change status logic.

 This also seems weird. The connector id status shouldn't go back to A,
 assuming you've left the cable unplugged.
>>>
>>> So I suspect this has something to do with the way the USB-A host
>>> ports on the board are wired up. As removing the usb-b plug seems to
>>> switch the device back into A mode.
>>>
>>> One quirk with this board is that the USB-A ports on the board do not
>>> function if anything is in the OTG/B plug (which is frustrating to use
>>> at times).
>>>
>>
>> Do you mean there are multiple A-ports on the board hooked up to the
>> same controller?
>>
>> If so, that would go a long way towards explaining things. Because the
>> hsotg is a single-port OTG controller. If there are multiple A-ports,
>> that means a hub has to be hard-wired internally to the port. But if
>> that's the case the OTG function won't work because OTG doesn't work
>> through a hub. It must go directly to the otg port. So there must be
>> some external logic kicking-in to switch routing to the OTG port or to
>> the HUB.
>>
>> This would explain this behavior with the ID pin status. Since hooking
>> up the HUB would make the controller an A-device whereas normally it
>> would be a B-device.
>>
>>> Guodong or Chen Yu understand the hardware details a bit better, and
>>> might be able to explain more if you need more information.
>>>
>>
>> Yeah it would be good to get some insight into this from a hardware
>> point of view.
>>
>
> Actually, I'm not very clear about the hardware details.
>
> In simple terms, there are two Type A USB 2.0 host ports and 
> one microUSB OTG port on the front edge of the board.
> The two Type A USB 2.0 host ports connect to a high-speed hub 
> and the hub connect to a USB Switch to which the microUSB OTG port
> also connect.
> If the Vbus of the microUSB OTG port was high or the ID of 
> the microUSB OTG port was low, the Switch will switch the DP and DM of the SOC
> to microUSB OTG port. If no cable was inserted to microUSB OTG port, 
> the Switch will switch the DP and DM of the SOC to the high-speed hub.
> There is another import point, the ID pin of soc will be 
> pulled high in both cases:
> 1.no cable is inserted to microUSB OTG port
> 2.cable is inserted to microUSB OTG port and ID of microUSB 
> OTG port is low.
>
> If my explanation confuse you,  maybe these documents can be helpful.
>
> 
> 1、https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/HardwareDocs/HardwareNotes.md
>
> USB Ports
>
> There are multiple USB ports on the HiKey board:
>
> One microUSB OTG port on the front edge of the board
> Two Type A USB 2.0 host ports on the front edge of the board
> One USB 2.0 host port on the high-speed expansion bus
>
> 
> 2、https://github.com/96boards/documentation/tree/master/ConsumerEdition/HiKey/AdditionalDocs
> Hardware User Guide

Yea, Page 12 in this pdf seems to explain it:
https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/AdditionalDocs/HiKey_Hardware_User_Manual_Rev0.2.pdf

There is a usb switch which enables the micro-usb-b port if a cable is
present, or switches to using the hub(which has its own limitations
wrt multi-speed support) for the usb-a ports.

thanks
-john

Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong

[John Stultz]

On Thu, Dec 8, 2016 at 11:09 PM, Chen Yu  wrote:
> On 2016/12/9 7:29, John Youn wrote:
>> On 12/8/2016 2:43 PM, John Stultz wrote:
>>> On Tue, Dec 6, 2016 at 7:52 PM, John Youn  wrote:
 On 12/6/2016 5:48 PM, John Stultz wrote:
> This patch works around the issue by re-reading the GOTGCTL
> state to check if the GOTGCTL_CONID_B is still set and if not
> restarting the change status logic.

 This also seems weird. The connector id status shouldn't go back to A,
 assuming you've left the cable unplugged.
>>>
>>> So I suspect this has something to do with the way the USB-A host
>>> ports on the board are wired up. As removing the usb-b plug seems to
>>> switch the device back into A mode.
>>>
>>> One quirk with this board is that the USB-A ports on the board do not
>>> function if anything is in the OTG/B plug (which is frustrating to use
>>> at times).
>>>
>>
>> Do you mean there are multiple A-ports on the board hooked up to the
>> same controller?
>>
>> If so, that would go a long way towards explaining things. Because the
>> hsotg is a single-port OTG controller. If there are multiple A-ports,
>> that means a hub has to be hard-wired internally to the port. But if
>> that's the case the OTG function won't work because OTG doesn't work
>> through a hub. It must go directly to the otg port. So there must be
>> some external logic kicking-in to switch routing to the OTG port or to
>> the HUB.
>>
>> This would explain this behavior with the ID pin status. Since hooking
>> up the HUB would make the controller an A-device whereas normally it
>> would be a B-device.
>>
>>> Guodong or Chen Yu understand the hardware details a bit better, and
>>> might be able to explain more if you need more information.
>>>
>>
>> Yeah it would be good to get some insight into this from a hardware
>> point of view.
>>
>
> Actually, I'm not very clear about the hardware details.
>
> In simple terms, there are two Type A USB 2.0 host ports and 
> one microUSB OTG port on the front edge of the board.
> The two Type A USB 2.0 host ports connect to a high-speed hub 
> and the hub connect to a USB Switch to which the microUSB OTG port
> also connect.
> If the Vbus of the microUSB OTG port was high or the ID of 
> the microUSB OTG port was low, the Switch will switch the DP and DM of the SOC
> to microUSB OTG port. If no cable was inserted to microUSB OTG port, 
> the Switch will switch the DP and DM of the SOC to the high-speed hub.
> There is another import point, the ID pin of soc will be 
> pulled high in both cases:
> 1.no cable is inserted to microUSB OTG port
> 2.cable is inserted to microUSB OTG port and ID of microUSB 
> OTG port is low.
>
> If my explanation confuse you,  maybe these documents can be helpful.
>
> 
> 1、https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/HardwareDocs/HardwareNotes.md
>
> USB Ports
>
> There are multiple USB ports on the HiKey board:
>
> One microUSB OTG port on the front edge of the board
> Two Type A USB 2.0 host ports on the front edge of the board
> One USB 2.0 host port on the high-speed expansion bus
>
> 
> 2、https://github.com/96boards/documentation/tree/master/ConsumerEdition/HiKey/AdditionalDocs
> Hardware User Guide

Yea, Page 12 in this pdf seems to explain it:
https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/AdditionalDocs/HiKey_Hardware_User_Manual_Rev0.2.pdf

There is a usb switch which enables the micro-usb-b port if a cable is
present, or switches to using the hub(which has its own limitations
wrt multi-speed support) for the usb-a ports.

thanks
-john

Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[Greg KH]

On Fri, Dec 09, 2016 at 12:05:53AM +, KY Srinivasan wrote:
> 
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 8, 2016 7:56 AM
> > To: KY Srinivasan 
> > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com;
> > jasow...@redhat.com; leann.ogasaw...@canonical.com;
> > bjorn.helg...@gmail.com; Haiyang Zhang 
> > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> > numbers
> > 
> > On Thu, Dec 08, 2016 at 12:33:43AM -0800, k...@exchange.microsoft.com
> > wrote:
> > > From: Haiyang Zhang 
> > >
> > > We currently use MAC address to match VF and synthetic NICs. Hyper-V
> > > provides a serial number to both devices for this purpose. This patch
> > > implements the matching based on VF serial numbers. This is the way
> > > specified by the protocol and more reliable.
> > >
> > > Signed-off-by: Haiyang Zhang 
> > > Signed-off-by: K. Y. Srinivasan 
> > > ---
> > >  drivers/net/hyperv/netvsc_drv.c |   55
> > ---
> > >  1 files changed, 51 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c
> > > index 9522763..c5778cf 100644
> > > --- a/drivers/net/hyperv/netvsc_drv.c
> > > +++ b/drivers/net/hyperv/netvsc_drv.c
> > > @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   free_netdev(netdev);
> > >  }
> > >
> > > -static struct net_device *get_netvsc_bymac(const u8 *mac)
> > > +static struct net_device *get_netvsc_byvfser(u32 vfser)
> > >  {
> > >   struct net_device *dev;
> > > + struct net_device_context *ndev_ctx;
> > >
> > >   ASSERT_RTNL();
> > >
> > > @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device
> > *netdev)
> > >   if (dev->netdev_ops != _ops)
> > >   continue;   /* not a netvsc device */
> > >
> > > - if (ether_addr_equal(mac, dev->perm_addr))
> > > + ndev_ctx = netdev_priv(dev);
> > > + if (ndev_ctx->vf_serial == vfser)
> > >   return dev;
> > >   }
> > >
> > > @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   return NULL;
> > >  }
> > >
> > > +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> > > +{
> > > + struct device *dev;
> > > + struct hv_device *hdev;
> > > + struct hv_pcibus_device *hbus = NULL;
> > > + struct list_head *iter;
> > > + struct hv_pci_dev *hpdev;
> > > + unsigned long flags;
> > > + u32 vfser = 0;
> > > + u32 count = 0;
> > > +
> > > + for (dev = _netdev->dev; dev; dev = dev->parent) {
> > 
> > You are going to walk the whole device tree backwards?  That's crazy.
> > And foolish.  And racy and broken (what happens if the tree changes
> > while you do this?)  Where is the lock being grabbed while this happens?
> > What about reference counts?  Do you see other drivers ever doing this
> > (if you do, point them out and I'll go yell at them too...)
> 
> Greg,
> 
> We are registering for netdev events. Coming into this function, the caller
> guarantees that the list of netdevs does not change - we assert this on entry:
> ASSERT_RTNL(). We are only walking up the device tree for the netdevs whose
> state change is being notified to us - the device tree being walked here is 
> limited to
> netdevs under question. 

But a netdev is a child of some type of "real" device, and you are now
walking the tree of all devices up to the "root" parent device, which
means you will hit PCI bridges, USB controllers, and all sorts of fun
things if you are a child of those types of devices.

And can't you tell if the netdev for this event, really is "your"
netdev?  Or are you getting called this for "all" netdevs?  Sorry, I
don't know this api, any pointers to it would be appreciated.

> We have a reference to the device and we know the device is not going away. 
> Is it not
> safe to dereference the parent pointer - after all the child has taken a 
> reference on
> the parent as part of  device_add() call.

It might be, and might not be.  There's a reason you don't see this
pattern anywhere in the kernel because of this...

> > > + if (!dev_is_vmbus(dev))
> > > + continue;
> > 
> > Ick.
> > 
> > Why isn't your parent pointer a vmbus device all the time?  How could
> > you get burried down in the device hierarchy when you are the driver for
> > a specific bus type in the first place?  How could this function ever be
> > called for a device that is NOT of this type?
> 
> We get notified when state changes on any of the netdev devices in the system.
> Not all netdevs in the system belong to vmbus. Consider for instance the 
> emulated NIC that can be configured. This is an emulated PCI NIC. We are only
> interested in 

Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

[Greg KH]

On Fri, Dec 09, 2016 at 12:05:53AM +, KY Srinivasan wrote:
> 
> 
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 8, 2016 7:56 AM
> > To: KY Srinivasan 
> > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org;
> > o...@aepfle.de; a...@canonical.com; vkuzn...@redhat.com;
> > jasow...@redhat.com; leann.ogasaw...@canonical.com;
> > bjorn.helg...@gmail.com; Haiyang Zhang 
> > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> > numbers
> > 
> > On Thu, Dec 08, 2016 at 12:33:43AM -0800, k...@exchange.microsoft.com
> > wrote:
> > > From: Haiyang Zhang 
> > >
> > > We currently use MAC address to match VF and synthetic NICs. Hyper-V
> > > provides a serial number to both devices for this purpose. This patch
> > > implements the matching based on VF serial numbers. This is the way
> > > specified by the protocol and more reliable.
> > >
> > > Signed-off-by: Haiyang Zhang 
> > > Signed-off-by: K. Y. Srinivasan 
> > > ---
> > >  drivers/net/hyperv/netvsc_drv.c |   55
> > ---
> > >  1 files changed, 51 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c
> > > index 9522763..c5778cf 100644
> > > --- a/drivers/net/hyperv/netvsc_drv.c
> > > +++ b/drivers/net/hyperv/netvsc_drv.c
> > > @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   free_netdev(netdev);
> > >  }
> > >
> > > -static struct net_device *get_netvsc_bymac(const u8 *mac)
> > > +static struct net_device *get_netvsc_byvfser(u32 vfser)
> > >  {
> > >   struct net_device *dev;
> > > + struct net_device_context *ndev_ctx;
> > >
> > >   ASSERT_RTNL();
> > >
> > > @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct net_device
> > *netdev)
> > >   if (dev->netdev_ops != _ops)
> > >   continue;   /* not a netvsc device */
> > >
> > > - if (ether_addr_equal(mac, dev->perm_addr))
> > > + ndev_ctx = netdev_priv(dev);
> > > + if (ndev_ctx->vf_serial == vfser)
> > >   return dev;
> > >   }
> > >
> > > @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct
> > net_device *netdev)
> > >   return NULL;
> > >  }
> > >
> > > +static u32 netvsc_get_vfser(struct net_device *vf_netdev)
> > > +{
> > > + struct device *dev;
> > > + struct hv_device *hdev;
> > > + struct hv_pcibus_device *hbus = NULL;
> > > + struct list_head *iter;
> > > + struct hv_pci_dev *hpdev;
> > > + unsigned long flags;
> > > + u32 vfser = 0;
> > > + u32 count = 0;
> > > +
> > > + for (dev = _netdev->dev; dev; dev = dev->parent) {
> > 
> > You are going to walk the whole device tree backwards?  That's crazy.
> > And foolish.  And racy and broken (what happens if the tree changes
> > while you do this?)  Where is the lock being grabbed while this happens?
> > What about reference counts?  Do you see other drivers ever doing this
> > (if you do, point them out and I'll go yell at them too...)
> 
> Greg,
> 
> We are registering for netdev events. Coming into this function, the caller
> guarantees that the list of netdevs does not change - we assert this on entry:
> ASSERT_RTNL(). We are only walking up the device tree for the netdevs whose
> state change is being notified to us - the device tree being walked here is 
> limited to
> netdevs under question. 

But a netdev is a child of some type of "real" device, and you are now
walking the tree of all devices up to the "root" parent device, which
means you will hit PCI bridges, USB controllers, and all sorts of fun
things if you are a child of those types of devices.

And can't you tell if the netdev for this event, really is "your"
netdev?  Or are you getting called this for "all" netdevs?  Sorry, I
don't know this api, any pointers to it would be appreciated.

> We have a reference to the device and we know the device is not going away. 
> Is it not
> safe to dereference the parent pointer - after all the child has taken a 
> reference on
> the parent as part of  device_add() call.

It might be, and might not be.  There's a reason you don't see this
pattern anywhere in the kernel because of this...

> > > + if (!dev_is_vmbus(dev))
> > > + continue;
> > 
> > Ick.
> > 
> > Why isn't your parent pointer a vmbus device all the time?  How could
> > you get burried down in the device hierarchy when you are the driver for
> > a specific bus type in the first place?  How could this function ever be
> > called for a device that is NOT of this type?
> 
> We get notified when state changes on any of the netdev devices in the system.
> Not all netdevs in the system belong to vmbus. Consider for instance the 
> emulated NIC that can be configured. This is an emulated PCI NIC. We are only
> interested in netdevs that correspond to the VF instance that we are 
> interested in.

Can you "know" this is your netdev by 

Re: [V9fs-developer] [PATCH 2/5] 9p: store req details and callback in struct p9_req_t

[Dominique Martinet]

Nice. I like the idea of async I/Os :)

Stefano Stabellini wrote on Thu, Dec 08, 2016:
> Add a few fields to struct p9_req_t. Callback is the function which will
> be called upon requestion completion. offset, rsize, pagevec and kiocb
> store important information regarding the read or write request,
> essential to complete the request.
> 
> Currently not utilized, but they will be used in a later patch.
> 
> Signed-off-by: Stefano Stabellini 
> ---
>  include/net/9p/client.h | 8 
>  net/9p/client.c | 9 -
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index aef19c6..69fc2f0 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -110,6 +110,7 @@ enum p9_req_status_t {
>   *
>   */
>  
> +struct p9_client;
>  struct p9_req_t {
>   int status;
>   int t_err;
> @@ -118,6 +119,13 @@ struct p9_req_t {
>   struct p9_fcall *rc;
>   void *aux;
>  
> +/* Used for async requests */
> + void (*callback)(struct p9_client *c, struct p9_req_t *req, int status);
> + size_t offset;
> + u64 rsize;
> + struct page **pagevec;
> + struct kiocb *kiocb;
> +
>   struct list_head req_list;
>  };
>  
> diff --git a/net/9p/client.c b/net/9p/client.c
> index b5ea9a3..bfe1715 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -405,6 +405,10 @@ static void p9_free_req(struct p9_client *c, struct 
> p9_req_t *r)
>   int tag = r->tc->tag;
>   p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
>  
> + r->offset = 0;
> + r->rsize = 0;
> + r->kiocb = NULL;
> + r->callback = NULL;

Probably want to cleanup r->pagevec here too, even if that doesn't seem
to have any implication short-term (e.g. only looked at if callback is
not empty from what I've seen)

>   r->status = REQ_STATUS_IDLE;
>   if (tag != P9_NOTAG && p9_idpool_check(tag, c->tagpool))
>   p9_idpool_put(tag, c->tagpool);
> @@ -427,7 +431,10 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t 
> *req, int status)
>   smp_wmb();
>   req->status = status;
>  
> - wake_up(req->wq);
> + if (req->callback != NULL)
> + req->callback(c, req, status);
> + else
> + wake_up(req->wq);
>   p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
>  }
>  EXPORT_SYMBOL(p9_client_cb);

Mostly a warning here, but p9_client_cb is called from an interrupt
context in 9P/RDMA.
This has been working up till now because we only do a wake_up and
there's no waiting, but (looking at later patches),
p9_client_read_complete for example does allocations and possibly other
unsafe operations from an interrupt context.

I don't know if the way forward is to move p9_client_cb from that
context or to have the callback be scheduled in a work queue instead;
but we'll need to fix that later.

-- 
Dominique Martinet

Re: [V9fs-developer] [PATCH 2/5] 9p: store req details and callback in struct p9_req_t

[Dominique Martinet]

Nice. I like the idea of async I/Os :)

Stefano Stabellini wrote on Thu, Dec 08, 2016:
> Add a few fields to struct p9_req_t. Callback is the function which will
> be called upon requestion completion. offset, rsize, pagevec and kiocb
> store important information regarding the read or write request,
> essential to complete the request.
> 
> Currently not utilized, but they will be used in a later patch.
> 
> Signed-off-by: Stefano Stabellini 
> ---
>  include/net/9p/client.h | 8 
>  net/9p/client.c | 9 -
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index aef19c6..69fc2f0 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -110,6 +110,7 @@ enum p9_req_status_t {
>   *
>   */
>  
> +struct p9_client;
>  struct p9_req_t {
>   int status;
>   int t_err;
> @@ -118,6 +119,13 @@ struct p9_req_t {
>   struct p9_fcall *rc;
>   void *aux;
>  
> +/* Used for async requests */
> + void (*callback)(struct p9_client *c, struct p9_req_t *req, int status);
> + size_t offset;
> + u64 rsize;
> + struct page **pagevec;
> + struct kiocb *kiocb;
> +
>   struct list_head req_list;
>  };
>  
> diff --git a/net/9p/client.c b/net/9p/client.c
> index b5ea9a3..bfe1715 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -405,6 +405,10 @@ static void p9_free_req(struct p9_client *c, struct 
> p9_req_t *r)
>   int tag = r->tc->tag;
>   p9_debug(P9_DEBUG_MUX, "clnt %p req %p tag: %d\n", c, r, tag);
>  
> + r->offset = 0;
> + r->rsize = 0;
> + r->kiocb = NULL;
> + r->callback = NULL;

Probably want to cleanup r->pagevec here too, even if that doesn't seem
to have any implication short-term (e.g. only looked at if callback is
not empty from what I've seen)

>   r->status = REQ_STATUS_IDLE;
>   if (tag != P9_NOTAG && p9_idpool_check(tag, c->tagpool))
>   p9_idpool_put(tag, c->tagpool);
> @@ -427,7 +431,10 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t 
> *req, int status)
>   smp_wmb();
>   req->status = status;
>  
> - wake_up(req->wq);
> + if (req->callback != NULL)
> + req->callback(c, req, status);
> + else
> + wake_up(req->wq);
>   p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc->tag);
>  }
>  EXPORT_SYMBOL(p9_client_cb);

Mostly a warning here, but p9_client_cb is called from an interrupt
context in 9P/RDMA.
This has been working up till now because we only do a wake_up and
there's no waiting, but (looking at later patches),
p9_client_read_complete for example does allocations and possibly other
unsafe operations from an interrupt context.

I don't know if the way forward is to move p9_client_cb from that
context or to have the callback be scheduled in a work queue instead;
but we'll need to fix that later.

-- 
Dominique Martinet

Re: [V9fs-developer] [PATCH 4/5] 9p: introduce async read requests

[Dominique Martinet]

Stefano Stabellini wrote on Thu, Dec 08, 2016:
> If the read is an async operation, send a 9p request and return
> EIOCBQUEUED. Do not wait for completion.
> 
> Complete the read operation from a callback instead.
> 
> Signed-off-by: Stefano Stabellini 
> ---
>  net/9p/client.c | 88 
> +++--
>  1 file changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index eb589ef..f9f09db 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1554,13 +1555,68 @@ int p9_client_unlinkat(struct p9_fid *dfid, const 
> char *name, int flags)
>  }
>  EXPORT_SYMBOL(p9_client_unlinkat);
>  
> +static void
> +p9_client_read_complete(struct p9_client *clnt, struct p9_req_t *req, int 
> status)
> +{
> + int err, count, n, i, total = 0;
> + char *dataptr, *to;
> +
> + if (req->status == REQ_STATUS_ERROR) {
> + p9_debug(P9_DEBUG_ERROR, "req_status error %d\n", req->t_err);
> + err = req->t_err;
> + goto out;
> + }
> + err = p9_check_errors(clnt, req);
> + if (err)
> + goto out;
> +
> + err = p9pdu_readf(req->rc, clnt->proto_version,
> + "D", , );
> + if (err) {
> + trace_9p_protocol_dump(clnt, req->rc);
> + goto out;
> + }
> + if (!count) {
> + p9_debug(P9_DEBUG_ERROR, "count=%d\n", count);
> + err = 0;
> + goto out;
> + }
> +
> + p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
> + if (count > req->rsize)
> + count = req->rsize;
> +
> + for (i = 0; i < ((req->rsize + PAGE_SIZE - 1) / PAGE_SIZE); i++) {
> + to = kmap(req->pagevec[i]);
> + to += req->offset;
> + n = PAGE_SIZE - req->offset;
> + if (n > count)
> + n = count;
> + memcpy(to, dataptr, n);
> + kunmap(req->pagevec[i]);
> + req->offset = 0;
> + count -= n;
> + total += n;
> + }
> +
> + err = total;
> + req->kiocb->ki_pos += total;
> +
> +out:
> + req->kiocb->ki_complete(req->kiocb, err, 0);
> +
> + release_pages(req->pagevec, (req->rsize + PAGE_SIZE - 1) / PAGE_SIZE, 
> false);
> + kvfree(req->pagevec);
> + p9_free_req(clnt, req);
> +}
> +
>  int
>  p9_client_read(struct p9_fid *fid, struct kiocb *iocb, u64 offset,
>   struct iov_iter *to, int *err)
>  {
>   struct p9_client *clnt = fid->clnt;
>   struct p9_req_t *req;
> - int total = 0;
> + int total = 0, i;
>   *err = 0;
>  
>   p9_debug(P9_DEBUG_9P, ">>> TREAD fid %d offset %llu %d\n",
> @@ -1587,10 +1643,38 @@ int p9_client_unlinkat(struct p9_fid *dfid, const 
> char *name, int flags)
>   req = p9_client_zc_rpc(clnt, P9_TREAD, to, NULL, rsize,
>  0, 11, "dqd", fid->fid,
>  offset, rsize);
> - } else {
> + /* sync request */
> + } else if(iocb == NULL || is_sync_kiocb(iocb)) {
>   non_zc = 1;
>   req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, 
> offset,
>   rsize);
> + /* async request */
> + } else {

I'm not too familiar with iocb/how async IOs should work, but a logic
question just to make sure that has been thought out:
We prefer zc here to async, even if zc can be slow?

Ideally at some point zc and async aren't exclusive so we'll have async
zc and async normal, but for now I'd say async comes before zc - yes
there will be an extra copy in memory, but it will be done
asynchronously.
Was it intentional to prefer zc here?

> + req = p9_client_get_req(clnt, P9_TREAD, "dqd", 
> fid->fid, offset, rsize);
> + if (IS_ERR(req)) {
> + *err = PTR_ERR(req);
> + break;
> + }
> + req->rsize = iov_iter_get_pages_alloc(to, 
> >pagevec, 
> + (size_t)rsize, >offset);
> + req->kiocb = iocb;
> + for (i = 0; i < req->rsize; i += PAGE_SIZE)
> + 
> page_cache_get_speculative(req->pagevec[i/PAGE_SIZE]);
> + req->callback = p9_client_read_complete;
> +
> + *err = clnt->trans_mod->request(clnt, req);
> + if (*err < 0) {
> + clnt->status = Disconnected;
> + release_pages(req->pagevec,
> + (req->rsize + PAGE_SIZE - 1) / 
> PAGE_SIZE,
> +   

Re: [V9fs-developer] [PATCH 4/5] 9p: introduce async read requests

[Dominique Martinet]

Stefano Stabellini wrote on Thu, Dec 08, 2016:
> If the read is an async operation, send a 9p request and return
> EIOCBQUEUED. Do not wait for completion.
> 
> Complete the read operation from a callback instead.
> 
> Signed-off-by: Stefano Stabellini 
> ---
>  net/9p/client.c | 88 
> +++--
>  1 file changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index eb589ef..f9f09db 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1554,13 +1555,68 @@ int p9_client_unlinkat(struct p9_fid *dfid, const 
> char *name, int flags)
>  }
>  EXPORT_SYMBOL(p9_client_unlinkat);
>  
> +static void
> +p9_client_read_complete(struct p9_client *clnt, struct p9_req_t *req, int 
> status)
> +{
> + int err, count, n, i, total = 0;
> + char *dataptr, *to;
> +
> + if (req->status == REQ_STATUS_ERROR) {
> + p9_debug(P9_DEBUG_ERROR, "req_status error %d\n", req->t_err);
> + err = req->t_err;
> + goto out;
> + }
> + err = p9_check_errors(clnt, req);
> + if (err)
> + goto out;
> +
> + err = p9pdu_readf(req->rc, clnt->proto_version,
> + "D", , );
> + if (err) {
> + trace_9p_protocol_dump(clnt, req->rc);
> + goto out;
> + }
> + if (!count) {
> + p9_debug(P9_DEBUG_ERROR, "count=%d\n", count);
> + err = 0;
> + goto out;
> + }
> +
> + p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
> + if (count > req->rsize)
> + count = req->rsize;
> +
> + for (i = 0; i < ((req->rsize + PAGE_SIZE - 1) / PAGE_SIZE); i++) {
> + to = kmap(req->pagevec[i]);
> + to += req->offset;
> + n = PAGE_SIZE - req->offset;
> + if (n > count)
> + n = count;
> + memcpy(to, dataptr, n);
> + kunmap(req->pagevec[i]);
> + req->offset = 0;
> + count -= n;
> + total += n;
> + }
> +
> + err = total;
> + req->kiocb->ki_pos += total;
> +
> +out:
> + req->kiocb->ki_complete(req->kiocb, err, 0);
> +
> + release_pages(req->pagevec, (req->rsize + PAGE_SIZE - 1) / PAGE_SIZE, 
> false);
> + kvfree(req->pagevec);
> + p9_free_req(clnt, req);
> +}
> +
>  int
>  p9_client_read(struct p9_fid *fid, struct kiocb *iocb, u64 offset,
>   struct iov_iter *to, int *err)
>  {
>   struct p9_client *clnt = fid->clnt;
>   struct p9_req_t *req;
> - int total = 0;
> + int total = 0, i;
>   *err = 0;
>  
>   p9_debug(P9_DEBUG_9P, ">>> TREAD fid %d offset %llu %d\n",
> @@ -1587,10 +1643,38 @@ int p9_client_unlinkat(struct p9_fid *dfid, const 
> char *name, int flags)
>   req = p9_client_zc_rpc(clnt, P9_TREAD, to, NULL, rsize,
>  0, 11, "dqd", fid->fid,
>  offset, rsize);
> - } else {
> + /* sync request */
> + } else if(iocb == NULL || is_sync_kiocb(iocb)) {
>   non_zc = 1;
>   req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, 
> offset,
>   rsize);
> + /* async request */
> + } else {

I'm not too familiar with iocb/how async IOs should work, but a logic
question just to make sure that has been thought out:
We prefer zc here to async, even if zc can be slow?

Ideally at some point zc and async aren't exclusive so we'll have async
zc and async normal, but for now I'd say async comes before zc - yes
there will be an extra copy in memory, but it will be done
asynchronously.
Was it intentional to prefer zc here?

> + req = p9_client_get_req(clnt, P9_TREAD, "dqd", 
> fid->fid, offset, rsize);
> + if (IS_ERR(req)) {
> + *err = PTR_ERR(req);
> + break;
> + }
> + req->rsize = iov_iter_get_pages_alloc(to, 
> >pagevec, 
> + (size_t)rsize, >offset);
> + req->kiocb = iocb;
> + for (i = 0; i < req->rsize; i += PAGE_SIZE)
> + 
> page_cache_get_speculative(req->pagevec[i/PAGE_SIZE]);
> + req->callback = p9_client_read_complete;
> +
> + *err = clnt->trans_mod->request(clnt, req);
> + if (*err < 0) {
> + clnt->status = Disconnected;
> + release_pages(req->pagevec,
> + (req->rsize + PAGE_SIZE - 1) / 
> PAGE_SIZE,
> + true);
> 

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> Hi Dave,
> 
> (CC'ing LKML and Greg KH)
> 
> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > Hi All.
> > 
> > I'm working with a USB webcam which has been seen to spontaneously
> > disconnect when in use. That's a separate issue, but when it does it
> > throws a load of warnings into the kernel log if there is a file handle
> > on the device open at the time, even if not streaming.
> > 
> > I've reproduced this with a generic Logitech C270 webcam on:
> > - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media tree
> > from linuxtv.org
> > - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > - an old 3.10.x tree on an embedded device.
> > 
> > To reproduce:
> > - connect USB webcam.
> > - run a simple app that opens /dev/videoX, sleeps for a while, and then
> > closes the handle.
> > - disconnect the webcam whilst the app is running.
> > - read kernel logs - observe warnings. We get the disconnect logged as
> > it occurs, but the warnings all occur when the file descriptor is
> > closed. (A copy of the logs from my Ubuntu 14.04 machine are below).
> > 
> > I can fully appreciate that the open file descriptor is holding
> > references to a now invalid device, but is there a way to avoid them? Or
> > do we really not care and have to put up with the log noise when doing
> > such silly things?
> 
> This is a known problem, caused by the driver core trying to remove the same 
> sysfs attributes group twice.

Ick, not good.

> The group is first removed when the USB device is disconnected. The input 
> device and media device created by the uvcvideo driver are children of the 
> USB 
> interface device, which is deleted from the system when the camera is 
> unplugged. Due to the parent-child relationship, all sysfs attribute groups 
> of 
> the children are removed.

Wait, why is the USB device being removed from sysfs at this point,
didn't the input and media subsystems grab a reference to it so that it
does not disappear just yet?

> Then, when the device node is closed, the media device and input device are 
> unregistered, causing the corresponding devices to be deleted too. The driver 
> core tries to remove the sysfs attributes groups related to those devices, 
> and 
> issues a warning as they have been removed already.
> 
> I'm not sure how to fix that, any hint from LKML would be appreciated.

Properly grab a reference to the USB device?  :)

If that's already happening, please let me know and I'll see what needs
to be done, but I think that should solve the issue for you.

thanks,

greg k-h

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> Hi Dave,
> 
> (CC'ing LKML and Greg KH)
> 
> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > Hi All.
> > 
> > I'm working with a USB webcam which has been seen to spontaneously
> > disconnect when in use. That's a separate issue, but when it does it
> > throws a load of warnings into the kernel log if there is a file handle
> > on the device open at the time, even if not streaming.
> > 
> > I've reproduced this with a generic Logitech C270 webcam on:
> > - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media tree
> > from linuxtv.org
> > - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > - an old 3.10.x tree on an embedded device.
> > 
> > To reproduce:
> > - connect USB webcam.
> > - run a simple app that opens /dev/videoX, sleeps for a while, and then
> > closes the handle.
> > - disconnect the webcam whilst the app is running.
> > - read kernel logs - observe warnings. We get the disconnect logged as
> > it occurs, but the warnings all occur when the file descriptor is
> > closed. (A copy of the logs from my Ubuntu 14.04 machine are below).
> > 
> > I can fully appreciate that the open file descriptor is holding
> > references to a now invalid device, but is there a way to avoid them? Or
> > do we really not care and have to put up with the log noise when doing
> > such silly things?
> 
> This is a known problem, caused by the driver core trying to remove the same 
> sysfs attributes group twice.

Ick, not good.

> The group is first removed when the USB device is disconnected. The input 
> device and media device created by the uvcvideo driver are children of the 
> USB 
> interface device, which is deleted from the system when the camera is 
> unplugged. Due to the parent-child relationship, all sysfs attribute groups 
> of 
> the children are removed.

Wait, why is the USB device being removed from sysfs at this point,
didn't the input and media subsystems grab a reference to it so that it
does not disappear just yet?

> Then, when the device node is closed, the media device and input device are 
> unregistered, causing the corresponding devices to be deleted too. The driver 
> core tries to remove the sysfs attributes groups related to those devices, 
> and 
> issues a warning as they have been removed already.
> 
> I'm not sure how to fix that, any hint from LKML would be appreciated.

Properly grab a reference to the USB device?  :)

If that's already happening, please let me know and I'll see what needs
to be done, but I think that should solve the issue for you.

thanks,

greg k-h

Re: [PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace_marker

[Greg KH]

On Fri, Dec 09, 2016 at 07:34:04AM +0100, Henrik Austad wrote:
> Instead of using get_user_pages_fast() and kmap_atomic() when writing
> to the trace_marker file, just allocate enough space on the ring buffer
> directly, and write into it via copy_from_user().
> 
> Writing into the trace_marker file use to allocate a temporary buffer
> to perform the copy_from_user(), as we didn't want to write into the
> ring buffer if the copy failed. But as a trace_marker write is suppose
> to be extremely fast, and allocating memory causes other tracepoints to
> trigger, Peter Zijlstra suggested using get_user_pages_fast() and
> kmap_atomic() to keep the user space pages in memory and reading it
> directly.
> 
> Instead, just allocate the space in the ring buffer and use
> copy_from_user() directly. If it faults, return -EFAULT and write
> "" into the ring buffer.
> 
> On architectures without a arch-specific get_user_pages_fast(), this
> will end up in the generic get_user_pages_fast() and this grabs
> mm->mmap_sem. Once you do this, then suddenly writing to the
> trace_marker can cause priority-inversions.
> 
> This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
> signed-off-chain by is somewhat uncertain at this stage.
> 
> The patch compiles, boots and does not immediately explode on impact. By
> definition [2] it must therefore be perfect
> 
> 2) https://www.spinics.net/lists/kernel/msg2400769.html
> 2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html
> 
> Cc: Ingo Molnar 
> Cc: Henrik Austad 
> Cc: Peter Zijlstra 
> Cc: Steven Rostedt 
> Cc: sta...@vger.kernel.org
> 
> Suggested-by: Thomas Gleixner 
> Used-to-be-signed-off-by: Steven Rostedt 
> Backported-by: Henrik Austad 
> Tested-by: Henrik Austad 
> Signed-off-by: Henrik Austad 
> ---
>  kernel/trace/trace.c | 78 
> +++-
>  1 file changed, 22 insertions(+), 56 deletions(-)

What is the git commit id of this patch in Linus's tree?  And what
stable trees do you feel it should be applied to?

thanks,

greg k-h

Re: [PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace_marker

[Greg KH]

On Fri, Dec 09, 2016 at 07:34:04AM +0100, Henrik Austad wrote:
> Instead of using get_user_pages_fast() and kmap_atomic() when writing
> to the trace_marker file, just allocate enough space on the ring buffer
> directly, and write into it via copy_from_user().
> 
> Writing into the trace_marker file use to allocate a temporary buffer
> to perform the copy_from_user(), as we didn't want to write into the
> ring buffer if the copy failed. But as a trace_marker write is suppose
> to be extremely fast, and allocating memory causes other tracepoints to
> trigger, Peter Zijlstra suggested using get_user_pages_fast() and
> kmap_atomic() to keep the user space pages in memory and reading it
> directly.
> 
> Instead, just allocate the space in the ring buffer and use
> copy_from_user() directly. If it faults, return -EFAULT and write
> "" into the ring buffer.
> 
> On architectures without a arch-specific get_user_pages_fast(), this
> will end up in the generic get_user_pages_fast() and this grabs
> mm->mmap_sem. Once you do this, then suddenly writing to the
> trace_marker can cause priority-inversions.
> 
> This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
> signed-off-chain by is somewhat uncertain at this stage.
> 
> The patch compiles, boots and does not immediately explode on impact. By
> definition [2] it must therefore be perfect
> 
> 2) https://www.spinics.net/lists/kernel/msg2400769.html
> 2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html
> 
> Cc: Ingo Molnar 
> Cc: Henrik Austad 
> Cc: Peter Zijlstra 
> Cc: Steven Rostedt 
> Cc: sta...@vger.kernel.org
> 
> Suggested-by: Thomas Gleixner 
> Used-to-be-signed-off-by: Steven Rostedt 
> Backported-by: Henrik Austad 
> Tested-by: Henrik Austad 
> Signed-off-by: Henrik Austad 
> ---
>  kernel/trace/trace.c | 78 
> +++-
>  1 file changed, 22 insertions(+), 56 deletions(-)

What is the git commit id of this patch in Linus's tree?  And what
stable trees do you feel it should be applied to?

thanks,

greg k-h

Re: [PATCH v2] USB: OHCI: pxa27x:fix warnings and error

[Greg Kroah-Hartman]

On Thu, Dec 08, 2016 at 10:30:35PM +, manju goudar wrote:
> 
> 
> On Thu, Dec 8, 2016 at 4:49 PM, Greg Kroah-Hartman 
> 
> wrote:
> 
> On Wed, Dec 07, 2016 at 11:37:45PM +, csmanjuvi...@gmail.com wrote:
> > From: Manjunath Goudar 
> >
> > This patch will fix the checkpatch.pl following warnings and error:
> > WARNING: Block comments use * on subsequent lines
> > WARNING: Block comments use a trailing */ on a separate line
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev,
> > ... then dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
> > ERROR: space prohibited after that open parenthesis '('
> >
> > Signed-off-by: Manjunath Goudar 
> > Cc: Alan Stern 
> > Cc: Greg Kroah-Hartman 
> > Cc: linux-...@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > changelog V1->V2:
> > Warnings and error message is added to the patch discrition.
> >
> >  drivers/usb/host/ohci-pxa27x.c | 24 +++-
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-
> pxa27x.c
> > index 79efde8f..73445ab 100644
> > --- a/drivers/usb/host/ohci-pxa27x.c
> > +++ b/drivers/usb/host/ohci-pxa27x.c
> > @@ -106,7 +106,8 @@
> >  #define UHCHIE_UPS2IE        (1 << 12)       /* Power Sense Port2 IntEn
> */
> >  #define UHCHIE_UPS1IE        (1 << 11)       /* Power Sense Port1 IntEn
> */
> >  #define UHCHIE_TAIE  (1 << 10)       /* HCI Interface Transfer Abort
> > -                                        Interrupt Enable*/
> > +                                      * Interrupt Enable
> > +                                      */
> >  #define UHCHIE_HBAIE (1 << 8)        /* HCI Buffer Active IntEn */
> >  #define UHCHIE_RWIE  (1 << 7)        /* Remote Wake-up IntEn */
> >
> > @@ -128,14 +129,14 @@ struct pxa27x_ohci {
> >  #define to_pxa27x_ohci(hcd)  (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->
> priv)
> >
> >  /*
> > -  PMM_NPS_MODE -- PMM Non-power switching mode
> > -      Ports are powered continuously.
> > -
> > -  PMM_GLOBAL_MODE -- PMM global switching mode
> > -      All ports are powered at the same time.
> > -
> > -  PMM_PERPORT_MODE -- PMM per port switching mode
> > -      Ports are powered individually.
> > + * PMM_NPS_MODE -- PMM Non-power switching mode
> > + *     Ports are powered continuously.
> > + *
> > + * PMM_GLOBAL_MODE -- PMM global switching mode
> > + *     All ports are powered at the same time.
> > + *
> > + * PMM_PERPORT_MODE -- PMM per port switching mode
> > + *     Ports are powered individually.
> >   */
> >  static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int
> mode)
> >  {
> > @@ -157,10 +158,7 @@ static int pxa27x_ohci_select_pmm(struct 
> pxa27x_ohci
> *pxa_ohci, int mode)
> >               uhcrhdb |= (0x7<<17);
> >               break;
> >       default:
> > -             printk( KERN_ERR
> > -                     "Invalid mode %d, set to non-power switch 
> mode.\n",
> > -                     mode );
> > -
> > +             dev_err(mode, "Invalid mode %d,set to non-power switch
> mode.\n");
> 
> Did you even compile this code?
> 
> 
> Yes It is successful compiled. 

I don't believe you.  Look at your change here and tell me how that
dev_err() function is correct.

> Please do so...
> 
> And don't mix different types of fixes in the same patch please.
> 
> don't mix up means each type of warning fix as a separate patch?  

Yes please.

thanks,

greg k-h

Re: [PATCH v2] USB: OHCI: pxa27x:fix warnings and error

[Greg Kroah-Hartman]

On Thu, Dec 08, 2016 at 10:30:35PM +, manju goudar wrote:
> 
> 
> On Thu, Dec 8, 2016 at 4:49 PM, Greg Kroah-Hartman 
> 
> wrote:
> 
> On Wed, Dec 07, 2016 at 11:37:45PM +, csmanjuvi...@gmail.com wrote:
> > From: Manjunath Goudar 
> >
> > This patch will fix the checkpatch.pl following warnings and error:
> > WARNING: Block comments use * on subsequent lines
> > WARNING: Block comments use a trailing */ on a separate line
> > WARNING: Prefer [subsystem eg: netdev]_err([subsystem]dev,
> > ... then dev_err(dev, ... then pr_err(...  to printk(KERN_ERR ...
> > ERROR: space prohibited after that open parenthesis '('
> >
> > Signed-off-by: Manjunath Goudar 
> > Cc: Alan Stern 
> > Cc: Greg Kroah-Hartman 
> > Cc: linux-...@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > changelog V1->V2:
> > Warnings and error message is added to the patch discrition.
> >
> >  drivers/usb/host/ohci-pxa27x.c | 24 +++-
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-
> pxa27x.c
> > index 79efde8f..73445ab 100644
> > --- a/drivers/usb/host/ohci-pxa27x.c
> > +++ b/drivers/usb/host/ohci-pxa27x.c
> > @@ -106,7 +106,8 @@
> >  #define UHCHIE_UPS2IE        (1 << 12)       /* Power Sense Port2 IntEn
> */
> >  #define UHCHIE_UPS1IE        (1 << 11)       /* Power Sense Port1 IntEn
> */
> >  #define UHCHIE_TAIE  (1 << 10)       /* HCI Interface Transfer Abort
> > -                                        Interrupt Enable*/
> > +                                      * Interrupt Enable
> > +                                      */
> >  #define UHCHIE_HBAIE (1 << 8)        /* HCI Buffer Active IntEn */
> >  #define UHCHIE_RWIE  (1 << 7)        /* Remote Wake-up IntEn */
> >
> > @@ -128,14 +129,14 @@ struct pxa27x_ohci {
> >  #define to_pxa27x_ohci(hcd)  (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->
> priv)
> >
> >  /*
> > -  PMM_NPS_MODE -- PMM Non-power switching mode
> > -      Ports are powered continuously.
> > -
> > -  PMM_GLOBAL_MODE -- PMM global switching mode
> > -      All ports are powered at the same time.
> > -
> > -  PMM_PERPORT_MODE -- PMM per port switching mode
> > -      Ports are powered individually.
> > + * PMM_NPS_MODE -- PMM Non-power switching mode
> > + *     Ports are powered continuously.
> > + *
> > + * PMM_GLOBAL_MODE -- PMM global switching mode
> > + *     All ports are powered at the same time.
> > + *
> > + * PMM_PERPORT_MODE -- PMM per port switching mode
> > + *     Ports are powered individually.
> >   */
> >  static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int
> mode)
> >  {
> > @@ -157,10 +158,7 @@ static int pxa27x_ohci_select_pmm(struct 
> pxa27x_ohci
> *pxa_ohci, int mode)
> >               uhcrhdb |= (0x7<<17);
> >               break;
> >       default:
> > -             printk( KERN_ERR
> > -                     "Invalid mode %d, set to non-power switch 
> mode.\n",
> > -                     mode );
> > -
> > +             dev_err(mode, "Invalid mode %d,set to non-power switch
> mode.\n");
> 
> Did you even compile this code?
> 
> 
> Yes It is successful compiled. 

I don't believe you.  Look at your change here and tell me how that
dev_err() function is correct.

> Please do so...
> 
> And don't mix different types of fixes in the same patch please.
> 
> don't mix up means each type of warning fix as a separate patch?  

Yes please.

thanks,

greg k-h

Re: [PATCH v2] kexec: add cond_resched into kimage_alloc_crash_control_pages

[Xunlei Pang]

On 12/09/2016 at 01:13 PM, zhong jiang wrote:
> On 2016/12/8 17:41, Xunlei Pang wrote:
>> On 12/08/2016 at 10:37 AM, zhongjiang wrote:
>>> From: zhong jiang 
>>>
>>> A soft lookup will occur when I run trinity in syscall kexec_load.
>>> the corresponding stack information is as follows.
>>>
>>> [  237.235937] BUG: soft lockup - CPU#6 stuck for 22s! [trinity-c6:13859]
>>> [  237.242699] Kernel panic - not syncing: softlockup: hung tasks
>>> [  237.248573] CPU: 6 PID: 13859 Comm: trinity-c6 Tainted: G   O L 
>>> V---   3.10.0-327.28.3.35.zhongjiang.x86_64 #1
>>> [  237.259984] Hardware name: Huawei Technologies Co., Ltd. Tecal BH622 
>>> V2/BC01SRSA0, BIOS RMIBV386 06/30/2014
>>> [  237.269752]  8187626b 18cfde31 88184c803e18 
>>> 81638f16
>>> [  237.277471]  88184c803e98 8163278f 0008 
>>> 88184c803ea8
>>> [  237.285190]  88184c803e48 18cfde31 88184c803e67 
>>> 
>>> [  237.292909] Call Trace:
>>> [  237.295404][] dump_stack+0x19/0x1b
>>> [  237.301352]  [] panic+0xd8/0x214
>>> [  237.306196]  [] watchdog_timer_fn+0x1cc/0x1e0
>>> [  237.312157]  [] ? watchdog_enable+0xc0/0xc0
>>> [  237.317955]  [] __hrtimer_run_queues+0xd2/0x260
>>> [  237.324087]  [] hrtimer_interrupt+0xb0/0x1e0
>>> [  237.329963]  [] ? call_softirq+0x1c/0x30
>>> [  237.335500]  [] local_apic_timer_interrupt+0x37/0x60
>>> [  237.342228]  [] smp_apic_timer_interrupt+0x3f/0x60
>>> [  237.348771]  [] apic_timer_interrupt+0x6d/0x80
>>> [  237.354967][] ? 
>>> kimage_alloc_control_pages+0x80/0x270
>>> [  237.362875]  [] ? kmem_cache_alloc_trace+0x1ce/0x1f0
>>> [  237.369592]  [] ? do_kimage_alloc_init+0x1f/0x90
>>> [  237.375992]  [] kimage_alloc_init+0x12a/0x180
>>> [  237.382103]  [] SyS_kexec_load+0x20a/0x260
>>> [  237.387957]  [] system_call_fastpath+0x16/0x1b
>>>
>>> the first time allocate control pages may take too much time because
>>> crash_res.end can be set to a higher value. we need to add cond_resched
>>> to avoid the issue.
>>>
>>> The patch have been tested and above issue is not appear.
>>>
>>> Signed-off-by: zhong jiang 
>>> ---
>>>  kernel/kexec_core.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>>> index 5616755..bfc9621 100644
>>> --- a/kernel/kexec_core.c
>>> +++ b/kernel/kexec_core.c
>>> @@ -441,6 +441,8 @@ static struct page 
>>> *kimage_alloc_crash_control_pages(struct kimage *image,
>>> while (hole_end <= crashk_res.end) {
>>> unsigned long i;
>>>  
>>> +   cond_resched();
>>> +
>> I can't see why it would take a long time to loop inside, the job it does is 
>> simply to find a control area
>> not overlapped with image->segment[], you can see the loop "for (i = 0; i < 
>> image->nr_segments; i++)",
>> @hole_end will be advanced to the end of its next nearby segment once 
>> overlap was detected each loop,
>> also there are limited (<=16) segments, so it won't take long to locate the 
>> right area.
>>
>> Am I missing something?
>>
>> Regards,
>> Xunlei
>   if the crashkernel = auto is set in cmdline.  it represent crashk_res.end 
> will exceed to 4G, the first allocate control pages will
>   loop  million times. if we set crashk_res.end to the higher value manually, 
>  you can image

How does "loop million times" happen? See my inlined comments prefixed with 
"pxl".

kimage_alloc_crash_control_pages():
while (hole_end <= crashk_res.end) {
unsigned long i;

if (hole_end > KEXEC_CRASH_CONTROL_MEMORY_LIMIT)
break;
/* See if I overlap any of the segments */
for (i = 0; i < image->nr_segments; i++) {  // pxl: max 16 loops, all 
existent segments are not overlapped, though may not sorted.
unsigned long mstart, mend;

mstart = image->segment[i].mem;
mend   = mstart + image->segment[i].memsz - 1;
if ((hole_end >= mstart) && (hole_start <= mend)) {
/* Advance the hole to the end of the segment */
hole_start = (mend + (size - 1)) & ~(size - 1);
hole_end   = hole_start + size - 1;
break;  // pxl: If overlap was found, break for loop, @hole_end 
starts after the overlapped segment area, and will while loop again
}
}
/* If I don't overlap any segments I have found my hole! */
if (i == image->nr_segments) {
pages = pfn_to_page(hole_start >> PAGE_SHIFT);
image->control_page = hole_end;
break;   // pxl: no overlap with all the segments, get the result 
and break the while loop. END.
}   
}

So, the worst "while" loops in theory would be (image->nr_segments + 1), no?

Regards,
Xunlei

Re: [PATCH v2] kexec: add cond_resched into kimage_alloc_crash_control_pages

[Xunlei Pang]

On 12/09/2016 at 01:13 PM, zhong jiang wrote:
> On 2016/12/8 17:41, Xunlei Pang wrote:
>> On 12/08/2016 at 10:37 AM, zhongjiang wrote:
>>> From: zhong jiang 
>>>
>>> A soft lookup will occur when I run trinity in syscall kexec_load.
>>> the corresponding stack information is as follows.
>>>
>>> [  237.235937] BUG: soft lockup - CPU#6 stuck for 22s! [trinity-c6:13859]
>>> [  237.242699] Kernel panic - not syncing: softlockup: hung tasks
>>> [  237.248573] CPU: 6 PID: 13859 Comm: trinity-c6 Tainted: G   O L 
>>> V---   3.10.0-327.28.3.35.zhongjiang.x86_64 #1
>>> [  237.259984] Hardware name: Huawei Technologies Co., Ltd. Tecal BH622 
>>> V2/BC01SRSA0, BIOS RMIBV386 06/30/2014
>>> [  237.269752]  8187626b 18cfde31 88184c803e18 
>>> 81638f16
>>> [  237.277471]  88184c803e98 8163278f 0008 
>>> 88184c803ea8
>>> [  237.285190]  88184c803e48 18cfde31 88184c803e67 
>>> 
>>> [  237.292909] Call Trace:
>>> [  237.295404][] dump_stack+0x19/0x1b
>>> [  237.301352]  [] panic+0xd8/0x214
>>> [  237.306196]  [] watchdog_timer_fn+0x1cc/0x1e0
>>> [  237.312157]  [] ? watchdog_enable+0xc0/0xc0
>>> [  237.317955]  [] __hrtimer_run_queues+0xd2/0x260
>>> [  237.324087]  [] hrtimer_interrupt+0xb0/0x1e0
>>> [  237.329963]  [] ? call_softirq+0x1c/0x30
>>> [  237.335500]  [] local_apic_timer_interrupt+0x37/0x60
>>> [  237.342228]  [] smp_apic_timer_interrupt+0x3f/0x60
>>> [  237.348771]  [] apic_timer_interrupt+0x6d/0x80
>>> [  237.354967][] ? 
>>> kimage_alloc_control_pages+0x80/0x270
>>> [  237.362875]  [] ? kmem_cache_alloc_trace+0x1ce/0x1f0
>>> [  237.369592]  [] ? do_kimage_alloc_init+0x1f/0x90
>>> [  237.375992]  [] kimage_alloc_init+0x12a/0x180
>>> [  237.382103]  [] SyS_kexec_load+0x20a/0x260
>>> [  237.387957]  [] system_call_fastpath+0x16/0x1b
>>>
>>> the first time allocate control pages may take too much time because
>>> crash_res.end can be set to a higher value. we need to add cond_resched
>>> to avoid the issue.
>>>
>>> The patch have been tested and above issue is not appear.
>>>
>>> Signed-off-by: zhong jiang 
>>> ---
>>>  kernel/kexec_core.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>>> index 5616755..bfc9621 100644
>>> --- a/kernel/kexec_core.c
>>> +++ b/kernel/kexec_core.c
>>> @@ -441,6 +441,8 @@ static struct page 
>>> *kimage_alloc_crash_control_pages(struct kimage *image,
>>> while (hole_end <= crashk_res.end) {
>>> unsigned long i;
>>>  
>>> +   cond_resched();
>>> +
>> I can't see why it would take a long time to loop inside, the job it does is 
>> simply to find a control area
>> not overlapped with image->segment[], you can see the loop "for (i = 0; i < 
>> image->nr_segments; i++)",
>> @hole_end will be advanced to the end of its next nearby segment once 
>> overlap was detected each loop,
>> also there are limited (<=16) segments, so it won't take long to locate the 
>> right area.
>>
>> Am I missing something?
>>
>> Regards,
>> Xunlei
>   if the crashkernel = auto is set in cmdline.  it represent crashk_res.end 
> will exceed to 4G, the first allocate control pages will
>   loop  million times. if we set crashk_res.end to the higher value manually, 
>  you can image

How does "loop million times" happen? See my inlined comments prefixed with 
"pxl".

kimage_alloc_crash_control_pages():
while (hole_end <= crashk_res.end) {
unsigned long i;

if (hole_end > KEXEC_CRASH_CONTROL_MEMORY_LIMIT)
break;
/* See if I overlap any of the segments */
for (i = 0; i < image->nr_segments; i++) {  // pxl: max 16 loops, all 
existent segments are not overlapped, though may not sorted.
unsigned long mstart, mend;

mstart = image->segment[i].mem;
mend   = mstart + image->segment[i].memsz - 1;
if ((hole_end >= mstart) && (hole_start <= mend)) {
/* Advance the hole to the end of the segment */
hole_start = (mend + (size - 1)) & ~(size - 1);
hole_end   = hole_start + size - 1;
break;  // pxl: If overlap was found, break for loop, @hole_end 
starts after the overlapped segment area, and will while loop again
}
}
/* If I don't overlap any segments I have found my hole! */
if (i == image->nr_segments) {
pages = pfn_to_page(hole_start >> PAGE_SHIFT);
image->control_page = hole_end;
break;   // pxl: no overlap with all the segments, get the result 
and break the while loop. END.
}   
}

So, the worst "while" loops in theory would be (image->nr_segments + 1), no?

Regards,
Xunlei

Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong

[Chen Yu]

On 2016/12/9 7:29, John Youn wrote:
> On 12/8/2016 2:43 PM, John Stultz wrote:
>> On Tue, Dec 6, 2016 at 7:52 PM, John Youn  wrote:
>>> On 12/6/2016 5:48 PM, John Stultz wrote:
 Hey John,
   Just wanted to send this by you, as it seems something is
 slightly off with the GOTGCTL state when removing a otg adapter
 cable. The following seems to work around the issue I'm seeing.


 When removing a USB-A to USB-otg adapter cable, we get a change
 status irq, and then in dwc2_conn_id_status_change, we
 erroniously see the GOTGCTL_CONID_B flag set. This causes us to
>>>
>>> This is the correct behavior for an OTG controller. When you unplug a
>>> cable or plug in the B end of a cable, the ID pin floats, indicating
>>> it is a B-Device.
>>>
>>> When you plug in an A-cable, which is what your adapter is, it will
>>> ground the pin, meaning A-device.
>>
>> Hrm... So normally, when I plug in the gadget cable into the OTG port,
>> I see the change_status irq comes in and the function sees:
>>
>> dwc2 f72c.usb: gotgctl=401
>> dwc2 f72c.usb: gotgctl.b.conidsts=1
>> dwc2 f72c.usb: Do port resume before switching to device mode
>> dwc2 f72c.usb: dwc2_hsotg_enqueue_setup: failed queue (-11)
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new address 37
>> configfs-gadget gadget: high-speed config #1: b
>>
>> Then when I unplug the cable:
>>
>> dwc2 f72c.usb: gotgctl=220
>> dwc2 f72c.usb: gotgctl.b.conidsts=0
>> usb 1-1: reset high-speed USB device number 13 using dwc2
>>
>>
>>
>> When I plug in the OTG to USB-A adapter cable w/ a mouse plugged in
>> (note I see no change interrupt):
>>
>> usb 1-1: USB disconnect, device number 13
>> usb 1-1: new low-speed USB device number 14 using dwc2
>> input: Logitech USB Optical Mouse as
>> /devices/platform/soc/f72c.usb/usb1/1-1/1-1:1.0/0003:046D:C058.0003/input/input3
>> hid-generic 0003:046D:C058.0003: input,hidraw0: USB HID v1.11 Mouse
>> [Logitech USB Optical Mouse] on usb-f72c.usb-1/input0
>>
>>
>> Then unplugging the OTG to USB-A adapter cable w/ mouse:
>>
>> dwc2 f72c.usb: gotgctl=401
>> dwc2 f72c.usb: gotgctl.b.conidsts=1
>> dwc2 f72c.usb: Do port resume before switching to device mode
>> dwc2 f72c.usb: Waiting for Peripheral Mode, Mode=Host
>>
>> > patch from this thread>
>>
>> usb 1-1: USB disconnect, device number 14
>> dwc2 f72c.usb: gotgctl=220
>> dwc2 f72c.usb: gotgctl.b.conidsts=0
>> usb 1-1: new high-speed USB device number 15 using dwc2
>> hub 1-1:1.0: USB hub found
>> hub 1-1:1.0: 3 ports detected
>>
>>
>> So I only get the change irq when:
>> * I plug in a micro-usb-B cable for gadget mode
>> * I remove the micro-usb-B cable being used for gadget mode
>> * I remove a OTG to USB-A adapter
>>
> 
> That's very strange. It's opposite of how it's supposed to work.
> 
>> One slight quirk, is that I don't always see the change irq when
>> removing the OTG to USB, as if I plug in a highspeed mass-storage
>> device, instead of the low-speed mouse, I don't see the change
>> interrupt and the device shows up and disappears the same as when I
>> plug into the normal USB-A host ports on the board.
>>
>>
 get  stuck in the "while (!dwc2_is_device_mode(hsotg))" loop,
 spitting out "Waiting for Peripheral Mode, Mode=Host" warnings
 until it fails out many seconds later.
>>>
>>> This is weird. Once the ID pin goes to B, the core should become a
>>> peripheral and this should be reflected in the status registers.
>>>

 This patch works around the issue by re-reading the GOTGCTL
 state to check if the GOTGCTL_CONID_B is still set and if not
 restarting the change status logic.
>>>
>>> This also seems weird. The connector id status shouldn't go back to A,
>>> assuming you've left the cable unplugged.
>>
>> So I suspect this has something to do with the way the USB-A host
>> ports on the board are wired up. As removing the usb-b plug seems to
>> switch the device back into A mode.
>>
>> One quirk with this board is that the USB-A ports on the board do not
>> function if anything is in the OTG/B plug (which is frustrating to use
>> at times).
>>
> 
> Do you mean there are multiple A-ports on the board hooked up to the
> same controller?
> 
> If so, that would go a long way towards explaining things. Because the
> hsotg is a single-port OTG controller. If there are multiple A-ports,
> that means a hub has to be hard-wired internally to the port. But if
> that's the case the OTG function won't work because OTG doesn't work
> through a hub. It must go directly to the otg port. So there must be
> some external logic kicking-in to switch routing to the OTG port or to
> the HUB.
> 
> This would explain this behavior with the ID pin status. Since hooking
> up the HUB would make the controller an A-device whereas 

Re: [RFC][PATCH] HACK: usb: dwc2: Workaround case where GOTGCTL state is wrong

[Chen Yu]

On 2016/12/9 7:29, John Youn wrote:
> On 12/8/2016 2:43 PM, John Stultz wrote:
>> On Tue, Dec 6, 2016 at 7:52 PM, John Youn  wrote:
>>> On 12/6/2016 5:48 PM, John Stultz wrote:
 Hey John,
   Just wanted to send this by you, as it seems something is
 slightly off with the GOTGCTL state when removing a otg adapter
 cable. The following seems to work around the issue I'm seeing.


 When removing a USB-A to USB-otg adapter cable, we get a change
 status irq, and then in dwc2_conn_id_status_change, we
 erroniously see the GOTGCTL_CONID_B flag set. This causes us to
>>>
>>> This is the correct behavior for an OTG controller. When you unplug a
>>> cable or plug in the B end of a cable, the ID pin floats, indicating
>>> it is a B-Device.
>>>
>>> When you plug in an A-cable, which is what your adapter is, it will
>>> ground the pin, meaning A-device.
>>
>> Hrm... So normally, when I plug in the gadget cable into the OTG port,
>> I see the change_status irq comes in and the function sees:
>>
>> dwc2 f72c.usb: gotgctl=401
>> dwc2 f72c.usb: gotgctl.b.conidsts=1
>> dwc2 f72c.usb: Do port resume before switching to device mode
>> dwc2 f72c.usb: dwc2_hsotg_enqueue_setup: failed queue (-11)
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new device is high-speed
>> dwc2 f72c.usb: new address 37
>> configfs-gadget gadget: high-speed config #1: b
>>
>> Then when I unplug the cable:
>>
>> dwc2 f72c.usb: gotgctl=220
>> dwc2 f72c.usb: gotgctl.b.conidsts=0
>> usb 1-1: reset high-speed USB device number 13 using dwc2
>>
>>
>>
>> When I plug in the OTG to USB-A adapter cable w/ a mouse plugged in
>> (note I see no change interrupt):
>>
>> usb 1-1: USB disconnect, device number 13
>> usb 1-1: new low-speed USB device number 14 using dwc2
>> input: Logitech USB Optical Mouse as
>> /devices/platform/soc/f72c.usb/usb1/1-1/1-1:1.0/0003:046D:C058.0003/input/input3
>> hid-generic 0003:046D:C058.0003: input,hidraw0: USB HID v1.11 Mouse
>> [Logitech USB Optical Mouse] on usb-f72c.usb-1/input0
>>
>>
>> Then unplugging the OTG to USB-A adapter cable w/ mouse:
>>
>> dwc2 f72c.usb: gotgctl=401
>> dwc2 f72c.usb: gotgctl.b.conidsts=1
>> dwc2 f72c.usb: Do port resume before switching to device mode
>> dwc2 f72c.usb: Waiting for Peripheral Mode, Mode=Host
>>
>> > patch from this thread>
>>
>> usb 1-1: USB disconnect, device number 14
>> dwc2 f72c.usb: gotgctl=220
>> dwc2 f72c.usb: gotgctl.b.conidsts=0
>> usb 1-1: new high-speed USB device number 15 using dwc2
>> hub 1-1:1.0: USB hub found
>> hub 1-1:1.0: 3 ports detected
>>
>>
>> So I only get the change irq when:
>> * I plug in a micro-usb-B cable for gadget mode
>> * I remove the micro-usb-B cable being used for gadget mode
>> * I remove a OTG to USB-A adapter
>>
> 
> That's very strange. It's opposite of how it's supposed to work.
> 
>> One slight quirk, is that I don't always see the change irq when
>> removing the OTG to USB, as if I plug in a highspeed mass-storage
>> device, instead of the low-speed mouse, I don't see the change
>> interrupt and the device shows up and disappears the same as when I
>> plug into the normal USB-A host ports on the board.
>>
>>
 get  stuck in the "while (!dwc2_is_device_mode(hsotg))" loop,
 spitting out "Waiting for Peripheral Mode, Mode=Host" warnings
 until it fails out many seconds later.
>>>
>>> This is weird. Once the ID pin goes to B, the core should become a
>>> peripheral and this should be reflected in the status registers.
>>>

 This patch works around the issue by re-reading the GOTGCTL
 state to check if the GOTGCTL_CONID_B is still set and if not
 restarting the change status logic.
>>>
>>> This also seems weird. The connector id status shouldn't go back to A,
>>> assuming you've left the cable unplugged.
>>
>> So I suspect this has something to do with the way the USB-A host
>> ports on the board are wired up. As removing the usb-b plug seems to
>> switch the device back into A mode.
>>
>> One quirk with this board is that the USB-A ports on the board do not
>> function if anything is in the OTG/B plug (which is frustrating to use
>> at times).
>>
> 
> Do you mean there are multiple A-ports on the board hooked up to the
> same controller?
> 
> If so, that would go a long way towards explaining things. Because the
> hsotg is a single-port OTG controller. If there are multiple A-ports,
> that means a hub has to be hard-wired internally to the port. But if
> that's the case the OTG function won't work because OTG doesn't work
> through a hub. It must go directly to the otg port. So there must be
> some external logic kicking-in to switch routing to the OTG port or to
> the HUB.
> 
> This would explain this behavior with the ID pin status. Since hooking
> up the HUB would make the controller an A-device whereas normally it
> would be a 

Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

[Greg KH]

On Fri, Dec 09, 2016 at 12:36:26AM +, Stuart Yoder wrote:
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 08, 2016 10:05 AM
> > To: Stuart Yoder 
> > Cc: de...@driverdev.osuosl.org; ag...@suse.de; a...@arndb.de; 
> > linux-kernel@vger.kernel.org; Leo Li
> > ; Catalin Horghidan ; Ioana 
> > Ciornei
> > ; Laurentiu Tudor 
> > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
> > 
> > On Wed, Dec 07, 2016 at 08:19:20PM +, Stuart Yoder wrote:
> > > > -Original Message-
> > > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > > Sent: Wednesday, December 07, 2016 9:53 AM
> > > > To: Stuart Yoder 
> > > > Cc: de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; 
> > > > ag...@suse.de; a...@arndb.de; Leo Li
> > > > ; Ioana Ciornei ; Catalin 
> > > > Horghidan
> > > > ; Laurentiu Tudor ; 
> > > > Ruxandra Ioana Radulescu
> > > > 
> > > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of 
> > > > staging
> > > >
> > > > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote:
> > > > > Move the source files out of staging into their final locations:
> > > > >   -include files in drivers/staging/fsl-mc/include go to 
> > > > > include/linux/fsl
> > > > >   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
> > > > >   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
> > > > >   -README.txt, providing and overview of DPAA goes to
> > > > >Documentation/dpaa2/overview.txt
> > > > >   -update MAINTAINERS with new location
> > > > >
> > > > > Delete other remaining staging files-- Makefile, Kconfig, TODO
> > > >
> > > > Ok, given that I haven't ever reviewed this code, I had a few questions
> > > > that I couldn't easily figure out by looking at your code:
> > > > - what is the lifecycle of your 'struct device' usage?  Who
> > > >   creates it, who frees it, and who accesses it?
> > >
> > > We embed a 'struct device' inside our bus specific device struct
> > > 'struct fsl_mc_device'.  So, when a new fsl-mc object is discovered
> > > on the bus during initial enumeration or hotplug we create a new
> > > 'struct fsl_mc_device' and do a device_initialize()/device_add().
> > > (see fsl_mc_device_add() for where this is done)
> > >
> > > 'struct device' is freed when a device is removed-- the reverse
> > > of the above.
> > 
> > Where is the device freed?  I see you trying to do some "odd" stuff in
> > fsl_mc_device_remove() by deleting and then putting a device structure.
> > I can't find a "release()" callback anywhere for your bus, where is it?
> > 
> > What happens when the reference count falls to 0 for your struct device?
> 
> Hrm...something seems wrong in free path, and I think this needs to
> be refactored.
> 
> IIRC, when German (former maintainer) wrote that code he loosely based
> it on the register/unregister platform bus code:
> 
> int platform_device_register(struct platform_device *pdev)
> {
> device_initialize(>dev);
> arch_setup_pdev_archdata(pdev);
> return platform_device_add(pdev);
> }
> void platform_device_unregister(struct platform_device *pdev)
> {
> platform_device_del(pdev);
> platform_device_put(pdev);
> }
> 
> ...I'm puzzling over how that code handles a refcount of zero
> as I see no 'release' callback anywhere, but I must be missing
> something.
> 
> In any case, we'll get this refactored.

Have you tried removing a device?  The kernel should complain loudly
about there not being a release function for your device.

> > > > - root_dprc_count, why are you using an atomic variable for
> > > >   this?  What is it for other than "look, I'm running!"?
> > >
> > > There can be multiple root buses, and this variable simply tracks the 
> > > count
> > > of them.
> > 
> > Why does it matter?
> > 
> > > It's is atomic there might be a theoretical race condition where 2
> > > buses might be added at the same time.  The root buses are found in
> > > the device tree and so if there is no chance that device tree
> > > processing happens in parallel on multiple cores then we could remove
> > > the atomic.
> > 
> > Why not just use a lock, or better yet, not care about a "count" at all?
> > I don't see you doing anything with the count, other than emitting a
> > WARN() if it drops down below 0 for some reason, or when you call
> > fsl_mc_bus_exists() which for some reason is exported yet no one uses
> > it...
> 
> We can drop this count.  At one time I think there was envisioned an 
> external user who needed it, but it's no longer the case.

Please do, we are trying to get rid of atomic_t abuse on other mailing
lists, 

Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

[Greg KH]

On Fri, Dec 09, 2016 at 12:36:26AM +, Stuart Yoder wrote:
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Thursday, December 08, 2016 10:05 AM
> > To: Stuart Yoder 
> > Cc: de...@driverdev.osuosl.org; ag...@suse.de; a...@arndb.de; 
> > linux-kernel@vger.kernel.org; Leo Li
> > ; Catalin Horghidan ; Ioana 
> > Ciornei
> > ; Laurentiu Tudor 
> > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
> > 
> > On Wed, Dec 07, 2016 at 08:19:20PM +, Stuart Yoder wrote:
> > > > -Original Message-
> > > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > > Sent: Wednesday, December 07, 2016 9:53 AM
> > > > To: Stuart Yoder 
> > > > Cc: de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; 
> > > > ag...@suse.de; a...@arndb.de; Leo Li
> > > > ; Ioana Ciornei ; Catalin 
> > > > Horghidan
> > > > ; Laurentiu Tudor ; 
> > > > Ruxandra Ioana Radulescu
> > > > 
> > > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of 
> > > > staging
> > > >
> > > > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote:
> > > > > Move the source files out of staging into their final locations:
> > > > >   -include files in drivers/staging/fsl-mc/include go to 
> > > > > include/linux/fsl
> > > > >   -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip
> > > > >   -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc
> > > > >   -README.txt, providing and overview of DPAA goes to
> > > > >Documentation/dpaa2/overview.txt
> > > > >   -update MAINTAINERS with new location
> > > > >
> > > > > Delete other remaining staging files-- Makefile, Kconfig, TODO
> > > >
> > > > Ok, given that I haven't ever reviewed this code, I had a few questions
> > > > that I couldn't easily figure out by looking at your code:
> > > > - what is the lifecycle of your 'struct device' usage?  Who
> > > >   creates it, who frees it, and who accesses it?
> > >
> > > We embed a 'struct device' inside our bus specific device struct
> > > 'struct fsl_mc_device'.  So, when a new fsl-mc object is discovered
> > > on the bus during initial enumeration or hotplug we create a new
> > > 'struct fsl_mc_device' and do a device_initialize()/device_add().
> > > (see fsl_mc_device_add() for where this is done)
> > >
> > > 'struct device' is freed when a device is removed-- the reverse
> > > of the above.
> > 
> > Where is the device freed?  I see you trying to do some "odd" stuff in
> > fsl_mc_device_remove() by deleting and then putting a device structure.
> > I can't find a "release()" callback anywhere for your bus, where is it?
> > 
> > What happens when the reference count falls to 0 for your struct device?
> 
> Hrm...something seems wrong in free path, and I think this needs to
> be refactored.
> 
> IIRC, when German (former maintainer) wrote that code he loosely based
> it on the register/unregister platform bus code:
> 
> int platform_device_register(struct platform_device *pdev)
> {
> device_initialize(>dev);
> arch_setup_pdev_archdata(pdev);
> return platform_device_add(pdev);
> }
> void platform_device_unregister(struct platform_device *pdev)
> {
> platform_device_del(pdev);
> platform_device_put(pdev);
> }
> 
> ...I'm puzzling over how that code handles a refcount of zero
> as I see no 'release' callback anywhere, but I must be missing
> something.
> 
> In any case, we'll get this refactored.

Have you tried removing a device?  The kernel should complain loudly
about there not being a release function for your device.

> > > > - root_dprc_count, why are you using an atomic variable for
> > > >   this?  What is it for other than "look, I'm running!"?
> > >
> > > There can be multiple root buses, and this variable simply tracks the 
> > > count
> > > of them.
> > 
> > Why does it matter?
> > 
> > > It's is atomic there might be a theoretical race condition where 2
> > > buses might be added at the same time.  The root buses are found in
> > > the device tree and so if there is no chance that device tree
> > > processing happens in parallel on multiple cores then we could remove
> > > the atomic.
> > 
> > Why not just use a lock, or better yet, not care about a "count" at all?
> > I don't see you doing anything with the count, other than emitting a
> > WARN() if it drops down below 0 for some reason, or when you call
> > fsl_mc_bus_exists() which for some reason is exported yet no one uses
> > it...
> 
> We can drop this count.  At one time I think there was envisioned an 
> external user who needed it, but it's no longer the case.

Please do, we are trying to get rid of atomic_t abuse on other mailing
lists, and this one fits the pattern of "no real need for it" :)

> Given the additional refactoring, I think the fsl-mc bus driver needs
> to stay in staging for a bit.  In order to facilitate further review
> I'm going to refactor the patch series:
>   staging: 

Re: Still OOM problems with 4.9er kernels

[Gerhard Wiesinger]

Hello,

same with latest kernel rc, dnf still killed with OOM (but sometimes 
better).


./update.sh: line 40:  1591 Killed  ${EXE} update ${PARAMS}
(does dnf clean all;dnf update)
Linux database.intern 4.9.0-0.rc8.git2.1.fc26.x86_64 #1 SMP Wed Dec 7 
17:53:29 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux


Updated bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=1314697

Any chance to get it fixed in 4.9.0 release?

Ciao,
Gerhard


On 30.11.2016 08:20, Gerhard Wiesinger wrote:

Hello,

See also:
Bug 1314697 - Kernel 4.4.3-300.fc23.x86_64 is not stable inside a KVM VM
https://bugzilla.redhat.com/show_bug.cgi?id=1314697

Ciao,
Gerhard


On 30.11.2016 08:10, Gerhard Wiesinger wrote:

Hello,

I'm having out of memory situations with my "low memory" VMs in KVM 
under Fedora (Kernel 4.7, 4.8 and also before). They started to get 
more and more sensitive to OOM. I recently found the following info:


https://marius.bloggt-in-braunschweig.de/2016/11/17/linuxkernel-4-74-8-und-der-oom-killer/ 


https://www.spinics.net/lists/linux-mm/msg113661.html

Therefore I tried the latest Fedora kernels: 
4.9.0-0.rc6.git2.1.fc26.x86_64


But OOM situation is still very easy to reproduce:

1.) VM with 128-384MB under Fedora 25

2.) Having some processes run without any load (e.g. Apache)

3.) run an update with: dnf clean all; dnf update

4.) dnf python process get's killed


Please make the VM system working again in Kernel 4.9 and to use swap 
again correctly.


Thnx.

Ciao,

Gerhard

Re: Still OOM problems with 4.9er kernels

[Gerhard Wiesinger]

Hello,

same with latest kernel rc, dnf still killed with OOM (but sometimes 
better).


./update.sh: line 40:  1591 Killed  ${EXE} update ${PARAMS}
(does dnf clean all;dnf update)
Linux database.intern 4.9.0-0.rc8.git2.1.fc26.x86_64 #1 SMP Wed Dec 7 
17:53:29 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux


Updated bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=1314697

Any chance to get it fixed in 4.9.0 release?

Ciao,
Gerhard


On 30.11.2016 08:20, Gerhard Wiesinger wrote:

Hello,

See also:
Bug 1314697 - Kernel 4.4.3-300.fc23.x86_64 is not stable inside a KVM VM
https://bugzilla.redhat.com/show_bug.cgi?id=1314697

Ciao,
Gerhard


On 30.11.2016 08:10, Gerhard Wiesinger wrote:

Hello,

I'm having out of memory situations with my "low memory" VMs in KVM 
under Fedora (Kernel 4.7, 4.8 and also before). They started to get 
more and more sensitive to OOM. I recently found the following info:


https://marius.bloggt-in-braunschweig.de/2016/11/17/linuxkernel-4-74-8-und-der-oom-killer/ 


https://www.spinics.net/lists/linux-mm/msg113661.html

Therefore I tried the latest Fedora kernels: 
4.9.0-0.rc6.git2.1.fc26.x86_64


But OOM situation is still very easy to reproduce:

1.) VM with 128-384MB under Fedora 25

2.) Having some processes run without any load (e.g. Apache)

3.) run an update with: dnf clean all; dnf update

4.) dnf python process get's killed


Please make the VM system working again in Kernel 4.9 and to use swap 
again correctly.


Thnx.

Ciao,

Gerhard

Re: [PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Greg KH]

On Fri, Dec 09, 2016 at 12:29:21PM +, Manoj Sawai wrote:
> Errors - Complex macro not a parentheses and trailing whitespace
> Also fixed other small checkpatch warnings and checks.

If you ever say "also" in a changelog, that's a huge hint that the patch
needs to be broken up into multiple patches.  That is the case here,
please only do one type of coding style fix at a time.

thanks,

greg k-h

Re: [PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Greg KH]

On Fri, Dec 09, 2016 at 12:29:21PM +, Manoj Sawai wrote:
> Errors - Complex macro not a parentheses and trailing whitespace
> Also fixed other small checkpatch warnings and checks.

If you ever say "also" in a changelog, that's a huge hint that the patch
needs to be broken up into multiple patches.  That is the case here,
please only do one type of coding style fix at a time.

thanks,

greg k-h

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 04:48:18PM +, Måns Rullgård wrote:
> Vinod Koul  writes:
> 
> > To make it efficient, disregarding your Sbox HW issue, the solution is
> > virtual channels. You can delink physical channels and virtual channels. If
> > one has SW controlled MUX then a channel can service any client. For few
> > controllers request lines are hard wired so they cant use any channel. But
> > if you dont have this restriction then driver can queue up many transactions
> > from different controllers.
> 
> Have you been paying attention at all?  This exactly what the driver
> ALREADY DOES.

And have you read what the question was?

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 04:48:18PM +, Måns Rullgård wrote:
> Vinod Koul  writes:
> 
> > To make it efficient, disregarding your Sbox HW issue, the solution is
> > virtual channels. You can delink physical channels and virtual channels. If
> > one has SW controlled MUX then a channel can service any client. For few
> > controllers request lines are hard wired so they cant use any channel. But
> > if you dont have this restriction then driver can queue up many transactions
> > from different controllers.
> 
> Have you been paying attention at all?  This exactly what the driver
> ALREADY DOES.

And have you read what the question was?

-- 
~Vinod

[PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Manoj Sawai]

Errors - Complex macro not a parentheses and trailing whitespace
Also fixed other small checkpatch warnings and checks.

Signed-off-by: Manoj Sawai 
---
 drivers/staging/ks7010/ks7010_sdio.h | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/ks7010_sdio.h
index 0f5fd848e23d..f12d41835b03 100644
--- a/drivers/staging/ks7010/ks7010_sdio.h
+++ b/drivers/staging/ks7010/ks7010_sdio.h
@@ -46,7 +46,7 @@
  */
 #define WSTATUS_RSIZE  0x14
 #define WSTATUS_MASK   0x80/* Write Status Register value */
-#define RSIZE_MASK 0x7F/* Read Data Size Register value [10:4] 
*/
+#define RSIZE_MASK 0x7F// Read Data Size Register value [10:4]
 
 /* ARM to SD interrupt Enable */
 #define INT_ENABLE 0x20
@@ -81,11 +81,11 @@
 
 /* AHB Data Window  0x01-0x01 */
 #define DATA_WINDOW0x01
-#define WINDOW_SIZE64*1024
+#define WINDOW_SIZE(64 * 1024)
 
 #define KS7010_IRAM_ADDRESS0x0600
 
-/* 
+/*
  * struct define
  */
 struct hw_info_t {
@@ -115,7 +115,7 @@ struct ks_sdio_card {
 struct tx_device_buffer {
unsigned char *sendp;   /* pointer of send req data */
unsigned int size;
-   void (*complete_handler) (void *arg1, void *arg2);
+   void (*complete_handler)(void *arg1, void *arg2);
void *arg1;
void *arg2;
 };
@@ -142,6 +142,7 @@ struct rx_device {
unsigned int qtail; /* rx buffer queue last pointer */
spinlock_t rx_dev_lock;
 };
+
 #defineROM_FILE "ks7010sd.rom"
 
 #endif /* _KS7010_SDIO_H */
-- 
2.11.0

[PATCH] Staging: ks7010: ks7010_sdio.h: Fixed coding style errors

[Manoj Sawai]

Errors - Complex macro not a parentheses and trailing whitespace
Also fixed other small checkpatch warnings and checks.

Signed-off-by: Manoj Sawai 
---
 drivers/staging/ks7010/ks7010_sdio.h | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/ks7010_sdio.h
index 0f5fd848e23d..f12d41835b03 100644
--- a/drivers/staging/ks7010/ks7010_sdio.h
+++ b/drivers/staging/ks7010/ks7010_sdio.h
@@ -46,7 +46,7 @@
  */
 #define WSTATUS_RSIZE  0x14
 #define WSTATUS_MASK   0x80/* Write Status Register value */
-#define RSIZE_MASK 0x7F/* Read Data Size Register value [10:4] 
*/
+#define RSIZE_MASK 0x7F// Read Data Size Register value [10:4]
 
 /* ARM to SD interrupt Enable */
 #define INT_ENABLE 0x20
@@ -81,11 +81,11 @@
 
 /* AHB Data Window  0x01-0x01 */
 #define DATA_WINDOW0x01
-#define WINDOW_SIZE64*1024
+#define WINDOW_SIZE(64 * 1024)
 
 #define KS7010_IRAM_ADDRESS0x0600
 
-/* 
+/*
  * struct define
  */
 struct hw_info_t {
@@ -115,7 +115,7 @@ struct ks_sdio_card {
 struct tx_device_buffer {
unsigned char *sendp;   /* pointer of send req data */
unsigned int size;
-   void (*complete_handler) (void *arg1, void *arg2);
+   void (*complete_handler)(void *arg1, void *arg2);
void *arg1;
void *arg2;
 };
@@ -142,6 +142,7 @@ struct rx_device {
unsigned int qtail; /* rx buffer queue last pointer */
spinlock_t rx_dev_lock;
 };
+
 #defineROM_FILE "ks7010sd.rom"
 
 #endif /* _KS7010_SDIO_H */
-- 
2.11.0

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
> I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> Eliminating the lock since the sock is dead anways eliminates the error.
>
> Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try to
> get the test case to compile.

It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
are updated as a whole and race between audit_receive_msg() and
NETLINK_URELEASE.


> @@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> +
> +   mutex_lock(_cmd_mutex);
> if (sock == audit_sock) {
> audit_pid = 0;
> +   audit_nlk_portid = 0;
> audit_sock = NULL;
> }
> +   mutex_unlock(_cmd_mutex);
>

If you decide to use NETLINK_URELEASE notifier, the above piece is no
longer needed, the net_exit path simply releases a refcnt.

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
> I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> Eliminating the lock since the sock is dead anways eliminates the error.
>
> Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try to
> get the test case to compile.

It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
are updated as a whole and race between audit_receive_msg() and
NETLINK_URELEASE.


> @@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> +
> +   mutex_lock(_cmd_mutex);
> if (sock == audit_sock) {
> audit_pid = 0;
> +   audit_nlk_portid = 0;
> audit_sock = NULL;
> }
> +   mutex_unlock(_cmd_mutex);
>

If you decide to use NETLINK_URELEASE notifier, the above piece is no
longer needed, the net_exit path simply releases a refcnt.

[PATCH] firmware: dmi_scan: Always show system identification string

[Kefeng Wang]

Let's keep consistent when print dmi_ids_string between SMBIOS 2.x
and SMBIOS 3.x, and always show the system identification string,
like Vendor, Product/Board name and BIOS infos.

Signed-off-by: Kefeng Wang 
---
 drivers/firmware/dmi_scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 88bebe1..54be60e 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -560,7 +560,7 @@ static int __init dmi_present(const u8 *buf)
dmi_ver >> 16, (dmi_ver >> 8) & 0xFF);
}
dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
-   printk(KERN_DEBUG "DMI: %s\n", dmi_ids_string);
+   pr_info("DMI: %s\n", dmi_ids_string);
return 0;
}
}
@@ -588,7 +588,7 @@ static int __init dmi_smbios3_present(const u8 *buf)
dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
dmi_ver & 0xFF);
dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
-   pr_debug("DMI: %s\n", dmi_ids_string);
+   pr_info("DMI: %s\n", dmi_ids_string);
return 0;
}
}
-- 
1.7.12.4

[PATCH] firmware: dmi_scan: Always show system identification string

[Kefeng Wang]

Let's keep consistent when print dmi_ids_string between SMBIOS 2.x
and SMBIOS 3.x, and always show the system identification string,
like Vendor, Product/Board name and BIOS infos.

Signed-off-by: Kefeng Wang 
---
 drivers/firmware/dmi_scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 88bebe1..54be60e 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -560,7 +560,7 @@ static int __init dmi_present(const u8 *buf)
dmi_ver >> 16, (dmi_ver >> 8) & 0xFF);
}
dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
-   printk(KERN_DEBUG "DMI: %s\n", dmi_ids_string);
+   pr_info("DMI: %s\n", dmi_ids_string);
return 0;
}
}
@@ -588,7 +588,7 @@ static int __init dmi_smbios3_present(const u8 *buf)
dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
dmi_ver & 0xFF);
dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
-   pr_debug("DMI: %s\n", dmi_ids_string);
+   pr_info("DMI: %s\n", dmi_ids_string);
return 0;
}
}
-- 
1.7.12.4

RE: ATH9 driver issues on ARM64

[Bharat Kumar Gogada]

Sorry, Forgot to add kernel version, we are using 4.6 kernel. 

> Hi,
> Can any one tell, when exactly the chip sends ASSERT & DEASSERT in driver.
> It might help us to debug issue further.
> 
> Thanks & Regards,
> Bharat
> 
> > >  > [+cc Kalle, ath9k list]
> >
> > Thanks, but please also CC linux-wireless. Full thread below for the folks 
> > there.
> >
> > >> On Thu, Dec 08, 2016 at 01:49:42PM +, Bharat Kumar Gogada wrote:
> > >> > Hi,
> > >> >
> > >> > Did anyone test Atheros ATH9
> > >> > driver(drivers/net/wireless/ath/ath9k/)
> > >> > on ARM64.  The end point is TP link wifi card with which supports
> > >> > only legacy interrupts.
> > >>
> > >> If it works on other arches and the arm64 PCI enumeration works, my
> > >> first guess would be an INTx issue, e.g., maybe the driver is
> > >> waiting for an interrupt that never arrives.
> > > We are not sure for now.
> > >>
> > >> > We are trying to test it on ARM64 with
> > >> > (drivers/pci/host/pcie-xilinx-nwl.c) as root port.
> > >> >
> > >> > EP is getting enumerated and able to link up.
> > >> >
> > >> > But when we start scan system gets hanged.
> > >>
> > >> When you say the system hangs when you start a scan, I assume you
> > >> mean a wifi scan, not the PCI enumeration.  A problem with a wifi
> > >> scan might cause a *process* to hang, but it shouldn't hang the
> > >> entire system.
> > >>
> > > Yes wifi scan.
> > >> > When we took trace we see that after we start scan assert message
> > >> > is sent but there is no de assert from end point.
> > >>
> > >> Are you talking about a trace from a PCIe analyzer?  Do you see an
> > >> Assert_INTx PCIe message on the link?
> > >>
> > > Yes lecroy trace, yes we do see Assert_INTx and Deassert_INTx
> > > happening
> > when we do interface link up.
> > > When we have less debug prints in Atheros driver, and do wifi scan
> > > we see Assert_INTx but never Deassert_INTx,
> > >> > What might cause end point not sending de assert ?
> > >>
> > >> If the endpoint doesn't send a Deassert_INTx message, I expect that
> > >> would mean the driver didn't service the interrupt and remove the
> > >> condition that caused the device to assert the interrupt in the
> > >> first place.
> > >>
> > >> If the driver didn't receive the interrupt, it couldn't service it,
> > >> of course.  You could add a printk in the ath9k interrupt service
> > >> routine to see if you ever get there.
> > >>
> > > The interrupt behavior is changing w.r.t amount of debug prints we
> > > add. (I kept many prints to aid debug) root@Xilinx-ZCU102-2016_3:~#
> > > iw dev
> > wlan0 scan
> > > [   83.064675] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.069486] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.074257] ath9k_hw_kill_interrupts793
> > > [   83.078260] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.083107] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.087882] ath9k_hw_kill_interrupts793
> > > [   83.095450] ath9k_hw_enable_interrupts  821
> > > [   83.099557] ath9k_hw_enable_interrupts  825
> > > [   83.103721] ath9k_hw_enable_interrupts  832
> > > [   83.107887] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.112748] AR_SREV_9100 0
> > > [   83.115438] ath9k_hw_enable_interrupts  848
> > > [   83.119607] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.124389] ath9k_hw_intrpend   762
> > > [   83.127761] (AR_SREV_9340(ah) val 0
> > > [   83.131234] ath9k_hw_intrpend   767
> > > [   83.134628] ath_isr 603
> > > [   83.137134] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.141995] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.146771] ath9k_hw_kill_interrupts793
> > > [   83.150864] ath9k_hw_enable_interrupts  821
> > > [   83.154971] ath9k_hw_enable_interrupts  825
> > > [   83.159135] ath9k_hw_enable_interrupts  832
> > > [   83.163300] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.168161] AR_SREV_9100 0
> > > [   83.170852] ath9k_hw_enable_interrupts  848
> > > [   83.170855] ath9k_hw_intrpend   762
> > > [   83.178398] (AR_SREV_9340(ah) val 0
> > > [   83.181873] ath9k_hw_intrpend   767
> > > [   83.185265] ath_isr 603
> > > [   83.187773] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.192635] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.197411] ath9k_hw_kill_interrupts793
> > > [   83.201414] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.206258] ath9k_hw_enable_interrupts  821
> > > [   83.210368] ath9k_hw_enable_interrupts  825
> > > [   83.214531] ath9k_hw_enable_interrupts  832
> > > [   83.218698] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.223558] AR_SREV_9100 0
> > > [   83.226243] ath9k_hw_enable_interrupts  848
> > > [   83.226246] ath9k_hw_intrpend   762
> > > [   83.233794] (AR_SREV_9340(ah) val 0
> > > [   83.237268] ath9k_hw_intrpend   767
> > > [   83.240661] ath_isr 603
> > > [   83.243169] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.248030] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.252806] 

RE: ATH9 driver issues on ARM64

[Bharat Kumar Gogada]

Sorry, Forgot to add kernel version, we are using 4.6 kernel. 

> Hi,
> Can any one tell, when exactly the chip sends ASSERT & DEASSERT in driver.
> It might help us to debug issue further.
> 
> Thanks & Regards,
> Bharat
> 
> > >  > [+cc Kalle, ath9k list]
> >
> > Thanks, but please also CC linux-wireless. Full thread below for the folks 
> > there.
> >
> > >> On Thu, Dec 08, 2016 at 01:49:42PM +, Bharat Kumar Gogada wrote:
> > >> > Hi,
> > >> >
> > >> > Did anyone test Atheros ATH9
> > >> > driver(drivers/net/wireless/ath/ath9k/)
> > >> > on ARM64.  The end point is TP link wifi card with which supports
> > >> > only legacy interrupts.
> > >>
> > >> If it works on other arches and the arm64 PCI enumeration works, my
> > >> first guess would be an INTx issue, e.g., maybe the driver is
> > >> waiting for an interrupt that never arrives.
> > > We are not sure for now.
> > >>
> > >> > We are trying to test it on ARM64 with
> > >> > (drivers/pci/host/pcie-xilinx-nwl.c) as root port.
> > >> >
> > >> > EP is getting enumerated and able to link up.
> > >> >
> > >> > But when we start scan system gets hanged.
> > >>
> > >> When you say the system hangs when you start a scan, I assume you
> > >> mean a wifi scan, not the PCI enumeration.  A problem with a wifi
> > >> scan might cause a *process* to hang, but it shouldn't hang the
> > >> entire system.
> > >>
> > > Yes wifi scan.
> > >> > When we took trace we see that after we start scan assert message
> > >> > is sent but there is no de assert from end point.
> > >>
> > >> Are you talking about a trace from a PCIe analyzer?  Do you see an
> > >> Assert_INTx PCIe message on the link?
> > >>
> > > Yes lecroy trace, yes we do see Assert_INTx and Deassert_INTx
> > > happening
> > when we do interface link up.
> > > When we have less debug prints in Atheros driver, and do wifi scan
> > > we see Assert_INTx but never Deassert_INTx,
> > >> > What might cause end point not sending de assert ?
> > >>
> > >> If the endpoint doesn't send a Deassert_INTx message, I expect that
> > >> would mean the driver didn't service the interrupt and remove the
> > >> condition that caused the device to assert the interrupt in the
> > >> first place.
> > >>
> > >> If the driver didn't receive the interrupt, it couldn't service it,
> > >> of course.  You could add a printk in the ath9k interrupt service
> > >> routine to see if you ever get there.
> > >>
> > > The interrupt behavior is changing w.r.t amount of debug prints we
> > > add. (I kept many prints to aid debug) root@Xilinx-ZCU102-2016_3:~#
> > > iw dev
> > wlan0 scan
> > > [   83.064675] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.069486] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.074257] ath9k_hw_kill_interrupts793
> > > [   83.078260] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.083107] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.087882] ath9k_hw_kill_interrupts793
> > > [   83.095450] ath9k_hw_enable_interrupts  821
> > > [   83.099557] ath9k_hw_enable_interrupts  825
> > > [   83.103721] ath9k_hw_enable_interrupts  832
> > > [   83.107887] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.112748] AR_SREV_9100 0
> > > [   83.115438] ath9k_hw_enable_interrupts  848
> > > [   83.119607] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.124389] ath9k_hw_intrpend   762
> > > [   83.127761] (AR_SREV_9340(ah) val 0
> > > [   83.131234] ath9k_hw_intrpend   767
> > > [   83.134628] ath_isr 603
> > > [   83.137134] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.141995] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.146771] ath9k_hw_kill_interrupts793
> > > [   83.150864] ath9k_hw_enable_interrupts  821
> > > [   83.154971] ath9k_hw_enable_interrupts  825
> > > [   83.159135] ath9k_hw_enable_interrupts  832
> > > [   83.163300] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.168161] AR_SREV_9100 0
> > > [   83.170852] ath9k_hw_enable_interrupts  848
> > > [   83.170855] ath9k_hw_intrpend   762
> > > [   83.178398] (AR_SREV_9340(ah) val 0
> > > [   83.181873] ath9k_hw_intrpend   767
> > > [   83.185265] ath_isr 603
> > > [   83.187773] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.192635] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.197411] ath9k_hw_kill_interrupts793
> > > [   83.201414] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.206258] ath9k_hw_enable_interrupts  821
> > > [   83.210368] ath9k_hw_enable_interrupts  825
> > > [   83.214531] ath9k_hw_enable_interrupts  832
> > > [   83.218698] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.223558] AR_SREV_9100 0
> > > [   83.226243] ath9k_hw_enable_interrupts  848
> > > [   83.226246] ath9k_hw_intrpend   762
> > > [   83.233794] (AR_SREV_9340(ah) val 0
> > > [   83.237268] ath9k_hw_intrpend   767
> > > [   83.240661] ath_isr 603
> > > [   83.243169] ath9k: ath9k_iowrite32 ff800a400024
> > > [   83.248030] ath9k: ath9k_ioread32 ff800a400024
> > > [   83.252806] 

Re: [RFC PATCH] mm: introduce kv[mz]alloc helpers

[Michal Hocko]

On Fri 09-12-16 06:38:04, Al Viro wrote:
> On Fri, Dec 09, 2016 at 07:22:25AM +0100, Michal Hocko wrote:
> 
> > > Easier to handle those in vmalloc() itself.
> > 
> > I think there were some attempts in the past but some of the code paths
> > are burried too deep and adding gfp_mask all the way down there seemed
> > like a major surgery.
> 
> No need to propagate gfp_mask - the same trick XFS is doing right now can
> be done in vmalloc.c in a couple of places and that's it; I'll resurrect the
> patches and post them tomorrow after I get some sleep.

That would work as an immediate mitigation. No question about that but
what I've tried to point out in the reply to Dave is that longerm we
shouldn't hide this trickiness inside the vmalloc and rather handle
those users who are requesting NOFS/NOIO context from vmalloc. We
already have a scope api for NOIO and I want to add the same for NOFS.
I believe that much more sane approach is to use the API at those places
which really start/stop reclaim recursion dangerous scope (e.g. the
transaction context) rather than using GFP_NOFS randomly because this
approach has proven to not work properly over years. We have so many
place using GFP_NOFS just because nobody bothered to think whether it is
needed but it must be safe for sure that it is not funny.

-- 
Michal Hocko
SUSE Labs

Re: [RFC PATCH] mm: introduce kv[mz]alloc helpers

[Michal Hocko]

On Fri 09-12-16 06:38:04, Al Viro wrote:
> On Fri, Dec 09, 2016 at 07:22:25AM +0100, Michal Hocko wrote:
> 
> > > Easier to handle those in vmalloc() itself.
> > 
> > I think there were some attempts in the past but some of the code paths
> > are burried too deep and adding gfp_mask all the way down there seemed
> > like a major surgery.
> 
> No need to propagate gfp_mask - the same trick XFS is doing right now can
> be done in vmalloc.c in a couple of places and that's it; I'll resurrect the
> patches and post them tomorrow after I get some sleep.

That would work as an immediate mitigation. No question about that but
what I've tried to point out in the reply to Dave is that longerm we
shouldn't hide this trickiness inside the vmalloc and rather handle
those users who are requesting NOFS/NOIO context from vmalloc. We
already have a scope api for NOIO and I want to add the same for NOFS.
I believe that much more sane approach is to use the API at those places
which really start/stop reclaim recursion dangerous scope (e.g. the
transaction context) rather than using GFP_NOFS randomly because this
approach has proven to not work properly over years. We have so many
place using GFP_NOFS just because nobody bothered to think whether it is
needed but it must be safe for sure that it is not funny.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] pci-error-recover: doc cleanup

[Andrew Donnellan]

On 09/12/16 17:24, Linas Vepstas wrote:

I suppose I'm confused, but I recall that link resets are non-fatal.
Fatal errors typically require that the the pci adapter be completely
reset, any adapter firmware to be reloaded from scratch, the device
driver has to kill all device state and start from scratch. Its huge.


Is there a difference in terminology between an AER fatal error and what 
EEH/IBM people think of as a fatal error?



If the fatal error is on pci device that is under a block device
holding a file system, then (usually) there is no way to recover,
because the block layer (and file system) cannot deal with a block
device that disappeared and then reappeared some few seconds later.
(maybe some future zfs or lvm or btrfs might be able to deal with
this, but not today)


Is this still true? I'm not at all familiar with the block device side 
of it, but the cxlflash driver has reasonably full EEH support, 
including surviving a full PHB fence and complete reset.


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

Re: [PATCH] pci-error-recover: doc cleanup

[Andrew Donnellan]

On 09/12/16 17:24, Linas Vepstas wrote:

I suppose I'm confused, but I recall that link resets are non-fatal.
Fatal errors typically require that the the pci adapter be completely
reset, any adapter firmware to be reloaded from scratch, the device
driver has to kill all device state and start from scratch. Its huge.


Is there a difference in terminology between an AER fatal error and what 
EEH/IBM people think of as a fatal error?



If the fatal error is on pci device that is under a block device
holding a file system, then (usually) there is no way to recover,
because the block layer (and file system) cannot deal with a block
device that disappeared and then reappeared some few seconds later.
(maybe some future zfs or lvm or btrfs might be able to deal with
this, but not today)


Is this still true? I'm not at all familiar with the block device side 
of it, but the cxlflash driver has reasonably full EEH support, 
including surviving a full PHB fence and complete reset.


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

Re: [PATCH 4/7] blk-flush: run the queue when inserting blk-mq flush

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Currently we pass in to run the queue async, but don't flag the
queue to be run. We don't need to run it async here, but we should
run it. So fixup the parameters.

Signed-off-by: Jens Axboe 
---
 block/blk-flush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 1bdbb3d3e5f5..27a42dab5a36 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -426,7 +426,7 @@ void blk_insert_flush(struct request *rq)
if ((policy & REQ_FSEQ_DATA) &&
!(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
if (q->mq_ops) {
-   blk_mq_insert_request(rq, false, false, true);
+   blk_mq_insert_request(rq, false, true, false);
} else
list_add_tail(>queuelist, >queue_head);
return;


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 4/7] blk-flush: run the queue when inserting blk-mq flush

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Currently we pass in to run the queue async, but don't flag the
queue to be run. We don't need to run it async here, but we should
run it. So fixup the parameters.

Signed-off-by: Jens Axboe 
---
 block/blk-flush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 1bdbb3d3e5f5..27a42dab5a36 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -426,7 +426,7 @@ void blk_insert_flush(struct request *rq)
if ((policy & REQ_FSEQ_DATA) &&
!(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
if (q->mq_ops) {
-   blk_mq_insert_request(rq, false, false, true);
+   blk_mq_insert_request(rq, false, true, false);
} else
list_add_tail(>queuelist, >queue_head);
return;


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/7] blk-mq: abstract out blk_mq_dispatch_rq_list() helper

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Takes a list of requests, and dispatches it. Moves any residual
requests to the dispatch list.

Signed-off-by: Jens Axboe 
---
 block/blk-mq.c | 85 --
 block/blk-mq.h |  1 +
 2 files changed, 48 insertions(+), 38 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/7] blk-mq: abstract out blk_mq_dispatch_rq_list() helper

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Takes a list of requests, and dispatches it. Moves any residual
requests to the dispatch list.

Signed-off-by: Jens Axboe 
---
 block/blk-mq.c | 85 --
 block/blk-mq.h |  1 +
 2 files changed, 48 insertions(+), 38 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/7] elevator: make the rqhash helpers exported

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Signed-off-by: Jens Axboe 
---
 block/elevator.c | 8 
 include/linux/elevator.h | 5 +
 2 files changed, 9 insertions(+), 4 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] pci-error-recover: doc cleanup

[Linas Vepstas]

On Fri, Dec 9, 2016 at 2:37 PM, Cao jin  wrote:
>
>
> On 12/09/2016 02:24 PM, Linas Vepstas wrote:
>> I suppose I'm confused, but I recall that link resets are non-fatal.
>> Fatal errors typically require that the the pci adapter be completely
>> reset, any adapter firmware to be reloaded from scratch, the device
>> driver has to kill all device state and start from scratch. Its huge.
>> If the fatal error is on pci device that is under a block device
>> holding a file system, then (usually) there is no way to recover,
>> because the block layer (and file system) cannot deal with a block
>> device that disappeared and then reappeared some few seconds later.
>> (maybe some future zfs or lvm or btrfs might be able to deal with
>> this, but not today)
>>
>> By contrast, link resets are far more gentle: the device driver might
>> have to discard some half-full FIFO's, or cancel some in-flight
>> commands, but can otherwise gracefully recover without telling the
>> higher layers that there were any problems.
>>
>> --linas
>>
>
> I am little confused too, even not sure if we are talking the same
> *fatal error*, I am talking the fatal error defined in PCI Express spec,
> chapter 6.2.2.2.1:
>
> Fatal errors are uncorrectable error conditions which render the
> particular Link and related hardware unreliable. For Fatal errors, a
> reset of the components on the Link may be required to return to
> reliable operation. Platform handling of Fatal errors, and any efforts
> to limit the effects of these errors, is platform implementation specific.
>
> Link reset means set *secondary bus reset* bit in pci bridge config
> space, can reset the link and device simultaneously, is the strongest
> kind of reset as I know.

OK, well, its been far too many years, and I don't have the PCI spec
at my fingertips.
Isn't there a link reset that can be performed, without forcing a device reset?

The intent was that some PCI link errors are due to vibration,
ground-bounce, humidity, etc. and that these errors can be detected
and do not corrupt the device state or the device driver state.  Since
they are not associated with data corruption (or rather, the
corruption is local to the link), these can be recovered by reseting
just the link, without resetting the whole adapter. They may require
reseting some device-driver state, but not all of it.

However, this was all decided before the PCI-E spec was written, so
maybe the newer PCI-E specs now say something different.

--linas

>
>> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:
>>>
>>>
>>> On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
 On Thu, 8 Dec 2016 16:16:14 +0800
 Cao jin  wrote:

>  The platform resets the link, and then calls the link_reset() callback
>  on all affected device drivers.  This is a PCI-Express specific state
> -and is done whenever a non-fatal error has been detected that can be
> +and is done whenever a fatal error has been detected that can be
>  "solved" by resetting the link. This call informs the driver of the

 As far as I can tell, the original text was correct here; why do you
 think this change needs to be made?

>>>
>>> See do_recovery() in aer core, reset_link() is called only seeing fatal
>>> error.
>>>
>>> --
>>> Sincerely,
>>> Cao jin
>>>
>>>
>>
>>
>>
>
> --
> Sincerely,
> Cao jin
>
>

Re: [PATCH 3/7] elevator: make the rqhash helpers exported

[Hannes Reinecke]

On 12/08/2016 09:13 PM, Jens Axboe wrote:

Signed-off-by: Jens Axboe 
---
 block/elevator.c | 8 
 include/linux/elevator.h | 5 +
 2 files changed, 9 insertions(+), 4 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] pci-error-recover: doc cleanup

[Linas Vepstas]

On Fri, Dec 9, 2016 at 2:37 PM, Cao jin  wrote:
>
>
> On 12/09/2016 02:24 PM, Linas Vepstas wrote:
>> I suppose I'm confused, but I recall that link resets are non-fatal.
>> Fatal errors typically require that the the pci adapter be completely
>> reset, any adapter firmware to be reloaded from scratch, the device
>> driver has to kill all device state and start from scratch. Its huge.
>> If the fatal error is on pci device that is under a block device
>> holding a file system, then (usually) there is no way to recover,
>> because the block layer (and file system) cannot deal with a block
>> device that disappeared and then reappeared some few seconds later.
>> (maybe some future zfs or lvm or btrfs might be able to deal with
>> this, but not today)
>>
>> By contrast, link resets are far more gentle: the device driver might
>> have to discard some half-full FIFO's, or cancel some in-flight
>> commands, but can otherwise gracefully recover without telling the
>> higher layers that there were any problems.
>>
>> --linas
>>
>
> I am little confused too, even not sure if we are talking the same
> *fatal error*, I am talking the fatal error defined in PCI Express spec,
> chapter 6.2.2.2.1:
>
> Fatal errors are uncorrectable error conditions which render the
> particular Link and related hardware unreliable. For Fatal errors, a
> reset of the components on the Link may be required to return to
> reliable operation. Platform handling of Fatal errors, and any efforts
> to limit the effects of these errors, is platform implementation specific.
>
> Link reset means set *secondary bus reset* bit in pci bridge config
> space, can reset the link and device simultaneously, is the strongest
> kind of reset as I know.

OK, well, its been far too many years, and I don't have the PCI spec
at my fingertips.
Isn't there a link reset that can be performed, without forcing a device reset?

The intent was that some PCI link errors are due to vibration,
ground-bounce, humidity, etc. and that these errors can be detected
and do not corrupt the device state or the device driver state.  Since
they are not associated with data corruption (or rather, the
corruption is local to the link), these can be recovered by reseting
just the link, without resetting the whole adapter. They may require
reseting some device-driver state, but not all of it.

However, this was all decided before the PCI-E spec was written, so
maybe the newer PCI-E specs now say something different.

--linas

>
>> On Thu, Dec 8, 2016 at 10:13 PM, Cao jin  wrote:
>>>
>>>
>>> On 12/08/2016 10:05 PM, Jonathan Corbet wrote:
 On Thu, 8 Dec 2016 16:16:14 +0800
 Cao jin  wrote:

>  The platform resets the link, and then calls the link_reset() callback
>  on all affected device drivers.  This is a PCI-Express specific state
> -and is done whenever a non-fatal error has been detected that can be
> +and is done whenever a fatal error has been detected that can be
>  "solved" by resetting the link. This call informs the driver of the

 As far as I can tell, the original text was correct here; why do you
 think this change needs to be made?

>>>
>>> See do_recovery() in aer core, reset_link() is called only seeing fatal
>>> error.
>>>
>>> --
>>> Sincerely,
>>> Cao jin
>>>
>>>
>>
>>
>>
>
> --
> Sincerely,
> Cao jin
>
>

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

[Al Viro]

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?

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

[Al Viro]

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?

Re: [PATCH] linux/types.h: enable endian checks for all sparse builds

[Madhani, Himanshu]

Hi Mike/Bart, 







On 12/8/16, 8:17 AM, "virtualization-boun...@lists.linux-foundation.org on 
behalf of Michael S. Tsirkin" 
 wrote:

>On Thu, Dec 08, 2016 at 06:38:11AM +, Bart Van Assche wrote:
>> On 12/07/16 21:54, Michael S. Tsirkin wrote:
>> > On Thu, Dec 08, 2016 at 05:21:47AM +, Bart Van Assche wrote:
>> >> Additionally, there are notable exceptions to the rule that most drivers
>> >> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it
>> >> would remain possible to check such drivers with sparse without enabling
>> >> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__
>> >> into e.g. #ifndef __DONT_CHECK_ENDIAN__?
>> >
>> > The right thing is probably just to fix these, isn't it?
>> > Until then, why not just ignore the warnings?
>> 
>> Neither option is realistic. With endian-checking enabled the qla2xxx 
>> driver triggers so many warnings that it becomes a real challenge to 
>> filter the non-endian warnings out manually:
>> 
>> $ for f in "" CF=-D__CHECK_ENDIAN__; do make M=drivers/scsi/qla2xxx C=2\
>>  $f |  -c ': warning:'; done
>> 4
>> 752
>
>You can always revert this patch in your tree, or whatever.  It does not
>look like this will get fixed otherwise.
>
>> If you think it would be easy to fix the endian warnings triggered by 
>> the qla2xxx driver, you are welcome to try to fix these.
>> 
>> Bart.
>
>Yea, this hardware was designed by someone who thought mixing
>LE and BE all over the place is a good idea.
>But who said it should be easy?
>
>Maybe this change will be enough to motivate the maintainers.
>
>Here's a minor buglet for you as a motivator:
>
>if (ct_rsp->header.response !=
>cpu_to_be16(CT_ACCEPT_RESPONSE)) {
>ql_dbg(ql_dbg_disc + ql_dbg_buffer, vha, 
> 0x2077,
>"%s failed rejected request on port_id: 
> %02x%02x%02x Compeltion status 0x%x, response 0x%x\n",
>routine, vha->d_id.b.domain,
>vha->d_id.b.area, vha->d_id.b.al_pa, 
> comp_status, ct_rsp->header.response);
>
>
>response is BE and isn't printed correctly.
>
>another:
>
>eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
>size += 4 + 4;
>
>ql_dbg(ql_dbg_disc, vha, 0x20bc,
>"Max_Frame_Size = %x.\n", eiter->a.max_frame_size);
>
>printed too late, it's be by that time.
>
>Here's another suspicious line
>
>ctio24->u.status1.flags = (atio->u.isp24.attr << 9) |
>cpu_to_le16(CTIO7_FLAGS_STATUS_MODE_1 |
>CTIO7_FLAGS_TERMINATE);
>
>shifting attr by 9 bits gives different results on BE and LE,
>mixing it with le16 looks rather strange.
>
>Another:
>
>ha->flags.dport_enabled =
>(mid_init_cb->init_cb.firmware_options_1 & BIT_7) != 0;
>
>BIT_7 is native endian, firmware_options_1 is LE I think.
>
>
>
>Look at qla27xx_find_valid_image as well.
>
>if (pri_image_status.signature != QLA27XX_IMG_STATUS_SIGN)
>
>qla27xx_image_status seems to be data coming from flash, but is
>somehow native-endian? Maybe ...
>
>
>lun = a->u.isp24.fcp_cmnd.lun;
>
>I think lun here is in hardware format (le?), code treats it
>as native.
>
>
>Not to speak about interface abuse all over the place.
>How about this:
>
>uint32_t *
>qla24xx_read_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t
>faddr,
>uint32_t dwords) 
>{
>uint32_t i; 
>struct qla_hw_data *ha = vha->hw;
>
>/* Dword reads to flash. */
>for (i = 0; i < dwords; i++, faddr++)
>dwptr[i] = cpu_to_le32(qla24xx_read_flash_dword(ha,
>flash_data_addr(ha, faddr)));
>
>return dwptr;   
>}
>
>OK so we convert to LE ...
>
>qla24xx_read_flash_data(vha, dcode, faddr, 4); 
>
>risc_addr = be32_to_cpu(dcode[2]);
>*srisc_addr = *srisc_addr == 0 ? risc_addr : *srisc_addr;
>risc_size = be32_to_cpu(dcode[3]);
>
>then happily assume it's BE.
>
>And again, coming from flash, it's unlikely to actually be in the native
>endian-ness as callers seem to assume. I'm guessing it's all BE.
>
>I poked at it a bit and was able to cut down # of warnings
>from 1700 to 1400 in an hour. Someone familiar with the code
>should look at it.

We’ll take a look and send patches to resolve these warnings. 

>
>-- 
>MST
>___
>Virtualization mailing list
>virtualizat...@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] linux/types.h: enable endian checks for all sparse builds

[Madhani, Himanshu]

Hi Mike/Bart, 







On 12/8/16, 8:17 AM, "virtualization-boun...@lists.linux-foundation.org on 
behalf of Michael S. Tsirkin" 
 wrote:

>On Thu, Dec 08, 2016 at 06:38:11AM +, Bart Van Assche wrote:
>> On 12/07/16 21:54, Michael S. Tsirkin wrote:
>> > On Thu, Dec 08, 2016 at 05:21:47AM +, Bart Van Assche wrote:
>> >> Additionally, there are notable exceptions to the rule that most drivers
>> >> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it
>> >> would remain possible to check such drivers with sparse without enabling
>> >> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__
>> >> into e.g. #ifndef __DONT_CHECK_ENDIAN__?
>> >
>> > The right thing is probably just to fix these, isn't it?
>> > Until then, why not just ignore the warnings?
>> 
>> Neither option is realistic. With endian-checking enabled the qla2xxx 
>> driver triggers so many warnings that it becomes a real challenge to 
>> filter the non-endian warnings out manually:
>> 
>> $ for f in "" CF=-D__CHECK_ENDIAN__; do make M=drivers/scsi/qla2xxx C=2\
>>  $f |  -c ': warning:'; done
>> 4
>> 752
>
>You can always revert this patch in your tree, or whatever.  It does not
>look like this will get fixed otherwise.
>
>> If you think it would be easy to fix the endian warnings triggered by 
>> the qla2xxx driver, you are welcome to try to fix these.
>> 
>> Bart.
>
>Yea, this hardware was designed by someone who thought mixing
>LE and BE all over the place is a good idea.
>But who said it should be easy?
>
>Maybe this change will be enough to motivate the maintainers.
>
>Here's a minor buglet for you as a motivator:
>
>if (ct_rsp->header.response !=
>cpu_to_be16(CT_ACCEPT_RESPONSE)) {
>ql_dbg(ql_dbg_disc + ql_dbg_buffer, vha, 
> 0x2077,
>"%s failed rejected request on port_id: 
> %02x%02x%02x Compeltion status 0x%x, response 0x%x\n",
>routine, vha->d_id.b.domain,
>vha->d_id.b.area, vha->d_id.b.al_pa, 
> comp_status, ct_rsp->header.response);
>
>
>response is BE and isn't printed correctly.
>
>another:
>
>eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
>size += 4 + 4;
>
>ql_dbg(ql_dbg_disc, vha, 0x20bc,
>"Max_Frame_Size = %x.\n", eiter->a.max_frame_size);
>
>printed too late, it's be by that time.
>
>Here's another suspicious line
>
>ctio24->u.status1.flags = (atio->u.isp24.attr << 9) |
>cpu_to_le16(CTIO7_FLAGS_STATUS_MODE_1 |
>CTIO7_FLAGS_TERMINATE);
>
>shifting attr by 9 bits gives different results on BE and LE,
>mixing it with le16 looks rather strange.
>
>Another:
>
>ha->flags.dport_enabled =
>(mid_init_cb->init_cb.firmware_options_1 & BIT_7) != 0;
>
>BIT_7 is native endian, firmware_options_1 is LE I think.
>
>
>
>Look at qla27xx_find_valid_image as well.
>
>if (pri_image_status.signature != QLA27XX_IMG_STATUS_SIGN)
>
>qla27xx_image_status seems to be data coming from flash, but is
>somehow native-endian? Maybe ...
>
>
>lun = a->u.isp24.fcp_cmnd.lun;
>
>I think lun here is in hardware format (le?), code treats it
>as native.
>
>
>Not to speak about interface abuse all over the place.
>How about this:
>
>uint32_t *
>qla24xx_read_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t
>faddr,
>uint32_t dwords) 
>{
>uint32_t i; 
>struct qla_hw_data *ha = vha->hw;
>
>/* Dword reads to flash. */
>for (i = 0; i < dwords; i++, faddr++)
>dwptr[i] = cpu_to_le32(qla24xx_read_flash_dword(ha,
>flash_data_addr(ha, faddr)));
>
>return dwptr;   
>}
>
>OK so we convert to LE ...
>
>qla24xx_read_flash_data(vha, dcode, faddr, 4); 
>
>risc_addr = be32_to_cpu(dcode[2]);
>*srisc_addr = *srisc_addr == 0 ? risc_addr : *srisc_addr;
>risc_size = be32_to_cpu(dcode[3]);
>
>then happily assume it's BE.
>
>And again, coming from flash, it's unlikely to actually be in the native
>endian-ness as callers seem to assume. I'm guessing it's all BE.
>
>I poked at it a bit and was able to cut down # of warnings
>from 1700 to 1400 in an hour. Someone familiar with the code
>should look at it.

We’ll take a look and send patches to resolve these warnings. 

>
>-- 
>MST
>___
>Virtualization mailing list
>virtualizat...@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [patch 5/6] [RFD] timekeeping: Provide optional 128bit math

[Peter Zijlstra]

On Fri, Dec 09, 2016 at 06:26:38AM +0100, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 08:49:39PM -, Thomas Gleixner wrote:
> 
> > +static inline u64 timekeeping_delta_to_ns(struct tk_read_base *tkr, u64 
> > delta)
> > +{
> > +   u32 dh, dl;
> > +   u64 nsec;
> > +
> > +   dl = delta;
> > +   dh = delta >> 32;
> > +
> > +   nsec = ((u64)dl * tkr->mult) + tkr->xtime_nsec;
> > +   nsec >>= tkr->shift;
> > +   if (unlikely(dh))
> > +   nsec += ((u64)dh * tkr->mult) << (32 - tkr->shift);
> > +   return nsec;
> > +}
> 
> Just for giggles, on tilegx the branch is actually slower than doing the
> mult unconditionally.
> 
> The problem is that the two multiplies would otherwise completely
> pipeline, whereas with the conditional you serialize them.

On my Haswell laptop the unconditional version is faster too.

> (came to light while talking about why the mul_u64_u32_shr() fallback
> didn't work right for them, which was a combination of the above issue
> and the fact that their compiler 'lost' the fact that these are
> 32x32->64 mults and did 64x64 ones instead).

Turns out using GCC-6.2.1 we have the same problem on i386, GCC doesn't
recognise the 32x32 mults and generates crap.

This used to work :/

Re: [patch 5/6] [RFD] timekeeping: Provide optional 128bit math

[Peter Zijlstra]

On Fri, Dec 09, 2016 at 06:26:38AM +0100, Peter Zijlstra wrote:
> On Thu, Dec 08, 2016 at 08:49:39PM -, Thomas Gleixner wrote:
> 
> > +static inline u64 timekeeping_delta_to_ns(struct tk_read_base *tkr, u64 
> > delta)
> > +{
> > +   u32 dh, dl;
> > +   u64 nsec;
> > +
> > +   dl = delta;
> > +   dh = delta >> 32;
> > +
> > +   nsec = ((u64)dl * tkr->mult) + tkr->xtime_nsec;
> > +   nsec >>= tkr->shift;
> > +   if (unlikely(dh))
> > +   nsec += ((u64)dh * tkr->mult) << (32 - tkr->shift);
> > +   return nsec;
> > +}
> 
> Just for giggles, on tilegx the branch is actually slower than doing the
> mult unconditionally.
> 
> The problem is that the two multiplies would otherwise completely
> pipeline, whereas with the conditional you serialize them.

On my Haswell laptop the unconditional version is faster too.

> (came to light while talking about why the mul_u64_u32_shr() fallback
> didn't work right for them, which was a combination of the above issue
> and the fact that their compiler 'lost' the fact that these are
> 32x32->64 mults and did 64x64 ones instead).

Turns out using GCC-6.2.1 we have the same problem on i386, GCC doesn't
recognise the 32x32 mults and generates crap.

This used to work :/

Re: [RFC PATCH] mm: introduce kv[mz]alloc helpers

[Al Viro]

On Fri, Dec 09, 2016 at 07:22:25AM +0100, Michal Hocko wrote:

> > Easier to handle those in vmalloc() itself.
> 
> I think there were some attempts in the past but some of the code paths
> are burried too deep and adding gfp_mask all the way down there seemed
> like a major surgery.

No need to propagate gfp_mask - the same trick XFS is doing right now can
be done in vmalloc.c in a couple of places and that's it; I'll resurrect the
patches and post them tomorrow after I get some sleep.

Re: [RFC PATCH] mm: introduce kv[mz]alloc helpers

[Al Viro]

On Fri, Dec 09, 2016 at 07:22:25AM +0100, Michal Hocko wrote:

> > Easier to handle those in vmalloc() itself.
> 
> I think there were some attempts in the past but some of the code paths
> are burried too deep and adding gfp_mask all the way down there seemed
> like a major surgery.

No need to propagate gfp_mask - the same trick XFS is doing right now can
be done in vmalloc.c in a couple of places and that's it; I'll resurrect the
patches and post them tomorrow after I get some sleep.

[PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace_marker

[Henrik Austad]

Instead of using get_user_pages_fast() and kmap_atomic() when writing
to the trace_marker file, just allocate enough space on the ring buffer
directly, and write into it via copy_from_user().

Writing into the trace_marker file use to allocate a temporary buffer
to perform the copy_from_user(), as we didn't want to write into the
ring buffer if the copy failed. But as a trace_marker write is suppose
to be extremely fast, and allocating memory causes other tracepoints to
trigger, Peter Zijlstra suggested using get_user_pages_fast() and
kmap_atomic() to keep the user space pages in memory and reading it
directly.

Instead, just allocate the space in the ring buffer and use
copy_from_user() directly. If it faults, return -EFAULT and write
"" into the ring buffer.

On architectures without a arch-specific get_user_pages_fast(), this
will end up in the generic get_user_pages_fast() and this grabs
mm->mmap_sem. Once you do this, then suddenly writing to the
trace_marker can cause priority-inversions.

This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
signed-off-chain by is somewhat uncertain at this stage.

The patch compiles, boots and does not immediately explode on impact. By
definition [2] it must therefore be perfect

2) https://www.spinics.net/lists/kernel/msg2400769.html
2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html

Cc: Ingo Molnar 
Cc: Henrik Austad 
Cc: Peter Zijlstra 
Cc: Steven Rostedt 
Cc: sta...@vger.kernel.org

Suggested-by: Thomas Gleixner 
Used-to-be-signed-off-by: Steven Rostedt 
Backported-by: Henrik Austad 
Tested-by: Henrik Austad 
Signed-off-by: Henrik Austad 
---
 kernel/trace/trace.c | 78 +++-
 1 file changed, 22 insertions(+), 56 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 18cdf91..94eb1ee 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4501,15 +4501,13 @@ tracing_mark_write(struct file *filp, const char __user 
*ubuf,
struct ring_buffer *buffer;
struct print_entry *entry;
unsigned long irq_flags;
-   struct page *pages[2];
-   void *map_page[2];
-   int nr_pages = 1;
+   const char faulted[] = "";
ssize_t written;
-   int offset;
int size;
int len;
-   int ret;
-   int i;
+
+/* Used in tracing_mark_raw_write() as well */
+#define FAULTED_SIZE (sizeof(faulted) - 1) /* '\0' is already accounted for */
 
if (tracing_disabled)
return -EINVAL;
@@ -4520,60 +4518,34 @@ tracing_mark_write(struct file *filp, const char __user 
*ubuf,
if (cnt > TRACE_BUF_SIZE)
cnt = TRACE_BUF_SIZE;
 
-   /*
-* Userspace is injecting traces into the kernel trace buffer.
-* We want to be as non intrusive as possible.
-* To do so, we do not want to allocate any special buffers
-* or take any locks, but instead write the userspace data
-* straight into the ring buffer.
-*
-* First we need to pin the userspace buffer into memory,
-* which, most likely it is, because it just referenced it.
-* But there's no guarantee that it is. By using get_user_pages_fast()
-* and kmap_atomic/kunmap_atomic() we can get access to the
-* pages directly. We then write the data directly into the
-* ring buffer.
-*/
BUILD_BUG_ON(TRACE_BUF_SIZE >= PAGE_SIZE);
 
-   /* check if we cross pages */
-   if ((addr & PAGE_MASK) != ((addr + cnt) & PAGE_MASK))
-   nr_pages = 2;
-
-   offset = addr & (PAGE_SIZE - 1);
-   addr &= PAGE_MASK;
-
-   ret = get_user_pages_fast(addr, nr_pages, 0, pages);
-   if (ret < nr_pages) {
-   while (--ret >= 0)
-   put_page(pages[ret]);
-   written = -EFAULT;
-   goto out;
-   }
+   local_save_flags(irq_flags);
+   size = sizeof(*entry) + cnt + 2; /* add '\0' and possible '\n' */
 
-   for (i = 0; i < nr_pages; i++)
-   map_page[i] = kmap_atomic(pages[i]);
+   /* If less than "", then make sure we can still add that */
+   if (cnt < FAULTED_SIZE)
+   size += FAULTED_SIZE - cnt;
 
-   local_save_flags(irq_flags);
-   size = sizeof(*entry) + cnt + 2; /* possible \n added */
buffer = tr->trace_buffer.buffer;
event = trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
  irq_flags, preempt_count());
-   if (!event) {
-   /* Ring buffer disabled, return as if not open for write */
-   written = -EBADF;
-   goto out_unlock;
-   }
+
+   if (unlikely(!event))
+   /* Ring buffer disabled, return as if not open for 

[PATCH] tracing: (backport) Replace kmap with copy_from_user() in trace_marker

[Henrik Austad]

Instead of using get_user_pages_fast() and kmap_atomic() when writing
to the trace_marker file, just allocate enough space on the ring buffer
directly, and write into it via copy_from_user().

Writing into the trace_marker file use to allocate a temporary buffer
to perform the copy_from_user(), as we didn't want to write into the
ring buffer if the copy failed. But as a trace_marker write is suppose
to be extremely fast, and allocating memory causes other tracepoints to
trigger, Peter Zijlstra suggested using get_user_pages_fast() and
kmap_atomic() to keep the user space pages in memory and reading it
directly.

Instead, just allocate the space in the ring buffer and use
copy_from_user() directly. If it faults, return -EFAULT and write
"" into the ring buffer.

On architectures without a arch-specific get_user_pages_fast(), this
will end up in the generic get_user_pages_fast() and this grabs
mm->mmap_sem. Once you do this, then suddenly writing to the
trace_marker can cause priority-inversions.

This is a backport of Steven Rostedts patch [1] and applied to 3.10.x so the
signed-off-chain by is somewhat uncertain at this stage.

The patch compiles, boots and does not immediately explode on impact. By
definition [2] it must therefore be perfect

2) https://www.spinics.net/lists/kernel/msg2400769.html
2) http://lkml.iu.edu/hypermail/linux/kernel/9804.1/0149.html

Cc: Ingo Molnar 
Cc: Henrik Austad 
Cc: Peter Zijlstra 
Cc: Steven Rostedt 
Cc: sta...@vger.kernel.org

Suggested-by: Thomas Gleixner 
Used-to-be-signed-off-by: Steven Rostedt 
Backported-by: Henrik Austad 
Tested-by: Henrik Austad 
Signed-off-by: Henrik Austad 
---
 kernel/trace/trace.c | 78 +++-
 1 file changed, 22 insertions(+), 56 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 18cdf91..94eb1ee 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4501,15 +4501,13 @@ tracing_mark_write(struct file *filp, const char __user 
*ubuf,
struct ring_buffer *buffer;
struct print_entry *entry;
unsigned long irq_flags;
-   struct page *pages[2];
-   void *map_page[2];
-   int nr_pages = 1;
+   const char faulted[] = "";
ssize_t written;
-   int offset;
int size;
int len;
-   int ret;
-   int i;
+
+/* Used in tracing_mark_raw_write() as well */
+#define FAULTED_SIZE (sizeof(faulted) - 1) /* '\0' is already accounted for */
 
if (tracing_disabled)
return -EINVAL;
@@ -4520,60 +4518,34 @@ tracing_mark_write(struct file *filp, const char __user 
*ubuf,
if (cnt > TRACE_BUF_SIZE)
cnt = TRACE_BUF_SIZE;
 
-   /*
-* Userspace is injecting traces into the kernel trace buffer.
-* We want to be as non intrusive as possible.
-* To do so, we do not want to allocate any special buffers
-* or take any locks, but instead write the userspace data
-* straight into the ring buffer.
-*
-* First we need to pin the userspace buffer into memory,
-* which, most likely it is, because it just referenced it.
-* But there's no guarantee that it is. By using get_user_pages_fast()
-* and kmap_atomic/kunmap_atomic() we can get access to the
-* pages directly. We then write the data directly into the
-* ring buffer.
-*/
BUILD_BUG_ON(TRACE_BUF_SIZE >= PAGE_SIZE);
 
-   /* check if we cross pages */
-   if ((addr & PAGE_MASK) != ((addr + cnt) & PAGE_MASK))
-   nr_pages = 2;
-
-   offset = addr & (PAGE_SIZE - 1);
-   addr &= PAGE_MASK;
-
-   ret = get_user_pages_fast(addr, nr_pages, 0, pages);
-   if (ret < nr_pages) {
-   while (--ret >= 0)
-   put_page(pages[ret]);
-   written = -EFAULT;
-   goto out;
-   }
+   local_save_flags(irq_flags);
+   size = sizeof(*entry) + cnt + 2; /* add '\0' and possible '\n' */
 
-   for (i = 0; i < nr_pages; i++)
-   map_page[i] = kmap_atomic(pages[i]);
+   /* If less than "", then make sure we can still add that */
+   if (cnt < FAULTED_SIZE)
+   size += FAULTED_SIZE - cnt;
 
-   local_save_flags(irq_flags);
-   size = sizeof(*entry) + cnt + 2; /* possible \n added */
buffer = tr->trace_buffer.buffer;
event = trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
  irq_flags, preempt_count());
-   if (!event) {
-   /* Ring buffer disabled, return as if not open for write */
-   written = -EBADF;
-   goto out_unlock;
-   }
+
+   if (unlikely(!event))
+   /* Ring buffer disabled, return as if not open for write */
+   return -EBADF;
 
entry = ring_buffer_event_data(event);
entry->ip = _THIS_IP_;
 
-   if (nr_pages == 2) {
-   len = 

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

[Cong Wang]

On Thu, Dec 8, 2016 at 5:32 PM, Al Viro  wrote:
> On Thu, Dec 08, 2016 at 04:08:27PM -0800, Cong Wang wrote:
>> On Thu, Dec 8, 2016 at 8:30 AM, Dmitry Vyukov  wrote:
>> > Chain exists of:
>> >  Possible unsafe locking scenario:
>> >
>> >CPU0CPU1
>> >
>> >   lock(sb_writers#5);
>> >lock(>bindlock);
>> >lock(sb_writers#5);
>> >   lock(>mutex/1);
>>
>> This looks false positive, probably just needs lockdep_set_class()
>> to set keys for pipe->mutex and unix->bindlock.
>
> I'm afraid that it's not a false positive at all.

Right, I was totally misled by the scenario output of lockdep, the stack
traces actually are much more reasonable.

The deadlock scenario is easy actually, comparing with the netlink one
which has 4 locks involved, it is:

unix_bind() path:
u->bindlock ==> sb_writer

do_splice() path:
sb_writer ==> pipe->mutex ==> u->bindlock

 *** DEADLOCK ***

>
> 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.

I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.

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

[Cong Wang]

On Thu, Dec 8, 2016 at 5:32 PM, Al Viro  wrote:
> On Thu, Dec 08, 2016 at 04:08:27PM -0800, Cong Wang wrote:
>> On Thu, Dec 8, 2016 at 8:30 AM, Dmitry Vyukov  wrote:
>> > Chain exists of:
>> >  Possible unsafe locking scenario:
>> >
>> >CPU0CPU1
>> >
>> >   lock(sb_writers#5);
>> >lock(>bindlock);
>> >lock(sb_writers#5);
>> >   lock(>mutex/1);
>>
>> This looks false positive, probably just needs lockdep_set_class()
>> to set keys for pipe->mutex and unix->bindlock.
>
> I'm afraid that it's not a false positive at all.

Right, I was totally misled by the scenario output of lockdep, the stack
traces actually are much more reasonable.

The deadlock scenario is easy actually, comparing with the netlink one
which has 4 locks involved, it is:

unix_bind() path:
u->bindlock ==> sb_writer

do_splice() path:
sb_writer ==> pipe->mutex ==> u->bindlock

 *** DEADLOCK ***

>
> 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.

I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.

[PATCH 4/4] dt-bindings: input: Specify the interrupt number of TPS65217 power button

[Milo Kim]

Specify the power button interrupt number which is from the datasheet.

Signed-off-by: Milo Kim 
---
 Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt 
b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
index 3e5b979..8682ab6 100644
--- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
+++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
@@ -8,8 +8,9 @@ This driver provides a simple power button event via an 
Interrupt.
 Required properties:
 - compatible: should be "ti,tps65217-pwrbutton" or "ti,tps65218-pwrbutton"
 
-Required properties for TPS65218:
+Required properties:
 - interrupts: should be one of the following
+   - <2>: For controllers compatible with tps65217
- <3 IRQ_TYPE_EDGE_BOTH>: For controllers compatible with tps65218
 
 Examples:
@@ -17,6 +18,7 @@ Examples:
  {
tps65217-pwrbutton {
compatible = "ti,tps65217-pwrbutton";
+   interrupts = <2>;
};
 };
 
-- 
2.9.3

[PATCH 3/4] dt-bindings: power/supply: Update TPS65217 properties

[Milo Kim]

Add interrupt specifiers for USB and AC charger input. Interrupt numbers
are from the datasheet.
Fix wrong property for compatible string.

Signed-off-by: Milo Kim 
---
 .../devicetree/bindings/power/supply/tps65217_charger.txt  | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt 
b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
index 98d131a..a11072c 100644
--- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
+++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
@@ -2,11 +2,16 @@ TPS65217 Charger
 
 Required Properties:
 -compatible: "ti,tps65217-charger"
+-interrupts: TPS65217 interrupt numbers for the AC and USB charger input 
change.
+ Should be <0> for the USB charger and <1> for the AC adapter.
+-interrupt-names: Should be "USB" and "AC"
 
 This node is a subnode of the tps65217 PMIC.
 
 Example:
 
tps65217-charger {
-   compatible = "ti,tps65090-charger";
+   compatible = "ti,tps65217-charger";
+   interrupts = <0>, <1>;
+   interrupt-names = "USB", "AC";
};
-- 
2.9.3

[PATCH 4/4] dt-bindings: input: Specify the interrupt number of TPS65217 power button

[Milo Kim]

Specify the power button interrupt number which is from the datasheet.

Signed-off-by: Milo Kim 
---
 Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt 
b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
index 3e5b979..8682ab6 100644
--- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
+++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
@@ -8,8 +8,9 @@ This driver provides a simple power button event via an 
Interrupt.
 Required properties:
 - compatible: should be "ti,tps65217-pwrbutton" or "ti,tps65218-pwrbutton"
 
-Required properties for TPS65218:
+Required properties:
 - interrupts: should be one of the following
+   - <2>: For controllers compatible with tps65217
- <3 IRQ_TYPE_EDGE_BOTH>: For controllers compatible with tps65218
 
 Examples:
@@ -17,6 +18,7 @@ Examples:
  {
tps65217-pwrbutton {
compatible = "ti,tps65217-pwrbutton";
+   interrupts = <2>;
};
 };
 
-- 
2.9.3

[PATCH 3/4] dt-bindings: power/supply: Update TPS65217 properties

[Milo Kim]

Add interrupt specifiers for USB and AC charger input. Interrupt numbers
are from the datasheet.
Fix wrong property for compatible string.

Signed-off-by: Milo Kim 
---
 .../devicetree/bindings/power/supply/tps65217_charger.txt  | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt 
b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
index 98d131a..a11072c 100644
--- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
+++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
@@ -2,11 +2,16 @@ TPS65217 Charger
 
 Required Properties:
 -compatible: "ti,tps65217-charger"
+-interrupts: TPS65217 interrupt numbers for the AC and USB charger input 
change.
+ Should be <0> for the USB charger and <1> for the AC adapter.
+-interrupt-names: Should be "USB" and "AC"
 
 This node is a subnode of the tps65217 PMIC.
 
 Example:
 
tps65217-charger {
-   compatible = "ti,tps65090-charger";
+   compatible = "ti,tps65217-charger";
+   interrupts = <0>, <1>;
+   interrupt-names = "USB", "AC";
};
-- 
2.9.3

[PATCH 2/4] dt-bindings: mfd: Remove TPS65217 interrupts

[Milo Kim]

Interrupt numbers are from the datasheet, so no need to keep them in
the ABI. Use the number in the DT file.

Signed-off-by: Milo Kim 
---
 arch/arm/boot/dts/am335x-bone-common.dtsi |  8 +++-
 include/dt-bindings/mfd/tps65217.h| 26 --
 2 files changed, 3 insertions(+), 31 deletions(-)
 delete mode 100644 include/dt-bindings/mfd/tps65217.h

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi 
b/arch/arm/boot/dts/am335x-bone-common.dtsi
index 14b6269..3e32dd1 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -6,8 +6,6 @@
  * published by the Free Software Foundation.
  */
 
-#include 
-
 / {
cpus {
cpu@0 {
@@ -319,13 +317,13 @@
ti,pmic-shutdown-controller;
 
charger {
-   interrupts = , ;
-   interrupt-names = "AC", "USB";
+   interrupts = <0>, <1>;
+   interrupt-names = "USB", "AC";
status = "okay";
};
 
pwrbutton {
-   interrupts = ;
+   interrupts = <2>;
status = "okay";
};
 
diff --git a/include/dt-bindings/mfd/tps65217.h 
b/include/dt-bindings/mfd/tps65217.h
deleted file mode 100644
index cafb9e6..000
--- a/include/dt-bindings/mfd/tps65217.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * This header provides macros for TI TPS65217 DT bindings.
- *
- * Copyright (C) 2016 Texas Instruments
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see .
- */
-
-#ifndef __DT_BINDINGS_TPS65217_H__
-#define __DT_BINDINGS_TPS65217_H__
-
-#define TPS65217_IRQ_USB   0
-#define TPS65217_IRQ_AC1
-#define TPS65217_IRQ_PB2
-
-#endif
-- 
2.9.3

[PATCH 1/4] ARM: dts: am335x: Fix the interrupt name of TPS65217

[Milo Kim]

Use 'interrupt-names' for getting the charger interrupt number.

Fixes: 1934e89a769b ("ARM: dts: am335x: Add the charger interrupt")
Signed-off-by: Milo Kim 
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi 
b/arch/arm/boot/dts/am335x-bone-common.dtsi
index dc561d5..14b6269 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -320,7 +320,7 @@
 
charger {
interrupts = , ;
-   interrupts-names = "AC", "USB";
+   interrupt-names = "AC", "USB";
status = "okay";
};
 
-- 
2.9.3

[PATCH 0/4] dt-bindings: mfd: Update TPS65217 interrupts

[Milo Kim]

This patch-set fixes wrong property name and uses TPS65217 HW interrupt 
number from the datasheet instead of the DT ABI. DT bindings are also 
updated.

Milo Kim (4):
  ARM: dts: am335x: Fix the interrupt name of TPS65217
  dt-bindings: mfd: Remove TPS65217 interrupts
  dt-bindings: power/supply: Update TPS65217 properties
  dt-bindings: input: Add interrupt number for TPS65217

 .../bindings/input/tps65218-pwrbutton.txt  |  4 +++-
 .../bindings/power/supply/tps65217_charger.txt |  7 +-
 arch/arm/boot/dts/am335x-bone-common.dtsi  |  8 +++
 include/dt-bindings/mfd/tps65217.h | 26 --
 4 files changed, 12 insertions(+), 33 deletions(-)
 delete mode 100644 include/dt-bindings/mfd/tps65217.h

-- 
2.9.3

[PATCH 2/4] dt-bindings: mfd: Remove TPS65217 interrupts

[Milo Kim]

Interrupt numbers are from the datasheet, so no need to keep them in
the ABI. Use the number in the DT file.

Signed-off-by: Milo Kim 
---
 arch/arm/boot/dts/am335x-bone-common.dtsi |  8 +++-
 include/dt-bindings/mfd/tps65217.h| 26 --
 2 files changed, 3 insertions(+), 31 deletions(-)
 delete mode 100644 include/dt-bindings/mfd/tps65217.h

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi 
b/arch/arm/boot/dts/am335x-bone-common.dtsi
index 14b6269..3e32dd1 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -6,8 +6,6 @@
  * published by the Free Software Foundation.
  */
 
-#include 
-
 / {
cpus {
cpu@0 {
@@ -319,13 +317,13 @@
ti,pmic-shutdown-controller;
 
charger {
-   interrupts = , ;
-   interrupt-names = "AC", "USB";
+   interrupts = <0>, <1>;
+   interrupt-names = "USB", "AC";
status = "okay";
};
 
pwrbutton {
-   interrupts = ;
+   interrupts = <2>;
status = "okay";
};
 
diff --git a/include/dt-bindings/mfd/tps65217.h 
b/include/dt-bindings/mfd/tps65217.h
deleted file mode 100644
index cafb9e6..000
--- a/include/dt-bindings/mfd/tps65217.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * This header provides macros for TI TPS65217 DT bindings.
- *
- * Copyright (C) 2016 Texas Instruments
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see .
- */
-
-#ifndef __DT_BINDINGS_TPS65217_H__
-#define __DT_BINDINGS_TPS65217_H__
-
-#define TPS65217_IRQ_USB   0
-#define TPS65217_IRQ_AC1
-#define TPS65217_IRQ_PB2
-
-#endif
-- 
2.9.3

[PATCH 1/4] ARM: dts: am335x: Fix the interrupt name of TPS65217

[Milo Kim]

Use 'interrupt-names' for getting the charger interrupt number.

Fixes: 1934e89a769b ("ARM: dts: am335x: Add the charger interrupt")
Signed-off-by: Milo Kim 
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi 
b/arch/arm/boot/dts/am335x-bone-common.dtsi
index dc561d5..14b6269 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -320,7 +320,7 @@
 
charger {
interrupts = , ;
-   interrupts-names = "AC", "USB";
+   interrupt-names = "AC", "USB";
status = "okay";
};
 
-- 
2.9.3

[PATCH 0/4] dt-bindings: mfd: Update TPS65217 interrupts

[Milo Kim]

This patch-set fixes wrong property name and uses TPS65217 HW interrupt 
number from the datasheet instead of the DT ABI. DT bindings are also 
updated.

Milo Kim (4):
  ARM: dts: am335x: Fix the interrupt name of TPS65217
  dt-bindings: mfd: Remove TPS65217 interrupts
  dt-bindings: power/supply: Update TPS65217 properties
  dt-bindings: input: Add interrupt number for TPS65217

 .../bindings/input/tps65218-pwrbutton.txt  |  4 +++-
 .../bindings/power/supply/tps65217_charger.txt |  7 +-
 arch/arm/boot/dts/am335x-bone-common.dtsi  |  8 +++
 include/dt-bindings/mfd/tps65217.h | 26 --
 4 files changed, 12 insertions(+), 33 deletions(-)
 delete mode 100644 include/dt-bindings/mfd/tps65217.h

-- 
2.9.3