Message ID | 20230307210014.1380102-1-gnstark@sberdevices.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp2671517wrd; Tue, 7 Mar 2023 13:28:29 -0800 (PST) X-Google-Smtp-Source: AK7set87sQx1Om83ruI0cev6BF26aQgTPLxEK2ekH8+jahBFFjpP28GKC9UOupsZ1eyVW8UxpFmD X-Received: by 2002:a17:906:5e0c:b0:872:82d3:4162 with SMTP id n12-20020a1709065e0c00b0087282d34162mr14552092eju.44.1678224509209; Tue, 07 Mar 2023 13:28:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678224509; cv=none; d=google.com; s=arc-20160816; b=IpN+cZHY3CFfvFxq0qwXHU6kBDuSnMpGRT5+fvreyf2+ZyjL2PvYGS6uNCINveu4lc 4EGAxODfml9EPQCsvDjfCBPzSvej2Drvo20R2RYS0jsmv0jFjlNVheE9sLb0rlff8svZ QBJPd4I5o5CZ1Nbo3Jt/ORq8VAoWUx1kwLZ/XFZEF2hMPCtVBO65f5hYWjYFDQFZw0Kg LSYk6lzQc02lpSKrxfE9627bFEdrUEq4h96l4t5OhOVMnGod+5uYzOOkiG5Rznt3W/cE rc4+EKGNZx0JLQK53MGFtW6UAbN274et5QnH4XdcfuB20Dkhe2dYgOKm/wJ3WzC5N/ay bD3A== 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=n7uelarKUUaeZ2SnEaQ3vwbd0u3Zv00FrAdmQ5cpVvY=; b=Ij/Sc/0Ioh++Qld9/kxf4mkXRBxec9XBZ2Ql3SlKnMsOFgluMmm+eYqz2nrohbFGnA 39t869E0LE4uKBCJCmieHbaFtx48LJexvvwbzPjCRaz7CYxjEHAffZY7iHqupp0xEbt/ OePOwZm+F4z4yE/K+cQFQUK7U8AL1AqSDILTwquyNPxEW/zJSSM26fUNvLRM8kAL0JwG i0HLDcgFhaKHYc9vcdWxenrY2DfSqxglVkPDlMuN0a491aY8gf/YpvceIM/VnMdvV9u0 //LGaSoWeS2n68hLlT1xpp79NTadgZuDsKmpnt14xder34tINZ2bNif1PKCRuIwTBwFV clFg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sberdevices.ru header.s=mail header.b=i1v+rF0z; 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=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n2-20020a05640206c200b004bd92259dedsi13343843edy.657.2023.03.07.13.28.05; Tue, 07 Mar 2023 13:28:29 -0800 (PST) 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=@sberdevices.ru header.s=mail header.b=i1v+rF0z; 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=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229540AbjCGVBM (ORCPT <rfc822;toshivichauhan@gmail.com> + 99 others); Tue, 7 Mar 2023 16:01:12 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230518AbjCGVBE (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 7 Mar 2023 16:01:04 -0500 Received: from mx.sberdevices.ru (mx.sberdevices.ru [45.89.227.171]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DBC559E521; Tue, 7 Mar 2023 13:00:56 -0800 (PST) Received: from s-lin-edge02.sberdevices.ru (localhost [127.0.0.1]) by mx.sberdevices.ru (Postfix) with ESMTP id 610075FD31; Wed, 8 Mar 2023 00:00:53 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1678222853; bh=n7uelarKUUaeZ2SnEaQ3vwbd0u3Zv00FrAdmQ5cpVvY=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type; b=i1v+rF0zh/wcscwgQJcUwBai8ousVafggtpd/rfB9noAmOTM9VfTk0Hxh06bHWz9w ZoRS8vKwLXINCde2jFPiXtCdxBTbrMUpAWuIwrr3lFjy5/0pK7RiCzRAsYa5+Rvpe4 472JEhHDOpQ/81/lyMZGtw3UKcXOX9Ehynf4TuTBGDfhenbtNw3vpfIiWdzOnUsoFO 419KseiiJqsuD6u9VcdL6OFrCECbYBBJCI6xuy0TwbwSzvLiw94eZEbIGlHDZgObN7 HY0kZLf8SpVogbj+WouX20gOPZAkeAa4Af00HDhqVPFkzhfONTw0uqFXfwiBKi4l4z 0Y+m4Wbs5X2Vg== Received: from S-MS-EXCH01.sberdevices.ru (S-MS-EXCH01.sberdevices.ru [172.16.1.4]) by mx.sberdevices.ru (Postfix) with ESMTP; Wed, 8 Mar 2023 00:00:51 +0300 (MSK) From: George Stark <gnstark@sberdevices.ru> To: <thierry.reding@gmail.com>, <u.kleine-koenig@pengutronix.de>, <krzysztof.kozlowski@linaro.org>, <alim.akhtar@samsung.com> CC: <linux-pwm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-samsung-soc@vger.kernel.org>, <kernel@sberdevices.ru>, <gnstark@sberdevices.ru>, George Stark <GNStark@sberdevices.ru> Subject: [RFC PATCH v1] Revert "pwm: Clear chip_data in pwm_put()" Date: Wed, 8 Mar 2023 00:00:14 +0300 Message-ID: <20230307210014.1380102-1-gnstark@sberdevices.ru> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [172.16.1.6] X-ClientProxiedBy: S-MS-EXCH01.sberdevices.ru (172.16.1.4) To S-MS-EXCH01.sberdevices.ru (172.16.1.4) X-KSMG-Rule-ID: 4 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Status: not scanned, disabled by settings X-KSMG-AntiSpam-Interceptor-Info: not scanned X-KSMG-AntiPhishing: not scanned, disabled by settings X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 1.1.2.30, bases: 2023/03/07 18:59:00 #20923273 X-KSMG-AntiVirus-Status: Clean, skipped X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED 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: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759745943015053207?= X-GMAIL-MSGID: =?utf-8?q?1759745943015053207?= |
Series |
[RFC,v1] Revert "pwm: Clear chip_data in pwm_put()"
|
|
Commit Message
George Stark
March 7, 2023, 9 p.m. UTC
From: George Stark <GNStark@sberdevices.ru> This reverts commit e926b12c611c2095c7976e2ed31753ad6eb5ff1a. There're several issues with the original change: - it breaks generic semantics of set_driver_data-like routines that only client code controls lifetime of it's own data. - it breaks pwm-sti.c driver: pwm_set_chip_data is used only in probe stage then pwm_get_chip_data used in capture callback Change-Id: I5787c6b4c520d4a0997567c416b26fa4e0806b94 Signed-off-by: George Stark <GNStark@sberdevices.ru> --- drivers/pwm/core.c | 1 - drivers/pwm/pwm-berlin.c | 1 + drivers/pwm/pwm-samsung.c | 1 + 3 files changed, 2 insertions(+), 1 deletion(-)
Comments
Hello George, On Wed, Mar 08, 2023 at 12:00:14AM +0300, George Stark wrote: > From: George Stark <GNStark@sberdevices.ru> > > This reverts commit e926b12c611c2095c7976e2ed31753ad6eb5ff1a. > > There're several issues with the original change: > > - it breaks generic semantics of set_driver_data-like routines that > only client code controls lifetime of it's own data. > > - it breaks pwm-sti.c driver: pwm_set_chip_data is used only in probe stage > then pwm_get_chip_data used in capture callback pwm-sti is also broken in other regards. pwm_set_chip_data() is only called after pwmchip_add(). But as soon as pwmchip_add() is called, the callbacks (e.g. capture) might be called. Then the call to pwm_set_chip_data() might be too late. > Change-Id: I5787c6b4c520d4a0997567c416b26fa4e0806b94 Please don't add a Change-Id footer for Linux submissions. If you ask me, better drop pwm_set_chip_data() completely. It adds no useful value. It's just a variant of driver data and using both complicates the driver and probably fragments memory allocations. Also the sematic of driver data is better known as it's the same for all subsystems. Do you use the capture functionality? In my eyes the capture part of the pwm subsystem is very alien. Only a small subset of the hardware supports this and the counter framework should be better suited for such tasks. Best regards Uwe
Hello Uwe On 3/8/23 00:28, Uwe Kleine-König wrote: > Hello George, > > On Wed, Mar 08, 2023 at 12:00:14AM +0300, George Stark wrote: >> From: George Stark <GNStark@sberdevices.ru> >> >> This reverts commit e926b12c611c2095c7976e2ed31753ad6eb5ff1a. >> >> There're several issues with the original change: >> >> - it breaks generic semantics of set_driver_data-like routines that >> only client code controls lifetime of it's own data. >> >> - it breaks pwm-sti.c driver: pwm_set_chip_data is used only in probe stage >> then pwm_get_chip_data used in capture callback > pwm-sti is also broken in other regards. pwm_set_chip_data() is only > called after pwmchip_add(). But as soon as pwmchip_add() is called, the > callbacks (e.g. capture) might be called. Then the call to > pwm_set_chip_data() might be too late. > >> Change-Id: I5787c6b4c520d4a0997567c416b26fa4e0806b94 > Please don't add a Change-Id footer for Linux submissions. missed it somehow. Sorry about that > > If you ask me, better drop pwm_set_chip_data() completely. It adds no > useful value. It's just a variant of driver data and using both > complicates the driver and probably fragments memory allocations. Also > the sematic of driver data is better known as it's the same for all > subsystems. > > Do you use the capture functionality? In my eyes the capture part of the > pwm subsystem is very alien. Only a small subset of the hardware > supports this and the counter framework should be better suited for such > tasks. I don't use pwm-sti driver. I update meson pwm driver for new chips and when started using pwm_set_chip_data in probe I was very surprised that my data is lost after sysfs export/unexport calls. Then I found the patch and checked other drivers for similar usecases. Probably you're right about dropping pwm_set_chip_data. > Best regards > Uwe > Best regards George
Hello George, On Wed, Mar 08, 2023 at 12:16:00PM +0000, George Stark wrote: > On 3/8/23 00:28, Uwe Kleine-König wrote: > > If you ask me, better drop pwm_set_chip_data() completely. It adds no > > useful value. It's just a variant of driver data and using both > > complicates the driver and probably fragments memory allocations. Also > > the sematic of driver data is better known as it's the same for all > > subsystems. > > > > Do you use the capture functionality? In my eyes the capture part of the > > pwm subsystem is very alien. Only a small subset of the hardware > > supports this and the counter framework should be better suited for such > > tasks. > I don't use pwm-sti driver. I update meson pwm driver for new chips > and when started using pwm_set_chip_data in probe I was very surprised that > my data is lost after sysfs export/unexport calls. Then I found the > patch and > checked other drivers for similar usecases. OK. > Probably you're right about dropping pwm_set_chip_data. If you want to tackle that, you might want to take https://lore.kernel.org/all/20210504132537.62072-2-u.kleine-koenig@pengutronix.de/ into account. (Both to reuse this patch to prepare pwm-berlin for dropping pwm_set_chip_data and to be prepared that back then Thierry opposed to the idea.) Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index e01147f66e15..3bc644cc16fe 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -1036,7 +1036,6 @@ void pwm_put(struct pwm_device *pwm) if (pwm->chip->ops->free) pwm->chip->ops->free(pwm->chip, pwm); - pwm_set_chip_data(pwm, NULL); pwm->label = NULL; module_put(pwm->chip->ops->owner); diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c index e157273fd2f7..953cc2bba314 100644 --- a/drivers/pwm/pwm-berlin.c +++ b/drivers/pwm/pwm-berlin.c @@ -84,6 +84,7 @@ static void berlin_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) { struct berlin_pwm_channel *channel = pwm_get_chip_data(pwm); + pwm_set_chip_data(pwm, NULL); kfree(channel); } diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c index 9c5b4f515641..7e5dbdd6fc64 100644 --- a/drivers/pwm/pwm-samsung.c +++ b/drivers/pwm/pwm-samsung.c @@ -249,6 +249,7 @@ static int pwm_samsung_request(struct pwm_chip *chip, struct pwm_device *pwm) static void pwm_samsung_free(struct pwm_chip *chip, struct pwm_device *pwm) { kfree(pwm_get_chip_data(pwm)); + pwm_set_chip_data(pwm, NULL); } static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)