linux-kernel - Re: [PATCH 1/3] bus: add Broadcom GISB arbiter bus timeout/error handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGVrzcag3aKiuuQsfZWrAJT+-cpO__kNF3Lc9XXkpswQxzL56A@mail.gmail.com>
Date:	Wed, 14 May 2014 12:46:06 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Marc Carino <marc.ceeeee@...il.com>,
	Brian Norris <computersforpeace@...il.com>,
	Gregory Fong <gregory.0xf0@...il.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>,
	Shawn Guo <shawn.guo@...aro.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Alexander Shiyan <shc_work@...l.ru>,
	Huang Shijie <b32955@...escale.com>,
	"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/3] bus: add Broadcom GISB arbiter bus timeout/error handler

2014-05-14 12:33 GMT-07:00 Arnd Bergmann <arnd@...db.de>:
> On Wednesday 14 May 2014 12:10:46 Florian Fainelli wrote:
>> This patch adds support for the Broadcom GISB arbiter bus timeout/error
>> handler. GISB is a proprietary bus used by Broadcom Set Top Box
>> System-on-a-chip devices (BCM7xxx) which allows multiple masters and
>> clients to be interfaced with each other.
>>
>> The bus arbiter offers support for generating two interrupts towards the
>> host CPU, thus allowing us to "catch" clock gated masters, or masters
>> being volontarily blocked for powersaving purposes, or do general system
>> troubleshooting.
>>
>> We also register a hook with the ARM fault exception handling to allow
>> printing a more informative message than "imprecise external abort at
>> 0x00000000" for instance.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>
> Looks pretty good to me.

Cool, thanks!, BTW, which tree should we attempt to get this merged into?

>
>> +static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
>> +                                     const char *reason)
>> +{
>> +     u32 cap_status;
>> +     unsigned long arb_addr;
>> +     u32 master;
>> +     const char *m_name;
>> +     char m_fmt[11];
>> +
>> +     cap_status = ioread32(gdev->base + ARB_ERR_CAP_STATUS);
>> +
>> +     /* Invalid captured address, bail out */
>> +     if (!(cap_status & ARB_ERR_CAP_STATUS_VALID))
>> +             return 1;
>> +
>> +     /* Read the address and master */
>> +     arb_addr = ioread32(gdev->base + ARB_ERR_CAP_ADDR) & 0xffffffff;
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> +     arb_addr |= (u64)ioread32(gdev->base + ARB_ERR_CAP_HI_ADDR) << 32;
>> +#endif
>
> Can you use 'if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))' here, or does that
> cause a warning?

No warning is issued, I will use that instead. Thanks!

>
>> +static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
>> +                                  struct pt_regs *regs)
>> +{
>> +     int ret = 0;
>> +     struct brcmstb_gisb_arb_device *gdev;
>> +
>> +     /* iterate over each GISB arb registered handlers */
>> +     list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
>> +             ret |= brcmstb_gisb_arb_decode_addr(gdev, "bus error");
>> +     /*
>> +      * If it was an imprecise abort, then we need to correct the
>> +      * return address to be _after_ the instruction.
>> +     */
>> +     if (fsr & (1 << 10))
>> +             regs->ARM_pc += 4;
>> +
>> +     return ret;
>> +}
>
> This will cause the normal abort handler to trigger if the GISB
> arbiter doesn't know what happened, right?

That's right, one of the very first things that the GISB arbiter
checks is whether the faulting error is valid or not, if it is not, it
bails out and so we keep on processing the other exception handlers.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/