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]
Date:   Thu, 25 May 2017 14:41:41 +0200
From:   Mason <slash.tmp@...e.fr>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Marc Gonzalez <marc_gonzalez@...madesigns.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        linux-pci <linux-pci@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Robin Murphy <robin.murphy@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Liviu Dudau <liviu.dudau@....com>,
        David Laight <david.laight@...lab.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Thibaud Cornic <thibaud_cornic@...madesigns.com>,
        Phuong Nguyen <phuong_nguyen@...madesigns.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

On 25/05/2017 14:23, Marc Zyngier wrote:
> On 25/05/17 13:00, Mason wrote:
>> On 25/05/2017 10:48, Marc Zyngier wrote:
>>> Please have some defines for these magic values.
>>
>> Typical driver do
>> #define MUX_OFFSET 0x48
>> and then access the register's value through
>> readl_relaxed(pcie->base + MUX_OFFSET);
>>
>> I can't do that because the registers were shuffled around
>> between revision 1 and revision 2. Thus, instead of an
>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>> named field (pcie->mux) and access the register's value
>> through readl_relaxed(pcie->mux);
> 
> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
> you can supplement with a V2 at some point.
> 
>> This is equivalent to providing the offset definitions in the
>> init functions, instead of at the top of the file.
> 
> Sorry, my brain parses text far better than hex number.

Well, the hex numbers do need to show up somewhere :-)

IIUC, you're saying that
#define MUX_OFFSET 0x48
is clearer than
pcie->mux = base + 0x48;

OK, I can accept that. Maybe our brains have been trained
to easily recognize and ingest the macro, or maybe it's
the caps, or maybe the fact that the statement does
several things (addition and assignment and hex).

Out of curiosity, how would you feel about
pcie->MUX_OFFSET = 0x48;
and then using
readl_relaxed(pcie->base + pcie->MUX_OFFSET);

It feels weird to me, I think mostly because it is
an unusual pattern.

Anyway, I'll add the macros, if that improves review and
maintenance.

Regards.

Powered by blists - more mailing lists