Message ID | 20230803111225.107572-1-Naresh.Solanki@9elements.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f41:0:b0:3e4:2afc:c1 with SMTP id v1csp1127125vqx; Thu, 3 Aug 2023 05:57:09 -0700 (PDT) X-Google-Smtp-Source: APBJJlEEUGgqrmn0uyH3kgxqNQhVTSljhmNyKVxixDbn/mKponVeRKnAk77L9laZeBTy91ezmgvA X-Received: by 2002:a2e:8687:0:b0:2b7:1005:931b with SMTP id l7-20020a2e8687000000b002b71005931bmr6143913lji.0.1691067429429; Thu, 03 Aug 2023 05:57:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1691067429; cv=none; d=google.com; s=arc-20160816; b=RoZEDG3IvydRwfwKHz2YwIL5/jBAQx/SGZMDPrSpK9N6Ygv7SNhRQDGnOSPFAXJ3PJ vY7OLphyGKBkYHaPeuHt9d+fPVxHEfREyDxkFCRMMqebvUfegON65GboWoGk4UEzrKkX QKNJ4CAFSvhmvR+Zx4QmtOH/0mLJFMdXGYTk40SBK3AxCRf+uU51DbRbNQCtndm8B31y honuhxqFdnM0u6pXZJLjZY665gJSCXf2zv5j1uud4HmSxt3j0QLZAtgs2hepREnBeke5 DABqGdoq7cAqrjaz+XBEy/hD5vYh0CB7SwTxgVrdLdEuXEPejyF9SKyk+SuS59uGZA4j xgGQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=s50wqnYcuW7vCWAmLFI5zKlzJEcFsQM28qc0cEKNUSo=; fh=Axn6FRjUxoQpBvsOlnGJdXTj1bswCbHu3A7GtAiC7fk=; b=GwxrMFjjWv3BLHrRuPdg3fCZK+MGOGGYtux5jquRBbcJHIcup84Ttkj9NbYVoE4qDW +uun/oPN62Hh2tvULvBWhM01dUknCCbBndfricS7X2cRekm+3DhTKr+GYsWI7A+Oi5q8 yG36m5SHMgpSH/Y3K5Z9+B+99ELn2GMSrsK3C18AxbMy1yd2N8QS1wndaYAiWF1+OJqO IQLXczLNvFV4PNvgUuDoI99+PR4oeTE8ojWz1/8MUil260p8difOCjKXBfB+K0Hp0gQy q8F7U5nXUsh9+XiruKiz4LTHRHkafd3Yw9LmtBWhfJldfYnmlzN5riWntHpPxKw+IHyx DCHA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=NNnOrn2g; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s1-20020a1709064d8100b0099bcdfb188fsi11443284eju.743.2023.08.03.05.56.44; Thu, 03 Aug 2023 05:57:09 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b=NNnOrn2g; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234078AbjHCLMw (ORCPT <rfc822;jeff.pang.chn@gmail.com> + 99 others); Thu, 3 Aug 2023 07:12:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51164 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234240AbjHCLMn (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 3 Aug 2023 07:12:43 -0400 Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D5003AA9 for <linux-kernel@vger.kernel.org>; Thu, 3 Aug 2023 04:12:36 -0700 (PDT) Received: by mail-ed1-x529.google.com with SMTP id 4fb4d7f45d1cf-5221cf2bb8cso1060305a12.1 for <linux-kernel@vger.kernel.org>; Thu, 03 Aug 2023 04:12:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; t=1691061154; x=1691665954; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=s50wqnYcuW7vCWAmLFI5zKlzJEcFsQM28qc0cEKNUSo=; b=NNnOrn2gpHzni6r0+I2E44v+NVp/J6WhE/ZyRjnjSW7UiRI21X/nEw241plybZTaFx MIj7txsnMcYyEMgjv9F2V8eRJJXf0z9bDfq/CAkBCa0EvowLEey2xKcSgK0Yk8Vh7HHj 5ZEyJvn7kzkfohoQIYhUF0XltgvgniI5rRd1HJEhCZQ+XkrOxEeT3l6IRZy3+dVRfaVX WqDkFEXmE0ATnWx9+reCF6zjZ0eeMtiZ5ihnmieiaA7tvVgEOCcpLWFG/ez4prQ8OoP8 Ed+TKcEBAF9dCb4fxh3s9sJk6jBxNHz9vzt4AEC6mMkOaKb/9Z3r5AEsjuiPpSYO6sYc +glw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691061154; x=1691665954; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=s50wqnYcuW7vCWAmLFI5zKlzJEcFsQM28qc0cEKNUSo=; b=CWCwvHoSlk4I7UC7w+BAAjraP94w5AshxcMJEPxcHZd2AcNiODDmzO2Jl/cLj/ER/o t8LN7JulTQjCy05dkddA1OXpBQuIalRio69a9Jh1D+jClQfb7LWExShF3ghcX4duros+ BZXOK2abc2QjqkZ5YC91ghMF9uJyiTUop5t+aMGOte7yzhnR2U9zm7b0/WSfFFsLbrdc t6oz2ajSEIMzOkyN/0BaSWubleemKWKOmOriht6o3CY90vyP0LL09Y0btlnR9Watwxp3 U45P7//ZhGGwhmP7vU5QlTKnsy3Nss4youDfU9Ldhes8QIReqng8MMPH7cpbfx86aleV jVgw== X-Gm-Message-State: ABy/qLbiBvuV43NjrI1q2bfFrtPriY0JjzlciMKA6Db7EnbRAW3hShe5 kwkWrQh+4dWH9ipX3NU8krqPhA== X-Received: by 2002:a05:6402:1847:b0:521:d23b:f2c5 with SMTP id v7-20020a056402184700b00521d23bf2c5mr7105134edy.14.1691061154721; Thu, 03 Aug 2023 04:12:34 -0700 (PDT) Received: from stroh80.sec.9e.network (ip-078-094-000-051.um19.pools.vodafone-ip.de. [78.94.0.51]) by smtp.gmail.com with ESMTPSA id n17-20020aa7c691000000b0051de018af1esm10027688edq.59.2023.08.03.04.12.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Aug 2023 04:12:34 -0700 (PDT) From: Naresh Solanki <naresh.solanki@9elements.com> X-Google-Original-From: Naresh Solanki <Naresh.Solanki@9elements.com> To: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org> Cc: zev@bewilderbeest.net, Naresh Solanki <Naresh.Solanki@9elements.com>, linux-kernel@vger.kernel.org Subject: [PATCH] regulator: userspace-consumer: Add regulator event support Date: Thu, 3 Aug 2023 13:12:25 +0200 Message-ID: <20230803111225.107572-1-Naresh.Solanki@9elements.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1773212721351768801 X-GMAIL-MSGID: 1773212721351768801 |
Series |
regulator: userspace-consumer: Add regulator event support
|
|
Commit Message
Naresh Solanki
Aug. 3, 2023, 11:12 a.m. UTC
Add sysfs attribute to track regulator events received from regulator
notifier block handler.
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b
Comments
On Thu, 03 Aug 2023 13:12:25 +0200, Naresh Solanki wrote: > Add sysfs attribute to track regulator events received from regulator > notifier block handler. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: userspace-consumer: Add regulator event support commit: 22475bcc2083196544fa55b861d76e0e7ee9da11 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote: >Add sysfs attribute to track regulator events received from regulator >notifier block handler. > Hi Naresh, Could you provide a bit more detail on how this is intended to be used? Some of the details (more below) seem a bit odd to me... >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >--- > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c >index 97f075ed68c9..a0b980022993 100644 >--- a/drivers/regulator/userspace-consumer.c >+++ b/drivers/regulator/userspace-consumer.c >@@ -29,6 +29,10 @@ struct userspace_consumer_data { > > int num_supplies; > struct regulator_bulk_data *supplies; >+ >+ struct kobject *kobj; >+ struct notifier_block nb; >+ unsigned long events; > }; > > static ssize_t name_show(struct device *dev, >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr, > return count; > } > >+static DEFINE_MUTEX(events_lock); >+ >+static ssize_t events_show(struct device *dev, >+ struct device_attribute *attr, char *buf) >+{ >+ struct userspace_consumer_data *data = dev_get_drvdata(dev); >+ unsigned long e; >+ >+ mutex_lock(&events_lock); >+ e = data->events; >+ data->events = 0; ...particularly this bit -- a read operation on a read-only file (and especially one with 0644 permissions) having side-effects (clearing the value it accesses) seems on the face of it like fairly surprising behavior. Is this a pattern that's used elsewhere in any other sysfs files? >+ mutex_unlock(&events_lock); >+ >+ return sprintf(buf, "0x%lx\n", e); >+} >+ > static DEVICE_ATTR_RO(name); > static DEVICE_ATTR_RW(state); >+static DEVICE_ATTR_RO(events); New sysfs attributes should be documented in Documentation/ABI, which this appears to be missing. However, it looks like this would expose the values of all the REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that something we really want to do? > > static struct attribute *attributes[] = { > &dev_attr_name.attr, > &dev_attr_state.attr, >+ &dev_attr_events.attr, > NULL, > }; > >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = { > .is_visible = attr_visible, > }; > >+static int regulator_userspace_notify(struct notifier_block *nb, >+ unsigned long event, >+ void *ignored) >+{ >+ struct userspace_consumer_data *data = >+ container_of(nb, struct userspace_consumer_data, nb); >+ >+ mutex_lock(&events_lock); >+ data->events |= event; >+ mutex_unlock(&events_lock); >+ Using a single global mutex (events_lock) to protect a single member of a per-device struct looks weird. Unless there's something subtle going on that I'm not seeing, it seems like the lock should be a member of the data struct instead of global, and since no blocking operations happen under it could it just be a spinlock? Or since it's just some simple updates to a single variable, why not just use an atomic_t and skip the lock entirely? >+ sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name); >+ >+ return NOTIFY_OK; >+} >+ > static int regulator_userspace_consumer_probe(struct platform_device *pdev) > { > struct regulator_userspace_consumer_data tmpdata; > struct regulator_userspace_consumer_data *pdata; > struct userspace_consumer_data *drvdata; >- int ret; >+ int i, ret; > > pdata = dev_get_platdata(&pdev->dev); > if (!pdata) { >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > drvdata->num_supplies = pdata->num_supplies; > drvdata->supplies = pdata->supplies; > drvdata->no_autoswitch = pdata->no_autoswitch; >+ drvdata->kobj = &pdev->dev.kobj; > > mutex_init(&drvdata->lock); > >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > } > drvdata->enabled = !!ret; > >+ drvdata->nb.notifier_call = regulator_userspace_notify; >+ for (i = 0; i < drvdata->num_supplies; i++) { >+ ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb); >+ if (ret) >+ goto err_enable; >+ } >+ > return 0; > > err_enable: >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > static int regulator_userspace_consumer_remove(struct platform_device *pdev) > { > struct userspace_consumer_data *data = platform_get_drvdata(pdev); >+ int i; >+ >+ for (i = 0; i < data->num_supplies; i++) >+ devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb); > > sysfs_remove_group(&pdev->dev.kobj, &attr_group); > > >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b >-- >2.41.0 >
Hi Zev, On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote: > > On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote: > >Add sysfs attribute to track regulator events received from regulator > >notifier block handler. > > > > Hi Naresh, > > Could you provide a bit more detail on how this is intended to be used? > Some of the details (more below) seem a bit odd to me... My application registers a event callback on the 'events' to track regulator events Reference: https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258 > > >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > >--- > > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++- > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c > >index 97f075ed68c9..a0b980022993 100644 > >--- a/drivers/regulator/userspace-consumer.c > >+++ b/drivers/regulator/userspace-consumer.c > >@@ -29,6 +29,10 @@ struct userspace_consumer_data { > > > > int num_supplies; > > struct regulator_bulk_data *supplies; > >+ > >+ struct kobject *kobj; > >+ struct notifier_block nb; > >+ unsigned long events; > > }; > > > > static ssize_t name_show(struct device *dev, > >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr, > > return count; > > } > > > >+static DEFINE_MUTEX(events_lock); > >+ > >+static ssize_t events_show(struct device *dev, > >+ struct device_attribute *attr, char *buf) > >+{ > >+ struct userspace_consumer_data *data = dev_get_drvdata(dev); > >+ unsigned long e; > >+ > >+ mutex_lock(&events_lock); > >+ e = data->events; > >+ data->events = 0; > > ...particularly this bit -- a read operation on a read-only file (and > especially one with 0644 permissions) having side-effects (clearing the > value it accesses) seems on the face of it like fairly surprising > behavior. Is this a pattern that's used elsewhere in any other sysfs > files? These are regulator events & are valid when it occurs. Userspace application is intended to consume them as soon as the event is notified by kernel sysfs_notify. > > >+ mutex_unlock(&events_lock); > >+ > >+ return sprintf(buf, "0x%lx\n", e); > >+} > >+ > > static DEVICE_ATTR_RO(name); > > static DEVICE_ATTR_RW(state); > >+static DEVICE_ATTR_RO(events); > > New sysfs attributes should be documented in Documentation/ABI, which > this appears to be missing. Sure I can check. > > However, it looks like this would expose the values of all the > REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that > something we really want to do? Yes. > > > > > static struct attribute *attributes[] = { > > &dev_attr_name.attr, > > &dev_attr_state.attr, > >+ &dev_attr_events.attr, > > NULL, > > }; > > > >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = { > > .is_visible = attr_visible, > > }; > > > >+static int regulator_userspace_notify(struct notifier_block *nb, > >+ unsigned long event, > >+ void *ignored) > >+{ > >+ struct userspace_consumer_data *data = > >+ container_of(nb, struct userspace_consumer_data, nb); > >+ > >+ mutex_lock(&events_lock); > >+ data->events |= event; > >+ mutex_unlock(&events_lock); > >+ > > Using a single global mutex (events_lock) to protect a single member of > a per-device struct looks weird. Unless there's something subtle going > on that I'm not seeing, it seems like the lock should be a member of the > data struct instead of global, and since no blocking operations happen > under it could it just be a spinlock? Or since it's just some simple > updates to a single variable, why not just use an atomic_t and skip the > lock entirely? Intent is that only one thread at a time is to be allowed to access/modify the data->events variable to prevent potential data corruption and race conditions. Sure can change it to spinlock or atomic_t. > > >+ sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name); > >+ > >+ return NOTIFY_OK; > >+} > >+ > > static int regulator_userspace_consumer_probe(struct platform_device *pdev) > > { > > struct regulator_userspace_consumer_data tmpdata; > > struct regulator_userspace_consumer_data *pdata; > > struct userspace_consumer_data *drvdata; > >- int ret; > >+ int i, ret; > > > > pdata = dev_get_platdata(&pdev->dev); > > if (!pdata) { > >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > > drvdata->num_supplies = pdata->num_supplies; > > drvdata->supplies = pdata->supplies; > > drvdata->no_autoswitch = pdata->no_autoswitch; > >+ drvdata->kobj = &pdev->dev.kobj; > > > > mutex_init(&drvdata->lock); > > > >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > > } > > drvdata->enabled = !!ret; > > > >+ drvdata->nb.notifier_call = regulator_userspace_notify; > >+ for (i = 0; i < drvdata->num_supplies; i++) { > >+ ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb); > >+ if (ret) > >+ goto err_enable; > >+ } > >+ > > return 0; > > > > err_enable: > >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > > static int regulator_userspace_consumer_remove(struct platform_device *pdev) > > { > > struct userspace_consumer_data *data = platform_get_drvdata(pdev); > >+ int i; > >+ > >+ for (i = 0; i < data->num_supplies; i++) > >+ devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb); > > > > sysfs_remove_group(&pdev->dev.kobj, &attr_group); > > > > > >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b > >-- > >2.41.0 > >
On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote: >Hi Zev, > > >On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote: >> >> On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote: >> >Add sysfs attribute to track regulator events received from regulator >> >notifier block handler. >> > >> >> Hi Naresh, >> >> Could you provide a bit more detail on how this is intended to be used? >> Some of the details (more below) seem a bit odd to me... >My application registers a event callback on the 'events' to track regulator >events >Reference: >https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258 >> >> >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> >> >--- >> > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++- >> > 1 file changed, 51 insertions(+), 1 deletion(-) >> > >> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c >> >index 97f075ed68c9..a0b980022993 100644 >> >--- a/drivers/regulator/userspace-consumer.c >> >+++ b/drivers/regulator/userspace-consumer.c >> >@@ -29,6 +29,10 @@ struct userspace_consumer_data { >> > >> > int num_supplies; >> > struct regulator_bulk_data *supplies; >> >+ >> >+ struct kobject *kobj; >> >+ struct notifier_block nb; >> >+ unsigned long events; >> > }; >> > >> > static ssize_t name_show(struct device *dev, >> >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr, >> > return count; >> > } >> > >> >+static DEFINE_MUTEX(events_lock); >> >+ >> >+static ssize_t events_show(struct device *dev, >> >+ struct device_attribute *attr, char *buf) >> >+{ >> >+ struct userspace_consumer_data *data = dev_get_drvdata(dev); >> >+ unsigned long e; >> >+ >> >+ mutex_lock(&events_lock); >> >+ e = data->events; >> >+ data->events = 0; >> >> ...particularly this bit -- a read operation on a read-only file (and >> especially one with 0644 permissions) having side-effects (clearing the >> value it accesses) seems on the face of it like fairly surprising >> behavior. Is this a pattern that's used elsewhere in any other sysfs >> files? >These are regulator events & are valid when it occurs. >Userspace application is intended to consume them as soon as the >event is notified by kernel sysfs_notify. > Sure, but that doesn't really address what I was concerned about -- as written this is a read operation on a read-only file (0444, not 0644 as I mistakenly wrote above) that nevertheless alters the state of an internal kernel data structure. Can you point to any other sysfs attributes that behave like that? I can't think of one offhand, and I'd be reluctant to establish the precedent. Would a uevent-based mechanism maybe be a better fit for the problem you're trying to solve? >> >> >+ mutex_unlock(&events_lock); >> >+ >> >+ return sprintf(buf, "0x%lx\n", e); >> >+} >> >+ >> > static DEVICE_ATTR_RO(name); >> > static DEVICE_ATTR_RW(state); >> >+static DEVICE_ATTR_RO(events); >> >> New sysfs attributes should be documented in Documentation/ABI, which >> this appears to be missing. >Sure I can check. >> >> However, it looks like this would expose the values of all the >> REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that >> something we really want to do? >Yes. Given that the REGULATOR_EVENT_* constants are defined in headers under include/linux and not include/uapi, it doesn't seem like they were intended to be used as part of a userspace-visible interface. If they're going to be, I think they should be moved to the uapi directory so that applications can use the proper definitions from the kernel instead of manually replicating it on their own (but I suspect we should probably find a different approach instead). >> >> > >> > static struct attribute *attributes[] = { >> > &dev_attr_name.attr, >> > &dev_attr_state.attr, >> >+ &dev_attr_events.attr, >> > NULL, >> > }; >> > >> >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = { >> > .is_visible = attr_visible, >> > }; >> > >> >+static int regulator_userspace_notify(struct notifier_block *nb, >> >+ unsigned long event, >> >+ void *ignored) >> >+{ >> >+ struct userspace_consumer_data *data = >> >+ container_of(nb, struct userspace_consumer_data, nb); >> >+ >> >+ mutex_lock(&events_lock); >> >+ data->events |= event; >> >+ mutex_unlock(&events_lock); >> >+ >> >> Using a single global mutex (events_lock) to protect a single member of >> a per-device struct looks weird. Unless there's something subtle going >> on that I'm not seeing, it seems like the lock should be a member of the >> data struct instead of global, and since no blocking operations happen >> under it could it just be a spinlock? Or since it's just some simple >> updates to a single variable, why not just use an atomic_t and skip the >> lock entirely? >Intent is that only one thread at a time is to be allowed to access/modify >the data->events variable to prevent potential data corruption and >race conditions. Sure can change it to spinlock or atomic_t. > >> >> >+ sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name); >> >+ >> >+ return NOTIFY_OK; >> >+} >> >+ >> > static int regulator_userspace_consumer_probe(struct platform_device *pdev) >> > { >> > struct regulator_userspace_consumer_data tmpdata; >> > struct regulator_userspace_consumer_data *pdata; >> > struct userspace_consumer_data *drvdata; >> >- int ret; >> >+ int i, ret; >> > >> > pdata = dev_get_platdata(&pdev->dev); >> > if (!pdata) { >> >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) >> > drvdata->num_supplies = pdata->num_supplies; >> > drvdata->supplies = pdata->supplies; >> > drvdata->no_autoswitch = pdata->no_autoswitch; >> >+ drvdata->kobj = &pdev->dev.kobj; >> > >> > mutex_init(&drvdata->lock); >> > >> >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) >> > } >> > drvdata->enabled = !!ret; >> > >> >+ drvdata->nb.notifier_call = regulator_userspace_notify; >> >+ for (i = 0; i < drvdata->num_supplies; i++) { >> >+ ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb); >> >+ if (ret) >> >+ goto err_enable; >> >+ } >> >+ >> > return 0; >> > >> > err_enable: >> >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) >> > static int regulator_userspace_consumer_remove(struct platform_device *pdev) >> > { >> > struct userspace_consumer_data *data = platform_get_drvdata(pdev); >> >+ int i; >> >+ >> >+ for (i = 0; i < data->num_supplies; i++) >> >+ devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb); >> > >> > sysfs_remove_group(&pdev->dev.kobj, &attr_group); >> > >> > >> >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b >> >-- >> >2.41.0 >> >
Hi Zev, On Fri, 4 Aug 2023 at 17:32, Zev Weiss <zev@bewilderbeest.net> wrote: > > On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote: > >Hi Zev, > > > > > >On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@bewilderbeest.net> wrote: > >> > >> On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote: > >> >Add sysfs attribute to track regulator events received from regulator > >> >notifier block handler. > >> > > >> > >> Hi Naresh, > >> > >> Could you provide a bit more detail on how this is intended to be used? > >> Some of the details (more below) seem a bit odd to me... > >My application registers a event callback on the 'events' to track regulator > >events > >Reference: > >https://github.com/9elements/pwrseqd/blob/main/src/VoltageRegulatorSysfs.cpp#L258 > >> > >> >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > >> >--- > >> > drivers/regulator/userspace-consumer.c | 52 +++++++++++++++++++++++++- > >> > 1 file changed, 51 insertions(+), 1 deletion(-) > >> > > >> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c > >> >index 97f075ed68c9..a0b980022993 100644 > >> >--- a/drivers/regulator/userspace-consumer.c > >> >+++ b/drivers/regulator/userspace-consumer.c > >> >@@ -29,6 +29,10 @@ struct userspace_consumer_data { > >> > > >> > int num_supplies; > >> > struct regulator_bulk_data *supplies; > >> >+ > >> >+ struct kobject *kobj; > >> >+ struct notifier_block nb; > >> >+ unsigned long events; > >> > }; > >> > > >> > static ssize_t name_show(struct device *dev, > >> >@@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr, > >> > return count; > >> > } > >> > > >> >+static DEFINE_MUTEX(events_lock); > >> >+ > >> >+static ssize_t events_show(struct device *dev, > >> >+ struct device_attribute *attr, char *buf) > >> >+{ > >> >+ struct userspace_consumer_data *data = dev_get_drvdata(dev); > >> >+ unsigned long e; > >> >+ > >> >+ mutex_lock(&events_lock); > >> >+ e = data->events; > >> >+ data->events = 0; > >> > >> ...particularly this bit -- a read operation on a read-only file (and > >> especially one with 0644 permissions) having side-effects (clearing the > >> value it accesses) seems on the face of it like fairly surprising > >> behavior. Is this a pattern that's used elsewhere in any other sysfs > >> files? > >These are regulator events & are valid when it occurs. > >Userspace application is intended to consume them as soon as the > >event is notified by kernel sysfs_notify. > > > > Sure, but that doesn't really address what I was concerned about -- as > written this is a read operation on a read-only file (0444, not 0644 as > I mistakenly wrote above) that nevertheless alters the state of an > internal kernel data structure. Can you point to any other sysfs > attributes that behave like that? I can't think of one offhand, and I'd > be reluctant to establish the precedent. I guess many hwmon properties on input are readonly & its possible to send sysfs_notify on the properties. Like in https://github.com/torvalds/linux/blob/master/drivers/hwmon/hwmon.c#L668 > > Would a uevent-based mechanism maybe be a better fit for the problem > you're trying to solve? If the application also needs uevent then that can be added as done in hwmon. > > >> > >> >+ mutex_unlock(&events_lock); > >> >+ > >> >+ return sprintf(buf, "0x%lx\n", e); > >> >+} > >> >+ > >> > static DEVICE_ATTR_RO(name); > >> > static DEVICE_ATTR_RW(state); > >> >+static DEVICE_ATTR_RO(events); > >> > >> New sysfs attributes should be documented in Documentation/ABI, which > >> this appears to be missing. > >Sure I can check. For Documentation/ABI, 'sysfs-driver-regulator-output' below. let me know if this looks ok. What: /sys/bus/platform/drivers/regulator-output/*/events Date: August 2023 Contact: Naresh Solanki <naresh.solanki@9elements.com> Description: Provided regulator events. Read provides various events the regulator associated with the driver has encountered. All REGULATOR_EVENT_* are defined in include/linux/regulator/consumer.h e.g. cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events 0x0 > >> > >> However, it looks like this would expose the values of all the > >> REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that > >> something we really want to do? > >Yes. > > Given that the REGULATOR_EVENT_* constants are defined in headers under > include/linux and not include/uapi, it doesn't seem like they were > intended to be used as part of a userspace-visible interface. If > they're going to be, I think they should be moved to the uapi directory > so that applications can use the proper definitions from the kernel > instead of manually replicating it on their own (but I suspect we should > probably find a different approach instead). Yes they have to be moved to include/uapi in that case. > > >> > >> > > >> > static struct attribute *attributes[] = { > >> > &dev_attr_name.attr, > >> > &dev_attr_state.attr, > >> >+ &dev_attr_events.attr, > >> > NULL, > >> > }; > >> > > >> >@@ -115,12 +137,28 @@ static const struct attribute_group attr_group = { > >> > .is_visible = attr_visible, > >> > }; > >> > > >> >+static int regulator_userspace_notify(struct notifier_block *nb, > >> >+ unsigned long event, > >> >+ void *ignored) > >> >+{ > >> >+ struct userspace_consumer_data *data = > >> >+ container_of(nb, struct userspace_consumer_data, nb); > >> >+ > >> >+ mutex_lock(&events_lock); > >> >+ data->events |= event; > >> >+ mutex_unlock(&events_lock); > >> >+ > >> > >> Using a single global mutex (events_lock) to protect a single member of > >> a per-device struct looks weird. Unless there's something subtle going > >> on that I'm not seeing, it seems like the lock should be a member of the > >> data struct instead of global, and since no blocking operations happen > >> under it could it just be a spinlock? Or since it's just some simple > >> updates to a single variable, why not just use an atomic_t and skip the > >> lock entirely? > >Intent is that only one thread at a time is to be allowed to access/modify > >the data->events variable to prevent potential data corruption and > >race conditions. Sure can change it to spinlock or atomic_t. > > > >> > >> >+ sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name); > >> >+ > >> >+ return NOTIFY_OK; > >> >+} > >> >+ > >> > static int regulator_userspace_consumer_probe(struct platform_device *pdev) > >> > { > >> > struct regulator_userspace_consumer_data tmpdata; > >> > struct regulator_userspace_consumer_data *pdata; > >> > struct userspace_consumer_data *drvdata; > >> >- int ret; > >> >+ int i, ret; > >> > > >> > pdata = dev_get_platdata(&pdev->dev); > >> > if (!pdata) { > >> >@@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > >> > drvdata->num_supplies = pdata->num_supplies; > >> > drvdata->supplies = pdata->supplies; > >> > drvdata->no_autoswitch = pdata->no_autoswitch; > >> >+ drvdata->kobj = &pdev->dev.kobj; > >> > > >> > mutex_init(&drvdata->lock); > >> > > >> >@@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > >> > } > >> > drvdata->enabled = !!ret; > >> > > >> >+ drvdata->nb.notifier_call = regulator_userspace_notify; > >> >+ for (i = 0; i < drvdata->num_supplies; i++) { > >> >+ ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb); > >> >+ if (ret) > >> >+ goto err_enable; > >> >+ } > >> >+ > >> > return 0; > >> > > >> > err_enable: > >> >@@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) > >> > static int regulator_userspace_consumer_remove(struct platform_device *pdev) > >> > { > >> > struct userspace_consumer_data *data = platform_get_drvdata(pdev); > >> >+ int i; > >> >+ > >> >+ for (i = 0; i < data->num_supplies; i++) > >> >+ devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb); > >> > > >> > sysfs_remove_group(&pdev->dev.kobj, &attr_group); > >> > > >> > > >> >base-commit: 4fb53b2377c364e3753d6e293913b57dad68e98b > >> >-- > >> >2.41.0 > >> >
diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c index 97f075ed68c9..a0b980022993 100644 --- a/drivers/regulator/userspace-consumer.c +++ b/drivers/regulator/userspace-consumer.c @@ -29,6 +29,10 @@ struct userspace_consumer_data { int num_supplies; struct regulator_bulk_data *supplies; + + struct kobject *kobj; + struct notifier_block nb; + unsigned long events; }; static ssize_t name_show(struct device *dev, @@ -89,12 +93,30 @@ static ssize_t state_store(struct device *dev, struct device_attribute *attr, return count; } +static DEFINE_MUTEX(events_lock); + +static ssize_t events_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct userspace_consumer_data *data = dev_get_drvdata(dev); + unsigned long e; + + mutex_lock(&events_lock); + e = data->events; + data->events = 0; + mutex_unlock(&events_lock); + + return sprintf(buf, "0x%lx\n", e); +} + static DEVICE_ATTR_RO(name); static DEVICE_ATTR_RW(state); +static DEVICE_ATTR_RO(events); static struct attribute *attributes[] = { &dev_attr_name.attr, &dev_attr_state.attr, + &dev_attr_events.attr, NULL, }; @@ -115,12 +137,28 @@ static const struct attribute_group attr_group = { .is_visible = attr_visible, }; +static int regulator_userspace_notify(struct notifier_block *nb, + unsigned long event, + void *ignored) +{ + struct userspace_consumer_data *data = + container_of(nb, struct userspace_consumer_data, nb); + + mutex_lock(&events_lock); + data->events |= event; + mutex_unlock(&events_lock); + + sysfs_notify(data->kobj, NULL, dev_attr_events.attr.name); + + return NOTIFY_OK; +} + static int regulator_userspace_consumer_probe(struct platform_device *pdev) { struct regulator_userspace_consumer_data tmpdata; struct regulator_userspace_consumer_data *pdata; struct userspace_consumer_data *drvdata; - int ret; + int i, ret; pdata = dev_get_platdata(&pdev->dev); if (!pdata) { @@ -153,6 +191,7 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) drvdata->num_supplies = pdata->num_supplies; drvdata->supplies = pdata->supplies; drvdata->no_autoswitch = pdata->no_autoswitch; + drvdata->kobj = &pdev->dev.kobj; mutex_init(&drvdata->lock); @@ -186,6 +225,13 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) } drvdata->enabled = !!ret; + drvdata->nb.notifier_call = regulator_userspace_notify; + for (i = 0; i < drvdata->num_supplies; i++) { + ret = devm_regulator_register_notifier(drvdata->supplies[i].consumer, &drvdata->nb); + if (ret) + goto err_enable; + } + return 0; err_enable: @@ -197,6 +243,10 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev) static int regulator_userspace_consumer_remove(struct platform_device *pdev) { struct userspace_consumer_data *data = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < data->num_supplies; i++) + devm_regulator_unregister_notifier(data->supplies[i].consumer, &data->nb); sysfs_remove_group(&pdev->dev.kobj, &attr_group);