From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x226sa39+IR5tgKj3xCdxZosLgo5L0+uWzUQXzdaa+y4BYeMaC8ONgcRbTHVDjT9yVlYhsszb ARC-Seal: i=1; a=rsa-sha256; t=1517024702; cv=none; d=google.com; s=arc-20160816; b=kOM5EsHkz87f9DueYTjpul07EpEM9J/0k742RpXiAp+RS1j7F8Rh9dTYeq1EY9nZ94 nOjG6V9NXogLnGZ19RrFKyD9KOFbqj0ohrLHFJtuIyBgjKR65p/vm68s9zm8Yp8Ex+y9 m0AoUhp/fBX5Gv0YABjt/S+lvFRjep3KJX9NxowC8qLO/ka5DdzidIE6IGFy7F1eJP99 HHbQcRqXLpL067zJRgupEdTp4eNC7hOtj9JnbM5+vDCqN8dguhKEAldqBWgMsGv7ZDJ0 HJH5C/emkIZbpoD0nf9289SHabOFREudQteWAMUYOgqrobIeb10OdaPrdoiNdqu+mOA6 BH+w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:user-agent:message-id :in-reply-to:date:references:subject:cc:to:from :arc-authentication-results; bh=e8fk4UR5q7o+uHTkIsVwktOY8WJHoabSN7c/p0g39OU=; b=QtH6eSJ7kzCGRW1tggHt7yQrJXNTUsOLX/QnQI0r6NBdOTBYaugg9g2N/7RAFCJ70w 8rjPeOF5F2Cv2bxZtmGZpkY80my0ZBl/d4cStVEGtUr2R3tai/5hw4INR4mz5K1dCSix UmMbOjL54Uv7CbjDSeAXhdu/FJUKgtbuv9P9pzv064+3BiZ7I9CjcHv+1uz40K19Twrh gsJ8m5RN1blEhbz8F7TBpm0DaONR/bEoQAXGu9ye7/hIW+AUxH1xlLnXo1BO2pd0ntCo 23PtzBSfg5q6KS64Fp1Y6FiC0mjPmxXj8gDCk3wm0I/TsZLB6UOP6Mwqqx9p+wdstbdX HOvQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of ashutosh.dixit@intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=ashutosh.dixit@intel.com Authentication-Results: mx.google.com; spf=pass (google.com: domain of ashutosh.dixit@intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=ashutosh.dixit@intel.com X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,420,1511856000"; d="scan'208";a="199180077" From: Ashutosh Dixit To: Al Viro Cc: Christopher =?utf-8?Q?D=C3=ADaz?= Riveros , "Dutt\, Sudeep" , "arnd\@arndb.de" , "gregkh\@linuxfoundation.org" , "linux-kernel\@vger.kernel.org" , "kernel-janitors\@vger.kernel.org" Subject: Re: [PATCH-next] misc: mic: Use PTR_ERR_OR_ZERO References: <20180123201009.12998-1-chrisadr@gentoo.org> <20180123215519.GP13338@ZenIV.linux.org.uk> Date: Fri, 26 Jan 2018 19:45:00 -0800 In-Reply-To: <20180123215519.GP13338@ZenIV.linux.org.uk> (Al Viro's message of "Tue, 23 Jan 2018 14:55:19 -0700") Message-ID: <87shasezib.fsf@intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1590415328315757561?= X-GMAIL-MSGID: =?utf-8?q?1590715694176256989?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Tue, Jan 23 2018 at 02:55:19 PM, Al Viro wrote: > On Tue, Jan 23, 2018 at 03:10:09PM -0500, Christopher D=C3=ADaz Riveros w= rote: >> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR >>=20 >> This issue was detected by using the Coccinelle software. > > ... and that's a wonderful demonstration of the reasons why PTR_ERR_OR_ZE= RO() > is in bad taste. > > Look at the callers of that sucker: > > err =3D scif_anon_inode_getfile(ep); > if (err) > goto err_anon_inode; > > and > > err =3D scif_anon_inode_getfile(cep); > if (err) > goto scif_accept_error_anon_inode; > > See the problem? What really happens here is that we > * call anon_inode_getfile() and stick the result into ->anon. > * if that was ERR_PTR(), we bugger off > > Checking that PTR_ERR_OR_ZERO() is non-zero is a bloody convoluted way of > spelling > if (IS_ERR(...)) { > err =3D PTR_ERR(...); > goto sod_off; > } > > Your change preserves the ugliness of the original calling conventions; > the mistake here is making it return an int. Add the fact that > all (both) callers are in drivers/misc/mic/scif/scif_api.c and take > a look at this: > ; git grep -n -w scif_anon_fops > drivers/misc/mic/scif/scif_api.c:47:const struct file_operations scif_ano= n_fops =3D { > drivers/misc/mic/scif/scif_epd.h:167: epd->anon =3D anon_inode_getfile(= "scif", &scif_anon_fops, NULL, 0); > drivers/misc/mic/scif/scif_main.h:213:extern const struct file_operations= scif_anon_fops; > ; > > So scif_anon_fops is > * defined in scif_api.c > * declared in scif_main.h > * used only in an inline function defined in scif_epd.h, which is used > only in scif_api.c > > Could you spell "way too convoluted"? > > Please, do the following: move that sucker scif_api.c, and make it return > struct file *. As in return epd->anon =3D anon_inode_getfile(...); > And turn the callers into > if (IS_ERR(whatever_you_call_it(ep))) { > err =3D PTR_ERR(ep->anon); > goto ...; > } > Then make scif_anon_fops static and kill that extern in scif_main.h. > > What the hell? for callers of scif_poll()> > > Let me see if I got it right - *ALL* callers of that thing pass it > an array consisting of 1 (one) entry. So playing with poll_wqueues > and __pollwait() is utterly pointless in scif_poll(). So is grabbing > references to that struct file, BTW, regardless of everything else. > And so is having the bloody ->anon at all - __scif_pollfd() should be > passed NULL as the first argument and that's it. All it takes is > a separate queue callback set via init_poll_funcptr() and to hell > with all those complication. Oh, and since we don't really need > dynmic allocations inside that queue callback, table.error crap > also goes to hell... > > Incidentally, since this is one of the two places outside of fs/select.c > using poll_{init,free}wait(), we can bloody well unexport those - the > other caller (in drivers/staging/farce^Wcomedi) also uses it for > single struct file, pinned by the caller. And while we are at it, > that would also make struct poll_wqueues private to fs/select.c... > > Could Intel folks describe the planned future uses of scif_poll()? > If it's not going to be used for large sets, the whole thing could > be simplified quite nicely... Cleanups and suggestions to improve the code such as those outlined at the top of this reply are of course welcome. The SCIF API has both user and kernel space clients, and the kernel API mirrors the user space API. Similar to user space poll which supports multiple file descriptors, the kernel client poll API was designed to allow poll on multiple SCIF endpoints as well. Since the API has been stable for a couple of years now, our preference is to preserve the API as far as possible.