From owner-freebsd-arch@freebsd.org Mon May 27 21:04:40 2019 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 299D515AB083 for ; Mon, 27 May 2019 21:04:40 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x734.google.com (mail-qk1-x734.google.com [IPv6:2607:f8b0:4864:20::734]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 716F28654A for ; Mon, 27 May 2019 21:04:38 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x734.google.com with SMTP id p18so19582454qkk.0 for ; Mon, 27 May 2019 14:04:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hbF59l8XGrym5wlBoLdpfL78s6P5a7836uVov9JPYw4=; b=evy+O2mAseA7+nqQUxpG8iaWcw+fYNH02K0Huy2KF4I5nL+Szty237gGJO/3OXrjdg EAIinTOvJ5zXeN7s9H7trqZOrzW2z7paxdjy20iG296J9uW1Tg+/vpQQNU+Dmjumt+/6 DDunSC58zlTnchsDr5JTFee1vbty3PEIL78NEuOg+aJhIAOCfjYh9fDimK1DL+2wOg+a vVTGcpM+Tr9nqlz6PmHP8m3fFa+IAKjgGUI4BvfK3AHbiUxMa0Q7Qz85T0TiMCSSPpMC 3KVwQpdg2L8zXllB0TfzunX8NLMmLpRBcWJoa252DvEnMBDApjUbCouzkajeyO7B701e 75vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hbF59l8XGrym5wlBoLdpfL78s6P5a7836uVov9JPYw4=; b=fTOcoZYmAbHh6j17KV/g6jrp5A5vaOt7mZSRrmyLJs31rnPHgXlYEadklyNvcmtabs kK2cHj9RneYkw5mPHSJ8aTsHjQX7Xuhs3lFbkLohr0GU/xzysIZRwtSE97oEj5wRgmS9 5WaBpzcY6L55FSsSG510K2QPu5QkkuRO4QZjGgmhBuYci9T6zgSPjdg+q3GVF2hC6h9P m4BOM+3uJj9toCOJQuerzUhwbOTJ1eI5FdKfy2FX5GNpNGthoZUvBA9yvFDkPQQNyG9N misZHBn4SGSiJP5vNNkIzDedEDVRVauKBxBronD3aoNSVu1bCy2y4d6ZFCHiQOlls+jN BVew== X-Gm-Message-State: APjAAAXJrGGopeOLeOsqKCpKzwv4k9pOjJcmqVw/1yjpjMUsfD/EXtqY OW7heniPn3ZeLDYa2TQmz2Ecb4SPx42QQU2ytDDLRA== X-Google-Smtp-Source: APXvYqxzBa0hyzpMVUaxZAMEU3WIqYr0Ptoyxps9FaY82xHNW0hkEYUYOWF6QJkTbj94LcZuVVPV/jROE+KPAijKs0E= X-Received: by 2002:aed:24ae:: with SMTP id t43mr96511230qtc.187.1558991077581; Mon, 27 May 2019 14:04:37 -0700 (PDT) MIME-Version: 1.0 References: <2b2ab28f-45c5-1c28-f923-170d95c20c3d@FreeBSD.org> In-Reply-To: From: Warner Losh Date: Mon, 27 May 2019 15:04:25 -0600 Message-ID: Subject: Re: proposal: require ivar accessors to succeed To: Andriy Gapon Cc: "Bjoern A. Zeeb" , freebsd-arch@freebsd.org X-Rspamd-Queue-Id: 716F28654A X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=evy+O2mA X-Spamd-Result: default: False [-5.88 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[freebsd-arch@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; MX_GOOD(-0.01)[ALT1.aspmx.l.google.com,aspmx.l.google.com,ALT2.aspmx.l.google.com]; RCVD_IN_DNSWL_NONE(0.00)[4.3.7.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; NEURAL_HAM_SHORT(-0.85)[-0.852,0]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; IP_SCORE(-3.01)[ip: (-9.44), ipnet: 2607:f8b0::/32(-3.29), asn: 15169(-2.29), country: US(-0.06)]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 May 2019 21:04:40 -0000 On Mon, May 27, 2019, 2:47 PM Andriy Gapon wrote: > On 27/05/2019 21:10, Bjoern A. Zeeb wrote: > > On 27 May 2019, at 5:44, Andriy Gapon wrote: > > > >> __BUS_ACCESSOR() macro is used to define accessors to bus IVAR > variables. > >> Unfortunately, accessors defined in such a fashion completely ignore > return > >> values of BUS_READ_IVAR() and BUS_WRITE_IVAR() method calls. There is > no way to > >> see if a call is successful. Typically, this should not be a problem > as a > >> device driver targets a specific bus (sometimes, buses) and it should > know what > >> IVARs the bus has. So, the driver should make only those IVAR calls > that are > >> supposed to always succeed on the bus. > >> But sometimes things can go wrong as with everything else. > >> > >> So, I am proposing to add some code to __BUS_ACCESSOR to verify the > success. > >> For example, we can panic when a call fails. The checks could be unde= r > >> INVARIANTS or under DIAGNOSTICS or under a new kernel option. > >> A less drastic option is to print a warning message on an error. > >> > >> This is mostly intended to help driver writers and maintainers. > >> > >> Opinions, suggestions, etc are welcome. > > > > What about =E2=80=9Cfixing=E2=80=9D the KPI (possibly adding a 2nd one)= , deprecating the > old > > one, and (slowly over time) migrating old stuff over? > > I think that the two proposals are not mutually exclusive. > And I think that both make sense. > However, it's hard for me to imagine a desire to replace code like this > devid =3D pci_get_devid(dev); > with this > err =3D pci_get2_devid(dev, &devid); > if (err !=3D 0) { > ... > } > > Especially given that, modulo bugs, dev is going to be a device on the pc= i > bus > and the call is going to succeed. > In other words, in my opinion, the only cases where an accessor is allowe= d > to > fail are: > - a driver somehow attached to a device on an unexpected bus > - uncoordinated changes in between a bus driver and a device driver > So, programming errors. > I'm cool with panic. The accessor functions are all supposed to be can't fail. And creating a new set of APIs that can return failure for can't fail things will just bloat the code with cargo cult error handler's that add no value. So put me down on the NO to the new API. If you want to test if the ivar is supported, use the lower level READ_IVAR. Let's not break the API because one person had a bug... Warner > -- > Andriy Gapon > _______________________________________________ > freebsd-arch@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" >