Date: Tue, 28 May 2019 09:43:28 -0700 From: John Baldwin <jhb@FreeBSD.org> To: Warner Losh <imp@bsdimp.com>, Andriy Gapon <avg@freebsd.org> Cc: "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net>, freebsd-arch@freebsd.org Subject: Re: proposal: require ivar accessors to succeed Message-ID: <86d87d65-2eb1-d1bf-2e9f-53420b84c1a9@FreeBSD.org> In-Reply-To: <CANCZdfqRcGujWmkAa-riv1aG08MJgkgTGLEtM9Uk8oTi0RD7WQ@mail.gmail.com> References: <2b2ab28f-45c5-1c28-f923-170d95c20c3d@FreeBSD.org> <E7700915-34BE-4371-A258-C010638CFA38@lists.zabbadoz.net> <f489acfb-c107-82d1-8d01-19f36a78d1c4@FreeBSD.org> <CANCZdfqRcGujWmkAa-riv1aG08MJgkgTGLEtM9Uk8oTi0RD7WQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 5/27/19 2:04 PM, Warner Losh wrote:
> On Mon, May 27, 2019, 2:47 PM Andriy Gapon <avg@freebsd.org> 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 under
>>>> 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 “fixing” 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 = pci_get_devid(dev);
>> with this
>> err = pci_get2_devid(dev, &devid);
>> if (err != 0) {
>> ...
>> }
>>
>> Especially given that, modulo bugs, dev is going to be a device on the pci
>> bus
>> and the call is going to succeed.
>> In other words, in my opinion, the only cases where an accessor is allowed
>> 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...
Fully agree with this.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86d87d65-2eb1-d1bf-2e9f-53420b84c1a9>