Date: Wed, 8 Feb 2017 13:58:41 +0100 From: Svatopluk Kraus <onwahe@gmail.com> To: Andrew Turner <andrew@fubar.geek.nz> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313339 - head/sys/kern Message-ID: <CAFHCsPW%2B=GUv=s5VBVNc4-S3HM_apPizROkkw-e=9i3JNaB2jw@mail.gmail.com> In-Reply-To: <20170207113844.2cd08852@zapp> References: <201702061308.v16D8nGC071178@repo.freebsd.org> <CAFHCsPUFnLUbqvqW%2BnG7K=J%2B4j7P0Nub79PQNh_LoUv9Eb-eaA@mail.gmail.com> <20170207113844.2cd08852@zapp>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 7, 2017 at 12:38 PM, Andrew Turner <andrew@fubar.geek.nz> wrote: > On Tue, 7 Feb 2017 11:35:48 +0100 > Svatopluk Kraus <onwahe@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@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?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPW%2B=GUv=s5VBVNc4-S3HM_apPizROkkw-e=9i3JNaB2jw>