On Tue, Jun 12, 2018 at 8:04 AM, John Garry <john.garry@xxxxxxxxxx> wrote:
> On 12/06/2018 15:31, Sebastian Andrzej Siewior wrote:
>>
>> 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?
>
>
> Updating the comment is not in itself something to motivate someone to
> change, but we should keep the comment reasonably accurate or get rid.
>
>> 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?
>
>
> I'd prefer an updated comment.
>
I think we should try to remove the unlock completely. I agree with
Sebastian that the audit is never coming. As it is libsas is the only
ata_port_operations implementation that drops the host_lock while
running ->qc_issue().
