* [PATCH v10 01/20] mtd: rawnand: Add a kernel doc to the ECC algorithm enumeration
From: Miquel Raynal @ 2020-06-03 17:57 UTC (permalink / raw)
To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, linux-mtd
Cc: Mark Rutland, devicetree, Julien Su, Miquel Raynal, Rob Herring,
Thomas Petazzoni, Boris Brezillon, Mason Yang, linux-arm-kernel
In-Reply-To: <20200603175759.19948-1-miquel.raynal@bootlin.com>
Before moving it to the generic raw NAND core, ensure the enumeration
is properly described.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
include/linux/mtd/rawnand.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 65b1c1c18b41..70b2ddd0aedc 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -92,6 +92,13 @@ enum nand_ecc_mode {
NAND_ECC_ON_DIE,
};
+/**
+ * enum nand_ecc_algo - NAND ECC algorithm
+ * @NAND_ECC_UNKNOWN: Unknown algorithm
+ * @NAND_ECC_HAMMING: Hamming algorithm
+ * @NAND_ECC_BCH: Bose-Chaudhuri-Hocquenghem algorithm
+ * @NAND_ECC_RS: Reed-Solomon algorithm
+ */
enum nand_ecc_algo {
NAND_ECC_UNKNOWN,
NAND_ECC_HAMMING,
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v10 00/20] Introduction of the generic ECC framework
From: Miquel Raynal @ 2020-06-03 17:57 UTC (permalink / raw)
To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, linux-mtd
Cc: Mark Rutland, devicetree, Julien Su, Miquel Raynal, Rob Herring,
Thomas Petazzoni, Boris Brezillon, Mason Yang, linux-arm-kernel
After discussing with Boris, he convinced me that the changes should
be done in another order. That's why first I update the nand_ecc_algo
enumeration in raw NAND, then I introduce the new bindings and the ECC
core itself. Only after that, I patch the raw NAND core (and slightly
SPI-NAND) to share the generic data with the NAND core/ECC framework.
Changes in v10:
* After reworking the entire series, I only kept 4 patches aside,
resending all of them.
* Fixed typos, updated commit logs as proposed.
* Introduced nanddev_set_ecc_requirements().
* Used spaces instead of tabs in an array.
* Renamed the nand_ecc_is_strong_enough() helper.
* Dropped the use of the MAXIMIZE flag in denali.c.
* Renamed this flag MAXIMIZE_STRENGTH.
* Renamed the of_get_nand_ecc_config*() helpers.
Changes in v9:
* This time sending the additional patchs, not just the old ones with
corrections. v8 should be ignored, sorry for the noise.
Changes in v8:
* Split "Convert generic NAND bits to ECC framework" into several peaces:
> added two helpers
> converted SPI-NAND then raw-NAND.
* Fixed a comment.
* Used the _ooblayout suffix instead of _layout.
Miquel Raynal (20):
mtd: rawnand: Add a kernel doc to the ECC algorithm enumeration
mtd: rawnand: Rename the ECC algorithm enumeration items
mtd: rawnand: Move the nand_ecc_algo enum to the generic NAND layer
mtd: nand: Add a NAND page I/O request type
dt-bindings: mtd: Document nand-ecc-placement
dt-bindings: mtd: Document nand-ecc-engine
dt-bindings: mtd: Document boolean NAND ECC properties
mtd: nand: Introduce the ECC engine framework
mtd: rawnand: Separate the ECC engine type and the ECC byte placement
mtd: rawnand: Use the new ECC engine type enumeration
mtd: nand: Create a helper to extract the ECC configuration
mtd: spinand: Use nanddev_get_ecc_conf() when relevant
mtd: nand: Create helpers to set/extract the ECC requirements
mtd: rawnand: Use nanddev_get/set_ecc_requirements() when relevant
mtd: nand: Use the new generic ECC object
mtd: rawnand: Make use of the ECC framework
mtd: rawnand: Use the ECC framework OOB layouts
mtd: rawnand: Use the ECC framework nand_ecc_is_strong_enough() helper
mtd: rawnand: Use the ECC framework user input parsing bits
mtd: rawnand: Use the NAND framework user_conf object for ECC flags
.../bindings/mtd/nand-controller.yaml | 28 +
arch/arm/mach-davinci/board-da830-evm.c | 2 +-
arch/arm/mach-davinci/board-da850-evm.c | 2 +-
arch/arm/mach-davinci/board-dm355-evm.c | 2 +-
arch/arm/mach-davinci/board-dm355-leopard.c | 3 +-
arch/arm/mach-davinci/board-dm365-evm.c | 2 +-
arch/arm/mach-davinci/board-dm644x-evm.c | 2 +-
arch/arm/mach-davinci/board-dm646x-evm.c | 2 +-
arch/arm/mach-davinci/board-mityomapl138.c | 2 +-
arch/arm/mach-davinci/board-neuros-osd2.c | 2 +-
arch/arm/mach-davinci/board-omapl138-hawk.c | 2 +-
arch/arm/mach-s3c24xx/common-smdk.c | 2 +-
arch/arm/mach-s3c24xx/mach-anubis.c | 2 +-
arch/arm/mach-s3c24xx/mach-at2440evb.c | 2 +-
arch/arm/mach-s3c24xx/mach-bast.c | 2 +-
arch/arm/mach-s3c24xx/mach-gta02.c | 2 +-
arch/arm/mach-s3c24xx/mach-jive.c | 2 +-
arch/arm/mach-s3c24xx/mach-mini2440.c | 2 +-
arch/arm/mach-s3c24xx/mach-osiris.c | 2 +-
arch/arm/mach-s3c24xx/mach-qt2410.c | 2 +-
arch/arm/mach-s3c24xx/mach-rx1950.c | 2 +-
arch/arm/mach-s3c24xx/mach-rx3715.c | 2 +-
arch/arm/mach-s3c24xx/mach-vstms.c | 2 +-
arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
drivers/mtd/nand/Kconfig | 8 +
drivers/mtd/nand/Makefile | 2 +
drivers/mtd/nand/ecc.c | 484 +++++++++++++++
drivers/mtd/nand/raw/Kconfig | 1 +
drivers/mtd/nand/raw/ams-delta.c | 4 +-
drivers/mtd/nand/raw/arasan-nand-controller.c | 16 +-
drivers/mtd/nand/raw/atmel/nand-controller.c | 31 +-
drivers/mtd/nand/raw/au1550nd.c | 4 +-
.../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c | 3 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 28 +-
.../mtd/nand/raw/cadence-nand-controller.c | 4 +-
drivers/mtd/nand/raw/cafe_nand.c | 3 +-
drivers/mtd/nand/raw/cs553x_nand.c | 2 +-
drivers/mtd/nand/raw/davinci_nand.c | 38 +-
drivers/mtd/nand/raw/denali.c | 3 +-
drivers/mtd/nand/raw/denali_pci.c | 2 +-
drivers/mtd/nand/raw/diskonchip.c | 3 +-
drivers/mtd/nand/raw/fsl_elbc_nand.c | 20 +-
drivers/mtd/nand/raw/fsl_ifc_nand.c | 12 +-
drivers/mtd/nand/raw/fsl_upm.c | 4 +-
drivers/mtd/nand/raw/fsmc_nand.c | 14 +-
drivers/mtd/nand/raw/gpio.c | 4 +-
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 15 +-
drivers/mtd/nand/raw/hisi504_nand.c | 6 +-
.../mtd/nand/raw/ingenic/ingenic_nand_drv.c | 20 +-
drivers/mtd/nand/raw/lpc32xx_mlc.c | 2 +-
drivers/mtd/nand/raw/lpc32xx_slc.c | 3 +-
drivers/mtd/nand/raw/marvell_nand.c | 35 +-
drivers/mtd/nand/raw/meson_nand.c | 2 +-
drivers/mtd/nand/raw/mpc5121_nfc.c | 4 +-
drivers/mtd/nand/raw/mtk_nand.c | 12 +-
drivers/mtd/nand/raw/mxc_nand.c | 25 +-
drivers/mtd/nand/raw/nand_base.c | 565 +++++++-----------
drivers/mtd/nand/raw/nand_esmt.c | 15 +-
drivers/mtd/nand/raw/nand_hynix.c | 44 +-
drivers/mtd/nand/raw/nand_jedec.c | 9 +-
drivers/mtd/nand/raw/nand_micron.c | 23 +-
drivers/mtd/nand/raw/nand_onfi.c | 17 +-
drivers/mtd/nand/raw/nand_samsung.c | 22 +-
drivers/mtd/nand/raw/nand_toshiba.c | 19 +-
drivers/mtd/nand/raw/nandsim.c | 8 +-
drivers/mtd/nand/raw/ndfc.c | 2 +-
drivers/mtd/nand/raw/omap2.c | 22 +-
drivers/mtd/nand/raw/orion_nand.c | 4 +-
drivers/mtd/nand/raw/pasemi_nand.c | 4 +-
drivers/mtd/nand/raw/plat_nand.c | 4 +-
drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
drivers/mtd/nand/raw/r852.c | 3 +-
drivers/mtd/nand/raw/s3c2410.c | 20 +-
drivers/mtd/nand/raw/sh_flctl.c | 6 +-
drivers/mtd/nand/raw/sharpsl.c | 2 +-
drivers/mtd/nand/raw/socrates_nand.c | 5 +-
drivers/mtd/nand/raw/stm32_fmc2_nand.c | 9 +-
drivers/mtd/nand/raw/sunxi_nand.c | 27 +-
drivers/mtd/nand/raw/tango_nand.c | 4 +-
drivers/mtd/nand/raw/tegra_nand.c | 37 +-
drivers/mtd/nand/raw/tmio_nand.c | 2 +-
drivers/mtd/nand/raw/txx9ndfmc.c | 2 +-
drivers/mtd/nand/raw/vf610_nfc.c | 6 +-
drivers/mtd/nand/raw/xway_nand.c | 4 +-
drivers/mtd/nand/spi/core.c | 12 +-
drivers/mtd/nand/spi/macronix.c | 7 +-
drivers/mtd/nand/spi/toshiba.c | 6 +-
include/linux/mtd/nand.h | 188 +++++-
include/linux/mtd/rawnand.h | 34 +-
include/linux/platform_data/mtd-davinci.h | 9 +-
.../linux/platform_data/mtd-nand-s3c2410.h | 2 +-
93 files changed, 1315 insertions(+), 723 deletions(-)
create mode 100644 drivers/mtd/nand/ecc.c
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Andrzej Pietrasiewicz @ 2020-06-03 17:54 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov
Cc: Nick Dyer, linux-iio, Benjamin Tissoires, platform-driver-x86,
ibm-acpi-devel, Laxman Dewangan, Peter Meerwald-Stadler, kernel,
Fabio Estevam, linux-samsung-soc, Krzysztof Kozlowski,
Jonathan Hunter, linux-acpi, Kukjin Kim, NXP Linux Team,
linux-input, Len Brown, Peter Hutterer, Michael Hennerich,
Sascha Hauer, Sylvain Lemieux, Henrique de Moraes Holschuh,
Vladimir Zapolskiy, Lars-Peter Clausen, linux-tegra,
linux-arm-kernel, Barry Song, Ferruh Yigit, patches,
Rafael J . Wysocki, Thierry Reding, Sangwon Jee,
Pengutronix Kernel Team, Hartmut Knaack, Shawn Guo,
Jonathan Cameron
In-Reply-To: <681abc14-ef0f-ff15-68ed-944b2f96bdaf@redhat.com>
W dniu 03.06.2020 o 19:38, Hans de Goede pisze:
> Hi,
>
> On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
>> Hi Hans, hi Dmitry,
>
> <snip>
>
>> I'm taking one step back and looking at the ->open() and ->close()
>> driver callbacks. They are called from input_open_device() and
>> input_close_device(), respectively:
>>
>> input_open_device():
>> "This function should be called by input handlers when they
>> want to start receive events from given input device."
>>
>> ->open() callback:
>> "this method is called when the very first user calls
>> input_open_device(). The driver must prepare the device to start
>> generating events (start polling thread, request an IRQ, submit
>> URB, etc.)"
>>
>> input_close_device():
>> "This function should be called by input handlers when they
>> want to stop receive events from given input device."
>>
>> ->close() callback:
>> "this method is called when the very last user calls
>> input_close_device()"
>>
>> It seems to me that the callback names do not reflect their
>> purpose: their meaning is not to "open" or to "close" but to
>> give drivers a chance to control when they start or stop
>> providing events to the input core.
>>
>> What would you say about changing the callbacks' names?
>> I'd envsion: ->provide_events() instead of ->open() and
>> ->stop_events() instead of ->close(). Of course drivers can
>> exploit the fact of knowing that nobody wants any events
>> from them and do whatever they consider appropriate, for
>> example go into a low power mode - but the latter is beyond
>> the scope of the input subsystem and is driver-specific.
>
> I don't have much of an opinion on changing the names,
> to me open/close have always means start/stop receiving
> events. This follows the everything is a file philosophy,
> e.g. you can also not really "open" a serial port,
> yet opening /dev/ttyS0 will activate the receive IRQ
> of the UART, etc. So maybe we just need to make the
> docs clearer rather then do the rename? Doing the
> rename is certainly going to cause a lot of churn.
Right, I can see now that the suggestion to change names is
too far fetched. (I feel that release() would be better
than close(), though). But it exposes the message I wanted to
pass.
>
> Anyways as said, I don't have much of an opinion,
> so I'll leave commenting (more) on this to Dmitry.
>
>> With such a naming change in mind let's consider inhibiting.
>> We want to be able to control when to disregard events from
>> a given device. It makes sense to do it at device level, otherwise
>> such an operation would have to be invoked in all associated
>> handlers (those that have an open handle associating them with
>> the device in question). But of course we can do better than
>> merely ignoring the events received: we can tell the drivers
>> that we don't want any events from them, and later, at uninhibit
>> time, tell them to start providing the events again. Conceptually,
>> the two operations (provide or don't provide envents) are exactly
>> the same thing we want to be happening at input_open_device() and
>> input_close_device() time. To me, changing the names of
>> ->open() and ->close() exposes this fact very well.
>>
>> Consequently, ->inhibit() and ->uninhibit() won't be needed,
>> and drivers which already implement ->provide_events() (formerly
>> ->open()) and ->stop_events() (formerly ->close()) will receive
>> full inhibit/uninhibit support for free (subject to how well they
>> implement ->provide_events()/->stop_events()). Unless we can come
>> up with what the drivers might be doing on top of ->stop_events()
>> and ->provide_events() when inhibiting/uninhibiting, but it seems
>> to me we can't. Can we?
>
> Right. I'm happy that you've come to see that both on open/close
> and on inhibit/uninhibit we want to "start receiving events" and
> "stop receiving events", so that we only need one set of callbacks.
>
Yeah, that's my conclusion - at least on a conceptual level.
That said, what I can imagine is an existing driver (e.g. elan_i2c)
which does not implement neither open() nor close(), but does have
suspend() and resume(). Then it is maybe a bit easier to add inhibit()
and uninhibit() /they would be similar to suspend and resume/ instead
of open() and close(): If only open() and close() are possible, then
the probe function needs to be extended to "close" the device before it
gets registered, because from the moment it is registered it might be
opened right away. And the device must be available earlier during the
course of probe to query some parameters through i2c:
+static int elan_reactivate(struct elan_tp_data *data)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ ret = elan_enable_power(data);
+ if (ret)
+ dev_err(dev, "failed to restore power: %d\n", ret);
+
+ ret = elan_initialize(data);
+ if (ret)
+ dev_err(dev, "failed to re-initialize touchpad: %d\n", ret);
+
+ return ret;
+}
+
+static int elan_open(struct input_dev *input)
+{
+ struct elan_tp_data *data = input_get_drvdata(input);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ dev_dbg(&client->dev, "uninhibiting\n");
+
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return ret;
+
+ ret = elan_reactivate(data);
+ if (ret == 0)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+ return ret;
+}
+
+static int elan_inhibit(struct input_dev *input)
+{
+ struct elan_tp_data *data = input_get_drvdata(input);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ dev_dbg(&client->dev, "closing\n");
+
+ /*
+ * We are taking the mutex to make sure sysfs operations are
+ * complete before we attempt to bring the device into low[er]
+ * power mode.
+ */
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return ret;
+
+ disable_irq(client->irq);
+
+ ret = elan_disable_power(data);
+ if (ret)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+ return ret;
+}
+
+static void elan_close(struct input_dev *input)
+{
+ elan_inhibit(input);
+}
+
static int elan_query_device_info(struct elan_tp_data *data)
{
int error;
u16 ic_type;
error = data->ops->get_version(data->client, false, &data->fw_version);
if (error)
return error;
error = data->ops->get_checksum(data->client, false,
&data->fw_checksum);
if (error)
return error;
error = data->ops->get_version(data->client, true, &data->iap_version);
if (error)
return error;
@@ -1071,34 +1141,36 @@ static int elan_setup_trackpoint_input_device(struct
elan_tp_data *data)
static int elan_setup_input_device(struct elan_tp_data *data)
{
struct device *dev = &data->client->dev;
struct input_dev *input;
unsigned int max_width = max(data->width_x, data->width_y);
unsigned int min_width = min(data->width_x, data->width_y);
int error;
input = devm_input_allocate_device(dev);
if (!input)
return -ENOMEM;
input->name = "Elan Touchpad";
input->id.bustype = BUS_I2C;
input->id.vendor = ELAN_VENDOR_ID;
input->id.product = data->product_id;
+ input->open = elan_open;
+ input->close = elan_close;
input_set_drvdata(input, data);
error = input_mt_init_slots(input, ETP_MAX_FINGERS,
INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
if (error) {
dev_err(dev, "failed to initialize MT slots: %d\n", error);
return error;
}
__set_bit(EV_ABS, input->evbit);
__set_bit(INPUT_PROP_POINTER, input->propbit);
if (data->clickpad) {
__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
} else {
__set_bit(BTN_RIGHT, input->keybit);
if (data->middle_button)
__set_bit(BTN_MIDDLE, input->keybit);
@@ -1253,34 +1325,40 @@ static int elan_probe(struct i2c_client *client,
if (!irqflags)
irqflags = IRQF_TRIGGER_FALLING;
error = devm_request_threaded_irq(dev, client->irq, NULL, elan_isr,
irqflags | IRQF_ONESHOT,
client->name, data);
if (error) {
dev_err(dev, "cannot register irq=%d\n", client->irq);
return error;
}
error = devm_device_add_groups(dev, elan_sysfs_groups);
if (error) {
dev_err(dev, "failed to create sysfs attributes: %d\n", error);
return error;
}
+ error = elan_inhibit(data->input);
+ if (error) {
+ dev_err(dev, "failed to inhibit input device before registering: %d\n", error);
+ return error;
+ }
+
error = input_register_device(data->input);
if (error) {
dev_err(dev, "failed to register input device: %d\n", error);
return error;
}
if (data->tp_input) {
error = input_register_device(data->tp_input);
if (error) {
dev_err(&client->dev,
"failed to register TrackPoint input device: %d\n",
error);
return error;
}
}
/*
@@ -1294,72 +1372,71 @@ static int elan_probe(struct i2c_client *client,
}
static int __maybe_unused elan_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
struct elan_tp_data *data = i2c_get_clientdata(client);
int ret;
/*
* We are taking the mutex to make sure sysfs operations are
* complete before we attempt to bring the device into low[er]
* power mode.
*/
ret = mutex_lock_interruptible(&data->sysfs_mutex);
if (ret)
return ret;
- disable_irq(client->irq);
+ mutex_lock(&data->input->mutex);
+ if (input_device_enabled(data->input)) {
+ disable_irq(client->irq);
- if (device_may_wakeup(dev)) {
- ret = elan_sleep(data);
- /* Enable wake from IRQ */
- data->irq_wake = (enable_irq_wake(client->irq) == 0);
- } else {
- ret = elan_disable_power(data);
+ if (device_may_wakeup(dev)) {
+ ret = elan_sleep(data);
+ /* Enable wake from IRQ */
+ data->irq_wake = (enable_irq_wake(client->irq) == 0);
+ } else {
+ ret = elan_disable_power(data);
+ }
}
+ mutex_unlock(&data->input->mutex);
mutex_unlock(&data->sysfs_mutex);
return ret;
}
static int __maybe_unused elan_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
struct elan_tp_data *data = i2c_get_clientdata(client);
- int error;
+ int ret = 0;
- if (device_may_wakeup(dev) && data->irq_wake) {
- disable_irq_wake(client->irq);
- data->irq_wake = false;
- }
+ mutex_lock(&data->input->mutex);
+ if (input_device_enabled(data->input)) {
+ if (data->irq_wake) {
+ disable_irq_wake(client->irq);
+ data->irq_wake = false;
+ }
- error = elan_enable_power(data);
- if (error) {
- dev_err(dev, "power up when resuming failed: %d\n", error);
- goto err;
+ ret = elan_reactivate(data);
+ enable_irq(data->client->irq);
}
+ mutex_unlock(&data->input->mutex);
- error = elan_initialize(data);
- if (error)
- dev_err(dev, "initialize when resuming failed: %d\n", error);
-
-err:
- enable_irq(data->client->irq);
- return error;
+ return ret;
}
Regards,
Andrzej
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF
From: Mathieu Poirier @ 2020-06-03 17:44 UTC (permalink / raw)
To: Robin Murphy
Cc: Sai Prakash Ranjan, Suzuki K Poulose, linux-arm-msm, Coresight ML,
Linux Kernel Mailing List, Stephen Boyd, linux-arm-kernel,
Mike Leach
In-Reply-To: <1a5a6a6d-b86d-df45-cf91-7081e70d88a3@arm.com>
On Wed, Jun 03, 2020 at 02:34:10PM +0100, Robin Murphy wrote:
> On 2020-06-03 14:22, Mike Leach wrote:
> > Hi Sai,
> >
> > On Wed, 3 Jun 2020 at 13:14, Sai Prakash Ranjan
> > <saiprakash.ranjan@codeaurora.org> wrote:
> > >
> > > Hi Mike,
> > >
> > > On 2020-06-03 16:57, Mike Leach wrote:
> > > > Hi,
> > > >
> > > > On Wed, 3 Jun 2020 at 11:24, Sai Prakash Ranjan
> > > > <saiprakash.ranjan@codeaurora.org> wrote:
> > > > >
> > > > > Hi Mike,
> > > > >
> > > > > Thanks again for looking at this.
> > > > >
> > > > > On 2020-06-03 03:42, Mike Leach wrote:
> > > > > [...]
> > > > >
> > > > > > >
> > > > > > > SMMU/IOMMU won't be able to do much here as it is the client's
> > > > > > > responsiblity to
> > > > > > > properly shutdown and SMMU device link just makes sure that
> > > > > > > SMMU(supplier) shutdown is
> > > > > > > called only after its consumers shutdown callbacks are called.
> > > > > >
> > > > > > I think this use case can be handled slightly differently than the
> > > > > > general requirements for modular CoreSight drivers.
> > > > > >
> > > > > > What is needed here is a way of stopping the underlying ETR hardware
> > > > > > from issuing data to the SMMU, until the entire device has been shut
> > > > > > down, in a way that does not remove the driver, breaking existing
> > > > > > references and causing a system crash.
> > > > > >
> > > > > > We could introduce a new mode to the ETR driver - e.g.
> > > > > > CS_MODE_SHUTDOWN.
> > > > > >
> > > > > > At the end of the block tmc_shutdown(struct amba_device *adev), set
> > > > > > drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister().
> > > > > > This new mode can be used to prevent the underlying hardware from
> > > > > > being able to restart until the device is re-powered.
> > > > > >
> > > > > > This mode can be detected in the code that enables / disables the ETR
> > > > > > and handled appropriately (updates to tmc_enable_etr_sink and
> > > > > > tmc_disable_etr_sink).
> > > > > > This mode will persist until the device is re-started - but because we
> > > > > > are on the device shutdown path this is not an issue.
> > > > > >
> > > > > > This should leave the CoreSight infrastructure stable until the
> > > > > > drivers are shut down normally as part of the device power down
> > > > > > process.
> > > > > >
> > > > >
> > > > > Sounds good to me, but if the coresight_unregister() is the trouble
> > > > > point
> > > > > causing these crashes, then can't we just remove that from
> > > > > tmc_shutdown()
> > > > > callback? This would be like maintaining the same behaviour as now
> > > > > where
> > > > > on reboot/shutdown we basically don't do anything except for disabling
> > > > > ETR.
> > > >
> > > > No - the new mode prevents race conditions where the thread shutting
> > > > down the SMMU does the ETR shutdown, but then another thread happens
> > > > to be trying to start trace and restarts the ETR.
> > > > It also prevents the condition Mathieu discussed where a thread might
> > > > be attempting to shutdown trace - this could try to disable the
> > > > hardware again re-releasing resources/ re-flushing and waiting for
> > > > stop.
> > > >
> > >
> > > I do not think there will a race between SMMU shutdown and ETR shutdown.
> > > Driver core takes care of calling SMMU shutdown after its consumer
> > > shutdown callbacks via device link, otherwise there would already be
> > > bugs in all other client drivers.
> > >
> >
> > I am not saying there could be a race between tmc_shutdowm and
> > Smmu_shutdown - there may be a case if the coresight_disable_path
> > sequence is running and gets to the point of disabling the ETR after
> > the SMMU callback has disabled it.
>
> I'm confused now - there is no "SMMU callback", we're talking about the
> system-wide cleanup from kernel_shutdown_prepare() or
> kernel_restart_prepare(). As far as I'm aware userspace should be long gone
> by that point, so although trace may have been left running, the chance of
> racing against other driver operations seems pretty unlikely.
Robin has a point - user space is long gone at this time. As such the first
question to ask is what kind of CS session was running at the time the system
was shutting down. Was it a perf session of a sysfs session?
I'm guessing it was a sysfs session because user space has been blown away a
while back and part of that process should have killed all perf sessions.
If I am correct then simply switching off the ETR HW in the shutdown() amba bus
callback should be fine - otherwise Mike's approach is mandatory. There is
also the exchange between Robin and Sai about removing the SMMU shutdown
callback, but that thread is still incomplete.
Thanks,
Mathieu
>
> Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH][v2] iommu: arm-smmu-v3: Copy SMMU table for kdump kernel
From: Prabhakar Kushwaha @ 2020-06-03 17:42 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Kuppuswamy Sathyanarayanan, Ganapatrao Prabhakerrao Kulkarni,
Myron Stowe, Vijay Mohan Pandarathil, Marc Zyngier,
Bhupesh Sharma, kexec mailing list, Robin Murphy, linux-pci,
Prabhakar Kushwaha, Will Deacon, linux-arm-kernel
In-Reply-To: <20200529193334.GA451372@bjorn-Precision-5520>
Hi Bjorn,
On Sat, May 30, 2020 at 1:03 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, May 29, 2020 at 07:48:10PM +0530, Prabhakar Kushwaha wrote:
> > On Thu, May 28, 2020 at 1:48 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Wed, May 27, 2020 at 05:14:39PM +0530, Prabhakar Kushwaha wrote:
> > > > On Fri, May 22, 2020 at 4:19 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Thu, May 21, 2020 at 09:28:20AM +0530, Prabhakar Kushwaha wrote:
> > > > > > On Wed, May 20, 2020 at 4:52 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Thu, May 14, 2020 at 12:47:02PM +0530, Prabhakar Kushwaha wrote:
> > > > > > > > On Wed, May 13, 2020 at 3:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > > > On Mon, May 11, 2020 at 07:46:06PM -0700, Prabhakar Kushwaha wrote:
> > > > > > > > > > An SMMU Stream table is created by the primary kernel. This table is
> > > > > > > > > > used by the SMMU to perform address translations for device-originated
> > > > > > > > > > transactions. Any crash (if happened) launches the kdump kernel which
> > > > > > > > > > re-creates the SMMU Stream table. New transactions will be translated
> > > > > > > > > > via this new table..
> > > > > > > > > >
> > > > > > > > > > There are scenarios, where devices are still having old pending
> > > > > > > > > > transactions (configured in the primary kernel). These transactions
> > > > > > > > > > come in-between Stream table creation and device-driver probe.
> > > > > > > > > > As new stream table does not have entry for older transactions,
> > > > > > > > > > it will be aborted by SMMU.
> > > > > > > > > >
> > > > > > > > > > Similar observations were found with PCIe-Intel 82576 Gigabit
> > > > > > > > > > Network card. It sends old Memory Read transaction in kdump kernel.
> > > > > > > > > > Transactions configured for older Stream table entries, that do not
> > > > > > > > > > exist any longer in the new table, will cause a PCIe Completion Abort.
> > > > > > > > >
> > > > > > > > > That sounds like exactly what we want, doesn't it?
> > > > > > > > >
> > > > > > > > > Or do you *want* DMA from the previous kernel to complete? That will
> > > > > > > > > read or scribble on something, but maybe that's not terrible as long
> > > > > > > > > as it's not memory used by the kdump kernel.
> > > > > > > >
> > > > > > > > Yes, Abort should happen. But it should happen in context of driver.
> > > > > > > > But current abort is happening because of SMMU and no driver/pcie
> > > > > > > > setup present at this moment.
> > > > > > >
> > > > > > > I don't understand what you mean by "in context of driver." The whole
> > > > > > > problem is that we can't control *when* the abort happens, so it may
> > > > > > > happen in *any* context. It may happen when a NIC receives a packet
> > > > > > > or at some other unpredictable time.
> > > > > > >
> > > > > > > > Solution of this issue should be at 2 place
> > > > > > > > a) SMMU level: I still believe, this patch has potential to overcome
> > > > > > > > issue till finally driver's probe takeover.
> > > > > > > > b) Device level: Even if something goes wrong. Driver/device should
> > > > > > > > able to recover.
> > > > > > > >
> > > > > > > > > > Returned PCIe completion abort further leads to AER Errors from APEI
> > > > > > > > > > Generic Hardware Error Source (GHES) with completion timeout.
> > > > > > > > > > A network device hang is observed even after continuous
> > > > > > > > > > reset/recovery from driver, Hence device is no more usable.
> > > > > > > > >
> > > > > > > > > The fact that the device is no longer usable is definitely a problem.
> > > > > > > > > But in principle we *should* be able to recover from these errors. If
> > > > > > > > > we could recover and reliably use the device after the error, that
> > > > > > > > > seems like it would be a more robust solution that having to add
> > > > > > > > > special cases in every IOMMU driver.
> > > > > > > > >
> > > > > > > > > If you have details about this sort of error, I'd like to try to fix
> > > > > > > > > it because we want to recover from that sort of error in normal
> > > > > > > > > (non-crash) situations as well.
> > > > > > > > >
> > > > > > > > Completion abort case should be gracefully handled. And device should
> > > > > > > > always remain usable.
> > > > > > > >
> > > > > > > > There are 2 scenario which I am testing with Ethernet card PCIe-Intel
> > > > > > > > 82576 Gigabit Network card.
> > > > > > > >
> > > > > > > > I) Crash testing using kdump root file system: De-facto scenario
> > > > > > > > - kdump file system does not have Ethernet driver
> > > > > > > > - A lot of AER prints [1], making it impossible to work on shell
> > > > > > > > of kdump root file system.
> > > > > > >
> > > > > > > In this case, I think report_error_detected() is deciding that because
> > > > > > > the device has no driver, we can't do anything. The flow is like
> > > > > > > this:
> > > > > > >
> > > > > > > aer_recover_work_func # aer_recover_work
> > > > > > > kfifo_get(aer_recover_ring, entry)
> > > > > > > dev = pci_get_domain_bus_and_slot
> > > > > > > cper_print_aer(dev, ...)
> > > > > > > pci_err("AER: aer_status:")
> > > > > > > pci_err("AER: [14] CmpltTO")
> > > > > > > pci_err("AER: aer_layer=")
> > > > > > > if (AER_NONFATAL)
> > > > > > > pcie_do_recovery(dev, pci_channel_io_normal)
> > > > > > > status = CAN_RECOVER
> > > > > > > pci_walk_bus(report_normal_detected)
> > > > > > > report_error_detected
> > > > > > > if (!dev->driver)
> > > > > > > vote = NO_AER_DRIVER
> > > > > > > pci_info("can't recover (no error_detected callback)")
> > > > > > > *result = merge_result(*, NO_AER_DRIVER)
> > > > > > > # always NO_AER_DRIVER
> > > > > > > status is now NO_AER_DRIVER
> > > > > > >
> > > > > > > So pcie_do_recovery() does not call .report_mmio_enabled() or .slot_reset(),
> > > > > > > and status is not RECOVERED, so it skips .resume().
> > > > > > >
> > > > > > > I don't remember the history there, but if a device has no driver and
> > > > > > > the device generates errors, it seems like we ought to be able to
> > > > > > > reset it.
> > > > > >
> > > > > > But how to reset the device considering there is no driver.
> > > > > > Hypothetically, this case should be taken care by PCIe subsystem to
> > > > > > perform reset at PCIe level.
> > > > >
> > > > > I don't understand your question. The PCI core (not the device
> > > > > driver) already does the reset. When pcie_do_recovery() calls
> > > > > reset_link(), all devices on the other side of the link are reset.
> > > > >
> > > > > > > We should be able to field one (or a few) AER errors, reset the
> > > > > > > device, and you should be able to use the shell in the kdump kernel.
> > > > > > >
> > > > > > here kdump shell is usable only problem is a "lot of AER Errors". One
> > > > > > cannot see what they are typing.
> > > > >
> > > > > Right, that's what I expect. If the PCI core resets the device, you
> > > > > should get just a few AER errors, and they should stop after the
> > > > > device is reset.
> > > > >
> > > > > > > > - Note kdump shell allows to use makedumpfile, vmcore-dmesg applications.
> > > > > > > >
> > > > > > > > II) Crash testing using default root file system: Specific case to
> > > > > > > > test Ethernet driver in second kernel
> > > > > > > > - Default root file system have Ethernet driver
> > > > > > > > - AER error comes even before the driver probe starts.
> > > > > > > > - Driver does reset Ethernet card as part of probe but no success.
> > > > > > > > - AER also tries to recover. but no success. [2]
> > > > > > > > - I also tries to remove AER errors by using "pci=noaer" bootargs
> > > > > > > > and commenting ghes_handle_aer() from GHES driver..
> > > > > > > > than different set of errors come which also never able to recover [3]
> > > > > > > >
> > > > > >
> > > > > > Please suggest your view on this case. Here driver is preset.
> > > > > > (driver/net/ethernet/intel/igb/igb_main.c)
> > > > > > In this case AER errors starts even before driver probe starts.
> > > > > > After probe, driver does the device reset with no success and even AER
> > > > > > recovery does not work.
> > > > >
> > > > > This case should be the same as the one above. If we can change the
> > > > > PCI core so it can reset the device when there's no driver, that would
> > > > > apply to case I (where there will never be a driver) and to case II
> > > > > (where there is no driver now, but a driver will probe the device
> > > > > later).
> > > >
> > > > Does this means change are required in PCI core.
> > >
> > > Yes, I am suggesting that the PCI core does not do the right thing
> > > here.
> > >
> > > > I tried following changes in pcie_do_recovery() but it did not help.
> > > > Same error as before.
> > > >
> > > > -- a/drivers/pci/pcie/err.c
> > > > +++ b/drivers/pci/pcie/err.c
> > > > pci_info(dev, "broadcast resume message\n");
> > > > pci_walk_bus(bus, report_resume, &status);
> > > > @@ -203,7 +207,12 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> > > > return status;
> > > >
> > > > failed:
> > > > pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
> > > > + pci_reset_function(dev);
> > > > + pci_aer_clear_device_status(dev);
> > > > + pci_aer_clear_nonfatal_status(dev);
> > >
> > > Did you confirm that this resets the devices in question (0000:09:00.0
> > > and 0000:09:00.1, I think), and what reset mechanism this uses (FLR,
> > > PM, etc)?
> >
> > Earlier reset was happening with P2P bridge(0000:00:09.0) this the
> > reason no effect. After making following changes, both devices are
> > now getting reset.
> > Both devices are using FLR.
> >
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index 117c0a2b2ba4..26b908f55aef 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -66,6 +66,20 @@ static int report_error_detected(struct pci_dev *dev,
> > if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> > vote = PCI_ERS_RESULT_NO_AER_DRIVER;
> > pci_info(dev, "can't recover (no
> > error_detected callback)\n");
> > +
> > + pci_save_state(dev);
> > + pci_cfg_access_lock(dev);
> > +
> > + /* Quiesce the device completely */
> > + pci_write_config_word(dev, PCI_COMMAND,
> > + PCI_COMMAND_INTX_DISABLE);
> > + if (!__pci_reset_function_locked(dev)) {
> > + vote = PCI_ERS_RESULT_RECOVERED;
> > + pci_info(dev, "recovered via pci level
> > reset\n");
> > + }
>
> Why do we need to save the state and quiesce the device? The reset
> should disable interrupts anyway. In this particular case where
> there's no driver, I don't think we should have to restore the state.
> We maybe should *remove* the device and re-enumerate it after the
> reset, but the state from before the reset should be irrelevant.
>
I tried pci_reset_fucntion_locked without save/restore then I got the
synchronous abort during igb_probe (case 2 i.e. with driver). This is
100% reproducible.
looks like pci_reset_function_locked is causing PCI configuration
space random. Same is mentioned here
https://www.kernel.org/doc/html/latest/driver-api/pci/pci.html
[ 16.492586] Internal error: synchronous external abort: 96000610 [#1] SMP
[ 16.499362] Modules linked in: mpt3sas(+) igb(+) nvme nvme_core
raid_class scsi_transport_sas i2c_algo_bit mdio libcrc32c gpio_xlp
i2c_xlp9xx(+) uas usb_storage
[ 16.513696] CPU: 0 PID: 477 Comm: systemd-udevd Not tainted 5.7.0-rc3+ #132
[ 16.520644] Hardware name: Cavium Inc. Saber/Saber, BIOS
TX2-FW-Release-3.1-build_01-2803-g74253a541a mm/dd/yyyy
[ 16.530805] pstate: 60400009 (nZCv daif +PAN -UAO)
[ 16.535598] pc : igb_rd32+0x24/0xe0 [igb]
[ 16.539603] lr : igb_get_invariants_82575+0xb0/0xde8 [igb]
[ 16.545074] sp : ffffffc012e2b7e0
[ 16.548375] x29: ffffffc012e2b7e0 x28: ffffffc008baa4d8
[ 16.553674] x27: 0000000000000001 x26: ffffffc008b99a70
[ 16.558972] x25: ffffff8cdef60900 x24: ffffff8cdef60e48
[ 16.564270] x23: ffffff8cf30b50b0 x22: ffffffc011359988
[ 16.569568] x21: ffffff8cdef612e0 x20: ffffff8cdef60e68
[ 16.574866] x19: ffffffc0140a0018 x18: 0000000000000000
[ 16.580164] x17: 0000000000000000 x16: 0000000000000000
[ 16.585463] x15: 0000000000000000 x14: 0000000000000000
[ 16.590761] x13: 0000000000000000 x12: 0000000000000000
[ 16.596059] x11: ffffffc008b86b08 x10: 0000000000000000
[ 16.601357] x9 : ffffffc008b88888 x8 : ffffffc008b81050
[ 16.606655] x7 : 0000000000000000 x6 : ffffff8cdef611a8
[ 16.611952] x5 : ffffffc008b887d8 x4 : ffffffc008ba7a68
[ 16.617250] x3 : 0000000000000000 x2 : ffffffc0140a0000
[ 16.622548] x1 : 0000000000000018 x0 : ffffff8cdef60e48
[ 16.627846] Call trace:
[ 16.630288] igb_rd32+0x24/0xe0 [igb]
[ 16.633943] igb_get_invariants_82575+0xb0/0xde8 [igb]
[ 16.639073] igb_probe+0x264/0xed8 [igb]
[ 16.642989] local_pci_probe+0x48/0xb8
[ 16.646727] pci_device_probe+0x120/0x1b8
[ 16.650735] really_probe+0xe4/0x448
[ 16.654298] driver_probe_device+0xe8/0x140
[ 16.658469] device_driver_attach+0x7c/0x88
[ 16.662638] __driver_attach+0xac/0x178
[ 16.666462] bus_for_each_dev+0x7c/0xd0
[ 16.670284] driver_attach+0x2c/0x38
[ 16.673846] bus_add_driver+0x1a8/0x240
[ 16.677670] driver_register+0x6c/0x128
[ 16.681492] __pci_register_driver+0x4c/0x58
[ 16.685754] igb_init_module+0x64/0x1000 [igb]
[ 16.690189] do_one_initcall+0x54/0x228
[ 16.694021] do_init_module+0x60/0x240
[ 16.697757] load_module+0x1614/0x1970
[ 16.701493] __do_sys_finit_module+0xb4/0x118
[ 16.705837] __arm64_sys_finit_module+0x28/0x38
[ 16.710367] do_el0_svc+0xf8/0x1b8
[ 16.713761] el0_sync_handler+0x12c/0x20c
[ 16.717757] el0_sync+0x158/0x180
[ 16.721062] Code: a90153f3 f9400402 b4000482 8b214053 (b9400273)
[ 16.727144] ---[ end trace 95523d7d37f1d883 ]---
[ 16.731748] Kernel panic - not syncing: Fatal exception
[ 16.736962] Kernel Offset: disabled
[ 16.740438] CPU features: 0x084002,22000c38
[ 16.744607] Memory Limit: none
> > + pci_cfg_access_unlock(dev);
> > + pci_restore_state(dev);
> > } else {
> > vote = PCI_ERS_RESULT_NONE;
> > }
> >
> > in order to take care of case 2 (driver comes after sometime) ==>
> > following code needs to be added to avoid crash during igb_probe. It
> > looks to be a race condition between AER and igb_probe().
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> > b/drivers/net/ethernet/intel/igb/igb_main.c
> > index b46bff8fe056..c48f0a54bb95 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -3012,6 +3012,11 @@ static int igb_probe(struct pci_dev *pdev,
> > const struct pci_device_id *ent)
> > /* Catch broken hardware that put the wrong VF device ID in
> > * the PCIe SR-IOV capability.
> > */
> > + if (pci_dev_trylock(pdev)) {
> > + mdelay(1000);
> > + pci_info(pdev,"device is locked, try waiting 1 sec\n");
> > + }
>
> This is interesting to learn about the AER/driver interaction, but of
> course, we wouldn't want to add code like this permanently.
>
> > Here are the observation with all above changes
> > A) AER errors are less but they are still there for both case 1 (No
> > driver at all) and case 2 (driver comes after some time)
>
> We'll certainly get *some* AER errors. We have to get one before we
> know to reset the device.
>
> > B) Each AER error(NON_FATAL) causes both devices to reset. It happens many times
>
> I'm not sure why we reset both devices. Are we seeing errors from
> both, or could we be more selective in the code?
>
I tried even with a reset of 09.01.0 *only* but again AER errors were
found from 09.00.0 as mentioned in previous mail.
So either do a reset of one or both devices, AER error from 09.00.0 is
inevitable. So better to do rest for all devices connected to the bus.
Following changes looks to be working with these observations for case
1 (No driver at all) & case 2 (driver comes after some time)
A) AER errors are less
B) For NON_FATAL AER errors both devices get reset.
C) Few AER errors(neither NON_FATAL nor FATAL) for 09.00.0 still
comes. (Note this device is never used for networking in the primary
kernel)
D) No action taking for "c" as below changes does not cover "c".
E) No AER errors from any device after some time (At least 8-10 AER
errors, all from 09.00.0)
F) Ping/SSH is working fine in case 2 for kudmp kernel.
Please let me know your view. I can send a patch after detailed testing.
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index 14bb8f54723e..585a43b9c0da 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -66,6 +66,19 @@ static int report_error_detected(struct pci_dev *dev,
if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
vote = PCI_ERS_RESULT_NO_AER_DRIVER;
pci_info(dev, "can't recover (no
error_detected callback)\n");
+
+ pci_save_state(dev);
+ pci_cfg_access_lock(dev);
+
+ pci_write_config_word(dev, PCI_COMMAND,
PCI_COMMAND_INTX_DISABLE);
+
+ if (!__pci_reset_function_locked(dev)) {
+ vote = PCI_ERS_RESULT_RECOVERED;
+ pci_info(dev, "Recovered via pci level
reset\n");
+ }
+
+ pci_cfg_access_unlock(dev);
+ pci_restore_state(dev);
} else {
vote = PCI_ERS_RESULT_NONE;
--pk
> > C) After that AER errors [1] comes is only for device 0000:09:00.0.
> > This is strange as this pci device is not being used during test.
> > Ping/ssh are happening with 0000:09:01.0
> > D) If wait for some more time. No more AER errors from any device
> > E) Ping is working fine in case 2.
> >
> > 09:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network
> > Connection (rev 01)
> > 09:00.1 Ethernet controller: Intel Corporation 82576 Gigabit Network
> > Connection (rev 01)
> >
> > # lspci -t -v
> >
> > \-[0000:00]-+-00.0 Cavium, Inc. CN99xx [ThunderX2] Integrated PCI Host bridge
> > +-01.0-[01]--
> > +-02.0-[02]--
> > +-03.0-[03]--
> > +-04.0-[04]--
> > +-05.0-[05]--+-00.0 Broadcom Inc. and subsidiaries
> > BCM57840 NetXtreme II 10 Gigabit Ethernet
> > | \-00.1 Broadcom Inc. and subsidiaries
> > BCM57840 NetXtreme II 10 Gigabit Ethernet
> > +-06.0-[06]--
> > +-07.0-[07]--
> > +-08.0-[08]--
> > +-09.0-[09-0a]--+-00.0 Intel Corporation 82576 Gigabit
> > Network Connection
> > | \-00.1 Intel Corporation 82576 Gigabit
> > Network Connection
> >
> >
> > [1] AER error which comes for 09:00.0:
> >
> > [ 81.659825] {7}[Hardware Error]: Hardware error from APEI Generic
> > Hardware Error Source: 0
> > [ 81.668080] {7}[Hardware Error]: It has been corrected by h/w and
> > requires no further action
> > [ 81.676503] {7}[Hardware Error]: event severity: corrected
> > [ 81.681975] {7}[Hardware Error]: Error 0, type: corrected
> > [ 81.687447] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.693004] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 81.698908] {7}[Hardware Error]: version: 3.0
> > [ 81.703424] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 81.709589] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 81.715059] {7}[Hardware Error]: slot: 0
> > [ 81.719141] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 81.724265] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 81.730864] {7}[Hardware Error]: class_code: 000002
> > [ 81.735901] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 81.742587] {7}[Hardware Error]: Error 1, type: corrected
> > [ 81.748058] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.753615] {7}[Hardware Error]: port_type: 4, root port
> > [ 81.759086] {7}[Hardware Error]: version: 3.0
> > [ 81.763602] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 81.769767] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 81.775237] {7}[Hardware Error]: slot: 0
> > [ 81.779319] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 81.784442] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 81.791041] {7}[Hardware Error]: class_code: 000406
> > [ 81.796078] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 81.803806] {7}[Hardware Error]: Error 2, type: corrected
> > [ 81.809276] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.814834] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 81.820738] {7}[Hardware Error]: version: 3.0
> > [ 81.825254] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 81.831419] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 81.836889] {7}[Hardware Error]: slot: 0
> > [ 81.840971] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 81.846094] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 81.852693] {7}[Hardware Error]: class_code: 000002
> > [ 81.857730] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 81.864416] {7}[Hardware Error]: Error 3, type: corrected
> > [ 81.869886] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.875444] {7}[Hardware Error]: port_type: 4, root port
> > [ 81.880914] {7}[Hardware Error]: version: 3.0
> > [ 81.885430] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 81.891595] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 81.897066] {7}[Hardware Error]: slot: 0
> > [ 81.901147] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 81.906271] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 81.912870] {7}[Hardware Error]: class_code: 000406
> > [ 81.917906] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 81.925634] {7}[Hardware Error]: Error 4, type: corrected
> > [ 81.931104] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.936662] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 81.942566] {7}[Hardware Error]: version: 3.0
> > [ 81.947082] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 81.953247] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 81.958717] {7}[Hardware Error]: slot: 0
> > [ 81.962799] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 81.967923] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 81.974522] {7}[Hardware Error]: class_code: 000002
> > [ 81.979558] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 81.986244] {7}[Hardware Error]: Error 5, type: corrected
> > [ 81.991715] {7}[Hardware Error]: section_type: PCIe error
> > [ 81.997272] {7}[Hardware Error]: port_type: 4, root port
> > [ 82.002743] {7}[Hardware Error]: version: 3.0
> > [ 82.007259] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 82.013424] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 82.018894] {7}[Hardware Error]: slot: 0
> > [ 82.022976] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 82.028099] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 82.034698] {7}[Hardware Error]: class_code: 000406
> > [ 82.039735] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 82.047463] {7}[Hardware Error]: Error 6, type: corrected
> > [ 82.052933] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.058491] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 82.064395] {7}[Hardware Error]: version: 3.0
> > [ 82.068911] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 82.075076] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 82.080547] {7}[Hardware Error]: slot: 0
> > [ 82.084628] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 82.089752] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 82.096351] {7}[Hardware Error]: class_code: 000002
> > [ 82.101387] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 82.108073] {7}[Hardware Error]: Error 7, type: corrected
> > [ 82.113544] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.119101] {7}[Hardware Error]: port_type: 4, root port
> > [ 82.124572] {7}[Hardware Error]: version: 3.0
> > [ 82.129087] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 82.135252] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 82.140723] {7}[Hardware Error]: slot: 0
> > [ 82.144805] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 82.149928] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 82.156527] {7}[Hardware Error]: class_code: 000406
> > [ 82.161564] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 82.169291] {7}[Hardware Error]: Error 8, type: corrected
> > [ 82.174762] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.180319] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 82.186224] {7}[Hardware Error]: version: 3.0
> > [ 82.190739] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 82.196904] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 82.202375] {7}[Hardware Error]: slot: 0
> > [ 82.206456] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 82.211580] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 82.218179] {7}[Hardware Error]: class_code: 000002
> > [ 82.223216] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 82.229901] {7}[Hardware Error]: Error 9, type: corrected
> > [ 82.235372] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.240929] {7}[Hardware Error]: port_type: 4, root port
> > [ 82.246400] {7}[Hardware Error]: version: 3.0
> > [ 82.250916] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 82.257081] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 82.262551] {7}[Hardware Error]: slot: 0
> > [ 82.266633] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 82.271756] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 82.278355] {7}[Hardware Error]: class_code: 000406
> > [ 82.283392] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 82.291119] {7}[Hardware Error]: Error 10, type: corrected
> > [ 82.296676] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.302234] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 82.308138] {7}[Hardware Error]: version: 3.0
> > [ 82.312654] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 82.318819] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 82.324290] {7}[Hardware Error]: slot: 0
> > [ 82.328371] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 82.333495] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 82.340094] {7}[Hardware Error]: class_code: 000002
> > [ 82.345131] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 82.351816] {7}[Hardware Error]: Error 11, type: corrected
> > [ 82.357374] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.362931] {7}[Hardware Error]: port_type: 4, root port
> > [ 82.368402] {7}[Hardware Error]: version: 3.0
> > [ 82.372917] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 82.379082] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 82.384553] {7}[Hardware Error]: slot: 0
> > [ 82.388635] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 82.393758] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 82.400357] {7}[Hardware Error]: class_code: 000406
> > [ 82.405394] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 82.413121] {7}[Hardware Error]: Error 12, type: corrected
> > [ 82.418678] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.424236] {7}[Hardware Error]: port_type: 0, PCIe end point
> > [ 82.430140] {7}[Hardware Error]: version: 3.0
> > [ 82.434656] {7}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 82.440821] {7}[Hardware Error]: device_id: 0000:09:00.0
> > [ 82.446291] {7}[Hardware Error]: slot: 0
> > [ 82.450373] {7}[Hardware Error]: secondary_bus: 0x00
> > [ 82.455497] {7}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 82.462096] {7}[Hardware Error]: class_code: 000002
> > [ 82.467132] {7}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 82.473818] {7}[Hardware Error]: Error 13, type: corrected
> > [ 82.479375] {7}[Hardware Error]: section_type: PCIe error
> > [ 82.484933] {7}[Hardware Error]: port_type: 4, root port
> > [ 82.490403] {7}[Hardware Error]: version: 3.0
> > [ 82.494919] {7}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 82.501084] {7}[Hardware Error]: device_id: 0000:00:09.0
> > [ 82.506555] {7}[Hardware Error]: slot: 0
> > [ 82.510636] {7}[Hardware Error]: secondary_bus: 0x09
> > [ 82.515760] {7}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 82.522359] {7}[Hardware Error]: class_code: 000406
> > [ 82.527395] {7}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 82.535171] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.542476] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.550301] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.558032] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.566296] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.573597] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.581421] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.589151] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.597411] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.604711] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.612535] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.620271] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.628525] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.635826] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.643649] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.651385] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.659645] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.666940] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.674763] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.682498] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.690759] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.698053] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.705876] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.713612] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 82.721872] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 82.729167] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 82.736990] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 82.744725] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 88.059225] {8}[Hardware Error]: Hardware error from APEI Generic
> > Hardware Error Source: 0
> > [ 88.067478] {8}[Hardware Error]: It has been corrected by h/w and
> > requires no further action
> > [ 88.075899] {8}[Hardware Error]: event severity: corrected
> > [ 88.081370] {8}[Hardware Error]: Error 0, type: corrected
> > [ 88.086841] {8}[Hardware Error]: section_type: PCIe error
> > [ 88.092399] {8}[Hardware Error]: port_type: 0, PCIe end point
> > [ 88.098303] {8}[Hardware Error]: version: 3.0
> > [ 88.102819] {8}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 88.108984] {8}[Hardware Error]: device_id: 0000:09:00.0
> > [ 88.114455] {8}[Hardware Error]: slot: 0
> > [ 88.118536] {8}[Hardware Error]: secondary_bus: 0x00
> > [ 88.123660] {8}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 88.130259] {8}[Hardware Error]: class_code: 000002
> > [ 88.135296] {8}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 88.141981] {8}[Hardware Error]: Error 1, type: corrected
> > [ 88.147452] {8}[Hardware Error]: section_type: PCIe error
> > [ 88.153009] {8}[Hardware Error]: port_type: 4, root port
> > [ 88.158480] {8}[Hardware Error]: version: 3.0
> > [ 88.162995] {8}[Hardware Error]: command: 0x0106, status: 0x4010
> > [ 88.169161] {8}[Hardware Error]: device_id: 0000:00:09.0
> > [ 88.174633] {8}[Hardware Error]: slot: 0
> > [ 88.180018] {8}[Hardware Error]: secondary_bus: 0x09
> > [ 88.185142] {8}[Hardware Error]: vendor_id: 0x177d, device_id: 0xaf84
> > [ 88.191914] {8}[Hardware Error]: class_code: 000406
> > [ 88.196951] {8}[Hardware Error]: bridge: secondary_status:
> > 0x6000, control: 0x0002
> > [ 88.204852] {8}[Hardware Error]: Error 2, type: corrected
> > [ 88.210323] {8}[Hardware Error]: section_type: PCIe error
> > [ 88.215881] {8}[Hardware Error]: port_type: 0, PCIe end point
> > [ 88.221786] {8}[Hardware Error]: version: 3.0
> > [ 88.226301] {8}[Hardware Error]: command: 0x0507, status: 0x0010
> > [ 88.232466] {8}[Hardware Error]: device_id: 0000:09:00.0
> > [ 88.237937] {8}[Hardware Error]: slot: 0
> > [ 88.242019] {8}[Hardware Error]: secondary_bus: 0x00
> > [ 88.247142] {8}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > [ 88.253741] {8}[Hardware Error]: class_code: 000002
> > [ 88.258778] {8}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > [ 88.265509] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 88.272812] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> > [ 88.280635] pcieport 0000:00:09.0: AER: aer_status: 0x00000000,
> > aer_mask: 0x00002000
> > [ 88.288363] pcieport 0000:00:09.0: AER: aer_layer=Transaction
> > Layer, aer_agent=Receiver ID
> > [ 88.296622] igb 0000:09:00.0: AER: aer_status: 0x00002000,
> > aer_mask: 0x00002000
> > [ 88.305391] igb 0000:09:00.0: AER: aer_layer=Transaction Layer,
> > aer_agent=Receiver ID
> >
> > > Case I is using APEI, and it looks like that can queue up 16 errors
> > > (AER_RECOVER_RING_SIZE), so that queue could be completely full before
> > > we even get a chance to reset the device. But I would think that the
> > > reset should *eventually* stop the errors, even though we might log
> > > 30+ of them first.
> > >
> > > As an experiment, you could reduce AER_RECOVER_RING_SIZE to 1 or 2 and
> > > see if it reduces the logging.
> >
> > Did not tried this experiment. I believe it is not required now
> >
> > --pk
> >
> > >
> > > > > > Problem mentioned in case I and II goes away if do pci_reset_function
> > > > > > during enumeration phase of kdump kernel.
> > > > > > can we thought of doing pci_reset_function for all devices in kdump
> > > > > > kernel or device specific quirk.
> > > > > >
> > > > > > --pk
> > > > > >
> > > > > >
> > > > > > > > As per my understanding, possible solutions are
> > > > > > > > - Copy SMMU table i.e. this patch
> > > > > > > > OR
> > > > > > > > - Doing pci_reset_function() during enumeration phase.
> > > > > > > > I also tried clearing "M" bit using pci_clear_master during
> > > > > > > > enumeration but it did not help. Because driver re-set M bit causing
> > > > > > > > same AER error again.
> > > > > > > >
> > > > > > > >
> > > > > > > > -pk
> > > > > > > >
> > > > > > > > ---------------------------------------------------------------------------------------------------------------------------
> > > > > > > > [1] with bootargs having pci=noaer
> > > > > > > >
> > > > > > > > [ 22.494648] {4}[Hardware Error]: Hardware error from APEI Generic
> > > > > > > > Hardware Error Source: 1
> > > > > > > > [ 22.512773] {4}[Hardware Error]: event severity: recoverable
> > > > > > > > [ 22.518419] {4}[Hardware Error]: Error 0, type: recoverable
> > > > > > > > [ 22.544804] {4}[Hardware Error]: section_type: PCIe error
> > > > > > > > [ 22.550363] {4}[Hardware Error]: port_type: 0, PCIe end point
> > > > > > > > [ 22.556268] {4}[Hardware Error]: version: 3.0
> > > > > > > > [ 22.560785] {4}[Hardware Error]: command: 0x0507, status: 0x4010
> > > > > > > > [ 22.576852] {4}[Hardware Error]: device_id: 0000:09:00.1
> > > > > > > > [ 22.582323] {4}[Hardware Error]: slot: 0
> > > > > > > > [ 22.586406] {4}[Hardware Error]: secondary_bus: 0x00
> > > > > > > > [ 22.591530] {4}[Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9
> > > > > > > > [ 22.608900] {4}[Hardware Error]: class_code: 000002
> > > > > > > > [ 22.613938] {4}[Hardware Error]: serial number: 0xff1b4580, 0x90e2baff
> > > > > > > > [ 22.803534] pci 0000:09:00.1: AER: aer_status: 0x00004000,
> > > > > > > > aer_mask: 0x00000000
> > > > > > > > [ 22.810838] pci 0000:09:00.1: AER: [14] CmpltTO (First)
> > > > > > > > [ 22.817613] pci 0000:09:00.1: AER: aer_layer=Transaction Layer,
> > > > > > > > aer_agent=Requester ID
> > > > > > > > [ 22.847374] pci 0000:09:00.1: AER: aer_uncor_severity: 0x00062011
> > > > > > > > [ 22.866161] mpt3sas_cm0: 63 BIT PCI BUS DMA ADDRESSING SUPPORTED,
> > > > > > > > total mem (8153768 kB)
> > > > > > > > [ 22.946178] pci 0000:09:00.0: AER: can't recover (no error_detected callback)
> > > > > > > > [ 22.995142] pci 0000:09:00.1: AER: can't recover (no error_detected callback)
> > > > > > > > [ 23.002300] pcieport 0000:00:09.0: AER: device recovery failed
> > > > > > > > [ 23.027607] pci 0000:09:00.1: AER: aer_status: 0x00004000,
> > > > > > > > aer_mask: 0x00000000
> > > > > > > > [ 23.044109] pci 0000:09:00.1: AER: [14] CmpltTO (First)
> > > > > > > > [ 23.060713] pci 0000:09:00.1: AER: aer_layer=Transaction Layer,
> > > > > > > > aer_agent=Requester ID
> > > > > > > > [ 23.068616] pci 0000:09:00.1: AER: aer_uncor_severity: 0x00062011
> > > > > > > > [ 23.122056] pci 0000:09:00.0: AER: can't recover (no error_detected callback)
> > >
> > > <snip>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 0/2] firmware/psci: PSCI checker cleanup
From: Valentin Schneider @ 2020-06-03 17:39 UTC (permalink / raw)
To: Sudeep Holla
Cc: Mark Rutland, Peter Zijlstra, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel
In-Reply-To: <20200603170511.GA23722@bogus>
On 03/06/20 18:05, Sudeep Holla wrote:
> On Fri, Apr 24, 2020 at 02:56:55PM +0100, Valentin Schneider wrote:
>> Hi folks,
>>
>> This is a small cleanup of the PSCI checker following Peter's objections
>> to its homegrown do_idle() implementation. It is based on his
>> sched_setscheduler() unexport series at [1].
>>
>> I've never really used the thing before, but it still seems to behave
>> correctly on my Juno r0 & HiKey960.
>>
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Thanks!
AIUI the plan is to have the base in for the following version, so we
can wait until then - or I can rebase this on top of mainline, and
whoever will be on the receiving end of the merge conflict will be
slightly annoyed :-)
I'm in no particular rush, and this isn't very hot code, so up to you.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCHv2 0/7] Support inhibiting input devices
From: Hans de Goede @ 2020-06-03 17:38 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, Dmitry Torokhov
Cc: Nick Dyer, linux-iio, Benjamin Tissoires, platform-driver-x86,
ibm-acpi-devel, Laxman Dewangan, Peter Meerwald-Stadler, kernel,
Fabio Estevam, linux-samsung-soc, Krzysztof Kozlowski,
Jonathan Hunter, linux-acpi, Kukjin Kim, NXP Linux Team,
linux-input, Len Brown, Peter Hutterer, Michael Hennerich,
Sascha Hauer, Sylvain Lemieux, Henrique de Moraes Holschuh,
Vladimir Zapolskiy, Lars-Peter Clausen, linux-tegra,
linux-arm-kernel, Barry Song, Ferruh Yigit, patches,
Rafael J . Wysocki, Thierry Reding, Sangwon Jee,
Pengutronix Kernel Team, Hartmut Knaack, Shawn Guo,
Jonathan Cameron
In-Reply-To: <fb5bee72-6a75-88aa-8157-75f07c491eeb@collabora.com>
Hi,
On 6/3/20 3:07 PM, Andrzej Pietrasiewicz wrote:
> Hi Hans, hi Dmitry,
<snip>
> I'm taking one step back and looking at the ->open() and ->close()
> driver callbacks. They are called from input_open_device() and
> input_close_device(), respectively:
>
> input_open_device():
> "This function should be called by input handlers when they
> want to start receive events from given input device."
>
> ->open() callback:
> "this method is called when the very first user calls
> input_open_device(). The driver must prepare the device to start
> generating events (start polling thread, request an IRQ, submit
> URB, etc.)"
>
> input_close_device():
> "This function should be called by input handlers when they
> want to stop receive events from given input device."
>
> ->close() callback:
> "this method is called when the very last user calls
> input_close_device()"
>
> It seems to me that the callback names do not reflect their
> purpose: their meaning is not to "open" or to "close" but to
> give drivers a chance to control when they start or stop
> providing events to the input core.
>
> What would you say about changing the callbacks' names?
> I'd envsion: ->provide_events() instead of ->open() and
> ->stop_events() instead of ->close(). Of course drivers can
> exploit the fact of knowing that nobody wants any events
> from them and do whatever they consider appropriate, for
> example go into a low power mode - but the latter is beyond
> the scope of the input subsystem and is driver-specific.
I don't have much of an opinion on changing the names,
to me open/close have always means start/stop receiving
events. This follows the everything is a file philosophy,
e.g. you can also not really "open" a serial port,
yet opening /dev/ttyS0 will activate the receive IRQ
of the UART, etc. So maybe we just need to make the
docs clearer rather then do the rename? Doing the
rename is certainly going to cause a lot of churn.
Anyways as said, I don't have much of an opinion,
so I'll leave commenting (more) on this to Dmitry.
> With such a naming change in mind let's consider inhibiting.
> We want to be able to control when to disregard events from
> a given device. It makes sense to do it at device level, otherwise
> such an operation would have to be invoked in all associated
> handlers (those that have an open handle associating them with
> the device in question). But of course we can do better than
> merely ignoring the events received: we can tell the drivers
> that we don't want any events from them, and later, at uninhibit
> time, tell them to start providing the events again. Conceptually,
> the two operations (provide or don't provide envents) are exactly
> the same thing we want to be happening at input_open_device() and
> input_close_device() time. To me, changing the names of
> ->open() and ->close() exposes this fact very well.
>
> Consequently, ->inhibit() and ->uninhibit() won't be needed,
> and drivers which already implement ->provide_events() (formerly
> ->open()) and ->stop_events() (formerly ->close()) will receive
> full inhibit/uninhibit support for free (subject to how well they
> implement ->provide_events()/->stop_events()). Unless we can come
> up with what the drivers might be doing on top of ->stop_events()
> and ->provide_events() when inhibiting/uninhibiting, but it seems
> to me we can't. Can we?
Right. I'm happy that you've come to see that both on open/close
and on inhibit/uninhibit we want to "start receiving events" and
"stop receiving events", so that we only need one set of callbacks.
> Optionally ->close() (only the callback, not input_close_device())
> can be made return a value, just as Hans suggests. The value
> can be ignored in input_close_device() but used in input_inhibit().
> No strong opinion here, though. (btw it seems to me that
> input_inhibit() should be renamed to input_inhibit_device()).
Ack.
Regards,
Hans
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3] usb: host: xhci-mtk: avoid runtime suspend when removing hcd
From: Sergei Shtylyov @ 2020-06-03 17:33 UTC (permalink / raw)
To: Macpaul Lin, Chunfeng Yun, Mathias Nyman, Greg Kroah-Hartman,
Matthias Brugger, stable
Cc: Mediatek WSD Upstream, linux-usb, linux-kernel, linux-mediatek,
Macpaul Lin, linux-arm-kernel
In-Reply-To: <1591189767-21988-1-git-send-email-macpaul.lin@mediatek.com>
Hello.
On 03.06.2020 16:09, Macpaul Lin wrote:
> When runtime suspend was enabled, runtime suspend might happened
Happen.
> when xhci is removing hcd. This might cause kernel panic when hcd
> has been freed but runtime pm suspend related handle need to
> reference it.
>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> Reviewed-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
[...]
MBR, Sergei
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 070/105] drm/vc4: hdmi: rework connectors and encoders
From: Stefan Wahren @ 2020-06-03 17:32 UTC (permalink / raw)
To: Maxime Ripard, Eric Anholt
Cc: Tim Gover, Dave Stevenson, linux-kernel, DRI Development,
bcm-kernel-feedback-list, linux-arm-kernel, Phil Elwell,
Nicolas Saenz Julienne, linux-rpi-kernel
In-Reply-To: <20200602155421.niyvpwqc42xh5c7v@gilmour>
Am 02.06.20 um 17:54 schrieb Maxime Ripard:
> On Wed, May 27, 2020 at 11:41:24AM -0700, Eric Anholt wrote:
>> On Wed, May 27, 2020 at 8:51 AM Maxime Ripard <maxime@cerno.tech> wrote:
>>> the vc4_hdmi driver has some custom structures to hold the data it needs to
>>> associate with the drm_encoder and drm_connector structures.
>>>
>>> However, it allocates them separately from the vc4_hdmi structure which
>>> makes it more complicated than it needs to be.
>>>
>>> Move those structures to be contained by vc4_hdmi and update the code
>>> accordingly.
>>
>>> @@ -1220,7 +1219,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>> struct drm_device *drm = dev_get_drvdata(master);
>>> struct vc4_dev *vc4 = drm->dev_private;
>>> struct vc4_hdmi *hdmi;
>>> - struct vc4_hdmi_encoder *vc4_hdmi_encoder;
>>> + struct drm_encoder *encoder;
>>> struct device_node *ddc_node;
>>> u32 value;
>>> int ret;
>>> @@ -1229,14 +1228,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>>> if (!hdmi)
>>> return -ENOMEM;
>>>
>>> - vc4_hdmi_encoder = devm_kzalloc(dev, sizeof(*vc4_hdmi_encoder),
>>> - GFP_KERNEL);
>>> - if (!vc4_hdmi_encoder)
>>> - return -ENOMEM;
>>> - vc4_hdmi_encoder->base.type = VC4_ENCODER_TYPE_HDMI0;
>>> - hdmi->encoder = &vc4_hdmi_encoder->base.base;
>>> -
>>> hdmi->pdev = pdev;
>>> + encoder = &hdmi->encoder.base.base;
>>> + encoder->base.type = VC4_ENCODER_TYPE_HDMI0;
>> Wait, does this patch build?
> All those patches were build tested, so yep
>
>> setting struct drm_encoder->base.type = VC4_* seems very wrong, when
>> previously we were setting struct vc4_hdmi_encoder->base.type (struct
>> vc4_encoder->type).
> So the structure layout now is that vc4_hdmi embeds vc4_hdmi_encoder as
> encoder. So &hdmi->encoder is a pointer to vc4_hdmi_encoder.
> vc4_hdmi_encoder's base is since that patch a struct vc4_encoder. and
> vc4_encoder's base is a drm_encoder.
>
> so encoder being a drm_encoder is correct there.
>
> However, drm_encoder's base is drm_mode_object that does have a type
> field, which is an uint32_t, which will accept a VC4_ENCODER_TYPE_* just
> fine...
>
> Now, drm_encoder_init will then kick in and call drm_mode_object_add
> which will override it to a proper value and since the clock select bit
> in the PV is the same for both HDMI0 and HDMI1, everything works just
> fine...
>
> Good catch, I'll fix it. And I guess it's a good indication we don't
> need a separate HDMI0 and HDMI1 encoder type.
>
FWIW this is the first patch which breaks X on my Raspberry Pi 3 B.
Here are the bisect results:
587d6e4a529a8d807a5c0bae583dd432d77064d6 bad (black screen, no heartbeat)
b0523c7b1c9d0edcd6c0fe6d2cb558a9ad5c60a8 good
2c6a651cac6359cb0244a40d3b7a14e72918f169 good
1705c3cb40906863ec0d24ee5ea5092f5ee2e994 bad (black screen, but heartbeat)
601527fea6bb226abd088a864e74b25368218e87 good
2165607ede34d229d0cbce916c70c7fb6c0337be good
f094f388fc2df848227e2ae648df2c97872df42b good
020de18840a1075b2671736c6cc2e451030fad74 bad (black screen, but heartbeat)
4c4da3823e4d1a8189e96a59a79451fff372f70b good
020de18840a1075b2671736c6cc2e451030fad74 is the first bad commit
commit 020de18840a1075b2671736c6cc2e451030fad74
Author: Maxime Ripard <maxime@cerno.tech>
Date: Mon Jan 6 17:17:29 2020 +0100
drm/vc4: hdmi: rework connectors and encoders
the vc4_hdmi driver has some custom structures to hold the data it
needs to
associate with the drm_encoder and drm_connector structures.
However, it allocates them separately from the vc4_hdmi structure which
makes it more complicated than it needs to be.
Move those structures to be contained by vc4_hdmi and update the code
accordingly.
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 01/10] dmaengine: Actions: get rid of bit fields from dma descriptor
From: Amit Tomer @ 2020-06-03 17:28 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Andre Przywara, linux-actions, linux-kernel, cristian.ciocaltea,
Vinod Koul, dmaengine, dan.j.williams, Andreas Färber,
linux-arm-kernel
In-Reply-To: <3D3E2940-11E3-4093-8F60-82EB2C11B617@linaro.org>
Hi,
Thanks for having a look.
On Wed, Jun 3, 2020 at 12:52 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
> Individual comments for these enums?
I was expecting this comment , and thought these fields are self explanatory
But if you prefer to have description about it, I would have it in next version.
> >+enum owl_dmadesc_offsets {
> >+ OWL_DMADESC_NEXT_LLI = 0,
> >+ OWL_DMADESC_SADDR,
> >+ OWL_DMADESC_DADDR,
> >+ OWL_DMADESC_FLEN,
> >+ OWL_DMADESC_SRC_STRIDE,
> >+ OWL_DMADESC_DST_STRIDE,
> >+ OWL_DMADESC_CTRLA,
> >+ OWL_DMADESC_CTRLB,
> >+ OWL_DMADESC_CONST_NUM,
> >+ OWL_DMADESC_SIZE
> > };
> >
> > /**
> >@@ -153,7 +144,7 @@ struct owl_dma_lli_hw {
> > * @node: node for txd's lli_list
> > */
> > struct owl_dma_lli {
> >- struct owl_dma_lli_hw hw;
> >+ u32 hw[OWL_DMADESC_SIZE];
> > dma_addr_t phys;
> > struct list_head node;
> > };
> >@@ -320,6 +311,11 @@ static inline u32 llc_hw_ctrlb(u32 int_ctl)
> > return ctl;
> > }
> >
> >+static u32 llc_hw_flen(struct owl_dma_lli *lli)
> >+{
> >+ return lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
> >+}
> >+
> > static void owl_dma_free_lli(struct owl_dma *od,
> > struct owl_dma_lli *lli)
> > {
> >@@ -351,8 +347,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct
> >owl_dma_txd *txd,
> > list_add_tail(&next->node, &txd->lli_list);
> >
> > if (prev) {
> >- prev->hw.next_lli = next->phys;
> >- prev->hw.ctrla |= llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> >+ prev->hw[OWL_DMADESC_NEXT_LLI] = next->phys;
> >+ prev->hw[OWL_DMADESC_CTRLA] |=
> >+ llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> > }
> >
> > return next;
> >@@ -365,8 +362,7 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> > struct dma_slave_config *sconfig,
> > bool is_cyclic)
> > {
> >- struct owl_dma_lli_hw *hw = &lli->hw;
> >- u32 mode;
> >+ u32 mode, ctrlb;
> >
> > mode = OWL_DMA_MODE_PW(0);
> >
> >@@ -407,22 +403,22 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> > return -EINVAL;
> > }
> >
> >- hw->next_lli = 0; /* One link list by default */
> >- hw->saddr = src;
> >- hw->daddr = dst;
> >-
> >- hw->fcnt = 1; /* Frame count fixed as 1 */
> >- hw->flen = len; /* Max frame length is 1MB */
> >- hw->src_stride = 0;
> >- hw->dst_stride = 0;
> >- hw->ctrla = llc_hw_ctrla(mode,
> >- OWL_DMA_LLC_SAV_LOAD_NEXT |
> >- OWL_DMA_LLC_DAV_LOAD_NEXT);
> >+ lli->hw[OWL_DMADESC_CTRLA] = llc_hw_ctrla(mode,
> >+ OWL_DMA_LLC_SAV_LOAD_NEXT |
> >+ OWL_DMA_LLC_DAV_LOAD_NEXT);
> >
> > if (is_cyclic)
> >- hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> >+ ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> > else
> >- hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+ ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+
> >+ lli->hw[OWL_DMADESC_NEXT_LLI] = 0;
>
> Again, please preserve the old comments.
Sure, would do it.
>
> >+ lli->hw[OWL_DMADESC_SADDR] = src;
> >+ lli->hw[OWL_DMADESC_DADDR] = dst;
> >+ lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >+ lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >+ lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
>
> Please explain what you're doing here.
Actually , in next the patch 2/10 there is comment that explains a bit
about it.
/*
* S700 put flen and fcnt at offset 0x0c and 0x1c respectively,
* whereas S900 put flen and fcnt at offset 0x0c.
*/
Shall I add more details to it in the next patch 02/10 ?
Thanks
-Amit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Bjorn Andersson @ 2020-06-03 17:23 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arm-msm, Joerg Roedel, iommu, Thierry Reding, John Stultz,
linux-tegra, Robin Murphy, linux-arm-kernel
In-Reply-To: <20200603111159.GA8408@willie-the-truck>
On Wed 03 Jun 04:11 PDT 2020, Will Deacon wrote:
> On Mon, Jun 01, 2020 at 11:32:10PM -0700, Bjorn Andersson wrote:
> > On Wed 27 May 04:03 PDT 2020, Will Deacon wrote:
> >
> > > Hi John, Bjorn,
> > >
> > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson@linaro.org> wrote:
> > > > >
> > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > > >
> > > > > Rob, Will, we're reaching the point where upstream has enough
> > > > > functionality that this is becoming a critical issue for us.
> > > > >
> > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > > similar on several devboards.
> > > > >
> > > > > As previously described, the only thing I want is the stream mapping
> > > > > related to the display controller in place, either with the CB with
> > > > > translation disabled or possibly with a way to specify the framebuffer
> > > > > region (although this turns out to mess things up in the display
> > > > > driver...)
> > > > >
> > > > > I did pick this up again recently and concluded that by omitting the
> > > > > streams for the USB controllers causes an instability issue seen on one
> > > > > of the controller to disappear. So I would prefer if we somehow could
> > > > > have a mechanism to only pick the display streams and the context
> > > > > allocation for this.
> > > > >
> > > > >
> > > > > Can you please share some pointers/insights/wishes for how we can
> > > > > conclude on this subject?
> > > >
> > > > Ping? I just wanted to follow up on this discussion as this small
> > > > series is crucial for booting mainline on the Dragonboard 845c
> > > > devboard. It would be really valuable to be able to get some solution
> > > > upstream so we can test mainline w/o adding additional patches.
> > >
> > > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > > about this on top of everything else. We're also carrying a hack in Android
> > > for you :)
> > >
> >
> > Thanks for taking the time to get back to us on this!
> >
> > > > The rest of the db845c series has been moving forward smoothly, but
> > > > this set seems to be very stuck with no visible progress since Dec.
> > > >
> > > > Are there any pointers for what folks would prefer to see?
> > >
> > > I've had a chat with Robin about this. Originally, I was hoping that
> > > people would all work together towards an idyllic future where firmware
> > > would be able to describe arbitrary pre-existing mappings for devices,
> > > irrespective of the IOMMU through which they master and Linux could
> > > inherit this configuration. However, that hasn't materialised (there was
> > > supposed to be an IORT update, but I don't know what happened to that)
> > > and, in actual fact, the problem that you have on db845 is /far/ more
> > > restricted than the general problem.
> > >
> > > Could you please try hacking something along the following lines and see
> > > how you get on? You may need my for-joerg/arm-smmu/updates branch for
> > > all the pieces:
> > >
> > > 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
> > > "pinning" and configure for bypass.
> > >
> > > 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
> > > for the display controller
> > >
> > > I /think/ that's sufficient, but note that it differs from the current
> > > approach because we don't end up reserving a CB -- bypass is configured
> > > in the S2CR instead. Some invalidation might therefore be needed in
> > > ->cfg_probe() after unhooking the CB.
> > >
> > > Thanks, and please yell if you run into problems with this approach.
> > >
> >
> > This sounded straight forward and cleaner, so I implemented it...
> >
> > Unfortunately the hypervisor is playing tricks on me when writing to
> > S2CR registers:
> > - TRANS writes lands as requested
> > - BYPASS writes ends up in the register as requested, with type FAULT
> > - FAULT writes are ignored
> >
> > In other words, the Qualcomm firmware prevents us from relying on
> > marking the relevant streams as BYPASS type.
>
> Is this for all S2CR registers, or only the ones in use by the display
> controller? Is there any scope for stopping the hypervisor from doing this?
>
This is for all S2CR registers. There's no chance to change this and get
it deployed on all the devices people care about.
As you know John need this for the RB3/db845c, where we might have a
chance to modify the firmware.
But I'm writing this on my Lenovo Yoga C630 and John and his colleagues
are working on various phones, such as Pixel 3. None of these boots
without this "workaround" and I don't expect that we can propagate a
firmware modification to any of them - and definitely not all of them.
> It makes it really difficult for the driver when the hardware is emulated
> in a way that doesn't match the architecture...
>
Understood.
Regards,
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Bjorn Andersson @ 2020-06-03 17:17 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-arm-msm, Joerg Roedel, Robin Murphy, iommu, John Stultz,
linux-tegra, Will Deacon, linux-arm-kernel, Laurentiu Tudor
In-Reply-To: <20200603102446.GA3478467@ulmo>
On Wed 03 Jun 03:24 PDT 2020, Thierry Reding wrote:
> On Tue, Jun 02, 2020 at 12:32:49PM -0700, Bjorn Andersson wrote:
> > On Tue 02 Jun 04:02 PDT 2020, Thierry Reding wrote:
> >
> > > On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote:
> > > > Hi John, Bjorn,
> > > >
> > > > On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > > > > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson@linaro.org> wrote:
> > > > > >
> > > > > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > > > > >
> > > > > > Rob, Will, we're reaching the point where upstream has enough
> > > > > > functionality that this is becoming a critical issue for us.
> > > > > >
> > > > > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > > > > mainline with display, GPU, WiFi and audio working and the story is
> > > > > > similar on several devboards.
> > > > > >
> > > > > > As previously described, the only thing I want is the stream mapping
> > > > > > related to the display controller in place, either with the CB with
> > > > > > translation disabled or possibly with a way to specify the framebuffer
> > > > > > region (although this turns out to mess things up in the display
> > > > > > driver...)
> > > > > >
> > > > > > I did pick this up again recently and concluded that by omitting the
> > > > > > streams for the USB controllers causes an instability issue seen on one
> > > > > > of the controller to disappear. So I would prefer if we somehow could
> > > > > > have a mechanism to only pick the display streams and the context
> > > > > > allocation for this.
> > > > > >
> > > > > >
> > > > > > Can you please share some pointers/insights/wishes for how we can
> > > > > > conclude on this subject?
> > > > >
> > > > > Ping? I just wanted to follow up on this discussion as this small
> > > > > series is crucial for booting mainline on the Dragonboard 845c
> > > > > devboard. It would be really valuable to be able to get some solution
> > > > > upstream so we can test mainline w/o adding additional patches.
> > > >
> > > > Sorry, it's been insanely busy recently and I haven't had a chance to think
> > > > about this on top of everything else. We're also carrying a hack in Android
> > > > for you :)
> > > >
> > > > > The rest of the db845c series has been moving forward smoothly, but
> > > > > this set seems to be very stuck with no visible progress since Dec.
> > > > >
> > > > > Are there any pointers for what folks would prefer to see?
> > > >
> > > > I've had a chat with Robin about this. Originally, I was hoping that
> > > > people would all work together towards an idyllic future where firmware
> > > > would be able to describe arbitrary pre-existing mappings for devices,
> > > > irrespective of the IOMMU through which they master and Linux could
> > > > inherit this configuration. However, that hasn't materialised (there was
> > > > supposed to be an IORT update, but I don't know what happened to that)
> > > > and, in actual fact, the problem that you have on db845 is /far/ more
> > > > restricted than the general problem.
> > >
> > > It doesn't sound to me like implementing platform-specific workarounds
> > > is a good long-term solution (especially since, according to Bjorn, they
> > > aren't as trivial to implement as it sounds). And we already have all
> > > the infrastructure in place to implement what you describe, so I don't
> > > see why we shouldn't do that. This patchset uses standard device tree
> > > bindings that were designed for exactly this kind of use-case.
> > >
> >
> > I think my results would imply that we would have to end up with (at
> > least) some special case of your proposal (i.e. we need a context bank
> > allocated).
>
> I wasn't talking about implementation details, but rather about the
> surrounding infrastructure. It seemed like Will was suggesting that
> there's no way of conveying what memory regions to direct-map from
> the firmware to the kernel. But that really isn't the problem here,
> is it? What we're really looking for is how to take what we have in
> device tree and use it in the ARM SMMU driver to create an early
> mapping that will stay in place until a device has been properly
> attached to the IOMMU domain.
>
I agree.
We do have the iommu properties from our display node and we typically
do have a reserved-memory region for the existing framebuffer that we
want to associate with those properties.
The one exception is on devices targeted to run Windows, where the
memory region is passed to the kernel using UEFI GOP.
But as you say, the information is available to the kernel already.
> > > So at least for device-tree based boot firmware can already describe
> > > these pre-existing mappings. If something standard materializes for ACPI
> > > eventually I'm sure we can find ways to integrate that into whatever we
> > > come up with now for DT.
> > >
> > > I think between Bjorn, John, Laurentiu and myself there's pretty broad
> > > consensus (correct me if I'm wrong, guys) that solving this via reserved
> > > memory regions is a good solution that works. So I think what's really
> > > missing is feedback on whether the changes proposed here or Laurentiu's
> > > updated proposal[0] are acceptable, and if not, what the preference is
> > > for getting something equivalent upstream.
> > >
> >
> > As described in my reply to your proposal, the one problem I ran into
> > was that I haven't figured out how to reliably "move" my display streams
> > from one mapping entry to another.
> >
> > With the current scheme I see that their will either be gaps in time
> > with no mapping for my display, or multiple mappings.
>
> I think you would inevitably end up with two mappings for a transitional
> period while you prepare the final mapping that you want to switch to.
>
I think that's fine, I saw that there's a bit to configure if multiple
SMR matches is considered a fault or not, so I will verify if this is
what causes the faults I saw on one of my platforms and if I can change
it.
I still would need that we ensure that we don't have lapses in time
where we're lacking mappings among the SMRs.
> > The other thing I noticed in your proposal was that I have a whole bunch
> > of DT nodes with both iommus and memory-region properties that I really
> > don't care to set up mappings for, but I've not finalized my thoughts on
> > this causing actual problems...
>
> Can you be more specific? It'd be useful to understand all of the
> existing uses of reserved memory regions in order to make sure we
> accomodate all of them.
>
I have reserved-memory regions for a number of co-processors, to
facilitate static access configuration, as such I ended up with having
100+MB of various DSP memory mapped into the temporary context.
> I'd be surprised, though, if setting up mappings for any of these
> regions would actually cause breakage. If a device tree node has a
> memory-region property it means that this memory region is eventually
> going to be used by the device and if the device tree node also has an
> iommus property it means that it's meant to use the IOMMU for
> translating memory accesses. It's therefore very likely that the memory
> region will need to be mapped. Whether it needs to be a direct mapping
> or not might be worth having a discussion about.
>
Right, I think that before any of these are actually used we will have a
device attached and the region properly mapped (as is done today
already).
> > > Just to highlight: the IOMMU framework already provides infrastructure
> > > to create direct mappings (via iommu_get_resv_regions(), called from
> > > iommu_create_device_direct_mappings()). I have patches that make use of
> > > this on Tegra210 and earlier where a non-ARM SMMU is used and where the
> > > IOMMU driver enables translations (and doesn't fault by default) only at
> > > device attachment time. That works perfectly using reserved-memory
> > > regions. Perhaps that infrastructure could be extended to cover the
> > > kinds of early mappings that we're discussing here. On the other hand it
> > > might be a bit premature at this point because the ARM SMMU is the only
> > > device that currently needs this, as far as I can tell.
> > >
> >
> > For Qualcomm we got patches picked up for 5.8 that will cause the
> > display controller to be attached with direct mapping, so I think all
> > missing now is the lack of stream mappings between arm-smmu probe and
> > display driver probe...
>
> Can you point me at those patches? I'd like to look at them and see if
> they match what I have planned for Tegra or if we can somehow converge
> on a common scheme.
>
You can find this in:
0e764a01015d ("iommu/arm-smmu: Allow client devices to select direct mapping")
Which conceptually follows what is done on x86 machines.
Regards,
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] mm/memblock: export max_pfn for kernel modules
From: Mike Rapoport @ 2020-06-03 17:06 UTC (permalink / raw)
To: Miles Chen
Cc: wsd_upstream, linux-kernel, linux-mm, linux-mediatek,
Matthias Brugger, Andrew Morton, linux-arm-kernel
In-Reply-To: <20200603161132.2441-1-miles.chen@mediatek.com>
On Thu, Jun 04, 2020 at 12:11:32AM +0800, Miles Chen wrote:
> max_pfn is uesd to get the highest pfn in the system. Drivers like
> drivers/iommu/mtk_iommu.c checks max_pfn to see if it should enable
> its "4GB mode".
>
> This patch exports the max_pfn symbol, so we can build the driver as
> a kernel module.
You can use totalram_pages(), like e.g.
drivers/media/platform/mtk-vpu/mtk_vpu.c:
$ git grep -np totalram_pages drivers/media/
drivers/media/platform/mtk-vpu/mtk_vpu.c=779=static int mtk_vpu_probe(struct platform_device *pdev)
drivers/media/platform/mtk-vpu/mtk_vpu.c:861: vpu->enable_4GB = !!(totalram_pages() > (SZ_2G >> PAGE_SHIFT));
> Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> ---
> mm/memblock.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index c79ba6f9920c..3b2b21ecebb6 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -99,6 +99,7 @@ EXPORT_SYMBOL(contig_page_data);
> unsigned long max_low_pfn;
> unsigned long min_low_pfn;
> unsigned long max_pfn;
> +EXPORT_SYMBOL(max_pfn);
> unsigned long long max_possible_pfn;
>
> static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> --
> 2.18.0
--
Sincerely yours,
Mike.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] scs: Report SCS usage in bytes rather than number of entries
From: Kees Cook @ 2020-06-03 17:06 UTC (permalink / raw)
To: Will Deacon; +Cc: Sami Tolvanen, linux-kernel, linux-arm-kernel
In-Reply-To: <20200603151218.11659-1-will@kernel.org>
On Wed, Jun 03, 2020 at 04:12:17PM +0100, Will Deacon wrote:
> Fix the SCS debug usage check so that we report the number of bytes
> usedm, rather than the number of entries.
typo: used
>
> Fixes: 5bbaf9d1fcb9 ("scs: Add support for stack usage debugging")
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/2] firmware/psci: PSCI checker cleanup
From: Sudeep Holla @ 2020-06-03 17:05 UTC (permalink / raw)
To: Valentin Schneider
Cc: Mark Rutland, Lorenzo Pieralisi, Peter Zijlstra, linux-kernel,
Sudeep Holla, linux-arm-kernel
In-Reply-To: <20200424135657.32519-1-valentin.schneider@arm.com>
On Fri, Apr 24, 2020 at 02:56:55PM +0100, Valentin Schneider wrote:
> Hi folks,
>
> This is a small cleanup of the PSCI checker following Peter's objections
> to its homegrown do_idle() implementation. It is based on his
> sched_setscheduler() unexport series at [1].
>
> I've never really used the thing before, but it still seems to behave
> correctly on my Juno r0 & HiKey960.
>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/2] arm64: Override SPSR.SS when single-stepping is enabled
From: Keno Fischer @ 2020-06-03 16:56 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Luis Machado, kernel-team, stable, linux-arm-kernel
In-Reply-To: <20200603155338.GA12036@willie-the-truck>
On Wed, Jun 3, 2020 at 11:53 AM Will Deacon <will@kernel.org> wrote:
> > However, at the same time as changing this, we should probably make sure
> > to enable the syscall exit pseudo-singlestep trap (similar issue as the other
> > patch I had sent for the signal pseudo-singlestep trap), since otherwise
> > ptracers might get confused about the lack of singlestep trap during a
> > singlestep -> seccomp -> singlestep path (which would give one trap
> > less with this patch than before).
>
> Hmm, I don't completely follow your example. Please could you spell it
> out a bit more? I fast-forward the stepping state machine on sigreturn,
> which I thought would be sufficient. Perhaps you're referring to a variant
> of the situation mentioned by Mark, which I didn't think could happen
> with ptrace [2].
Sure suppose we have code like the following:
0x0: svc #0
0x4: str x0, [x7]
...
Then, if there's a seccomp filter active that just does
SECCOMP_RET_TRACE of everything, right now we get traps:
<- (ip: 0x0)
-> PTRACE_SINGLESTEP
<- (ip: 0x4 - seccomp trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x4 - TRAP_TRACE trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x8 - TRAP_TRACE trap)
With your proposed patch, we instead get
<- (ip: 0x0)
-> PTRACE_SINGLESTEP
<- (ip: 0x4 - seccomp trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x8 - TRAP_TRACE trap)
This is problematic, because the ptracer may want to inspect the
result of the syscall instruction. On other architectures, this
problem is solved with a pseudo-singlestep trap that gets executed
if you resume from a syscall-entry-like trap with PTRACE_SINGLESTEP.
See the below patch for the change I'm proposing. There is a slight
issue with that patch, still: It now makes the x7 issue apply to the
singlestep trap at exit, so we should do the patch to fix that issue
before we apply that change (or manually check for this situation
and issue the pseudo-singlestep trap manually).
My proposed patch below also changes
<- (ip: 0x0)
-> PTRACE_SYSCALL
<- (ip: 0x4 - syscall entry trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x8 - TRAP_TRACE trap)
to
<- (ip: 0x0)
-> PTRACE_SYSCALL
<- (ip: 0x4 - syscall entry trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x4 - pseudo-singlestep exit trap)
-> PTRACE_SINGLESTEP
<- SIGTRAP (ip: 0x8 - TRAP_TRACE trap)
But I consider that a bugfix, since that's how other architectures
behave and I was going to send in this patch for that reason anyway
(since this was another one of the aarch64 ptrace quirks we had to
work around).
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index b3d3005d9515..104cfcf117d0 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1820,7 +1820,7 @@ static void tracehook_report_syscall(struct pt_regs *regs,
regs->regs[regno] = dir;
if (dir == PTRACE_SYSCALL_EXIT)
- tracehook_report_syscall_exit(regs, 0);
+ tracehook_report_syscall_exit(regs,
test_thread_flag(TIF_SINGLESTEP));
else if (tracehook_report_syscall_entry(regs))
forget_syscall(regs);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
From: Lukasz Luba @ 2020-06-03 16:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Nishanth Menon, Juri Lelli, Peter Zijlstra, Viresh Kumar,
Liviu Dudau, dri-devel, Bjorn Andersson, Benjamin Segall,
alyssa.rosenzweig, Fabio Estevam, Matthias Kaehlcke, Rob Herring,
Amit Kucheria, Lorenzo Pieralisi, Kevin Hilman, Andy Gross,
Daniel Lezcano, steven.price, Chanwoo Choi, Ingo Molnar,
dl-linux-imx, Zhang, Rui, Mel Gorman, orjan.eide, Daniel Vetter,
Linux PM, linux-arm-msm, Sascha Hauer, Steven Rostedt,
moderated list:ARM/Mediatek SoC..., Matthias Brugger,
Linux OMAP Mailing List, Dietmar Eggemann, Linux ARM,
David Airlie, Tomeu Vizoso, Quentin Perret, Stephen Boyd,
Randy Dunlap, Rafael J. Wysocki, Linux Kernel Mailing List,
Bartlomiej Zolnierkiewicz, Sascha Hauer, Sudeep Holla,
patrick.bellasi, Shawn Guo
In-Reply-To: <CAJZ5v0jL0+TXDGXaO=WfYg6QM3=B83LLZ90xtc2HtX70jdoiYQ@mail.gmail.com>
On 6/3/20 5:22 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 3, 2020 at 6:12 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 6/3/20 4:40 PM, Rafael J. Wysocki wrote:
>>> On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote:
>>>>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>>>
>>>>>> Hi Daniel,
>>>>>>
>>>>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote:
>>>>>>> On 27/05/2020 11:58, Lukasz Luba wrote:
>>>>>>>> Add support for other devices than CPUs. The registration function
>>>>>>>> does not require a valid cpumask pointer and is ready to handle new
>>>>>>>> devices. Some of the internal structures has been reorganized in order to
>>>>>>>> keep consistent view (like removing per_cpu pd pointers).
>>>>>>>>
>>>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> [ ... ]
>>>>>>>
>>>>>>>> }
>>>>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
>>>>>>>> + * @dev : Device for which the EM is registered
>>>>>>>> + *
>>>>>>>> + * Try to unregister the EM for the specified device (but not a CPU).
>>>>>>>> + */
>>>>>>>> +void em_dev_unregister_perf_domain(struct device *dev)
>>>>>>>> +{
>>>>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + if (_is_cpu_device(dev))
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + mutex_lock(&em_pd_mutex);
>>>>>>>
>>>>>>> Is the mutex really needed?
>>>>>>
>>>>>> I just wanted to align this unregister code with register. Since there
>>>>>> is debugfs dir lookup and the device's EM existence checks I thought it
>>>>>> wouldn't harm just to lock for a while and make sure the registration
>>>>>> path is not used. These two paths shouldn't affect each other, but with
>>>>>> modules loading/unloading I wanted to play safe.
>>>>>>
>>>>>> I can change it maybe to just dmb() and the end of the function if it's
>>>>>> a big performance problem in this unloading path. What do you think?
>>>>>
>>>>> I would rather leave the mutex locking as is.
>>>>>
>>>>> However, the question to ask is what exactly may go wrong without that
>>>>> locking in place? Is there any specific race condition that you are
>>>>> concerned about?
>>>>>
>>>>
>>>> I tried to test this with module loading & unloading with panfrost
>>>> driver and CPU hotplug (which should bail out quickly) and was OK.
>>>> I don't see any particular race. I don't too much about the
>>>> debugfs code, though. That's why I tried to protect from some
>>>> scripts/services which try to re-load the driver.
>>>>
>>>> Apart from that, maybe just this dev->em = NULL to be populated to all
>>>> CPUs, which mutex_unlock synchronizes for free here.
>>>
>>> If it may run concurrently with the registration for the same device,
>>> the locking is necessary, but in that case the !dev->em_pd check needs
>>> to go under the mutex too IMO, or you may end up leaking the pd if the
>>> registration can run between that check and the point at which the
>>> mutex is taken.
>>
>> They don't run concurrently for the same device and users of that EM are
>> already gone.
>> I just wanted to be sure that everything is cleaned and synced properly.
>> Here is some example of the directories under
>> /sys/kernel/debug/energy_model
>> cpu0, cpu4, gpu, dsp, etc
>>
>> The only worry that I had was the debugfs dir name, which is a
>> string from dev_name() and will be the same for the next registration
>> if module is re-loaded.
>
> OK, so that needs to be explained in a comment.
OK, I will add it.
>
>> So the 'name' is reused and debugfs_create_dir()
>> and debugfs_remove_recursive() uses this fsnotify, but they are
>> operating under inode_lock/unlock() on the parent dir 'energy_model'.
>> Then there is also this debugfs_lookup() which is slightly different.
>>
>> That's why I put a mutex to separate all registration and unregistration
>> for all devices.
>> It should work without the mutex in unregister path, but I think it does
>> not harm to take
>
> Well, fair enough, but I still think that the !dev->em_pd check should
> be done under the mutex or it will be confusing.
>
>> it just in case and also have the CPU variable sync for free.
>
> I'm not sure what you mean by the last part here?
The mutex_unlock for me also means dmb() took place. ARM has slightly
different memory model than x86 and I just wanted to be sure that
this new values reach memory and become visible to other cores.
mutex_unlock just guaranties this for me.
>
>>>
>>> Apart from this your kerneldoc comments might me improved IMO.
>>>
>>> First of all, you can use @dev inside of a kerneldoc (if @dev
>>> represents an argument of the documented function) and that will
>>> produce the right output automatically.
>>
>> OK
>>
>>>
>>> Second, it is better to avoid saying things like "Try to unregister
>>> ..." in kerneldoc comments (the "Try to" part is redundant). Simply
>>> say "Unregister ..." instead.
>>
>> Good point, thanks, I will use "Unregister ..." then.
>>
>>>
>>> Thanks!
>>>
>>
>> Shell I send a 'resend patch' which changes these @dev and
>> 'unregister' comments?
>
> Yes, please, but see the comments above too.
Saw it
>
>> Or wait for you to finish reviewing the other patches and send v9?
>
> That is not necessary, unless you want to make kerneldoc improvements
> in the other patches.
I will check them, but if they are OKish then I will just resend this
one.
Thank you for the review.
Regards,
Lukasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
From: Rafael J. Wysocki @ 2020-06-03 16:22 UTC (permalink / raw)
To: Lukasz Luba
Cc: Nishanth Menon, Juri Lelli, Rafael J. Wysocki, Peter Zijlstra,
Viresh Kumar, Liviu Dudau, dri-devel, Bjorn Andersson,
Benjamin Segall, alyssa.rosenzweig, Fabio Estevam,
Matthias Kaehlcke, Rob Herring, Amit Kucheria, Lorenzo Pieralisi,
Kevin Hilman, Andy Gross, Daniel Lezcano, steven.price,
Chanwoo Choi, Ingo Molnar, dl-linux-imx, Zhang, Rui, Mel Gorman,
orjan.eide, Daniel Vetter, Linux PM, linux-arm-msm, Sascha Hauer,
Steven Rostedt, moderated list:ARM/Mediatek SoC...,
Matthias Brugger, Linux OMAP Mailing List, Dietmar Eggemann,
Linux ARM, David Airlie, Tomeu Vizoso, Quentin Perret,
Stephen Boyd, Randy Dunlap, Rafael J. Wysocki,
Linux Kernel Mailing List, Bartlomiej Zolnierkiewicz,
Sascha Hauer, Sudeep Holla, patrick.bellasi, Shawn Guo
In-Reply-To: <d0894383-1362-fdea-f74c-7dd8ecdc33ca@arm.com>
On Wed, Jun 3, 2020 at 6:12 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 6/3/20 4:40 PM, Rafael J. Wysocki wrote:
> > On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >>
> >>
> >> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote:
> >>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>
> >>>> Hi Daniel,
> >>>>
> >>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote:
> >>>>> On 27/05/2020 11:58, Lukasz Luba wrote:
> >>>>>> Add support for other devices than CPUs. The registration function
> >>>>>> does not require a valid cpumask pointer and is ready to handle new
> >>>>>> devices. Some of the internal structures has been reorganized in order to
> >>>>>> keep consistent view (like removing per_cpu pd pointers).
> >>>>>>
> >>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >>>>>> ---
> >>>>>
> >>>>> [ ... ]
> >>>>>
> >>>>>> }
> >>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain);
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
> >>>>>> + * @dev : Device for which the EM is registered
> >>>>>> + *
> >>>>>> + * Try to unregister the EM for the specified device (but not a CPU).
> >>>>>> + */
> >>>>>> +void em_dev_unregister_perf_domain(struct device *dev)
> >>>>>> +{
> >>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
> >>>>>> + return;
> >>>>>> +
> >>>>>> + if (_is_cpu_device(dev))
> >>>>>> + return;
> >>>>>> +
> >>>>>> + mutex_lock(&em_pd_mutex);
> >>>>>
> >>>>> Is the mutex really needed?
> >>>>
> >>>> I just wanted to align this unregister code with register. Since there
> >>>> is debugfs dir lookup and the device's EM existence checks I thought it
> >>>> wouldn't harm just to lock for a while and make sure the registration
> >>>> path is not used. These two paths shouldn't affect each other, but with
> >>>> modules loading/unloading I wanted to play safe.
> >>>>
> >>>> I can change it maybe to just dmb() and the end of the function if it's
> >>>> a big performance problem in this unloading path. What do you think?
> >>>
> >>> I would rather leave the mutex locking as is.
> >>>
> >>> However, the question to ask is what exactly may go wrong without that
> >>> locking in place? Is there any specific race condition that you are
> >>> concerned about?
> >>>
> >>
> >> I tried to test this with module loading & unloading with panfrost
> >> driver and CPU hotplug (which should bail out quickly) and was OK.
> >> I don't see any particular race. I don't too much about the
> >> debugfs code, though. That's why I tried to protect from some
> >> scripts/services which try to re-load the driver.
> >>
> >> Apart from that, maybe just this dev->em = NULL to be populated to all
> >> CPUs, which mutex_unlock synchronizes for free here.
> >
> > If it may run concurrently with the registration for the same device,
> > the locking is necessary, but in that case the !dev->em_pd check needs
> > to go under the mutex too IMO, or you may end up leaking the pd if the
> > registration can run between that check and the point at which the
> > mutex is taken.
>
> They don't run concurrently for the same device and users of that EM are
> already gone.
> I just wanted to be sure that everything is cleaned and synced properly.
> Here is some example of the directories under
> /sys/kernel/debug/energy_model
> cpu0, cpu4, gpu, dsp, etc
>
> The only worry that I had was the debugfs dir name, which is a
> string from dev_name() and will be the same for the next registration
> if module is re-loaded.
OK, so that needs to be explained in a comment.
> So the 'name' is reused and debugfs_create_dir()
> and debugfs_remove_recursive() uses this fsnotify, but they are
> operating under inode_lock/unlock() on the parent dir 'energy_model'.
> Then there is also this debugfs_lookup() which is slightly different.
>
> That's why I put a mutex to separate all registration and unregistration
> for all devices.
> It should work without the mutex in unregister path, but I think it does
> not harm to take
Well, fair enough, but I still think that the !dev->em_pd check should
be done under the mutex or it will be confusing.
> it just in case and also have the CPU variable sync for free.
I'm not sure what you mean by the last part here?
> >
> > Apart from this your kerneldoc comments might me improved IMO.
> >
> > First of all, you can use @dev inside of a kerneldoc (if @dev
> > represents an argument of the documented function) and that will
> > produce the right output automatically.
>
> OK
>
> >
> > Second, it is better to avoid saying things like "Try to unregister
> > ..." in kerneldoc comments (the "Try to" part is redundant). Simply
> > say "Unregister ..." instead.
>
> Good point, thanks, I will use "Unregister ..." then.
>
> >
> > Thanks!
> >
>
> Shell I send a 'resend patch' which changes these @dev and
> 'unregister' comments?
Yes, please, but see the comments above too.
> Or wait for you to finish reviewing the other patches and send v9?
That is not necessary, unless you want to make kerneldoc improvements
in the other patches.
Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] arm: dts: vexpress: Move mcc node back into motherboard node
From: Andre Przywara @ 2020-06-03 16:22 UTC (permalink / raw)
To: Rob Herring, Liviu Dudau, Sudeep Holla, Lorenzo Pieralisi
Cc: devicetree, Guenter Roeck, linux-arm-kernel
Commit d9258898ad49 ("arm64: dts: arm: vexpress: Move fixed devices
out of bus node") moved the "mcc" DT node into the root node, because
it does not have any children using "reg" properties, so does violate
some dtc checks about "simple-bus" nodes.
However this broke the vexpress config-bus code, which walks up the
device tree to find the first node with an "arm,vexpress,site" property.
This gave the wrong result (matching the root node instead of the
motherboard node), so broke the clocks and some other devices for
VExpress boards.
Move the whole node back into its original position. This re-introduces
the dtc warning, but is conceptually the right thing to do. The dtc
warning seems to be overzealous here, there are discussions on fixing or
relaxing this check instead.
Fixes: d9258898ad49 ("arm64: dts: arm: vexpress: Move fixed devices out of bus node")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
P.S. The broken commit has not reached mainline yet, but is already in
arm-soc/arm/dt.
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 146 ++++++++++++------------
1 file changed, 73 insertions(+), 73 deletions(-)
diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index e6308fb76183..a88ee5294d35 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -100,79 +100,6 @@
};
};
- mcc {
- compatible = "arm,vexpress,config-bus";
- arm,vexpress,config-bridge = <&v2m_sysreg>;
-
- oscclk0 {
- /* MCC static memory clock */
- compatible = "arm,vexpress-osc";
- arm,vexpress-sysreg,func = <1 0>;
- freq-range = <25000000 60000000>;
- #clock-cells = <0>;
- clock-output-names = "v2m:oscclk0";
- };
-
- v2m_oscclk1: oscclk1 {
- /* CLCD clock */
- compatible = "arm,vexpress-osc";
- arm,vexpress-sysreg,func = <1 1>;
- freq-range = <23750000 65000000>;
- #clock-cells = <0>;
- clock-output-names = "v2m:oscclk1";
- };
-
- v2m_oscclk2: oscclk2 {
- /* IO FPGA peripheral clock */
- compatible = "arm,vexpress-osc";
- arm,vexpress-sysreg,func = <1 2>;
- freq-range = <24000000 24000000>;
- #clock-cells = <0>;
- clock-output-names = "v2m:oscclk2";
- };
-
- volt-vio {
- /* Logic level voltage */
- compatible = "arm,vexpress-volt";
- arm,vexpress-sysreg,func = <2 0>;
- regulator-name = "VIO";
- regulator-always-on;
- label = "VIO";
- };
-
- temp-mcc {
- /* MCC internal operating temperature */
- compatible = "arm,vexpress-temp";
- arm,vexpress-sysreg,func = <4 0>;
- label = "MCC";
- };
-
- reset {
- compatible = "arm,vexpress-reset";
- arm,vexpress-sysreg,func = <5 0>;
- };
-
- muxfpga {
- compatible = "arm,vexpress-muxfpga";
- arm,vexpress-sysreg,func = <7 0>;
- };
-
- shutdown {
- compatible = "arm,vexpress-shutdown";
- arm,vexpress-sysreg,func = <8 0>;
- };
-
- reboot {
- compatible = "arm,vexpress-reboot";
- arm,vexpress-sysreg,func = <9 0>;
- };
-
- dvimode {
- compatible = "arm,vexpress-dvimode";
- arm,vexpress-sysreg,func = <11 0>;
- };
- };
-
bus@8000000 {
motherboard-bus {
model = "V2M-P1";
@@ -435,6 +362,79 @@
};
};
};
+
+ mcc {
+ compatible = "arm,vexpress,config-bus";
+ arm,vexpress,config-bridge = <&v2m_sysreg>;
+
+ oscclk0 {
+ /* MCC static memory clock */
+ compatible = "arm,vexpress-osc";
+ arm,vexpress-sysreg,func = <1 0>;
+ freq-range = <25000000 60000000>;
+ #clock-cells = <0>;
+ clock-output-names = "v2m:oscclk0";
+ };
+
+ v2m_oscclk1: oscclk1 {
+ /* CLCD clock */
+ compatible = "arm,vexpress-osc";
+ arm,vexpress-sysreg,func = <1 1>;
+ freq-range = <23750000 65000000>;
+ #clock-cells = <0>;
+ clock-output-names = "v2m:oscclk1";
+ };
+
+ v2m_oscclk2: oscclk2 {
+ /* IO FPGA peripheral clock */
+ compatible = "arm,vexpress-osc";
+ arm,vexpress-sysreg,func = <1 2>;
+ freq-range = <24000000 24000000>;
+ #clock-cells = <0>;
+ clock-output-names = "v2m:oscclk2";
+ };
+
+ volt-vio {
+ /* Logic level voltage */
+ compatible = "arm,vexpress-volt";
+ arm,vexpress-sysreg,func = <2 0>;
+ regulator-name = "VIO";
+ regulator-always-on;
+ label = "VIO";
+ };
+
+ temp-mcc {
+ /* MCC internal operating temperature */
+ compatible = "arm,vexpress-temp";
+ arm,vexpress-sysreg,func = <4 0>;
+ label = "MCC";
+ };
+
+ reset {
+ compatible = "arm,vexpress-reset";
+ arm,vexpress-sysreg,func = <5 0>;
+ };
+
+ muxfpga {
+ compatible = "arm,vexpress-muxfpga";
+ arm,vexpress-sysreg,func = <7 0>;
+ };
+
+ shutdown {
+ compatible = "arm,vexpress-shutdown";
+ arm,vexpress-sysreg,func = <8 0>;
+ };
+
+ reboot {
+ compatible = "arm,vexpress-reboot";
+ arm,vexpress-sysreg,func = <9 0>;
+ };
+
+ dvimode {
+ compatible = "arm,vexpress-dvimode";
+ arm,vexpress-sysreg,func = <11 0>;
+ };
+ };
};
};
};
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v4 1/7] perf: arm64: Add test to check userspace access to hardware counters.
From: Rob Herring @ 2020-06-03 16:19 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, raph.gault+kdev, peterz, catalin.marinas,
will.deacon, linux-kernel, acme, Raphael Gault, mingo,
linux-arm-kernel
In-Reply-To: <20190827191755.00007a57@huawei.com>
On Tue, Aug 27, 2019 at 07:17:55PM +0800, Jonathan Cameron wrote:
> On Thu, 22 Aug 2019 15:42:14 +0100
> Raphael Gault <raphael.gault@arm.com> wrote:
>
> > This test relies on the fact that the PMU registers are accessible
> > from userspace. It then uses the perf_event_mmap_page to retrieve
> > the counter index and access the underlying register.
> >
> > This test uses sched_setaffinity(2) in order to run on all CPU and thus
> > check the behaviour of the PMU of all cpus in a big.LITTLE environment.
> >
> > Signed-off-by: Raphael Gault <raphael.gault@arm.com>
>
> Hi Raphael,
>
> I just tested this on 1620 and it works fairly nicely with one exception...
I'm working on reviving this series.
> The test will run and generate garbage numbers if the rest of the
> series isn't yet applied to the kernel. Is there anything we can do
> to prevent that?
I've added a check that user access is enabled which should prevent
that. It also validates pmc_width is set which was missing in this
series.
> It's a slightly silly complaint, but this also take a while compared to all
> the other tests if you have lots of cores, so maybe a slightly shorter
> test?
I'm not sure what the value of running on every core was supposed to be.
If we want to check big.LITTLE, then the test should detect that and
pass if user access is disabled on all cores. If we're not on
big.LITTLE, then I don't see the point in this test running on every
core.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] mm/memblock: export max_pfn for kernel modules
From: David Hildenbrand @ 2020-06-03 16:16 UTC (permalink / raw)
To: Miles Chen, Mike Rapoport, Andrew Morton, Matthias Brugger
Cc: linux-mm, linux-mediatek, linux-kernel, linux-arm-kernel,
wsd_upstream
In-Reply-To: <20200603161132.2441-1-miles.chen@mediatek.com>
On 03.06.20 18:11, Miles Chen wrote:
> max_pfn is uesd to get the highest pfn in the system. Drivers like
> drivers/iommu/mtk_iommu.c checks max_pfn to see if it should enable
> its "4GB mode".
>
> This patch exports the max_pfn symbol, so we can build the driver as
> a kernel module.
Please add that change to the respective user patch (and cc MM-people
for that patch), so we have the actual user right along the change and
can figure out if this is the right thing to do.
>
> Signed-off-by: Miles Chen <miles.chen@mediatek.com>
> ---
> mm/memblock.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index c79ba6f9920c..3b2b21ecebb6 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -99,6 +99,7 @@ EXPORT_SYMBOL(contig_page_data);
> unsigned long max_low_pfn;
> unsigned long min_low_pfn;
> unsigned long max_pfn;
> +EXPORT_SYMBOL(max_pfn);
> unsigned long long max_possible_pfn;
>
> static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
>
--
Thanks,
David / dhildenb
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] dt-bindings: rtc: Convert imxdi rtc to json-schema
From: Rob Herring @ 2020-06-03 16:15 UTC (permalink / raw)
To: Anson Huang
Cc: Roland Stigge, Alessandro Zummo, Alexandre Belloni, devicetree,
Shawn Guo, Sascha Hauer, linux-kernel@vger.kernel.org,
NXP Linux Team, Sascha Hauer, Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM
In-Reply-To: <1591184925-13055-1-git-send-email-Anson.Huang@nxp.com>
On Wed, Jun 3, 2020 at 5:59 AM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Convert the i.MXDI RTC binding to DT schema format using json-schema
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> .../devicetree/bindings/rtc/imxdi-rtc.txt | 20 -----------
> .../devicetree/bindings/rtc/imxdi-rtc.yaml | 42 ++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 20 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> create mode 100644 Documentation/devicetree/bindings/rtc/imxdi-rtc.yaml
>
> diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> deleted file mode 100644
> index c797bc9..0000000
> --- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -* i.MX25 Real Time Clock controller
> -
> -Required properties:
> -- compatible: should be: "fsl,imx25-rtc"
> -- reg: physical base address of the controller and length of memory mapped
> - region.
> -- clocks: should contain the phandle for the rtc clock
> -- interrupts: rtc alarm interrupt
> -
> -Optional properties:
> -- interrupts: dryice security violation interrupt (second entry)
> -
> -Example:
> -
> -rtc@53ffc000 {
> - compatible = "fsl,imx25-rtc";
> - reg = <0x53ffc000 0x4000>;
> - clocks = <&clks 81>;
> - interrupts = <25 56>;
> -};
> diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.yaml b/Documentation/devicetree/bindings/rtc/imxdi-rtc.yaml
> new file mode 100644
> index 0000000..6e43926
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/imxdi-rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: i.MX25 Real Time Clock controller
> +
> +maintainers:
> + - Roland Stigge <stigge@antcom.de>
> +
> +properties:
> + compatible:
> + const: fsl,imx25-rtc
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + items:
> + - description: rtc alarm interrupt
> + - description: dryice security violation interrupt
> + minItems: 1
> + maxItems: 2
> +
> + clocks:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
Needs:
additionalProperties: false
(or if you have a top level $ref, 'unevaluatedProperties: false')
I fixed these up in what I applied already, but please check all of
yours pending and fix.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
From: Lukasz Luba @ 2020-06-03 16:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Nishanth Menon, Juri Lelli, Peter Zijlstra, Viresh Kumar,
Liviu Dudau, dri-devel, Bjorn Andersson, Benjamin Segall,
alyssa.rosenzweig, Fabio Estevam, Matthias Kaehlcke, Rob Herring,
Amit Kucheria, Lorenzo Pieralisi, Kevin Hilman, Andy Gross,
Daniel Lezcano, steven.price, Chanwoo Choi, Ingo Molnar,
dl-linux-imx, Zhang, Rui, Mel Gorman, orjan.eide, Daniel Vetter,
Linux PM, linux-arm-msm, Sascha Hauer, Steven Rostedt,
moderated list:ARM/Mediatek SoC..., Matthias Brugger,
Linux OMAP Mailing List, Dietmar Eggemann, Linux ARM,
David Airlie, Tomeu Vizoso, Quentin Perret, Stephen Boyd,
Randy Dunlap, Rafael J. Wysocki, Linux Kernel Mailing List,
Bartlomiej Zolnierkiewicz, Sascha Hauer, Sudeep Holla,
patrick.bellasi, Shawn Guo
In-Reply-To: <CAJZ5v0iDNH7tZmKsYgW1xp-g3WmOod+Wo-AzJmszXuv_wztwwA@mail.gmail.com>
On 6/3/20 4:40 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>
>>
>> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote:
>>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote:
>>>>> On 27/05/2020 11:58, Lukasz Luba wrote:
>>>>>> Add support for other devices than CPUs. The registration function
>>>>>> does not require a valid cpumask pointer and is ready to handle new
>>>>>> devices. Some of the internal structures has been reorganized in order to
>>>>>> keep consistent view (like removing per_cpu pd pointers).
>>>>>>
>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>>> ---
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(em_register_perf_domain);
>>>>>> +
>>>>>> +/**
>>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
>>>>>> + * @dev : Device for which the EM is registered
>>>>>> + *
>>>>>> + * Try to unregister the EM for the specified device (but not a CPU).
>>>>>> + */
>>>>>> +void em_dev_unregister_perf_domain(struct device *dev)
>>>>>> +{
>>>>>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
>>>>>> + return;
>>>>>> +
>>>>>> + if (_is_cpu_device(dev))
>>>>>> + return;
>>>>>> +
>>>>>> + mutex_lock(&em_pd_mutex);
>>>>>
>>>>> Is the mutex really needed?
>>>>
>>>> I just wanted to align this unregister code with register. Since there
>>>> is debugfs dir lookup and the device's EM existence checks I thought it
>>>> wouldn't harm just to lock for a while and make sure the registration
>>>> path is not used. These two paths shouldn't affect each other, but with
>>>> modules loading/unloading I wanted to play safe.
>>>>
>>>> I can change it maybe to just dmb() and the end of the function if it's
>>>> a big performance problem in this unloading path. What do you think?
>>>
>>> I would rather leave the mutex locking as is.
>>>
>>> However, the question to ask is what exactly may go wrong without that
>>> locking in place? Is there any specific race condition that you are
>>> concerned about?
>>>
>>
>> I tried to test this with module loading & unloading with panfrost
>> driver and CPU hotplug (which should bail out quickly) and was OK.
>> I don't see any particular race. I don't too much about the
>> debugfs code, though. That's why I tried to protect from some
>> scripts/services which try to re-load the driver.
>>
>> Apart from that, maybe just this dev->em = NULL to be populated to all
>> CPUs, which mutex_unlock synchronizes for free here.
>
> If it may run concurrently with the registration for the same device,
> the locking is necessary, but in that case the !dev->em_pd check needs
> to go under the mutex too IMO, or you may end up leaking the pd if the
> registration can run between that check and the point at which the
> mutex is taken.
They don't run concurrently for the same device and users of that EM are
already gone.
I just wanted to be sure that everything is cleaned and synced properly.
Here is some example of the directories under
/sys/kernel/debug/energy_model
cpu0, cpu4, gpu, dsp, etc
The only worry that I had was the debugfs dir name, which is a
string from dev_name() and will be the same for the next registration
if module is re-loaded. So the 'name' is reused and debugfs_create_dir()
and debugfs_remove_recursive() uses this fsnotify, but they are
operating under inode_lock/unlock() on the parent dir 'energy_model'.
Then there is also this debugfs_lookup() which is slightly different.
That's why I put a mutex to separate all registration and unregistration
for all devices.
It should work without the mutex in unregister path, but I think it does
not harm to take it just in case and also have the CPU variable sync for
free.
>
> Apart from this your kerneldoc comments might me improved IMO.
>
> First of all, you can use @dev inside of a kerneldoc (if @dev
> represents an argument of the documented function) and that will
> produce the right output automatically.
OK
>
> Second, it is better to avoid saying things like "Try to unregister
> ..." in kerneldoc comments (the "Try to" part is redundant). Simply
> say "Unregister ..." instead.
Good point, thanks, I will use "Unregister ..." then.
>
> Thanks!
>
Shell I send a 'resend patch' which changes these @dev and
'unregister' comments?
Or wait for you to finish reviewing the other patches and send v9?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] mm/memblock: export max_pfn for kernel modules
From: Miles Chen @ 2020-06-03 16:11 UTC (permalink / raw)
To: Mike Rapoport, Andrew Morton, Matthias Brugger
Cc: wsd_upstream, linux-kernel, linux-mm, Miles Chen, linux-mediatek,
linux-arm-kernel
max_pfn is uesd to get the highest pfn in the system. Drivers like
drivers/iommu/mtk_iommu.c checks max_pfn to see if it should enable
its "4GB mode".
This patch exports the max_pfn symbol, so we can build the driver as
a kernel module.
Signed-off-by: Miles Chen <miles.chen@mediatek.com>
---
mm/memblock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/memblock.c b/mm/memblock.c
index c79ba6f9920c..3b2b21ecebb6 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -99,6 +99,7 @@ EXPORT_SYMBOL(contig_page_data);
unsigned long max_low_pfn;
unsigned long min_low_pfn;
unsigned long max_pfn;
+EXPORT_SYMBOL(max_pfn);
unsigned long long max_possible_pfn;
static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 1/2] arm64: Override SPSR.SS when single-stepping is enabled
From: Will Deacon @ 2020-06-03 15:53 UTC (permalink / raw)
To: Keno Fischer
Cc: Mark Rutland, Luis Machado, kernel-team, stable, linux-arm-kernel
In-Reply-To: <CABV8kRwrnixNc074-jQhZzeucGHx9_e5FnQmBS=VuL=tFGjY-Q@mail.gmail.com>
Hi Keno,
Thanks for having a look.
On Wed, Jun 03, 2020 at 11:42:46AM -0400, Keno Fischer wrote:
> On Wed, Jun 3, 2020 at 11:10 AM Will Deacon <will@kernel.org> wrote:
> >
> > Luis reports that, when reverse debugging with GDB, single-step does not
> > function as expected on arm64:
> >
> > | I've noticed, under very specific conditions, that a PTRACE_SINGLESTEP
> > | request by GDB won't execute the underlying instruction. As a consequence,
> > | the PC doesn't move, but we return a SIGTRAP just like we would for a
> > | regular successful PTRACE_SINGLESTEP request.
> >
> > The underlying problem is that when the CPU register state is restored
> > as part of a reverse step, the SPSR.SS bit is cleared and so the hardware
> > single-step state can transition to the "active-pending" state, causing
> > an unexpected step exception to be taken immediately if a step operation
> > is attempted.
>
> We saw this issue also and worked around it in user-space [1]. That said,
> I think I'm ok with this change in the kernel, since I can't think of
> a particularly useful usecase for this feature.
>
> However, at the same time as changing this, we should probably make sure
> to enable the syscall exit pseudo-singlestep trap (similar issue as the other
> patch I had sent for the signal pseudo-singlestep trap), since otherwise
> ptracers might get confused about the lack of singlestep trap during a
> singlestep -> seccomp -> singlestep path (which would give one trap
> less with this patch than before).
Hmm, I don't completely follow your example. Please could you spell it
out a bit more? I fast-forward the stepping state machine on sigreturn,
which I thought would be sufficient. Perhaps you're referring to a variant
of the situation mentioned by Mark, which I didn't think could happen
with ptrace [2].
Cheers,
Will
[2] https://lore.kernel.org/r/20200531095216.GB30204@willie-the-truck
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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