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 6227CC433EF for ; Fri, 17 Dec 2021 18:17:12 +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=t+LvN/Zx4TOgziSc04WmTc3bdTLin2q48A2izOJiDKc=; b=Z0rpr1pOV7Z4uZ u1bzHVWLwc+K8Yd7qmKgtCr+e0GfvVaBQPzA/G4jJsEbT4BANKbQkMeZBj8Js2qAYyjqpzafXb+0G TphdVwJ0evRJnMukLRmqpIdC5vlrVosy+D4VQA4IsgUToncrCM2jQmwSdXvNmyXGazy9lvTBsNRE4 yTnNR01Y4UFIdIePv3HWO/BTyLoL2auSu+Ze10B+PwARy+RCatkZhloBBRzuPTPVA564cuKqwZiFV VTxEYiM2xoF8efQomuJohUnjKJuhRgVzm4lveH9Xj7UEhphRE9HjgPlxFWySfXpsmtClev91BwD6t /5TWCRpgLVammysqCopQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1myHlP-00Bjqt-Gq; Fri, 17 Dec 2021 18:15:28 +0000 Received: from mail-pl1-x629.google.com ([2607:f8b0:4864:20::629]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1myGsp-00BRi2-Oy for linux-arm-kernel@lists.infradead.org; Fri, 17 Dec 2021 17:19:06 +0000 Received: by mail-pl1-x629.google.com with SMTP id z6so2407974plk.6 for ; Fri, 17 Dec 2021 09:19:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=FadQaZ8DoNqlMGpO097Z5psRmT3rJTCP60EnoHLHsg8=; b=DZNr73CTz4COQH1OqIDQ6cuoN4Xkj30JVtmBmkxWNTgfG8j/0kkFOMWf3DddvVgt6B RrHtXfee6kwF5ratN9m+xkfcQc8a3cPqoA/XiFa1s7x50KcTO4E28JtA7N78+2V5+xrH oSX0mWBO/6jYSAESDao9RXWmly5XLPtd60p6rady66W/LAK5Py7lljTZZ+xfpiL0Ysod NfjQKoi51T8WSvONf5NWQB9JXxyuxacsJRVwN4kEAU4XLomMX475wC7O5mkDyj1e4EOt c5LUyaO1NVCuxk1Xl4spW3BQyqPovSfSql3JKlUfLAUrGDvX9O1pAG5+HrC/MFM8pZ+X j8ow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=FadQaZ8DoNqlMGpO097Z5psRmT3rJTCP60EnoHLHsg8=; b=KFC2iZyDmk2A5jWeLHkgmh2gwFfF5XDtNy0okbSqwyGHAaVLKPG82/5GRENQFc4Ex3 osVdFPoAk/lOqmvuVCWqz2SHJjdopWPes7GvDjex9bwEU5n9dxwecRTT7MRjxaWzaDNP 7gHhmsaOxusLoz9TV0aa7ggYOZQUQ8oeHMA2bWS6hQdFmpzfYGzuqmecGFIyQJvd0gKy 0pJy8VeTU0W/fWgOsCthVbwovaiAWZZMWNUuB55ZnL/dkJRHKlUBbOLzXjh2d6HYIeyu BU0TAYTgnoj8gTzq5QtCvIz00Xxw6OXOxVhImntatcIxksmk1RokfS22HUmP+GmZKTky +1uw== X-Gm-Message-State: AOAM530CsmbzFsY9OrFbcjOgjcD181+EJy698rV208v8+tDmvx3Lz1+H +gTd9t2HAnXvYttqw+b85WcnAg== X-Google-Smtp-Source: ABdhPJzos+r0b36n/10iG0z0z6DKo141IZEcUnLRexoXIdDXmjCQRnGAn/ANcWc8Iq6x7wkIKQfTfQ== X-Received: by 2002:a17:902:708b:b0:148:b0b0:2ad3 with SMTP id z11-20020a170902708b00b00148b0b02ad3mr3979118plk.115.1639761542531; Fri, 17 Dec 2021 09:19:02 -0800 (PST) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id t38sm10586234pfg.218.2021.12.17.09.19.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Dec 2021 09:19:01 -0800 (PST) Date: Fri, 17 Dec 2021 10:18:59 -0700 From: Mathieu Poirier To: Leo Yan Cc: Jay Chen , suzuki.poulose@arm.com, linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org, mike.leach@linaro.org, zhangliguang@linux.alibaba.com Subject: Re: [RFC V2 PATCH] coresight: change tmc from misc device to cdev device Message-ID: <20211217171859.GB123267@p14s> References: <20211217034208.64290-1-jkchen@linux.alibaba.com> <20211217075427.GB371207@leoy-ThinkPad-X240s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20211217075427.GB371207@leoy-ThinkPad-X240s> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211217_091903_878406_DF93C8B2 X-CRM114-Status: GOOD ( 44.49 ) 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 On Fri, Dec 17, 2021 at 03:54:27PM +0800, Leo Yan wrote: > Hi Jay, > > On Fri, Dec 17, 2021 at 11:42:08AM +0800, Jay Chen wrote: > > From: root > > Please use your company's email address rather than using an invalid > email address. > > > For coresight in the server scenario, if there are too many > > funnel (about 130) and tmc (about 130) peripherals, the error > > of insufficient device numbers will occur by misc device > > Incomplete commit log ... > > For a better practice, it's deserve to write a high quality commit log > which is helpful for maintainers to understand your patch, at the end, > it also can accelerate your patch's merging. Let me quote a suggestion > for the commit log format from an old reply [1]: > > ... Please use the customary changelog style we use in the kernel: > > " Current code does (A), this has a problem when (B). > We can improve this doing (C), because (D)." > > [1] https://lkml.org/lkml/2013/11/11/146 > > > Signed-off-by: root > > > --- > > .../hwtracing/coresight/coresight-tmc-core.c | 84 +++++++++++++++---- > > drivers/hwtracing/coresight/coresight-tmc.h | 10 ++- > > 2 files changed, 75 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c > > index d0276af82494..03b114c0616a 100644 > > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c > > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c > > @@ -31,6 +31,11 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb"); > > DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf"); > > DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr"); > > > > +static dev_t tmc_major; > > +static struct class *tmc_class; > > + > > +#define TMC_DEV_MAX (MINORMASK + 1) > > + > > void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata) > > { > > struct coresight_device *csdev = drvdata->csdev; > > @@ -147,7 +152,7 @@ static int tmc_open(struct inode *inode, struct file *file) > > { > > int ret; > > struct tmc_drvdata *drvdata = container_of(file->private_data, > > - struct tmc_drvdata, miscdev); > > + struct tmc_drvdata, cdev); > > > > ret = tmc_read_prepare(drvdata); > > if (ret) > > @@ -179,7 +184,7 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len, > > char *bufp; > > ssize_t actual; > > struct tmc_drvdata *drvdata = container_of(file->private_data, > > - struct tmc_drvdata, miscdev); > > + struct tmc_drvdata, cdev); > > actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp); > > if (actual <= 0) > > return 0; > > @@ -200,7 +205,7 @@ static int tmc_release(struct inode *inode, struct file *file) > > { > > int ret; > > struct tmc_drvdata *drvdata = container_of(file->private_data, > > - struct tmc_drvdata, miscdev); > > + struct tmc_drvdata, cdev); > > > > ret = tmc_read_unprepare(drvdata); > > if (ret) > > @@ -449,8 +454,9 @@ static u32 tmc_etr_get_max_burst_size(struct device *dev) > > > > static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > > { > > - int ret = 0; > > + int ret = 0, tmc_num = 0; > > u32 devid; > > + dev_t devt; > > void __iomem *base; > > struct device *dev = &adev->dev; > > struct coresight_platform_data *pdata = NULL; > > @@ -546,14 +552,26 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id) > > goto out; > > } > > > > - drvdata->miscdev.name = desc.name; > > - drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; > > - drvdata->miscdev.fops = &tmc_fops; > > - ret = misc_register(&drvdata->miscdev); > > - if (ret) > > + tmc_num = etb_devs.nr_idx + etf_devs.nr_idx + > > + etr_devs.nr_idx; > > I am struggling to understand this line code, but looks to me this is > right. The system might have both ETR and ETF components, so we > need to calculate index for all sinks. > > > + > > + cdev_init(&drvdata->cdev.cdev, &tmc_fops); > > + drvdata->cdev.cdev.owner = THIS_MODULE; > > + devt = MKDEV(MAJOR(tmc_major), tmc_num - 1); > > + ret = cdev_add(&drvdata->cdev.cdev, devt, 1); > > + if (ret) { > > coresight_unregister(drvdata->csdev); > > - else > > + goto out; > > + } > > + > > + drvdata->cdev.dev = device_create(tmc_class, NULL, devt, &drvdata->cdev, desc.name); > > + if (IS_ERR(drvdata->cdev.dev)) { > > + ret = PTR_ERR(drvdata->cdev.dev); > > + cdev_del(&drvdata->cdev.cdev); > > + coresight_unregister(drvdata->csdev); > > + } else > > pm_runtime_put(&adev->dev); > > + > > out: > > return ret; > > } > > @@ -584,12 +602,8 @@ static void tmc_remove(struct amba_device *adev) > > { > > struct tmc_drvdata *drvdata = dev_get_drvdata(&adev->dev); > > > > - /* > > - * Since misc_open() holds a refcount on the f_ops, which is > > - * etb fops in this case, device is there until last file > > - * handler to this device is closed. > > - */ > > - misc_deregister(&drvdata->miscdev); > > + device_destroy(tmc_class, drvdata->cdev.dev->devt); > > + cdev_del(&drvdata->cdev.cdev); > > coresight_unregister(drvdata->csdev); > > } > > > > @@ -618,7 +632,43 @@ static struct amba_driver tmc_driver = { > > .id_table = tmc_ids, > > }; > > > > -module_amba_driver(tmc_driver); > > +static int __init tmc_init(void) > > +{ > > + int ret; > > + > > + ret = alloc_chrdev_region(&tmc_major, 0, TMC_DEV_MAX, "tmc"); > > + if (ret < 0) { > > + pr_err("tmc: failed to allocate char dev region\n"); > > + return ret; > > + } > > + > > + tmc_class = class_create(THIS_MODULE, "tmc"); > > + if (IS_ERR(tmc_class)) { > > + pr_err("failed to create tmc class\n"); > > + ret = PTR_ERR(tmc_class); > > + unregister_chrdev_region(tmc_major, TMC_DEV_MAX); > > + return ret; > > + } > > + > > + ret = amba_driver_register(&tmc_driver); > > + if (ret) { > > + pr_err("tmc: error registering amba driver\n"); > > + class_destroy(tmc_class); > > + unregister_chrdev_region(tmc_major, TMC_DEV_MAX); > > + } > > + > > + return ret; > > +} > > + > > +static void __exit tmc_exit(void) > > +{ > > + amba_driver_unregister(&tmc_driver); > > + class_destroy(tmc_class); > > + unregister_chrdev_region(tmc_major, TMC_DEV_MAX); > > +} > > + > > +module_init(tmc_init); > > Should invoke tmc_init() in an ealier phase so it can prepare device class > prior to the TMC devices registration? In other words, I think here > should be: > > subsys_initcall(tmc_init); TMC devices won't be registered until a TMC driver is registered, which happens after the creation of the class. Anything I am missing? > > Thanks, > Leo > > > +module_exit(tmc_exit); > > > > MODULE_AUTHOR("Pratik Patel "); > > MODULE_DESCRIPTION("Arm CoreSight Trace Memory Controller driver"); > > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h > > index 6bec20a392b3..b65ac363f9e4 100644 > > --- a/drivers/hwtracing/coresight/coresight-tmc.h > > +++ b/drivers/hwtracing/coresight/coresight-tmc.h > > @@ -7,6 +7,7 @@ > > #ifndef _CORESIGHT_TMC_H > > #define _CORESIGHT_TMC_H > > > > +#include > > #include > > #include > > #include > > @@ -163,11 +164,16 @@ struct etr_buf { > > void *private; > > }; > > > > +struct tmc_cdev { > > + struct cdev cdev; > > + struct device *dev; > > +}; > > + > > /** > > * struct tmc_drvdata - specifics associated to an TMC component > > * @base: memory mapped base address for this component. > > * @csdev: component vitals needed by the framework. > > - * @miscdev: specifics to handle "/dev/xyz.tmc" entry. > > + * @tmc_cdev: specifics to handle "/dev/xyz.tmc" entry. > > * @spinlock: only one at a time pls. > > * @pid: Process ID of the process being monitored by the session > > * that is using this component. > > @@ -191,7 +197,7 @@ struct etr_buf { > > struct tmc_drvdata { > > void __iomem *base; > > struct coresight_device *csdev; > > - struct miscdevice miscdev; > > + struct tmc_cdev cdev; > > spinlock_t spinlock; > > pid_t pid; > > bool reading; > > -- > > 2.27.0 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel