On Tue, Jun 12, 2018 at 8:04 AM, John Garry <john.ga...@huawei.com> 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().

Reply via email to