From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753470AbdKFOUO (ORCPT ); Mon, 6 Nov 2017 09:20:14 -0500 Received: from mail-eopbgr00078.outbound.protection.outlook.com ([40.107.0.78]:3920 "EHLO EUR02-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752829AbdKFOUM (ORCPT ); Mon, 6 Nov 2017 09:20:12 -0500 From: Laurentiu Tudor To: Greg KH CC: "devel@driverdev.osuosl.org" , "stuyoder@gmail.com" , "arnd@arndb.de" , "marc.zyngier@arm.com" , Stuart Yoder , Roy Pledge , "linux-kernel@vger.kernel.org" , "agraf@suse.de" , Catalin Horghidan , Ioana Ciornei , "Thomas Gleixner" , Leo Li , Bharat Bhushan , "linux-arm-kernel@lists.infradead.org" , Jason Cooper 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/YCABKcMgA== Date: Mon, 6 Nov 2017 14:20:08 +0000 Message-ID: <5A006F97.1080409@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: [192.88.146.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;VI1PR0401MB2271;6:smE4bY7uGlKPsWVuTahM98lsSGMKd6sVkFso2a6U8GhKCOTWkUOI1U3TGeaLe+Fuf0Z6KLA0DHvf50iWBbyyWKEs5ArYqx8+aeqLUQU47LKnVFi3yNLNKhPdbjF2nRJofY9EbxuCyTcA8MJKabVQqDDm2mNM1EF7GuhcPt70J9lyCBde0vYMRFrXsmKyaYnYxCLrovYD4M7HM3JlwjDQEkvo7pJQ2YIjKpxgKo0zhHD+nABItMHezWqbqyOvB7rYLwU1eDAFu+gKuAXFurmmGM5xiiUX0B+9Gul5ke/N+ssrw45S6Xlpfd38Ei8RnE3KkCXSjnwEXzwLwI4Kz6EWF7hqIETB6ttu+6SIwfSBgzU=;5:kMpyzg/9JThFJR2CJ6wUDkrnYFe1UTTuI1Fg1Va5NloRKGMTG2kXr9Mwf/HEvXFFuFO+F17Tlz6YiVWg5dgibUqXm3j7X4q9uZV1vdU/C36R0KBr3lVTS2hgiWGJ5N9Bk5qaX9KcH8QGDiQvnRFUIKGWwmhB/UbkcPBLXJT9j6U=;24:ntUG0iAiRJi89zS5ZIaVrA0n4Qk/Is2WT+tSUenDL5bFs7Nc4MQJmIH2ScTKAD2cieGkgU+aJvZQwJMXbXoQzvdFz2wISck4ODqklcWvXXs=;7:2b1R1J/2kmOeI0EpDk9GkhGSppFuV0SpN72FVKU0ygaWGXj4kjtS6yLyZ1ydWMwD2DpMV+XPNjU405TsYkWdiU4dKvxSdIM7Idf7vXRI6P5yTGNumld/vIvdfN5uiLGxmRZq8B5B13rsQfetPjTZV5TYTZB5MFH72jFiLppB1cujkO4L2eZDO1Ay76bSV3suld2HTn+dVFAUu77SZANqXNEHIFmdk4mp2JsFKkcQPV32+phUBA0fIZiyHONxda6Z x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI;SCL:-1;SFV:NSPM;SFS:(10009020)(6009001)(39860400002)(376002)(346002)(189002)(24454002)(199003)(7736002)(80316001)(105586002)(5250100002)(6116002)(3846002)(102836003)(101416001)(2906002)(3280700002)(59896002)(33656002)(97736004)(68736007)(2900100001)(305945005)(229853002)(106356001)(66066001)(4326008)(53936002)(5660300001)(8936002)(6246003)(36756003)(39060400002)(2950100002)(6506006)(8676002)(6486002)(6512007)(6436002)(81156014)(86362001)(53546010)(81166006)(25786009)(14454004)(76176999)(7416002)(189998001)(87266999)(50986999)(54356999)(6916009)(478600001)(54906003)(99286004)(316002)(65816999)(3660700001);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR0401MB2271;H:VI1PR0401MB1856.eurprd04.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; x-ms-office365-filtering-correlation-id: 3c63cb0e-33f0-4f07-8cc5-08d525217c0e x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(48565401081)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(2017052603199);SRVR:VI1PR0401MB2271; x-ms-traffictypediagnostic: VI1PR0401MB2271: x-exchange-antispam-report-test: UriScan:(180628864354917)(185117386973197); x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3002001)(100000703101)(100105400095)(3231021)(6055026)(6041248)(20161123558100)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123562025)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:VI1PR0401MB2271;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:VI1PR0401MB2271; x-forefront-prvs: 048396AFA0 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="Windows-1252" Content-ID: <84D2882CF9ABD741AD1EE7F6257B0661@eurprd04.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3c63cb0e-33f0-4f07-8cc5-08d525217c0e X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Nov 2017 14:20:08.7019 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2271 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 vA6EKKGc031791 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? Right. There's an inconsistent mixture of EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() usage. I'll change them all to EXPORT_SYMBOL_GPL(). > 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. Actually, most of the dpcon_* APIs are used in the ethernet driver here: drivers/staging/fsl-dpaa2/ethernet. I think i saw only a couple of functions that are not used so I'll drop those. > Same for your dpaa2_* exported symbols, most are not used from what I > can see. I'll check these too and drop the unused ones. > 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. I think we can drop this refcount altogether as it's not used. Roy, any comment on this? > 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. Right, there are quite of few (100+) WARN_ON()s. I'll go through them and clean them up ... most of them seem paranoid checks anyway. > 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. > I had a quick look over greybus and noticed the device types declared in drivers/staging/greybus/greybus.h plus some helper macros to check the device type. I'll give this a go, though i might return with some questions (e.g. I don't yet understand what device_type::release op is used for). --- Thanks & Best Regards, Laurentiu