From patchwork Thu Feb 4 17:30:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Shishkin X-Patchwork-Id: 8226641 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C10249F37A for ; Thu, 4 Feb 2016 17:51:49 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D645A20389 for ; Thu, 4 Feb 2016 17:51:48 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C9C492039D for ; Thu, 4 Feb 2016 17:51:47 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aRO39-0003Gr-Qc; Thu, 04 Feb 2016 17:50:35 +0000 Received: from mga01.intel.com ([192.55.52.88]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aRNkN-0000UB-1t for linux-arm-kernel@lists.infradead.org; Thu, 04 Feb 2016 17:31:19 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 04 Feb 2016 09:30:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,396,1449561600"; d="scan'208";a="905644616" Received: from um.fi.intel.com (HELO localhost) ([10.237.72.212]) by orsmga002.jf.intel.com with ESMTP; 04 Feb 2016 09:30:44 -0800 From: Alexander Shishkin To: Chunyan Zhang , mathieu.poirier@linaro.org Subject: Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name In-Reply-To: <1454576179-27224-1-git-send-email-zhang.chunyan@linaro.org> References: <1454487337-30184-3-git-send-email-zhang.chunyan@linaro.org> <1454576179-27224-1-git-send-email-zhang.chunyan@linaro.org> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Thu, 04 Feb 2016 19:30:43 +0200 Message-ID: <871t8se62k.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160204_093111_647408_B8D17542 X-CRM114-Status: GOOD ( 22.77 ) X-Spam-Score: -7.4 (-------) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, al.grant@arm.com, corbet@lwn.net, zhang.lyra@gmail.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, tor@ti.com, broonie@kernel.org, mike.leach@arm.com, linux-api@vger.kernel.org, pratikp@codeaurora.org, nicolas.guion@st.com, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Chunyan Zhang writes: I few comments below: > The node name of STM master management policy is a concatenation of an > STM device name to which this policy applies and following an arbitrary > string, these two strings are concatenated with a dot. This is true. > This patch adds a loop for extracting the STM device name when an > arbitrary number of dot(s) are found in this STM device name. It's not very easy to tell what's going on here from this description. The reader be left curious as to why an arbitrary number of dots is a reason to run a loop. When in doubt, try to imagine as if you're seeing this patch for the first time and ask yourself, does the message give a clear explanation of what's going on in it. > Signed-off-by: Chunyan Zhang > --- > drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c > index 11ab6d0..691686e 100644 > --- a/drivers/hwtracing/stm/policy.c > +++ b/drivers/hwtracing/stm/policy.c > @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name) > /* > * node must look like ., where > * is the name of an existing stm device and > - * is an arbitrary string > + * is an arbitrary string, when an arbitrary > + * number of dot(s) are found in the , the > + * first matched STM device name would be extracted. > */ This leaves room for a number of suspicious situations. What if both "xyz" and "xyz.0" are stm devices, how would you create a policy for the latter, for example? The rules should be such that you can tell exactly what the intended stm device is from a policy directory name, otherwise it's just asking for trouble. > - p = strchr(devname, '.'); > - if (!p) { > - kfree(devname); > - return ERR_PTR(-EINVAL); > - } > + for (p = devname; ; p++) { > + p = strchr(p, '.'); > + if (!p) { > + kfree(devname); > + return ERR_PTR(-EINVAL); > + } > > - *p++ = '\0'; > + *p = '\0'; > > - stm = stm_find_device(devname); > - kfree(devname); > + stm = stm_find_device(devname); > + if (stm) > + break; > + *p = '.'; > + } > > - if (!stm) > - return ERR_PTR(-ENODEV); > + kfree(devname); In the existing code there is a clear distinction between -ENODEV, which is to say "we didn't find the device" and -EINVAL, "directory name breaks rules/is badly formatted". After the change, it's all -EINVAL, which also becomes "we tried everything, sorry". So, having said all that, does the following patch solve your problem: From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Thu, 4 Feb 2016 18:56:34 +0200 Subject: [PATCH] stm class: Support devices with multiple instances By convention, the name of the stm policy directory in configfs consists of the device name to which it applies and the actual policy name, separated by a dot. Now, some devices already have dots in their names that separate name of the actual device from its instance identifier. Such devices will result in two (or more, who can tell) dots in the policy directory name. Existing policy code, however, will treat the first dot as the one that separates device name from policy name, therefore failing the above case. This patch makes the last dot in the directory name be the separator, thus prohibiting dots from being used in policy names. Suggested-by: Chunyan Zhang Signed-off-by: Alexander Shishkin --- drivers/hwtracing/stm/policy.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c index 94d3abfb73..1db189657b 100644 --- a/drivers/hwtracing/stm/policy.c +++ b/drivers/hwtracing/stm/policy.c @@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name) /* * node must look like ., where - * is the name of an existing stm device and - * is an arbitrary string + * is the name of an existing stm device; may + * contain dots; + * is an arbitrary string; may not contain dots */ - p = strchr(devname, '.'); + p = strrchr(devname, '.'); if (!p) { kfree(devname); return ERR_PTR(-EINVAL);