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 3255AC74A5B for ; Tue, 21 Mar 2023 13:36:03 +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=azB4gmfUmz2wzQazzeITuJAvtxZMDzoohZlRpaqT7aE=; b=r7TpE5C10Sx/MW Y9VZFgV0tsUhcU3V/JKpvJtqazGMYKFKq8dsq0u0f2MX6dqqmi6r4fvs4FcLX2hDvfKCtRoYXYuTO 1cryDVdrS/+v9/KFly1HxSJfWyT0juFLHWpM9dXNp/blukEKm7aXe2n1j1xKO7KnMInBo3jhyRNWX S18Mz9+glHxihecdDbOXgai8B8OmjjSu6DDQ0ahnN5xm7yw7kFdVTqCvw8sndIA0KdQ3//jbowgRC kCHGbf346hErXyK2dxviub9RqRFf6j+8cRQZhEt234VSI2u0TqvqMM4GBEvA+4TpaUpt58HBpK+yp HywRc5s+3sIx5+3rVAWg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pec8p-00CYJO-1a; Tue, 21 Mar 2023 13:35:07 +0000 Received: from mail-wm1-x330.google.com ([2a00:1450:4864:20::330]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pec8k-00CYG1-16 for linux-arm-kernel@lists.infradead.org; Tue, 21 Mar 2023 13:35:05 +0000 Received: by mail-wm1-x330.google.com with SMTP id l15-20020a05600c4f0f00b003ed58a9a15eso9464140wmq.5 for ; Tue, 21 Mar 2023 06:34:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20210112.gappssmtp.com; s=20210112; t=1679405697; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=GrENT+Bmj91DUGrS8eAgiWjucLYxnLyFAeYYdHsziOc=; b=BdM/UtBOYA+F5CIZlXubGB/bS37BhYp0OyWo8Fa0tepR3GFBm988Qn22OBX2ihoz+G x2ZUJKC4Xz7b1hKmzW1uMuqaD2OuszbJIsBE/Oiz+1vsJFt9a8nKXNU8kj/D/dFuwwA/ lVV69WzmLLw7F+tcT1CESaXrJ7CdnzEt8zjxgHy/KRVduuMIL3Mgd2K+fS3aLmdCayRj eQOtiYsMLmP24jgS/nKaA/pgO+e6hukVCODkKAPNvqsSIhKPKPyXx/nNplLWu5sRXW/+ 5UGmHz/1/aLZwmTNRH9yCEsX8snFQNN7wZRe3mpQAdGi37QVYiopC4++G50rT29Nolff 3I9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679405697; 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 :message-id:reply-to; bh=GrENT+Bmj91DUGrS8eAgiWjucLYxnLyFAeYYdHsziOc=; b=S4O5YV4ZZPk8qRY9+X/DPlvf0JbewRrD5o0VhfK4D6kxgoAPoJMZSxJeZF/4fBQwaz YFAD1QnWcP0H53T8LZG9fkaQX6ZH4/ORrTS+dQOTXlf4yrHv0GVWLlEOwrOfuB176q8e Y/3RgJK1J/8WWJvBp0aIY8y1SGjFf2YsCYarNQjrtD0/trtrFJupQ/JR3MbhusQRqvxl ima3jzKOMo1QntuzCHPVr66OUtwuKCY5Yi9piGE55w2LYH6bG80/pJsNoAw1i1nVED3F Mdq0/5JrGndPnVFhuTNeyhSnkvPcrIRurk6DjmryF4goE1Uu8QjfNd1+vyzwRZqDnp8d dr/A== X-Gm-Message-State: AO0yUKWi7doPvZDSaCAN7KAA8omK/jKd/8yvP25nr7Do7Kw8ZC7QNGJz zfJxXGIJL6ArIn2UaqYDL2VjBw== X-Google-Smtp-Source: AK7set/otdogiOsl2XYCUC7V1wbfsXSVyEvekWorAgLY37lUAh2OB176uEHSDWtn3rp/IPkAuyqJdQ== X-Received: by 2002:a05:600c:295:b0:3ed:492f:7f37 with SMTP id 21-20020a05600c029500b003ed492f7f37mr2313076wmk.10.1679405697267; Tue, 21 Mar 2023 06:34:57 -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 g14-20020a05600c310e00b003eddf30bab6sm7625547wmo.27.2023.03.21.06.34.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Mar 2023 06:34:56 -0700 (PDT) Date: Tue, 21 Mar 2023 14:34:55 +0100 From: Jiri Pirko To: Vadim Fedorenko Cc: Jakub Kicinski , Arkadiusz Kubalewski , Jonathan Lemon , Paolo Abeni , Vadim Fedorenko , poros@redhat.com, mschmidt@redhat.com, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, Milena Olech , Michal Michalik Subject: Re: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions Message-ID: References: <20230312022807.278528-1-vadfed@meta.com> <20230312022807.278528-3-vadfed@meta.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230312022807.278528-3-vadfed@meta.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230321_063502_610904_02768295 X-CRM114-Status: GOOD ( 14.37 ) 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, Mar 12, 2023 at 03:28:03AM CET, vadfed@meta.com wrote: [...] >+static int >+dpll_event_device_change(struct sk_buff *msg, struct dpll_device *dpll, >+ struct dpll_pin *pin, struct dpll_pin *parent, >+ enum dplla attr) >+{ >+ int ret = dpll_msg_add_dev_handle(msg, dpll); >+ struct dpll_pin_ref *ref = NULL; >+ enum dpll_pin_state state; >+ >+ if (ret) >+ return ret; >+ if (pin && nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id)) >+ return -EMSGSIZE; >+ >+ switch (attr) { >+ case DPLL_A_MODE: >+ ret = dpll_msg_add_mode(msg, dpll, NULL); >+ break; >+ case DPLL_A_SOURCE_PIN_IDX: >+ ret = dpll_msg_add_source_pin_idx(msg, dpll, NULL); >+ break; >+ case DPLL_A_LOCK_STATUS: >+ ret = dpll_msg_add_lock_status(msg, dpll, NULL); On top of what I wrote about the notifications, I found another two issues: 1) You don't take any lock calling this from drivers. You need to hold the xarray locks you have now. I have to repear, I think that we definitelly need to convert the overall locking scheme to have this per-instance, in a similar way we did that for devlink. I noted this in another email, but wanted to say that again. 2) You have possible race condition: 1) -> driver gets a state change event 2) -> driver calls into this function 3) -> this code does call the driver op to get the state, driver queries the state again Between 1) and 3) state can easily change, multiple times. That might lead to oddities observed by the user (like getting a notification of change with the original values) I see only 1 solutions to this: Pass the value of changed item from the driver here and just pass it on over netlink without doing calling into driver again. >+ break; >+ case DPLL_A_TEMP: >+ ret = dpll_msg_add_temp(msg, dpll, NULL); >+ break; >+ case DPLL_A_PIN_FREQUENCY: >+ ret = dpll_msg_add_pin_freq(msg, pin, NULL, false); >+ break; >+ case DPLL_A_PIN_PRIO: >+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll); >+ if (!ref) >+ return -EFAULT; >+ ret = dpll_msg_add_pin_prio(msg, pin, ref, NULL); >+ break; >+ case DPLL_A_PIN_STATE: >+ if (parent) { >+ ref = dpll_xa_ref_pin_find(&pin->parent_refs, parent); >+ if (!ref) >+ return -EFAULT; >+ if (!ref->ops || !ref->ops->state_on_pin_get) >+ return -EOPNOTSUPP; >+ ret = ref->ops->state_on_pin_get(pin, parent, &state, >+ NULL); >+ if (ret) >+ return ret; >+ if (nla_put_u32(msg, DPLL_A_PIN_PARENT_IDX, >+ parent->dev_driver_id)) >+ return -EMSGSIZE; >+ } else { >+ ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll); >+ if (!ref) >+ return -EFAULT; >+ ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, >+ NULL); >+ if (ret) >+ return ret; >+ } >+ break; >+ default: >+ break; >+ } >+ >+ return ret; >+} >+ >+static int >+dpll_send_event_create(enum dpll_event event, struct dpll_device *dpll) >+{ >+ struct sk_buff *msg; >+ int ret = -EMSGSIZE; >+ void *hdr; >+ >+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); >+ if (!msg) >+ return -ENOMEM; >+ >+ hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, event); >+ if (!hdr) >+ goto out_free_msg; >+ >+ ret = dpll_msg_add_dev_handle(msg, dpll); >+ if (ret) >+ goto out_cancel_msg; >+ genlmsg_end(msg, hdr); >+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL); >+ >+ return 0; >+ >+out_cancel_msg: >+ genlmsg_cancel(msg, hdr); >+out_free_msg: >+ nlmsg_free(msg); >+ >+ return ret; >+} >+ >+static int >+dpll_send_event_change(struct dpll_device *dpll, struct dpll_pin *pin, >+ struct dpll_pin *parent, enum dplla attr) >+{ >+ struct sk_buff *msg; >+ int ret = -EMSGSIZE; >+ void *hdr; >+ >+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); >+ if (!msg) >+ return -ENOMEM; >+ >+ hdr = genlmsg_put(msg, 0, 0, &dpll_nl_family, 0, >+ DPLL_EVENT_DEVICE_CHANGE); >+ if (!hdr) >+ goto out_free_msg; >+ >+ ret = dpll_event_device_change(msg, dpll, pin, parent, attr); >+ if (ret) >+ goto out_cancel_msg; >+ genlmsg_end(msg, hdr); >+ genlmsg_multicast(&dpll_nl_family, msg, 0, 0, GFP_KERNEL); >+ >+ return 0; >+ >+out_cancel_msg: >+ genlmsg_cancel(msg, hdr); >+out_free_msg: >+ nlmsg_free(msg); >+ >+ return ret; >+} >+ >+int dpll_notify_device_create(struct dpll_device *dpll) >+{ >+ return dpll_send_event_create(DPLL_EVENT_DEVICE_CREATE, dpll); >+} >+ >+int dpll_notify_device_delete(struct dpll_device *dpll) Please change the function names to "register/unregister" to be consistent with the rest of the code. >+{ >+ return dpll_send_event_create(DPLL_EVENT_DEVICE_DELETE, dpll); >+} >+ >+int dpll_device_notify(struct dpll_device *dpll, enum dplla attr) >+{ >+ if (WARN_ON(!dpll)) >+ return -EINVAL; >+ >+ return dpll_send_event_change(dpll, NULL, NULL, attr); >+} >+EXPORT_SYMBOL_GPL(dpll_device_notify); >+ >+int dpll_pin_notify(struct dpll_device *dpll, struct dpll_pin *pin, >+ enum dplla attr) The driver should be aware of netlink attributes. Should be abstracted out. just have per-item notification like: dpll_pin_state_notify() dpll_pin_prio_notify() ... Then you can easily pass changed value that would allow solution to the issue 2) I described above. >+{ >+ return dpll_send_event_change(dpll, pin, NULL, attr); >+} >+ [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel