On 2018-06-12 13:54:36 [+0100], John Garry wrote:
> +Dan
>
> On 11/06/2018 19:23, Sebastian Andrzej Siewior wrote:
> > On 2018-06-11 18:12:55 [+0100], John Garry wrote:
> > > On 11/06/2018 15:40, Sebastian Andrzej Siewior wrote:
> > > > This is the repost of the two patches I posted in earlier this month:
> > > >
> > > > - [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()
> > > > Received feedback but nothing really changed. I explained that this is
> > > > not about "local_irqsave() + spin_lock()" *but* "local_irq_save() +
> > > > spin_unlock()". This seemed to have been overseen twice.
> > > > Also there were two opinions about the TODO comment:
> > > > /* TODO: audit callers to ensure they are ready for qc_issue to
> > > > * unconditionally re-enable interrupts
> > > > It is unclear to me if this comment should be removed because it
> > > > makes no sense or if the intention was indeed to audit callers code
> > > > for a possible "spin_unlock_irq(ap->lock);".
> > >
> > > Hi,
> > >
> > > As I said previously, since it is not clear now what the comment meant,
> > > then
> > > removing the irq save/restore calls will only make it even less clear, and
> > > should be fixed.
> >
> > how should it be fixed? The discussion went forth and back. The comment
> > as of now does not match the code. It disables interrupts which are
> > already disabled. It does not enable them at any point.
>
> As I see, at the point we release the lock, the question is if we can just
> re-enable interrupts as probably we disabled interrupts earlier for taking
> the same lock.
>
> However, as mentioned, we should not need to disable interrupts as they
> should have been already disabled.
The irq_save + unlock combo got first introduced in commit ce4f75def399
("isci: Free host lock for SATA/STP abort escalation at submission
time."). It then got moved forth and back until it ended where it is
today with the comment Dan put there. Back then the code path was:
- ata_exec_internal_sg()
spin_lock_irqsave(ap->lock, flags);
-> ata_qc_issue() (saying LOCKING:spin_lock_irqsave(host lock)
-> qc->err_mask |= ap->ops->qc_issue(qc); => sas_ata_qc_issue()
-> i->dft->lldd_execute_task(task, 1, GFP_ATOMIC); =>
isci_task_execute_task()
->isci_request_execute(ihost, task, &request, gfp_flags);
if (dev_is_sata(task->dev)) {
/* Since we are still in the submit path, and since
* libsas takes the host lock on behalf of SATA
* devices before I/O starts, we need to unlock
* before we can put the task in the error path.
*/
raw_local_irq_save(flags);
spin_unlock(isci_host->shost->host_lock);
sas_task_abort(task)
So. This when the mistake was introduced - it shouldn't get in there
like that.
> Personally I would rather not change the code. But, if you must, I suggest
of course.
> update the comment to read something like this:
> - TODO: It may be possible to unconditionally re-enable interrupts for
> period of having the lock released. Audit callers to check.
We had this comment for 6 years or so and nothing happend. What makes
you think that an updated version of that comment will motivate someone
to make change here in the near future?
It looks to me like a stale comment which won't change a thing because
it does not point out the benefit of doing so (re-enabling interrupts
while dropping the lock) and the price, that is paid for not doing so
(keeping the code as it is) is small enough to not bother.
So if updating the comment as suggested instead of keeping it as-is or
removing it is the blocker *here* then I can send an updated version.
Any comments?
> Cheers,
> John
Sebastian
