linux-kernel - Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander

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:   Sun, 10 Jan 2021 15:56:39 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Mark Brown <broonie@...nel.org>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org, phone-devel@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO
 Expander

Il 10/01/21 15:32, AngeloGioacchino Del Regno ha scritto:
> Il 10/01/21 01:24, Linus Walleij ha scritto:
>> On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@...ainline.org> wrote:
>>> Il 09/01/21 23:11, Linus Walleij ha scritto:
>>
>>>> The major review comment is that it'd be nice if you look into
>>>> using regmaps register cache instead of rolling your own,
>>>> and also possibly using regmaps locking rather than your own
>>>> as a result of that.
>>>>
>>> Actually, I really tried to use regmap's FLAT register cache and after
>>> many, many tries... I had to give up. I just couldn't get it working. :(
>>
>> This needs to be root-caused. The register cache in regmap is for
>> using not for decoration.
>>
>> What is the problems you are seeing?
>>
>> If it is fundamentally so that regmap has limitations that is one thing,
>> but I want to rule out that we're just not using it wrong or that there
>> is a bug in it that we should fix.
>>
>> Looping in Mark Brown the regmap maintainer.
>>
>> Yours,
>> Linus Walleij
>>
> 
> Oh don't misunderstand me, I would really be happy to make use of the 
> regmap register cache, it's a nice one.
> 
> So, I've retried some basic usage of the regcache, relevant snippets here:
> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
> 
> {
> 
>      return reg == AW9523_REG_IN_STATE(0) ||
> 
>             reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
> 
>             reg == AW9523_REG_CHIPID;
> 
> }
> 
> static const struct regmap_config aw9523_regmap = {
> 
> ....
>      .volatile_reg = aw9523_volatile_reg,
> 
> 
>      .cache_type = REGCACHE_FLAT,
> 
> ....
> 
> }
> 
> Since REG_IN_STATE is used to read the GPIO input level, it's not 
> cacheable, then CHIPID was set as not cacheable for safety: that may be 
> avoided, but that may make no sense.. since it's a one-time readout for 
> init putposes, it'd be useless to keep it cached.
> 
> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were 
> replaced with calls to regmap_update_bits_async, example:
> 
>      regmap_update_bits_async(awi->regmap,
> 
>                   AW9523_REG_INTR_DIS(d->hwirq),
> 
>                   BIT(n), BIT(n));
> 
> 
> 
> Where of course the value is either BIT(n) or 0 for mask and unmask 
> respectively.
> Also, the bus_sync_unlock callback was changed as follows:
> 
> static void aw9523_irq_mask(struct irq_data *d)
> 
> {
> 
>      struct aw9523 *awi =
>          gpiochip_get_data(irq_data_get_irq_chip_data(d));
> 
>      unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
> 
> 
> 
>      regmap_update_bits_async(awi->regmap,
> 
>                   AW9523_REG_INTR_DIS(d->hwirq),
> 
>                   BIT(n), BIT(n));
> 
> }
Sorry, wrong paste! This is the right one:
static void aw9523_irq_bus_sync_unlock(struct irq_data *d)

{

	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));



	regcache_mark_dirty(awi->regmap);

	regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),

			     AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));

	mutex_unlock(&awi->irq->lock);

}

> 
> One of the biggest / oddest issues that I get when trying to use 
> regcache is that I'm getting badbadbad scheduling while atomic warnings 
> all over and I don't get why, since regcache_default_sync is just 
> calling _regmap_write, which is exactly what (non _prefix) regmap_write 
> also calls...
> 
> As a reference, this is one out of "many" (as you can imagine) stacktraces:
> 
> <3>[    1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
> 
> <4>[    1.061575] Modules linked in:
> 
> <4>[    1.061716] CPU: 3 PID: 119 Comm: kworker/3:1 Tainted: G        W 
>          5.10.0-rc3-31949-ge1680e3101bc-dirty #1162
> 
> <4>[    1.061956] Hardware name: F(x)tec Pro1 (QX1000) (DT)
> 
> <4>[    1.062081] Workqueue: events deferred_probe_work_func
> 
> <4>[    1.062205] Call trace:
> 
> <4>[    1.062333]  dump_backtrace+0x0/0x1e0
> 
> <4>[    1.062439]  show_stack+0x14/0x60
> 
> <4>[    1.062539]  dump_stack+0xd4/0x12c
> 
> <4>[    1.062680]  __schedule_bug+0x50/0x70
> 
> <4>[    1.062785]  __schedule+0x618/0x650
> 
> <4>[    1.062888]  schedule+0x6c/0xf8
> 
> <4>[    1.062985]  schedule_timeout+0x1d0/0x260
> 
> <4>[    1.063134]  wait_for_completion_timeout+0x8c/0x110
> 
> <4>[    1.063257]  qup_i2c_wait_for_complete.isra.18+0x1c/0x80
> 
> <4>[    1.063429]  qup_i2c_xfer_v2_msg+0x2d4/0x3f0
> 
> <4>[    1.063543]  qup_i2c_xfer_v2+0x290/0xa28
> 
> <4>[    1.063652]  __i2c_transfer+0x16c/0x380
> 
> <4>[    1.063798]  i2c_transfer+0x5c/0x138
> 
> <4>[    1.063903]  i2c_transfer_buffer_flags+0x58/0x80
> 
> <4>[    1.064060]  regmap_i2c_write+0x1c/0x50
> 
> <4>[    1.064168]  _regmap_raw_write_impl+0x35c/0x688
> 
> <4>[    1.064285]  _regmap_bus_raw_write+0x64/0x80
> 
> <4>[    1.064440]  _regmap_write+0x58/0xa8
> 
> <4>[    1.064545]  regcache_default_sync+0xcc/0x1a0
> 
> <4>[    1.064660]  regcache_sync_region+0xdc/0xe8
> 
> <4>[    1.064811]  aw9523_irq_bus_sync_unlock+0x30/0x48
> 
> <4>[    1.064931]  __setup_irq+0x798/0x890
> 
> <4>[    1.065034]  request_threaded_irq+0xe0/0x198
> 
> <4>[    1.065188]  devm_request_threaded_irq+0x78/0xf8
> 
> <4>[    1.065311]  gpio_keyboard_probe+0x2a8/0x468
> 
> <4>[    1.065465]  platform_drv_probe+0x50/0xa0
> 
> <4>[    1.065576]  really_probe+0x290/0x4e8
> 
> <4>[    1.065682]  driver_probe_device+0xf4/0x160
> 
> <4>[    1.065834]  __device_attach_driver+0x98/0x110
> 
> <4>[    1.065950]  bus_for_each_drv+0x64/0xc8
> 
> <4>[    1.066063]  __device_attach+0xe4/0x168
> 
> <4>[    1.066211]  device_initial_probe+0x10/0x18
> 
> <4>[    1.066325]  bus_probe_device+0x90/0x98
> 
> <4>[    1.066434]  deferred_probe_work_func+0x88/0xe0
> 
> <4>[    1.066591]  process_one_work+0x1e4/0x358
> 
> <4>[    1.066702]  worker_thread+0x208/0x478
> 
> <4>[    1.073273]  kthread+0x14c/0x150
> 
> <4>[    1.079858]  ret_from_fork+0x10/0x18
> 
> 
> 
> P.S.: Infinite thanks for being so nice and helpful!