From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752773AbdK0Pcf (ORCPT ); Mon, 27 Nov 2017 10:32:35 -0500 Received: from mail-db5eur01on0084.outbound.protection.outlook.com ([104.47.2.84]:58598 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752291AbdK0Pce (ORCPT ); Mon, 27 Nov 2017 10:32:34 -0500 From: Laurentiu Tudor To: Greg KH CC: "devel@driverdev.osuosl.org" , "stuyoder@gmail.com" , Stuart Yoder , Roy Pledge , "linux-kernel@vger.kernel.org" , "agraf@suse.de" , Catalin Horghidan , Ioana Ciornei , Leo Li , Bharat Bhushan , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging Thread-Topic: [PATCH v2] staging: fsl-mc: move bus driver out of staging Thread-Index: AQHTH+wFpJBJc2M940O/dZz67KMOpqKepc8AgGSH/YCAJbwRgA== Date: Mon, 27 Nov 2017 15:32:28 +0000 Message-ID: <5A1C2FEB.2080006@nxp.com> References: <20170828105405.19552-1-laurentiu.tudor@nxp.com> <20170831160430.GB11200@kroah.com> <20171103151711.GA22453@kroah.com> In-Reply-To: <20171103151711.GA22453@kroah.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=laurentiu.tudor@nxp.com; x-originating-ip: [86.34.165.90] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;VI1PR0401MB2560;6:88ZAPjzuWoXSxfpnUyEburXOEmMrUG4HFpb52Met8dLlGCwX2OdL6BZx5VVv4FkpG2P2/6O2UULDhi++lSqIJKC5nJ6SeO025nZevbyzlko3EvwKR4o83WT87y+g2blJ9Uw24LkIJsUtKLXMj4cEDodrwh/O5oloZlp0gn9RCSfRU7r8w6f4ljeISzjZrgf0HK/sEX92nSBmemfOegofSIeh4t+MjTmekiiUxDEn6FIUxpf5mkDHKfBpCJlSrjO3iTxY+FR7uEo1ZPtiU1cYREoEsbiycbGCLQjPEBZbzutZWXkCbr1Xr1ALhOrGwK7ks9C7nOl/VEWbPBYV4s7rPeyb+oS0rlPSnOHa+31f+1A=;5:cHrsAZ9msYPYhn9U7L1tOkhOaMhPveMtJbqAWM3dfnZzkzSFR6JujGff5shoL7rdUwaNXLwV2nkU/HTKXvjM0Vg4JUDYlolKGl2S6FSsledroidNTTc7u+3MHngzHBul3bKd22LkL/CIPDihos04GdYCb/WsJAKvK5L4JWPwcdM=;24:F5arUXlwKBkMDR18haSOTNJVidP8M0/36Du3DCiU4U02iaxVUAYmiqk9XpAmt5LrhjuWqTCxuepfd2uXM5hzKu+Jyc+ojKIkAb9doMUvjSI=;7:FgfgZldVJybxRsKwDIYPweMObabHLtsVqaPE4un4/zC1+G1hR4KFcf2lNyHsdpsWjT+cUowo0c3bJrQCB77/qIMil3XzBXEtYFFd47yCEv4ksaNisQ91L1yet86KuRmaCFA779RSMlgXX8i0CT9rYataM/tM40WDbQrcOtlxVsTTf2uZ/1q0jPZBWqihNQdnnR/IxOsOp6kl+xMpsuFQmhLJAH66dkMkKBUtAtTBj5PYAdoPkzIfkYCREXAbi/Xl x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI;SCL:-1;SFV:NSPM;SFS:(10009020)(6009001)(366004)(39860400002)(376002)(346002)(189002)(24454002)(199003)(65816999)(54356999)(50986999)(76176999)(81156014)(81166006)(8676002)(33656002)(6916009)(5660300001)(25786009)(2950100002)(66066001)(2900100001)(59896002)(3660700001)(6116002)(68736007)(105586002)(106356001)(101416001)(7736002)(305945005)(8936002)(36756003)(97736004)(6486002)(189998001)(39060400002)(5250100002)(316002)(6512007)(478600001)(4326008)(3846002)(102836003)(53546010)(54906003)(87266999)(6246003)(14454004)(3280700002)(2906002)(86362001)(6506006)(6436002)(53936002)(99286004)(80316001)(229853002);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR0401MB2560;H:VI1PR0401MB1856.eurprd04.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 22fb70bf-9be5-48b6-d5d7-08d535ac11a8 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(48565401081)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(5600026)(4604075)(2017052603199);SRVR:VI1PR0401MB2560; x-ms-traffictypediagnostic: VI1PR0401MB2560: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917)(185117386973197); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040450)(2401047)(5005006)(8121501046)(3002001)(93006095)(93001095)(10201501046)(3231022)(6055026)(6041248)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123562025)(20161123560025)(20161123558100)(6072148)(201708071742011);SRVR:VI1PR0401MB2560;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:VI1PR0401MB2560; x-forefront-prvs: 0504F29D72 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="Windows-1252" Content-ID: <0E33B0CA0613DD49928F605F34FB3DC9@eurprd04.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 22fb70bf-9be5-48b6-d5d7-08d535ac11a8 X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Nov 2017 15:32:28.8155 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2560 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id vARFWfhs003446 On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tudor@nxp.com wrote: >>> From: Stuart Yoder >>> >>> Move the source files out of staging into their final locations: >>> -include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>> -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>> -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>> -README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > other exports and global namespace, right? > > You export a lot of dpcon_* symbols that no one uses, please do not do > that. Verify that all of them are actually used right now, if not, > remove them. If you think you are going to use them in the future, > wonderful, add them in then. > > Same for your dpaa2_* exported symbols, most are not used from what I > can see. > > struct dpaa2_io { > atomic_t refs; > > That's a kref, please use it instead of trying to roll your own. > > And even for this, your locking is not correct (i.e. you do not have > any), that needs to be fixed so that teardown works correctly. > > You have a lot of WARN_ON() calls, that's going to be ignored and should > all not be needed now that the code is debugged and working properly. > Please remove them, or turn them into dev_err() calls with a real if () > check instead. > > You are checking "strings" for the type of device in a lot of places, > like this: > if (strcmp(obj_desc->type, "dprc") == 0) { > why are you not just using the built-in driver model .type field and > comparing that to the different type structures? It's much easier, and > you don't have to again, "roll your own". See the USB or Greybus code > for examples of busses that have different "types" of devices on them at > the same time. > > Ok, that's enough for now, please work on this, and we can go from > there... > What would the next steps be, now that the patches are in staging-next? Are there plans for a new round of review? --- Thanks & Best Regards, Laurentiu