proposal: require ivar accessors to succeed

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

proposal: require ivar accessors to succeed

Andriy Gapon

__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.
Thank you.
--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

Conrad Meyer-2
Seems like a good idea to me.

Best, Conrad

On Sun, May 26, 2019 at 10:45 PM Andriy Gapon <[hidden email]> 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.
> Thank you.
> --
> Andriy Gapon
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

Bjoern A. Zeeb
In reply to this post by Andriy Gapon
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?

/bz
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

Andriy Gapon
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.


--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

Warner Losh
On Mon, May 27, 2019, 2:47 PM Andriy Gapon <[hidden email]> 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...

Warner


> --
> Andriy Gapon
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

John Baldwin
On 5/27/19 2:04 PM, Warner Losh wrote:

> On Mon, May 27, 2019, 2:47 PM Andriy Gapon <[hidden email]> 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
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: proposal: require ivar accessors to succeed

Andriy Gapon
In reply to this post by Andriy Gapon
On 27/05/2019 08: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.
> Thank you.
>

I've create a review request for this suggestion: https://reviews.freebsd.org/D20458
Please join in if you are interested.

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"