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 CC5EDC7618E for ; Wed, 26 Apr 2023 13:29: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:MIME-Version:Content-ID:In-Reply-To: References:Message-ID:Date:Subject:CC:To:From:Reply-To:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3Fj+NMPbp0qLilfZHs6mEI/PUMh4TNUbzfJ6p0iYx/E=; b=pTHv7Z0oaPzttE KTm6T8iBB13N3QS7LTV1wIQ9aFwYfL3EXRQgAnDeJy3zIG/Lsr6kIbpyYOjXoRmzzlRK38FcfR6Av +4v1pKU5Xv3w70vj+uhhCdE6NuMH+oTj/GCfZ149VeOBuZfJAOkwicH2szmZoU8iG4OSqIPw5q+6d UCbDmG9m6KmZ8TqqZIxtS+tsJkLrCBsyHkBwZH5dxs1KzW+DxOuOWr6Hu+sGrKqE+1HSzBFZZJDRp sODpLInXcFH9DScQ+cx6sLtE+QEscsRgHu6iNFT9MF5IgT0jafovlyNkDyeneCzPAVOlqxkakOXXb mWpvEWgut447t6HQyJyw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1prfCF-0045Ly-2W; Wed, 26 Apr 2023 13:28:35 +0000 Received: from mx0b-0039f301.pphosted.com ([148.163.137.242]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1prfCA-0045Kk-2i for linux-arm-kernel@lists.infradead.org; Wed, 26 Apr 2023 13:28:33 +0000 Received: from pps.filterd (m0174683.ppops.net [127.0.0.1]) by mx0b-0039f301.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 33QBaAGX021507; Wed, 26 Apr 2023 13:28:26 GMT Received: from eur05-vi1-obe.outbound.protection.outlook.com (mail-vi1eur05lp2175.outbound.protection.outlook.com [104.47.17.175]) by mx0b-0039f301.pphosted.com (PPS) with ESMTPS id 3q6gecv0gt-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 26 Apr 2023 13:28:26 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bHC0e4uRJJROsHCt1B/ccdDVPfkpL0Y+PTYv35YxNsANydGaOgIkyP4HXn04rftKjj9rLskf4KYGe7Ga1Q+AOFG5BDgIxa7e4u1Mk7PrPnNz0Ov7ctW+anDY1t+bZ//uKCqMX3i1T55VkcKEVIxloSNPqpC0NFvlfV8ZCKxSneAGFY0fNCnPUi6RMlA9O710dLdN62yCQ34OQ+4r3jbFKUaKeFoLHnXpy2SvH8AMRVP1eye1G7gUo6cZajgkxHGiMMSAgX4NHRH3/GRqlvNWcXxaAk983jU1WoYGjAsS9CI7xysOybA7BtnwXXgOcthbtg3pRiTqu8B5tdV2M7Ox1A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=B19jKXSappv5pdOuVBC50DP8tgr0TsB9TmQFq8kVaRQ=; b=C9Kh29QeSLoE0GlWXCBfWaKwxTcxiOf2DdQIHTo7InMxytQUl+KDAPm7Rsw5fpaoPmixCFfpwB+C4ehe6rGcCNhycnZZSbvs9LySk9HMps2YpM25q6Stp0MLa1larie3jKq0+uFjLuZ8KBb/WI9KRvtD+bwopOxzK4v/yJM697W/TMqFwz2nao1leIY646dXvgIbDk5ZQxtpjOskCc1/qqaPBo57jqbuodd46ZYv691s4tBP5Q7zb4kBedADA8rBT55pQRk99Ywc1AaZPoYZ+qJe9+4+BiPKKzY0jET6fu4kZ5c35pA4/TEQO2HjR47xU2ZdQ/+hiMfnHI6dRfSt0A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=epam.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=B19jKXSappv5pdOuVBC50DP8tgr0TsB9TmQFq8kVaRQ=; b=kTP4Y5v/zeDZqYk/Ix704BcCYqqmFad+1QNqV9meU20SblI68yskUTt/ykN708zySHX+fQEqFzzVSB/z5ELXI4VYa8AZVNtEeOLMBC9yPNAvkqeZAiVtICQmlsy03mWHMm6XxfAfKKuTtPDwQZR4vlBGYw8EeT00+PbQv/4N53+QsafSoDeITCChUlTncte2iuze1lVrf0MnUPJQuDC44YBOmJT09lyqn7P606jbYTyldMuFty+YywNPi1blm3Txuri0KRp6FTAra4bLizsJzMvLf+9ny0SQXu2Pkysu2X2WxdeX1td6JW9Ab1qaVAGdGHMQ20FdIkar7nu4ZYEYjw== Received: from PA4PR03MB7136.eurprd03.prod.outlook.com (2603:10a6:102:ea::23) by GV2PR03MB9380.eurprd03.prod.outlook.com (2603:10a6:150:d2::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6298.54; Wed, 26 Apr 2023 13:28:22 +0000 Received: from PA4PR03MB7136.eurprd03.prod.outlook.com ([fe80::bcf5:cd14:fd35:1300]) by PA4PR03MB7136.eurprd03.prod.outlook.com ([fe80::bcf5:cd14:fd35:1300%8]) with mapi id 15.20.6340.021; Wed, 26 Apr 2023 13:28:22 +0000 From: Oleksii Moisieiev To: Cristian Marussi CC: "sudeep.holla@arm.com" , Linus Walleij , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-gpio@vger.kernel.org" , "michal.simek@amd.com" , "vincent.guittot@linaro.org" , "souvik.chakravarty@arm.com" Subject: Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver Thread-Topic: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver Thread-Index: AQHZaTpLwL07BD9460y7/F64OnMs/q8oQvCAgBRJR4CAAQNkgIAAI98A Date: Wed, 26 Apr 2023 13:28:22 +0000 Message-ID: <850d286c-3f30-0209-7bf2-e8fcf400f8ae@epam.com> References: <54119b2cb43e29f69c5858a5320d3a58f23fed21.1680793130.git.oleksii_moisieiev@epam.com> <71f48fcf-db04-b09f-2ab2-95e6562c8359@epam.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: PA4PR03MB7136:EE_|GV2PR03MB9380:EE_ x-ms-office365-filtering-correlation-id: ba46e7e5-ab0c-4db0-abe3-08db465a1b73 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 7BdDR79yY/zdE6g73D3YAGlzP1ykaMQ+lq/KHeGIl61fuFCeuikEOiqqns7CyR3ZTA0QAg9LOebngDbJi2fC9vuVHQ0Ttr9Ci8mdq0n+wOw6OchRexEAth8za8/c/r6DBA4FzUYNmc67lQYKzMNQxmAw1caSr4TofFX6VOyN6XVB9IID3JD6XR/eXHa2JoJiX0b5Jjtx7tkDlVtcYwV6Eh7RTycJD8Y1b7OwYpcjhWsqzDsVF8xdUtGYpDP2+ZCSkEQlrYnifKjervQr3JWzu5Ln3S9mwS9oJc8R0rcILstMv8hqoWRKDwq9DTkcS+9inKt52b0+7MzqB/WoXx4qDqilZGz3NyD7SuuK+FIi3z922d297VO7UTEZkeFeUGmoRL8olN5xNlB13QMgBmtCuhk8+T4JSAs9oGX2WzcGhGoffqjC6FUmSMd9SMR1FSSXvHae71YGSfd610MVWxgFFLzs30T3tyt7/7V+6Rxoxtml4+1FhYstlxdWFxc29BfgALxeXjIgrWIhVqQzjzRydrlmKw9637btsKsMhthRjeL5F6HeQrEP+bQtP2WECFiAJ+Xzqj05ZCLafj0XvLUqgaA/YQyEKjwthAIPwKiN8cMATkBENQc3zwDMA+vs/xm6a6kYbZA+ClN0JjF4/cPeqA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PA4PR03MB7136.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(4636009)(346002)(396003)(376002)(136003)(366004)(39860400002)(451199021)(8936002)(31696002)(2616005)(122000001)(8676002)(478600001)(36756003)(91956017)(966005)(316002)(76116006)(66946007)(71200400001)(66476007)(66446008)(41300700001)(6916009)(4326008)(66556008)(64756008)(6486002)(83380400001)(86362001)(54906003)(6506007)(26005)(31686004)(6512007)(186003)(30864003)(2906002)(38070700005)(5660300002)(53546011)(38100700002)(45980500001);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?utf-8?B?akVJc3B3OC9kS3RiRHRrWWZVcUJkZlBrN3hFNG5UV2k1SHRFT1pTTlEvS0Iv?= =?utf-8?B?dkFMRFBzb1hadFQyZ05KdS9jUEphS0FIVEw0bDJqZDFBWjFTTy84bmNKTDYw?= =?utf-8?B?RFFnM2d3RXJacmdyNWhyY2NwTUJQRzVYeTU4eU1hdnIvVkQxZ1ZHZUMzRnpa?= =?utf-8?B?UVFmY25lY211ZGJ3dFpCVkxVU0hHcFFuOWxtVnpGdmxCMDRIZmg2OGVGTldh?= =?utf-8?B?NnB2Ni9YOE91SlJHNmJjQW82aUxheC9ORnM5Qk12Y2tJQWhWY2V5ekRSTmxN?= =?utf-8?B?OU8vbFlKWUZLaHZQcjVxYy8xdWVLOGFOZmY4THA1RTMyTWNsejhPdGE5eXFZ?= =?utf-8?B?Z2I4QVRDemszYzFKanpCWUdoSklJd3JhNDVjRTkycGtNYk1xeDZpOU01bWR6?= =?utf-8?B?L0NCbFFMeWh3L3J3R0QxcmVQMlF2cm5NdE82RlZ6Y0tETTBBYlFpSEFSY1JL?= =?utf-8?B?K3lqbU9sMkNORitmWGlZOTkvcnI0T2tobzI5ZWFDN0dZOTAraUNybENnNC9y?= =?utf-8?B?NzVvRnJKRDY4MGIxUnZ1ZlhXUElocFBBaU1rekdxTnlWbTJLeHlodFNNbWM0?= =?utf-8?B?Y1NzdDFPbUwyWDRWMmNMb1paeElhNU14YVZLbkh6RlhDcThIQytXSlBxNG1p?= =?utf-8?B?VHRzZnRvaVJXV3Ftd1lEbjFWZllYU0M3VlRSK002OURUQzZzYUpDNHd6Y2RX?= =?utf-8?B?ck8rVHd5U2prSkpOQlhXbmRhckxuWWpCRElKK0Rlb0JTd29uay85eU1JMEgz?= =?utf-8?B?K29rRXRXd1hLam4reGVSYytBWGxyZ0pCamhyaFM1cVczMExreVZtTE9GMDJ3?= =?utf-8?B?ckZ3cWNvUVN0dzEzcUJCVWkxUjJYT2x6OTV3Sy9qUkFPMmY4am5yU1BJb3FZ?= =?utf-8?B?ajhaZmJuc1Y2Z2JMSFlMN0pVMmdzQXpyMU1wRUxuMm9UOGxOQ2FDVllzNUdo?= =?utf-8?B?S01Pd09TU0Z1NnlTcjA5VG5RQjNFY1BpV2Z4N3NLMmkzUllYOXMyTDR1RzZB?= =?utf-8?B?S2xobXJveGRXZXZDT2lzUThqUDFYTlBPQStzaUcyRWFWL01EbkpZMTZWdkMy?= =?utf-8?B?NDNxSHpuYkQvcTgzZ2VERk0yWnBSUnkvZGNIMmdwekwzT2dZaHRiWVRFOWV4?= =?utf-8?B?VVRQNGUwb2xxYlM5MlpEY1pxQ0pMNWpzVk16OVpnVFV2ZXUyNGYrdlUyWEc1?= =?utf-8?B?Vk5pZFVaU0Z1eVNVb0RPc3RWQmpJdkVXWU9hS292QVZnbFpldlQ3SDFLd0tE?= =?utf-8?B?V1loSDVEazNCd0Y5ZUwwWHFTNThEcUhLOE81TmxWdjYvTFIreVZIUkNtU1lo?= =?utf-8?B?MFQrTkVWUlVDYWxNNnZVUHR5WFZtT3ZWU2lyYnpkVU4xdmxSbjUzOTFDUzlu?= =?utf-8?B?TVhXQnVicHBRVkVxTzdEbjNLRmtmSHhwa1pITWhsRldEa0RvL0ZmT1RqZCtC?= =?utf-8?B?NUQ3RTJHVWRaKzhjbDgrSU4vSXA4bnhWMTVLTmpMVHdrNU1xeWlkVDY3ZWwr?= =?utf-8?B?L1FZK1htZHRYZW01dEFCYTlzejduRHJ1Y1VkcU5xeGlmYTBKQm13Zkh5SHox?= =?utf-8?B?N0w3aUxHQkN1ODFsQjZGelBZWGs5MU93ZjNFY1V5WW9yaE9vYTVwejl2UFlV?= =?utf-8?B?bHN5MlBFNjlYV3BLTGpJV0tyQVlqMDhsTTR5WXNFdHdxRHVYZjAyZU1aZXIw?= =?utf-8?B?dDBOZTZ5Nzc2b1BLUENMVjZWby8zMnAzUEduMjEvVFozQkNnQVNiT1kwTkxB?= =?utf-8?B?djhhRW5SNHpnNHZRQVZzWkVWZFpsQm12ZVc5aWg5ejNlUmZSVVpLa3p3dFp1?= =?utf-8?B?a2R0WDRsWDRHVEZ2SnBKRTVpdS9VMlJ5MS9BNkhZckZuWWtmMVpLZTdoazcx?= =?utf-8?B?Y29IeUlzV2FLS21wcy9Gb0U4N0QwblJpN2xtSlJUa3ZLNVpNK0lCR3RVT3lt?= =?utf-8?B?b1IxRnJVTUpwUzdLZ1VVQVNXTlRiQlAzQ2RHMGZPclpnaHlRZmN0ZUtBaXBk?= =?utf-8?B?d2FaMDZkRld2VXJEWU9pVUZtQlVCb1pXODFTT00rNUZKc3JoT1dDamRKa3R4?= =?utf-8?B?RUQ1cVdHZ1lITGwvdEVqdkZrQS9YWHNxakhBOHNiZGxsc1NSUlFhMElwMkIx?= =?utf-8?B?b0pvT2dpQkZyOU1jTm52VVo2KzRjeDlwR1BxdlJ2NmIrdENtVjdJTzBMeVJM?= =?utf-8?B?anc9PQ==?= Content-ID: MIME-Version: 1.0 X-OriginatorOrg: epam.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: PA4PR03MB7136.eurprd03.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ba46e7e5-ab0c-4db0-abe3-08db465a1b73 X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Apr 2023 13:28:22.4215 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b41b72d0-4e9f-4c26-8a69-f949f367c91d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: cBYeZaa30p/U62aZaYQWKLySfXFfnh5ZSEbxMlp0fmAwE7vODyb7y+k2XcFrH997x/sMizenJL04TSJ6lXZj7cYxxM/HADNqm/0Hj4UiVkI= X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV2PR03MB9380 X-Proofpoint-GUID: 2K2iJ2TPuWa4kDOMXL4xHreEvYCgmFK9 X-Proofpoint-ORIG-GUID: 2K2iJ2TPuWa4kDOMXL4xHreEvYCgmFK9 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-04-26_06,2023-04-26_03,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 bulkscore=0 clxscore=1015 suspectscore=0 mlxlogscore=999 lowpriorityscore=0 adultscore=0 priorityscore=1501 spamscore=0 impostorscore=0 mlxscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2304260120 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230426_062831_002528_3898549D X-CRM114-Status: GOOD ( 27.86 ) 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 Hi Cristian, On 26.04.23 14:19, Cristian Marussi wrote: > On Tue, Apr 25, 2023 at 07:51:34PM +0000, Oleksii Moisieiev wrote: >> Hi Cristian, >> >> On 13.04.23 01:04, Cristian Marussi wrote: >> >> On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote: >> >> Implementation of the SCMI client driver, which implements >> PINCTRL_PROTOCOL. This protocol has ID 19 and is described >> in the latest DEN0056 document. >> >> Hi, >> > > Hi Oleksii, > > please do NOT post mail with html on the mailing list, it is very hard to > read and comment while useing text-only mail-reader (and not so much > appreciated :D) ; even readers with an UI can be setup to properly avoid > html stuff and properly format for Kernel upstream work: > > https://urldefense.com/v3/__https://www.kernel.org/doc/html/v4.10/process/email-clients.html__;!!GF_29dbcQIUBPA!w2wEJQrXQbVSfn4q6_Sot9NQhyl_GTC4UNjmHUcubxpIHDt5dPU6emDEkfOvtSSvjjzZR2J-VQDTOovNXDto8RP_FSJatQ$ [kernel[.]org] > > Indeed your original message now cannot even be found on lore, probably > discarded for the same reason ? (not sure). > > My comments inline. > I'm sorry for that. Somehow Thunderbird managed to switch to HTML after update. I've sent v2. >> >> This protocol is part of the feature that was designed to >> separate the pinctrl subsystem from the SCP firmware. >> The idea is to separate communication of the pin control >> subsystem with the hardware to SCP firmware >> (or a similar system, such as ATF), which provides an interface >> to give the OS ability to control the hardware through SCMI protocol. >> This is a generic driver that implements SCMI protocol, >> independent of the platform type. >> >> DEN0056 document: >> [1]https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/l >> atest__;!!GF_29dbcQIUBPA!wnEVmBR_V-0llnzQFsQfeZq5Ov7xJ87H364gqo1_UvsilzKNfJoy81u >> 5GR1f0EBIyXOGyesjURGdxT_U5tzLvqT5lgjNpw$ [developer[.]arm[.]com] >> >> >> No need to specify all of this in the commit message, just a note that >> you are adding a new SCMIv3.2 Pincontrol protocol, highlighting anything >> that has been left out in this patch (if any) will be enough. >> You can look at the very first commit logs of existing protos as an >> example like: drivers/firmware/arm_scmi/powercap.c >> >> Some more comments down below, I'll mostly skip anything related to the >> SCMI API change I mentioned before... >> >> I'll also wont comment on more trivial stuff related to style, BUT there >> are lots of them: you should run >> >> ./scripts/checkpatch.pl --strict >> >> for each patch in the series. (and fix accordingly..spacing, brackets...etc) >> >> Done. >> >> +static int scmi_pinctrl_list_associations(const struct scmi_handle *handle, > > [snip] > >> + u32 selector, >> + enum scmi_pinctrl_selector_type type, >> + uint16_t size, unsigned int *array) >> +{ >> >> This is the other functionalities you could implement straight away using >> ph->hops helpers (iterators) but just leave it this way, and I'll port it later >> (once we retested all of this as working with the new API but without any >> ph->hops usage..I think it is safer to change one bit at time... :P) >> >> Rewritten using iterators and checked with the unittests. > > Good, thanks. > > [snip] > >> +static int scmi_pinctrl_get_group_info(const struct scmi_handle *handle, >> + u32 selector, >> + struct scmi_group_info *group) >> +{ >> + int ret = 0; >> + struct scmi_pinctrl_info *pi; >> + >> + if (!handle || !handle->pinctrl_priv || !group) >> + return -EINVAL; >> + >> + pi = handle->pinctrl_priv; >> + >> + ret = scmi_pinctrl_attributes(handle, GROUP_TYPE, selector, >> + &group->name, >> + &group->nr_pins); >> + if (ret) >> + return ret; >> + >> + if (!group->nr_pins) { >> + dev_err(handle->dev, "Group %d has 0 elements", selector); >> + return -ENODATA; >> + } >> + >> + group->group_pins = devm_kmalloc_array(handle->dev, group->nr_pins, >> + sizeof(*group->group_pins), >> + GFP_KERNEL); >> >> I think you can just use for the array allocation >> >> devm_kcalloc(dev, n, size, flags) >> >> and it will add also __GFP_ZERO internally to clear it. >> (indeed it calls in turn devm_kmalloc_array) >> >> ...BUT I think there is a further tricky issue here related to memory allocation >> ... >> >> You call this and others function of this kind from some scmi_pinctrl_ops, >> like in scmi_pinctrl_get_group_pins (scmi_pinctrl_ops->get_group_pins), >> and then this is in turn called by the SCMI Pinctrl driver via >> pinctrl_ops->get_group_pins AND you set a present flag so that you issue a >> PINCTRL_LIST_ASSOCIATIONS and allocate here a new group_pins array just >> the first time: but these are never released anywhere, since, even though >> lazily dynamically allocated when asked for, these are static data that >> you pass to the caller/user of this protocol and so you cannot release >> them anytime soon, indeed. >> >> The core SCMI stack usually takes care to track and release all the devm_ >> resources allocated by the protocol ONLY if they were allocated with devres >> while inside scmi_pinctrl_protocol_init() function. >> (see drivers/firmware/arm-scmi/driver.c:scmi_alloc_init_protocol_instance() >> and scmi_protocol_release) >> >> BUT you do not allocate these arrays inside the protocol-init function, >> you allocate them the first time these ops are called at runtime. >> >> If you unbind/unload all the drivers using this protocol and then reload >> them, all the devm_ allocations in protocol_init will be freed and >> reallocated BUT these arrays will never be freed (they are boudn to handle->dev) >> and instead they will be reallocated multiple times (present flag will be cleare >> d >> on unload), remained unused and freed finally only when the whole SCMI stack is >> unbind/unloaded. >> >> You use a present flag to avoid reissuing the same query and >> reallocating all the arrays each time a driver calls these >> protocol_ops one, but really all these data is available early on at >> protocol init time and they are not supposed to change at runtime, dont they ? >> >> Even in a virtualized environment, you boot an agent and the SCMI >> platform server provides to the agent the list of associations when >> queried but then this does not change until the next reboot right ? >> (indeed you do not query more than once...) >> >> The agent can only change the PIN status with CONFIG_SET or >> FUNCTION_SELECT or REQUEST the exclusive use of a pin/group, but it is >> not that the platform can change the pin/groups associaion for the same >> agent at run time, this are static data for the whole life of the agent. >> >> Am I right ? >> >> IOW I think there is some potential memory leak on unbind/bind and it would >> be better to query and allocate all of these resources at init time and keep >> them ready to be retrieved by subsequent operations, since the lifetime >> of these resources is pretty long and they are basically representing >> static data that does not change after the init/probe phases. >> >> Indeed, all the other protocols usually allocate all the needed >> resources and query all the available SCMI resources once for all during >> the protocol_init, storing all the retrieved info in some struct *_info >> exposed in scmi_protocol.h and then provide some related protocol_ops to >> get the number of resources and to retrieve specific domain info descriptors. >> (voltage.c is an example and more on this down below...) >> >> This way, any dynamic allocation is done during protocol_init, so >> it can be automatically freed by the SCMI core once there are no more >> users of that protocol, and all of this static info data is queried >> and retrieved once for all at protocol initialization time, avoiding >> unneeded message exchanges to retrieve always the same data. >> (which you avoid anyway with the present flag) >> >> If you have a good reason to instead perform this sort of lazy >> allocation/query performed only at the last minute when someone ask for >> that specific resource, you will have to provide also a .instance_deinit >> function to clean anything you allocated out of the .instance_init >> routine; but this would seem strange to me since any resource that is >> discovered at init will be eventually immediately queried by a driver >> which uses this protocol...am I missing something ? >> >> >> This is a good point. But there is some reason why I've made such lazy >> allocations: >> >> I agree that we have all data on the early stage, but we probably do >> not want to request all associations. >> >> Let's assume we have partial pinctrl configuration with 2-3 groups, 2 >> functions and 10-15 pins involved. >> >> We don't want to request all 250 groups and 32 functions info during >> init because this will impact boottime and memory consumption. > > Yes I supposed this was the reason, and it could be reasonable to just > query the associations last minute when you need them for the stuff you > need which was put in the DT...I want to think a bit more about this, > being the only protocol that needs this behaviour. Good for now. > >> >> Pinctrl subsystem will request all needed data during device-tree node >> parsing. >> >> I have an idea to implement .instance_deinit callback from >> scmi_protocol, which will cleanup all allocated data. >> >> What do you think about that? If it is ok for you - I'll push v2. >> > > I'd say do the cleanup with the available .instance_deinit as proposed > in the meantime, so we can see how all the changes and the update to > mainline kernel pans out...then we can discuss this further down the > line, maybe finding a better way to serve you lazy allocation from the > SCMI core (or not) and also see what Sudeep thinks abou these lazy > allocations. > > [snip] > >> + * struct scmi_pinctrl_ops - represents the various operations provided >> + * by SCMI Pinctrl Protocol >> + * >> + * @get_groups_count: returns count of the registered groups >> + * @get_group_name: returns group name by index >> + * @get_group_pins: returns the set of pins, assigned to the specified group >> + * @get_functions_count: returns count of the registered fucntions >> + * @get_function_name: returns function name by indes >> + * @get_function_groups: returns the set of groups, assigned to the specified >> + * function >> + * @set_mux: set muxing function for groups of pins >> + * @get_pins: returns the set of pins, registered in driver >> + * @get_config: returns configuration parameter for pin >> + * @set_config: sets the configuration parameter for pin >> + * @get_config_group: returns the configuration parameter for a group of pins >> + * @set_config_group: sets the configuration parameter for a groups of pins >> + * @request_pin: aquire pin before selecting mux setting >> + * @free_pin: frees pin, acquired by request_pin call >> + */ >> +struct scmi_pinctrl_ops { >> + int (*get_groups_count)(const struct scmi_handle *handle); >> + int (*get_group_name)(const struct scmi_handle *handles, u32 selector, >> + const char **name); >> + int (*get_group_pins)(const struct scmi_handle *handle, u32 selector, >> + const unsigned int **pins, unsigned int *nr_pins); >> + int (*get_functions_count)(const struct scmi_handle *handle); >> + int (*get_function_name)(const struct scmi_handle *handle, u32 selector, >> + const char **name); >> + int (*get_function_groups)(const struct scmi_handle *handle, >> + u32 selector, unsigned int *nr_groups, >> + const unsigned int **groups); >> + int (*set_mux)(const struct scmi_handle *handle, u32 selector, >> + u32 group); >> + int (*get_pin_name)(const struct scmi_handle *handle, u32 selector, >> + const char **name); >> + int (*get_pins_count)(const struct scmi_handle *handle); >> + int (*get_config)(const struct scmi_handle *handle, u32 pin, >> + u32 *config); >> + int (*set_config)(const struct scmi_handle *handle, u32 pin, >> + u32 config); >> + int (*get_config_group)(const struct scmi_handle *handle, u32 pin, >> + u32 *config); >> + int (*set_config_group)(const struct scmi_handle *handle, u32 pin, >> + u32 config); >> + int (*request_pin)(const struct scmi_handle *handle, u32 pin); >> + int (*free_pin)(const struct scmi_handle *handle, u32 pin); >> +}; >> + >> >> As mentioned above, here you could drop a lot of this get_X_count/name/pins >> and instead expose a few of the internal proocol struct scmi__X_info and then >> provide just a mean to query how many resource are there and then get the info >> descriptor you want for the specific domain_id, i.e.: >> >> int (*num_domains_get)(ph, type) >> void *(*info_get)(ph, type, domain_id); >> >> Thanks, >> Cristian >> >> Updated. Exposed selector_type to scmi_protocol, which helped me to >> reduce number of call. >> >> Looking forward for your thoughts about .instance_deinit callback >> implementation I've mentioned above and I will be ready to push v2. >> > > As said, let'see V2 with cleanups in .instance_deinit and lazy > allocations as they are now and move on from there. > > Thanks, > Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel