Re: [PATCH v3 03/11] spi: Lock controller idling transition inside the io_mutex

[David Jander <david@xxxxxxxxxxx>]

On Tue, 21 Jun 2022 15:36:23 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Tue, Jun 21, 2022 at 8:15 AM David Jander <david@xxxxxxxxxxx> wrote:
> >
> > This way, the spi sync path does not need to deal with the idling
> > transition.  
> 
> ...
> 
> > -       mutex_lock(&ctlr->io_mutex);
> >         ret = __spi_pump_transfer_message(ctlr, msg, was_busy);
> >         mutex_unlock(&ctlr->io_mutex);
> >
> >         /* Prod the scheduler in case transfer_one() was busy waiting */  
> 
> >         if (!ret)
> >                 cond_resched();  
> 
> In the similar way
> 
> 
> ret = ...
> if (ret)
>   goto out_unlock;
> 
> mutex_unlock();
> cond_resched();
> return;

Trying to add this change as an incremental patch to the whole series now that
it hit linux-next, I am not so sure about how to do this, since this code has
changed:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi.c?id=dc3029056b02414c29b6627e3dd7b16624725ae9#n1729

I understand that the blank line at line 1730 should go after 1732, so that
the ret = ... and the if (!ret) kthread... are not separated by a blank line.

So far so good, but modifying the rest of the code into this "if (!ret)
goto..." idiom will mess that up, since the code in lines 1733 and 1734 is now
common to both paths, so the simplest way I see it is to move those two lines
in between the "ret = ..." and "if (!ret)...". Is that more desirable than not
having the "if (!ret) goto" idiom?

Code would look like this:

	ret = __spi_pump_transfer_message(ctlr, msg, was_busy);
	ctlr->cur_msg = NULL;
	ctlr->fallback = false;
	if (!ret) {
		kthread_queue_work(ctlr->kworker, &ctlr->pump_messages);
		goto out_mutex;
	}

	mutex_unlock(&ctlr->io_mutex);

	/* Prod the scheduler in case transfer_one() was busy waiting */
	cond_resched();
	return;

out_unlock:
	spin_unlock_irqrestore(&ctlr->queue_lock, flags);
out_mutex:
	mutex_unlock(&ctlr->io_mutex);
}

Please advice: is this really more desirable to what it is now? Or can I
better leave it as is and only move the blank line?

> > +       return;
> > +
> > +out_unlock:
> > +       mutex_unlock(&ctlr->io_mutex);  
> 

Best regards,

-- 
David Jander