linux-kernel - Re: [PATCH v2] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

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] [day] [month] [year] [list]

Message-ID: <CAO=notw4TH_wW90rGefcbDxu4o8xVsYH-mjs8UEiAi=b_xvJMA@mail.gmail.com>
Date:   Wed, 5 Jul 2017 12:47:22 -0700
From:   Patrick Venture <venture@...gle.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Joel Stanley <joel@....id.au>, Robert Lippert <roblip@...il.com>,
        gregkh <gregkh@...uxfoundation.org>,
        Rick Altherr <raltherr@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

On Wed, Jul 5, 2017 at 12:43 PM, Arnd Bergmann <arnd@...db.de> wrote:
> On Wed, Jul 5, 2017 at 9:04 PM, Patrick Venture <venture@...gle.com> wrote:
>> This driver can be used on the aspeed ast2400 with minor
>> modifications.
>>
>> Tested: ast2400 on quanta-q71l
>>
>> Signed-off-by: Patrick Venture <venture@...gle.com>
>> ---
>> v2: added aspeed-g5 area because ast2400 doesn't use those bits.
>>     also updated commit message.
>> ---
>>  drivers/misc/aspeed-lpc-snoop.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
>> index 593905565b74..83f9a9e5a7cf 100644
>> --- a/drivers/misc/aspeed-lpc-snoop.c
>> +++ b/drivers/misc/aspeed-lpc-snoop.c
>> @@ -155,8 +155,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>>         regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
>>         regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask,
>>                            lpc_port << snpwadr_shift);
>> +#ifdef CONFIG_MACH_ASPEED_G5
>>         regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
>> -
>> +#endif
>>         return rc;
>>  }
>
> Hi Patrick,
>
> Sorry for bringing up yet another point on a fairly trivial patch, but
> in general,
> I'd recommend making this a runtime check rather than compile-time.
>
> At the moment, your version is safe because CONFIG_MACH_ASPEED_G5
> and CONFIG_MACH_ASPEED_G4 are mutually exclusive and there is
> always one of them set, but once we get support for G6, G7 etc,
> the driver might silently break when it behaves differently depending
> on a configuration option that may or may not be set on a particular
> kernel build.
>
> You can use the .data field in the of_device_id to add a trigger for the
> behavior change.
>
>        Arnd

No problem whatsoever.  You're quite right.  I forgot about future
aspeed platforms.  I'm going to dig around a few drivers and see what
the right way is to handle this at run-time.  Presumably something
like:

#ifdef CONFIG_MACH_ASPEED_G4
.data = G4,
#else
.data = G5,
#endif

Since we're holding that maybe the future should default to this
behaviour.  Is it linux standard to do something like #(error) in the
#else so that it fails if they didn't update this driver instead of
defaulting to the G5 setting?

Thanks,
Patrick