linux-kernel - Re: [PATCH v3] 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]

Date:   Thu, 6 Jul 2017 10:02:19 -0700
From:   Patrick Venture <venture@...gle.com>
To:     Patrick Venture <venture@...gle.com>,
        Joel Stanley <joel@....id.au>, Rob Lippert <roblip@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     Rick Altherr <raltherr@...gle.com>, Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

On Thu, Jul 6, 2017 at 10:00 AM, Patrick Venture <venture@...gle.com> wrote:
> On Wed, Jul 5, 2017 at 2:51 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>
>> ---
>> v3: added .data object to determine behavior difference between ast2400 and
>>     ast2500.
>> v2: added aspeed-g5 area because ast2400 doesn't use those bits.
>>     also updated commit message.
>> ---
>>  drivers/misc/aspeed-lpc-snoop.c | 34 +++++++++++++++++++++++++++++-----
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c
>> index 593905565b74..696ad20d8f46 100644
>> --- a/drivers/misc/aspeed-lpc-snoop.c
>> +++ b/drivers/misc/aspeed-lpc-snoop.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/regmap.h>
>>
>> @@ -51,6 +52,13 @@
>>  #define HICRB_ENSNP0D          BIT(14)
>>  #define HICRB_ENSNP1D          BIT(15)
>>
>> +struct aspeed_lpc_snoop_model_data {
>> +       /* The ast2400 has bits 14 and 15 as reserved, whereas the ast2500
>> +        * can use them.
>> +        */
>> +       unsigned int has_hicrb_ensnp;
>> +};
>> +
>>  struct aspeed_lpc_snoop {
>>         struct regmap           *regmap;
>>         int                     irq;
>> @@ -123,10 +131,13 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
>>  }
>>
>>  static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>> -                                 int channel, u16 lpc_port)
>> +                                  struct device *dev,
>> +                                  int channel, u16 lpc_port)
>>  {
>>         int rc = 0;
>>         u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
>> +       const struct aspeed_lpc_snoop_model_data *model_data =
>> +               of_device_get_match_data(dev);
>>
>>         /* Create FIFO datastructure */
>>         rc = kfifo_alloc(&lpc_snoop->snoop_fifo[channel],
>> @@ -155,7 +166,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);
>> -       regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en);
>> +       if (model_data->has_hicrb_ensnp)
>> +               regmap_update_bits(lpc_snoop->regmap, HICRB,
>> +                               hicrb_en, hicrb_en);
>>
>>         return rc;
>>  }
>> @@ -213,14 +226,14 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>>         if (rc)
>>                 return rc;
>>
>> -       rc = aspeed_lpc_enable_snoop(lpc_snoop, 0, port);
>> +       rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
>>         if (rc)
>>                 return rc;
>>
>>         /* Configuration of 2nd snoop channel port is optional */
>>         if (of_property_read_u32_index(dev->of_node, "snoop-ports",
>>                                        1, &port) == 0) {
>> -               rc = aspeed_lpc_enable_snoop(lpc_snoop, 1, port);
>> +               rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
>>                 if (rc)
>>                         aspeed_lpc_disable_snoop(lpc_snoop, 0);
>>         }
>> @@ -239,8 +252,19 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>>         return 0;
>>  }
>>
>> +static const struct aspeed_lpc_snoop_model_data ast2400_model_data = {
>> +       .has_hicrb_ensnp = 0,
>> +};
>> +
>> +static const struct aspeed_lpc_snoop_model_data ast2500_model_data = {
>> +       .has_hicrb_ensnp = 1,
>> +};
>> +
>>  static const struct of_device_id aspeed_lpc_snoop_match[] = {
>> -       { .compatible = "aspeed,ast2500-lpc-snoop" },
>> +       { .compatible = "aspeed,ast2500-lpc-snoop",
>> +         .data = &ast2400_model_data },
>> +       { .compatible = "aspeed,ast2400-lpc-snoop",
>> +         .data = &ast2500_model_data },
>>         { },
>
> This looks backwards.  Let me check in a non-diff and then resubmit.
>

Since the register setting is harmless on the ast2400, it had a null
effect for my testing, but ast2500 would have an issue. :D

>>  };
>>
>> --
>> 2.13.2.725.g09c95d1e9-goog
>>