From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C99E3EE4996 for ; Mon, 21 Aug 2023 12:42:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235023AbjHUMmH (ORCPT ); Mon, 21 Aug 2023 08:42:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50304 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235022AbjHUMmG (ORCPT ); Mon, 21 Aug 2023 08:42:06 -0400 Received: from mail-ua1-x933.google.com (mail-ua1-x933.google.com [IPv6:2607:f8b0:4864:20::933]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 082D291 for ; Mon, 21 Aug 2023 05:42:05 -0700 (PDT) Received: by mail-ua1-x933.google.com with SMTP id a1e0cc1a2514c-79b4d2c0621so691209241.2 for ; Mon, 21 Aug 2023 05:42:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20221208.gappssmtp.com; s=20221208; t=1692621724; x=1693226524; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=9/XUKnH0ODMsj7aJ2rH2loczLALoUCOwlJiRi5R0GRw=; b=2VMC4Et+gZb2nhck9gfQ7jcfJTDmTXeFwR/gYdD2xbhlQwucu9eJan7yiURvjfugI/ qVzMVR7nIeVAPQS+yBsCC8DuvVnlylgTruS7lJjvxsk8yy2ib4a/VcKZfAJQVIORbsSc 4Ak/QiOY4oIZEPmrccPMUDuX8TwXncyKQttcVJOOqzqKJtxt1LKjiAikqV/pbpumM4aJ UtivtOTrWlwjONxWOS3r9u2rf3M+tR1YBO2L7ApjAI0nbApFh66wspIpfifYKLM10kfe kffhNopBdaUtoCeQKmmPNiFdzdQB78+l6mXKGB8GjrSf4mQ29HrOJV0bxeqQoELx4ovl 2NYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692621724; x=1693226524; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9/XUKnH0ODMsj7aJ2rH2loczLALoUCOwlJiRi5R0GRw=; b=PU5ycbclBmXgGGfGy6s+xGhVApKn5FqVqbcnbaXGcCDY0A91ULoknJVn3f0LD85mVt vI1P+t6EpzvN6O8H6OtLK0EQIXO6CiniedkE5lYjTe5lXAzY4sbtb3se08EiHIMGrTPV wi1/F7PH+Dw2f/DqhKBuobMFz2DaT+1S0zxx/Z/CRUeCa3HwaYwprZD8mcqzUg+ocWXU RY39joxl3ibfVD4XQaTc2ePyg+gg3AeQaYn6bVmmizKnxjXcPLcj0WINrmw1iQnGnnlJ f+KHSuFrVYP+DViIXn3NnpnXSAR2RQPPbRv1EJoINZxxOenD0wcqgYQ3Q0oZTmUtT9uz bi2w== X-Gm-Message-State: AOJu0Yz65AVMUXMKzys4XLMLUDGgBETR8vxko0pTj4Onf7A4ne0wJAY0 fJmFRpkTrskNjpPd014PQPftaqmGlLHxkKEbW5ELuSDHgsKUMXvEOsc= X-Google-Smtp-Source: AGHT+IFeTlzQ49lQSO4nLHVwpJ1/aco2mrW93aFGnKhTYAhfDwMRdC3G43F6dClNQP1Fxsl2q3tlFyRTAgiMKtoDGu4= X-Received: by 2002:a67:f74c:0:b0:445:228e:62db with SMTP id w12-20020a67f74c000000b00445228e62dbmr2724787vso.8.1692621724150; Mon, 21 Aug 2023 05:42:04 -0700 (PDT) MIME-Version: 1.0 References: <20230817184356.25020-1-brgl@bgdev.pl> In-Reply-To: From: Bartosz Golaszewski Date: Mon, 21 Aug 2023 14:41:53 +0200 Message-ID: Subject: Re: [PATCH v6] gpio: consumer: new virtual driver To: Andy Shevchenko Cc: Linus Walleij , Kent Gibson , linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Fri, Aug 18, 2023 at 11:16=E2=80=AFAM Andy Shevchenko wrote: > > On Thu, Aug 17, 2023 at 08:43:56PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > The GPIO subsystem has a serious problem with undefined behavior and > > use-after-free bugs on hot-unplug of GPIO chips. This can be considered= a > > corner-case by some as most GPIO controllers are enabled early in the > > boot process and live until the system goes down but most GPIO drivers > > do allow unbind over sysfs, many are loadable modules that can be (forc= e) > > unloaded and there are also GPIO devices that can be dynamically detach= ed, > > for instance CP2112 which is a USB GPIO expender. > > > > Bugs can be triggered both from user-space as well as by in-kernel user= s. > > We have the means of testing it from user-space via the character devic= e > > but the issues manifest themselves differently in the kernel. > > > > This is a proposition of adding a new virtual driver - a configurable > > GPIO consumer that can be configured over configfs (similarly to > > gpio-sim). > > > > The configfs interface allows users to create dynamic GPIO lookup table= s > > that are registered with the GPIO subsystem. Every config group > > represents a consumer device. Every sub-group represents a single GPIO > > lookup. The device can work in three modes: just keeping the line > > active, toggling it every second or requesting its interrupt and > > reporting edges. Every lookup allows to specify the key, offset and > > flags as per the lookup struct defined in linux/gpio/machine.h. > > > > The module together with gpio-sim allows to easily trigger kernel > > hot-unplug errors. A simple use-case is to create a simulated chip, > > setup the consumer to lookup one of its lines in 'monitor' mode, unbind > > the simulator, unbind the consumer and observe the fireworks in dmesg. > > > > This driver is aimed as a helper in tackling the hot-unplug problem in > > GPIO as well as basis for future regression testing once the fixes are > > upstream. > > ... > > > @@ -46,6 +46,7 @@ obj-$(CONFIG_GPIO_BT8XX) +=3D gpio-bt8xx.o > > obj-$(CONFIG_GPIO_CADENCE) +=3D gpio-cadence.o > > obj-$(CONFIG_GPIO_CLPS711X) +=3D gpio-clps711x.o > > obj-$(CONFIG_GPIO_SNPS_CREG) +=3D gpio-creg-snps.o > > +obj-$(CONFIG_GPIO_CONSUMER) +=3D gpio-consumer.o > > Order? > > > obj-$(CONFIG_GPIO_CRYSTAL_COVE) +=3D gpio-crystalcove.o > > obj-$(CONFIG_GPIO_CS5535) +=3D gpio-cs5535.o > > obj-$(CONFIG_GPIO_DA9052) +=3D gpio-da9052.o > > ... > > > + return dev_err_probe(dev, ret, > > + "Failed to read GPIO line names\n"); > > With one line it takes 83 characters (and note, that long before checkpat= ch > went for 100, the string literals at the end of a long line were accepted= )... > > ... > > > + return dev_err_probe(dev, ret, > > + "Failed to request GPIO l= ine interrupt\n"); > > And here with broken indentation you got 91. > Can you be consistent? > > (I prefer as you know less LoCs) > > ... > > > +static ssize_t > > +gpio_consumer_lookup_config_drive_store(struct config_item *item, > > + const char *page, size_t count) > > +{ > > + struct gpio_consumer_lookup *lookup =3D to_gpio_consumer_lookup(i= tem); > > + struct gpio_consumer_device *dev =3D lookup->parent; > > + > > + guard(mutex)(&dev->lock); > > + > > + if (gpio_consumer_device_is_live_unlocked(dev)) > > + return -EBUSY; > > + > > + if (sysfs_streq(page, "push-pull")) { > > + lookup->flags &=3D ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE); > > + } else if (sysfs_streq(page, "open-drain")) { > > + lookup->flags &=3D ~GPIO_OPEN_SOURCE; > > + lookup->flags |=3D GPIO_OPEN_DRAIN; > > + } else if (sysfs_streq(page, "open-source")) { > > + lookup->flags &=3D ~GPIO_OPEN_DRAIN; > > + lookup->flags |=3D GPIO_OPEN_SOURCE; > > + } else { > > > + count =3D -EINVAL; > > Strictly speaking this is incorrect. > You need > > ssize_t ret; > ... > ret =3D count; > if (...) > ret =3D -EINVAL; > > > + } > > + > > + return count; > > +} > > > +static ssize_t > > +gpio_consumer_lookup_config_pull_store(struct config_item *item, > > + const char *page, size_t count) > > +{ > > As per above. > > > +} > > ... > > > + curr->chip_hwnum =3D lookup->offset < 0 ? > > + U16_MAX : lookup->offset; > > I found this way better > > curr->chip_hwnum =3D > lookup->offset < 0 ? U16_MAX : lookup->offset; > > > ... > > > + return ret ?: count; > > Also possible way in the above mentioned cases. > > ... > > > I'm not going to bikeshed, I believe you can fix above accordingly, > either way > > Reviewed-by: Andy Shevchenko > Actually when experimenting more, I started thinking we could improve the driver even more. Expose a sysfs interface that would allow to control the GPIOs from the kernel on user-space's behalf. For instance: change value, change direction, toggle active-low watch for interrupts (and log them to a sysfs buffer) etc. I won't be queueing it for v6.6, I want to spend some more time on it. Thanks for the reviews. Bart > -- > With Best Regards, > Andy Shevchenko > >