svn commit: r313339 - head/sys/kern
Svatopluk Kraus
onwahe at gmail.com
Tue Feb 7 10:35:49 UTC 2017
So, you did it again. No discussion, you have just committed what you
wanted even if I requested a change. It means that I wanted to review
it! Or you think that I forget about that when you wait for three
months?
Your change is not explained at all !! So, explain it better or revert!
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?
Except for the flag, intr_pic_register() is same as intr_msi_register() now.
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
>
> Modified:
> head/sys/kern/subr_intr.c
>
> Modified: head/sys/kern/subr_intr.c
> ==============================================================================
> --- head/sys/kern/subr_intr.c Mon Feb 6 11:37:20 2017 (r313338)
> +++ head/sys/kern/subr_intr.c Mon Feb 6 13:08:48 2017 (r313339)
> @@ -105,8 +105,10 @@ struct intr_pic {
> SLIST_ENTRY(intr_pic) pic_next;
> intptr_t pic_xref; /* hardware identification */
> device_t pic_dev;
> +/* Only one of FLAG_PIC or FLAG_MSI may be set */
> #define FLAG_PIC (1 << 0)
> #define FLAG_MSI (1 << 1)
> +#define FLAG_TYPE_MASK (FLAG_PIC | FLAG_MSI)
> u_int pic_flags;
> struct mtx pic_child_lock;
> SLIST_HEAD(, intr_pic_child) pic_children;
> @@ -115,7 +117,7 @@ struct intr_pic {
> static struct mtx pic_list_lock;
> static SLIST_HEAD(, intr_pic) pic_list;
>
> -static struct intr_pic *pic_lookup(device_t dev, intptr_t xref);
> +static struct intr_pic *pic_lookup(device_t dev, intptr_t xref, int flags);
>
> /* Interrupt source definition. */
> static struct mtx isrc_table_lock;
> @@ -688,7 +690,7 @@ isrc_add_handler(struct intr_irqsrc *isr
> * Lookup interrupt controller locked.
> */
> static inline struct intr_pic *
> -pic_lookup_locked(device_t dev, intptr_t xref)
> +pic_lookup_locked(device_t dev, intptr_t xref, int flags)
> {
> struct intr_pic *pic;
>
> @@ -699,6 +701,10 @@ pic_lookup_locked(device_t dev, intptr_t
>
> /* Note that pic->pic_dev is never NULL on registered PIC. */
> SLIST_FOREACH(pic, &pic_list, pic_next) {
> + if ((pic->pic_flags & FLAG_TYPE_MASK) !=
> + (flags & FLAG_TYPE_MASK))
> + continue;
> +
> if (dev == NULL) {
> if (xref == pic->pic_xref)
> return (pic);
> @@ -715,12 +721,12 @@ pic_lookup_locked(device_t dev, intptr_t
> * Lookup interrupt controller.
> */
> static struct intr_pic *
> -pic_lookup(device_t dev, intptr_t xref)
> +pic_lookup(device_t dev, intptr_t xref, int flags)
> {
> struct intr_pic *pic;
>
> mtx_lock(&pic_list_lock);
> - pic = pic_lookup_locked(dev, xref);
> + pic = pic_lookup_locked(dev, xref, flags);
> mtx_unlock(&pic_list_lock);
> return (pic);
> }
> @@ -729,12 +735,12 @@ pic_lookup(device_t dev, intptr_t xref)
> * Create interrupt controller.
> */
> static struct intr_pic *
> -pic_create(device_t dev, intptr_t xref)
> +pic_create(device_t dev, intptr_t xref, int flags)
> {
> struct intr_pic *pic;
>
> mtx_lock(&pic_list_lock);
> - pic = pic_lookup_locked(dev, xref);
> + pic = pic_lookup_locked(dev, xref, flags);
> if (pic != NULL) {
> mtx_unlock(&pic_list_lock);
> return (pic);
> @@ -746,6 +752,7 @@ pic_create(device_t dev, intptr_t xref)
> }
> pic->pic_xref = xref;
> pic->pic_dev = dev;
> + pic->pic_flags = flags;
> mtx_init(&pic->pic_child_lock, "pic child lock", NULL, MTX_SPIN);
> SLIST_INSERT_HEAD(&pic_list, pic, pic_next);
> mtx_unlock(&pic_list_lock);
> @@ -757,12 +764,12 @@ pic_create(device_t dev, intptr_t xref)
> * Destroy interrupt controller.
> */
> static void
> -pic_destroy(device_t dev, intptr_t xref)
> +pic_destroy(device_t dev, intptr_t xref, int flags)
> {
> struct intr_pic *pic;
>
> mtx_lock(&pic_list_lock);
> - pic = pic_lookup_locked(dev, xref);
> + pic = pic_lookup_locked(dev, xref, flags);
> if (pic == NULL) {
> mtx_unlock(&pic_list_lock);
> return;
> @@ -783,12 +790,10 @@ intr_pic_register(device_t dev, intptr_t
>
> if (dev == NULL)
> return (NULL);
> - pic = pic_create(dev, xref);
> + pic = pic_create(dev, xref, FLAG_PIC);
> if (pic == NULL)
> return (NULL);
>
> - pic->pic_flags |= FLAG_PIC;
> -
> debugf("PIC %p registered for %s <dev %p, xref %x>\n", pic,
> device_get_nameunit(dev), dev, xref);
> return (pic);
> @@ -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!
> @@ -870,7 +875,8 @@ intr_pic_add_handler(device_t parent, st
> struct intr_pic_child *child;
> #endif
>
> - parent_pic = pic_lookup(parent, 0);
> + /* Find the parent PIC */
> + parent_pic = pic_lookup(parent, 0, FLAG_PIC);
> if (parent_pic == NULL)
> return (NULL);
>
> @@ -904,13 +910,14 @@ intr_resolve_irq(device_t dev, intptr_t
> if (data == NULL)
> return (EINVAL);
>
> - pic = pic_lookup(dev, xref);
> + pic = pic_lookup(dev, xref,
> + (data->type == INTR_MAP_DATA_MSI) ? FLAG_MSI : FLAG_PIC);
> if (pic == NULL)
> return (ESRCH);
>
> switch (data->type) {
> case INTR_MAP_DATA_MSI:
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
> msi = (struct intr_map_data_msi *)data;
> @@ -918,7 +925,7 @@ intr_resolve_irq(device_t dev, intptr_t
> return (0);
>
> default:
> - 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)));
> return (PIC_MAP_INTR(pic->pic_dev, data, isrc));
> @@ -1255,12 +1262,10 @@ intr_msi_register(device_t dev, intptr_t
>
> if (dev == NULL)
> return (EINVAL);
> - pic = pic_create(dev, xref);
> + pic = pic_create(dev, xref, FLAG_MSI);
> if (pic == NULL)
> return (ENOMEM);
>
> - pic->pic_flags |= FLAG_MSI;
> -
> debugf("PIC %p registered for %s <dev %p, xref %jx>\n", pic,
> device_get_nameunit(dev), dev, (uintmax_t)xref);
> return (0);
> @@ -1276,11 +1281,11 @@ intr_alloc_msi(device_t pci, device_t ch
> struct intr_map_data_msi *msi;
> int err, i;
>
> - pic = pic_lookup(NULL, xref);
> + pic = pic_lookup(NULL, xref, FLAG_MSI);
> if (pic == NULL)
> return (ESRCH);
>
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
>
> @@ -1313,11 +1318,11 @@ intr_release_msi(device_t pci, device_t
> struct intr_map_data_msi *msi;
> int i, err;
>
> - pic = pic_lookup(NULL, xref);
> + pic = pic_lookup(NULL, xref, FLAG_MSI);
> if (pic == NULL)
> return (ESRCH);
>
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
>
> @@ -1352,11 +1357,11 @@ intr_alloc_msix(device_t pci, device_t c
> struct intr_map_data_msi *msi;
> int err;
>
> - pic = pic_lookup(NULL, xref);
> + pic = pic_lookup(NULL, xref, FLAG_MSI);
> if (pic == NULL)
> return (ESRCH);
>
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
>
> @@ -1380,11 +1385,11 @@ intr_release_msix(device_t pci, device_t
> struct intr_map_data_msi *msi;
> int err;
>
> - pic = pic_lookup(NULL, xref);
> + pic = pic_lookup(NULL, xref, FLAG_MSI);
> if (pic == NULL)
> return (ESRCH);
>
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
>
> @@ -1413,11 +1418,11 @@ intr_map_msi(device_t pci, device_t chil
> struct intr_pic *pic;
> int err;
>
> - pic = pic_lookup(NULL, xref);
> + pic = pic_lookup(NULL, xref, FLAG_MSI);
> if (pic == NULL)
> return (ESRCH);
>
> - KASSERT((pic->pic_flags & FLAG_MSI) != 0,
> + KASSERT((pic->pic_flags & FLAG_TYPE_MASK) == FLAG_MSI,
> ("%s: Found a non-MSI controller: %s", __func__,
> device_get_name(pic->pic_dev)));
>
>
More information about the svn-src-all
mailing list