+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.
Personally I would rather not change the code. But, if you must, I
suggest 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.
Cheers,
John
Sebastian
.
