proposal: require ivar accessors to succeed
John Baldwin
jhb at FreeBSD.org
Tue May 28 16:43:31 UTC 2019
On 5/27/19 2:04 PM, Warner Losh wrote:
> On Mon, May 27, 2019, 2:47 PM Andriy Gapon <avg at 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
More information about the freebsd-arch
mailing list