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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E0C2DC433F5 for ; Thu, 29 Sep 2022 12:14:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=Tmi3wV/3LKBE79n0KfHsjxLZo3/gCFl6sRnP8Z42mb4=; b=yOsyK5a8rYxlwy 5MdFhLFi1XvnZXfpdZfkeelB7mgqSZh92sxux0+vo4flU8oZDt5B7p3PdQM6xKe4PQVTSWrvqqBh2 POerLlxfwCTqGhfKRTw7ocvPSRi57T6qPKIUiNzF2Gr5x18KVYQvlRcgHjkNHk2HW1eltkYzDKVsE mKJ6a0WME9L/RMf/9Gk85vxbMzyw0yqXs20jlZaU9Da/9+9S6x25fWcd6wuj84yRwOfCMXsBxWdqk 7MqO6se5KeUyKAilsG+nH6zdUGWr3vCnNwXQRJCUsRePztS+8+J0XCLVyYWWD2kdULcw+Ila+M70t aEwf3j1b8M6RrFlb92Sg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1odsPj-002sTr-Ih; Thu, 29 Sep 2022 12:13:15 +0000 Received: from mail-ej1-x62a.google.com ([2a00:1450:4864:20::62a]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1odsPe-002sT4-Jg for linux-arm-kernel@lists.infradead.org; Thu, 29 Sep 2022 12:13:13 +0000 Received: by mail-ej1-x62a.google.com with SMTP id nb11so2376594ejc.5 for ; Thu, 29 Sep 2022 05:13:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=TWfYOVwvTRjWIA4RZEYSetTqYlpxzdA06vkMaN0uTAs=; b=RxLKg96cFC1EkxlWDFl4LbMEt9bdRg5LiNPsPGcBgKZWK1w9mCdTCU7sZk+JOHU8Cc TTOY19OKOX/tEVso1kFDmG00+Zs3IP3bE1SWGArS8mHc6TstgZDdC5shFIlsYQ/gSGLW otoAXcmOknNHVEA67gy2QyXDJ+lZEXKAe920LZz6nfoWiL7rKiVi9myLfH7AoHhnHJD9 3yT+EbKi99wt+itd67V1+TDDYcYA7OGenH/CklhwdzWWVB0kaAGykOdT1nX/woS6qIh/ IS9TvQ1GWp3km8aA4l2Xt/1g+lbtMwAMD0PTagc3/QTdnc4H64qxxLACF91npztENo7U 7B8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=TWfYOVwvTRjWIA4RZEYSetTqYlpxzdA06vkMaN0uTAs=; b=u1/PCEH+eduxc1H6I0R/rZLAp6/HImBLgR5nDwjQJDN+fOvh/H+wLq9QXI8H2XY5Bc +y7nJn3K+moQWjXywW90GHbBJFNUAqc5AgKXZDG9Tko8wb6Sx7c66dWfX7AcoWAfcCGx EGQljRxjGvGSVRwhPCdOeyAGIm4q8FjMJ/qFaO3i06Bkk7nQLEElY6Ag+K9wDT6cR0xc 4QIIfDY9/7IIAcdxVhQ8PssdygiC4/tb8OzSksqjiRPxBxYDNrvsCuRrbZT8ZuSLuIZA VpPbXOD7hISCrr/KFaagf/fAZJaStfgUMovrYXEG7+jM3PYk1ErIMWI3AUg2kchKXnuu e4fA== X-Gm-Message-State: ACrzQf3avNetSCHn3RTMN0/9MygsdGmX7uScBNYGenLLoOoW9X9VwnZt 2PUl3Ra3K/L1jVxgOBxjoDIQJQ== X-Google-Smtp-Source: AMsMyM7oIIds9bRKxxMofnuB8GvqPVD5b7xk843c5yUZpTNxUSmi9iSoDCac+QpzbJFPef3sVq5xKg== X-Received: by 2002:a17:907:a079:b0:770:78cb:6650 with SMTP id ia25-20020a170907a07900b0077078cb6650mr2420390ejc.515.1664453587896; Thu, 29 Sep 2022 05:13:07 -0700 (PDT) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id r8-20020aa7da08000000b00456f2dbb379sm5282523eds.62.2022.09.29.05.13.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Sep 2022 05:13:07 -0700 (PDT) Date: Thu, 29 Sep 2022 14:13:05 +0200 From: Jiri Pirko To: Vadim Fedorenko Cc: Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Vadim Fedorenko , Aya Levin , netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org Subject: Re: [RFC PATCH v2 2/3] dpll: add netlink events Message-ID: References: <20220626192444.29321-1-vfedorenko@novek.ru> <20220626192444.29321-3-vfedorenko@novek.ru> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220626192444.29321-3-vfedorenko@novek.ru> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220929_051310_674753_E8DF40F3 X-CRM114-Status: GOOD ( 18.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@novek.ru wrote: >From: Vadim Fedorenko > >Add netlink interface to enable notification of users about >events in DPLL framework. Part of this interface should be >used by drivers directly, i.e. lock status changes. I don't get why this is a separate patch. I believe it should be squashed to the previous one, making it easier to review as a whole thing. > >Signed-off-by: Vadim Fedorenko >--- > drivers/dpll/dpll_core.c | 2 + > drivers/dpll/dpll_netlink.c | 141 ++++++++++++++++++++++++++++++++++++ > drivers/dpll/dpll_netlink.h | 7 ++ > 3 files changed, 150 insertions(+) > >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >index dc0330e3687d..387644aa910e 100644 >--- a/drivers/dpll/dpll_core.c >+++ b/drivers/dpll/dpll_core.c >@@ -97,6 +97,8 @@ struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_c > mutex_unlock(&dpll_device_xa_lock); > dpll->priv = priv; > >+ dpll_notify_device_create(dpll->id, dev_name(&dpll->dev)); >+ > return dpll; > > error: >diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c >index e15106f30377..4b1684fcf41e 100644 >--- a/drivers/dpll/dpll_netlink.c >+++ b/drivers/dpll/dpll_netlink.c >@@ -48,6 +48,8 @@ struct param { > int dpll_source_type; > int dpll_output_id; > int dpll_output_type; >+ int dpll_status; >+ const char *dpll_name; > }; > > struct dpll_dump_ctx { >@@ -239,6 +241,8 @@ static int dpll_genl_cmd_set_source(struct param *p) > ret = dpll->ops->set_source_type(dpll, src_id, type); > mutex_unlock(&dpll->lock); > >+ dpll_notify_source_change(dpll->id, src_id, type); >+ > return ret; > } > >@@ -262,6 +266,8 @@ static int dpll_genl_cmd_set_output(struct param *p) > ret = dpll->ops->set_source_type(dpll, out_id, type); > mutex_unlock(&dpll->lock); > >+ dpll_notify_source_change(dpll->id, out_id, type); >+ > return ret; > } > >@@ -438,6 +444,141 @@ static struct genl_family dpll_gnl_family __ro_after_init = { > .pre_doit = dpll_pre_doit, > }; > >+static int dpll_event_device_create(struct param *p) >+{ >+ if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) || >+ nla_put_string(p->msg, DPLLA_DEVICE_NAME, p->dpll_name)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int dpll_event_device_delete(struct param *p) >+{ >+ if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int dpll_event_status(struct param *p) >+{ >+ if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) || >+ nla_put_u32(p->msg, DPLLA_LOCK_STATUS, p->dpll_status)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int dpll_event_source_change(struct param *p) >+{ >+ if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) || >+ nla_put_u32(p->msg, DPLLA_SOURCE_ID, p->dpll_source_id) || >+ nla_put_u32(p->msg, DPLLA_SOURCE_TYPE, p->dpll_source_type)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int dpll_event_output_change(struct param *p) >+{ >+ if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) || >+ nla_put_u32(p->msg, DPLLA_OUTPUT_ID, p->dpll_output_id) || >+ nla_put_u32(p->msg, DPLLA_OUTPUT_TYPE, p->dpll_output_type)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static cb_t event_cb[] = { >+ [DPLL_EVENT_DEVICE_CREATE] = dpll_event_device_create, >+ [DPLL_EVENT_DEVICE_DELETE] = dpll_event_device_delete, >+ [DPLL_EVENT_STATUS_LOCKED] = dpll_event_status, >+ [DPLL_EVENT_STATUS_UNLOCKED] = dpll_event_status, >+ [DPLL_EVENT_SOURCE_CHANGE] = dpll_event_source_change, >+ [DPLL_EVENT_OUTPUT_CHANGE] = dpll_event_output_change, >+}; >+/* >+ * Generic netlink DPLL event encoding >+ */ >+static int dpll_send_event(enum dpll_genl_event event, >+ struct param *p) "struct param". Can't you please maintain some namespace for function/struct names? >+{ >+ struct sk_buff *msg; >+ int ret = -EMSGSIZE; >+ void *hdr; >+ >+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); >+ if (!msg) >+ return -ENOMEM; >+ p->msg = msg; >+ >+ hdr = genlmsg_put(msg, 0, 0, &dpll_gnl_family, 0, event); >+ if (!hdr) >+ goto out_free_msg; >+ >+ ret = event_cb[event](p); >+ if (ret) >+ goto out_cancel_msg; >+ >+ genlmsg_end(msg, hdr); >+ >+ genlmsg_multicast(&dpll_gnl_family, msg, 0, 1, GFP_KERNEL); >+ >+ return 0; >+ >+out_cancel_msg: >+ genlmsg_cancel(msg, hdr); >+out_free_msg: >+ nlmsg_free(msg); >+ >+ return ret; >+} >+ >+int dpll_notify_device_create(int dpll_id, const char *name) >+{ >+ struct param p = { .dpll_id = dpll_id, .dpll_name = name }; >+ >+ return dpll_send_event(DPLL_EVENT_DEVICE_CREATE, &p); Just do that automatically in register() and avoid the need to rely on drivers to be so good to do it themselves. They won't. >+} >+ >+int dpll_notify_device_delete(int dpll_id) >+{ >+ struct param p = { .dpll_id = dpll_id }; >+ >+ return dpll_send_event(DPLL_EVENT_DEVICE_DELETE, &p); >+} >+ >+int dpll_notify_status_locked(int dpll_id) For all notification functions called from the driver, please use struct dpll as an arg. >+{ >+ struct param p = { .dpll_id = dpll_id, .dpll_status = 1 }; >+ >+ return dpll_send_event(DPLL_EVENT_STATUS_LOCKED, &p); >+} >+ >+int dpll_notify_status_unlocked(int dpll_id) >+{ >+ struct param p = { .dpll_id = dpll_id, .dpll_status = 0 }; >+ >+ return dpll_send_event(DPLL_EVENT_STATUS_UNLOCKED, &p); >+} >+ >+int dpll_notify_source_change(int dpll_id, int source_id, int source_type) >+{ >+ struct param p = { .dpll_id = dpll_id, .dpll_source_id = source_id, >+ .dpll_source_type = source_type }; >+ >+ return dpll_send_event(DPLL_EVENT_SOURCE_CHANGE, &p); >+} >+ >+int dpll_notify_output_change(int dpll_id, int output_id, int output_type) >+{ >+ struct param p = { .dpll_id = dpll_id, .dpll_output_id = output_id, >+ .dpll_output_type = output_type }; >+ >+ return dpll_send_event(DPLL_EVENT_OUTPUT_CHANGE, &p); >+} >+ > int __init dpll_netlink_init(void) > { > return genl_register_family(&dpll_gnl_family); >diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h >index e2d100f59dd6..0dc81320f982 100644 >--- a/drivers/dpll/dpll_netlink.h >+++ b/drivers/dpll/dpll_netlink.h >@@ -3,5 +3,12 @@ > * Copyright (c) 2021 Meta Platforms, Inc. and affiliates > */ > >+int dpll_notify_device_create(int dpll_id, const char *name); >+int dpll_notify_device_delete(int dpll_id); >+int dpll_notify_status_locked(int dpll_id); >+int dpll_notify_status_unlocked(int dpll_id); >+int dpll_notify_source_change(int dpll_id, int source_id, int source_type); >+int dpll_notify_output_change(int dpll_id, int output_id, int output_type); Why these are not returning void? Does driver care about the return value? Why? >+ > int __init dpll_netlink_init(void); > void dpll_netlink_finish(void); >-- >2.27.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel