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 X-Spam-Level: X-Spam-Status: No, score=-15.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98114C433E0 for ; Thu, 21 Jan 2021 18:29:19 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4C07C23A40 for ; Thu, 21 Jan 2021 18:29:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4C07C23A40 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=Huawei.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=7lAjNQ9bZ+fee4wWXQL0csS2kCWT1bZhoVHaeSIQJMQ=; b=bqtbbCVshV1Lv37gGlPbcr7Qs vHQX7S/GG+roLmeknk1zHrIrp3SHMUc3eLwgH+vPu3b2wNQ87DCnI0ksanbHpWW3ZWUm7q+Dealej Vx8Qgl2+hKwUxpudbuO7glyUHbysjMS3jLShdpANkDOMXaxDK0htD8vtgEh61tgHQ2Z4SZvJBN+dk tGQxoebzzZXZRQ7UwYyE1NQKJeaHKMesBMlAyC3LixbvBdh2ecftdlMDhpIGoyHnI2JnGqC0IJbCu 7/ZBySh05ed/11AKKuJeVTCLIVW/NwNC4qJdSUGXsKKsQnCqWppeAAmYhs0IqJP6/f2BmiebLvCMX sJwxuk4rg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2egS-0002Ww-Py; Thu, 21 Jan 2021 18:27:52 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2egQ-0002VK-Q3 for linux-arm-kernel@lists.infradead.org; Thu, 21 Jan 2021 18:27:51 +0000 Received: from fraeml701-chm.china.huawei.com (unknown [172.18.147.201]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4DM9ht26mvz67cZB; Fri, 22 Jan 2021 02:22:06 +0800 (CST) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by fraeml701-chm.china.huawei.com (10.206.15.50) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2106.2; Thu, 21 Jan 2021 19:27:40 +0100 Received: from localhost (10.47.68.198) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Thu, 21 Jan 2021 18:27:39 +0000 Date: Thu, 21 Jan 2021 18:26:58 +0000 From: Jonathan Cameron To: William Breathitt Gray Subject: Re: [PATCH v6 3/5] counter: Add character device interface Message-ID: <20210121182658.00000d95@Huawei.com> In-Reply-To: References: <20210119092022.GA14502@pengutronix.de> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; i686-w64-mingw32) MIME-Version: 1.0 X-Originating-IP: [10.47.68.198] X-ClientProxiedBy: lhreml719-chm.china.huawei.com (10.201.108.70) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210121_132751_043639_A45C2A0E X-CRM114-Status: GOOD ( 38.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kernel@pengutronix.de, kamel.bouhara@bootlin.com, gwendal@chromium.org, a.fatoum@pengutronix.de, david@lechnology.com, linux-iio@vger.kernel.org, patrick.havelange@essensium.com, jic23@kernel.org, alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org, Oleksij Rempel , Dan Carpenter , mcoquelin.stm32@gmail.com, David Jander , fabrice.gasnier@st.com, syednwaris@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, alexandre.torgue@st.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 21 Jan 2021 17:03:11 +0900 William Breathitt Gray wrote: > On Tue, Jan 19, 2021 at 10:20:22AM +0100, Oleksij Rempel wrote: > > On Sun, Nov 22, 2020 at 03:29:54PM -0500, William Breathitt Gray wrote: > > > This patch introduces a character device interface for the Counter > > > subsystem. Device data is exposed through standard character device read > > > operations. Device data is gathered when a Counter event is pushed by > > > the respective Counter device driver. Configuration is handled via ioctl > > > operations on the respective Counter character device node. > > > > > > Cc: David Lechner > > > Cc: Gwendal Grignou > > > Cc: Dan Carpenter > > > Signed-off-by: William Breathitt Gray > > > --- > > > > Hello William, > > > > the series looks quite interesting, we have some thoughts... see below: > > > > [...] > > > +/** > > > + * counter_push_event - queue event for userspace reading > > > + * @counter: pointer to Counter structure > > > + * @event: triggered event > > > + * @channel: event channel > > > + * > > > + * Note: If no one is watching for the respective event, it is silently > > > + * discarded. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int counter_push_event(struct counter_device *const counter, const u8 event, > > > + const u8 channel) > > > +{ > > > + struct counter_event ev = {0}; > > > + unsigned int copied = 0; > > > + unsigned long flags; > > > + struct counter_event_node *event_node; > > > + struct counter_comp_node *comp_node; > > > + int err = 0; > > > + > > > + ev.timestamp = ktime_get_ns(); > > > + ev.watch.event = event; > > > + ev.watch.channel = channel; > > > + > > > + raw_spin_lock_irqsave(&counter->events_lock, flags); > > > + > > > + /* Search for event in the list */ > > > + list_for_each_entry(event_node, &counter->events_list, l) > > > + if (event_node->event == event && > > > + event_node->channel == channel) > > > + break; > > > + > > > + /* If event is not in the list */ > > > + if (&event_node->l == &counter->events_list) > > > + goto exit_early; > > > + > > > + /* Read and queue relevant comp for userspace */ > > > + list_for_each_entry(comp_node, &event_node->comp_list, l) { > > > + err = counter_get_data(counter, comp_node, &ev.value); > > > + if (err) > > > + goto exit_early; > > > + > > > + ev.watch.component = comp_node->component; > > > + > > > + copied += kfifo_put(&counter->events, ev); > > > > We want to calculate the frequency of some IRQ pulses in user space and > > counter values with time stamps really fits well here. As the pulses are > > from a physical system (rotating wheel), they will only change at a > > certain rate. We want to have the possibility to read from the counter > > device less often, we intentionally want to skip (meaning miss) > > events. > > > > When reading we're interested in the newest events. The kfifo implements > > a "tail" drop FIFO, which means new values are added at the end, and if > > the FIFO is full, they are dropped. We need a "head" drop FIFO which > > discards the oldest events, keeping only the recent ones. > > > > As far as we know, kfifo doesn't offer a head drop mode, but I think > > this can be added. > > I'm not sure if kfifo has this kind of mode, but it seems like something > that should be there if it is not already -- I imagine this kind of > operation mode would be pretty common. Perhaps someone knows how to > achieve this and will share here. > I don't think it does. In IIO we started with a ring (effectively this) - hopefully no one else remembers the horrible mess I made of it ;), but eventually decided that there were very few usecases that actually demanded it so just switched over to kfifo. It's actually pretty rare that you aren't better off draining to a backing store of some type. Not had any demand to bring a ring back to IIO since we dropped it. > > > > [...] > > > > > struct counter_device { > > > const char *name; > > > @@ -270,12 +270,20 @@ struct counter_device { > > > > > > int id; > > > struct device dev; > > > + struct cdev chrdev; > > > + raw_spinlock_t events_lock; > > > + struct list_head events_list; > > > + struct list_head next_events_list; > > > + DECLARE_KFIFO(events, struct counter_event, 64); > > > > Do you plan to make the size of the FIFO configurable? > > > > regards, > > Oleksij & Marc > > I suppose it wouldn't be a problem to make this a configurable setting; > I think I can implement this via kfifo_alloc() and kfifo_free(). How > would users control this -- maybe using a sysfs attribute? Would make sense to do it from sysfs. Jonathan > > William Breathitt Gray > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel