From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from s3.sipsolutions.net ([2a01:4f8:191:4433::2] helo=sipsolutions.net) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khgK5-0006oo-Tt for linux-um@lists.infradead.org; Tue, 24 Nov 2020 21:58:06 +0000 Message-ID: Subject: Re: [PATCH 7/7] um: simplify IRQ handling code From: Johannes Berg Date: Tue, 24 Nov 2020 22:58:03 +0100 In-Reply-To: <9cc47f08-2ca2-d4df-d0b0-09552a9a209b@kot-begemot.co.uk> References: <20201123195621.275470-1-johannes@sipsolutions.net> <20201123205446.6370687fa983.I811873233b8d71fbde9154b84c85b498521e3b12@changeid> <9cc47f08-2ca2-d4df-d0b0-09552a9a209b@kot-begemot.co.uk> MIME-Version: 1.0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: Anton Ivanov , linux-um@lists.infradead.org On Tue, 2020-11-24 at 21:50 +0000, Anton Ivanov wrote: > > > static int activate_fd(int irq, int fd, enum um_irq_type type, void *dev_id) > > { > > - struct irq_reg *new_fd; > > struct irq_entry *irq_entry; > > - int i, err, events; > > + int err, events = os_event_mask(type); > > unsigned long flags; > > > > err = os_set_fd_async(fd); > > @@ -151,73 +171,33 @@ static int activate_fd(int irq, int fd, enum um_irq_type type, void *dev_id) > > goto out; > > > > spin_lock_irqsave(&irq_lock, flags); > > - > > - /* Check if we have an entry for this fd */ > > - > > - err = -EBUSY; > > - for (irq_entry = active_fds; > > - irq_entry != NULL; irq_entry = irq_entry->next) { > > - if (irq_entry->fd == fd) > > - break; > > - } > > - > > - if (irq_entry == NULL) { > > - /* This needs to be atomic as it may be called from an > > - * IRQ context. > > - */ > > - irq_entry = kmalloc(sizeof(struct irq_entry), GFP_ATOMIC); > > - if (irq_entry == NULL) { > > - printk(KERN_ERR > > - "Failed to allocate new IRQ entry\n"); > > + irq_entry = get_irq_entry_by_fd(fd); > > + if (irq_entry) { > > This is not right. > > You can re-register the same IRQ/fd, but with a different mask - f.e. > turn on/off write or read on the same fd. F.E. - you have registered a > read IRQ, you after that register same IRQ for write and you can alter > the mask. Hmm. You snipped some code, and it continued like this: irq_entry = get_irq_entry_by_fd(fd); if (irq_entry) { /* cannot register the same FD twice with the same type */ if (WARN_ON(irq_entry->reg[type].events)) { // basically return -EALREADY I'm not sure I see this is different from what it was before? If the irq_entry was found before (by fd, so that's equivalent to get_irq_entry_by_fd(), was just open-coded), then previously it would have gone into - if (irq_entry->irq_array[type] != NULL) { - printk(KERN_ERR - "Trying to reregister IRQ %d FD %d TYPE %d ID %p\n", - irq, fd, type, dev_id - ); - goto out_unlock; which was a failure? But you meant a *different* type (IRQ_READ vs. IRQ_WRITE), and then in my new code we also accept that: irq_entry = get_irq_entry_by_fd(fd); if (irq_entry) { /* cannot register the same FD twice with the same type */ if (WARN_ON(irq_entry->reg[type].events)) { err = -EALREADY; goto out_unlock; } /* temporarily disable to avoid IRQ-side locking */ os_del_epoll_fd(fd); } else { [...] } irq_entry->reg[type].irq = irq; irq_entry->reg[type].active = true; irq_entry->reg[type].events = events; So not sure I see the difference wrt. this? You can't really *alter* the mask, even in the old code, you can just register a new IRQ (or even the same) for the other type (read/write), and then unregister the old type or such. Ok ... what am I missing? johannes _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um