* [PATCH v2] Fix some potential warnings
From: Alexandre Bailon @ 2016-10-25 12:11 UTC (permalink / raw)
To: linux-arm-kernel
Some changes I'm working on causes some warning because two included
headers defines the same macros.
Change in V2:
Update the d830 evm board file to use the da8xx-cfgchip.h
These changes are required as I'm sending this patch apart from
the series "[PATCH/RFT v2 00/17] Add DT support for ohci-da8xx"
Alexandre Bailon (1):
ARM: davinci: da8xx: Fix some redefined symbol warnings
arch/arm/mach-davinci/board-da830-evm.c | 3 ++-
include/linux/platform_data/usb-davinci.h | 23 -----------------------
2 files changed, 2 insertions(+), 24 deletions(-)
--
2.7.3
^ permalink raw reply
* [PATCH v3 3/6] dt-bindings: pinctrl: Deprecate sunxi pinctrl bindings
From: Linus Walleij @ 2016-10-25 12:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024194917.g5oezqc4uacsyt24@lukather>
On Mon, Oct 24, 2016 at 9:49 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> However, it looks like the first patch from this serie is missing from
> your tree, is there a reason for that?
No can you point it out?
> Also, in order to preserve bisectability, could you create an
> immutable branch for those sunxi patches so that I can merge the DT
> bits?
It's too late because they are already in the devel branch
and mixed up with merged of *other* immutable stuff.
However I think it is plain wrong to try to keep any bisectability
between the kernel at large and arch/*/boot/dts/*, because
the DTS stuff is supposed to at some point be maintained outside
of the kernel and for all OSes, they simply shouldn't be sync:ed.
Yours,
Linus Walleij
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pali Rohár @ 2016-10-25 11:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025112757.GC25855@amd>
On Tuesday 25 October 2016 13:27:57 Pavel Machek wrote:
> On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > * Pavel Machek <pavel@ucw.cz> [161024 14:24]:
> > > Hi!
> > >
> > > What about something like this? N900 will drain the battery down to
> > > system crash, which is quite uncool.
> >
> > Can't we make that generic and configurable for the voltage somehow?
> >
> > Also, the shutdown voltage can depend on external devices connected.
> > It could be for example 3.3V depending on eMMC on some devices while
> > devices with no eMMC could have it at 3.0V.
>
> Actually, do we need to make it configurable? It looks like we should
> respect hardware telling us battery is dead, and only use (low)
> hardcoded voltages as a fallback.
>
> Currently patch looks like this. generic_protect() should work for
> other batteries drivers, too.
Now I checked Maemo's BME replacement code and it shutdown device 10
seconds after it read that capacity level is LOW or CRITICAL. bq27xxx
driver reports LOW when EDV1 is set and CRITICAL when EDVF is set. And
bq27xxx reports those LOW/CRITICAL based on EDV1/EDVF flags even if
battery is not calibrated.
So I would be happy if kernel does not issue emergency shutdown before
Maemo's BME replacement issue own "correct" system shutdown. As Maemo is
doing it on EDV1 flag (not EDVF as I thought!) with 10 seconds delay and
check is done every 30 seconds it means that Maemo shutdown process in
worst case is started 40 seconds after EDV1 is set. Shutdown process is
about 60 seconds (probably max.), can we ensure that kernel does not do
its own emergency shutdown earlier then 2 minutes before first see EDV1
flag? Or can test that EDVF flag is set in most cases 2 minutes after
EDV1?
It is really bad idea to start emergency kernel shutdown before even
userspace do it!
> Pavel
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 0fe278b..04094ad 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -46,6 +46,7 @@
> #include <linux/delay.h>
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> +#include <linux/reboot.h>
> #include <linux/slab.h>
> #include <linux/of.h>
>
> @@ -679,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> /* Unlikely but important to return first */
> if (unlikely(bq27xxx_battery_overtemp(di, flags)))
> return POWER_SUPPLY_HEALTH_OVERHEAT;
> - if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> - return POWER_SUPPLY_HEALTH_COLD;
> if (unlikely(bq27xxx_battery_dead(di, flags)))
> return POWER_SUPPLY_HEALTH_DEAD;
> + if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> + return POWER_SUPPLY_HEALTH_COLD;
>
> return POWER_SUPPLY_HEALTH_GOOD;
> }
> @@ -740,6 +741,49 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
> }
> EXPORT_SYMBOL_GPL(bq27xxx_battery_update);
>
> +static void shutdown(char *reason)
> +{
> + pr_alert("%s Forcing shutdown\n", reason);
> + orderly_poweroff(true);
> +}
> +
> +static int generic_protect(struct power_supply *psy)
> +{
> + union power_supply_propval val;
> + int res;
> + int mV, mA, mOhm = 430, mVadj = 0;
> +
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_HEALTH, &val);
> + if (res)
> + return res;
> +
> + if (val.intval == POWER_SUPPLY_HEALTH_OVERHEAT)
> + shutdown("Battery overheat.");
> + if (val.intval == POWER_SUPPLY_HEALTH_DEAD)
> + shutdown("Battery dead.");
Generally this is not a good idea. On some boards with bq27xxx devices
you can have another battery device or connected power device. You could
have "dead" battery but device supplied e.g. from wallcharger.
N900 cannot be powered from wallcharger by default, but in specific
conditions (turned everything except display) you can do battery
hotswap (when wallcharger is connected; it can power system).
So this patch basically break lot of other self powered devices.
I would propose check for am_i_power_supplied() (function with such name
in power_supply interface exist) and do that only for negative response.
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &val);
> + if (res)
> + return res;
> + mV = val.intval / 1000;
> +
> + if (mV < 2950)
> + shutdown("Battery below 2.95V.");
> +
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW, &val);
> + if (res)
> + return res;
> + mA = val.intval / 1000;
> + mVadj = mV + (mA * mOhm) / 1000;
> +
> + if (mVadj < 3150)
> + shutdown("Battery internal voltage below 3.15.");
> +
> + printk(KERN_INFO "Main battery %d mV, internal voltage %d mV\n",
> + mV, mVadj);
Please no printk. There is dev_info or how is that function called. And
spamming dmesg for every poll is not good idea. It should be probably
DEBUG not INFO.
> + return 0;
> +}
> +
> static void bq27xxx_battery_poll(struct work_struct *work)
> {
> struct bq27xxx_device_info *di =
> @@ -747,6 +791,7 @@ static void bq27xxx_battery_poll(struct work_struct *work)
> work.work);
>
> bq27xxx_battery_update(di);
> + generic_protect(di->bat);
>
> if (poll_interval > 0)
> schedule_delayed_work(&di->work, poll_interval * HZ);
>
--
Pali Roh?r
pali.rohar at gmail.com
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pavel Machek @ 2016-10-25 11:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024212932.uhjz752z2cy5hohl@atomide.com>
On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [161024 14:24]:
> > Hi!
> >
> > What about something like this? N900 will drain the battery down to
> > system crash, which is quite uncool.
>
> Can't we make that generic and configurable for the voltage somehow?
>
> Also, the shutdown voltage can depend on external devices connected.
> It could be for example 3.3V depending on eMMC on some devices while
> devices with no eMMC could have it at 3.0V.
Actually, do we need to make it configurable? It looks like we should
respect hardware telling us battery is dead, and only use (low)
hardcoded voltages as a fallback.
Currently patch looks like this. generic_protect() should work for
other batteries drivers, too.
Pavel
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 0fe278b..04094ad 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -46,6 +46,7 @@
#include <linux/delay.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
+#include <linux/reboot.h>
#include <linux/slab.h>
#include <linux/of.h>
@@ -679,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
/* Unlikely but important to return first */
if (unlikely(bq27xxx_battery_overtemp(di, flags)))
return POWER_SUPPLY_HEALTH_OVERHEAT;
- if (unlikely(bq27xxx_battery_undertemp(di, flags)))
- return POWER_SUPPLY_HEALTH_COLD;
if (unlikely(bq27xxx_battery_dead(di, flags)))
return POWER_SUPPLY_HEALTH_DEAD;
+ if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+ return POWER_SUPPLY_HEALTH_COLD;
return POWER_SUPPLY_HEALTH_GOOD;
}
@@ -740,6 +741,49 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
}
EXPORT_SYMBOL_GPL(bq27xxx_battery_update);
+static void shutdown(char *reason)
+{
+ pr_alert("%s Forcing shutdown\n", reason);
+ orderly_poweroff(true);
+}
+
+static int generic_protect(struct power_supply *psy)
+{
+ union power_supply_propval val;
+ int res;
+ int mV, mA, mOhm = 430, mVadj = 0;
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_HEALTH, &val);
+ if (res)
+ return res;
+
+ if (val.intval == POWER_SUPPLY_HEALTH_OVERHEAT)
+ shutdown("Battery overheat.");
+ if (val.intval == POWER_SUPPLY_HEALTH_DEAD)
+ shutdown("Battery dead.");
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &val);
+ if (res)
+ return res;
+ mV = val.intval / 1000;
+
+ if (mV < 2950)
+ shutdown("Battery below 2.95V.");
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW, &val);
+ if (res)
+ return res;
+ mA = val.intval / 1000;
+ mVadj = mV + (mA * mOhm) / 1000;
+
+ if (mVadj < 3150)
+ shutdown("Battery internal voltage below 3.15.");
+
+ printk(KERN_INFO "Main battery %d mV, internal voltage %d mV\n",
+ mV, mVadj);
+ return 0;
+}
+
static void bq27xxx_battery_poll(struct work_struct *work)
{
struct bq27xxx_device_info *di =
@@ -747,6 +791,7 @@ static void bq27xxx_battery_poll(struct work_struct *work)
work.work);
bq27xxx_battery_update(di);
+ generic_protect(di->bat);
if (poll_interval > 0)
schedule_delayed_work(&di->work, poll_interval * HZ);
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/db1c8616/attachment.sig>
^ permalink raw reply related
* [PATCH] convert orion5x ls-chl to device tree
From: Andrew Lunn @ 2016-10-25 11:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <14522899-679b-2f8b-73ca-493788299790@gmail.com>
On Tue, Oct 25, 2016 at 12:04:49AM +0100, Ash Hughes wrote:
> Hi all,
>
> This patch converts my orion5x ls-chl Linkstation device to device tree.
>
> Signed-off-by: Ashley Hughes <ashley.hughes@blueyonder.co.uk>
Hi Ash
Nicely done.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [PATCH 5/5] ARM: dts: Add LEGO MINDSTORTMS EV3 dts
From: Sekhar Nori @ 2016-10-25 10:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <84648cb0-f1fa-d88b-bac2-79208af01ca6@lechnology.com>
On Tuesday 25 October 2016 02:50 AM, David Lechner wrote:
> On 10/24/2016 02:50 PM, David Lechner wrote:
>> On 10/24/2016 10:50 AM, David Lechner wrote:
>>> On 10/24/2016 06:58 AM, Sekhar Nori wrote:
>>>> On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
>>>>
>>>>> +&ehrpwm1 {
>>>>> + status = "disabled";
>>>>
>>>> Hmm, disabled? Can you add this node when you actually use it?
>>>
>>> Not sure why I have this disabled. Like the gpios, the pwms can be used
>>> via sysfs, so I would like to leave them.
>>>
>>
>> Now I remember why these are disabled. The clock matching is broken.
>> Only the first ehrpwm and the first ecap get clocks. The others fail.
>>
>> I can change these to "okay". It will just result in a kernel error
>> message until the clocks are fixed.
>>
>
> correction: it is not the clocks that are broken. it is the device names.
>
> In arch/arm/mach-davinci/da8xx-dt.c, we have...
>
>
> OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm", NULL),
> OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm", NULL),
> OF_DEV_AUXDATA("ti,da850-ecap", 0x01f06000, "ecap", NULL),
> OF_DEV_AUXDATA("ti,da850-ecap", 0x01f07000, "ecap", NULL),
> OF_DEV_AUXDATA("ti,da850-ecap", 0x01f08000, "ecap", NULL),
>
>
> Which causes each device to have the same device node name. This causes
> sysfs errors because it is trying to register a second device at the
> same sysfs path.
>
> If you change the names here, then the device do not work because the
> clock lookup fails.
Yeah, this is incorrect (I should have caught it in review). The device
id should have been present in the lookup. Can you fix auxdata and clock
lookup too and send a separate patch? Its probably a v4.9-rc candidate.
Thanks,
Sekhar
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pali Rohár @ 2016-10-25 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025105604.GA25855@amd>
On Tuesday 25 October 2016 12:56:04 Pavel Machek wrote:
> Take a look at code. Health is not read from hardware unless battery
> is calibrated.
Then it is bug. EDVF should be accepted also when battery is not
calibrated.
--
Pali Roh?r
pali.rohar at gmail.com
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pavel Machek @ 2016-10-25 10:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025105320.GK12154@pali>
On Tue 2016-10-25 12:53:20, Pali Roh?r wrote:
> On Tuesday 25 October 2016 12:24:35 Pavel Machek wrote:
> > On Mon 2016-10-24 23:48:47, Pali Roh?r wrote:
> > > On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > > > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > > > Also, the shutdown voltage can depend on external devices
> > > > > connected. It could be for example 3.3V depending on eMMC on some
> > > > > devices while devices with no eMMC could have it at 3.0V.
> > > >
> > > > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > > > going below that is pretty mean to the battery. But if I set
> > > > threshold too high, GSM activity will push it below that for a very
> > > > short while, and I'll shutdown too soon.
> > > >
> > > > Ideas welcome...
> > >
> > > bq27x00 has EDVF flag which means that battery is empty. Maemo with
> > > bq27x00 driver is configured to issue system shutdown when EDVF is set.
> > >
> > > Maybe kernel should issue emergency shutdown e.g. after minute or two
> > > after EDVF flag is set?
> >
> > Thanks for pointer.
> >
> > EDVF seems to be exposed as health. ... but only if battery is
> > calibrated, AFAICT.
>
> No, EDVF is available also for uncalibrated battery. There are EDV1 and
> EDVF flags. Both are set based on battery voltage and some other
> parameters from bq EEPROM.
>
> > if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
> > dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
>
> Yes, it ignores only capacity values (which needs calibration), not
> those raw flags which works also without calibration.
>
> > ...
> > cache.health = -ENODATA;
Take a look at code. Health is not read from hardware unless battery
is calibrated.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/61eece66/attachment.sig>
^ permalink raw reply
* [PATCH/RFT v2 00/17] Add DT support for ohci-da8xx
From: Sekhar Nori @ 2016-10-25 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024164634.4330-1-ahaslam@baylibre.com>
Hi Axel,
On Monday 24 October 2016 10:16 PM, ahaslam at baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
>
> The purpose of this patch series is to add DT support and modernize
> the ohci-da8xx glue driver without breaking the non-DT boot,
> which is still used in unconverted davinci devices.
>From a mach-davinci perspective, there are some patches which seem to be
safe to apply and some which depend on corresponding driver changes to
get in.
In order to speed up the process of applying this series, can you split
the mach-davinci updates which are safe to apply into a separate series.
For DT patches, the bindings should be accepted. For other patches, they
should not be causing any regressions.
Thanks,
Sekhar
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pali Rohár @ 2016-10-25 10:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025102435.GA6916@amd>
On Tuesday 25 October 2016 12:24:35 Pavel Machek wrote:
> On Mon 2016-10-24 23:48:47, Pali Roh?r wrote:
> > On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > > Also, the shutdown voltage can depend on external devices
> > > > connected. It could be for example 3.3V depending on eMMC on some
> > > > devices while devices with no eMMC could have it at 3.0V.
> > >
> > > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > > going below that is pretty mean to the battery. But if I set
> > > threshold too high, GSM activity will push it below that for a very
> > > short while, and I'll shutdown too soon.
> > >
> > > Ideas welcome...
> >
> > bq27x00 has EDVF flag which means that battery is empty. Maemo with
> > bq27x00 driver is configured to issue system shutdown when EDVF is set.
> >
> > Maybe kernel should issue emergency shutdown e.g. after minute or two
> > after EDVF flag is set?
>
> Thanks for pointer.
>
> EDVF seems to be exposed as health. ... but only if battery is
> calibrated, AFAICT.
No, EDVF is available also for uncalibrated battery. There are EDV1 and
EDVF flags. Both are set based on battery voltage and some other
parameters from bq EEPROM.
> if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
> dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
Yes, it ignores only capacity values (which needs calibration), not
those raw flags which works also without calibration.
> ...
> cache.health = -ENODATA;
>
> Plus, it prioritizes battery cold over battery dead. IMO we don't need
> to shutdown on battery cold (we just may not charge the battery), but
> we need to shutdown on battery dead.
>
> So something like this?
>
> Pavel
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 8eb2f8f..5ddf6d7 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -680,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> /* Unlikely but important to return first */
> if (unlikely(bq27xxx_battery_overtemp(di, flags)))
> return POWER_SUPPLY_HEALTH_OVERHEAT;
> - if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> - return POWER_SUPPLY_HEALTH_COLD;
> if (unlikely(bq27xxx_battery_dead(di, flags)))
> return POWER_SUPPLY_HEALTH_DEAD;
> + if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> + return POWER_SUPPLY_HEALTH_COLD;
>
> return POWER_SUPPLY_HEALTH_GOOD;
> }
>
>
Looks like this is OK.
--
Pali Roh?r
pali.rohar at gmail.com
^ permalink raw reply
* [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent
From: Axel Haslam @ 2016-10-25 10:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d7e2b356-4874-fdc1-c505-70e57e3908de@ti.com>
On Tue, Oct 25, 2016 at 12:43 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam at baylibre.com wrote:
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
>> board files to handle vbus and overcurrent irqs form the power supply.
>> However, this does not play nice when moving to a DT based boot were
>> we wont have board files.
>>
>> Instead of requesting and handling the gpio, use the regulator framework
>> to take care of enabling and disabling vbus power. This has the benefit
>> that we dont need to pass any more platform data to the driver:
>>
>> These will be handled by the regulator framework:
>> set_power -> regulator_enable/regulator_disable
>> get_power -> regulator_is_enabled
>> get_oci -> regulator_get_mode
>> ocic_notify -> regulator notification
>>
>> We can keep the default potpgt and use the regulator start delay instead:
>> potpgt -> regulator startup delay time
>>
>> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
>> (they should not have been decleared/reserved) so, just remove those
>> definitions from the hwk board file.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>> arch/arm/mach-davinci/board-da830-evm.c | 97 ++++++++----------------
>> arch/arm/mach-davinci/board-omapl138-hawk.c | 96 +-----------------------
>> arch/arm/mach-davinci/include/mach/da8xx.h | 2 +-
>> arch/arm/mach-davinci/usb-da8xx.c | 3 +-
>> drivers/usb/host/ohci-da8xx.c | 111 ++++++++++++++++++----------
>> include/linux/platform_data/usb-davinci.h | 19 -----
>> 6 files changed, 105 insertions(+), 223 deletions(-)
>
> Can you separate out the driver enhancement from the platform
> (mach-davinci) changes? They need to go through different trees.
>
Ok, i will do that, (it might require intermediate code to have
the driver working on each patch)
> Thanks,
> Sekhar
>
>
^ permalink raw reply
* [PATCH] ARM: dts: sun8i: fix the pinmux for UART1
From: Maxime Ripard @ 2016-10-25 10:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024170831.38819-1-icenowy@aosc.xyz>
On Tue, Oct 25, 2016 at 01:08:31AM +0800, Icenowy Zheng wrote:
> When the patch is applied, the allwinner,driver and allwinner,pull
> properties are removed.
>
> Although they're described to be optional in the devicetree binding,
> without them, the pinmux cannot be initialized, and the uart cannot
> be used.
>
> Add them back to fix the problem, and makes the bluetooth on iNet D978
> Rev2 board work.
>
> Fixes: 82eec384249f (ARM: dts: sun8i: add pinmux for UART1 at PG)
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Applied, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/1babf742/attachment-0001.sig>
^ permalink raw reply
* [PATCH v7 11/11] arm64: dts: r8a7796: salvator-x: Enable UHS-I SDR-104
From: Geert Uytterhoeven @ 2016-10-25 10:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473764228-24768-12-git-send-email-horms+renesas@verge.net.au>
On Tue, Sep 13, 2016 at 12:57 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> Add the sd-uhs-sdr104 property to SDHI0 and SDHI1.
SDHI0 and SDHI3.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v7 03/11] arm64: dts: r8a7795: salvator-x: Enable UHS-I SDR-104
From: Geert Uytterhoeven @ 2016-10-25 10:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473764228-24768-4-git-send-email-horms+renesas@verge.net.au>
On Tue, Sep 13, 2016 at 12:57 PM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> Add the sd-uhs-sdr104 property to SDHI0 and SDHI1.
SDHI0 and SDHI3?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Disabling an interrupt in the handler locks the system up
From: Marc Zyngier @ 2016-10-25 10:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <580F1992.2070602@free.fr>
On 25/10/16 09:36, Mason wrote:
> On 25/10/2016 10:29, Sebastian Frias wrote:
>
>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>
>>> On Mon, 24 Oct 2016, Mason wrote:
>>>
>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>> makes the system lock-up disappear.
>>>
>>> The way how lazy irq disabling works is:
>>>
>>> 1) Interrupt is marked disabled in software, but the hardware is not masked
>>>
>>> 2) If the interrupt fires befor the interrupt is reenabled, then it's
>>> masked at the hardware level in the low level interrupt flow handler.
>>
>> Would you mind explaining what is the intention behind?
>> Because it does not seem obvious why there isn't a direct map between
>> "disable_irq*()" and "mask_irq()"
>
> I had a similar, but slightly different question:
>
> What is the difference between struct irq_chip's
>
> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL)
> * @irq_disable: disable the interrupt
> * @irq_mask: mask an interrupt source
One important difference between disable and mask is that disable is
perfectly allowed not to care about pending signals, whereas mask must
preserve an interrupt becoming pending whilst masked.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH/RFT v2 12/17] USB: ochi-da8xx: Use a regulator for vbus/overcurrent
From: Sekhar Nori @ 2016-10-25 10:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024164634.4330-13-ahaslam@baylibre.com>
On Monday 24 October 2016 10:16 PM, ahaslam at baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
>
> Currently, the da8xx ohci driver uses a set of gpios and callbacks in
> board files to handle vbus and overcurrent irqs form the power supply.
> However, this does not play nice when moving to a DT based boot were
> we wont have board files.
>
> Instead of requesting and handling the gpio, use the regulator framework
> to take care of enabling and disabling vbus power. This has the benefit
> that we dont need to pass any more platform data to the driver:
>
> These will be handled by the regulator framework:
> set_power -> regulator_enable/regulator_disable
> get_power -> regulator_is_enabled
> get_oci -> regulator_get_mode
> ocic_notify -> regulator notification
>
> We can keep the default potpgt and use the regulator start delay instead:
> potpgt -> regulator startup delay time
>
> The hawk board does not have a GPIO/OVERCURRENT gpio to control vbus,
> (they should not have been decleared/reserved) so, just remove those
> definitions from the hwk board file.
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
> arch/arm/mach-davinci/board-da830-evm.c | 97 ++++++++----------------
> arch/arm/mach-davinci/board-omapl138-hawk.c | 96 +-----------------------
> arch/arm/mach-davinci/include/mach/da8xx.h | 2 +-
> arch/arm/mach-davinci/usb-da8xx.c | 3 +-
> drivers/usb/host/ohci-da8xx.c | 111 ++++++++++++++++++----------
> include/linux/platform_data/usb-davinci.h | 19 -----
> 6 files changed, 105 insertions(+), 223 deletions(-)
Can you separate out the driver enhancement from the platform
(mach-davinci) changes? They need to go through different trees.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 1/3] phy: sun4i: check PHY id when poking unknown bit of pmu
From: Maxime Ripard @ 2016-10-25 10:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1773661477366364@web4j.yandex.ru>
Hi,
On Tue, Oct 25, 2016 at 11:32:44AM +0800, Icenowy Zheng wrote:
>
>
> > On Mon, Oct 24, 2016 at 11:59:30AM +0800, Icenowy Zheng wrote:
> >
> >> Allwinner SoC's PHY 0, when used as OTG controller, have no pmu part.
> >> The code that poke some unknown bit of PMU for H3/A64 didn't check
> >> the PHY, and will cause kernel oops when PHY 0 is used.
> >>
> >> Fixes: b3e0d141ca9f (phy: sun4i: add support for A64 usb phy)
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >
> > Cc'ing stable would be nice.
>
> Yes... As it's also used by H3...
>
> >
> > Apart from that, Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> If I change it to check whether phy->pmu is null, will you keep the ACK?
Yep, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/09742e34/attachment.sig>
^ permalink raw reply
* [PATCH 5/8] ARM: gr8: Add missing pwm channel 1 pin
From: Maxime Ripard @ 2016-10-25 10:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGb2v64Ai+8sUPyH9aTnKUw1ROmQNxzCEqUXrT4v8Q-nipqR3Q@mail.gmail.com>
On Tue, Oct 25, 2016 at 12:10:26PM +0800, Chen-Yu Tsai wrote:
> On Fri, Oct 21, 2016 at 1:07 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Thu, Oct 20, 2016 at 10:10:03PM +0800, Chen-Yu Tsai wrote:
> >> On Thu, Oct 20, 2016 at 4:12 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > The PWM controller has two different channels, but only the first pin was
> >> > exposed in the DTSI. Add the other one.
> >> >
> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>
> >> Acked-by: Chen-Yu Tsai <wens@csie.org>
> >>
> >> > ---
> >> > arch/arm/boot/dts/ntc-gr8.dtsi | 7 +++++++
> >> > 1 file changed, 7 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/arch/arm/boot/dts/ntc-gr8.dtsi b/arch/arm/boot/dts/ntc-gr8.dtsi
> >> > index 74aff795e723..fad7381630f3 100644
> >> > --- a/arch/arm/boot/dts/ntc-gr8.dtsi
> >> > +++ b/arch/arm/boot/dts/ntc-gr8.dtsi
> >> > @@ -854,6 +854,13 @@
> >> > allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> >> > };
> >> >
> >> > + pwm1_pins_a: pwm1 at 0 {
> >>
> >> Nit: really don't need "_a" and "@0" here.
> >
> > Fixed and applied.
>
> Oops, you forgot to fix the label in the chip-pro dts:
>
> DTC arch/arm/boot/dts/ntc-gr8-chip-pro.dtb
> ERROR (phandle_references): Reference to non-existent node or label
> "pwm1_pins_a"
>
> ERROR: Input tree has errors, aborting (use -f to force output)
> scripts/Makefile.lib:313: recipe for target
> 'arch/arm/boot/dts/ntc-gr8-chip-pro.dtb' failed
Yeah, it was noticed by linux-next too, and I fixed it...
Sorry for that.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/7c71abee/attachment.sig>
^ permalink raw reply
* [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Thomas Gleixner @ 2016-10-25 10:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdYUVXny57AC9rKX3VRAyooEk_2XLvqe9jOuT3rOaE75rg@mail.gmail.com>
On Tue, 25 Oct 2016, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> >> Isn't this usecase (also as described in the cover letter) a textbook
> >> example of when you should be using hierarchical irqdomain?
> >>
> >> Please check with Marc et al on hierarchical irqdomains.
> >
> > Linus,
> > Do you mean I should create a new hierarchical irqdomains in each of
> > the two pinctrl instances we have in these SoC, these domains being
> > stacked on the one I just added for controller in irqchip ?
> >
> > I did not understand this is what you meant when I asked you the
> > question at ELCE.
>
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.
Hierarchical irqdomains are used when you have several levels of interrupt
hardware to deliver an interrupt.
For example on x86 we have:
device --- [IOAPIC] -- [VECTOR]
and we can have this expanded to
device --- [IOAPIC] -- [IRQ Remapping] -- [VECTOR]
and we have more things hanging off the VECTOR domain
device --- [IOAPIC] ---
|--- [VECTOR]
device --- [PCIMSI] ---
So with irq remapping this might look like this:
device --- [IOAPIC] ---
|-----------------------
device --- [PCIMSI] --- |
|---[VECTOR]
device --- [IOAPIC] --- |
|--[IRQ Remapping]------
device --- [PCIMSI] ---
The important part is that this hierarchy deals with a single Linux virq
and all parts of the hierarchy are required for setup and possibly for
mask/ack/eoi.
This is different from a demultiplex interrupt
device --- [DEMUX] --- [GIC]
where the demultiplex interrupt is a different virq than the device
virq. The demux interrupt chip can have a parent relation ship, which can
be required to propagate information, e.g. wake on a device behind the
demux must keep the gic as a wake irq as well. But it's not hierarchical in
the sense of our hierarchical irq domains.
Hope that helps.
Thanks,
tglx
^ permalink raw reply
* [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Marc Zyngier @ 2016-10-25 10:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdYUVXny57AC9rKX3VRAyooEk_2XLvqe9jOuT3rOaE75rg@mail.gmail.com>
On 25/10/16 10:14, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>>> Isn't this usecase (also as described in the cover letter) a textbook
>>> example of when you should be using hierarchical irqdomain?
>>>
>>> Please check with Marc et al on hierarchical irqdomains.
>>
>> Linus,
>> Do you mean I should create a new hierarchical irqdomains in each of
>> the two pinctrl instances we have in these SoC, these domains being
>> stacked on the one I just added for controller in irqchip ?
>>
>> I did not understand this is what you meant when I asked you the
>> question at ELCE.
>
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.
I probably didn't do that good a job explaining it then. Let's try
again. You want to use hierarchical domains when you want to describe an
interrupt whose path traverses multiple controllers without ever being
multiplexed with other signals. As long as you have this 1:1
relationship between controllers, you can use them.
> Which is problematic since quite a few GPIO drivers now
> need to use them.
>
> I will review his slides, in the meantime I would say: whatever
> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> could ask that he ACK the entire chain of patches
> from GIC->specialchip->GPIO.
Man, I don't even trust myself... ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot
From: Axel Haslam @ 2016-10-25 10:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <522eeb9a-6fb1-88eb-5c58-7ee209a50fc3@ti.com>
On Tue, Oct 25, 2016 at 12:28 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Monday 24 October 2016 10:16 PM, ahaslam at baylibre.com wrote:
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> The phy framework requests an optional "phy" regulator. If it does
>> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
>> for the omap138-lcdk board, this would prevent the usb11 phy to probe
>> correctly and ohci would not enumerate.
>>
>> By calling "regulator_has_full_constraints", An error would be returned
>
> nit: prefer regulator_has_full_constraints()
>
>> instead of DEFER for the "optional" regulator, and the probe of
>> the phy driver can continue normally without a regulator.
>>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>
> Looks good to me. Just drop the "hawk: from subject line since you also
> touch da830 evm. I am not sure what "ohci plat boot" means. How about
> the following:
>
> "ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"
>
Will do.
Thanks
Axel.
> Thanks,
> Sekhar
^ permalink raw reply
* [PATCH/RFT v2 08/17] ARM: davinci: hawk: add full constraints for ohci plat boot
From: Sekhar Nori @ 2016-10-25 10:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024164634.4330-9-ahaslam@baylibre.com>
On Monday 24 October 2016 10:16 PM, ahaslam at baylibre.com wrote:
> From: Axel Haslam <ahaslam@baylibre.com>
>
> The phy framework requests an optional "phy" regulator. If it does
> not find one, it returns -EPROBE_DEFER. In the case of non-DT based boot
> for the omap138-lcdk board, this would prevent the usb11 phy to probe
> correctly and ohci would not enumerate.
>
> By calling "regulator_has_full_constraints", An error would be returned
nit: prefer regulator_has_full_constraints()
> instead of DEFER for the "optional" regulator, and the probe of
> the phy driver can continue normally without a regulator.
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Looks good to me. Just drop the "hawk: from subject line since you also
touch da830 evm. I am not sure what "ohci plat boot" means. How about
the following:
"ARM: davinci: da8xx: fix OHCI PHY probe for non-DT boot"
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 2/3] ARM: convert to generated system call tables
From: Arnd Bergmann @ 2016-10-25 10:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025091210.rjfhvq4mqtuquei5@tower>
On Tuesday, October 25, 2016 10:12:10 PM CEST Michael Cree wrote:
> On Fri, Oct 21, 2016 at 03:06:45PM +0200, Arnd Bergmann wrote:
> > I see your point, but I think there are serious issues with the current
> > approach as well:
> >
> > - a lot of the less common architectures just don't get updated
> > in time, out of 22 architectures that don't use asm-generic/unistd.h,
> > only 12 have pwritev2 in linux-next, and only three have pkey_mprotect
> >
> > - some architectures that add all syscalls sometimes make a mistake
> > and forget one, e.g. alpha apparently never added __NR_bpf, but it
> > did add the later __NR_execveat.
>
> __NR_bpf was not forgotten on Alpha. It was not wired up because
> extra architecture support is needed which has not been implemented.
>
> But maybe we should just wire it up to sys_ni_syscall in the meantime
> so a syscall number is reserved for it, and user space can call it to
> get -ENOSYS returned.
Ah, I must have misinterpreted the code then. I assumed that the
bpf syscall always works on all architectures, but that only the
jit compiler for it required architecture specific code to make it
more efficient.
The implementation of sys_bfp is compile-time selectable at the moment
and falls back to sys_no_syscall if CONFIG_BPF_SYSCALL is disabled.
If it doesn't work on Alpha, maybe that symbol could have a "depends
on !ALPHA" or "depends on BPF_SUPPORT"?
sys_seccomp is another one that falls into a similar category, but
it already depends on HAVE_ARCH_SECCOMP_FILTER, and most other
architectures have assigned a syscall number but not set this symbol.
This one will actually allow you to set strict seccomp mode even
without the Kconfig symbol, just not allow to set a filter.
Arnd
^ permalink raw reply
* [RFC] shutdown machine when li-ion battery goes below 3V
From: Pavel Machek @ 2016-10-25 10:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201610242348.47484@pali>
On Mon 2016-10-24 23:48:47, Pali Roh?r wrote:
> On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > Also, the shutdown voltage can depend on external devices
> > > connected. It could be for example 3.3V depending on eMMC on some
> > > devices while devices with no eMMC could have it at 3.0V.
> >
> > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > going below that is pretty mean to the battery. But if I set
> > threshold too high, GSM activity will push it below that for a very
> > short while, and I'll shutdown too soon.
> >
> > Ideas welcome...
>
> bq27x00 has EDVF flag which means that battery is empty. Maemo with
> bq27x00 driver is configured to issue system shutdown when EDVF is set.
>
> Maybe kernel should issue emergency shutdown e.g. after minute or two
> after EDVF flag is set?
Thanks for pointer.
EDVF seems to be exposed as health. ... but only if battery is
calibrated, AFAICT.
if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
...
cache.health = -ENODATA;
Plus, it prioritizes battery cold over battery dead. IMO we don't need
to shutdown on battery cold (we just may not charge the battery), but
we need to shutdown on battery dead.
So something like this?
Pavel
Signed-off-by: Pavel Machek <pavel@ucw.cz>
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 8eb2f8f..5ddf6d7 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -680,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
/* Unlikely but important to return first */
if (unlikely(bq27xxx_battery_overtemp(di, flags)))
return POWER_SUPPLY_HEALTH_OVERHEAT;
- if (unlikely(bq27xxx_battery_undertemp(di, flags)))
- return POWER_SUPPLY_HEALTH_COLD;
if (unlikely(bq27xxx_battery_dead(di, flags)))
return POWER_SUPPLY_HEALTH_DEAD;
+ if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+ return POWER_SUPPLY_HEALTH_COLD;
return POWER_SUPPLY_HEALTH_GOOD;
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/199ed265/attachment.sig>
^ permalink raw reply related
* [PATCH] arm/arm64: KVM: Perform local TLB invalidation when multiplexing vcpus on a single CPU
From: Marc Zyngier @ 2016-10-25 10:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024161656.GM15620@leverpostej>
Hi Mark,
On 24/10/16 17:16, Mark Rutland wrote:
> Hi Marc,
>
> On Mon, Oct 24, 2016 at 04:31:28PM +0100, Marc Zyngier wrote:
>> Architecturally, TLBs are private to the (physical) CPU they're
>> associated with. But when multiple vcpus from the same VM are
>> being multiplexed on the same CPU, the TLBs are not private
>> to the vcpus (and are actually shared across the VMID).
>>
>> Let's consider the following scenario:
>>
>> - vcpu-0 maps PA to VA
>> - vcpu-1 maps PA' to VA
>>
>> If run on the same physical CPU, vcpu-1 can hit TLB entries generated
>> by vcpu-0 accesses, and access the wrong physical page.
>
> It might be worth noting that this could also lead to TLB conflicts and
> other such fun usually associated with missing TLB maintenance,
> depending on the two mappings (or the intermediate stages thereof).
>
>> The solution to this is to keep a per-VM map of which vcpu ran last
>> on each given physical CPU, and invalidate local TLBs when switching
>> to a different vcpu from the same VM.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> Modulo my comment on preemptiblity of kvm_arch_sched_in, everything
> below is a nit. Assuming that's not preemptible, this looks right to me.
>
> FWIW, with or without the other comments considered:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>
>> ---
>> arch/arm/include/asm/kvm_host.h | 5 +++++
>> arch/arm/include/asm/kvm_hyp.h | 1 +
>> arch/arm/kvm/arm.c | 35 ++++++++++++++++++++++++++++++++++-
>> arch/arm/kvm/hyp/switch.c | 9 +++++++++
>> arch/arm64/include/asm/kvm_host.h | 6 +++++-
>> arch/arm64/kvm/hyp/switch.c | 8 ++++++++
>> 6 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 2d19e02..035e744 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -57,6 +57,8 @@ struct kvm_arch {
>> /* VTTBR value associated with below pgd and vmid */
>> u64 vttbr;
>>
>> + int __percpu *last_vcpu_ran;
>> +
>> /* Timer */
>> struct arch_timer_kvm timer;
>>
>> @@ -174,6 +176,9 @@ struct kvm_vcpu_arch {
>> /* vcpu power-off state */
>> bool power_off;
>>
>> + /* TLBI required */
>> + bool requires_tlbi;
>
> A bit of a nit, but it's not clear which class of TLBI is required, or
> why. It's probably worth a comment (and perhaps a bikeshed), like:
>
> /*
> * Local TLBs potentially contain conflicting entries from
> * another vCPU within this VMID. All entries for this VMID must
> * be invalidated from (local) TLBs before we run this vCPU.
> */
> bool tlb_vmid_stale;
Yup, looks good.
>
> [...]
>
>> +void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
>> +{
>> + int *last_ran;
>> +
>> + last_ran = per_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran, cpu);
>> +
>> + /*
>> + * If we're very unlucky and get preempted before having ran
>> + * this vcpu for real, we'll end-up in a situation where any
>> + * vcpu that gets scheduled will perform an invalidation (this
>> + * vcpu explicitely requires it, and all the others will have
>> + * a different vcpu_id).
>> + */
>
> Nit: s/explicitely/explicitly/
>
> To bikeshed a little further, perhaps:
>
> /*
> * We might get preempted before the vCPU actually runs, but
> * this is fine. Our TLBI stays pending until we actually make
> * it to __activate_vm, so we won't miss a TLBI. If another vCPU
> * gets scheduled, it will see our vcpu_id in last_ran, and pend
> * a TLBI for itself.
> */
Looks good too. I'll incorporate this into v2.
>
>> + if (*last_ran != vcpu->vcpu_id) {
>> + if (*last_ran != -1)
>> + vcpu->arch.requires_tlbi = true;
>> +
>> + *last_ran = vcpu->vcpu_id;
>> + }
>> +}
>
> I take it this function is run in some non-preemptible context; if so,
> this looks correct to me.
>
> If this is preemptible, then this looks racy.
This function is called from a preempt notifier, which itself isn't
preemptible.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox