svn commit: r313339 - head/sys/kern

[Svatopluk Kraus]

On Tue, Feb 7, 2017 at 12:38 PM, Andrew Turner <andrew at fubar.geek.nz> wrote:
> On Tue, 7 Feb 2017 11:35:48 +0100
> Svatopluk Kraus <onwahe at gmail.com> wrote:
>> Does an xref refer only to one hardware? Or two hardwares may have
>> same xrefs? Why one hardware cannot be represented by one PIC in
>> INTRNG even if two drivers exist for it?
>
> The xref is an FDT thing, in INTRNG we use it as a handle to hardware.
> This changes it so each of these handles refers to hardware within one
> of two spaces, the PIC space and MSI space. It may be that for FDT
> these spaces are non overlapping, however with ACPI it will simplify
> the code to allow us to have the same handle refer to different
> controllers in different spaces.


In INTRNG, the xref is not FDT thing, but general hardware identifier
which is intented to be unique in a system. See

intr_map_irq(device_t dev, intptr_t xref, struct intr_map_data *data).

The xref should be independent on mapping data. INTRNG should know
nothing about mapping data, but your change just changed that. The
xref is opaque for INTRNG too. So, I do not see where is the problem
to have unique xref identifiers in a system. The xref can be defined
that way.

So, do you have a problem with overlapping spaces or you just want to
have more PICs for one xref?


>
>> On Mon, Feb 6, 2017 at 2:08 PM, Andrew Turner <andrew at freebsd.org>
>> wrote:
>> > Author: andrew
>> > Date: Mon Feb  6 13:08:48 2017
>> > New Revision: 313339
>> > URL: https://svnweb.freebsd.org/changeset/base/313339
>> >
>> > Log:
>> >   Only allow the pic type to be either a PIC or MSI type. All
>> > interrupt controller drivers handle either MSI/MSI-X interrupts, or
>> > regular interrupts, as such enforce this in the interrupt handling
>> > framework. If a later driver was to handle both it would need to
>> > create one of each.
>> >
>> >   This will allow future changes to allow the xref space to
>> > overlap, but refer to different drivers.
>> >
>> >   Obtained from:        ABT Systems Ltd
>> >   Sponsored by: The FreeBSD Foundation
>> >   X-Differential Revision:      https://reviews.freebsd.org/D8616
> ...
>> > @@ -822,13 +827,13 @@ intr_pic_claim_root(device_t dev, intptr
>> >  {
>> >         struct intr_pic *pic;
>> >
>> > -       pic = pic_lookup(dev, xref);
>> > +       pic = pic_lookup(dev, xref, FLAG_PIC);
>> >         if (pic == NULL) {
>> >                 device_printf(dev, "not registered\n");
>> >                 return (EINVAL);
>> >         }
>> >
>> > -       KASSERT((pic->pic_flags & FLAG_PIC) != 0,
>> > +       KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_PIC,
>> >             ("%s: Found a non-PIC controller: %s", __func__,
>> >              device_get_name(pic->pic_dev)));
>> >
>>
>> So, you would check that  pic_lookup(dev, xref, FLAG_PIC) returns PIC
>> with FLAG_PIC set?
>> Really? This nonsense is in other places too. If you would like to be
>> this way, check it only in pic_lookup() then!
>
> It's there so if someone makes a change to pic_lookup that breaks
> this assumption they will be told about it.

For me, it's too much. It's like calling mtx_lock() and immediately
check that the mutex is locked. It's mental to check that a function
really returned what was requested. Especially, when the function
returns NULL in case that the request cannot be met. Should anyone
check any function that it does what is announced?