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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 201A6C433FE for ; Sat, 25 Sep 2021 10:01:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 03B5D61076 for ; Sat, 25 Sep 2021 10:01:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244119AbhIYKDK (ORCPT ); Sat, 25 Sep 2021 06:03:10 -0400 Received: from mail.kernel.org ([198.145.29.99]:32862 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234958AbhIYKDH (ORCPT ); Sat, 25 Sep 2021 06:03:07 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8A581610C7; Sat, 25 Sep 2021 10:01:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1632564093; bh=vUQvWAePLzdLRZc+ChBxFiJ4phqK0Y1Dzy6kDhpJQjQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=D0RfZDMUicf0Nrzi/I154oj+a7L6krXbMFxL6f7OAeo8Z04aUeN1Hx0PsYmQj/rhd JU4tYtwi9tTNK7v4dxzx0tns/ms73lEwQhhWWb8TPCMXPSjXjl3RE+6mheH6ORhiIY exH7Mhs4jo2IWpb2FgIbU9j5E0vf6zlwGKSJarVLrxpJ5jpuQhQeCKoFstG1MrxkWy wuvhA56lJq/ntXQfDpjKsrb1LcA52GxnQGFmFLSHAoIiCSCUkhMLD3675/Av/t+N2q 5UFJR6wtk3Q+HYEssMyA1zsvkPZW3eS2KBqQp+rE7h0mhFCeMlD7obG50wFu5gsBBh FuSDkIKvRepsg== Date: Sat, 25 Sep 2021 13:01:29 +0300 From: Leon Romanovsky To: Edwin Peer Cc: Jakub Kicinski , "David S . Miller" , Alexander Lobakin , Anirudh Venkataramanan , Ariel Elior , GR-everest-linux-l2@marvell.com, GR-QLogic-Storage-Upstream@marvell.com, Igor Russkikh , intel-wired-lan@lists.osuosl.org, "James E.J. Bottomley" , Javed Hasan , Jeff Kirsher , Jesse Brandeburg , Jiri Pirko , Linux Kernel Mailing List , linux-scsi@vger.kernel.org, "Martin K. Petersen" , Michael Chan , Michal Kalderon , netdev , Sathya Perla , Saurav Kashyap , Tony Nguyen , Vasundhara Volam Subject: Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status Message-ID: References: <20210923183956.506bfde2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Sep 24, 2021 at 10:20:32AM -0700, Edwin Peer wrote: > On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski wrote: > > > On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote: > > > > minor nit: There's obviously nothing incorrect about doing this (and > > > > adding the additional error label in the cleanup code above), but bnxt > > > > has generally adopted a style of having cleanup functions being > > > > idempotent. It generally makes error handling simpler and less error > > > > prone. > > > > > > I would argue that opposite is true. Such "impossible" checks hide unwind > > > flow errors, missing releases e.t.c. > > > > +1, fwiw > > I appreciate that being more explicit can improve visibility, but it > does not make error handling inherently less error prone, nor is it > simpler (ie. the opposite isn't true). Idempotency is orthogonal to > unwind flow or the presence or not of a particular unwind handler (one > can still enforce either in review). But, if release handlers are > independent (most in bnxt are), then permitting other orderings can be > perfectly valid and places less burden on achieving the canonical form > for correctness (ie. usage is simpler and less error prone). That's > not to say we should throw caution to the wind and allow arbitrary > unwind flows, but it does mean certain mistakes don't result in actual > bugs. There are other flexibility benefits too. A single, unwind > everything, handler can be reused in more than one context. And this is where the fun begins. Different context means different lifetime expectations, maybe need of locking and unpredictable flows from reader perspective. For example, in this devlink case, it took me time to check all driver to see that pf can't be null. The idea that adding code that maybe will be used can be seen as anti-pattern. Thanks