From owner-svn-src-all@freebsd.org Tue Feb 7 10:35:49 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7E875CD4E4F; Tue, 7 Feb 2017 10:35:49 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4A73F1C80; Tue, 7 Feb 2017 10:35:49 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: by mail-it0-x243.google.com with SMTP id f200so11956319itf.3; Tue, 07 Feb 2017 02:35:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=JRYvN2xMbvtDWPp7sxYp156YqPkm4tkvaYYoYAFmNa8=; b=XQI5x8kWnmnIgej6Fe6Ed4ajKfbiAaQvnwj+9nwerSDSZL2eyci2sUkc21lYrKB6LH t87PaSD8OkQgVcyWfW6Hreto/09m95Ut1+q3rv5KD1vhdqKWXUwi8bpNg0qCDeqdfu5L QxBjFTUOQOIa98ouVtitpBbjpHAvkdCKBpukIROm4shSRYXDu7qcfxdv/VIedmf/OGEx WWX+f1M9U1oy4CSLyZDI/Q0isnRzWMdeTUw4JhkEIDViCKbwmtD0EdSAwaLwINQ217QA 2zsU3zLFDmYBioqGDaK6lCxKbMyRJagsGKOwmPvJtFVRAf5Fz2URXIkQcNvAcu6s/rMe jYSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=JRYvN2xMbvtDWPp7sxYp156YqPkm4tkvaYYoYAFmNa8=; b=FQNv4Uf9MDvkvGUfuIyPpfN+23njhp+aC/jXV0x3MGH9hs5ZrnrgCmCxU1pYKDvhdl FYfBAInoIHWoLXLFt82jzQ2A79O1H2G3D9UlXW0NDEGUwjUUEYwWwWU6Q8qZvK870FPy BwQAb8t4ODgd8MWspa1lhP7myG93KN96Bw0S+d9ZZduikI7Bx+CFB/T4FmagasTmb3qe /LM8xT98H9Om1L9zSrMC9oLcJ++RK3TvdJljPPdX10MzdqsAz8n21DYUjJUCkwwLu8yY s5xxAkm71LK0feZ5tIbzlA7w0DM0qD9YiwV3sAcrErZZf/+31nKK2dY71aTx3fb8a6T1 Ylgw== X-Gm-Message-State: AIkVDXJdyPPSUIsiwlxcco6778flnchhPO4QjnhPjdoiIZr/4QjhywT4QDg86zxgcvZpamN94/01FqpQ3aRZcg== X-Received: by 10.36.131.65 with SMTP id d62mr11433780ite.111.1486463748435; Tue, 07 Feb 2017 02:35:48 -0800 (PST) MIME-Version: 1.0 Received: by 10.64.101.194 with HTTP; Tue, 7 Feb 2017 02:35:48 -0800 (PST) In-Reply-To: <201702061308.v16D8nGC071178@repo.freebsd.org> References: <201702061308.v16D8nGC071178@repo.freebsd.org> From: Svatopluk Kraus Date: Tue, 7 Feb 2017 11:35:48 +0100 Message-ID: Subject: Re: svn commit: r313339 - head/sys/kern To: Andrew Turner Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Feb 2017 10:35:49 -0000 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 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 \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 \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))); > >