* [PATCHv2 0/4] WX checking for arm64
From: Laura Abbott @ 2016-10-12 22:31 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
This is v2 of the implementation to check for writable and executable pages on
arm64.
Major changes since v1:
- I realized my concerns about initialization and registration were unfounded
so registration to register page tables with debugfs is simplified.
- New patch to remove max_addr since it was pointed out it was unused.
- Rebased to include changes for the EFI page tables as well.
- Checking is now only done on the init_mm page tables. It was mentioned that
we should check the hyp page tables as well but that can be follow on work.
- Checking for UXN per suggestion from Mark Rutland.
Laura Abbott (4):
arm64: dump: Make ptdump debugfs a separate option
arm64: dump: Make the page table dumping seq_file optional
arm64: dump: Remove max_addr
arm64: dump: Add checking for writable and exectuable pages
arch/arm64/Kconfig.debug | 34 ++++++++++++++-
arch/arm64/include/asm/ptdump.h | 22 +++++++---
arch/arm64/mm/Makefile | 3 +-
arch/arm64/mm/dump.c | 89 ++++++++++++++++++++++++++------------
arch/arm64/mm/mmu.c | 2 +
arch/arm64/mm/ptdump_debugfs.c | 31 +++++++++++++
drivers/firmware/efi/arm-runtime.c | 5 +--
7 files changed, 147 insertions(+), 39 deletions(-)
create mode 100644 arch/arm64/mm/ptdump_debugfs.c
--
2.7.4
^ permalink raw reply
* [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
From: Baicar, Tyler @ 2016-10-12 22:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161011185236.GC1041@n2100.armlinux.org.uk>
Hello Russell,
Thank you for the feedback! Responses below
On 10/11/2016 12:52 PM, Russell King - ARM Linux wrote:
> On Fri, Oct 07, 2016 at 03:31:14PM -0600, Tyler Baicar wrote:
>> +static void cper_estatus_print_section_v300(const char *pfx,
>> + const struct acpi_hest_generic_data_v300 *gdata)
>> +{
>> + __u8 hour, min, sec, day, mon, year, century, *timestamp;
>> +
>> + if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>> + timestamp = (__u8 *)&(gdata->time_stamp);
>> + memcpy(&sec, timestamp, 1);
>> + memcpy(&min, timestamp + 1, 1);
>> + memcpy(&hour, timestamp + 2, 1);
>> + memcpy(&day, timestamp + 4, 1);
>> + memcpy(&mon, timestamp + 5, 1);
>> + memcpy(&year, timestamp + 6, 1);
>> + memcpy(¢ury, timestamp + 7, 1);
> This is utterly silly. Why are you using memcpy() to access individual
> bytes of a u8 pointer? What's wrong with:
>
> sec = timestamp[0];
> min = timestamp[1];
> hour = timestamp[2];
> day = timestamp[4];
> mon = timestamp[5];
> year = timestamp[6];
> century = timestamp[7];
>
> or even do the conversion here:
>
> sec = bcd2bin(timestamp[0]);
> ... etc ...
Yes, that will be a lot cleaner especially with moving the conversion here.
>
>> + printk("%stime: ", pfx);
>> + printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>> + printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
>> + bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
>> + bcd2bin(century), bcd2bin(year), bcd2bin(mon),
>> + bcd2bin(day));
>> + }
> It's also a good idea to (as much as possible) keep to single printk()
> statements - which makes the emission of the string more atomic wrt
> other CPUs and contexts. So, this should probably become (with the
> conversion being done at the assignment of sec etc):
>
> printk("%stime: %7s %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> pfx, 0x01 & timestamp[3] ? "precise" : "",
> hour, min, sec, century, year, mon, day);
>
> which, IMHO, looks a lot nicer and doesn't risk some other printk()
> getting between each individual part of the line.
I will make this change in the next version. This printk does look a lot
nicer and avoids other prints from getting in the middle (I actually
just saw that happen in testing a couple days ago)
>> +}
>> +
>> static void cper_estatus_print_section(
>> - const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
>> + const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>> {
>> uuid_le *sec_type = (uuid_le *)gdata->section_type;
>> __u16 severity;
>> char newpfx[64];
>>
>> + if ((gdata->revision >> 8) >= 0x03)
>> + cper_estatus_print_section_v300(pfx,
>> + (const struct acpi_hest_generic_data_v300 *)gdata);
>> +
>> severity = gdata->error_severity;
>> printk("%s""Error %d, type: %s\n", pfx, sec_no,
>> cper_severity_str(severity));
> Not sure why you have the "" here - %sError works just as well and the
> "" is just obfuscation - the compiler will eliminate the double-double
> quote and merge the strings anyway.
>
I will remove the "" in the next version.
Thanks,
Tyler
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH v3 3/6] pwm: imx: support output polarity inversion
From: Lukasz Majewski @ 2016-10-12 22:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5325a332099473a2d8382530f79c5a8e@agner.ch>
Hi Stefan,
> On 2016-10-07 08:11, Bhuvanchandra DV wrote:
> > From: Lothar Wassmann <LW@KARO-electronics.de>
> >
> > The i.MX pwm unit on i.MX27 and newer SoCs provides a configurable
> > output polarity. This patch adds support to utilize this feature
> > where available.
> >
> > Signed-off-by: Lothar Wa?mann <LW@KARO-electronics.de>
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> > Acked-by: Shawn Guo <shawn.guo@linaro.org>
> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> > Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +--
> > drivers/pwm/pwm-imx.c | 51
> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > index e00c2e9..c61bdf8 100644
> > --- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > @@ -6,8 +6,8 @@ Required properties:
> > - "fsl,imx1-pwm" for PWM compatible with the one integrated on
> > i.MX1
> > - "fsl,imx27-pwm" for PWM compatible with the one integrated on
> > i.MX27
> > - reg: physical base address and length of the controller's
> > registers -- #pwm-cells: should be 2. See pwm.txt in this directory
> > for a description of
> > - the cells format.
> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See
> > pwm.txt
> > + in this directory for a description of the cells format.
> > - clocks : Clock specifiers for both ipg and per clocks.
> > - clock-names : Clock names should include both "ipg" and "per"
> > See the clock consumer binding,
> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > Example:
> >
> > pwm1: pwm at 53fb4000 {
> > - #pwm-cells = <2>;
> > + #pwm-cells = <3>;
> > compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > reg = <0x53fb4000 0x4000>;
> > clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..c37d223 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -38,6 +38,7 @@
> > #define MX3_PWMCR_DOZEEN (1 << 24)
> > #define MX3_PWMCR_WAITEN (1 << 23)
> > #define MX3_PWMCR_DBGEN (1 << 22)
> > +#define MX3_PWMCR_POUTC (1 << 18)
> > #define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
> > #define MX3_PWMCR_CLKSRC_IPG (1 << 16)
> > #define MX3_PWMCR_SWR (1 << 3)
> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (enable)
> > cr |= MX3_PWMCR_EN;
> >
> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > + cr |= MX3_PWMCR_POUTC;
> > +
>
> This seems wrong to me, the config callback is meant for period/duty
> cycle only.
If it is meant only for that, then the polarity should be removed from
it.
However after very quick testing, at least on my setup, it turns out
that removing this lines causes polarity to _not_ being set (and the
polarity is not inverted).
I will investigate this further on my setup and hopefully sent proper
patch.
> The set_polarity callback should get called in case a
> different polarity is requested.
On my setup the pwm2 is set from DT and pwm_backlight_probe() calls
pwm_apply_args(), so everything should work. However, as I mentioned
above there still is some problem with inversion setting.
>
>
> > writel(cr, imx->mmio_base + MX3_PWMCR);
> >
> > return 0;
> > @@ -240,27 +244,62 @@ static void imx_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm)
> > clk_disable_unprepare(imx->clk_per);
> > }
> >
> > -static struct pwm_ops imx_pwm_ops = {
> > +static int imx_pwm_set_polarity(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + u32 val;
> > +
> > + if (polarity == pwm->args.polarity)
> > + return 0;
>
> I don't think that this is right. Today, pwm_apply_args (in
> include/linux/pwm.h) copies the polarity from args to state.polarity,
> which is then passed as polarity argument to this function. So this
> will always return 0 afaict.
Yes, I've overlooked it (that the state is copied).
It can be dropped.
>
> I would just drop that.
>
> There is probably one little problem in the current state of affairs:
> If the bootloader makes use of a PWM channel with inverted state,
> then the kernel would not know about that and currently assume a
> wrong initial state... I guess at one point in time we should
> implement the state retrieval callback and move to the new atomic PWM
> API, which would mean to implement apply callback.
Are there any patches on the horizon?
>
> --
> Stefan
>
>
> > +
> > + val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > + if (polarity == PWM_POLARITY_INVERSED)
> > + val |= MX3_PWMCR_POUTC;
> > + else
> > + val &= ~MX3_PWMCR_POUTC;
> > +
> > + writel(val, imx->mmio_base + MX3_PWMCR);
> > +
> > + dev_dbg(imx->chip.dev, "%s: polarity set to %s\n",
> > __func__,
> > + polarity == PWM_POLARITY_INVERSED ? "inverted" :
> > "normal"); +
> > + return 0;
> > +}
> > +
> > +static struct pwm_ops imx_pwm_ops_v1 = {
> > .enable = imx_pwm_enable,
> > .disable = imx_pwm_disable,
> > .config = imx_pwm_config,
> > .owner = THIS_MODULE,
> > };
> >
> > +static struct pwm_ops imx_pwm_ops_v2 = {
> > + .enable = imx_pwm_enable,
> > + .disable = imx_pwm_disable,
> > + .set_polarity = imx_pwm_set_polarity,
> > + .config = imx_pwm_config,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > struct imx_pwm_data {
> > int (*config)(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int
> > period_ns); void (*set_enable)(struct pwm_chip *chip, bool enable);
> > + struct pwm_ops *pwm_ops;
> > };
> >
> > static struct imx_pwm_data imx_pwm_data_v1 = {
> > .config = imx_pwm_config_v1,
> > .set_enable = imx_pwm_set_enable_v1,
> > + .pwm_ops = &imx_pwm_ops_v1,
> > };
> >
> > static struct imx_pwm_data imx_pwm_data_v2 = {
> > .config = imx_pwm_config_v2,
> > .set_enable = imx_pwm_set_enable_v2,
> > + .pwm_ops = &imx_pwm_ops_v2,
> > };
> >
> > static const struct of_device_id imx_pwm_dt_ids[] = {
> > @@ -282,6 +321,8 @@ static int imx_pwm_probe(struct platform_device
> > *pdev) if (!of_id)
> > return -ENODEV;
> >
> > + data = of_id->data;
> > +
> > imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > if (imx == NULL)
> > return -ENOMEM;
> > @@ -300,18 +341,22 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_ipg);
> > }
> >
> > - imx->chip.ops = &imx_pwm_ops;
> > + imx->chip.ops = data->pwm_ops;
> > imx->chip.dev = &pdev->dev;
> > imx->chip.base = -1;
> > imx->chip.npwm = 1;
> > imx->chip.can_sleep = true;
> > + if (data->pwm_ops->set_polarity) {
> > + dev_dbg(&pdev->dev, "PWM supports output
> > inversion\n");
> > + imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > + imx->chip.of_pwm_n_cells = 3;
> > + }
> >
> > r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> > if (IS_ERR(imx->mmio_base))
> > return PTR_ERR(imx->mmio_base);
> >
> > - data = of_id->data;
> > imx->config = data->config;
> > imx->set_enable = data->set_enable;
>
Best regards,
?ukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161013/c11675fd/attachment-0001.sig>
^ permalink raw reply
* [PATCH V2 1/3] Revert "ACPI,PCI,IRQ: reduce static IRQ array size to 16"
From: Bjorn Helgaas @ 2016-10-12 22:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475615720-31047-2-git-send-email-okaya@codeaurora.org>
Hi Sinan,
I have to apologize because I haven't followed all the discussion and
now I'm trying to figure it out from the patches and changelogs. But
I guess that's not all bad, because future interested folks *should*
be able to figure things out from that :)
On Tue, Oct 04, 2016 at 05:15:17PM -0400, Sinan Kaya wrote:
> This reverts commit 5c5087a55390 ("ACPI,PCI,IRQ: reduce static IRQ array
> size to 16").
>
> The code maintains a fixed size array for IRQ penalties. The array
> gets updated by external calls such as acpi_penalize_sci_irq,
> acpi_penalize_isa_irq to reflect the actual interrupt usage of the
> system. Since the IRQ distribution is platform specific, this is
> not known ahead of time. The IRQs get updated based on the SCI
> interrupt number BIOS has chosen or the ISA IRQs that were assigned
> to existing peripherals.
>
> By the time ACPI gets initialized, this code tries to determine an
> IRQ number based on penalty values in this array. It will try to locate
> the IRQ with the least penalty assignment so that interrupt sharing is
> avoided if possible.
>
> A couple of notes about the external APIs:
> 1. These API can be called before the ACPI is started. Therefore, one
> cannot assume that the PCI link objects are initialized for calculating
> penalties.
Which API are you thinking about here? pcibios_penalize_isa_irq() is
called by ACPI and PNP, which should both be after ACPI is started.
My guess is you're thinking about acpi_penalize_sci_irq() (added back
later in this series), which is called here, which is definitely
before ACPI objects are available:
setup_arch
acpi_boot_init
acpi_process_madt
acpi_parse_madt_ioapic_entries
acpi_table_parse_madt
acpi_parse_int_src_ovr
acpi_sci_ioapic_setup
acpi_penalize_sci_irq # <---
> 2. The polarity and trigger information passed via the
> acpi_penalize_sci_irq from the BIOS may not match what the IRQ subsystem
> is reporting as the call might have been placed before the IRQ is
> registered by the interrupt subsystem.
>
> The previous change was in the direction to remove these external API and
> try to calculate the penalties at runtime for the ISA path as well. This
> didn't work out well with the existing platforms.
>
> Restoring the old behavior for IRQ < 256 and the new behavior will remain
> effective for IRQ >= 256.
IIRC, this all started because we needed more than 256 IRQs, but we
didn't know how to size a static table to be large enough without
being wasteful.
Prior to 5c5087a55390, we tracked penalties for IRQs 0-255. After it,
we only tracked penalties for IRQs 0-15. I think this patch basically
makes it so we track 0-255 again.
*This* patch only increases the range for pcibios_penalize_isa_irq()
(and command-line hints, but hopefully nobody cares about those). A
subsequent patch increases it for SCI as well.
The name "ACPI_MAX_IRQS" is now slightly misleading (because we do
support more than 256 IRQs) and the 256 value is sort of an
unjustified magic number. 16 is explainable as the number of ISA
IRQs, but I don't know what 256 is based on (other than historical
practice, of course). ACPI device IRQs can be much larger, and I
think the SCI IRQ can be, too (the FADT SCI_INT field is 16 bits).
Can you tie this back to the specific problem on the broken machine
somehow? Do we need a penalty for an IRQ in the 16-255 range?
In a subsequent patch, I see something about the IRQ type not being
updated at the right time, but I can't quite connect the dots.
To be clear, I'm not asking for any changes in the patch; I'm just
trying to understand what's going on.
> Tested-by: Jonathan Liu <net147@gmail.com>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Link: http://www.gossamer-threads.com/lists/linux/kernel/2537016#2537016
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> drivers/acpi/pci_link.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..f3792f4 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -438,6 +438,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> * enabled system.
> */
>
> +#define ACPI_MAX_IRQS 256
> #define ACPI_MAX_ISA_IRQS 16
>
> #define PIRQ_PENALTY_PCI_POSSIBLE (16*16)
> @@ -446,7 +447,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> #define PIRQ_PENALTY_ISA_USED (16*16*16*16*16)
> #define PIRQ_PENALTY_ISA_ALWAYS (16*16*16*16*16*16)
>
> -static int acpi_isa_irq_penalty[ACPI_MAX_ISA_IRQS] = {
> +static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ0 timer */
> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ1 keyboard */
> PIRQ_PENALTY_ISA_ALWAYS, /* IRQ2 cascade */
> @@ -511,7 +512,7 @@ static int acpi_irq_get_penalty(int irq)
> }
>
> if (irq < ACPI_MAX_ISA_IRQS)
> - return penalty + acpi_isa_irq_penalty[irq];
> + return penalty + acpi_irq_penalty[irq];
>
> penalty += acpi_irq_pci_sharing_penalty(irq);
> return penalty;
> @@ -538,14 +539,14 @@ int __init acpi_irq_penalty_init(void)
>
> for (i = 0; i < link->irq.possible_count; i++) {
> if (link->irq.possible[i] < ACPI_MAX_ISA_IRQS)
> - acpi_isa_irq_penalty[link->irq.
> + acpi_irq_penalty[link->irq.
> possible[i]] +=
> penalty;
> }
>
> } else if (link->irq.active &&
> - (link->irq.active < ACPI_MAX_ISA_IRQS)) {
> - acpi_isa_irq_penalty[link->irq.active] +=
> + (link->irq.active < ACPI_MAX_IRQS)) {
> + acpi_irq_penalty[link->irq.active] +=
> PIRQ_PENALTY_PCI_POSSIBLE;
> }
> }
> @@ -828,7 +829,7 @@ static void acpi_pci_link_remove(struct acpi_device *device)
> }
>
> /*
> - * modify acpi_isa_irq_penalty[] from cmdline
> + * modify acpi_irq_penalty[] from cmdline
> */
> static int __init acpi_irq_penalty_update(char *str, int used)
> {
> @@ -837,24 +838,24 @@ static int __init acpi_irq_penalty_update(char *str, int used)
> for (i = 0; i < 16; i++) {
> int retval;
> int irq;
> - int new_penalty;
>
> retval = get_option(&str, &irq);
>
> if (!retval)
> break; /* no number found */
>
> - /* see if this is a ISA IRQ */
> - if ((irq < 0) || (irq >= ACPI_MAX_ISA_IRQS))
> + if (irq < 0)
> + continue;
> +
> + if (irq >= ARRAY_SIZE(acpi_irq_penalty))
> continue;
>
> if (used)
> - new_penalty = acpi_irq_get_penalty(irq) +
> - PIRQ_PENALTY_ISA_USED;
> + acpi_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> + PIRQ_PENALTY_ISA_USED;
> else
> - new_penalty = 0;
> + acpi_irq_penalty[irq] = 0;
>
> - acpi_isa_irq_penalty[irq] = new_penalty;
> if (retval != 2) /* no next number */
> break;
> }
> @@ -870,14 +871,14 @@ static int __init acpi_irq_penalty_update(char *str, int used)
> */
> void acpi_penalize_isa_irq(int irq, int active)
> {
> - if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty)))
> - acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> - (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> + if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty))
> + acpi_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> + (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> }
>
> bool acpi_isa_irq_available(int irq)
> {
> - return irq >= 0 && (irq >= ARRAY_SIZE(acpi_isa_irq_penalty) ||
> + return irq >= 0 && (irq >= ARRAY_SIZE(acpi_irq_penalty) ||
> acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS);
> }
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
From: Baicar, Tyler @ 2016-10-12 22:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3f17d0a8-6b63-5792-903a-341effaae432@arm.com>
Hello Suzuki,
Thank you for the feedback! Responses below.
On 10/11/2016 11:28 AM, Suzuki K Poulose wrote:
> On 07/10/16 22:31, Tyler Baicar wrote:
>> Currently when a RAS error is reported it is not timestamped.
>> The ACPI 6.1 spec adds the timestamp field to the generic error
>> data entry v3 structure. The timestamp of when the firmware
>> generated the error is now being reported.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>
> Please could you keep the people who reviewed/commented on your series
> in the past,
> whenever you post a new version ?
Do you mean to just send the new version to their e-mail directly in
addition to the lists? If so, I will do that next time.
I know you provided good feedback on the previous patchset, but I did
not have anyone specifically respond to add "reviewed-by:...". I don't
think I should add reviewed-by for someone unless they specifically add
it in a response :)
>
>> ---
>> drivers/acpi/apei/ghes.c | 25 ++++++++++--
>> drivers/firmware/efi/cper.c | 97
>> +++++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 105 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 3021f0e..c8488f1 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -80,6 +80,10 @@
>> ((struct acpi_hest_generic_status *) \
>> ((struct ghes_estatus_node *)(estatus_node) + 1))
>>
>> +#define acpi_hest_generic_data_version(gdata) \
>> + (gdata->revision >> 8)
>
> ...
>
>> +inline void *acpi_hest_generic_data_payload(struct
>> acpi_hest_generic_data *gdata)
>> +{
>> + return acpi_hest_generic_data_version(gdata) >= 3 ?
>> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
>> + gdata + 1;
>> +}
>> +
>
>
>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index d425374..9fa1317 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>
>> +#define acpi_hest_generic_data_version(gdata) \
>> + (gdata->revision >> 8)
>> +
>
> ...
>
>> +static inline void *acpi_hest_generic_data_payload(struct
>> acpi_hest_generic_data *gdata)
>> +{
>> + return acpi_hest_generic_data_version(gdata) >= 3 ?
>> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
>> + gdata + 1;
>> +}
>
> Could these go to a header file, so that we don't need duplicate
> definitions of these helpers in
> different files ?
>
I think that should work to avoid duplication. I will move them to a
header file in the next patchset.
>> +
>> +static void cper_estatus_print_section_v300(const char *pfx,
>> + const struct acpi_hest_generic_data_v300 *gdata)
>> +{
>> + __u8 hour, min, sec, day, mon, year, century, *timestamp;
>> +
>> + if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
>> + timestamp = (__u8 *)&(gdata->time_stamp);
>> + memcpy(&sec, timestamp, 1);
>> + memcpy(&min, timestamp + 1, 1);
>> + memcpy(&hour, timestamp + 2, 1);
>> + memcpy(&day, timestamp + 4, 1);
>> + memcpy(&mon, timestamp + 5, 1);
>> + memcpy(&year, timestamp + 6, 1);
>> + memcpy(¢ury, timestamp + 7, 1);
>> + printk("%stime: ", pfx);
>> + printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
>
> What format is the (timestamp + 3) stored in ? Does it need conversion ?
The third byte of the timestamp is currently only used to determine if
the time is precise or not. Bit 0 is used to specify that and the other
bits in this byte are marked as reserved. This is shown in table 247 of
the UEFI spec 2.6:
Byte 3:
Bit 0 ? Timestamp is precise if this bit is set and correlates to the
time of the error event.
Bit 7:1 ? Reserved
>
>> + printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
>> + bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
>> + bcd2bin(century), bcd2bin(year), bcd2bin(mon),
>> + bcd2bin(day));
>> + }
>
> minor nit: Would it be easier to order/parse the error messages if the
> date
> is printed first followed by time ?
>
> i.e,
> 17:20:14 2016-09-15 Mon
> vs
> 2016-09-15 Mon 17:20:14
>
> e.g, people looking at a huge log, looking for logs from a specific
> date might
> find the latter more useful to skip the messages.
>
The latter does seem like it would be better for parsing large logs. I
can rearrange the order in the next patchset.
>> +}
>> +
>> static void cper_estatus_print_section(
>> - const char *pfx, const struct acpi_hest_generic_data *gdata, int
>> sec_no)
>> + const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
>> {
>> uuid_le *sec_type = (uuid_le *)gdata->section_type;
>> __u16 severity;
>> char newpfx[64];
>>
>> + if ((gdata->revision >> 8) >= 0x03)
>
> Could we use the helper defined above ?
Yes, I'll change this to use acpi_hest_generic_data_version(gdata) instead.
>
>> @@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
>> printk("%s""event severity: %s\n", pfx,
>> cper_severity_str(severity));
>> data_len = estatus->data_length;
>> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> + if ((gdata->revision >> 8) >= 0x03)
>
> Same as above, use the macro ?
Yes, I'll change this to use acpi_hest_generic_data_version(gdata) instead.
>
>> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
>> +
>> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
>> +
>> while (data_len >= sizeof(*gdata)) {
>> gedata_len = gdata->error_data_length;
>> cper_estatus_print_section(newpfx, gdata, sec_no);
>> - data_len -= gedata_len + sizeof(*gdata);
>> - gdata = (void *)(gdata + 1) + gedata_len;
>> + if(gdata_v3) {
>> + data_len -= gedata_len + sizeof(*gdata_v3);
>> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
>> + gdata = (struct acpi_hest_generic_data *)gdata_v3;
>> + } else {
>> + data_len -= gedata_len + sizeof(*gdata);
>> + gdata = (void *)(gdata + 1) + gedata_len;
>> + }
>> sec_no++;
>> }
>
> ...
>
>>
>> @@ -486,15 +543,29 @@ int cper_estatus_check(const struct
>> acpi_hest_generic_status *estatus)
>> return rc;
>> data_len = estatus->data_length;
>> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
>> - while (data_len >= sizeof(*gdata)) {
>> - gedata_len = gdata->error_data_length;
>> - if (gedata_len > data_len - sizeof(*gdata))
>> +
>> + if ((gdata->revision >> 8) >= 0x03) {
>> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
>> + while (data_len >= sizeof(*gdata_v3)) {
>> + gedata_len = gdata_v3->error_data_length;
>> + if (gedata_len > data_len - sizeof(*gdata_v3))
>> + return -EINVAL;
>> + data_len -= gedata_len + sizeof(*gdata_v3);
>> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
>> + }
>> + if (data_len)
>> + return -EINVAL;
>> + } else {
>> + while (data_len >= sizeof(*gdata)) {
>> + gedata_len = gdata->error_data_length;
>> + if (gedata_len > data_len - sizeof(*gdata))
>> + return -EINVAL;
>> + data_len -= gedata_len + sizeof(*gdata);
>> + gdata = (void *)(gdata + 1) + gedata_len;
>> + }
>> + if (data_len)
>
> As mentioned in the previous version, would it make sense to add some
> more
> helpers to deal with record versions ? We seem to be doing the version
> switch and
> code duplication at different places.
>
> Does the following help ? Thoughts ?
>
> #define acpi_hest_generic_data_error_length(gdata) (((struct
> acpi_hest_generic_data *)(gdata))->error_data_length)
> #define acpi_hest_generic_data_size(gdata) \
> ((acpi_hest_generic_data_version(gdata) >= 3) ? \
> sizeof(struct acpi_hest_generic_data_v300) : \
> sizeof(struct acpi_hest_generic_data))
> #define acpi_hest_generic_data_record_size(gdata)
> (acpi_hest_generic_data_size(gdata) + \
> acpi_hest_generic_data_error_length(gdata))
> #define acpi_hest_generic_data_next(gdata) \
> ((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
>
>
> Suzuki
These helpers will definitely help consolidate this code. I will use
these in the next version to remove the code duplication here.
Thanks,
Tyler
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [arm-soc:to-build 1/3] arch/x86/include/asm/thread_info.h:174:11: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe
From: kbuild test robot @ 2016-10-12 22:06 UTC (permalink / raw)
To: linux-arm-kernel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git to-build
head: b0a8f2c6d232f3f8463955e5927c56dd057495b1
commit: 3ddcb978f68b765b3af38bf45bf4a1a4b0a87df1 [1/3] Revert "Disable "maybe-uninitialized" warning globally"
config: i386-randconfig-s1-201641 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
git checkout 3ddcb978f68b765b3af38bf45bf4a1a4b0a87df1
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/thread_info.h:69:0,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:59,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/gfp.h:5,
from include/linux/mm.h:9,
from mm/usercopy.c:17:
arch/x86/include/asm/thread_info.h: In function 'check_stack_object':
>> arch/x86/include/asm/thread_info.h:174:11: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
oldframe = __builtin_frame_address(1);
~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/include/asm/thread_info.h:176:9: warning: calling '__builtin_frame_address' with a nonzero argument is unsafe [-Wframe-address]
frame = __builtin_frame_address(2);
~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/__builtin_frame_address +174 arch/x86/include/asm/thread_info.h
0f60a8ef Kees Cook 2016-07-12 158 * Walks up the stack frames to make sure that the specified object is
0f60a8ef Kees Cook 2016-07-12 159 * entirely contained by a single stack frame.
0f60a8ef Kees Cook 2016-07-12 160 *
0f60a8ef Kees Cook 2016-07-12 161 * Returns:
0f60a8ef Kees Cook 2016-07-12 162 * 1 if within a frame
0f60a8ef Kees Cook 2016-07-12 163 * -1 if placed across a frame boundary (or outside stack)
0f60a8ef Kees Cook 2016-07-12 164 * 0 unable to determine (no frame pointers, etc)
0f60a8ef Kees Cook 2016-07-12 165 */
0f60a8ef Kees Cook 2016-07-12 166 static inline int arch_within_stack_frames(const void * const stack,
0f60a8ef Kees Cook 2016-07-12 167 const void * const stackend,
0f60a8ef Kees Cook 2016-07-12 168 const void *obj, unsigned long len)
0f60a8ef Kees Cook 2016-07-12 169 {
0f60a8ef Kees Cook 2016-07-12 170 #if defined(CONFIG_FRAME_POINTER)
0f60a8ef Kees Cook 2016-07-12 171 const void *frame = NULL;
0f60a8ef Kees Cook 2016-07-12 172 const void *oldframe;
0f60a8ef Kees Cook 2016-07-12 173
0f60a8ef Kees Cook 2016-07-12 @174 oldframe = __builtin_frame_address(1);
0f60a8ef Kees Cook 2016-07-12 175 if (oldframe)
0f60a8ef Kees Cook 2016-07-12 176 frame = __builtin_frame_address(2);
0f60a8ef Kees Cook 2016-07-12 177 /*
0f60a8ef Kees Cook 2016-07-12 178 * low ----------------------------------------------> high
0f60a8ef Kees Cook 2016-07-12 179 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
0f60a8ef Kees Cook 2016-07-12 180 * ^----------------^
0f60a8ef Kees Cook 2016-07-12 181 * allow copies only within here
0f60a8ef Kees Cook 2016-07-12 182 */
:::::: The code at line 174 was first introduced by commit
:::::: 0f60a8efe4005ab5e65ce000724b04d4ca04a199 mm: Implement stack frame object validation
:::::: TO: Kees Cook <keescook@chromium.org>
:::::: CC: Kees Cook <keescook@chromium.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 24527 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161013/8e639a65/attachment-0001.gz>
^ permalink raw reply
* [PATCH V3 09/10] trace, ras: add ARM processor error trace event
From: Baicar, Tyler @ 2016-10-12 21:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161007173959.5ff3fcfa@gandalf.local.home>
Hello Steve,
Thank you for your feedback! Responses below.
On 10/7/2016 3:39 PM, Steven Rostedt wrote:
> On Fri, 7 Oct 2016 15:31:21 -0600
> Tyler Baicar <tbaicar@codeaurora.org> wrote:
>
>> Currently there are trace events for the various RAS
>> errors with the exception of ARM processor type errors.
>> Add a new trace event for such errors so that the user
>> will know when they occur. These trace events are
>> consistent with the ARM processor error section type
>> defined in UEFI 2.6 spec section N.2.4.4.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>> drivers/firmware/efi/cper.c | 9 ++++++
>> drivers/ras/ras.c | 1 +
>> include/ras/ras_event.h | 67 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 77 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
>> index f9ffba6..21b8a6f 100644
>> --- a/drivers/firmware/efi/cper.c
>> +++ b/drivers/firmware/efi/cper.c
>> @@ -34,6 +34,7 @@
>> #include <linux/aer.h>
>> #include <linux/printk.h>
>> #include <linux/bcd.h>
>> +#include <ras/ras_event.h>
>>
>> #define INDENT_SP " "
>>
>> @@ -256,6 +257,14 @@ static void cper_print_proc_armv8(const char *pfx,
>> CPER_ARMV8_INFO_VALID_PHYSICAL_ADDR)
>> printk("%sphysical fault address: 0x%016llx\n",
>> newpfx, err_info->physical_fault_addr);
>> + trace_arm_event(proc->affinity_level, proc->mpidr, proc->midr,
>> + proc->running_state, proc->psci_state,
>> + err_info->version, err_info->type,
>> + err_info->multiple_error,
>> + err_info->validation_bits,
>> + err_info->error_info,
>> + err_info->virt_fault_addr,
>> + err_info->physical_fault_addr);
> Why waste all the effort into passing each individual field. Why not
> just pass the structure in and sort it out in the TP_fast_assign()?
That should be a lot cleaner, I will make that change in the next patchset.
>> err_info += 1;
>> }
>>
>> diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
>> index fb2500b..8ba5a94 100644
>> --- a/drivers/ras/ras.c
>> +++ b/drivers/ras/ras.c
>> @@ -28,3 +28,4 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
>> #endif
>> EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
>> EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
>> +EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
>> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
>> index 5861b6f..eb2719a 100644
>> --- a/include/ras/ras_event.h
>> +++ b/include/ras/ras_event.h
>> @@ -162,6 +162,73 @@ TRACE_EVENT(mc_event,
>> );
>>
>> /*
>> + * ARM Processor Events Report
>> + *
>> + * This event is generated when hardware detects an ARM processor error
>> + * has occurred. UEFI 2.6 spec section N.2.4.4.
>> + */
>> +TRACE_EVENT(arm_event,
>> +
>> + TP_PROTO(const u8 affinity,
>> + const u64 mpidr,
>> + const u64 midr,
>> + const u32 running_state,
>> + const u32 psci_state,
>> + const u8 version,
>> + const u8 type,
>> + const u16 err_count,
>> + const u8 flags,
>> + const u64 info,
>> + const u64 virt_fault_addr,
>> + const u64 phys_fault_addr),
>> +
>> + TP_ARGS(affinity, mpidr, midr, running_state, psci_state,
>> + version, type, err_count, flags, info, virt_fault_addr,
>> + phys_fault_addr),
>> +
>> + TP_STRUCT__entry(
>> + __field(u8, affinity)
>> + __field(u64, mpidr)
>> + __field(u64, midr)
>> + __field(u32, running_state)
>> + __field(u32, psci_state)
>> + __field(u8, version)
>> + __field(u8, type)
>> + __field(u16, err_count)
>> + __field(u8, flags)
>> + __field(u64, info)
>> + __field(u64, virt_fault_addr)
>> + __field(u64, phys_fault_addr)
> The above creates a structure with lots of holes in it. Pack it better.
> You want something like:
>
> __field(u64, mpidr)
> __field(u64, midr)
> __field(u64, info)
> __field(u64, virt_fault_addr)
> __field(u64, phys_fault_addr)
> __field(u32, running_state)
> __field(u32, psci_state)
> __field(u16, err_count)
> __field(u8, affinity)
> __field(u8, version)
> __field(u8, type)
> __field(u8, flags)
>
> The above is a total of 54 bytes. Your original was at a minimum, 64
> bytes.
>
> -- Steve
I will reorder the structure in the next patchset. I originally used
this order because that is the order the entries appear in the spec
(table 260 and 261 of UEFI spec 2.6). It makes more sense to save the
space though.
Thanks,
Tyler
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->affinity = affinity;
>> + __entry->mpidr = mpidr;
>> + __entry->midr = midr;
>> + __entry->running_state = running_state;
>> + __entry->psci_state = psci_state;
>> + __entry->version = version;
>> + __entry->type = type;
>> + __entry->err_count = err_count;
>> + __entry->flags = flags;
>> + __entry->info = info;
>> + __entry->virt_fault_addr = virt_fault_addr;
>> + __entry->phys_fault_addr = phys_fault_addr;
>> + ),
>> +
>> + TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
>> + "running state: %d; PSCI state: %d; version: %d; type: %d; "
>> + "error count: 0x%04x; flags: 0x%02x; info: %016llx; "
>> + "virtual fault address: %016llx; "
>> + "physical fault address: %016llx",
>> + __entry->affinity, __entry->mpidr, __entry->midr,
>> + __entry->running_state, __entry->psci_state, __entry->version,
>> + __entry->type, __entry->err_count, __entry->flags,
>> + __entry->info, __entry->virt_fault_addr,
>> + __entry->phys_fault_addr)
>> +);
>> +
>> +/*
>> * Unknown Section Report
>> *
>> * This event is generated when hardware detected a hardware
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative
From: Maxime Ripard @ 2016-10-12 21:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161003181840.0cb369d8@bbrezillon>
On Mon, Oct 03, 2016 at 06:18:40PM +0200, Boris Brezillon wrote:
> On Mon, 3 Oct 2016 14:58:11 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>
> > Hi Boris,
> >
> > On Fri, Sep 30, 2016 at 06:08:26PM +0200, Boris Brezillon wrote:
> > > On Fri, 30 Sep 2016 16:33:20 +0200
> > > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > >
> > > > Our planes cannot be set at negative coordinates. Make sure we reject such
> > > > configuration.
> > > >
> > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > ---
> > > > drivers/gpu/drm/sun4i/sun4i_layer.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > index f0035bf5efea..f5463c4c2cde 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > @@ -29,6 +29,9 @@ struct sun4i_plane_desc {
> > > > static int sun4i_backend_layer_atomic_check(struct drm_plane *plane,
> > > > struct drm_plane_state *state)
> > > > {
> > > > + if ((state->crtc_x < 0) || (state->crtc_y < 0))
> > > > + return -EINVAL;
> > > > +
> > >
> > > Hm, I think it's a perfectly valid use case from the DRM framework and
> > > DRM user PoV: you may want to place your plane at a negative CRTC
> > > offset (which means part of the plane will be hidden).
> > >
> > > Maybe I'm wrong, but it seems you can support that by adapting the
> > > start address of your framebuffer pointer and the layer size.
> >
> > Indeed, that would probably work. This is even somewhat what we've
> > been using to implement the VGA hack we use on the CHIP.
> >
> > Can you send that patch?
>
> Actually, Ville suggested a slightly different approach: use the
> ->src and ->dst in drm_plane_state.
After more tests, it turns out that it's actually simpler than
that....
The hardware is able to take negative coordinates.
Sorry for the noise,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161012/14a89d77/attachment.sig>
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: bdegraaf at codeaurora.org @ 2016-10-12 20:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a0aa2dba916aafb188f8b1b11aca1599@codeaurora.org>
On 2016-10-05 11:30, bdegraaf at codeaurora.org wrote:
> On 2016-10-05 11:10, Peter Zijlstra wrote:
>> On Wed, Oct 05, 2016 at 10:55:57AM -0400, bdegraaf at codeaurora.org
>> wrote:
>>> On 2016-10-04 15:12, Mark Rutland wrote:
>>> >Hi Brent,
>>> >
>>> >Could you *please* clarify if you are trying to solve:
>>> >
>>> >(a) a correctness issue (e.g. data corruption) seen in practice.
>>> >(b) a correctness issue (e.g. data corruption) found by inspection.
>>> >(c) A performance issue, seen in practice.
>>> >(d) A performance issue, found by inspection.
>>> >
>>> >Any one of these is fine; we just need to know in order to be able to
>>> >help effectively, and so far it hasn't been clear.
>>
>> Brent, you forgot to state which: 'a-d' is the case here.
>>
>>> I found the problem.
>>>
>>> Back in September of 2013, arm64 atomics were broken due to missing
>>> barriers
>>> in certain situations, but the problem at that time was undiscovered.
>>>
>>> Will Deacon's commit d2212b4dce596fee83e5c523400bf084f4cc816c went in
>>> at
>>> that
>>> time and changed the correct cmpxchg64 in lockref.c to
>>> cmpxchg64_relaxed.
>>>
>>> d2212b4 appeared to be OK at that time because the additional barrier
>>> requirements of this specific code sequence were not yet discovered,
>>> and
>>> this change was consistent with the arm64 atomic code of that time.
>>>
>>> Around February of 2014, some discovery led Will to correct the
>>> problem with
>>> the atomic code via commit 8e86f0b409a44193f1587e87b69c5dcf8f65be67,
>>> which
>>> has an excellent explanation of potential ordering problems with the
>>> same
>>> code sequence used by lockref.c.
>>>
>>> With this updated understanding, the earlier commit
>>> (d2212b4dce596fee83e5c523400bf084f4cc816c) should be reverted.
>>>
>>> Because acquire/release semantics are insufficient for the full
>>> ordering,
>>> the single barrier after the store exclusive is the best approach,
>>> similar
>>> to Will's atomic barrier fix.
>>
>> This again does not in fact describe the problem.
>>
>> What is the problem with lockref, and how (refer the earlier a-d
>> multiple choice answer) was this found.
>>
>> Now, I have been looking, and we have some idea what you _might_ be
>> alluding to, but please explain which accesses get reordered how and
>> cause problems.
>
> Sorry for the confusion, this was a "b" item (correctness fix based on
> code
> inspection. I had sent an answer to this yesterday, but didn't realize
> that
> it was in a separate, private email thread.
>
> I'll work out the before/after problem scenarios and send them along
> once
> I've hashed them out (it may take a while for me to paint a clear
> picture).
> In the meantime, however, consider that even without the spinlock code
> in
> the picture, lockref needs to treat the cmpxchg as a full system-level
> atomic,
> because multiple agents could access the value in a variety of timings.
> Since
> atomics similar to this are barriered on arm64 since 8e86f0b, the
> access to
> lockref should be similar.
>
> Brent
I am still working through some additional analyses for mixed accesses,
but I
thought I'd send along some sample commit text for the fix as it
currently stands.
Please feel free to comment if you see something that needs
clarification.
Brent
Text:
All arm64 lockref accesses that occur without taking the spinlock must
behave like
true atomics, ensuring successive operations are all done sequentially.
Currently
the lockref accesses, when decompiled, look like the following sequence:
<Lockref "unlocked" Access [A]>
// Lockref "unlocked" (B)
1: ldxr x0, [B] // Exclusive load
<change lock_count B>
stxr w1, x0, [B]
cbnz w1, 1b
<Lockref "unlocked" Access [C]>
Even though access to the lock_count is protected by exclusives, this is
not enough
to guarantee order: The lock_count must change atomically, in order, so
the only
permitted ordering would be:
A -> B -> C
Unfortunately, this is not the case by the letter of the architecture
and, in fact,
the accesses to A and C are not protected by any sort of barrier, and
hence are
permitted to reorder freely, resulting in orderings such as
Bl -> A -> C -> Bs
In this specific scenario, since "change lock_count" could be an
increment, a decrement
or even a set to a specific value, there could be trouble. With more
agents accessing
the lockref without taking the lock, even scenarios where the cmpxchg
passes falsely
can be encountered, as there is no guarantee that the the "old" value
will not match
exactly a newer value due to out-of-order access by a combination of
agents that
increment and decrement the lock_count by the same amount.
Since multiple agents are accessing this without locking the spinlock,
this access
must have the same protections in place as atomics do in the arch's
atomic.h.
Fortunately, the fix is not complicated: merely removing the errant
_relaxed option
on the cmpxchg64 is enough to introduce exactly the same code sequence
justified
in commit 8e86f0b409a44193f1587e87b69c5dcf8f65be67 to fix arm64 atomics.
1: ldxr x0, [B]
<change lock_count>
stlxr w1, x0, [B]
cbnz w1, 1b
dmb ish
^ permalink raw reply
* [PATCH V2 2/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts
From: Bjorn Helgaas @ 2016-10-12 19:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475615720-31047-3-git-send-email-okaya@codeaurora.org>
On Tue, Oct 04, 2016 at 05:15:18PM -0400, Sinan Kaya wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI and IRQ interrupts.
>
> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.
>
> Tested-by: Jonathan Liu <net147@gmail.com>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
> drivers/acpi/pci_link.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index f3792f4..06c2a11 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -620,6 +620,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
> acpi_device_bid(link->device));
> return -ENODEV;
> } else {
> + if (link->irq.active < ACPI_MAX_IRQS)
> + acpi_irq_penalty[link->irq.active] +=
> + PIRQ_PENALTY_PCI_USING;
Nit: elsewhere you use ARRAY_SIZE(acpi_irq_penalty), so it'd be nice
to be consistent, e.g.,
if (link->irq.active < ARRAY_SIZE(acpi_irq_penalty))
acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING;
There are a couple other similar uses in acpi_irq_get_penalty() and
acpi_irq_penalty_init().
> printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
> acpi_device_name(link->device),
> acpi_device_bid(link->device), link->irq.active);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] arm64: defconfig: enable EEPROM_AT25 config option
From: Scott Branden @ 2016-10-12 18:51 UTC (permalink / raw)
To: linux-arm-kernel
Enable support for on board SPI EEPROM by turning on
CONFIG_EEPROM_AT25. This needs to be on in order to
boot and test the kernel with a static rootfs image
that is not rebuilt everytime the kernel is rebuilt.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index eadf485..9955ee1 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -136,6 +136,7 @@ CONFIG_MTD_SPI_NOR=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_BLK_DEV_NBD=m
CONFIG_VIRTIO_BLK=y
+CONFIG_EEPROM_AT25=y
CONFIG_SRAM=y
# CONFIG_SCSI_PROC_FS is not set
CONFIG_BLK_DEV_SD=y
--
2.5.0
^ permalink raw reply related
* [PATCH 2/2] mmc: sdhci-iproc: support standard byte register accesses
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476297352-7812-1-git-send-email-scott.branden@broadcom.com>
Add bytewise register accesses support for newer versions of IPROC
SDHCI controllers.
Previous sdhci-iproc versions of SDIO controllers
(such as Raspberry Pi and Cygnus) only allowed for 32-bit register
accesses.
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
drivers/mmc/host/sdhci-iproc.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 7262466..d7046d6 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -143,6 +143,14 @@ static void sdhci_iproc_writeb(struct sdhci_host *host, u8 val, int reg)
}
static const struct sdhci_ops sdhci_iproc_ops = {
+ .set_clock = sdhci_set_clock,
+ .get_max_clock = sdhci_pltfm_clk_get_max_clock,
+ .set_bus_width = sdhci_set_bus_width,
+ .reset = sdhci_reset,
+ .set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static const struct sdhci_ops sdhci_iproc_32only_ops = {
.read_l = sdhci_iproc_readl,
.read_w = sdhci_iproc_readw,
.read_b = sdhci_iproc_readb,
@@ -156,6 +164,28 @@ static const struct sdhci_ops sdhci_iproc_ops = {
.set_uhs_signaling = sdhci_set_uhs_signaling,
};
+static const struct sdhci_pltfm_data sdhci_iproc_cygnus_pltfm_data = {
+ .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+ .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+ .ops = &sdhci_iproc_32only_ops,
+};
+
+static const struct sdhci_iproc_data iproc_cygnus_data = {
+ .pdata = &sdhci_iproc_cygnus_pltfm_data,
+ .caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)
+ & SDHCI_MAX_BLOCK_MASK) |
+ SDHCI_CAN_VDD_330 |
+ SDHCI_CAN_VDD_180 |
+ SDHCI_CAN_DO_SUSPEND |
+ SDHCI_CAN_DO_HISPD |
+ SDHCI_CAN_DO_ADMA2 |
+ SDHCI_CAN_DO_SDMA,
+ .caps1 = SDHCI_DRIVER_TYPE_C |
+ SDHCI_DRIVER_TYPE_D |
+ SDHCI_SUPPORT_DDR50,
+ .mmc_caps = MMC_CAP_1_8V_DDR,
+};
+
static const struct sdhci_pltfm_data sdhci_iproc_pltfm_data = {
.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
@@ -182,7 +212,7 @@ static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
SDHCI_QUIRK_MISSING_CAPS,
- .ops = &sdhci_iproc_ops,
+ .ops = &sdhci_iproc_32only_ops,
};
static const struct sdhci_iproc_data bcm2835_data = {
@@ -194,7 +224,8 @@ static const struct sdhci_iproc_data bcm2835_data = {
static const struct of_device_id sdhci_iproc_of_match[] = {
{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
- { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
+ { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_cygnus_data},
+ { .compatible = "brcm,sdhci-iproc", .data = &iproc_data },
{ }
};
MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
--
2.5.0
^ permalink raw reply related
* [PATCH 1/2] mmc: sdhci-iproc: Add brcm, sdhci-iproc compat string in bindings document
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476297352-7812-1-git-send-email-scott.branden@broadcom.com>
Adds brcm,sdhci-iproc compat string to DT bindings document for
the iProc SDHCI driver.
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
index be56d2b..aa58b94 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -7,6 +7,7 @@ Required properties:
- compatible : Should be one of the following
"brcm,bcm2835-sdhci"
"brcm,sdhci-iproc-cygnus"
+ "brcm,sdhci-iproc"
- clocks : The clock feeding the SDHCI controller.
--
2.5.0
^ permalink raw reply related
* [PATCH 0/2] mmc: sdhci-iproc: Add byte register access support
From: Scott Branden @ 2016-10-12 18:35 UTC (permalink / raw)
To: linux-arm-kernel
Add brcm,sdhci-iproc compat string and code for support of newer versions of
sdhci-iproc controller that allow byte-wise register accesses.
Scott Branden (2):
mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings
document
mmc: sdhci-iproc: support standard byte register accesses
.../devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 1 +
drivers/mmc/host/sdhci-iproc.c | 35 ++++++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
--
2.5.0
^ permalink raw reply
* [PATCH V3 05/10] acpi: apei: handle SEA notification type for ARMv8
From: Punit Agrawal @ 2016-10-12 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475875882-2604-6-git-send-email-tbaicar@codeaurora.org>
Tyler Baicar <tbaicar@codeaurora.org> writes:
> ARM APEI extension proposal added SEA (Synchrounous External
> Abort) notification type for ARMv8.
> Add a new GHES error source handling function for SEA. If an error
> source's notification type is SEA, then this function can be registered
> into the SEA exception handler. That way GHES will parse and report
> SEA exceptions when they occur.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
This patch fails to apply for me on v4.8. Is there a different tree this
is based on?
One comment below.
[...]
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index c8488f1..28d5a09 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -50,6 +50,10 @@
> #include <acpi/apei.h>
> #include <asm/tlbflush.h>
>
> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
> +#include <asm/system_misc.h>
> +#endif
> +
> #include "apei-internal.h"
>
> #define GHES_PFX "GHES: "
> @@ -779,6 +783,62 @@ static struct notifier_block ghes_notifier_sci = {
> .notifier_call = ghes_notify_sci,
> };
>
> +#ifdef CONFIG_HAVE_ACPI_APEI_SEA
> +static LIST_HEAD(ghes_sea);
> +
> +static int ghes_notify_sea(struct notifier_block *this,
> + unsigned long event, void *data)
> +{
> + struct ghes *ghes;
> + int ret = NOTIFY_DONE;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(ghes, &ghes_sea, list) {
> + if (!ghes_proc(ghes))
> + ret = NOTIFY_OK;
Not something you've introduced but it looks like ghes_proc erroneously
never returns anything other than 0. I plan to post the below fix to
address it.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 60746ef..caea575 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -662,7 +662,7 @@ static int ghes_proc(struct ghes *ghes)
ghes_do_proc(ghes, ghes->estatus);
out:
ghes_clear_estatus(ghes);
- return 0;
+ return rc;
}
> + }
> + rcu_read_unlock();
> +
> + return ret;
> +}
> +
[...]
^ permalink raw reply related
* [PATCH V3 04/10] arm64: exception: handle Synchronous External Abort
From: Punit Agrawal @ 2016-10-12 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475875882-2604-5-git-send-email-tbaicar@codeaurora.org>
Hi Tyler,
A couple of hopefully not bike shedding comments below.
Tyler Baicar <tbaicar@codeaurora.org> writes:
> SEA exceptions are often caused by an uncorrected hardware
> error, and are handled when data abort and instruction abort
> exception classes have specific values for their Fault Status
> Code.
> When SEA occurs, before killing the process, go through
> the handlers registered in the notification list.
> Update fault_info[] with specific SEA faults so that the
> new SEA handler is used.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
> arch/arm64/include/asm/system_misc.h | 13 ++++++++
> arch/arm64/mm/fault.c | 58 +++++++++++++++++++++++++++++-------
> 2 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index 57f110b..90daf4a 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>
> #endif /* __ASSEMBLY__ */
>
> +/*
> + * The functions below are used to register and unregister callbacks
> + * that are to be invoked when a Synchronous External Abort (SEA)
> + * occurs. An SEA is raised by certain fault status codes that have
> + * either data or instruction abort as the exception class, and
> + * callbacks may be registered to parse or handle such hardware errors.
> + *
> + * Registered callbacks are run in an interrupt/atomic context. They
> + * are not allowed to block or sleep.
> + */
> +int sea_register_handler_chain(struct notifier_block *nb);
> +void sea_unregister_handler_chain(struct notifier_block *nb);
> +
> #endif /* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 05d2bd7..81cb7ad 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -39,6 +39,22 @@
> #include <asm/pgtable.h>
> #include <asm/tlbflush.h>
>
> +/*
> + * GHES SEA handler code may register a notifier call here to
> + * handle HW error record passed from platform.
> + */
> +static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
> +
> +int sea_register_handler_chain(struct notifier_block *nb)
> +{
> + return atomic_notifier_chain_register(&sea_handler_chain, nb);
> +}
> +
> +void sea_unregister_handler_chain(struct notifier_block *nb)
> +{
> + atomic_notifier_chain_unregister(&sea_handler_chain, nb);
> +}
> +
What do you think of naming the above functions as
[un]register_synchonous_ext_abort_notifier?
For an API, I find "sea" doesn't quite convey the message.
One more comment below.
> static const char *fault_name(unsigned int esr);
>
> #ifdef CONFIG_KPROBES
> @@ -480,6 +496,28 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> return 1;
> }
>
> +/*
> + * This abort handler deals with Synchronous External Abort.
> + * It calls notifiers, and then returns "fault".
> + */
> +static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
> +{
> + struct siginfo info;
> +
> + atomic_notifier_call_chain(&sea_handler_chain, 0, NULL);
> +
> + pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
> + fault_name(esr), esr, addr);
> +
> + info.si_signo = SIGBUS;
> + info.si_errno = 0;
> + info.si_code = 0;
> + info.si_addr = (void __user *)addr;
> + arm64_notify_die("", regs, &info, esr);
> +
> + return 0;
> +}
> +
> static const struct fault_info {
> int (*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
> int sig;
> @@ -502,22 +540,22 @@ static const struct fault_info {
> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 1 permission fault" },
> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 2 permission fault" },
> { do_page_fault, SIGSEGV, SEGV_ACCERR, "level 3 permission fault" },
> - { do_bad, SIGBUS, 0, "synchronous external abort" },
> + { do_sea, SIGBUS, 0, "synchronous external abort" },
> { do_bad, SIGBUS, 0, "unknown 17" },
> { do_bad, SIGBUS, 0, "unknown 18" },
> { do_bad, SIGBUS, 0, "unknown 19" },
> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
> - { do_bad, SIGBUS, 0, "synchronous abort (translation table walk)" },
> - { do_bad, SIGBUS, 0, "synchronous parity error" },
> + { do_sea, SIGBUS, 0, "level 0 SEA (trans tbl walk)" },
> + { do_sea, SIGBUS, 0, "level 1 SEA (trans tbl walk)" },
> + { do_sea, SIGBUS, 0, "level 2 SEA (trans tbl walk)" },
> + { do_sea, SIGBUS, 0, "level 3 SEA (trans tbl walk)" },
^^^
The comment about naming applies here as well.
Thanks,
Punit
[...]
^ permalink raw reply
* [PATCH] perf: xgene: Remove bogus IS_ERR() check
From: Tai Nguyen @ 2016-10-12 16:39 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes the warning issue with static checker.
The bug is reported in [1]
[1] https://www.spinics.net/lists/arm-kernel/msg535957.html
Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
---
drivers/perf/xgene_pmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index c2ac764..a8ac4bc 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1011,7 +1011,7 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
rc = acpi_dev_get_resources(adev, &resource_list,
acpi_pmu_dev_add_resource, &res);
acpi_dev_free_resource_list(&resource_list);
- if (rc < 0 || IS_ERR(&res)) {
+ if (rc < 0) {
dev_err(dev, "PMU type %d: No resource address found\n", type);
goto err;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v2 0/2] PCI: aardvark: Cleanup
From: Bjorn Helgaas @ 2016-10-12 16:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161012123001.22186.4053.stgit@bhelgaas-glaptop2.roam.corp.google.com>
On Wed, Oct 12, 2016 at 07:36:21AM -0500, Bjorn Helgaas wrote:
> Add local "dev" pointers to reduce repetition of things like "&pdev->dev"
> and remove unused drvdata.
>
> Changes from v1:
> I dropped the following because they added a lot of churn for
> questionable benefit:
> PCI: aardvark: Name private struct pointer "advk" consistently
> PCI: aardvark: Reorder accessor functions
> PCI: aardvark: Swap order of advk_write() reg/val arguments
>
> ---
>
> Bjorn Helgaas (2):
> PCI: aardvark: Add local struct device pointers
> PCI: aardvark: Remove unused platform data
I applied these to pci/host-aardvark for v4.9. I hope to ask Linus to
pull them tomorrow, so if you see any issues, let me know soon.
^ permalink raw reply
* [PATCH V3 01/10] acpi: apei: read ack upon ghes record consumption
From: Punit Agrawal @ 2016-10-12 15:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475875882-2604-2-git-send-email-tbaicar@codeaurora.org>
Hi Tyler,
A few comments below.
Tyler Baicar <tbaicar@codeaurora.org> writes:
> A RAS (Reliability, Availability, Serviceability) controller
> may be a separate processor running in parallel with OS
> execution, and may generate error records for consumption by
> the OS. If the RAS controller produces multiple error records,
> then they may be overwritten before the OS has consumed them.
>
> The Generic Hardware Error Source (GHES) v2 structure
> introduces the capability for the OS to acknowledge the
> consumption of the error record generated by the RAS
> controller. A RAS controller supporting GHESv2 shall wait for
> the acknowledgment before writing a new error record, thus
> eliminating the race condition.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Richard Ruigrok <rruigrok@codeaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
> drivers/acpi/apei/ghes.c | 41 +++++++++++++++++++++++++++++++++++++++++
> drivers/acpi/apei/hest.c | 7 +++++--
> include/acpi/ghes.h | 1 +
> 3 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 60746ef..3021f0e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -45,6 +45,7 @@
> #include <linux/aer.h>
> #include <linux/nmi.h>
>
> +#include <acpi/actbl1.h>
> #include <acpi/ghes.h>
> #include <acpi/apei.h>
> #include <asm/tlbflush.h>
> @@ -244,10 +245,22 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
> struct ghes *ghes;
> unsigned int error_block_length;
> int rc;
> + struct acpi_hest_header *hest_hdr;
>
> ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
> if (!ghes)
> return ERR_PTR(-ENOMEM);
> +
> + hest_hdr = (struct acpi_hest_header *)generic;
> + if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2) {
> + ghes->generic_v2 = (struct acpi_hest_generic_v2 *)generic;
> + rc = apei_map_generic_address(
> + &ghes->generic_v2->read_ack_register);
> + if (rc)
> + goto err_unmap;
> + } else
> + ghes->generic_v2 = NULL;
Since you kzalloc ghes, shouldn't ghes->generic_v2 be NULL already?
> +
> ghes->generic = generic;
> rc = apei_map_generic_address(&generic->error_status_address);
> if (rc)
> @@ -270,6 +283,9 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>
> err_unmap:
> apei_unmap_generic_address(&generic->error_status_address);
> + if (ghes->generic_v2)
> + apei_unmap_generic_address(
> + &ghes->generic_v2->read_ack_register);
> err_free:
> kfree(ghes);
> return ERR_PTR(rc);
> @@ -279,6 +295,9 @@ static void ghes_fini(struct ghes *ghes)
> {
> kfree(ghes->estatus);
> apei_unmap_generic_address(&ghes->generic->error_status_address);
> + if (ghes->generic_v2)
> + apei_unmap_generic_address(
> + &ghes->generic_v2->read_ack_register);
> }
>
> static inline int ghes_severity(int severity)
> @@ -648,6 +667,22 @@ static void ghes_estatus_cache_add(
> rcu_read_unlock();
> }
>
> +static int ghes_do_read_ack(struct acpi_hest_generic_v2 *generic_v2)
> +{
> + int rc;
> + u64 val = 0;
> +
> + rc = apei_read(&val, &generic_v2->read_ack_register);
> + if (rc)
> + return rc;
> + val &= generic_v2->read_ack_preserve <<
> + generic_v2->read_ack_register.bit_offset;
> + val |= generic_v2->read_ack_write;
Reading the spec, it is not clear whether you need the left shift
above.
Having said that, if you do it for read_ack_preserve, do you also need
to left shift read_ack_write by read_ack_register.bit_offset?
> + rc = apei_write(val, &generic_v2->read_ack_register);
> +
> + return rc;
> +}
> +
> static int ghes_proc(struct ghes *ghes)
> {
> int rc;
> @@ -660,6 +695,12 @@ static int ghes_proc(struct ghes *ghes)
> ghes_estatus_cache_add(ghes->generic, ghes->estatus);
> }
> ghes_do_proc(ghes, ghes->estatus);
> +
> + if (ghes->generic_v2) {
> + rc = ghes_do_read_ack(ghes->generic_v2);
> + if (rc)
> + return rc;
> + }
> out:
> ghes_clear_estatus(ghes);
> return 0;
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 792a0d9..ef725a9 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -52,6 +52,7 @@ static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = {
> [ACPI_HEST_TYPE_AER_ENDPOINT] = sizeof(struct acpi_hest_aer),
> [ACPI_HEST_TYPE_AER_BRIDGE] = sizeof(struct acpi_hest_aer_bridge),
> [ACPI_HEST_TYPE_GENERIC_ERROR] = sizeof(struct acpi_hest_generic),
> + [ACPI_HEST_TYPE_GENERIC_ERROR_V2] = sizeof(struct acpi_hest_generic_v2),
> };
>
> static int hest_esrc_len(struct acpi_hest_header *hest_hdr)
> @@ -146,7 +147,8 @@ static int __init hest_parse_ghes_count(struct acpi_hest_header *hest_hdr, void
> {
> int *count = data;
>
> - if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR)
> + if (hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR ||
> + hest_hdr->type == ACPI_HEST_TYPE_GENERIC_ERROR_V2)
> (*count)++;
> return 0;
> }
> @@ -157,7 +159,8 @@ static int __init hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
> struct ghes_arr *ghes_arr = data;
> int rc, i;
>
> - if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR)
> + if (hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR &&
> + hest_hdr->type != ACPI_HEST_TYPE_GENERIC_ERROR_V2)
> return 0;
>
> if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 720446c..d0108b6 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -14,6 +14,7 @@
>
> struct ghes {
> struct acpi_hest_generic *generic;
> + struct acpi_hest_generic_v2 *generic_v2;
You either have a GHES or a GHESv2 structure. Instead of duplication,
could this be represented as a union?
Thanks,
Punit
> struct acpi_hest_generic_status *estatus;
> u64 buffer_paddr;
> unsigned long flags;
^ permalink raw reply
* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
From: Tai Tri Nguyen @ 2016-10-12 15:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161012152326.GB21489@remoulade>
Hi Mark,
Sure I'm working on it.
Regards,
Tai
On Wed, Oct 12, 2016 at 8:23 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Dan,
>
> Thanks for the report.
>
> On Wed, Oct 12, 2016 at 02:32:29PM +0300, Dan Carpenter wrote:
>> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
>> Monitoring Unit driver" from Jul 15, 2016, leads to the following
>> static checker warning:
>>
>> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
>> warn: '&res' isn't an ERR_PTR
>>
>> drivers/perf/xgene_pmu.c
>> 992 static struct
>> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
>> 994 struct acpi_device *adev, u32 type)
>> 995 {
>> 996 struct device *dev = xgene_pmu->dev;
>> 997 struct list_head resource_list;
>> 998 struct xgene_pmu_dev_ctx *ctx;
>> 999 const union acpi_object *obj;
>> 1000 struct hw_pmu_info *inf;
>> 1001 void __iomem *dev_csr;
>> 1002 struct resource res;
>> 1003 int enable_bit;
>> 1004 int rc;
>> 1005
>> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> 1007 if (!ctx)
>> 1008 return NULL;
>> 1009
>> 1010 INIT_LIST_HEAD(&resource_list);
>> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
>> 1012 acpi_pmu_dev_add_resource, &res);
>> 1013 acpi_dev_free_resource_list(&resource_list);
>> 1014 if (rc < 0 || IS_ERR(&res)) {
>> ^^^^
>> Obviously this is a stack address and not an error pointer. I'm not
>> sure what was intended here.
>
> Urrgh; I should have caught that in review. It's been this way since v1 [1], so
> I can't reverse-engineer the intent. However, I don't believe that it's
> necessary to check anything other than the rc value here.
>
> This should always evaluate as false, and shouldn't result in a functional
> problem, but it is completely bogus.
>
> Tai, can you plase send a patch deleting The IS_ERR() check here?
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/419173.html
--
Tai
^ permalink raw reply
* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
From: Mark Rutland @ 2016-10-12 15:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161012113229.GA16447@mwanda>
Hi Dan,
Thanks for the report.
On Wed, Oct 12, 2016 at 02:32:29PM +0300, Dan Carpenter wrote:
> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> Monitoring Unit driver" from Jul 15, 2016, leads to the following
> static checker warning:
>
> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
> warn: '&res' isn't an ERR_PTR
>
> drivers/perf/xgene_pmu.c
> 992 static struct
> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> 994 struct acpi_device *adev, u32 type)
> 995 {
> 996 struct device *dev = xgene_pmu->dev;
> 997 struct list_head resource_list;
> 998 struct xgene_pmu_dev_ctx *ctx;
> 999 const union acpi_object *obj;
> 1000 struct hw_pmu_info *inf;
> 1001 void __iomem *dev_csr;
> 1002 struct resource res;
> 1003 int enable_bit;
> 1004 int rc;
> 1005
> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> 1007 if (!ctx)
> 1008 return NULL;
> 1009
> 1010 INIT_LIST_HEAD(&resource_list);
> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
> 1012 acpi_pmu_dev_add_resource, &res);
> 1013 acpi_dev_free_resource_list(&resource_list);
> 1014 if (rc < 0 || IS_ERR(&res)) {
> ^^^^
> Obviously this is a stack address and not an error pointer. I'm not
> sure what was intended here.
Urrgh; I should have caught that in review. It's been this way since v1 [1], so
I can't reverse-engineer the intent. However, I don't believe that it's
necessary to check anything other than the rc value here.
This should always evaluate as false, and shouldn't result in a functional
problem, but it is completely bogus.
Tai, can you plase send a patch deleting The IS_ERR() check here?
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/419173.html
^ permalink raw reply
* [PATCH v3 2/5] arm64: mm: replace 'block_mappings_allowed' with 'page_mappings_only'
From: Mark Rutland @ 2016-10-12 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476271425-19401-3-git-send-email-ard.biesheuvel@linaro.org>
On Wed, Oct 12, 2016 at 12:23:42PM +0100, Ard Biesheuvel wrote:
> In preparation of adding support for contiguous PTE and PMD mappings,
> let's replace 'block_mappings_allowed' with 'page_mappings_only', which
> will be a more accurate description of the nature of the setting once we
> add such contiguous mappings into the mix.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Regardless of the contiguous bit stuff, I think this makes the code
clearer. As far as I can tell, this is correct. So FWIW:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
> ---
> arch/arm64/include/asm/mmu.h | 2 +-
> arch/arm64/kernel/efi.c | 8 ++---
> arch/arm64/mm/mmu.c | 32 ++++++++++----------
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index 8d9fce037b2f..a81454ad5455 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -34,7 +34,7 @@ extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
> extern void init_mem_pgprot(void);
> extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
> unsigned long virt, phys_addr_t size,
> - pgprot_t prot, bool allow_block_mappings);
> + pgprot_t prot, bool page_mappings_only);
> extern void *fixmap_remap_fdt(phys_addr_t dt_phys);
>
> #endif
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index ba9bee389fd5..5d17f377d905 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -62,8 +62,8 @@ struct screen_info screen_info __section(.data);
> int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> {
> pteval_t prot_val = create_mapping_protection(md);
> - bool allow_block_mappings = (md->type != EFI_RUNTIME_SERVICES_CODE &&
> - md->type != EFI_RUNTIME_SERVICES_DATA);
> + bool page_mappings_only = (md->type == EFI_RUNTIME_SERVICES_CODE ||
> + md->type == EFI_RUNTIME_SERVICES_DATA);
>
> if (!PAGE_ALIGNED(md->phys_addr) ||
> !PAGE_ALIGNED(md->num_pages << EFI_PAGE_SHIFT)) {
> @@ -76,12 +76,12 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> * from the MMU routines. So avoid block mappings altogether in
> * that case.
> */
> - allow_block_mappings = false;
> + page_mappings_only = true;
> }
>
> create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
> md->num_pages << EFI_PAGE_SHIFT,
> - __pgprot(prot_val | PTE_NG), allow_block_mappings);
> + __pgprot(prot_val | PTE_NG), page_mappings_only);
> return 0;
> }
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e1c34e5a1d7d..bf1d71b62c4f 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -139,7 +139,7 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
> static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
> phys_addr_t phys, pgprot_t prot,
> phys_addr_t (*pgtable_alloc)(void),
> - bool allow_block_mappings)
> + bool page_mappings_only)
> {
> pmd_t *pmd;
> unsigned long next;
> @@ -166,7 +166,7 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
>
> /* try section mapping first */
> if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
> - allow_block_mappings) {
> + !page_mappings_only) {
> pmd_set_huge(pmd, phys, prot);
>
> /*
> @@ -204,7 +204,7 @@ static inline bool use_1G_block(unsigned long addr, unsigned long next,
> static void alloc_init_pud(pgd_t *pgd, unsigned long addr, unsigned long end,
> phys_addr_t phys, pgprot_t prot,
> phys_addr_t (*pgtable_alloc)(void),
> - bool allow_block_mappings)
> + bool page_mappings_only)
> {
> pud_t *pud;
> unsigned long next;
> @@ -226,7 +226,7 @@ static void alloc_init_pud(pgd_t *pgd, unsigned long addr, unsigned long end,
> /*
> * For 4K granule only, attempt to put down a 1GB block
> */
> - if (use_1G_block(addr, next, phys) && allow_block_mappings) {
> + if (use_1G_block(addr, next, phys) && !page_mappings_only) {
> pud_set_huge(pud, phys, prot);
>
> /*
> @@ -238,7 +238,7 @@ static void alloc_init_pud(pgd_t *pgd, unsigned long addr, unsigned long end,
> ~modifiable_attr_mask) != 0);
> } else {
> alloc_init_pmd(pud, addr, next, phys, prot,
> - pgtable_alloc, allow_block_mappings);
> + pgtable_alloc, page_mappings_only);
>
> BUG_ON(pud_val(old_pud) != 0 &&
> pud_val(old_pud) != pud_val(*pud));
> @@ -253,7 +253,7 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
> unsigned long virt, phys_addr_t size,
> pgprot_t prot,
> phys_addr_t (*pgtable_alloc)(void),
> - bool allow_block_mappings)
> + bool page_mappings_only)
> {
> unsigned long addr, length, end, next;
> pgd_t *pgd = pgd_offset_raw(pgdir, virt);
> @@ -273,7 +273,7 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
> do {
> next = pgd_addr_end(addr, end);
> alloc_init_pud(pgd, addr, next, phys, prot, pgtable_alloc,
> - allow_block_mappings);
> + page_mappings_only);
> phys += next - addr;
> } while (pgd++, addr = next, addr != end);
> }
> @@ -302,17 +302,17 @@ static void __init create_mapping_noalloc(phys_addr_t phys, unsigned long virt,
> &phys, virt);
> return;
> }
> - __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot, NULL, true);
> + __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot, NULL, false);
> }
>
> void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
> unsigned long virt, phys_addr_t size,
> - pgprot_t prot, bool allow_block_mappings)
> + pgprot_t prot, bool page_mappings_only)
> {
> BUG_ON(mm == &init_mm);
>
> __create_pgd_mapping(mm->pgd, phys, virt, size, prot,
> - pgd_pgtable_alloc, allow_block_mappings);
> + pgd_pgtable_alloc, page_mappings_only);
> }
>
> static void create_mapping_late(phys_addr_t phys, unsigned long virt,
> @@ -325,7 +325,7 @@ static void create_mapping_late(phys_addr_t phys, unsigned long virt,
> }
>
> __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot,
> - NULL, !debug_pagealloc_enabled());
> + NULL, debug_pagealloc_enabled());
> }
>
> static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end)
> @@ -343,7 +343,7 @@ static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end
> __create_pgd_mapping(pgd, start, __phys_to_virt(start),
> end - start, PAGE_KERNEL,
> early_pgtable_alloc,
> - !debug_pagealloc_enabled());
> + debug_pagealloc_enabled());
> return;
> }
>
> @@ -356,13 +356,13 @@ static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end
> __phys_to_virt(start),
> kernel_start - start, PAGE_KERNEL,
> early_pgtable_alloc,
> - !debug_pagealloc_enabled());
> + debug_pagealloc_enabled());
> if (kernel_end < end)
> __create_pgd_mapping(pgd, kernel_end,
> __phys_to_virt(kernel_end),
> end - kernel_end, PAGE_KERNEL,
> early_pgtable_alloc,
> - !debug_pagealloc_enabled());
> + debug_pagealloc_enabled());
>
> /*
> * Map the linear alias of the [_text, __init_begin) interval as
> @@ -372,7 +372,7 @@ static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end
> */
> __create_pgd_mapping(pgd, kernel_start, __phys_to_virt(kernel_start),
> kernel_end - kernel_start, PAGE_KERNEL_RO,
> - early_pgtable_alloc, !debug_pagealloc_enabled());
> + early_pgtable_alloc, debug_pagealloc_enabled());
> }
>
> static void __init map_mem(pgd_t *pgd)
> @@ -422,7 +422,7 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
> BUG_ON(!PAGE_ALIGNED(size));
>
> __create_pgd_mapping(pgd, pa_start, (unsigned long)va_start, size, prot,
> - early_pgtable_alloc, !debug_pagealloc_enabled());
> + early_pgtable_alloc, debug_pagealloc_enabled());
>
> vma->addr = va_start;
> vma->phys_addr = pa_start;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v3 1/5] arm64: mm: BUG on unsupported manipulations of live kernel mappings
From: Catalin Marinas @ 2016-10-12 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476271425-19401-2-git-send-email-ard.biesheuvel@linaro.org>
On Wed, Oct 12, 2016 at 12:23:41PM +0100, Ard Biesheuvel wrote:
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -28,8 +28,6 @@
> #include <linux/memblock.h>
> #include <linux/fs.h>
> #include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/stop_machine.h>
>
> #include <asm/barrier.h>
> #include <asm/cputype.h>
> @@ -95,6 +93,12 @@ static phys_addr_t __init early_pgtable_alloc(void)
> return phys;
> }
>
> +/*
> + * The following mapping attributes may be updated in live
> + * kernel mappings without the need for break-before-make.
> + */
> +static const pteval_t modifiable_attr_mask = PTE_PXN | PTE_RDONLY | PTE_WRITE;
> +
> static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
> unsigned long end, unsigned long pfn,
> pgprot_t prot,
> @@ -115,8 +119,18 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
>
> pte = pte_set_fixmap_offset(pmd, addr);
> do {
> + pte_t old_pte = *pte;
> +
> set_pte(pte, pfn_pte(pfn, prot));
> pfn++;
> +
> + /*
> + * After the PTE entry has been populated once, we
> + * only allow updates to the permission attributes.
> + */
> + BUG_ON(pte_val(old_pte) != 0 &&
> + ((pte_val(old_pte) ^ pte_val(*pte)) &
> + ~modifiable_attr_mask) != 0);
Please turn this check into a single macro. You have it in three places
already (though with different types but a macro would do). Something
like below (feel free to come up with a better macro name):
BUG_ON(!safe_pgattr_change(old_pte, *pte));
--
Catalin
^ permalink raw reply
* [PATCH/RFT 07/12] USB: ohci-da8xx: Request gpios and handle interrupt in the driver
From: Axel Haslam @ 2016-10-12 15:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <22f1532a-b651-3330-b2ce-9a64a35c85af@lechnology.com>
Hi David
On Tue, Oct 11, 2016 at 1:18 AM, David Lechner <david@lechnology.com> wrote:
> On 10/07/2016 11:42 AM, ahaslam at baylibre.com wrote:
>>
>> From: Axel Haslam <ahaslam@baylibre.com>
>>
>> Currently requesting the vbus and overcurrent gpio is handled on
>> the board specific file. But this does not play well moving to
>> device tree.
>>
>> In preparation to migrate to a device tree boot, handle requesting
>> gpios and overcurrent interrupt on the usb driver itself, thus avoiding
>> callbacks to arch/mach*
>>
>
> Instead of using gpios, it seems like it would be better to use a regulator
> here. I don't know of any real-life cases, but who is to say someone
> not design a board that uses a regulator controlled by I2C instead of gpios
> or something like that.
>
> Then, boards that don't have gpios can just use a fixed regulator (or you
> can make the regulator optional). Using a regulator would also allow users
> to decide how to respond to overcurrent events (by supplying their own
> regulator driver) instead of the behavior being dictated by the ohci driver.
I agree that we should use a regulator for the vbus power.
i will make that change. However, im not so sure about using the
regulator for the overcurrent handling. There seems to be no other
driver doing this, and as you mention, we would need to change the regulator
framework, which might not be justifiable. I think there is not another way
to handle the over current notification other than powering the port off.
>
> In my particular area of interest (LEGO MINDSTORMS EV3), the 5V (hardware)
> regulator for VBUS does use gpios, but the 5V is also shared with the LEGO
> input and output ports. So what I would ultimately like to be able to do is
> have userspace notified of an overcurrent event and let userspace decided
> when to turn the vbus back on. For example, someone might plug something
> into one of the LEGO input or output ports that causes a short circuit. I
> would like to display a notification to the user and wait for them to
> correct the problem and then press a button to turn the power back on.
>
how about using regulator for vbus, but keeping gpio for overcurrent
notifications?
For the usersapce handling you describe above, maybe we should be able to
listen for an usb overcurrent uevent in userspace? it seems this
question was asked
a couple of years back[1], but im not sure what the conclusion was. In any case,
we could have DT and non-DT based ohci-da8xx working,
And could work on a uevent notification for the scenario you describe
above which
i think is not specific to the ohci-da8xx.
[1]http://linux-usb.vger.kernel.narkive.com/SjcUB5hk/how-best-to-get-over-current-notification-to-user-application
Regards
Axel
> This will require some modifications to the regulator subsystem though. I
> actually started work on this a while back, but haven't had the time to
> pursue it any farther.
>
> Here are my WIP patches in case there is any interest:
> *
> https://github.com/dlech/ev3dev-kernel/commit/541a42b3b8ed639e95bbc835df3292f80190c789
> *
> https://github.com/dlech/ev3dev-kernel/commit/2ba99b1ad6a06c944dd33a073f54044e71b75ae6
> *
> https://github.com/dlech/ev3dev-kernel/commit/cdb03caa50e64931d4f2836c648739aa4385ed3b
> *
> https://github.com/dlech/ev3dev-kernel/commit/9d6b50cde34b51309c74d97c26b1430c7ff6aa0f
^ permalink raw reply
* [linux-sunxi] [PATCH 3/5] Input: add driver for Ilitek ili2139 touch IC
From: Hans de Goede @ 2016-10-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161011132149.LZ40KToV@smtp1h.mail.yandex.net>
Hi,
On 10/11/2016 12:21 PM, Icenowy Zheng wrote:
>
> 2016?10?11? ??5:37? Hans de Goede <hdegoede@redhat.com>???
>>
>> Hi,
>
> I have a request: could you please test this driver on your E708 Q1? (According to the info you published on github, your E708 has also ili)
I'm afraid the touchscreen on my E708 Q1 is broken (does not even work in android),
so I cannot test this.
>
>>
>> On 10/11/2016 02:33 AM, Icenowy Zheng wrote:
>>> This driver adds support for Ilitek ili2139 touch IC, which is used in
>>> several Colorfly tablets (for example, Colorfly E708 Q1, which is an
>>> Allwinner A31s tablet with mainline kernel support).
>>>
>>> Theortically it may support more Ilitek touch ICs, however, only ili2139
>>> is used in any mainlined device.
>>>
>>> It supports device tree enumeration, with screen resolution and axis
>>> quirks configurable.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>> ---
>>> drivers/input/touchscreen/Kconfig | 14 ++
>>> drivers/input/touchscreen/Makefile | 1 +
>>> drivers/input/touchscreen/ili2139.c | 320 ++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 335 insertions(+)
>>> create mode 100644 drivers/input/touchscreen/ili2139.c
>>>
>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>> index 5079813..bb4d9d2 100644
>>> --- a/drivers/input/touchscreen/Kconfig
>>> +++ b/drivers/input/touchscreen/Kconfig
>>> @@ -348,6 +348,20 @@ config TOUCHSCREEN_ILI210X
>>> To compile this driver as a module, choose M here: the
>>> module will be called ili210x.
>>>
>>> +config TOUCHSCREEN_ILI2139
>>> + tristate "Ilitek ILI2139 based touchscreen"
>>> + depends on I2C
>>> + depends on OF
>>> + help
>>> + Say Y here if you have a ILI2139 based touchscreen
>>> + controller. Such kind of chipsets can be found in several
>>> + Colorfly tablets.
>>> +
>>> + If unsure, say N.
>>> +
>>> + To compile this driver as a module, choose M here; the
>>> + module will be called ili2139.
>>> +
>>> config TOUCHSCREEN_IPROC
>>> tristate "IPROC touch panel driver support"
>>> depends on ARCH_BCM_IPROC || COMPILE_TEST
>>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>>> index 81b8645..930b5e2 100644
>>> --- a/drivers/input/touchscreen/Makefile
>>> +++ b/drivers/input/touchscreen/Makefile
>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix.o
>>> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
>>> +obj-$(CONFIG_TOUCHSCREEN_ILI2139) += ili2139.o
>>> obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC) += imx6ul_tsc.o
>>> obj-$(CONFIG_TOUCHSCREEN_INEXIO) += inexio.o
>>> obj-$(CONFIG_TOUCHSCREEN_INTEL_MID) += intel-mid-touch.o
>>> diff --git a/drivers/input/touchscreen/ili2139.c b/drivers/input/touchscreen/ili2139.c
>>> new file mode 100644
>>> index 0000000..65c2dea
>>> --- /dev/null
>>> +++ b/drivers/input/touchscreen/ili2139.c
>>> @@ -0,0 +1,320 @@
>>> +/* -------------------------------------------------------------------------
>>> + * Copyright (C) 2016, Icenowy Zheng <icenowy@aosc.xyz>
>>> + *
>>> + * Derived from:
>>> + * ili210x.c
>>> + * Copyright (C) Olivier Sobrie <olivier@sobrie.be>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + * -------------------------------------------------------------------------
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/input.h>
>>> +#include <linux/input/mt.h>
>>> +#include <linux/input/touchscreen.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define DEFAULT_POLL_PERIOD 20
>>> +
>>> +#define MAX_TOUCHES 10
>>> +#define COMPATIBLE_TOUCHES 2
>>> +
>>> +/* Touchscreen commands */
>>> +#define REG_TOUCHDATA 0x10
>>> +#define REG_TOUCHSUBDATA 0x11
>>> +#define REG_PANEL_INFO 0x20
>>> +#define REG_FIRMWARE_VERSION 0x40
>>> +#define REG_PROTO_VERSION 0x42
>>> +
>>> +#define SUBDATA_STATUS_TOUCH_POINT 0x80
>>> +#define SUBDATA_STATUS_RELEASE_POINT 0x00
>>> +
>>> +struct finger {
>>> + u8 x_low;
>>> + u8 x_high;
>>> + u8 y_low;
>>> + u8 y_high;
>>> +} __packed;
>>> +
>>> +struct touchdata {
>>> + u8 length;
>>> + struct finger finger[COMPATIBLE_TOUCHES];
>>> +} __packed;
>>> +
>>> +struct touch_subdata {
>>> + u8 status;
>>> + struct finger finger;
>>> +} __packed;
>>> +
>>> +struct panel_info {
>>> + struct finger finger_max;
>>> + u8 xchannel_num;
>>> + u8 ychannel_num;
>>> +} __packed;
>>> +
>>> +struct firmware_version {
>>> + u8 id;
>>> + u8 major;
>>> + u8 minor;
>>> +} __packed;
>>> +
>>> +struct ili2139 {
>>> + struct i2c_client *client;
>>> + struct input_dev *input;
>>> + unsigned int poll_period;
>>> + struct delayed_work dwork;
>>> + struct touchscreen_properties prop;
>>> + int slots[MAX_TOUCHES];
>>> + int ids[MAX_TOUCHES];
>>> + struct input_mt_pos pos[MAX_TOUCHES];
>>> +};
>>> +
>>> +static int ili2139_read_reg(struct i2c_client *client, u8 reg, void *buf,
>>> + size_t len)
>>> +{
>>> + struct i2c_msg msg[2] = {
>>> + {
>>> + .addr = client->addr,
>>> + .flags = 0,
>>> + .len = 1,
>>> + .buf = ®,
>>> + },
>>> + {
>>> + .addr = client->addr,
>>> + .flags = I2C_M_RD,
>>> + .len = len,
>>> + .buf = buf,
>>> + }
>>> + };
>>> +
>>> + if (i2c_transfer(client->adapter, msg, 2) != 2) {
>>> + dev_err(&client->dev, "i2c transfer failed\n");
>>> + return -EIO;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> This just i2c_smbus_read_i2c_block_data, please use that instead.
>>
>>> +static void ili2139_work(struct work_struct *work)
>>> +{
>>> + int id;
>>> + struct ili2139 *priv = container_of(work, struct ili2139,
>>> + dwork.work);
>>> + struct i2c_client *client = priv->client;
>>> + struct touchdata touchdata;
>>> + struct touch_subdata subdata;
>>> + int error;
>>> +
>>> + error = ili2139_read_reg(client, REG_TOUCHDATA,
>>> + &touchdata, sizeof(touchdata));
>>> + if (error) {
>>> + dev_err(&client->dev,
>>> + "Unable to get touchdata, err = %d\n", error);
>>> + return;
>>> + }
>>> +
>>> + for (id = 0; id < touchdata.length; id++) {
>>> + error = ili2139_read_reg(client, REG_TOUCHSUBDATA, &subdata,
>>> + sizeof(subdata));
>>> + if (error) {
>>> + dev_err(&client->dev,
>>> + "Unable to get touch subdata, err = %d\n",
>>> + error);
>>> + return;
>>> + }
>>> +
>>> + priv->ids[id] = subdata.status & 0x3F;
>>> +
>>> + /* The sequence changed in the v2 subdata protocol. */
>>> + touchscreen_set_mt_pos(&priv->pos[id], &priv->prop,
>>> + (subdata.finger.x_high | (subdata.finger.x_low << 8)),
>>> + (subdata.finger.y_high | (subdata.finger.y_low << 8)));
>>> + }
>>> +
>>> + input_mt_assign_slots(priv->input, priv->slots, priv->pos,
>>> + touchdata.length, 0);
>>> +
>>> + for (id = 0; id < touchdata.length; id++) {
>>> + input_mt_slot(priv->input, priv->slots[id]);
>>> + input_mt_report_slot_state(priv->input, MT_TOOL_FINGER,
>>> + subdata.status &
>>> + SUBDATA_STATUS_TOUCH_POINT);
>>> + input_report_abs(priv->input, ABS_MT_POSITION_X,
>>> + priv->pos[id].x);
>>> + input_report_abs(priv->input, ABS_MT_POSITION_Y,
>>> + priv->pos[id].y);
>>> + }
>>> +
>>> + input_mt_sync_frame(priv->input);
>>> + input_sync(priv->input);
>>> +
>>> + schedule_delayed_work(&priv->dwork,
>>> + msecs_to_jiffies(priv->poll_period));
>>
>> If the irq is working properly there should be no need for this,
>> can you try with this schedule call removed ?
>
> Thanks. I'm glad to know this.
> The driver that I modelled after is too old and unmaintained, and even nearly not usable now (as it requires platform data)
>
>>
>>> +}
>>> +
>>> +static irqreturn_t ili2139_irq(int irq, void *irq_data)
>>> +{
>>> + struct ili2139 *priv = irq_data;
>>> +
>>> + schedule_delayed_work(&priv->dwork, 0);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ili2139_i2c_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct ili2139 *priv;
>>> + struct input_dev *input;
>>> + struct panel_info panel;
>>> + struct firmware_version firmware;
>>> + int xmax, ymax;
>>> + int error;
>>> +
>>> + dev_dbg(dev, "Probing for ILI2139 I2C Touschreen driver");
>>> +
>>> + if (client->irq <= 0) {
>>> + dev_err(dev, "No IRQ!\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + /* Get firmware version */
>>> + error = ili2139_read_reg(client, REG_FIRMWARE_VERSION,
>>> + &firmware, sizeof(firmware));
>>> + if (error) {
>>> + dev_err(dev, "Failed to get firmware version, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + /* get panel info */
>>> + error = ili2139_read_reg(client, REG_PANEL_INFO, &panel, sizeof(panel));
>>> + if (error) {
>>> + dev_err(dev, "Failed to get panel information, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + xmax = panel.finger_max.x_low | (panel.finger_max.x_high << 8);
>>> + ymax = panel.finger_max.y_low | (panel.finger_max.y_high << 8);
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + input = devm_input_allocate_device(dev);
>>> + if (!priv || !input)
>>> + return -ENOMEM;
>>> +
>>> + priv->client = client;
>>> + priv->input = input;
>>> + priv->poll_period = DEFAULT_POLL_PERIOD;
>>> + INIT_DELAYED_WORK(&priv->dwork, ili2139_work);
>>> +
>>> + /* Setup input device */
>>> + input->name = "ILI2139 Touchscreen";
>>> + input->id.bustype = BUS_I2C;
>>> + input->dev.parent = dev;
>>> +
>>> + __set_bit(EV_SYN, input->evbit);
>>> + __set_bit(EV_KEY, input->evbit);
>>> + __set_bit(EV_ABS, input->evbit);
>>> +
>>> + /* Multi touch */
>>> + input_mt_init_slots(input, MAX_TOUCHES, INPUT_MT_DIRECT |
>>> + INPUT_MT_DROP_UNUSED | INPUT_MT_TRACK);
>>> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, xmax, 0, 0);
>>> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, ymax, 0, 0);
>>> +
>>> + touchscreen_parse_properties(input, true, &priv->prop);
>>> +
>>> + input_set_drvdata(input, priv);
>>> + i2c_set_clientdata(client, priv);
>>> +
>>> + error = devm_request_irq(dev, client->irq, ili2139_irq,
>>> + IRQF_TRIGGER_FALLING, client->name, priv);
>>
>> If things work with the re-scheduleing of the delayed work
>> from the work removed, then you can use request_threaded_irq here,
>> pass in ili2139_irq as the threaded handler (and NULL as the non threaded
>> handler) and do all the i2c reading directly in ili2139_irq without needing
>> to use any work struct at all.
>
> I'll test it, and remember request_threaded_irq function. I've seen the usage of the function in silead.c.
Good :)
Regards,
Hans
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>> + if (error) {
>>> + dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + error = input_register_device(priv->input);
>>> + if (error) {
>>> + dev_err(dev, "Cannot register input device, err: %d\n", error);
>>> + return error;
>>> + }
>>> +
>>> + device_init_wakeup(&client->dev, 1);
>>> +
>>> + dev_dbg(dev,
>>> + "ILI2139 initialized (IRQ: %d), firmware version %d.%d.%d",
>>> + client->irq, firmware.id, firmware.major, firmware.minor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ili2139_i2c_remove(struct i2c_client *client)
>>> +{
>>> + struct ili2139 *priv = i2c_get_clientdata(client);
>>> +
>>> + cancel_delayed_work_sync(&priv->dwork);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused ili2139_i2c_suspend(struct device *dev)
>>> +{
>>> + struct i2c_client *client = to_i2c_client(dev);
>>> +
>>> + if (device_may_wakeup(&client->dev))
>>> + enable_irq_wake(client->irq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused ili2139_i2c_resume(struct device *dev)
>>> +{
>>> + struct i2c_client *client = to_i2c_client(dev);
>>> +
>>> + if (device_may_wakeup(&client->dev))
>>> + disable_irq_wake(client->irq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static SIMPLE_DEV_PM_OPS(ili2139_i2c_pm,
>>> + ili2139_i2c_suspend, ili2139_i2c_resume);
>>> +
>>> +static const struct i2c_device_id ili2139_i2c_id[] = {
>>> + { "ili2139", 0 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ili2139_i2c_id);
>>> +
>>> +static struct i2c_driver ili2139_ts_driver = {
>>> + .driver = {
>>> + .name = "ili2139_i2c",
>>> + .pm = &ili2139_i2c_pm,
>>> + },
>>> + .id_table = ili2139_i2c_id,
>>> + .probe = ili2139_i2c_probe,
>>> + .remove = ili2139_i2c_remove,
>>> +};
>>> +
>>> +module_i2c_driver(ili2139_ts_driver);
>>> +
>>> +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
>>> +MODULE_DESCRIPTION("ILI2139 I2C Touchscreen Driver");
>>> +MODULE_LICENSE("GPL");
>>>
^ 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