Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V2] i2c: tegra: disable irq in tegra_i2c_xfer_msg
To:     Jon Hunter <jonathanh@nvidia.com>,
        Bitan Biswas <bbiswas@nvidia.com>,
        Laxman Dewangan <ldewangan@nvidia.com>,
        Thierry Reding <treding@nvidia.com>, linux-i2c@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        Peter Rosin <peda@axentia.se>, Wolfram Sang <wsa@the-dreams.de>
Cc:     Shardar Mohammed <smohammed@nvidia.com>,
        Sowjanya Komatineni <skomatineni@nvidia.com>,
        Mantravadi Karthik <mkarthik@nvidia.com>
References: <1560847368-16069-1-git-send-email-bbiswas@nvidia.com>
 <61c2dbcd-85f2-2094-7596-78ac01f55421@nvidia.com>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <28cd06ca-3bd4-87e1-6d52-504cd00e1adb@gmail.com>
Date:   Tue, 18 Jun 2019 21:34:08 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <61c2dbcd-85f2-2094-7596-78ac01f55421@nvidia.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

18.06.2019 12:13, Jon Hunter пишет:
> 
> On 18/06/2019 09:42, Bitan Biswas wrote:
>> tegra_i2c_xfer_msg initiates the I2C transfer in DMA
>> or PIO mode. It involves steps that need FIFO register
>> access, DMA API calls like dma_sync_single_for_device, etc.
>> Tegra I2C ISR has calls to tegra_i2c_empty_rx_fifo in PIO mode
>> and in DMA/PIO mode writes different I2C registers including
>> I2C interrupt status. ISR cannot start processing
>> before the preparation step at tegra_i2c_xfer_msg is complete.
>> Hence, a synchronization between ISR and tegra_i2c_xfer_msg
>> is in place today using spinlock.
>>
>> Spinlock busy waits and can add avoidable delays.
>>
>> In this patch needed synchronization is achieved by disabling
>> I2C interrupt during preparation step and enabling interrupt
>> once preparation is over and spinlock is no longer needed.
> Sorry but I still don't understand the problem you are trying to solve.
> Yes spinlocks are busy waits but is this busy wait an actual problem? If
> so what is the problem with this?

There is no real problem here, the intent is to clean up the code more and that should
be mentioned in the commit message for clarity. This was actually my suggestion that
Bitan is now started to implement, which is very nice! It should be a good result in
the end.

> It appears that the spinlock was added to prevent error interrupts
> occurring until the transfer has started. If this is for error cases,
> then probably it is not often that the CPU is stuck busy waiting on the
> spinlock.
> 
> Furthermore, in addition to the spinlock we also have calls to
> tegra_i2c_unmask_irq/tegra_i2c_mask_irq. Therefore, if we are going to
> change this it would seem like a good idea to consolidate the
> masking/unmasking of IRQs and the spinlock, if possible.
> 
> Finally, I still see that we have a spinlock in the downstream kernels
> we are shipping and so I would prefer to see such a change also be
> tested in the downstream kernels we are releasing.