Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Feb 25, 2025 at 11:13 AM Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
>
> On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > As per the API contract, the get_direction() callback can only
> > return 0, 1 or a negative error number. Add a wrapper around the callback
> > calls that filters out anything else.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> This patch landed in today's linux-next as commit e623c4303ed11
> ("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It
> introduced a lockdep warning from the gpiochip_get_direction() function.
> IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu
> locking/annotation for the newly created gpio chip. Here is the log:
>
> gpio gpiochip1: Static allocation of GPIO base is deprecated, use
> dynamic allocation.
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349
> gpiochip_get_direction+0x48/0x66
> Modules linked in: cdns_usb_common roles cdns3_starfive
> snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx
> clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp
> jh7110_trng sfctemp dwmac_starfive stmmac_platform
> spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac
> clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022
> phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm
> drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables
> CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W
> 6.14.0-rc4-next-20250225 #1054
> Tainted: [W]=WARN
> Hardware name: StarFive VisionFive 2 v1.2A (DT)
> Workqueue: events_unbound deferred_probe_work_func
> epc : gpiochip_get_direction+0x48/0x66
>   ra : gpiochip_get_direction+0x46/0x66
> ...
> [<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66
> [<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2
> [<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a
> [<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa
> [<ffffffff80731116>] platform_probe+0x4e/0x92
> [<ffffffff8000c366>] really_probe+0x10a/0x2de
> [<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0
> [<ffffffff8072ee34>] driver_probe_device+0x78/0xc4
> [<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6
> [<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0
> [<ffffffff8072f33e>] __device_attach+0x84/0x13c
> [<ffffffff8072f55e>] device_initial_probe+0xe/0x16
> [<ffffffff8072e002>] bus_probe_device+0x88/0x8a
> [<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee
> [<ffffffff80047b7e>] process_one_work+0x1d0/0x57a
> [<ffffffff8004854e>] worker_thread+0x166/0x2cc
> [<ffffffff80051568>] kthread+0xdc/0x1b4
> [<ffffffff80bcb942>] ret_from_fork+0xe/0x18
> irq event stamp: 17857
> hardirqs last  enabled at (17857): [<ffffffff80bca986>]
> _raw_spin_unlock_irqrestore+0x4c/0x4e
> hardirqs last disabled at (17856): [<ffffffff80bca73c>]
> _raw_spin_lock_irqsave+0x5e/0x64
> softirqs last  enabled at (17322): [<ffffffff80adff1a>]
> inet6_fill_ifla6_attrs+0x3d0/0x420
> softirqs last disabled at (17320): [<ffffffff80adfefe>]
> inet6_fill_ifla6_attrs+0x3b4/0x420
> ---[ end trace 0000000000000000 ]---
>

Thanks for the report. We don't need to hold the SRCU when registering
the chip. I'm now thinking I should revert using
gpiochip_get_direction() in gpiochip_add_data_with_key() to directly
calling the get_direction() callback and not checking its return
value.

Bartosz