Re: [PATCH 7/7] um: simplify IRQ handling code

[Johannes Berg <johannes@sipsolutions.net>]

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